diff mbox series

[v2] r8169: Avoid duplicate sysfs entry creation error

Message ID 20210624154945.19177-1-andre.przywara@arm.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [v2] r8169: Avoid duplicate sysfs entry creation error | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Guessed tree name to be net-next
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cc_maintainers success CCed 5 of 5 maintainers
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 9 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link

Commit Message

Andre Przywara June 24, 2021, 3:49 p.m. UTC 
From: Sayanta Pattanayak <sayanta.pattanayak@arm.com>

When registering the MDIO bus for a r8169 device, we use the PCI B/D/F
specifier as a (seemingly) unique device identifier.
However the very same BDF number can be used on another PCI segment,
which makes the driver fail probing:

[ 27.544136] r8169 0002:07:00.0: enabling device (0000 -> 0003)
[ 27.559734] sysfs: cannot create duplicate filename '/class/mdio_bus/r8169-700'
....…
[ 27.684858] libphy: mii_bus r8169-700 failed to register
[ 27.695602] r8169: probe of 0002:07:00.0 failed with error -22

Add the segment number to the device name to make it more unique.

This fixes operation on an ARM N1SDP board, where two boards might be
connected together to form an SMP system, and all on-board devices show
up twice, just on different PCI segments.

Signed-off-by: Sayanta Pattanayak <sayanta.pattanayak@arm.com>
[Andre: expand commit message, use pci_domain_nr()]
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
Now compile-tested on ARM, arm64, ppc64, sparc64, mips64, hppa, x86-64,
i386.

Changes v1 ... v2:
- use pci_domain_nr() wrapper to fix compilation on various arches

 drivers/net/ethernet/realtek/r8169_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Heiner Kallweit June 24, 2021, 6:41 p.m. UTC | #1 
On 24.06.2021 17:49, Andre Przywara wrote:
> From: Sayanta Pattanayak <sayanta.pattanayak@arm.com>
> 
> When registering the MDIO bus for a r8169 device, we use the PCI B/D/F
> specifier as a (seemingly) unique device identifier.
> However the very same BDF number can be used on another PCI segment,
> which makes the driver fail probing:
> 
> [ 27.544136] r8169 0002:07:00.0: enabling device (0000 -> 0003)
> [ 27.559734] sysfs: cannot create duplicate filename '/class/mdio_bus/r8169-700'
> ....…
> [ 27.684858] libphy: mii_bus r8169-700 failed to register
> [ 27.695602] r8169: probe of 0002:07:00.0 failed with error -22
> 
> Add the segment number to the device name to make it more unique.
> 
> This fixes operation on an ARM N1SDP board, where two boards might be
> connected together to form an SMP system, and all on-board devices show
> up twice, just on different PCI segments.
> 
> Signed-off-by: Sayanta Pattanayak <sayanta.pattanayak@arm.com>
> [Andre: expand commit message, use pci_domain_nr()]
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
> Now compile-tested on ARM, arm64, ppc64, sparc64, mips64, hppa, x86-64,
> i386.
> 
Good. Patch is missing the net vs. net-next annotation, therefore the
remaining question is whether to treat this as a fix. Seems nobody but
you was affected so far, therefore handling it as an improvement should
be fine as well.

If you need this change on previous kernel versions:
Add net annotation and add a Fixes tag (I think when driver was switched
to phylib with 4.19). Else add a net-next annotation.

See the following link for details:
https://www.kernel.org/doc/Documentation/networking/netdev-FAQ.txt

> Changes v1 ... v2:
> - use pci_domain_nr() wrapper to fix compilation on various arches
> 
>  drivers/net/ethernet/realtek/r8169_main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 2c89cde7da1e..5f7f0db7c502 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -5086,7 +5086,8 @@ static int r8169_mdio_register(struct rtl8169_private *tp)
>  	new_bus->priv = tp;
>  	new_bus->parent = &pdev->dev;
>  	new_bus->irq[0] = PHY_MAC_INTERRUPT;
> -	snprintf(new_bus->id, MII_BUS_ID_SIZE, "r8169-%x", pci_dev_id(pdev));
> +	snprintf(new_bus->id, MII_BUS_ID_SIZE, "r8169-%x-%x",
> +		 pci_domain_nr(pdev->bus), pci_dev_id(pdev));
>  
>  	new_bus->read = r8169_mdio_read_reg;
>  	new_bus->write = r8169_mdio_write_reg;
>
Andre Przywara July 20, 2021, 4:04 p.m. UTC | #2 
On Thu, 24 Jun 2021 20:41:25 +0200
Heiner Kallweit <hkallweit1@gmail.com> wrote:

Hi Heiner,

> On 24.06.2021 17:49, Andre Przywara wrote:
> > From: Sayanta Pattanayak <sayanta.pattanayak@arm.com>
> > 
> > When registering the MDIO bus for a r8169 device, we use the PCI B/D/F
> > specifier as a (seemingly) unique device identifier.
> > However the very same BDF number can be used on another PCI segment,
> > which makes the driver fail probing:
> > 
> > [ 27.544136] r8169 0002:07:00.0: enabling device (0000 -> 0003)
> > [ 27.559734] sysfs: cannot create duplicate filename '/class/mdio_bus/r8169-700'
> > ....…
> > [ 27.684858] libphy: mii_bus r8169-700 failed to register
> > [ 27.695602] r8169: probe of 0002:07:00.0 failed with error -22
> > 
> > Add the segment number to the device name to make it more unique.
> > 
> > This fixes operation on an ARM N1SDP board, where two boards might be
> > connected together to form an SMP system, and all on-board devices show
> > up twice, just on different PCI segments.
> > 
> > Signed-off-by: Sayanta Pattanayak <sayanta.pattanayak@arm.com>
> > [Andre: expand commit message, use pci_domain_nr()]
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> > Now compile-tested on ARM, arm64, ppc64, sparc64, mips64, hppa, x86-64,
> > i386.
> >   
> Good. Patch is missing the net vs. net-next annotation, therefore the
> remaining question is whether to treat this as a fix. Seems nobody but
> you was affected so far, therefore handling it as an improvement should
> be fine as well.
> 
> If you need this change on previous kernel versions:
> Add net annotation and add a Fixes tag (I think when driver was switched
> to phylib with 4.19). Else add a net-next annotation.

Many thanks for the instructions! I decided to mark it as a fix, since
the problem is definitely there. I guess it just was not reported since
multiple RTL8169 devices are not often used in big systems with
multiple PCIe segments, and even if, there might be no collisions, by
sheer luck and virtue of a favourable bus enumeration.

Let me know if you disagree. At least I would like to see it in v5.10.y.

Sending v3 with the proper annotation in a minute.

Cheers,
Andre

> 
> See the following link for details:
> https://www.kernel.org/doc/Documentation/networking/netdev-FAQ.txt
> 
> > Changes v1 ... v2:
> > - use pci_domain_nr() wrapper to fix compilation on various arches
> > 
> >  drivers/net/ethernet/realtek/r8169_main.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> > index 2c89cde7da1e..5f7f0db7c502 100644
> > --- a/drivers/net/ethernet/realtek/r8169_main.c
> > +++ b/drivers/net/ethernet/realtek/r8169_main.c
> > @@ -5086,7 +5086,8 @@ static int r8169_mdio_register(struct rtl8169_private *tp)
> >  	new_bus->priv = tp;
> >  	new_bus->parent = &pdev->dev;
> >  	new_bus->irq[0] = PHY_MAC_INTERRUPT;
> > -	snprintf(new_bus->id, MII_BUS_ID_SIZE, "r8169-%x", pci_dev_id(pdev));
> > +	snprintf(new_bus->id, MII_BUS_ID_SIZE, "r8169-%x-%x",
> > +		 pci_domain_nr(pdev->bus), pci_dev_id(pdev));
> >  
> >  	new_bus->read = r8169_mdio_read_reg;
> >  	new_bus->write = r8169_mdio_write_reg;
> >   
>
diff mbox series

Patch

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 2c89cde7da1e..5f7f0db7c502 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -5086,7 +5086,8 @@  static int r8169_mdio_register(struct rtl8169_private *tp)
 	new_bus->priv = tp;
 	new_bus->parent = &pdev->dev;
 	new_bus->irq[0] = PHY_MAC_INTERRUPT;
-	snprintf(new_bus->id, MII_BUS_ID_SIZE, "r8169-%x", pci_dev_id(pdev));
+	snprintf(new_bus->id, MII_BUS_ID_SIZE, "r8169-%x-%x",
+		 pci_domain_nr(pdev->bus), pci_dev_id(pdev));
 
 	new_bus->read = r8169_mdio_read_reg;
 	new_bus->write = r8169_mdio_write_reg;