On Wed, Nov 06, 2024 at 04:22:11PM +0200, Ilpo Järvinen wrote:
> On Fri, 25 Oct 2024, Michał Winiarski wrote:
>
> > There are multiple places where conversions between IOV resources and
> > standard resources are done.
> >
> > Extract the logic to pci_resource_to_iov() and pci_resource_from_iov()
> > helpers.
> >
> > Suggested-by: Ilpo Järvinen <[email protected]>
> > Signed-off-by: Michał Winiarski <[email protected]>
> > ---
> > drivers/pci/iov.c | 20 ++++++++++----------
> > drivers/pci/pci.h | 18 ++++++++++++++++++
> > drivers/pci/setup-bus.c | 2 +-
> > 3 files changed, 29 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> > index 6bdc9950b9787..eedc1df56c49e 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_from_iov(resno)];
> > }
> >
> > static void pci_read_vf_config_common(struct pci_dev *virtfn)
> > @@ -322,12 +322,12 @@ 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_to_iov(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_to_iov(i));
> > virtfn->resource[i].start = res->start + size * id;
> > virtfn->resource[i].end = virtfn->resource[i].start + size - 1;
> > rc = request_resource(res, &virtfn->resource[i]);
> > @@ -624,8 +624,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_to_iov(i));
> > + res = &dev->resource[pci_resource_to_iov(i)];
> > if (res->parent)
> > nres++;
> > }
> > @@ -786,8 +786,8 @@ 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_to_iov(i)];
> > + res_name = pci_resource_name(dev, pci_resource_to_iov(i));
> >
> > /*
> > * If it is already FIXED, don't change it, something
> > @@ -844,7 +844,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_to_iov(i)];
> > res->flags = 0;
> > }
> >
> > @@ -906,7 +906,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_to_iov(i));
> >
> > pci_write_config_dword(dev, iov->pos + PCI_SRIOV_SYS_PGSIZE, iov->pgsz);
> > pci_iov_set_numvfs(dev, iov->num_VFs);
> > @@ -972,7 +972,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 = dev->resource + resno;
> > - int vf_bar = resno - PCI_IOV_RESOURCES;
> > + int vf_bar = pci_resource_from_iov(resno);
> > struct pci_bus_region region;
> > u16 cmd;
> > u32 new;
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index 48d345607e57e..1f8d88f0243b7 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -584,6 +584,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_to_iov(int resno)
> > +{
> > + return resno + PCI_IOV_RESOURCES;
> > +}
> > +
> > +static inline int pci_resource_from_iov(int resno)
> > +{
> > + return resno - PCI_IOV_RESOURCES;
> > +}
>
> to/from feels wrong way around for me. What is named as "PCI resource from
> IOV" converts from PCI resource indexing to IOV compatible indexing, and
> vice versa.
It converts "from" IOV BAR (resource 7+) to VF BAR (resource 0+).
I don't think swapping the to/from would make it clearer.
I had it as pci_iov_resource_to_vf_bar() / pci_vf_bar_to_iov_resource()
before, but went with a shorter naming instead.
Would that be clearer?
pci_iov_resource_to_vf_bar() / pci_iov_resource_from_vf_bar()?
Or perhaps you have a better suggestion?
>
> > extern const struct attribute_group sriov_pf_dev_attr_group;
> > extern const struct attribute_group sriov_vf_dev_attr_group;
> > #else
> > @@ -608,6 +617,15 @@ static inline bool pci_resource_is_iov(int resno)
> > {
> > return false;
> > }
> > +static inline int pci_resource_to_iov(int resno)
> > +{
> > + return -ENODEV;
> > +}
> > +
> > +static inline int pci_resource_from_iov(int resno)
> > +{
> > + return -ENODEV;
> > +}
>
> These seem dangerous as the errors are not checked by the callers. Perhaps
> put something like BUG_ON(1) there instead as it really is something that
> should never be called for real if CONFIG_PCI_IOV is not enabled, they are
> just to make compiler happy without #ifdefs in C code.
I'll add a WARN_ON_ONCE(1) to make it more visible.
-Michał
>
> --
> i.
>
> > #endif /* CONFIG_PCI_IOV */
> >
> > #ifdef CONFIG_PCIE_PTM
> > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> > index ba293df10c050..c5ad7c4ad6eb1 100644
> > --- a/drivers/pci/setup-bus.c
> > +++ b/drivers/pci/setup-bus.c
> > @@ -1778,7 +1778,7 @@ 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_to_iov(i)];
> > struct pci_bus_region region;
> >
> > /* Not assigned or rejected by kernel? */
> >
