Re: [PATCHv3 2/4] pci: allow all bus devices to use the same slot

[<dan.j.williams@intel.com>]

Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> A pcie hot plug slot applies to the entire subordinate bus. Thus, pciehp
> only allocates a single hotplug slot for the bridge to that bus. The pci
> slot, though, would only match to functions on device 0, meaning any
> other device beyond that are not matched to any slot even though they
> all share the same slot.
> 
> Allow a slot to be created to claim all devices on a bus, not just a
> matching device. This is done by introducing a sential value, named

s/sential/sentinel/

> PCI_SLOT_ALL_DEVICES, which then has the pci slot match to any device on
> the bus. This fixes walking the slot's device list for ports that don't
> have device specific slots.
> 
> Since 0xff already has special meaning, the chose sentinal value for

s/sentinal/sentinel/

> this new feature is 0xfe. This will not clash with any actual slot
> number since they are limited to 5 bits.

Can you say a bit more about why other hotplug controller drivers can
continue to get away with a single device number as a hotplug slot
target?  For example, would you expect that the ACPI hotplug driver is
broken if an ARI device is added?

It looks like the goal of this sentinel is to turn all
pci_try_reset_slot() into pci_try_reset_bus() only for the native PCIe
hotplug controller, or did I miss a detail?

> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  drivers/pci/hotplug/pciehp_core.c |  3 ++-
>  drivers/pci/slot.c                | 22 ++++++++++++++++++----
>  include/linux/pci.h               |  8 +++++++-
>  3 files changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
> index f59baa9129709..d80346d567049 100644
> --- a/drivers/pci/hotplug/pciehp_core.c
> +++ b/drivers/pci/hotplug/pciehp_core.c
> @@ -79,7 +79,8 @@ static int init_slot(struct controller *ctrl)
>  	snprintf(name, SLOT_NAME_SIZE, "%u", PSN(ctrl));
>  
>  	retval = pci_hp_initialize(&ctrl->hotplug_slot,
> -				   ctrl->pcie->port->subordinate, 0, name);
> +				   ctrl->pcie->port->subordinate,
> +				   PCI_SLOT_ALL_DEVICES, name);
>  	if (retval) {
>  		ctrl_err(ctrl, "pci_hp_initialize failed: error %d\n", retval);
>  		kfree(ops);
> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
> index 50fb3eb595fe6..647f6d2cceef6 100644
> --- a/drivers/pci/slot.c
> +++ b/drivers/pci/slot.c
> @@ -41,6 +41,10 @@ static ssize_t address_read_file(struct pci_slot *slot, char *buf)
>  		return sysfs_emit(buf, "%04x:%02x\n",
>  				  pci_domain_nr(slot->bus),
>  				  slot->bus->number);
> +	if (slot->number == PCI_SLOT_ALL_DEVICES)
> +		return sysfs_emit(buf, "%04x:%02x:00\n",
> +				  pci_domain_nr(slot->bus),
> +				  slot->bus->number);

Why not treat the PCI_SLOT_ALL_DEVICES case the same as the 0xff case?

>  
>  	return sysfs_emit(buf, "%04x:%02x:%02x\n",
>  			  pci_domain_nr(slot->bus),
> @@ -73,7 +77,8 @@ static void pci_slot_release(struct kobject *kobj)
>  
>  	down_read(&pci_bus_sem);
>  	list_for_each_entry(dev, &slot->bus->devices, bus_list)
> -		if (PCI_SLOT(dev->devfn) == slot->number)
> +		if (slot->number == PCI_SLOT_ALL_DEVICES ||
> +		    PCI_SLOT(dev->devfn) == slot->number)

Maybe add a pci_slot_matches() helper for this pattern?