| Message ID | 20240320-mv88e6xxx-truncate-busid-v1-1-cface50b2efb@solid-run.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: mv88e6xxx: add warning for truncated mdio bus id | expand |
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> > >
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> >> >>
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.
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> >>> >>>
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.
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> >>>> >>>>
> 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
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.
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
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, "
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;
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(-) --- base-commit: e8f897f4afef0031fe618a8e94127a0934896aba change-id: 20240320-mv88e6xxx-truncate-busid-34a1d2769bbf Sincerely,