Re: [PATCH v7 4/7] PCI: pci_alloc_child_bus() return NULL if ->add_bus() returns -ENOLINK

From: "Pali Rohár" <pali@kernel.org>
To: Jim Quinlan <jim2101024@gmail.com>
Cc: linux-pci@vger.kernel.org,
	Nicolas Saenz Julienne <nsaenz@kernel.org>,
	Rob Herring <robh@kernel.org>, Mark Brown <broonie@kernel.org>,
	bcm-kernel-feedback-list@broadcom.com,
	james.quinlan@broadcom.com, Bjorn Helgaas <bhelgaas@google.com>,
	open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v7 4/7] PCI: pci_alloc_child_bus() return NULL if ->add_bus() returns -ENOLINK
Date: Thu, 4 Nov 2021 00:30:26 +0100	[thread overview]
Message-ID: <20211103233026.trzas3cwrb26mqb3@pali> (raw)
In-Reply-To: <20211103184939.45263-5-jim2101024@gmail.com>

On Wednesday 03 November 2021 14:49:34 Jim Quinlan wrote:
> Currently, if the call to the pci_ops add_bus() method returns an error, a
> WARNING and dev_err() occurs.  We keep this behavior for all errors except
> -ENOLINK; for -ENOLINK we want to skip the WARNING and immediately return
> NULL.  The argument for this case is that one does not want to continue
> enumerating if pcie-link has not been established.  The real reason is that
> without doing this the pcie-brcmstb.c driver panics when the dev/id is
> read, as this controller panics on such accesses rather than returning
> 0xffffffff.

I think that this is something which should be fixed in the driver, not
in the pci core code. Check in driver code that you can touch HW and if
not return fabricated value 0xffffffff.

> It appears that there are only a few uses of the pci_ops add_bus() method
> in the kernel and none of them currently return -ENOLINK so it should be
> safe to do this.
> 
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> ---
>  drivers/pci/probe.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index d9fc02a71baa..fdc3f42634b7 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1122,6 +1122,9 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
>  
>  	if (child->ops->add_bus) {
>  		ret = child->ops->add_bus(child);
> +		/* Don't return the child if w/o pcie link-up */
> +		if (ret == -ENOLINK)

In my opinion ENOLINK is not the correct errno code for signaling
"no link-up" error. IIRC ENOLINK was defined for file/inode links. For
network connections there is ENETDOWN errno code which is more similar
to "no link-up" than inode link.

Anyway, I still do not think if it is a good idea to have this check in
core pci code.

(This is just my opinion... wait for Bjorn with maintainer's hat what
will say that is the best way to handle above issue)

> +			return NULL;
>  		if (WARN_ON(ret < 0))
>  			dev_err(&child->dev, "failed to add bus: %d\n", ret);
>  	}
> -- 
> 2.17.1
> 