[PATCH] net: dsa: mv88e6xxx: add warning for truncated mdio bus id

[PATCH] net: dsa: mv88e6xxx: add warning for truncated mdio bus id

[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>

Re: [PATCH] net: dsa: mv88e6xxx: add warning for truncated mdio bus id

[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.


>---
> 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>
>
>

Re: [PATCH] net: dsa: mv88e6xxx: add warning for truncated mdio bus id

[Josua Mayer]

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>
>>
>>

Re: [PATCH] net: dsa: mv88e6xxx: add warning for truncated mdio bus id

[Florian Fainelli]

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

Re: [PATCH] net: dsa: mv88e6xxx: add warning for truncated mdio bus id

[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.


>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>
>>>
>>>

Re: [PATCH] net: dsa: mv88e6xxx: add warning for truncated mdio bus id

[Josua Mayer]

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.

Re: [PATCH] net: dsa: mv88e6xxx: add warning for truncated mdio bus id

[Josua Mayer]

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>
>>>>
>>>>

Re: [PATCH] net: dsa: mv88e6xxx: add warning for truncated mdio bus id

[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

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

Re: [PATCH] net: dsa: mv88e6xxx: add warning for truncated mdio bus id

[Josua Mayer]

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.

Re: [PATCH] net: dsa: mv88e6xxx: add warning for truncated mdio bus id

[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.

> > 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

Re: [PATCH] net: dsa: mv88e6xxx: add warning for truncated mdio bus id

[Josua Mayer]

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, "