* [PATCH] net: dsa: mv88e6xxx: add warning for truncated mdio bus id @ 2024-03-20 13:48 Josua Mayer 2024-03-20 14:09 ` Jiri Pirko 0 siblings, 1 reply; 11+ messages in thread From: Josua Mayer @ 2024-03-20 13:48 UTC (permalink / raw) To: Andrew Lunn, Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: netdev, linux-kernel, Josua Mayer mv88e6xxx supports multiple mdio buses as children, e.g. to model both internal and external phys. If the child buses mdio ids are truncated, they might collide which each other leading to an obscure error from kobject_add. The maximum length of bus id is currently defined as 61 (MII_BUS_ID_SIZE). Truncation can occur on platforms with long node names and multiple levels before the parent bus on whiich the dsa switch sits such as on CN9130 [1]. Test whether the return value of snprintf exceeds the maximum bus id length and print a warning. [1] [ 8.324631] mv88e6085 f212a200.mdio-mii:04: switch 0x1760 detected: Marvell 88E6176, revision 1 [ 8.389516] mv88e6085 f212a200.mdio-mii:04: Truncated bus-id may collide. [ 8.592367] mv88e6085 f212a200.mdio-mii:04: Truncated bus-id may collide. [ 8.623593] sysfs: cannot create duplicate filename '/devices/platform/cp0/cp0:config-space@f2000000/f212a200.mdio/mdio_bus/f212a200.mdio-mii/f212a200.mdio-mii:04/mdio_bus/!cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi' [ 8.785480] kobject: kobject_add_internal failed for !cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi with -EEXIST, don't try to register things with the same name in the same directory. [ 8.936514] libphy: mii_bus /cp0/config-space@f2000000/mdio@12a200/ethernet-switch@4/mdi failed to register [ 8.946300] mdio_bus !cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi: __mdiobus_register: -22 [ 8.956003] mv88e6085 f212a200.mdio-mii:04: Cannot register MDIO bus (-22) [ 8.965329] mv88e6085: probe of f212a200.mdio-mii:04 failed with error -22 Signed-off-by: Josua Mayer <josua@solid-run.com> --- drivers/net/dsa/mv88e6xxx/chip.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 614cabb5c1b0..1c40f7631ab1 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -3731,10 +3731,12 @@ static int mv88e6xxx_mdio_register(struct mv88e6xxx_chip *chip, if (np) { bus->name = np->full_name; - snprintf(bus->id, MII_BUS_ID_SIZE, "%pOF", np); + if (snprintf(bus->id, MII_BUS_ID_SIZE, "%pOF", np) >= MII_BUS_ID_SIZE) + dev_warn(chip->dev, "Truncated bus-id may collide.\n"); } else { bus->name = "mv88e6xxx SMI"; - snprintf(bus->id, MII_BUS_ID_SIZE, "mv88e6xxx-%d", index++); + if (snprintf(bus->id, MII_BUS_ID_SIZE, "mv88e6xxx-%d", index++) >= MII_BUS_ID_SIZE) + dev_warn(chip->dev, "Truncated bus-id may collide.\n"); } bus->read = mv88e6xxx_mdio_read; --- base-commit: e8f897f4afef0031fe618a8e94127a0934896aba change-id: 20240320-mv88e6xxx-truncate-busid-34a1d2769bbf Sincerely, -- Josua Mayer <josua@solid-run.com> ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] net: dsa: mv88e6xxx: add warning for truncated mdio bus id 2024-03-20 13:48 [PATCH] net: dsa: mv88e6xxx: add warning for truncated mdio bus id Josua Mayer @ 2024-03-20 14:09 ` Jiri Pirko 2024-03-20 14:33 ` Josua Mayer 0 siblings, 1 reply; 11+ messages in thread From: Jiri Pirko @ 2024-03-20 14:09 UTC (permalink / raw) To: Josua Mayer Cc: Andrew Lunn, Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel Wed, Mar 20, 2024 at 02:48:55PM CET, josua@solid-run.com wrote: >mv88e6xxx supports multiple mdio buses as children, e.g. to model both >internal and external phys. If the child buses mdio ids are truncated, >they might collide which each other leading to an obscure error from >kobject_add. > >The maximum length of bus id is currently defined as 61 >(MII_BUS_ID_SIZE). Truncation can occur on platforms with long node >names and multiple levels before the parent bus on whiich the dsa switch s/whiich/which/ >sits such as on CN9130 [1]. > >Test whether the return value of snprintf exceeds the maximum bus id >length and print a warning. > >[1] >[ 8.324631] mv88e6085 f212a200.mdio-mii:04: switch 0x1760 detected: Marvell 88E6176, revision 1 >[ 8.389516] mv88e6085 f212a200.mdio-mii:04: Truncated bus-id may collide. >[ 8.592367] mv88e6085 f212a200.mdio-mii:04: Truncated bus-id may collide. >[ 8.623593] sysfs: cannot create duplicate filename '/devices/platform/cp0/cp0:config-space@f2000000/f212a200.mdio/mdio_bus/f212a200.mdio-mii/f212a200.mdio-mii:04/mdio_bus/!cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi' >[ 8.785480] kobject: kobject_add_internal failed for !cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi with -EEXIST, don't try to register things with the same name in the same directory. >[ 8.936514] libphy: mii_bus /cp0/config-space@f2000000/mdio@12a200/ethernet-switch@4/mdi failed to register >[ 8.946300] mdio_bus !cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi: __mdiobus_register: -22 >[ 8.956003] mv88e6085 f212a200.mdio-mii:04: Cannot register MDIO bus (-22) >[ 8.965329] mv88e6085: probe of f212a200.mdio-mii:04 failed with error -22 > >Signed-off-by: Josua Mayer <josua@solid-run.com> This is not bug fix, assume you target net-next. Please: 1) Next time, indicate that in the patch subject like this: [patch net-next] xxx 2) net-next is currently closed, repost next week. >--- > drivers/net/dsa/mv88e6xxx/chip.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > >diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c >index 614cabb5c1b0..1c40f7631ab1 100644 >--- a/drivers/net/dsa/mv88e6xxx/chip.c >+++ b/drivers/net/dsa/mv88e6xxx/chip.c >@@ -3731,10 +3731,12 @@ static int mv88e6xxx_mdio_register(struct mv88e6xxx_chip *chip, > > if (np) { > bus->name = np->full_name; >- snprintf(bus->id, MII_BUS_ID_SIZE, "%pOF", np); >+ if (snprintf(bus->id, MII_BUS_ID_SIZE, "%pOF", np) >= MII_BUS_ID_SIZE) >+ dev_warn(chip->dev, "Truncated bus-id may collide.\n"); How about instead of warn&fail fallback to some different name in this case? > } else { > bus->name = "mv88e6xxx SMI"; >- snprintf(bus->id, MII_BUS_ID_SIZE, "mv88e6xxx-%d", index++); >+ if (snprintf(bus->id, MII_BUS_ID_SIZE, "mv88e6xxx-%d", index++) >= MII_BUS_ID_SIZE) How exactly this may happen? >+ dev_warn(chip->dev, "Truncated bus-id may collide.\n"); > } > > bus->read = mv88e6xxx_mdio_read; > >--- >base-commit: e8f897f4afef0031fe618a8e94127a0934896aba >change-id: 20240320-mv88e6xxx-truncate-busid-34a1d2769bbf > >Sincerely, >-- >Josua Mayer <josua@solid-run.com> > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net: dsa: mv88e6xxx: add warning for truncated mdio bus id 2024-03-20 14:09 ` Jiri Pirko @ 2024-03-20 14:33 ` Josua Mayer 2024-03-20 15:13 ` Florian Fainelli ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Josua Mayer @ 2024-03-20 14:33 UTC (permalink / raw) To: Jiri Pirko Cc: Andrew Lunn, Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel Am 20.03.24 um 15:09 schrieb Jiri Pirko: > Wed, Mar 20, 2024 at 02:48:55PM CET, josua@solid-run.com wrote: >> mv88e6xxx supports multiple mdio buses as children, e.g. to model both >> internal and external phys. If the child buses mdio ids are truncated, >> they might collide which each other leading to an obscure error from >> kobject_add. >> >> The maximum length of bus id is currently defined as 61 >> (MII_BUS_ID_SIZE). Truncation can occur on platforms with long node >> names and multiple levels before the parent bus on whiich the dsa switch > s/whiich/which/ > > >> sits such as on CN9130 [1]. >> >> Test whether the return value of snprintf exceeds the maximum bus id >> length and print a warning. >> >> [1] >> [ 8.324631] mv88e6085 f212a200.mdio-mii:04: switch 0x1760 detected: Marvell 88E6176, revision 1 >> [ 8.389516] mv88e6085 f212a200.mdio-mii:04: Truncated bus-id may collide. >> [ 8.592367] mv88e6085 f212a200.mdio-mii:04: Truncated bus-id may collide. >> [ 8.623593] sysfs: cannot create duplicate filename '/devices/platform/cp0/cp0:config-space@f2000000/f212a200.mdio/mdio_bus/f212a200.mdio-mii/f212a200.mdio-mii:04/mdio_bus/!cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi' >> [ 8.785480] kobject: kobject_add_internal failed for !cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi with -EEXIST, don't try to register things with the same name in the same directory. >> [ 8.936514] libphy: mii_bus /cp0/config-space@f2000000/mdio@12a200/ethernet-switch@4/mdi failed to register >> [ 8.946300] mdio_bus !cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi: __mdiobus_register: -22 >> [ 8.956003] mv88e6085 f212a200.mdio-mii:04: Cannot register MDIO bus (-22) >> [ 8.965329] mv88e6085: probe of f212a200.mdio-mii:04 failed with error -22 >> >> Signed-off-by: Josua Mayer <josua@solid-run.com> > This is not bug fix, assume you target net-next. Please: > 1) Next time, indicate that in the patch subject like this: > [patch net-next] xxx > 2) net-next is currently closed, repost next week. Correct, thanks - will do. Just for future reference for those occasional contributors - is there such a thing as an lkml calendar? > >> --- >> drivers/net/dsa/mv88e6xxx/chip.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c >> index 614cabb5c1b0..1c40f7631ab1 100644 >> --- a/drivers/net/dsa/mv88e6xxx/chip.c >> +++ b/drivers/net/dsa/mv88e6xxx/chip.c >> @@ -3731,10 +3731,12 @@ static int mv88e6xxx_mdio_register(struct mv88e6xxx_chip *chip, >> >> if (np) { >> bus->name = np->full_name; >> - snprintf(bus->id, MII_BUS_ID_SIZE, "%pOF", np); >> + if (snprintf(bus->id, MII_BUS_ID_SIZE, "%pOF", np) >= MII_BUS_ID_SIZE) >> + dev_warn(chip->dev, "Truncated bus-id may collide.\n"); > How about instead of warn&fail fallback to some different name in this > case? Duplicate could be avoided by truncating from the start, however I don't know if that is a good idea. It affects naming of paths in sysfs, and the root cause is difficult to spot. >> } else { >> bus->name = "mv88e6xxx SMI"; >> - snprintf(bus->id, MII_BUS_ID_SIZE, "mv88e6xxx-%d", index++); >> + if (snprintf(bus->id, MII_BUS_ID_SIZE, "mv88e6xxx-%d", index++) >= MII_BUS_ID_SIZE) > How exactly this may happen? It can happen on switch nodes at deep levels in the device-tree, while describing both internal and external mdio buses of a switch. E.g. Documentation/devicetree/bindings/net/dsa/marvell,mv88e6xxx.yaml On CN9130 platform device-tree looks like this: / { cp0 { config-space@f2000000 { mdio@12a200 { ethernet-switch@4 { mdio { ... }; mdio-external { ... }; }; }; }; }; }; For mdio-external child all the names alone, without separators, make up 66 characters, exceeding: MII_BUS_ID_SIZE: cp0config-space@f2000000mdio@12a200ethernet-switch@4mdio-external With separators ('!') we have: cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdio cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdio-external Truncated to MII_BUS_ID_SIZE: cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi They become duplicates. >> + dev_warn(chip->dev, "Truncated bus-id may collide.\n"); Another option (imo) is to force the issue and return error code. Then the only way out would be increase of MII_BUS_ID_SIZE. >> } >> >> bus->read = mv88e6xxx_mdio_read; >> >> --- >> base-commit: e8f897f4afef0031fe618a8e94127a0934896aba >> change-id: 20240320-mv88e6xxx-truncate-busid-34a1d2769bbf >> >> Sincerely, >> -- >> Josua Mayer <josua@solid-run.com> >> >> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net: dsa: mv88e6xxx: add warning for truncated mdio bus id 2024-03-20 14:33 ` Josua Mayer @ 2024-03-20 15:13 ` Florian Fainelli 2024-03-20 16:28 ` Josua Mayer 2024-03-20 16:03 ` Jiri Pirko 2024-03-20 18:57 ` Andrew Lunn 2 siblings, 1 reply; 11+ messages in thread From: Florian Fainelli @ 2024-03-20 15:13 UTC (permalink / raw) To: Josua Mayer, Jiri Pirko Cc: Andrew Lunn, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel On 3/20/2024 7:33 AM, Josua Mayer wrote: > Am 20.03.24 um 15:09 schrieb Jiri Pirko: >> Wed, Mar 20, 2024 at 02:48:55PM CET, josua@solid-run.com wrote: >>> mv88e6xxx supports multiple mdio buses as children, e.g. to model both >>> internal and external phys. If the child buses mdio ids are truncated, >>> they might collide which each other leading to an obscure error from >>> kobject_add. >>> >>> The maximum length of bus id is currently defined as 61 >>> (MII_BUS_ID_SIZE). Truncation can occur on platforms with long node >>> names and multiple levels before the parent bus on whiich the dsa switch >> s/whiich/which/ >> >> >>> sits such as on CN9130 [1]. >>> >>> Test whether the return value of snprintf exceeds the maximum bus id >>> length and print a warning. >>> >>> [1] >>> [ 8.324631] mv88e6085 f212a200.mdio-mii:04: switch 0x1760 detected: Marvell 88E6176, revision 1 >>> [ 8.389516] mv88e6085 f212a200.mdio-mii:04: Truncated bus-id may collide. >>> [ 8.592367] mv88e6085 f212a200.mdio-mii:04: Truncated bus-id may collide. >>> [ 8.623593] sysfs: cannot create duplicate filename '/devices/platform/cp0/cp0:config-space@f2000000/f212a200.mdio/mdio_bus/f212a200.mdio-mii/f212a200.mdio-mii:04/mdio_bus/!cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi' >>> [ 8.785480] kobject: kobject_add_internal failed for !cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi with -EEXIST, don't try to register things with the same name in the same directory. >>> [ 8.936514] libphy: mii_bus /cp0/config-space@f2000000/mdio@12a200/ethernet-switch@4/mdi failed to register >>> [ 8.946300] mdio_bus !cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi: __mdiobus_register: -22 >>> [ 8.956003] mv88e6085 f212a200.mdio-mii:04: Cannot register MDIO bus (-22) >>> [ 8.965329] mv88e6085: probe of f212a200.mdio-mii:04 failed with error -22 >>> >>> Signed-off-by: Josua Mayer <josua@solid-run.com> >> This is not bug fix, assume you target net-next. Please: >> 1) Next time, indicate that in the patch subject like this: >> [patch net-next] xxx >> 2) net-next is currently closed, repost next week. > Correct, thanks - will do. > Just for future reference for those occasional contributors - > is there such a thing as an lkml calendar? There is this: https://patchwork.hopto.org/net-next.html >> >>> --- >>> drivers/net/dsa/mv88e6xxx/chip.c | 6 ++++-- >>> 1 file changed, 4 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c >>> index 614cabb5c1b0..1c40f7631ab1 100644 >>> --- a/drivers/net/dsa/mv88e6xxx/chip.c >>> +++ b/drivers/net/dsa/mv88e6xxx/chip.c >>> @@ -3731,10 +3731,12 @@ static int mv88e6xxx_mdio_register(struct mv88e6xxx_chip *chip, >>> >>> if (np) { >>> bus->name = np->full_name; >>> - snprintf(bus->id, MII_BUS_ID_SIZE, "%pOF", np); >>> + if (snprintf(bus->id, MII_BUS_ID_SIZE, "%pOF", np) >= MII_BUS_ID_SIZE) >>> + dev_warn(chip->dev, "Truncated bus-id may collide.\n"); >> How about instead of warn&fail fallback to some different name in this >> case? > Duplicate could be avoided by truncating from the start, > however I don't know if that is a good idea. > It affects naming of paths in sysfs, and the root cause is > difficult to spot. >>> } else { >>> bus->name = "mv88e6xxx SMI"; >>> - snprintf(bus->id, MII_BUS_ID_SIZE, "mv88e6xxx-%d", index++); >>> + if (snprintf(bus->id, MII_BUS_ID_SIZE, "mv88e6xxx-%d", index++) >= MII_BUS_ID_SIZE) >> How exactly this may happen? > It can happen on switch nodes at deep levels in the device-tree, > while describing both internal and external mdio buses of a switch. > E.g. Documentation/devicetree/bindings/net/dsa/marvell,mv88e6xxx.yaml We should consider moving these types of checks into the MDIO bus core, or at least introduce a helper function such that it embeds the check in it. -- Florian ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net: dsa: mv88e6xxx: add warning for truncated mdio bus id 2024-03-20 15:13 ` Florian Fainelli @ 2024-03-20 16:28 ` Josua Mayer 0 siblings, 0 replies; 11+ messages in thread From: Josua Mayer @ 2024-03-20 16:28 UTC (permalink / raw) To: Florian Fainelli, Jiri Pirko Cc: Andrew Lunn, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel Am 20.03.24 um 16:13 schrieb Florian Fainelli: > On 3/20/2024 7:33 AM, Josua Mayer wrote: >> Just for future reference for those occasional contributors - >> is there such a thing as an lkml calendar? > > There is this: https://patchwork.hopto.org/net-next.html Thank you :) > We should consider moving these types of checks into the MDIO bus core, or at least introduce a helper function such that it embeds the check in it. I will look into this. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net: dsa: mv88e6xxx: add warning for truncated mdio bus id 2024-03-20 14:33 ` Josua Mayer 2024-03-20 15:13 ` Florian Fainelli @ 2024-03-20 16:03 ` Jiri Pirko 2024-03-20 16:41 ` Josua Mayer 2024-03-20 18:57 ` Andrew Lunn 2 siblings, 1 reply; 11+ messages in thread From: Jiri Pirko @ 2024-03-20 16:03 UTC (permalink / raw) To: Josua Mayer Cc: Andrew Lunn, Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel Wed, Mar 20, 2024 at 03:33:24PM CET, josua@solid-run.com wrote: >Am 20.03.24 um 15:09 schrieb Jiri Pirko: >> Wed, Mar 20, 2024 at 02:48:55PM CET, josua@solid-run.com wrote: >>> mv88e6xxx supports multiple mdio buses as children, e.g. to model both >>> internal and external phys. If the child buses mdio ids are truncated, >>> they might collide which each other leading to an obscure error from >>> kobject_add. >>> >>> The maximum length of bus id is currently defined as 61 >>> (MII_BUS_ID_SIZE). Truncation can occur on platforms with long node >>> names and multiple levels before the parent bus on whiich the dsa switch >> s/whiich/which/ >> >> >>> sits such as on CN9130 [1]. >>> >>> Test whether the return value of snprintf exceeds the maximum bus id >>> length and print a warning. >>> >>> [1] >>> [ 8.324631] mv88e6085 f212a200.mdio-mii:04: switch 0x1760 detected: Marvell 88E6176, revision 1 >>> [ 8.389516] mv88e6085 f212a200.mdio-mii:04: Truncated bus-id may collide. >>> [ 8.592367] mv88e6085 f212a200.mdio-mii:04: Truncated bus-id may collide. >>> [ 8.623593] sysfs: cannot create duplicate filename '/devices/platform/cp0/cp0:config-space@f2000000/f212a200.mdio/mdio_bus/f212a200.mdio-mii/f212a200.mdio-mii:04/mdio_bus/!cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi' >>> [ 8.785480] kobject: kobject_add_internal failed for !cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi with -EEXIST, don't try to register things with the same name in the same directory. >>> [ 8.936514] libphy: mii_bus /cp0/config-space@f2000000/mdio@12a200/ethernet-switch@4/mdi failed to register >>> [ 8.946300] mdio_bus !cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi: __mdiobus_register: -22 >>> [ 8.956003] mv88e6085 f212a200.mdio-mii:04: Cannot register MDIO bus (-22) >>> [ 8.965329] mv88e6085: probe of f212a200.mdio-mii:04 failed with error -22 >>> >>> Signed-off-by: Josua Mayer <josua@solid-run.com> >> This is not bug fix, assume you target net-next. Please: >> 1) Next time, indicate that in the patch subject like this: >> [patch net-next] xxx >> 2) net-next is currently closed, repost next week. >Correct, thanks - will do. >Just for future reference for those occasional contributors - >is there such a thing as an lkml calendar? >> >>> --- >>> drivers/net/dsa/mv88e6xxx/chip.c | 6 ++++-- >>> 1 file changed, 4 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c >>> index 614cabb5c1b0..1c40f7631ab1 100644 >>> --- a/drivers/net/dsa/mv88e6xxx/chip.c >>> +++ b/drivers/net/dsa/mv88e6xxx/chip.c >>> @@ -3731,10 +3731,12 @@ static int mv88e6xxx_mdio_register(struct mv88e6xxx_chip *chip, >>> >>> if (np) { >>> bus->name = np->full_name; >>> - snprintf(bus->id, MII_BUS_ID_SIZE, "%pOF", np); >>> + if (snprintf(bus->id, MII_BUS_ID_SIZE, "%pOF", np) >= MII_BUS_ID_SIZE) >>> + dev_warn(chip->dev, "Truncated bus-id may collide.\n"); >> How about instead of warn&fail fallback to some different name in this >> case? >Duplicate could be avoided by truncating from the start, >however I don't know if that is a good idea. >It affects naming of paths in sysfs, and the root cause is >difficult to spot. >>> } else { >>> bus->name = "mv88e6xxx SMI"; >>> - snprintf(bus->id, MII_BUS_ID_SIZE, "mv88e6xxx-%d", index++); >>> + if (snprintf(bus->id, MII_BUS_ID_SIZE, "mv88e6xxx-%d", index++) >= MII_BUS_ID_SIZE) >> How exactly this may happen? >It can happen on switch nodes at deep levels in the device-tree, Read again, my question is about the else branch. >while describing both internal and external mdio buses of a switch. >E.g. Documentation/devicetree/bindings/net/dsa/marvell,mv88e6xxx.yaml > >On CN9130 platform device-tree looks like this: > >/ { > cp0 { > config-space@f2000000 { > mdio@12a200 { > ethernet-switch@4 { > mdio { ... }; > mdio-external { ... }; > }; > }; > }; > }; >}; > >For mdio-external child all the names alone, without separators, >make up 66 characters, exceeding: MII_BUS_ID_SIZE: >cp0config-space@f2000000mdio@12a200ethernet-switch@4mdio-external > >With separators ('!') we have: >cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdio >cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdio-external >Truncated to MII_BUS_ID_SIZE: >cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi >cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi >They become duplicates. > >>> + dev_warn(chip->dev, "Truncated bus-id may collide.\n"); >Another option (imo) is to force the issue and return error code. >Then the only way out would be increase of MII_BUS_ID_SIZE. >>> } >>> >>> bus->read = mv88e6xxx_mdio_read; >>> >>> --- >>> base-commit: e8f897f4afef0031fe618a8e94127a0934896aba >>> change-id: 20240320-mv88e6xxx-truncate-busid-34a1d2769bbf >>> >>> Sincerely, >>> -- >>> Josua Mayer <josua@solid-run.com> >>> >>> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net: dsa: mv88e6xxx: add warning for truncated mdio bus id 2024-03-20 16:03 ` Jiri Pirko @ 2024-03-20 16:41 ` Josua Mayer 0 siblings, 0 replies; 11+ messages in thread From: Josua Mayer @ 2024-03-20 16:41 UTC (permalink / raw) To: Jiri Pirko Cc: Andrew Lunn, Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel Am 20.03.24 um 17:03 schrieb Jiri Pirko: > Wed, Mar 20, 2024 at 03:33:24PM CET, josua@solid-run.com wrote: >> Am 20.03.24 um 15:09 schrieb Jiri Pirko: >>> Wed, Mar 20, 2024 at 02:48:55PM CET, josua@solid-run.com wrote: >>>> mv88e6xxx supports multiple mdio buses as children, e.g. to model both >>>> internal and external phys. If the child buses mdio ids are truncated, >>>> they might collide which each other leading to an obscure error from >>>> kobject_add. >>>> >>>> The maximum length of bus id is currently defined as 61 >>>> (MII_BUS_ID_SIZE). Truncation can occur on platforms with long node >>>> names and multiple levels before the parent bus on whiich the dsa switch >>> s/whiich/which/ >>> >>> >>>> sits such as on CN9130 [1]. >>>> >>>> Test whether the return value of snprintf exceeds the maximum bus id >>>> length and print a warning. >>>> >>>> [1] >>>> [ 8.324631] mv88e6085 f212a200.mdio-mii:04: switch 0x1760 detected: Marvell 88E6176, revision 1 >>>> [ 8.389516] mv88e6085 f212a200.mdio-mii:04: Truncated bus-id may collide. >>>> [ 8.592367] mv88e6085 f212a200.mdio-mii:04: Truncated bus-id may collide. >>>> [ 8.623593] sysfs: cannot create duplicate filename '/devices/platform/cp0/cp0:config-space@f2000000/f212a200.mdio/mdio_bus/f212a200.mdio-mii/f212a200.mdio-mii:04/mdio_bus/!cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi' >>>> [ 8.785480] kobject: kobject_add_internal failed for !cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi with -EEXIST, don't try to register things with the same name in the same directory. >>>> [ 8.936514] libphy: mii_bus /cp0/config-space@f2000000/mdio@12a200/ethernet-switch@4/mdi failed to register >>>> [ 8.946300] mdio_bus !cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi: __mdiobus_register: -22 >>>> [ 8.956003] mv88e6085 f212a200.mdio-mii:04: Cannot register MDIO bus (-22) >>>> [ 8.965329] mv88e6085: probe of f212a200.mdio-mii:04 failed with error -22 >>>> >>>> Signed-off-by: Josua Mayer <josua@solid-run.com> >>> This is not bug fix, assume you target net-next. Please: >>> 1) Next time, indicate that in the patch subject like this: >>> [patch net-next] xxx >>> 2) net-next is currently closed, repost next week. >> Correct, thanks - will do. >> Just for future reference for those occasional contributors - >> is there such a thing as an lkml calendar? >>>> --- >>>> drivers/net/dsa/mv88e6xxx/chip.c | 6 ++++-- >>>> 1 file changed, 4 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c >>>> index 614cabb5c1b0..1c40f7631ab1 100644 >>>> --- a/drivers/net/dsa/mv88e6xxx/chip.c >>>> +++ b/drivers/net/dsa/mv88e6xxx/chip.c >>>> @@ -3731,10 +3731,12 @@ static int mv88e6xxx_mdio_register(struct mv88e6xxx_chip *chip, >>>> >>>> if (np) { >>>> bus->name = np->full_name; >>>> - snprintf(bus->id, MII_BUS_ID_SIZE, "%pOF", np); >>>> + if (snprintf(bus->id, MII_BUS_ID_SIZE, "%pOF", np) >= MII_BUS_ID_SIZE) >>>> + dev_warn(chip->dev, "Truncated bus-id may collide.\n"); >>> How about instead of warn&fail fallback to some different name in this >>> case? >> Duplicate could be avoided by truncating from the start, >> however I don't know if that is a good idea. >> It affects naming of paths in sysfs, and the root cause is >> difficult to spot. >>>> } else { >>>> bus->name = "mv88e6xxx SMI"; >>>> - snprintf(bus->id, MII_BUS_ID_SIZE, "mv88e6xxx-%d", index++); >>>> + if (snprintf(bus->id, MII_BUS_ID_SIZE, "mv88e6xxx-%d", index++) >= MII_BUS_ID_SIZE) >>> How exactly this may happen? >> It can happen on switch nodes at deep levels in the device-tree, > Read again, my question is about the else branch. Oh! The else case occurs when the switch node does not have a child node named "mdio": /* Always register one mdio bus for the internal/default mdio * bus. This maybe represented in the device tree, but is * optional. */ child = of_get_child_by_name(np, "mdio"); err = mv88e6xxx_mdio_register(chip, child, false); of_node_put(child); In this case child is passed to np as NULL. For example if we have no "mdio" child node, but do have "mdio-external", the else case creates an mdio bus named "mv88e6xxx-%d". Then we would end up - in my example - with these two: cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mv88e6xxx-0 cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdio-external Truncated to MII_BUS_ID_SIZE: cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mv8 cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi They are not duplicates, but too close for comfort. Different device may exceed maximum size again. >> while describing both internal and external mdio buses of a switch. >> E.g. Documentation/devicetree/bindings/net/dsa/marvell,mv88e6xxx.yaml >> >> On CN9130 platform device-tree looks like this: >> >> / { >> cp0 { >> config-space@f2000000 { >> mdio@12a200 { >> ethernet-switch@4 { >> mdio { ... }; >> mdio-external { ... }; >> }; >> }; >> }; >> }; >> }; >> >> For mdio-external child all the names alone, without separators, >> make up 66 characters, exceeding: MII_BUS_ID_SIZE: >> cp0config-space@f2000000mdio@12a200ethernet-switch@4mdio-external >> >> With separators ('!') we have: >> cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdio >> cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdio-external >> Truncated to MII_BUS_ID_SIZE: >> cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi >> cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi >> They become duplicates. >> >>>> + dev_warn(chip->dev, "Truncated bus-id may collide.\n"); >> Another option (imo) is to force the issue and return error code. >> Then the only way out would be increase of MII_BUS_ID_SIZE. >>>> } >>>> >>>> bus->read = mv88e6xxx_mdio_read; >>>> >>>> --- >>>> base-commit: e8f897f4afef0031fe618a8e94127a0934896aba >>>> change-id: 20240320-mv88e6xxx-truncate-busid-34a1d2769bbf >>>> >>>> Sincerely, >>>> -- >>>> Josua Mayer <josua@solid-run.com> >>>> >>>> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net: dsa: mv88e6xxx: add warning for truncated mdio bus id 2024-03-20 14:33 ` Josua Mayer 2024-03-20 15:13 ` Florian Fainelli 2024-03-20 16:03 ` Jiri Pirko @ 2024-03-20 18:57 ` Andrew Lunn 2024-03-21 10:26 ` Josua Mayer 2 siblings, 1 reply; 11+ messages in thread From: Andrew Lunn @ 2024-03-20 18:57 UTC (permalink / raw) To: Josua Mayer Cc: Jiri Pirko, Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel > With separators ('!') we have: > cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdio > cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdio-external > Truncated to MII_BUS_ID_SIZE: > cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi > cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi This has been made worse by the DT maintainers wanting ethernet-switch@4, not switch@4. And i guess config-space was also something shorter in the past. I think your idea of cropping from the beginning, not the end, is in general a good solution. However, is there any danger of cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdio-external and cp1!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdio-external I assume the two instances of cp have the same peripherals, at the same address? Another option would be if the name needs to be truncated, use the fallback as if there was no np: bus->name = "mv88e6xxx SMI"; snprintf(bus->id, MII_BUS_ID_SIZE, "mv88e6xxx-%d", index++); That at least gives you unique names. Andrew ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net: dsa: mv88e6xxx: add warning for truncated mdio bus id 2024-03-20 18:57 ` Andrew Lunn @ 2024-03-21 10:26 ` Josua Mayer 2024-03-21 15:10 ` Andrew Lunn 0 siblings, 1 reply; 11+ messages in thread From: Josua Mayer @ 2024-03-21 10:26 UTC (permalink / raw) To: Andrew Lunn Cc: Jiri Pirko, Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel Am 20.03.24 um 19:57 schrieb Andrew Lunn: >> With separators ('!') we have: >> cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdio >> cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdio-external >> Truncated to MII_BUS_ID_SIZE: >> cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi >> cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi > This has been made worse by the DT maintainers wanting > ethernet-switch@4, not switch@4. And i guess config-space was also > something shorter in the past. > > I think your idea of cropping from the beginning, not the end, is in > general a good solution. However, is there any danger of > > cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdio-external > > and > > cp1!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdio-external Since these will appear as links in /sys/bus/mdio_bus/devices, this danger exists. If the prefix is too similar, we can run into duplicates also when cropping from the front. So we could crop at the front and reduce likelihood of this situation, but (imo) should still print a warning since it is not working as intended. > > I assume the two instances of cp have the same peripherals, at the > same address? In this particular platform the config-space layer uses the actual base address for each cp: cp0!config-space@f2000000 cp1!config-space@f4000000 cp2!config-space@f6000000 > > Another option would be if the name needs to be truncated, use the > fallback as if there was no np: > > bus->name = "mv88e6xxx SMI"; > snprintf(bus->id, MII_BUS_ID_SIZE, "mv88e6xxx-%d", index++); > > That at least gives you unique names. This ensures a unique suffix within a branch of device-tree. It could still collide with same structure on a cp1 or cp2. The platform where this is triggered does not (currently) require declaring external mdio bus (because hardware bug renders that bus useless). For now my priority is helping future developers running into this. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net: dsa: mv88e6xxx: add warning for truncated mdio bus id 2024-03-21 10:26 ` Josua Mayer @ 2024-03-21 15:10 ` Andrew Lunn 2024-03-21 15:54 ` Josua Mayer 0 siblings, 1 reply; 11+ messages in thread From: Andrew Lunn @ 2024-03-21 15:10 UTC (permalink / raw) To: Josua Mayer Cc: Jiri Pirko, Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel On Thu, Mar 21, 2024 at 10:26:54AM +0000, Josua Mayer wrote: > Am 20.03.24 um 19:57 schrieb Andrew Lunn: > >> With separators ('!') we have: > >> cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdio > >> cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdio-external > >> Truncated to MII_BUS_ID_SIZE: > >> cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi > >> cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi > > This has been made worse by the DT maintainers wanting > > ethernet-switch@4, not switch@4. And i guess config-space was also > > something shorter in the past. > > > > I think your idea of cropping from the beginning, not the end, is in > > general a good solution. However, is there any danger of > > > > cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdio-external > > > > and > > > > cp1!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdio-external > Since these will appear as links in /sys/bus/mdio_bus/devices, > this danger exists. > If the prefix is too similar, we can run into duplicates also when > cropping from the front. > > So we could crop at the front and reduce likelihood of this situation, > but (imo) should still print a warning since it is not working as intended. The problem with a warning is, what do you tell people who ask how to fix the warning? Hack your device tree to short the node names? A warning is best done when there is something which can be done to fix the problem. If it is not fixable, it is just noise. > > Another option would be if the name needs to be truncated, use the > > fallback as if there was no np: > > > > bus->name = "mv88e6xxx SMI"; > > snprintf(bus->id, MII_BUS_ID_SIZE, "mv88e6xxx-%d", index++); > > > > That at least gives you unique names. > This ensures a unique suffix within a branch of device-tree. > It could still collide with same structure on a cp1 or cp2. static int mv88e6xxx_mdio_register(struct mv88e6xxx_chip *chip, struct device_node *np, bool external) { static int index; struct mv88e6xxx_mdio_bus *mdio_bus; struct mii_bus *bus; int err; index is static, so it is simply a counter. So you should get the names mv88e6xxx-0, mv88e6xxx-1, mv88e6xxx-2, mv88e6xxx-3... Andrew ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net: dsa: mv88e6xxx: add warning for truncated mdio bus id 2024-03-21 15:10 ` Andrew Lunn @ 2024-03-21 15:54 ` Josua Mayer 0 siblings, 0 replies; 11+ messages in thread From: Josua Mayer @ 2024-03-21 15:54 UTC (permalink / raw) To: Andrew Lunn Cc: Jiri Pirko, Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel Am 21.03.24 um 16:10 schrieb Andrew Lunn: > On Thu, Mar 21, 2024 at 10:26:54AM +0000, Josua Mayer wrote: >> Am 20.03.24 um 19:57 schrieb Andrew Lunn: >>>> With separators ('!') we have: >>>> cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdio >>>> cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdio-external >>>> Truncated to MII_BUS_ID_SIZE: >>>> cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi >>>> cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdi >>> This has been made worse by the DT maintainers wanting >>> ethernet-switch@4, not switch@4. And i guess config-space was also >>> something shorter in the past. >>> >>> I think your idea of cropping from the beginning, not the end, is in >>> general a good solution. However, is there any danger of >>> >>> cp0!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdio-external >>> >>> and >>> >>> cp1!config-space@f2000000!mdio@12a200!ethernet-switch@4!mdio-external >> Since these will appear as links in /sys/bus/mdio_bus/devices, >> this danger exists. >> If the prefix is too similar, we can run into duplicates also when >> cropping from the front. >> >> So we could crop at the front and reduce likelihood of this situation, >> but (imo) should still print a warning since it is not working as intended. > The problem with a warning is, what do you tell people who ask how to > fix the warning? Hack your device tree to short the node names? > > A warning is best done when there is something which can be done to > fix the problem. If it is not fixable, it is just noise. Well, one option is making a future case for increasing MII_BUS_ID_SIZE. > >>> Another option would be if the name needs to be truncated, use the >>> fallback as if there was no np: >>> >>> bus->name = "mv88e6xxx SMI"; >>> snprintf(bus->id, MII_BUS_ID_SIZE, "mv88e6xxx-%d", index++); >>> >>> That at least gives you unique names. >> This ensures a unique suffix within a branch of device-tree. >> It could still collide with same structure on a cp1 or cp2. > static int mv88e6xxx_mdio_register(struct mv88e6xxx_chip *chip, > struct device_node *np, > bool external) > { > static int index; > struct mv88e6xxx_mdio_bus *mdio_bus; > struct mii_bus *bus; > int err; > > index is static, Good point! > so it is simply a counter. So you should get the > names mv88e6xxx-0, mv88e6xxx-1, mv88e6xxx-2, mv88e6xxx-3... This could work as a fall-back within mv88e6xxx driver. Alternatively - how about having a generic helper somewhere (not sure where) which can add a marker and use a global counter? E.g. appending "!...-%d" Across the tree I found 39 instances of "snprintf(bus->id, " ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2024-03-21 15:55 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-03-20 13:48 [PATCH] net: dsa: mv88e6xxx: add warning for truncated mdio bus id Josua Mayer 2024-03-20 14:09 ` Jiri Pirko 2024-03-20 14:33 ` Josua Mayer 2024-03-20 15:13 ` Florian Fainelli 2024-03-20 16:28 ` Josua Mayer 2024-03-20 16:03 ` Jiri Pirko 2024-03-20 16:41 ` Josua Mayer 2024-03-20 18:57 ` Andrew Lunn 2024-03-21 10:26 ` Josua Mayer 2024-03-21 15:10 ` Andrew Lunn 2024-03-21 15:54 ` Josua Mayer
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.