Hi Saravana,

Sorry for the delayed response.

On 8/23/21 8:50 PM, Saravana Kannan wrote:

On Sun, Aug 22, 2021 at 7:19 AM Alvin Šipraga [off-list ref] wrote:

quoted

Hi Saravana,

Thanks for the follow-up. I tested your change and it does the trick:
there is no deferral and the PHY driver gets probed first-try during the
mdiobus registration during the call to dsa_register_switch().

I'm fairly certain the mdiobus registration happens before
dsa_register_switch(). It's in the probe call path of the DSA. The
connecting of the PHYs with the DSA is what happens when
dsa_register_switch() is called.

Hm, are you sure about this? My understanding is that mdiobus 
registration happens as follows:

dsa_register_switch()
     -> ...
         -> rtl836{6,5mb}_setup() # see [1] for RFC patch with rtl8365mb
             -> realtek_smi_setup_mdio()
                 -> of_mdiobus_register()

As it stands, dsa_register_switch() is currently called from 
realtek_smi_probe(). Your patch just moves this call to 
realtek_smi_sync_state(), but per the above, the mdiobus registration 
happens inside dsa_register_switch(), meaning it doesn't happen in the 
probe call path. Or am I missing something? I'm happy to be wrong :-)

[1] https://lore.kernel.org/netdev/20210822193145.1312668-1-alvin@pqrs.dk/T/ (local)

quoted

I tested
with the switch, PHY, and tagging drivers all builtin, or all modules,
and it worked in both cases.

On 8/20/21 6:52 PM, Saravana Kannan wrote:

quoted

Hi Alvin,

Can you give this a shot to see if it fixes your issue? It basically
delays the registration of dsa_register_switch() until all the
consumers of this switch have probed. So it has a couple of caveats:

Hm, weren't the only consumers the PHYs themselves? It seems like the
main effect of your change is that - by doing the actual
dsa_register_switch() call after the switch driver probe - the
ethernet-switch (provider) is already probed, thereby allowing the PHY
(consumer) to probe immediately.

Correct-ish -- if you modify this to account for what I said above.

quoted

quoted

1. I'm hoping the PHYs are the only consumers of this switch.

In my case that is true, if you count the mdio_bus as well:

/sys/devices/platform/ethernet-switch# ls -l consumer\:*
lrwxrwxrwx    1 root     root             0 Aug 22 16:00
consumer:mdio_bus:SMI-0 ->
../../virtual/devlink/platform:ethernet-switch--mdio_bus:SMI-0
lrwxrwxrwx    1 root     root             0 Aug 22 16:00
consumer:mdio_bus:SMI-0:00 ->
../../virtual/devlink/platform:ethernet-switch--mdio_bus:SMI-0:00
lrwxrwxrwx    1 root     root             0 Aug 22 16:00
consumer:mdio_bus:SMI-0:01 ->
../../virtual/devlink/platform:ethernet-switch--mdio_bus:SMI-0:01
lrwxrwxrwx    1 root     root             0 Aug 22 16:00
consumer:mdio_bus:SMI-0:02 ->
../../virtual/devlink/platform:ethernet-switch--mdio_bus:SMI-0:02
lrwxrwxrwx    1 root     root             0 Aug 22 16:00
consumer:mdio_bus:SMI-0:03 ->
../../virtual/devlink/platform:ethernet-switch--mdio_bus:SMI-0:03

Hmm... mdio_bus being a consumer should prevent the sync_state() from
being called on "ethernet-switch". What's the value of the "status"
and "sync_state_only" files inside that mdio_bus folder?

Without your patch:

/sys/devices/platform/ethernet-switch# ls -l consumer\:*
lrwxrwxrwx    1 root     root             0 Aug 25 13:42 
consumer:mdio_bus:SMI-0 -> 
../../virtual/devlink/platform:ethernet-switch--mdio_bus:SMI-0
/sys/devices/platform/ethernet-switch# cat consumer\:*/status
available
/sys/devices/platform/ethernet-switch# cat consumer\:*/sync_state_only
1


With your patch:

0.0.0.0@CA44-:/sys/devices/platform/ethernet-switch# ls -l consumer\:*
lrwxrwxrwx    1 root     root             0 Aug 25 15:03 
consumer:mdio_bus:SMI-0 -> 
../../virtual/devlink/platform:ethernet-switch--mdio_bus:SMI-0
lrwxrwxrwx    1 root     root             0 Aug 25 15:03 
consumer:mdio_bus:SMI-0:00 -> 
../../virtual/devlink/platform:ethernet-switch--mdio_bus:SMI-0:00
lrwxrwxrwx    1 root     root             0 Aug 25 15:03 
consumer:mdio_bus:SMI-0:01 -> 
../../virtual/devlink/platform:ethernet-switch--mdio_bus:SMI-0:01
lrwxrwxrwx    1 root     root             0 Aug 25 15:03 
consumer:mdio_bus:SMI-0:02 -> 
../../virtual/devlink/platform:ethernet-switch--mdio_bus:SMI-0:02
lrwxrwxrwx    1 root     root             0 Aug 25 15:03 
consumer:mdio_bus:SMI-0:03 -> 
../../virtual/devlink/platform:ethernet-switch--mdio_bus:SMI-0:03
0.0.0.0@CA44-:/sys/devices/platform/ethernet-switch#
0.0.0.0@CA44-:/sys/devices/platform/ethernet-switch#
0.0.0.0@CA44-:/sys/devices/platform/ethernet-switch# cat 
consumer\:*/status
available
active
active
active
active
0.0.0.0@CA44-:/sys/devices/platform/ethernet-switch# cat 
consumer\:*/sync_state_only
1
0
0
0
0

Hope that helps you understand what's going on better.

BTW, I noticed that when I build realtek-smi as a module with your 
patch, my kernel crashes if I unload the module. I haven't debugged this 
because it was just a test, nor did I get a stacktrace. LMK if you want 
more info.

quoted

quoted

2. All of them have to probe successfully before the switch will
register itself.

Yes.

Right, it's a yes in your case. But will it be a yes for all instances
of "realtek,rtl8366rb"?

quoted

quoted

3. If dsa_register_switch() fails, we can't defer the probe (because
it already succeeded). But I'm not sure if it's a likely error code.

It's of course possible that dsa_register_switch() fails. Assuming
fw_devlink is doing its job properly, I think the reason is most likely
going to be something specific to the driver, such as a communication
timeout with the switch hardware itself.

But what if someone sets fw_devlink=permissive? Is it okay to break
this? There are ways to make this work for fw_devlink=permissive and
=on -- you check for each and decide where to call
dsa_register_switch() based on that.

I am new to DSA myself so I think I am unqualified to answer whether 
it's OK to break things or not.

quoted

I get the impression that you don't necessarily regard this change as a
proper fix, so I'm happy to do further tests if you choose to
investigate further.

I thought about this in the background the past few days. I think
there are a couple of options:
1. We (community/Andrew) agree that this driver would only work with
fw_devlink=on and we can confirm that the other upstream uses of
"realtek,rtl8366rb" won't have any unprobed consumers problem and
switch to using my patch. Benefit is that it's a trivial and quick
change that gets things working again.
2. The "realtek,rtl8366rb" driver needs to be fixed to use a
"component device". A component device is a logical device that
represents a group of other devices. It's only initialized after all
these devices have probed successfully. The actual switch should be a
component device and it should call dsa_register_switch() in it's
"bind" (equivalent of probe). That way you can explicitly control what
devices need to be probed instead of depending on sync_state() that
have a bunch of caveats.

Alvin, do you want to take up (2)?

I can give it a shot, but first:

   - It seems Andrew may also need some convincing that this is the 
right approach.
   - Are you sure that this will solve the problem? See what I wrote 
upstairs in this email.
   - I have never written - nor can I recall reading - a component 
driver, so I would appreciate if you could point me to an upstream 
example that you think is illustrative.

There's one more issue: I do not have an RTL8366 to test on - rather, I 
encountered this problem in realtek-smi while writing a new subdriver 
for it to support the RTL8365MB. So any proposed fix may be perceived as 
speculative if I cannot test on the '66 hardware. In that case this may 
have to wait until the '65MB subdriver is accepted. Many ifs and maybes, 
but don't take it to mean I'm not interested in helping. On the contrary 
- I would like to fix this bug since I am affected by it!

Kind regards,
Alvin

-Saravana