Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <54426ccd-8c1d-c4e5-6fec-07c77c0f3d59@linux.ibm.com>
Date:   Thu, 30 Jun 2022 14:53:37 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.0
Subject: Re: [PATCH v6 5/5] s390/pci: allow zPCI zbus without a function zero
Content-Language: en-US
To:     Niklas Schnelle <schnelle@linux.ibm.com>,
        Bjorn Helgaas <bhelgaas@google.com>
Cc:     Jan Kiszka <jan.kiszka@siemens.com>,
        Matthew Rosato <mjrosato@linux.ibm.com>,
        linux-s390@vger.kernel.org, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20220628143100.3228092-1-schnelle@linux.ibm.com>
 <20220628143100.3228092-6-schnelle@linux.ibm.com>
From:   Pierre Morel <pmorel@linux.ibm.com>
In-Reply-To: <20220628143100.3228092-6-schnelle@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 6/28/22 16:31, Niklas Schnelle wrote:
> Currently the zPCI code block PCI bus creation and probing of a zPCI
> zbus unless there is a PCI function with devfn 0. This is always the
> case for the PCI functions with hidden RID but may keep PCI functions
> from a multi-function PCI device with RID information invisible until
> the function 0 becomes visible. Worse as a PCI bus is necessary to even
> present a PCI hotplug slot even that remains invisible.
> 
> With the probing of these so called isolated PCI functions enabled for
> s390 in common code this restriction is no longer necessary. On network
> cards with multiple ports and a PF per port this also allows using each
> port on its own while still providing the physical PCI topology
> information in the devfn needed to associate VFs with their parent PF.
> 
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> ---
>   arch/s390/pci/pci_bus.c | 82 ++++++++++-------------------------------
>   1 file changed, 20 insertions(+), 62 deletions(-)
> 
> diff --git a/arch/s390/pci/pci_bus.c b/arch/s390/pci/pci_bus.c
> index 5d77acbd1c87..6a8da1b742ae 100644
> --- a/arch/s390/pci/pci_bus.c
> +++ b/arch/s390/pci/pci_bus.c
> @@ -145,9 +145,6 @@ int zpci_bus_scan_bus(struct zpci_bus *zbus)
>   	struct zpci_dev *zdev;
>   	int devfn, rc, ret = 0;
>   
> -	if (!zbus->function[0])
> -		return 0;
> -
>   	for (devfn = 0; devfn < ZPCI_FUNCTIONS_PER_BUS; devfn++) {
>   		zdev = zbus->function[devfn];
>   		if (zdev && zdev->state == ZPCI_FN_STATE_CONFIGURED) {
> @@ -184,26 +181,26 @@ void zpci_bus_scan_busses(void)
>   
>   /* zpci_bus_create_pci_bus - Create the PCI bus associated with this zbus
>    * @zbus: the zbus holding the zdevices
> - * @f0: function 0 of the bus
> + * @fr: PCI root function that will determine the bus's domain, and bus speeed
>    * @ops: the pci operations
>    *
> - * Function zero is taken as a parameter as this is used to determine the
> - * domain, multifunction property and maximum bus speed of the entire bus.
> + * The PCI function @fr determines the domain (its UID), multifunction property
> + * and maximum bus speed of the entire bus.
>    *
>    * Return: 0 on success, an error code otherwise
>    */
> -static int zpci_bus_create_pci_bus(struct zpci_bus *zbus, struct zpci_dev *f0, struct pci_ops *ops)
> +static int zpci_bus_create_pci_bus(struct zpci_bus *zbus, struct zpci_dev *fr, struct pci_ops *ops)
>   {
>   	struct pci_bus *bus;
>   	int domain;
>   
> -	domain = zpci_alloc_domain((u16)f0->uid);
> +	domain = zpci_alloc_domain((u16)fr->uid);
>   	if (domain < 0)
>   		return domain;
>   
>   	zbus->domain_nr = domain;
> -	zbus->multifunction = f0->rid_available;
> -	zbus->max_bus_speed = f0->max_bus_speed;
> +	zbus->multifunction = fr->rid_available;
> +	zbus->max_bus_speed = fr->max_bus_speed;
>   
>   	/*
>   	 * Note that the zbus->resources are taken over and zbus->resources
> @@ -303,47 +300,6 @@ void pcibios_bus_add_device(struct pci_dev *pdev)
>   	}
>   }
>   
> -/* zpci_bus_create_hotplug_slots - Add hotplug slot(s) for device added to bus
> - * @zdev: the zPCI device that was newly added
> - *
> - * Add the hotplug slot(s) for the newly added PCI function. Normally this is
> - * simply the slot for the function itself. If however we are adding the
> - * function 0 on a zbus, it might be that we already registered functions on
> - * that zbus but could not create their hotplug slots yet so add those now too.
> - *
> - * Return: 0 on success, an error code otherwise
> - */
> -static int zpci_bus_create_hotplug_slots(struct zpci_dev *zdev)
> -{
> -	struct zpci_bus *zbus = zdev->zbus;
> -	int devfn, rc = 0;
> -
> -	rc = zpci_init_slot(zdev);
> -	if (rc)
> -		return rc;
> -	zdev->has_hp_slot = 1;
> -
> -	if (zdev->devfn == 0 && zbus->multifunction) {
> -		/* Now that function 0 is there we can finally create the
> -		 * hotplug slots for those functions with devfn != 0 that have
> -		 * been parked in zbus->function[] waiting for us to be able to
> -		 * create the PCI bus.
> -		 */
> -		for  (devfn = 1; devfn < ZPCI_FUNCTIONS_PER_BUS; devfn++) {
> -			zdev = zbus->function[devfn];
> -			if (zdev && !zdev->has_hp_slot) {
> -				rc = zpci_init_slot(zdev);
> -				if (rc)
> -					return rc;
> -				zdev->has_hp_slot = 1;
> -			}
> -		}
> -
> -	}
> -
> -	return rc;
> -}
> -
>   static int zpci_bus_add_device(struct zpci_bus *zbus, struct zpci_dev *zdev)
>   {
>   	int rc = -EINVAL;
> @@ -352,21 +308,19 @@ static int zpci_bus_add_device(struct zpci_bus *zbus, struct zpci_dev *zdev)
>   		pr_err("devfn %04x is already assigned\n", zdev->devfn);
>   		return rc;
>   	}
> +

Unnecessary CR

>   	zdev->zbus = zbus;
>   	zbus->function[zdev->devfn] = zdev;
>   	zpci_nb_devices++;
>   
> -	if (zbus->bus) {
> -		if (zbus->multifunction && !zdev->rid_available) {
> -			WARN_ONCE(1, "rid_available not set for multifunction\n");
> -			goto error;
> -		}
> -
> -		zpci_bus_create_hotplug_slots(zdev);
> -	} else {
> -		/* Hotplug slot will be created once function 0 appears */
> -		zbus->multifunction = 1;
> +	if (zbus->multifunction && !zdev->rid_available) {
> +		WARN_ONCE(1, "rid_available not set for multifunction\n");
> +		goto error;
>   	}
> +	rc = zpci_init_slot(zdev);
> +	if (rc)
> +		goto error;
> +	zdev->has_hp_slot = 1;
>   
>   	return 0;
>   
> @@ -400,7 +354,11 @@ int zpci_bus_device_register(struct zpci_dev *zdev, struct pci_ops *ops)
>   			return -ENOMEM;
>   	}
>   
> -	if (zdev->devfn == 0) {
> +	if (!zbus->bus) {
> +		/* The UID of the first PCI function registered with a zpci_bus
> +		 * is used as the domain number for that bus. Currently there
> +		 * is exactly one zpci_bus per domain.
> +		 */
>   		rc = zpci_bus_create_pci_bus(zbus, zdev, ops);
>   		if (rc)
>   			goto error;
> 


Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>


-- 
Pierre Morel
IBM Lab Boeblingen