| Message ID | 20240505-mv88e6xxx-truncate-busid-v3-1-e70d6ec2f3db@solid-run.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v3] net: dsa: mv88e6xxx: control mdio bus-id truncation for long paths | expand |
Le 5 mai 2024 02:52:45 GMT-07:00, Josua Mayer <josua@solid-run.com> a écrit : >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 with 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 which the dsa switch >itself sits, e.g. CN9130 [1]. > >Compare the return value of snprintf against maximum bus-id length to >detect truncation. In that case write an incrementing marker to the end >to avoid name collisions. >This changes the problematic bus-ids mdio and mdio-external from [1] >to [2]. > >Truncation at the beginning was considered as a workaround, however that >is still subject to name collisions in sysfs where only the first >characters differ. > >[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 > >[2] >/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...!-0 >/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...!-1 > >Signed-off-by: Josua Mayer <josua@solid-run.com> >--- The idea and implementation is reasonable but this could affect other drivers than mv88e6xxx, why not move that logic to mdiobus_register() and tracking the truncation index globally within the MDIO bus layer? If we prefer a driver based solution, the mii_bus object could carry a truncation format, at the risk of creating more variation between drivers in case of truncation. We could also wait until we have another driver requiring a similar solution before promoting this to a wider range. Florian
Am 05.05.24 um 21:11 schrieb Florian Fainelli: > Le 5 mai 2024 02:52:45 GMT-07:00, Josua Mayer <josua@solid-run.com> a écrit : >> 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 with 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 which the dsa switch >> itself sits, e.g. CN9130 [1]. >> >> Compare the return value of snprintf against maximum bus-id length to >> detect truncation. In that case write an incrementing marker to the end >> to avoid name collisions. >> This changes the problematic bus-ids mdio and mdio-external from [1] >> to [2]. >> >> Truncation at the beginning was considered as a workaround, however that >> is still subject to name collisions in sysfs where only the first >> characters differ. >> >> [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 >> >> [2] >> /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...!-0 >> /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...!-1 >> >> Signed-off-by: Josua Mayer <josua@solid-run.com> >> --- > The idea and implementation is reasonable but this could affect other drivers than mv88e6xxx, why not move that logic to mdiobus_register() and tracking the truncation index globally within the MDIO bus layer? Conceptually I agree, it would be nice to have a centralized solution to this problem, it probably can occur in multiple places. My reasoning is that solving the problem within a single driver is a much smaller task, especially for sporadic contributors who lack a deep understanding for how all layers interact. Perhaps agreeing on a good solution within this driver can inform a more general solution to be added later. > If we prefer a driver based solution, the mii_bus object could carry a truncation format, at the risk of creating more variation between drivers in case of truncation. We could also wait until we have another driver requiring a similar solution before promoting this to a wider range. br Josua Mayer
On Tue, 7 May 2024 12:03:31 +0000 Josua Mayer wrote: > > The idea and implementation is reasonable but this could affect other drivers than mv88e6xxx, why not move that logic to mdiobus_register() and tracking the truncation index globally within the MDIO bus layer? > Conceptually I agree, it would be nice to have a centralized > solution to this problem, it probably can occur in multiple places. > > My reasoning is that solving the problem within a single driver > is a much smaller task, especially for sporadic contributors > who lack a deep understanding for how all layers interact. > > Perhaps agreeing on a good solution within this driver > can inform a more general solution to be added later. I agree with Florian, FWIW. The choice of how to truncate is a bit arbitrary, if core does it at least it will be consistent.
Am 13.05.24 um 17:32 schrieb Jakub Kicinski: > On Tue, 7 May 2024 12:03:31 +0000 Josua Mayer wrote: >>> The idea and implementation is reasonable but this could affect other drivers than mv88e6xxx, why not move that logic to mdiobus_register() and tracking the truncation index globally within the MDIO bus layer? >> Conceptually I agree, it would be nice to have a centralized >> solution to this problem, it probably can occur in multiple places. >> >> My reasoning is that solving the problem within a single driver >> is a much smaller task, especially for sporadic contributors >> who lack a deep understanding for how all layers interact. >> >> Perhaps agreeing on a good solution within this driver >> can inform a more general solution to be added later. > I agree with Florian, FWIW. The choice of how to truncate is a bit > arbitrary, if core does it at least it will be consistent. Very true. However the names themselves are so far generated by each driver, if mdiobus_register should define truncation behaviour, then the name format must be passed as an argument, too. How about adding new properties to mii_bus? But then how to pass the variable arguments .... Is it acceptable to have variadic function?: int __mdiobus_register(struct mii_bus *bus, struct module *owner, const char *name_format, ...); Alternatively the core could have a helper function: mdiobus_allocate_name() But if it is stateful that will lead to issues when probe fails afterwards. Any ideas or opinions?
On Mon, Nov 18, 2024 at 02:57:45PM +0000, Josua Mayer wrote: > Am 13.05.24 um 17:32 schrieb Jakub Kicinski: > > On Tue, 7 May 2024 12:03:31 +0000 Josua Mayer wrote: > >>> The idea and implementation is reasonable but this could affect other drivers than mv88e6xxx, why not move that logic to mdiobus_register() and tracking the truncation index globally within the MDIO bus layer? > >> Conceptually I agree, it would be nice to have a centralized > >> solution to this problem, it probably can occur in multiple places. > >> > >> My reasoning is that solving the problem within a single driver > >> is a much smaller task, especially for sporadic contributors > >> who lack a deep understanding for how all layers interact. > >> > >> Perhaps agreeing on a good solution within this driver > >> can inform a more general solution to be added later. > > I agree with Florian, FWIW. The choice of how to truncate is a bit > > arbitrary, if core does it at least it will be consistent. > > Very true. > > However the names themselves are so far generated by each driver, > if mdiobus_register should define truncation behaviour, > then the name format must be passed as an argument, too. > > How about adding new properties to mii_bus? > But then how to pass the variable arguments .... > > Is it acceptable to have variadic function?: > > int __mdiobus_register(struct mii_bus *bus, struct module *owner, const char *name_format, ...); Probably not. The format string needs to be fully under control and constant because otherwise it can be abused. If you look around, you will see patches converting invocations of variadic functions from (foo), to ("%s", foo), because if foo can be under user control, it might contain "%s%s%d%x%lld" causing it to access stuff never passed to it. Andrew
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 964c7b847fd3..9c996be00362 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -3774,10 +3774,10 @@ static int mv88e6xxx_mdio_register(struct mv88e6xxx_chip *chip, struct device_node *np, bool external) { - static int index; + static int index, trunc; struct mv88e6xxx_mdio_bus *mdio_bus; struct mii_bus *bus; - int err; + int err, len; if (external) { mv88e6xxx_reg_lock(chip); @@ -3803,10 +3803,28 @@ 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); + len = snprintf(bus->id, MII_BUS_ID_SIZE, "%pOF", np); } else { bus->name = "mv88e6xxx SMI"; - snprintf(bus->id, MII_BUS_ID_SIZE, "mv88e6xxx-%d", index++); + len = snprintf(bus->id, MII_BUS_ID_SIZE, "mv88e6xxx-%d", index++); + } + if (len < 0) { + return len; + } else if (len >= MII_BUS_ID_SIZE) { + /* If generated bus id is truncated, names in sysfs + * may collide. + * Generate a special numeric suffix. If it fits + * within MII_BUS_ID_SIZE, insert at the end to mark + * truncation and avoid name collisions. + */ + len = snprintf(NULL, 0, "...!-%d", trunc); + if (len < 0) + return err; + else if (len >= MII_BUS_ID_SIZE) + return -ENOBUFS; + + snprintf(bus->id + MII_BUS_ID_SIZE - err - 1, + MII_BUS_ID_SIZE - err, "...!-%d", trunc++); } 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 with 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 which the dsa switch itself sits, e.g. CN9130 [1]. Compare the return value of snprintf against maximum bus-id length to detect truncation. In that case write an incrementing marker to the end to avoid name collisions. This changes the problematic bus-ids mdio and mdio-external from [1] to [2]. Truncation at the beginning was considered as a workaround, however that is still subject to name collisions in sysfs where only the first characters differ. [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 [2] /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...!-0 /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...!-1 Signed-off-by: Josua Mayer <josua@solid-run.com> --- Bcc: Mor Nagli <mor.nagli@solid-run.com> --- Changes in v3: - Added better named variable "len" for snprintf return value to improve readability (Reported-by: Andrew Lunn <andrew@lunn.ch>) - Added short explanation why there are two calls of snprintf in comment - Link to v2: https://lore.kernel.org/r/20240404-mv88e6xxx-truncate-busid-v2-1-69683f67008b@solid-run.com Changes in v2: - fixed typo in commit message (Reportby: Jiri Pirko <jiri@resnulli.us>) - replaced warning message with controlled truncation - Link to v1: https://lore.kernel.org/r/20240320-mv88e6xxx-truncate-busid-v1-1-cface50b2efb@solid-run.com --- drivers/net/dsa/mv88e6xxx/chip.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) --- base-commit: 173e7622ccb3f46834bd4176ed363f435e142942 change-id: 20240320-mv88e6xxx-truncate-busid-34a1d2769bbf Sincerely,