Re: Possible bug into DSA2 code.

Re: Possible bug into DSA2 code.

[Rodolfo Giometti]

Hello,

I'm working with EPRESSObin and DSA2 where I added the ability to dynamically 
load and unload switch configurations by using DT-overlay (a patchwork from here 
https://lore.kernel.org/patchwork/patch/468129/). During my tests I notice that 
when I remove the overlay in order to disable the switch I got the following BUG 
message:

[   24.862079] ------------[ cut here ]------------
[   24.866767] kernel BUG at drivers/net/phy/mdio_bus.c:448!
[   24.872328] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
[   24.877967] Modules linked in:
[   24.881109] CPU: 0 PID: 2189 Comm: rmdir Not tainted 4.19.0-sw3720tsn1-038561
[   24.890509] Hardware name: Kbact sw3720tsn1 smart switch (DT)
[   24.896426] pstate: 20000005 (nzCv daif -PAN -UAO)
[   24.901365] pc : mdiobus_unregister+0x90/0x98
[   24.905838] lr : mv88e6xxx_mdios_unregister+0x64/0x88
[   24.911028] sp : ffff80006aea7730
[   24.914434] x29: ffff80006aea7730 x28: ffff80006adc27c0
[   24.919898] x27: ffff0000091e3000 x26: ffff0000090d9000
[   24.925365] x25: ffff80006c9420a0 x24: ffff80006c3e1c10
[   24.930828] x23: 0000000000000060 x22: ffff80006c9e6018
[   24.936294] x21: ffff80006c9e6110 x20: ffff80006c942800
[   24.941758] x19: ffff80006c942d40 x18: ffffffffffffffff
[   24.947225] x17: 0000000000000000 x16: ffff80006adc27c0
[   24.952690] x15: ffff0000090d96c8 x14: ffff000089198737
[   24.958156] x13: ffff000009198745 x12: ffff0000090d9940
[   24.963621] x11: ffff0000086be4b0 x10: 0000000000000040
[   24.969087] x9 : ffff0000090f4710 x8 : 0000000040000000
[   24.974553] x7 : ffff0000090d96c8 x6 : ffff80006a97a921
[   24.980018] x5 : ffff80006a97a920 x4 : 0000000000000fff
[   24.985483] x3 : 0000000000000000 x2 : 0000000000000000
[   24.990948] x1 : 0000000000000003 x0 : ffff80006c942800
[   24.996416] Process rmdir (pid: 2189, stack limit = 0x(____ptrval____))
[   25.003225] Call trace:
[   25.005737]  mdiobus_unregister+0x90/0x98
[   25.009858]  mv88e6xxx_mdios_unregister+0x64/0x88
[   25.014696]  mv88e6xxx_remove+0x2c/0x88
[   25.018637]  mdio_remove+0x20/0x48
[   25.022135]  device_release_driver_internal+0x1a8/0x240
[   25.027509]  device_release_driver+0x14/0x20
[   25.031899]  bus_remove_device+0x110/0x128
[   25.036109]  device_del+0x124/0x340
[   25.039693]  mdio_device_remove+0x14/0x28
[   25.043815]  mdiobus_unregister+0x50/0x98
[   25.047940]  orion_mdio_remove+0x34/0xb0
[   25.051970]  platform_drv_remove+0x24/0x50
[   25.056181]  device_release_driver_internal+0x1a8/0x240
[   25.061557]  device_release_driver+0x14/0x20
[   25.065947]  bus_remove_device+0x110/0x128
[   25.070158]  device_del+0x124/0x340
[   25.073742]  platform_device_del.part.3+0x24/0x90
[   25.078580]  platform_device_unregister+0x18/0x30
[   25.083422]  of_platform_device_destroy+0xb4/0xb8
[   25.088257]  of_platform_notify+0xa8/0x170
[   25.092471]  notifier_call_chain+0x54/0x98
[   25.096679]  blocking_notifier_call_chain+0x48/0x70
[   25.101697]  of_property_notify+0x60/0xa0
[   25.105819]  __of_changeset_entry_notify+0x54/0x100
[   25.110836]  __of_changeset_revert_notify+0x3c/0x70
[   25.115857]  of_overlay_remove+0x2ac/0x378
[   25.120066]  cfs_overlay_release+0x28/0x50
[   25.124278]  config_item_put.part.0+0x70/0xb0
[   25.128757]  config_item_put+0x10/0x20
[   25.132609]  configfs_rmdir+0x1ec/0x2e0
[   25.136554]  vfs_rmdir+0x7c/0x170
[   25.139956]  do_rmdir+0x17c/0x1d0
[   25.143361]  __arm64_sys_unlinkat+0x4c/0x60
[   25.147664]  el0_svc_common+0x60/0xe8
[   25.151426]  el0_svc_handler+0x2c/0x80
[   25.155279]  el0_svc+0x8/0xc
[   25.158236] Code: a94153f3 a9425bf5 a8c37bfd d65f03c0 (d4210000)
[   25.164509] ---[ end trace 5138591d8b9c9222 ]---

After looking into the kernel code I discovered that this depends to the commit
1eb59443e72c69edbb836626f9f7f7e82427eeac which modifications I report below:

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 921a36fd139d..4e0f3c268103 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -312,6 +312,18 @@ static int dsa_ds_apply(struct dsa_switch_tree *dst, struct
dsa_switch *ds)
          if (err < 0)
                  return err;

+       if (!ds->slave_mii_bus && ds->drv->phy_read) {
+               ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev);
+               if (!ds->slave_mii_bus)
+                       return -ENOMEM;
+
+               dsa_slave_mii_bus_init(ds);
+
+               err = mdiobus_register(ds->slave_mii_bus);
+               if (err < 0)
+                       return err;
+       }
+
          for (index = 0; index < DSA_MAX_PORTS; index++) {
                  port = ds->ports[index].dn;
                  if (!port)
@@ -361,6 +373,9 @@ static void dsa_ds_unapply(struct dsa_switch_tree *dst,
struct dsa_switch *ds)

                  dsa_user_port_unapply(port, index, ds);
          }
+
+       if (ds->slave_mii_bus && ds->drv->phy_read)
+               mdiobus_unregister(ds->slave_mii_bus);
   }

   static int dsa_dst_apply(struct dsa_switch_tree *dst)

This patch looks buggy to me because if this patch has the target to catch 
drivers that call dsa_ds_apply() having ds->slave_mii_bus set to NULL with a 
defined ds->ops->phy_read, then it should take into account also those drivers 
that have both ds->slave_mii_bus and ds->ops->phy_read already defined and then 
DO NOT call mdiobus_unregister() during dsa_ds_unapply()! This because DSA 
should NOT undo an operation it never did.

So we I see two possible solutions:

1) having both ds->slave_mii_bus and ds->ops->phy_read already defined is an 
error, then it must be signaled to the calling code, or

2) we have to use a flag to signal dsa_ds_unapply() what to do.

I don't know DSA too much to provide the rigth-thing(TM) so I'm waiting for a 
reply before proposing a patch. :-)

Ciao,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti

Re: Possible bug into DSA2 code.

[Andrew Lunn]

> So we I see two possible solutions:
> 
> 1) having both ds->slave_mii_bus and ds->ops->phy_read already defined is an
> error, then it must be signaled to the calling code, or

I don't think we can do that. mv88e6xxx optionally instantiates the
MDIO busses, depending on what is in device tree. If there is no mdio
property, we need the DSA core to create an MDIO bus.

Looking at the driver, ds->slave_mii_bus is assigned in
mv88e6xxx_setup().

We have talked about adding a teardown() to the ops structure. This
seems like another argument we should do it. The mv88e6xxx_teardown()
can set ds->slave_mii_bus back to NULL, undoing what it did in the
setup code.

      Andrew

Re: Possible bug into DSA2 code.

[Rodolfo Giometti]

On 09/02/2019 20:34, Andrew Lunn wrote:
>> So we I see two possible solutions:
>>
>> 1) having both ds->slave_mii_bus and ds->ops->phy_read already defined is an
>> error, then it must be signaled to the calling code, or
> 
> I don't think we can do that. mv88e6xxx optionally instantiates the
> MDIO busses, depending on what is in device tree. If there is no mdio
> property, we need the DSA core to create an MDIO bus.

OK, but using the following check to know if DSA did such allocation is not 
correct because DSA drivers can allocate it by their own:

static void dsa_switch_teardown(struct dsa_switch *ds)
{
         if (ds->slave_mii_bus && ds->ops->phy_read)
                 mdiobus_unregister(ds->slave_mii_bus);

Maybe can we add a flag to register ds->slave_mii_bus allocation by DSA?

> Looking at the driver, ds->slave_mii_bus is assigned in
> mv88e6xxx_setup().
> 
> We have talked about adding a teardown() to the ops structure. This
> seems like another argument we should do it. The mv88e6xxx_teardown()
> can set ds->slave_mii_bus back to NULL, undoing what it did in the
> setup code.

This seems reasonable to me, but in this case you have to call teardown() 
operation before calling mdiobus_unregister() into dsa_switch_teardown() or we 
still have the problem...

Ciao,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti

Re: Possible bug into DSA2 code.

[Florian Fainelli]

On 2/10/19 3:45 AM, Rodolfo Giometti wrote:
> On 09/02/2019 20:34, Andrew Lunn wrote:
>>> So we I see two possible solutions:
>>>
>>> 1) having both ds->slave_mii_bus and ds->ops->phy_read already
>>> defined is an
>>> error, then it must be signaled to the calling code, or
>>
>> I don't think we can do that. mv88e6xxx optionally instantiates the
>> MDIO busses, depending on what is in device tree. If there is no mdio
>> property, we need the DSA core to create an MDIO bus.
> 
> OK, but using the following check to know if DSA did such allocation is
> not correct because DSA drivers can allocate it by their own:
> 
> static void dsa_switch_teardown(struct dsa_switch *ds)
> {
>         if (ds->slave_mii_bus && ds->ops->phy_read)
>                 mdiobus_unregister(ds->slave_mii_bus);
> 
> Maybe can we add a flag to register ds->slave_mii_bus allocation by DSA?

If drivers allocate the slave_mii_bus, or use it as a pointer to their
bus, then they should not be providing a ds->ops->phy_read() callback
since we assume they would have mii_bus::read and mii_bus::write set to
their driver internal version.

> 
>> Looking at the driver, ds->slave_mii_bus is assigned in
>> mv88e6xxx_setup().
>>
>> We have talked about adding a teardown() to the ops structure. This
>> seems like another argument we should do it. The mv88e6xxx_teardown()
>> can set ds->slave_mii_bus back to NULL, undoing what it did in the
>> setup code.
> 
> This seems reasonable to me, but in this case you have to call
> teardown() operation before calling mdiobus_unregister() into
> dsa_switch_teardown() or we still have the problem...
> 
> Ciao,
> 
> Rodolfo
> 


-- 
Florian

Re: Possible bug into DSA2 code.

[Rodolfo Giometti]

On 11/02/2019 18:28, Florian Fainelli wrote:
> On 2/10/19 3:45 AM, Rodolfo Giometti wrote:
>> On 09/02/2019 20:34, Andrew Lunn wrote:
>>>> So we I see two possible solutions:
>>>>
>>>> 1) having both ds->slave_mii_bus and ds->ops->phy_read already
>>>> defined is an
>>>> error, then it must be signaled to the calling code, or
>>>
>>> I don't think we can do that. mv88e6xxx optionally instantiates the
>>> MDIO busses, depending on what is in device tree. If there is no mdio
>>> property, we need the DSA core to create an MDIO bus.
>>
>> OK, but using the following check to know if DSA did such allocation is
>> not correct because DSA drivers can allocate it by their own:
>>
>> static void dsa_switch_teardown(struct dsa_switch *ds)
>> {
>>          if (ds->slave_mii_bus && ds->ops->phy_read)
>>                  mdiobus_unregister(ds->slave_mii_bus);
>>
>> Maybe can we add a flag to register ds->slave_mii_bus allocation by DSA?
> 
> If drivers allocate the slave_mii_bus, or use it as a pointer to their
> bus, then they should not be providing a ds->ops->phy_read() callback
> since we assume they would have mii_bus::read and mii_bus::write set to
> their driver internal version.

I see, so having ds->slave_mii_bus and ds->ops->phy_read both not NULL into 
dsa_switch_setup() is a potential bug, I suppose... If so why not adding a 
BUG_ON() call to signal it instead of doing nothing? :-o

Ciao,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti

Re: Possible bug into DSA2 code.

[Florian Fainelli]

On 2/11/19 9:51 AM, Rodolfo Giometti wrote:
> On 11/02/2019 18:28, Florian Fainelli wrote:
>> On 2/10/19 3:45 AM, Rodolfo Giometti wrote:
>>> On 09/02/2019 20:34, Andrew Lunn wrote:
>>>>> So we I see two possible solutions:
>>>>>
>>>>> 1) having both ds->slave_mii_bus and ds->ops->phy_read already
>>>>> defined is an
>>>>> error, then it must be signaled to the calling code, or
>>>>
>>>> I don't think we can do that. mv88e6xxx optionally instantiates the
>>>> MDIO busses, depending on what is in device tree. If there is no mdio
>>>> property, we need the DSA core to create an MDIO bus.
>>>
>>> OK, but using the following check to know if DSA did such allocation is
>>> not correct because DSA drivers can allocate it by their own:
>>>
>>> static void dsa_switch_teardown(struct dsa_switch *ds)
>>> {
>>>          if (ds->slave_mii_bus && ds->ops->phy_read)
>>>                  mdiobus_unregister(ds->slave_mii_bus);
>>>
>>> Maybe can we add a flag to register ds->slave_mii_bus allocation by DSA?
>>
>> If drivers allocate the slave_mii_bus, or use it as a pointer to their
>> bus, then they should not be providing a ds->ops->phy_read() callback
>> since we assume they would have mii_bus::read and mii_bus::write set to
>> their driver internal version.
> 
> I see, so having ds->slave_mii_bus and ds->ops->phy_read both not NULL
> into dsa_switch_setup() is a potential bug, I suppose... If so why not
> adding a BUG_ON() call to signal it instead of doing nothing? :-o

If you have both non NULL, then your driver did allocate
ds->slave_mii_bus on its own, and also assigned a valid
ds->ops->phy_read() then things will work, except that
ds->ops->phy_read() will not be used. And yes, that is going to be
blowing away when the whole DSA tree gets teardowned.

If you want to add a check for that condition, that would be a good
thing, just not a BUG_ON(), propagate an error back to the caller and
abort the tree/switch probing.
-- 
Florian

Re: Possible bug into DSA2 code.

[Florian Fainelli]

On 2/11/19 10:01 AM, Florian Fainelli wrote:
> On 2/11/19 9:51 AM, Rodolfo Giometti wrote:
>> On 11/02/2019 18:28, Florian Fainelli wrote:
>>> On 2/10/19 3:45 AM, Rodolfo Giometti wrote:
>>>> On 09/02/2019 20:34, Andrew Lunn wrote:
>>>>>> So we I see two possible solutions:
>>>>>>
>>>>>> 1) having both ds->slave_mii_bus and ds->ops->phy_read already
>>>>>> defined is an
>>>>>> error, then it must be signaled to the calling code, or
>>>>>
>>>>> I don't think we can do that. mv88e6xxx optionally instantiates the
>>>>> MDIO busses, depending on what is in device tree. If there is no mdio
>>>>> property, we need the DSA core to create an MDIO bus.
>>>>
>>>> OK, but using the following check to know if DSA did such allocation is
>>>> not correct because DSA drivers can allocate it by their own:
>>>>
>>>> static void dsa_switch_teardown(struct dsa_switch *ds)
>>>> {
>>>>          if (ds->slave_mii_bus && ds->ops->phy_read)
>>>>                  mdiobus_unregister(ds->slave_mii_bus);
>>>>
>>>> Maybe can we add a flag to register ds->slave_mii_bus allocation by DSA?
>>>
>>> If drivers allocate the slave_mii_bus, or use it as a pointer to their
>>> bus, then they should not be providing a ds->ops->phy_read() callback
>>> since we assume they would have mii_bus::read and mii_bus::write set to
>>> their driver internal version.
>>
>> I see, so having ds->slave_mii_bus and ds->ops->phy_read both not NULL
>> into dsa_switch_setup() is a potential bug, I suppose... If so why not
>> adding a BUG_ON() call to signal it instead of doing nothing? :-o
> 
> If you have both non NULL, then your driver did allocate
> ds->slave_mii_bus on its own, and also assigned a valid
> ds->ops->phy_read() then things will work, except that
> ds->ops->phy_read() will not be used. And yes, that is going to be
> blowing away when the whole DSA tree gets teardowned.
> 
> If you want to add a check for that condition, that would be a good
> thing, just not a BUG_ON(), propagate an error back to the caller and
> abort the tree/switch probing.

Does that work:

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index a1917025e155..54cf6a5c865d 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -368,6 +368,9 @@ static int dsa_switch_setup(struct dsa_switch *ds)
        if (err)
                return err;

+       if (ds->slave_mii_bus && (ds->ops->phy_read || ds->ops->phy_write))
+               return -EINVAL;
+
        if (!ds->slave_mii_bus && ds->ops->phy_read) {
                ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev);
                if (!ds->slave_mii_bus)
diff --git a/net/dsa/legacy.c b/net/dsa/legacy.c
index cb42939db776..0796c6213be6 100644
--- a/net/dsa/legacy.c
+++ b/net/dsa/legacy.c
@@ -176,6 +176,9 @@ static int dsa_switch_setup_one(struct dsa_switch *ds,
        if (ret)
                return ret;

+       if (ds->slave_mii_bus && (ops->phy_read || ops->phy_write))
+               return -EINVAL;
+
        if (!ds->slave_mii_bus && ops->phy_read) {
                ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev);
                if (!ds->slave_mii_bus)

> 


-- 
Florian

Re: Possible bug into DSA2 code.

[Rodolfo Giometti]

On 11/02/2019 20:13, Florian Fainelli wrote:
> Does that work:
> 
> diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
> index a1917025e155..54cf6a5c865d 100644
> --- a/net/dsa/dsa2.c
> +++ b/net/dsa/dsa2.c
> @@ -368,6 +368,9 @@ static int dsa_switch_setup(struct dsa_switch *ds)
>          if (err)
>                  return err;
> 
> +       if (ds->slave_mii_bus && (ds->ops->phy_read || ds->ops->phy_write))
> +               return -EINVAL;
> +
>          if (!ds->slave_mii_bus && ds->ops->phy_read) {
>                  ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev);
>                  if (!ds->slave_mii_bus)
> diff --git a/net/dsa/legacy.c b/net/dsa/legacy.c
> index cb42939db776..0796c6213be6 100644
> --- a/net/dsa/legacy.c
> +++ b/net/dsa/legacy.c
> @@ -176,6 +176,9 @@ static int dsa_switch_setup_one(struct dsa_switch *ds,
>          if (ret)
>                  return ret;
> 
> +       if (ds->slave_mii_bus && (ops->phy_read || ops->phy_write))
> +               return -EINVAL;
> +
>          if (!ds->slave_mii_bus && ops->phy_read) {
>                  ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev);
>                  if (!ds->slave_mii_bus)

OK, now probing of mv88e6085 fails!

[   42.745004] mv88e6085: probe of d0032004.mdio-mii:01 failed with error -22

Now I suppose mv88e6085's driver should set ds->slave_mii_bus as follow:

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 54a5b660640a..bb46ebbb2bb8 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2399,7 +2399,7 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
         int i;

         chip->ds = ds;
-       ds->slave_mii_bus = mv88e6xxx_default_mdio_bus(chip);
+       ds->slave_mii_bus = NULL;

         mutex_lock(&chip->reg_lock);

Is that right?

Ciao,

Rodolfo


-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti

Re: Possible bug into DSA2 code.

[Andrew Lunn]

> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 54a5b660640a..bb46ebbb2bb8 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -2399,7 +2399,7 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
>         int i;
> 
>         chip->ds = ds;
> -       ds->slave_mii_bus = mv88e6xxx_default_mdio_bus(chip);
> +       ds->slave_mii_bus = NULL;
> 
>         mutex_lock(&chip->reg_lock);
> 
> Is that right?

Hi Rodolfo

Humm, that needs testing. There are two used combinations you need to
test:

No MDIO node in device tree, e.g.
arch/arm/boot/dts/kirkwood-dir665.dts

MDIO node in device tree, e.g:
arch/arm/boot/dts/vf610-zii-dev-rev-b.dts

There is a third combination which will appear soon. There is only the
external mdio bus in device tree:

                                port@9 {
                                        reg = <9>;
                                        label = "eth_cu_1000_2";
                                        phy-handle = <&phy9>;
                                        phy-mode = "sgmii";
                                        managed = "in-band-status";
                                };
                        };

                        mdio1 {
                                compatible = "marvell,mv88e6xxx-mdio-external";
                                #address-cells = <1>;
                                #size-cells = <0>;

                                phy9: phy9@0 {
                                        compatible = "ethernet-phy-ieee802.3-c45";
                                        pinctrl-0 = <&pinctrl_gpio_phy9>;
                                        pinctrl-names = "default";
                                        interrupt-parent = <&gpio2>;
                                        interrupts = <30 IRQ_TYPE_LEVEL_LOW>;
                                        reg = <0>;
                                };
                        };

Here port 9 uses the external MDIO bus and all the other ports
implicitly make use of the internal MDIO bus.

     Andrew

Re: Possible bug into DSA2 code.

[Rodolfo Giometti]

On 19/02/2019 01:03, Andrew Lunn wrote:
>> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
>> index 54a5b660640a..bb46ebbb2bb8 100644
>> --- a/drivers/net/dsa/mv88e6xxx/chip.c
>> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
>> @@ -2399,7 +2399,7 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
>>          int i;
>>
>>          chip->ds = ds;
>> -       ds->slave_mii_bus = mv88e6xxx_default_mdio_bus(chip);
>> +       ds->slave_mii_bus = NULL;
>>
>>          mutex_lock(&chip->reg_lock);
>>
>> Is that right?
> 
> Hi Rodolfo
> 
> Humm, that needs testing. There are two used combinations you need to
> test:
> 
> No MDIO node in device tree, e.g.
> arch/arm/boot/dts/kirkwood-dir665.dts
> 
> MDIO node in device tree, e.g:
> arch/arm/boot/dts/vf610-zii-dev-rev-b.dts

I'm sorry but I haven't such boards... :'(

> There is a third combination which will appear soon. There is only the
> external mdio bus in device tree:
> 
>                                  port@9 {
>                                          reg = <9>;
>                                          label = "eth_cu_1000_2";
>                                          phy-handle = <&phy9>;
>                                          phy-mode = "sgmii";
>                                          managed = "in-band-status";
>                                  };
>                          };
> 
>                          mdio1 {
>                                  compatible = "marvell,mv88e6xxx-mdio-external";
>                                  #address-cells = <1>;
>                                  #size-cells = <0>;
> 
>                                  phy9: phy9@0 {
>                                          compatible = "ethernet-phy-ieee802.3-c45";
>                                          pinctrl-0 = <&pinctrl_gpio_phy9>;
>                                          pinctrl-names = "default";
>                                          interrupt-parent = <&gpio2>;
>                                          interrupts = <30 IRQ_TYPE_LEVEL_LOW>;
>                                          reg = <0>;
>                                  };
>                          };
> 
> Here port 9 uses the external MDIO bus and all the other ports
> implicitly make use of the internal MDIO bus.

Mmm... maybe should code into drivers/net/dsa/mv88e6xxx use private data to hold 
used mdio busses instead of ds->slave_mii_bus pointer?

Ciao,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti

Re: Possible bug into DSA2 code.

[Andrew Lunn]

On Wed, Feb 20, 2019 at 08:54:01AM +0100, Rodolfo Giometti wrote:
> On 19/02/2019 01:03, Andrew Lunn wrote:
> >>diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> >>index 54a5b660640a..bb46ebbb2bb8 100644
> >>--- a/drivers/net/dsa/mv88e6xxx/chip.c
> >>+++ b/drivers/net/dsa/mv88e6xxx/chip.c
> >>@@ -2399,7 +2399,7 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
> >>         int i;
> >>
> >>         chip->ds = ds;
> >>-       ds->slave_mii_bus = mv88e6xxx_default_mdio_bus(chip);
> >>+       ds->slave_mii_bus = NULL;
> >>
> >>         mutex_lock(&chip->reg_lock);
> >>
> >>Is that right?
> >
> >Hi Rodolfo
> >
> >Humm, that needs testing. There are two used combinations you need to
> >test:
> >
> >No MDIO node in device tree, e.g.
> >arch/arm/boot/dts/kirkwood-dir665.dts
> >
> >MDIO node in device tree, e.g:
> >arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
> 
> I'm sorry but I haven't such boards... :'(

Hi Rodolfo

I know. But you should be able to modify the device tree of the boards
you are using to follow these patterns, and test them.

> 
> >There is a third combination which will appear soon. There is only the
> >external mdio bus in device tree:
> >
> >                                 port@9 {
> >                                         reg = <9>;
> >                                         label = "eth_cu_1000_2";
> >                                         phy-handle = <&phy9>;
> >                                         phy-mode = "sgmii";
> >                                         managed = "in-band-status";
> >                                 };
> >                         };
> >
> >                         mdio1 {
> >                                 compatible = "marvell,mv88e6xxx-mdio-external";
> >                                 #address-cells = <1>;
> >                                 #size-cells = <0>;
> >
> >                                 phy9: phy9@0 {
> >                                         compatible = "ethernet-phy-ieee802.3-c45";
> >                                         pinctrl-0 = <&pinctrl_gpio_phy9>;
> >                                         pinctrl-names = "default";
> >                                         interrupt-parent = <&gpio2>;
> >                                         interrupts = <30 IRQ_TYPE_LEVEL_LOW>;
> >                                         reg = <0>;
> >                                 };
> >                         };
> >
> >Here port 9 uses the external MDIO bus and all the other ports
> >implicitly make use of the internal MDIO bus.
> 
> Mmm... maybe should code into drivers/net/dsa/mv88e6xxx use private data to
> hold used mdio busses instead of ds->slave_mii_bus pointer?

I suspend that might break some of these different use cases.

Either there needs to be a well argued analysis, or some testing of
these setups.

      Andrew