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 resourceN_resize sysfs attributes allow users to resize
> > Resizable BARs (ReBAR).  After a successful resize, the resource
> > attribute groups are removed and recreated so that the updated
> > BAR sizes are reflected in the sysfs files.
> 
> Out of curiosity, where does this removal/recreation happen?  I don't
> see it in pci_resize_resource() or
> pci_do_resource_release_and_resize().

The __resource_resize_store() helper carries the relevant code, per:

  sysfs_remove_groups(&pdev->dev.kobj, pci_dev_resource_attr_groups);

  ret = pci_resize_resource(pdev, n, size, 0);
  if (ret)
  	pci_warn(pdev, "Failed to resize BAR %d: %pe\n",
  		 n, ERR_PTR(ret));

  pci_assign_unassigned_bus_resources(bus);

  if (sysfs_create_groups(&pdev->dev.kobj, pci_dev_resource_attr_groups))
  	pci_warn(pdev, "Failed to recreate resource groups after BAR resizing\n");

  pci_write_config_word(pdev, PCI_COMMAND, cmd);

This takes place under the device_lock() with a given device woken up using
pci_config_pm_runtime_get().

See for reference:

  https://elixir.bootlin.com/linux/v7.0.1/source/drivers/pci/pci-sysfs.c#L1596

> > Resizable BARs are a PCI Express extended capability
> > (PCI_EXT_CAP_ID_REBAR), which requires PCIe extended config
> > space.
> 
> It sounds like the fact that ReBAR requires extended config space is
> important somehow (beyond just the fact that we can't discover ReBAR
> without it)?  Is there some connection between extended config space
> and mmap?

No, there is no connection between extended configuration space and mmap.

I was trying to establish two separate facts:

  - ReBAR requires PCI Express (for the extended configuration
    space support).
  - Every PCI Express-capable architecture defines either
    HAVE_PCI_MMAP or ARCH_GENERIC_PCI_MMAP_RESOURCE.

The commit message should have captured the reasoning better.
 
> > Every PCIe-capable architecture defines either HAVE_PCI_MMAP or
> > ARCH_GENERIC_PCI_MMAP_RESOURCE (via the relevant arch headers
> > or the generic asm-generic/pci.h fallback).  On platforms that
> > define neither, the resource files are not created and the sysfs
> > group remove and create calls in __resource_resize_store() are
> > no-ops.
> 
> What's the connection between ReBAR and mmap?

The connection is somewhat indirect, through the sysfs resource files.

The resourceN_resize attribute exists so userspace can resize a BAR and
then access it through the resourceN sysfs files.  Those resource files
only exist on platforms that define HAVE_PCI_MMAP or
ARCH_GENERIC_PCI_MMAP_RESOURCE.

On architectures without either of these defines, pci_dev_resource_attr_groups
array will be NULL and the sysfs_remove_groups() and sysfs_create_groups()
calls in __resource_resize_store() are no-ops.  And there will be no
resource files to tear down or recreate.

To add, there are some kernel drivers that need ReBAR call pci_resize_resource()
directly (e.g., amdgpu, xe, i915), not through the sysfs attribute.

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.

Hope this helps.

Thank you!

	Krzysztof