On Wed, Mar 26, 2025 at 04:46:19PM +0200, Ilpo Järvinen wrote:
> On Thu, 20 Mar 2025, Michał Winiarski wrote:
>
> > There are multiple places where conversions between IOV resources and
> > corresponding VF BAR numbers are done.
> >
> > Extract the logic to pci_resource_num_from_vf_bar() and
> > pci_resource_num_to_vf_bar() helpers.
> >
> > Suggested-by: Ilpo Järvinen <ilpo.jarvi...@linux.intel.com>
> > Signed-off-by: Michał Winiarski <michal.winiar...@intel.com>
> > Acked-by: Christian König <christian.koe...@amd.com>
> > ---
> > drivers/pci/iov.c | 22 ++++++++++++----------
> > drivers/pci/pci.h | 19 +++++++++++++++++++
> > drivers/pci/setup-bus.c | 3 ++-
> > 3 files changed, 33 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> > index bf95387993cd5..985ea11339c45 100644
> > --- a/drivers/pci/iov.c
> > +++ b/drivers/pci/iov.c
> > @@ -151,7 +151,7 @@ resource_size_t pci_iov_resource_size(struct pci_dev
> > *dev, int resno)
> > if (!dev->is_physfn)
> > return 0;
> >
> > - return dev->sriov->barsz[resno - PCI_IOV_RESOURCES];
> > + return dev->sriov->barsz[pci_resource_num_to_vf_bar(resno)];
> > }
> >
> > static void pci_read_vf_config_common(struct pci_dev *virtfn)
> > @@ -322,12 +322,13 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id)
> > virtfn->multifunction = 0;
> >
> > for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> > - res = &dev->resource[i + PCI_IOV_RESOURCES];
> > + res = &dev->resource[pci_resource_num_from_vf_bar(i)];
> > if (!res->parent)
> > continue;
> > virtfn->resource[i].name = pci_name(virtfn);
> > virtfn->resource[i].flags = res->flags;
> > - size = pci_iov_resource_size(dev, i + PCI_IOV_RESOURCES);
> > + size = pci_iov_resource_size(dev,
> > + pci_resource_num_from_vf_bar(i));
> > resource_set_range(&virtfn->resource[i],
> > res->start + size * id, size);
> > rc = request_resource(res, &virtfn->resource[i]);
> > @@ -624,8 +625,8 @@ static int sriov_enable(struct pci_dev *dev, int
> > nr_virtfn)
> >
> > nres = 0;
> > for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> > - bars |= (1 << (i + PCI_IOV_RESOURCES));
> > - res = &dev->resource[i + PCI_IOV_RESOURCES];
> > + bars |= (1 << pci_resource_num_from_vf_bar(i));
> > + res = &dev->resource[pci_resource_num_from_vf_bar(i)];
> > if (res->parent)
> > nres++;
> > }
> > @@ -791,8 +792,9 @@ static int sriov_init(struct pci_dev *dev, int pos)
> >
> > nres = 0;
> > for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> > - res = &dev->resource[i + PCI_IOV_RESOURCES];
> > - res_name = pci_resource_name(dev, i + PCI_IOV_RESOURCES);
> > + res = &dev->resource[pci_resource_num_from_vf_bar(i)];
> > + res_name = pci_resource_name(dev,
> > + pci_resource_num_from_vf_bar(i));
>
> All these get easier to read if you add (same comment for the cases
> above):
>
> int idx = pci_resource_num_from_vf_bar(i);
Ok.
>
> >
> > /*
> > * If it is already FIXED, don't change it, something
> > @@ -851,7 +853,7 @@ static int sriov_init(struct pci_dev *dev, int pos)
> > dev->is_physfn = 0;
> > failed:
> > for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> > - res = &dev->resource[i + PCI_IOV_RESOURCES];
> > + res = &dev->resource[pci_resource_num_from_vf_bar(i)];
> > res->flags = 0;
> > }
> >
> > @@ -913,7 +915,7 @@ static void sriov_restore_state(struct pci_dev *dev)
> > pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, ctrl);
> >
> > for (i = 0; i < PCI_SRIOV_NUM_BARS; i++)
> > - pci_update_resource(dev, i + PCI_IOV_RESOURCES);
> > + pci_update_resource(dev, pci_resource_num_from_vf_bar(i));
> >
> > pci_write_config_dword(dev, iov->pos + PCI_SRIOV_SYS_PGSIZE, iov->pgsz);
> > pci_iov_set_numvfs(dev, iov->num_VFs);
> > @@ -979,7 +981,7 @@ void pci_iov_update_resource(struct pci_dev *dev, int
> > resno)
> > {
> > struct pci_sriov *iov = dev->is_physfn ? dev->sriov : NULL;
> > struct resource *res = pci_resource_n(dev, resno);
> > - int vf_bar = resno - PCI_IOV_RESOURCES;
> > + int vf_bar = pci_resource_num_to_vf_bar(resno);
> > struct pci_bus_region region;
> > u16 cmd;
> > u32 new;
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index adc54bb2c8b34..f44840ee3c327 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -693,6 +693,15 @@ static inline bool pci_resource_is_iov(int resno)
> > {
> > return resno >= PCI_IOV_RESOURCES && resno <= PCI_IOV_RESOURCE_END;
> > }
> > +static inline int pci_resource_num_from_vf_bar(int resno)
> > +{
> > + return resno + PCI_IOV_RESOURCES;
> > +}
> > +
> > +static inline int pci_resource_num_to_vf_bar(int resno)
> > +{
> > + return resno - PCI_IOV_RESOURCES;
> > +}
> > extern const struct attribute_group sriov_pf_dev_attr_group;
> > extern const struct attribute_group sriov_vf_dev_attr_group;
> > #else
> > @@ -717,6 +726,16 @@ static inline bool pci_resource_is_iov(int resno)
> > {
> > return false;
> > }
> > +static inline int pci_resource_num_from_vf_bar(int resno)
> > +{
> > + WARN_ON_ONCE(1);
> > + return -ENODEV;
> > +}
> > +static inline int pci_resource_num_to_vf_bar(int resno)
> > +{
> > + WARN_ON_ONCE(1);
> > + return -ENODEV;
> > +}
> > #endif /* CONFIG_PCI_IOV */
> >
> > #ifdef CONFIG_PCIE_TPH
> > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> > index 54d6f4fa3ce16..55e91ba1e74a2 100644
> > --- a/drivers/pci/setup-bus.c
> > +++ b/drivers/pci/setup-bus.c
> > @@ -1885,7 +1885,8 @@ static int iov_resources_unassigned(struct pci_dev
> > *dev, void *data)
> > bool *unassigned = data;
> >
> > for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> > - struct resource *r = &dev->resource[i + PCI_IOV_RESOURCES];
> > + struct resource *r =
> > + &dev->resource[pci_resource_num_from_vf_bar(i)];
>
> I'd add int idx here as well.
Ok, makes sense, I'll use the similar approach in the following patches
as well.
Thanks,
-Michał
>
> > struct pci_bus_region region;
> >
> > /* Not assigned or rejected by kernel? */
> >
>
> --
> i.
