Re: [PATCH v6 24/24] PCI/sysfs: Limit BAR resize attribute scope to platforms with PCI mmap

["Krzysztof Wilczyński" <kwilczynski@kernel.org>]

Hello,

> > > The only platform without these aforementioned defines is Alpha, which is
> > > conventional PCI only and cannot have ReBAR.  So this guard removes dead
> > > sysfs code on platforms where it can never be executed.
> > 
> > Having a closer look:
> > 
> >   resource_resize_attr_is_visible()
> >     pci_rebar_get_current_size()      <- returns -ENOTSUPP, so is_visible callback returns 0
> >       pci_rebar_find_pos()
> >         pos = pdev->rebar_cap         <- set to 0 on a conventional PCI
> >         if (!pos)
> >           return -ENOTSUPP            <- no ReBAR support
> > 
> > The pdev->rebar_cap is set during PCI enumeration:
> > 
> >   pci_init_capabilities()
> >     pci_rebar_init()
> >       pdev->rebar_cap = pci_find_ext_capability()
> >         pci_find_next_ext_capability()
> >           if (dev->cfg_size <= PCI_CFG_SPACE_SIZE)
> >             return 0;                 <- dev->cfg_size set to 256 here for conventional PCI
> > 
> > The PCI_CFG_SPACE_SIZE is 256 here.
> > 
> > When the platform has support for PCI Express, the dev->cfg_size is then
> > set to 4096.  On an architecture that supports conventional PCI only (such
> > as Alpha), the pdev->rebar_cap will be set to 0, the is_visible callback
> > will then return 0, and the resize sysfs attribute is never created, as
> > such, the __resource_resize_store() callback will never be executed.
> > 
> > To make the connetion here to the #ifdef guards:
> > 
> > For the pci_rebar_get_current_size() to return >= 0, the device needs
> > PCI Express and extended configuration space support.  As of today,
> > every architecture with PCI Express support defines HAVE_PCI_MMAP or
> > ARCH_GENERIC_PCI_MMAP_RESOURCE.
> > 
> > I hope the reasoning here works.
> 
> We're talking about this #ifdef:
> 
>   +#if defined(HAVE_PCI_MMAP) || defined(ARCH_GENERIC_PCI_MMAP_RESOURCE)
>    static ssize_t __resource_resize_show(struct device *dev, int n, char *buf)
>    ...
>   +#endif
> 
> I follow the reasoning now but by next week I won't, so I think it
> requires too much background knowledge.  Future changes involving
> HAVE_PCI_MMAP or ARCH_GENERIC_PCI_MMAP_RESOURCE could easily break
> this.
> 
> IIUC it's basically doing what "#ifdef CONFIG_PCI_REBAR" or even
> "#ifdef CONFIG_PCI_EXPRESS" would do, if we had such a thing.

We don't sadly have such guards at the moment, so using the guards like
HAVE_PCI_MMAP or ARCH_GENERIC_PCI_MMAP_RESOURCE would be the next best
thing.  We already rely on these throughout the pci-sysfs.c already, and
will also rely on either for the static sysfs attributes, so if there was
some issues with either of these, if these "broke" somehow, then we would
have other more severe problems.  Note, that architectures rely on these
macros to let us know about what "feature" is expected to be enabled, so to
speak, so I would imagine nobody would break this accidentally (famous last
words) and perhaps with extreme caution would do it on purpose.

> How terrible would it be if we just accepted this dead code on Alpha?

I would prefer not to build any dead code anywhere if it can be helped,
especially with as little as two lines of code added.  Plus, most of the
code setting up resources that are not specific to Alpha, would have been
disabled using exactly the same guard as the one proposed to be added
here, not like this sets any precedent or adds something new.

That being said, I will drop this from the next version.

Thank you!

	Krzysztof