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

> On 21/11/2023 23:15, Krzysztof Kozlowski wrote:
> > How is this even related to the problem? Please respond with concise
> > messages.
> >
> >> the "XXXX" for modprobe. For DSA tags, module names are generated
> >> according to a fixed rule based on the tag name. However, the switch
> >> variants do not have a fixed relation between module name and switch
> >> families (actually, there is not even a switch family name). I could
> >> (and did in a previous RFC) use the match data to inform each module
> >> name. However, it adds a non-obvious relation between the module name
> >> (defined in Makefile) and a string in code. I was starting to
> >> implement a lookup table to match compatible strings to their module
> >> names when I realized that I was just mapping a string to a module
> >> name, something like what module alias already does. That's when the
> >> MODULE_ALIAS("<the compatible string>") was introduced.
> >>
> >> After this discussion, I have some questions:
> >>
> >> I'm declaring the of_device_id match table in realtek-common as it is
> >> the same for both interfaces. I also moved MODULE_DEVICE_TABLE(of,
> >> realtek_common_of_match) to realtek-common. Should I keep the
> >> MODULE_DEVICE_TABLE() on each interface module (referencing the same
> >> table), or is it okay to keep it in the realtek-common module? If I
> >
> > Why would you have the same compatible entries in different modules? You
> > do understand that device node will become populated on first bind (bind
> > of the first device)?
> >
> >> need to move it to each interface module, can I reuse a shared
> >> of_device_id match table declared in realtek-common?
> >>
> >> If MODULE_ALIAS is not an acceptable solution, what would be the right
> >> one? Go back to the static mapping between the compatible string and
> >> the module name or is there a better solution?
>
> Probably the solution is to make the design the same as for all other
> complex drivers supporting more than one bus. If your ID table is
> defined in modules A and B, then their loading should not depend on
> aliases put in some additional "common" module. We solved this many
> times for devices residing on multiple buses (e.g. I2C and SPI), so why
> this has to be done in reverse order?
>
> If you ask what would be the acceptable solution, then my answer is: do
> the same as for most of other drivers, do not reinvent stuff like
> putting same ID table or module alias in two modules. The table is
> defined only once in each driver being loaded on uevent. From that
> driver you probe whatever device you have, including calling any common
> code, subprobes, subvariants etc.

Thank you, Krzysztof. I didn't design the driver in question; I was
just trying to cooperate.

Linus, Alvin, these are originally your work. I might have
misunderstood something and would appreciate some help in this
discussion.

I noticed that drivers like bmp280, inv_mpu6050, and adxl345 share a
common core with two interface drivers, both declaring the same
compatible strings. Is it an issue to have the same compatible string
in different modules? Is there a better reference to follow?

In realtek DSA drivers, the "interface" modules (realtek-smi and
realtek-mdio) are the actual drivers, containing compatible strings.
The subdriver/variant/family modules are just the switch logic with no
driver declaration. Currently, all subdrivers are loaded, which might
not be necessary as a device may not use two different realtek switch
families. Ideally, only one should be loaded. Can we request a module
at runtime, and is it acceptable to use MODULE_ALIAS() for a
"non-driver module" to make that easier?

The driver works without the module alias/request if it was loaded
before the interface driver. I aim to make it work in more scenarios.
These drivers are for embedded systems, so handling uevents might not
be feasible. I used the compatible string to avoid creating a new
string for the family name, but I could use anything else like
"rtl8366rb" only when the module name does not match the model used by
the compatible string. I didn't expect it to be used by anything
monitoring uevents.

For the driver structure, we have a few options:

1) Merge everything into a single realtek-switch, declaring the
compatible strings once and implementing it as both a platform and an
mdio driver.
2) Have a module for each interface (realtek-smi and realtek-mdio),
both repeating the compatible strings for all families, and load the
family as an interface dependency (the current approach).
3) Introduce a realtek-common module, a module for each interface,
both repeating the compatible strings for all families, and load the
family as an interface dependency (the first patch in this series).
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.
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).
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).

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.

Regards,

Luiz

Powered by blists - more mailing lists