Date: Mon, 27 Nov 2023 19:24:16 -0300
From: Luiz Angelo Daros de Luca <luizluca@...il.com>
To: Alvin Šipraga <ALSI@...g-olufsen.dk>
Cc: Krzysztof Kozlowski <krzk@...nel.org>, "linus.walleij@...aro.org" <linus.walleij@...aro.org>, 
	Vladimir Oltean <olteanv@...il.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>, 
	"andrew@...n.ch" <andrew@...n.ch>, "f.fainelli@...il.com" <f.fainelli@...il.com>, 
	"davem@...emloft.net" <davem@...emloft.net>, "edumazet@...gle.com" <edumazet@...gle.com>, 
	"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>, 
	"arinc.unal@...nc9.com" <arinc.unal@...nc9.com>
Subject: Re: [net-next 2/2] net: dsa: realtek: load switch variants on demand

Hi Alvin,

> > 4) Introduce a realtek-common module, a module for each interface,
> > both repeating the compatible strings for all families, and expect the
> > family module to be already loaded.
>
> The kernel should be able to autoload. Skip.
>
> > 5) Introduce a realtek-common module, a module for each interface,
> > both repeating the compatible strings for all families, and request
> > the family module to be loaded (the last patch in this series).
>
> I had reservations before about this approach and I think I am not
> the only one. Let's consider the other options.

I'm not sure if getting/putting a module is a problem or if I can
request it when missing. I would like some options on that specific
topic from the experts. It seems to happen in many places, even in DSA
tag code.

> > 6) Introduce a realtek-common module, merging everything from
> > realtek-smi and realtek-mdio but the driver declarations and implement
> > each family as a single module that works both as a platform and an
> > mdio driver.
> > 7) Introduce a realtek-common module, merging everything from
> > realtek-smi and realtek-mdio but the driver declarations and create
> > two modules for each family as real drivers, repeating the compatible
> > strings on each family (e.g., {rtl8366rb, rtl8365mb}_{smi,mdio}.ko).
>
> Yeah, something like this. Roughly I think we want:

Just to be sure, you are suggesting something like 6), not 7), right?

> - generic boilerplate probe/remove and regmap setup code in
>   realtek-common.c
> - interface code in realtek-{smi,mdio}.c
> - chip variant code in rtl8365mb.c and rtl8366rb.c
>
> The module dependencies only need to go upwards, i.e.:
>
>        realtek-common
>          ^       ^
>          |       |  use symbols realtek_common_probe()
>          |       |              realtek_common_remove()
>          |       |
>  realtek-smi   realtek-mdio
>       ^   ^     ^   ^
>       |   |     |   |  use symbols realtek_mdio_probe()
>       |    \   /    |              realtek_mdio_remove()
>       |     \ /     |
>       |      X      |  or symbols realtek_smi_probe()
>       |     / \     |             realtek_smi_remove()
>       |    /   \    |
>       |   /     \   |  or both
>       |  /       \  |
>   rtl8365mb     rtl8366rb

It makes sense to turn rtl8365mb/rtl8366rb into both a platform and an
MDIO driver, similar to the merge between realtek-mdio/realtek-smi
Vladmir suggested before, with a custom module_init/exit doing the
job. I didn't invest too much time thinking about how data structures
would fit in this new model. realtek_priv would probably be allocated
by the variant modules with little left for interface to probe/setup.

> The setup_interface function pointer is only needed for SMI, and
> currently the chip interface drivers are the ones calling it. It is kind
> of ugly that this gets passed all the way up, copied into realtek_priv,
> and then called all the way down again... there is probably a more
> elegant solution. I am mainly trying to illustrate how to handle the
> module knot you are trying to unpick.

The "realtek_smi_setup_mdio()" used in setup_interface isn't really
necessary (like it happens in realtek-mdio). It could be used (or not)
by both interfaces. The "realtek,smi-mdio" compatible string is
misleading, indicating it might be something specific to the SMI
interface HW while it is just how the driver was implemented. A
"realtek,slave_mdio" or "realtek,user_mii" would be better.

I believe the strange relations between realtek interface modules and
variants tend to go away if we put things in the right place. The
probe will happen mostly in common and variant modules, leaving just a
minor probe job for the interface module called from the variant
driver (using pre/post approach) or from common (using a
realtek_interface_ops). Anyway, I'll leave the discussion of who calls
who after we settle the role of each module.

The most likely proposal is to convert both variant modules from a
subdriver code into a both platform(for SMI) and mdio driver. Probe
will use both realtek_<interface>_probe() and realtek_common_probe()
when appropriated. Variants will call the interface and not the other
way around.

> > Solutions 1, 2, and 3 may use more resources than needed. My test
> > device, for example, cannot handle them. Solution 7 is similar to the
> > examples I found in the kernel. Solutions 1 and 6 are the only ones
> > not repeating the same compatible strings in different modules, if
> > that's a problem.
>
> You will have noticed that with the above solution, the chip variant
> modules will invariably require both interface modules to be loaded if
> the kernel is built with both REALTEK_SMI and REALTEK_MDIO. I hope that
> your resource-constrained system has enough headroom to accommodate
> that. If not then I am afraid you might simply be out of luck.

I wouldn't say it will invariably require both interface modules to be
loaded. The dynamic load would be much simpler if variants request the
interface module as we only have two (at most 3 with a future
realtek-spi) modules. We would just need to call a
realtek_interface_get() and realtek_interface_put() on each respective
probe. The module names will be well-known with no issues with
module_alias.

Thanks for your help, Alvin. I'll wait for a couple of more days for
others to manifest.

Regards,

Luiz

Powered by blists - more mailing lists