* [PATCH net-next v2] net: dsa: mv88e6xxx: control mdio bus-id truncation for long paths
@ 2024-04-04 14:06 Josua Mayer
2024-04-04 14:29 ` Andrew Lunn
0 siblings, 1 reply; 3+ messages in thread
From: Josua Mayer @ 2024-04-04 14:06 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, Mor Nagli
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 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 | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 9ed1821184ec..2b181b432bbc 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3705,7 +3705,7 @@ 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;
@@ -3734,10 +3734,26 @@ 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);
+ err = snprintf(bus->id, MII_BUS_ID_SIZE, "%pOF", np);
} else {
bus->name = "mv88e6xxx SMI";
- snprintf(bus->id, MII_BUS_ID_SIZE, "mv88e6xxx-%d", index++);
+ err = snprintf(bus->id, MII_BUS_ID_SIZE, "mv88e6xxx-%d", index++);
+ }
+ if (err < 0) {
+ return err;
+ } else if (err >= MII_BUS_ID_SIZE) {
+ /* If generated bus id is truncated, names in sysfs
+ * may collide. Insert a special numeric suffix to mark
+ * truncation and avoid name collisions.
+ */
+ err = snprintf(NULL, 0, "...!-%d", trunc);
+ if (err < 0)
+ return err;
+ else if (err >= 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;
---
base-commit: d72b735712e65cccb85f81e6b26a14ea53cc6438
change-id: 20240320-mv88e6xxx-truncate-busid-34a1d2769bbf
Sincerely,
--
Josua Mayer <josua@solid-run.com>
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH net-next v2] net: dsa: mv88e6xxx: control mdio bus-id truncation for long paths
2024-04-04 14:06 [PATCH net-next v2] net: dsa: mv88e6xxx: control mdio bus-id truncation for long paths Josua Mayer
@ 2024-04-04 14:29 ` Andrew Lunn
2024-04-15 8:01 ` Josua Mayer
0 siblings, 1 reply; 3+ messages in thread
From: Andrew Lunn @ 2024-04-04 14:29 UTC (permalink / raw
To: Josua Mayer
Cc: Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, netdev, linux-kernel, Mor Nagli
> + } else if (err >= MII_BUS_ID_SIZE) {
> + /* If generated bus id is truncated, names in sysfs
> + * may collide. Insert a special numeric suffix to mark
> + * truncation and avoid name collisions.
> + */
> + err = snprintf(NULL, 0, "...!-%d", trunc);
> + if (err < 0)
> + return err;
It took me a while to figure out what this was doing. Rather than err,
maybe add a new variable postfix_len, to give a clue that this is used
to determine how long the post fix is, and so how much needs to be
truncated from the end of the string to make room for it.
Andrew
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH net-next v2] net: dsa: mv88e6xxx: control mdio bus-id truncation for long paths
2024-04-04 14:29 ` Andrew Lunn
@ 2024-04-15 8:01 ` Josua Mayer
0 siblings, 0 replies; 3+ messages in thread
From: Josua Mayer @ 2024-04-15 8:01 UTC (permalink / raw
To: Andrew Lunn
Cc: Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, Mor Nagli
Am 04.04.24 um 16:29 schrieb Andrew Lunn:
>> + } else if (err >= MII_BUS_ID_SIZE) {
>> + /* If generated bus id is truncated, names in sysfs
>> + * may collide. Insert a special numeric suffix to mark
>> + * truncation and avoid name collisions.
>> + */
>> + err = snprintf(NULL, 0, "...!-%d", trunc);
>> + if (err < 0)
>> + return err;
> It took me a while to figure out what this was doing. Rather than err,
> maybe add a new variable postfix_len, to give a clue that this is used
> to determine how long the post fix is, and so how much needs to be
> truncated from the end of the string to make room for it.
I considered adding a new variable (e.g. busid_len) to capture all results
from snprintf calls. But it is confusing because in some calls it is
length of parts of bus-id only.
Maybe just using a new variable "len" for all snprintf can be readable?
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-04-15 8:01 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-04 14:06 [PATCH net-next v2] net: dsa: mv88e6xxx: control mdio bus-id truncation for long paths Josua Mayer
2024-04-04 14:29 ` Andrew Lunn
2024-04-15 8:01 ` 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.