| Message ID | 20231117235140.1178-3-luizluca@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: realtek: Introduce realtek_common, load variants on demand | expand |
On Fri, Nov 17, 2023 at 08:50:01PM -0300, Luiz Angelo Daros de Luca wrote: > diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c > index b865e11955ca..c447dd815a59 100644 > --- a/drivers/net/dsa/realtek/realtek-mdio.c > +++ b/drivers/net/dsa/realtek/realtek-mdio.c > @@ -145,7 +145,7 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev) > ret = priv->ops->detect(priv); > if (ret) { > dev_err(dev, "unable to detect switch\n"); > - return ret; > + goto err_variant_put; > } > > priv->ds->ops = priv->variant->ds_ops_mdio; > @@ -154,10 +154,15 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev) > ret = dsa_register_switch(priv->ds); > if (ret) { > dev_err_probe(dev, ret, "unable to register switch\n"); > - return ret; > + goto err_variant_put; > } > > return 0; > + > +err_variant_put: > + realtek_variant_put(priv->variant); > + > + return ret; > } > > static void realtek_mdio_remove(struct mdio_device *mdiodev) This is not so great at all from an API presentation point of view - the fact that the caller needs to know that realtek_variant_put() undoes the effect of realtek_common_probe(). You said you don't like too many abstractions, and fair enough, but maybe we could have - realtek_common_probe_pre(), realtek_common_remove_pre() - realtek_common_probe_post(), realtek_common_remove_post() which leads to even more code sharing from probe(), as well as an opportunity to have a clearly matched unwind function for everything that is done in probe()?
On 18/11/2023 00:50, Luiz Angelo Daros de Luca wrote: > realtek-common had a hard dependency on both switch variants. As a > result, it was not possible to selectively load only one model at > runtime. Now, variants are registered in the realtek-common module, and > interface modules look for a variant using the compatible string. ... > diff --git a/drivers/net/dsa/realtek/realtek-common.h b/drivers/net/dsa/realtek/realtek-common.h > index 90a949386518..6de4991d8b5c 100644 > --- a/drivers/net/dsa/realtek/realtek-common.h > +++ b/drivers/net/dsa/realtek/realtek-common.h > @@ -5,6 +5,37 @@ > > #include <linux/regmap.h> > > +struct realtek_variant_entry { > + const struct realtek_variant *variant; > + const char *compatible; > + struct module *owner; > + struct list_head list; > +}; > + > +#define module_realtek_variant(__variant, __compatible) \ > +static struct realtek_variant_entry __variant ## _entry = { \ > + .compatible = __compatible, \ > + .variant = &(__variant), \ > + .owner = THIS_MODULE, \ > +}; \ > +static int __init realtek_variant_module_init(void) \ > +{ \ > + realtek_variant_register(&__variant ## _entry); \ > + return 0; \ > +} \ > +module_init(realtek_variant_module_init) \ > + \ > +static void __exit realtek_variant_module_exit(void) \ > +{ \ > + realtek_variant_unregister(&__variant ## _entry); \ > +} \ > +module_exit(realtek_variant_module_exit); \ > + \ > +MODULE_ALIAS(__compatible) No, why do you need it? You should not need MODULE_ALIAS() in normal cases. If you need it, usually it means your device ID table is wrong (e.g. misses either entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute for incomplete ID table. Entire abstraction/macro is pointless and make the code less readable. Best regards, Krzysztof
Hi Krzysztof, On Mon, Nov 20, 2023 at 10:20:13AM +0100, Krzysztof Kozlowski wrote: > No, why do you need it? You should not need MODULE_ALIAS() in normal > cases. If you need it, usually it means your device ID table is wrong > (e.g. misses either entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is > not a substitute for incomplete ID table. > > Entire abstraction/macro is pointless and make the code less readable. Are you saying that the line MODULE_DEVICE_TABLE(of, realtek_common_of_match); should be put in all of realtek-mdio.c, realtek-smi.c, rtl8365mb.c and rtl8366rb.c, but not in realtek-common.c? There are 5 kernel modules involved, 2 for interfaces and 2 for switches. Even if the same OF device ID table could be used to load multiple modules, I'm not sure (a) how to avoid loading the interface driver which will not be used (SMI if it's a MDIO-connected switch, or MDIO if it's an SMI connected switch) (b) how to ensure that the drivers are loaded in the right order, i.e. the switch drivers are loaded before the interface drivers
On 20/11/2023 14:48, Vladimir Oltean wrote: > Hi Krzysztof, > > On Mon, Nov 20, 2023 at 10:20:13AM +0100, Krzysztof Kozlowski wrote: >> No, why do you need it? You should not need MODULE_ALIAS() in normal >> cases. If you need it, usually it means your device ID table is wrong >> (e.g. misses either entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is >> not a substitute for incomplete ID table. >> >> Entire abstraction/macro is pointless and make the code less readable. > > Are you saying that the line > > MODULE_DEVICE_TABLE(of, realtek_common_of_match); > > should be put in all of realtek-mdio.c, realtek-smi.c, rtl8365mb.c and > rtl8366rb.c, but not in realtek-common.c? Driver should use MODULE_DEVICE_TABLE() for the table not MODULE_ALIAS() for each entry. I don't judge where should it be put. I just dislike usage of aliases as a incomplete-substitute of proper table. > > There are 5 kernel modules involved, 2 for interfaces and 2 for switches. > > Even if the same OF device ID table could be used to load multiple > modules, I'm not sure > (a) how to avoid loading the interface driver which will not be used > (SMI if it's a MDIO-connected switch, or MDIO if it's an SMI > connected switch) > (b) how to ensure that the drivers are loaded in the right order, i.e. > the switch drivers are loaded before the interface drivers I am sorry, I do not understand the problem. The MODULE_DEVICE_TABLE and MODULE_ALIAS create exactly the same behavior. How any of above would happen with table but not with alias having exactly the same compatibles? Best regards, Krzysztof
Hi Krzysztof, > > On Mon, Nov 20, 2023 at 10:20:13AM +0100, Krzysztof Kozlowski wrote: > >> No, why do you need it? You should not need MODULE_ALIAS() in normal > >> cases. If you need it, usually it means your device ID table is wrong > >> (e.g. misses either entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is > >> not a substitute for incomplete ID table. > >> > >> Entire abstraction/macro is pointless and make the code less readable. > > > > Are you saying that the line > > > > MODULE_DEVICE_TABLE(of, realtek_common_of_match); > > > > should be put in all of realtek-mdio.c, realtek-smi.c, rtl8365mb.c and > > rtl8366rb.c, but not in realtek-common.c? > > Driver should use MODULE_DEVICE_TABLE() for the table not MODULE_ALIAS() > for each entry. I don't judge where should it be put. I just dislike > usage of aliases as a incomplete-substitute of proper table. MODULE_ALIAS() is in use here because of its relation with modprobe, not inside the kernel. MODULE_DEVICE_TABLE is also in use but it does not seem to generate any information usable by modprobe. > > > > There are 5 kernel modules involved, 2 for interfaces and 2 for switches. > > > > Even if the same OF device ID table could be used to load multiple > > modules, I'm not sure > > (a) how to avoid loading the interface driver which will not be used > > (SMI if it's a MDIO-connected switch, or MDIO if it's an SMI > > connected switch) > > (b) how to ensure that the drivers are loaded in the right order, i.e. > > the switch drivers are loaded before the interface drivers > > I am sorry, I do not understand the problem. The MODULE_DEVICE_TABLE and > MODULE_ALIAS create exactly the same behavior. How any of above would > happen with table but not with alias having exactly the same compatibles? Realtek switches can be managed through different interfaces. In the current kernel implementation, there is an MDIO driver (realtek-mdio) for switches connected to the MDIO bus, and a platform driver implementing the SMI protocol (a simple GPIO bit-bang). The actual switch logic is implemented in two different switch family/variant modules: rtl8365mb and rtl8366 (currently only for rtl8366rb). As of today, both Realtek interface modules directly reference each switch variant symbol, creating a hard dependency and forcing the interface module to load both switch family variants. Each interface module provides the same compatible strings for both variants, but I haven't investigated whether this is problematic or not. It appears that there is no mechanism to autoload modules based on compatible strings from the device tree, and each interface module is a different type of driver. This patch set accomplishes two things: it moves some shared code to a new Realtek common module and changes the hard dependency between interface and variant modules into a more dynamic relation. Each variant module registers itself in realtek-common, and interface modules look for the appropriate variant. However, as interface modules do not directly depend on variant modules, they might not have been loaded yet, causing the driver probe to fail. The solution I opted for was to request the module during the interface probe (similar to what happens with DSA tag modules), triggering a userland "modprobe XXXX." Even without the variant module loaded, we know the compatible string that matched the interface driver. We can also have some extra info from match data, but I chose to simply keep using the compatible string. The issue is how to get 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 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? Regards, Luiz
On 21/11/2023 15:40, Luiz Angelo Daros de Luca wrote: > Hi Krzysztof, > >>> On Mon, Nov 20, 2023 at 10:20:13AM +0100, Krzysztof Kozlowski wrote: >>>> No, why do you need it? You should not need MODULE_ALIAS() in normal >>>> cases. If you need it, usually it means your device ID table is wrong >>>> (e.g. misses either entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is >>>> not a substitute for incomplete ID table. >>>> >>>> Entire abstraction/macro is pointless and make the code less readable. >>> >>> Are you saying that the line >>> >>> MODULE_DEVICE_TABLE(of, realtek_common_of_match); >>> >>> should be put in all of realtek-mdio.c, realtek-smi.c, rtl8365mb.c and >>> rtl8366rb.c, but not in realtek-common.c? >> >> Driver should use MODULE_DEVICE_TABLE() for the table not MODULE_ALIAS() >> for each entry. I don't judge where should it be put. I just dislike >> usage of aliases as a incomplete-substitute of proper table. > > MODULE_ALIAS() is in use here because of its relation with modprobe, > not inside the kernel. The same as MODULE_DEVICE_TABLE. > MODULE_DEVICE_TABLE is also in use but it does not seem to generate > any information usable by modprobe. It does, exactly the same from functional point of view... which information are you missing? > >>> >>> There are 5 kernel modules involved, 2 for interfaces and 2 for switches. >>> >>> Even if the same OF device ID table could be used to load multiple >>> modules, I'm not sure >>> (a) how to avoid loading the interface driver which will not be used >>> (SMI if it's a MDIO-connected switch, or MDIO if it's an SMI >>> connected switch) >>> (b) how to ensure that the drivers are loaded in the right order, i.e. >>> the switch drivers are loaded before the interface drivers >> >> I am sorry, I do not understand the problem. The MODULE_DEVICE_TABLE and >> MODULE_ALIAS create exactly the same behavior. How any of above would >> happen with table but not with alias having exactly the same compatibles? > > Realtek switches can be managed through different interfaces. In the > current kernel implementation, there is an MDIO driver (realtek-mdio) > for switches connected to the MDIO bus, and a platform driver > implementing the SMI protocol (a simple GPIO bit-bang). > > The actual switch logic is implemented in two different switch > family/variant modules: rtl8365mb and rtl8366 (currently only for > rtl8366rb). As of today, both Realtek interface modules directly > reference each switch variant symbol, creating a hard dependency and > forcing the interface module to load both switch family variants. Each > interface module provides the same compatible strings for both > variants, but I haven't investigated whether this is problematic or > not. It appears that there is no mechanism to autoload modules based > on compatible strings from the device tree, and each interface module > is a different type of driver. ??? So entire Linux kernel is broken? Somehow autoloading modules based on compatible strings from the device tree works for every other driver properly using tables... So again, I am repeating: stop using MODULE_ALIAS for missing/incomplete tables > > This patch set accomplishes two things: it moves some shared code to a Which should not... One thing per patch. > new Realtek common module and changes the hard dependency between > interface and variant modules into a more dynamic relation. Each > variant module registers itself in realtek-common, and interface > modules look for the appropriate variant. However, as interface > modules do not directly depend on variant modules, they might not have > been loaded yet, causing the driver probe to fail. > > The solution I opted for was to request the module during the > interface probe (similar to what happens with DSA tag modules), > triggering a userland "modprobe XXXX." Even without the variant module > loaded, we know the compatible string that matched the interface > driver. We can also have some extra info from match data, but I chose > to simply keep using the compatible string. The issue is how to get 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? Best regards, Krzysztof
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. Best regards, Krzysztof
> 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
On Wed, Nov 22, 2023 at 07:44:44PM -0300, Luiz Angelo Daros de Luca wrote: > 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). As you say below, this is too much for your test device. Skip. > 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. > 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: - 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 We already see what realtek_common_probe() etc. look like in your patch series here. If an interface driver needs to do something specific and it can't do that before or after calling realtek_common_probe(), then it can be abstracted away into some realtek_common_info struct and the interface driver can populate this and pass it along. The interface driver also ought to provide its own probe function that the chip variant driver can call. Again, if there is something specific that needs doing in the middle of this process it can call into some realtek_{smi,mdio}_info function pointer. I made similar points in my review of your last series so you can look there for specific instances where I think this is valuable. After that, the chip variant driver can do some #if ENABLED() logic to selectively register itself as either a platform driver (SMI), MDIO driver (MDIO), or both. The MODULE_DEVICE_TABLE is just used by the bus driver in question (platform or MDIO) to match with the appropriate driver. Compatible strings are the same, but it's the same chip after all. In fact, I don't think you even need two MODULE_DEVICE_TABLEs. You can pass the same one to both the mdio_driver and platform_driver structs. I think one needs to look critically at the data being passed around between the constituent modules at the moment. Some observations: - for MDIO, the only thing the chip variant driver needs to give it is a detect function and its dsa_switch_ops, so why not: struct realtek_mdio_info { int (*detect)(struct realtek_priv *priv); const struct dsa_switch_ops *ds_ops; } - for SMI, some more info is needed: struct realtek_smi_info { int (*detect)(struct realtek_priv *priv); int (*phy_read)(struct realtek_priv *priv, int phy, int regnum); int (*phy_write)(struct realtek_priv *priv, int phy, int regnum, u16 val); const struct dsa_switch_ops *ds_ops; } Yes, there is a lot more in struct realtek_ops, but all of that is used by rtl8366-core.c. I think this is trivial to disentangle but it might result in a fairly large diff if one tries to remove this from struct realtek_ops. My point being you don't need to actually populate this for the interface driver itself. With that we can define reasonable signatures for the interface drivers' probe functions: int realtek_smi_probe(struct platform_device *pdev, const struct realtek_smi_info *info); int realtek_mdio_probe(struct mdio_device *mdiodev, const struct realtek_mdio_info *info); EXPORT_SYMBOL_GPL(both of these functions); The remove functions can just take the pdev or mdiodev pointers to keep things balanced. Now the interface drivers can populate realtek_priv sufficiently for the realtek_common_probe() to do its job. struct realtek_interface_info { static const struct regmap_config *regmap_config; static const struct regmap_config *regmap_config_nolock; int (*detect)(struct realtek_priv *priv); int (*setup_interface)(struct dsa_switch *ds); const struct dsa_switch_ops *ds_ops; }; The regmap configs are self explanatory. 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. Now I recall that Vladimir suggested something more along the lines of realtek_common_probe_pre() and post, and ditto for remove(). You might prefer that approach. Something to experiment with. You might need something like that because there are those GPIOs that SMI wants to fiddle with, and they need to be ready before calling dsa_register_switch(), and the priv datastructure is not allocated before calling the common probe function. Anyway, with the above suggestion I think one can move the vast majority of common code into realtek-common while keeping the chip variant drivers mostly unmodified. All that's needed is to move the MODULE_DEVICE_TABLES into those files and create custom module init/exit functions which conditionally register themselves as platform and mdio drivers. This is what the module_platform_driver() and mdio_module_driver() macros do - they just make the boilerplate init/exit for you. But nothing stops you from registering both a struct platform_driver and a struct mdio_driver. Just guard everything with #ifs to ensure clean compilation. The relevant probe functions will look like this (rtl8365mb example): #if IS_ENABLED(CONFIG_NET_DSA_REALTEK_SMI) static const struct realtek_smi_info rtl8365mb_smi_info = { .detect = rtl8365mb_detect, .phy_read = rtl8365mb_phy_read, .phy_write = rtl8365mb_phy_write, .ds_ops = rtl8365mb_switch_ops, }; int rtl8365mb_smi_probe(struct platform_device *pdev) { return realtek_smi_probe(pdev, &rtl8365mb_smi_info); } #endif #if IS_ENABLED(CONFIG_NET_DSA_REALTEK_MDIO) static const struct realtek_mdio_info rtl8365mb_mdio_info = { .detect = rtl8365mb_detect, .ds_ops = rtl8365mb_switch_ops, }; int rtl8365mb_mdio_probe(struct mdio_device *mdiodev) { return realtek_mdio_probe(mdiodev, &rtl8365mb_mdio_info); } #endif One thing I dislike is that all the driver private data is getting allocated way up in the common driver. It would be nicer if the chip variant or interface drivers could also allocate their own stuff and just put it in a priv pointer of the parent. Likewise I was never a fan of the fact that the chip variant drivers always receiving realtek_priv pointers rather than pointers to their driver private data. You can see a lot of diving into chip_data especially in rtl8365mb for this reason. I regret not addressing that before I added the new driver. > > 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. Kind regards, Alvin
Hi Luiz,
kernel test robot noticed the following build warnings:
url: https://github.com/intel-lab-lkp/linux/commits/Luiz-Angelo-Daros-de-Luca/net-dsa-realtek-create-realtek-common/20231118-075444
base: net-next/main
patch link: https://lore.kernel.org/r/20231117235140.1178-3-luizluca%40gmail.com
patch subject: [net-next 2/2] net: dsa: realtek: load switch variants on demand
config: mips-randconfig-r081-20231121 (https://download.01.org/0day-ci/archive/20231125/202311251132.QKdGl71R-lkp@intel.com/config)
compiler: mips-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20231125/202311251132.QKdGl71R-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202311251132.QKdGl71R-lkp@intel.com/
smatch warnings:
drivers/net/dsa/realtek/realtek-smi.c:418 realtek_smi_probe() warn: passing zero to 'PTR_ERR'
vim +/PTR_ERR +418 drivers/net/dsa/realtek/realtek-smi.c
d8652956cf37c5 drivers/net/dsa/realtek-smi.c Linus Walleij 2018-07-14 398 static int realtek_smi_probe(struct platform_device *pdev)
d8652956cf37c5 drivers/net/dsa/realtek-smi.c Linus Walleij 2018-07-14 399 {
d8652956cf37c5 drivers/net/dsa/realtek-smi.c Linus Walleij 2018-07-14 400 struct device *dev = &pdev->dev;
f5f119077b1cd6 drivers/net/dsa/realtek/realtek-smi-core.c Luiz Angelo Daros de Luca 2022-01-28 401 struct realtek_priv *priv;
d8652956cf37c5 drivers/net/dsa/realtek-smi.c Linus Walleij 2018-07-14 402 int ret;
d8652956cf37c5 drivers/net/dsa/realtek-smi.c Linus Walleij 2018-07-14 403
217d45f6e61f5d drivers/net/dsa/realtek/realtek-smi.c Luiz Angelo Daros de Luca 2023-11-17 404 priv = realtek_common_probe(dev, realtek_smi_regmap_config,
217d45f6e61f5d drivers/net/dsa/realtek/realtek-smi.c Luiz Angelo Daros de Luca 2023-11-17 405 realtek_smi_nolock_regmap_config);
217d45f6e61f5d drivers/net/dsa/realtek/realtek-smi.c Luiz Angelo Daros de Luca 2023-11-17 406 if (IS_ERR(priv))
217d45f6e61f5d drivers/net/dsa/realtek/realtek-smi.c Luiz Angelo Daros de Luca 2023-11-17 407 return PTR_ERR(priv);
d8652956cf37c5 drivers/net/dsa/realtek-smi.c Linus Walleij 2018-07-14 408
d8652956cf37c5 drivers/net/dsa/realtek-smi.c Linus Walleij 2018-07-14 409 /* Fetch MDIO pins */
f5f119077b1cd6 drivers/net/dsa/realtek/realtek-smi-core.c Luiz Angelo Daros de Luca 2022-01-28 410 priv->mdc = devm_gpiod_get_optional(dev, "mdc", GPIOD_OUT_LOW);
7e61e799f3f92c drivers/net/dsa/realtek/realtek-smi.c Luiz Angelo Daros de Luca 2023-11-17 411 if (IS_ERR(priv->mdc)) {
7e61e799f3f92c drivers/net/dsa/realtek/realtek-smi.c Luiz Angelo Daros de Luca 2023-11-17 412 ret = PTR_ERR(priv->mdc);
7e61e799f3f92c drivers/net/dsa/realtek/realtek-smi.c Luiz Angelo Daros de Luca 2023-11-17 413 goto err_variant_put;
7e61e799f3f92c drivers/net/dsa/realtek/realtek-smi.c Luiz Angelo Daros de Luca 2023-11-17 414 }
217d45f6e61f5d drivers/net/dsa/realtek/realtek-smi.c Luiz Angelo Daros de Luca 2023-11-17 415
f5f119077b1cd6 drivers/net/dsa/realtek/realtek-smi-core.c Luiz Angelo Daros de Luca 2022-01-28 416 priv->mdio = devm_gpiod_get_optional(dev, "mdio", GPIOD_OUT_LOW);
7e61e799f3f92c drivers/net/dsa/realtek/realtek-smi.c Luiz Angelo Daros de Luca 2023-11-17 417 if (IS_ERR(priv->mdio)) {
7e61e799f3f92c drivers/net/dsa/realtek/realtek-smi.c Luiz Angelo Daros de Luca 2023-11-17 @418 ret = PTR_ERR(priv->mdc);
s/mdc/mdio/.
7e61e799f3f92c drivers/net/dsa/realtek/realtek-smi.c Luiz Angelo Daros de Luca 2023-11-17 419 goto err_variant_put;
7e61e799f3f92c drivers/net/dsa/realtek/realtek-smi.c Luiz Angelo Daros de Luca 2023-11-17 420 }
d8652956cf37c5 drivers/net/dsa/realtek-smi.c Linus Walleij 2018-07-14 421
217d45f6e61f5d drivers/net/dsa/realtek/realtek-smi.c Luiz Angelo Daros de Luca 2023-11-17 422 priv->setup_interface = realtek_smi_setup_mdio;
217d45f6e61f5d drivers/net/dsa/realtek/realtek-smi.c Luiz Angelo Daros de Luca 2023-11-17 423 priv->write_reg_noack = realtek_smi_write_reg_noack;
d8652956cf37c5 drivers/net/dsa/realtek-smi.c Linus Walleij 2018-07-14 424
f5f119077b1cd6 drivers/net/dsa/realtek/realtek-smi-core.c Luiz Angelo Daros de Luca 2022-01-28 425 ret = priv->ops->detect(priv);
d8652956cf37c5 drivers/net/dsa/realtek-smi.c Linus Walleij 2018-07-14 426 if (ret) {
d8652956cf37c5 drivers/net/dsa/realtek-smi.c Linus Walleij 2018-07-14 427 dev_err(dev, "unable to detect switch\n");
7e61e799f3f92c drivers/net/dsa/realtek/realtek-smi.c Luiz Angelo Daros de Luca 2023-11-17 428 goto err_variant_put;
d8652956cf37c5 drivers/net/dsa/realtek-smi.c Linus Walleij 2018-07-14 429 }
d8652956cf37c5 drivers/net/dsa/realtek-smi.c Linus Walleij 2018-07-14 430
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
On Mon, Nov 27, 2023 at 07:24:16PM -0300, Luiz Angelo Daros de Luca wrote: > 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. > > 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. I'm not an expert on this topic either, but Alvin's suggestion makes sense to have the switch variant drivers be both platform and MDIO device drivers, and call symbols exported by the interface drivers as needed. If you are able to make the variant driver depend on just the interface driver in use based on some request_module() calls, I don't think that will be a problem with Krzysztof either, since he just said to not duplicate the MODULE_DEVICE_TABLE() functionality. I think it's down to prototyping something and seeing what are the pros and cons.
On Mon, Nov 27, 2023 at 07:24:16PM -0300, Luiz Angelo Daros de Luca wrote: > 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. The compatible string is about picking a driver for a device. It is supposed to uniquely describe the register layout and functionality of that IP block, not its functional role in the kernel. "slave_mdio" and "user_mii" are too ingrained with "this MDIO controller gives access to internal PHY ports of DSA slave ports". Even if the MDIO controller doesn't currently have its own struct device and driver, you'd have to think of the fact that it could, when picking an appropriate compatible string. If you have very specific information that the MDIO controller is based on some reusable/licensable IP block and there were no modifications made to it, you could use that compatible string. Otherwise, another sensible choice would be "realtek,<precise-soc-name>-mdio", because it leaves room for future extensions with other compatible strings, more generic or just as specific, that all bind to the same driver. It's always good to start being too specific rather than too generic, because a future Realtek switch might have a different IP block for its MDIO controller. Then a driver for your existing "realtek,smi-mdio" or "realtek,slave_mdio" or "realtek,user_mii" compatible string sounds like it could handle it, but it can't. You can always add secondary compatible strings to a node, but changing the existing one breaks the ABI between the kernel and the DT.
> On Mon, Nov 27, 2023 at 07:24:16PM -0300, Luiz Angelo Daros de Luca wrote: > > 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. > > The compatible string is about picking a driver for a device. It is > supposed to uniquely describe the register layout and functionality of > that IP block, not its functional role in the kernel. "slave_mdio" and > "user_mii" are too ingrained with "this MDIO controller gives access to > internal PHY ports of DSA slave ports". > > Even if the MDIO controller doesn't currently have its own struct device > and driver, you'd have to think of the fact that it could, when picking > an appropriate compatible string. > > If you have very specific information that the MDIO controller is based on > some reusable/licensable IP block and there were no modifications made > to it, you could use that compatible string. > > Otherwise, another sensible choice would be "realtek,<precise-soc-name>-mdio", > because it leaves room for future extensions with other compatible > strings, more generic or just as specific, that all bind to the same > driver. > > It's always good to start being too specific rather than too generic, > because a future Realtek switch might have a different IP block for its > MDIO controller. Then a driver for your existing "realtek,smi-mdio" or > "realtek,slave_mdio" or "realtek,user_mii" compatible string sounds like > it could handle it, but it can't. > > You can always add secondary compatible strings to a node, but changing > the existing one breaks the ABI between the kernel and the DT. HI Vladmir, We discussed something about that in the past: https://lkml.kernel.org/netdev/20220630200423.tieprdu5fpabflj7@bang-olufsen.dk/T/#m04e6cf7d0c1f18b02c2bf40266ada915b1f02f3d The code is able to handle only a single node and binding docs say it should be named "mdio". The compatible string wasn't a requirement since the beginning and I don't think it is worth it to rename the compatible string. I suggest we simply switch to of_get_child_by_name() and look for a node named "mdio". If that node is not found, we can still look for the old compatible string (backwards compatibility) and probably warn the "user" (targeting not the end-user but the one creating the DT for a new device). I don't know how to handle the binding docs as the compatible string is still a requirement for older kernel versions. Is it ok to update the device-tree bindings docs in such a way it would break old drivers? Or should we keep it there until the last LTS kernel requiring it reaches EOL? As device-tree bindings docs should not consider how the driver was implemented, I think it would be strange to have a note like "required by kernel up to 6.x". Regards, Luiz
Hi Vladimir, > > 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. > > > > 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. > > I'm not an expert on this topic either, but Alvin's suggestion makes > sense to have the switch variant drivers be both platform and MDIO > device drivers, and call symbols exported by the interface drivers as > needed. Yes, it does. It looks like the driver was upside down. > If you are able to make the variant driver depend on just the interface > driver in use based on some request_module() calls, I don't think that > will be a problem with Krzysztof either, since he just said to not > duplicate the MODULE_DEVICE_TABLE() functionality. The interface modules are quite small, multiple times smaller than the variant module. It wasn't worth it to load them on demand as the code to handle that might be close to the interface module size. Indeed, as we'll have a common module, I think the best solution would be to merge both interfaces into the common module. It would make things much simpler: two variant/families modules that require a single common module. It is also closer to what we see in other DSA drivers. > I think it's down to prototyping something and seeing what are the pros > and cons. I already did that and I'm finishing some tests before submitting it. It looks like it fits nicely. I avoided some struct refactoring Alvim suggested to keep the change as small as possible but I went a little further migrating the user mdio driver to common and use it for both interfaces. Regards, Luiz
On Thu, Dec 07, 2023 at 04:50:12PM -0300, Luiz Angelo Daros de Luca wrote: > HI Vladmir, > > We discussed something about that in the past: > > https://lkml.kernel.org/netdev/20220630200423.tieprdu5fpabflj7@bang-olufsen.dk/T/#m04e6cf7d0c1f18b02c2bf40266ada915b1f02f3d > > The code is able to handle only a single node and binding docs say it > should be named "mdio". The compatible string wasn't a requirement > since the beginning and I don't think it is worth it to rename the > compatible string. I suggest we simply switch to > of_get_child_by_name() and look for a node named "mdio". If that node > is not found, we can still look for the old compatible string > (backwards compatibility) and probably warn the "user" (targeting not > the end-user but the one creating the DT for a new device). > > I don't know how to handle the binding docs as the compatible string > is still a requirement for older kernel versions. Is it ok to update > the device-tree bindings docs in such a way it would break old > drivers? Or should we keep it there until the last LTS kernel > requiring it reaches EOL? As device-tree bindings docs should not > consider how the driver was implemented, I think it would be strange > to have a note like "required by kernel up to 6.x". > > Regards, > > Luiz And did you ever answer this question? "And why do you even need to remove the compatible string from the MDIO node, can't you just ignore it, does it bother you in any way?" I'm very confused as to what you're after.
> > We discussed something about that in the past: > > > > https://lkml.kernel.org/netdev/20220630200423.tieprdu5fpabflj7@bang-olufsen.dk/T/#m04e6cf7d0c1f18b02c2bf40266ada915b1f02f3d > > > > The code is able to handle only a single node and binding docs say it > > should be named "mdio". The compatible string wasn't a requirement > > since the beginning and I don't think it is worth it to rename the > > compatible string. I suggest we simply switch to > > of_get_child_by_name() and look for a node named "mdio". If that node > > is not found, we can still look for the old compatible string > > (backwards compatibility) and probably warn the "user" (targeting not > > the end-user but the one creating the DT for a new device). > > > > I don't know how to handle the binding docs as the compatible string > > is still a requirement for older kernel versions. Is it ok to update > > the device-tree bindings docs in such a way it would break old > > drivers? Or should we keep it there until the last LTS kernel > > requiring it reaches EOL? As device-tree bindings docs should not > > consider how the driver was implemented, I think it would be strange > > to have a note like "required by kernel up to 6.x". > > > > Regards, > > > > Luiz > > And did you ever answer this question? > > "And why do you even need to remove the compatible string from the MDIO > node, can't you just ignore it, does it bother you in any way?" > > I'm very confused as to what you're after. The device-tree bindings should delineate the hardware characteristics rather than specifying the implementation details of a particular driver. The requirement of an "mdio" node with a compatible string such as "realtek,smi-mdio" may be misleading, implying a potential correlation between the host-switch interface (SMI, SPI, or MDIO) and a specific user MDIO it describes. It's important to note that how we describe the user mdio could vary for other future switch families, but not with a distinct management interface. I am currently conducting tests using the same user MDIO driver for both realtek-smi and realtek-mdio. However, it's noteworthy that unlike realtek-smi, the current user MDIO for realtek-mdio does not require a compatible string; only a node named "mdio". Realtek-mdio is presently utilizing the generic DSA user MDIO, but you mentioned it's not considered a "core functionality." I assume this implies I shouldn't depend on it. That's the reason for my switch to the existing user MDIO driver from realtek-smi. Regarding the absence of a compatible string for realtek-mdio, we have a few options: introducing a new compatible string exclusively for realtek-mdio, such as "realtek,mdio-mdio"; creating a new generic one for both interfaces like "realtek,user-mdio" or "rtl836x-user-mdio"; or simply ignore the compatible string, as you suggested. However, if I opt to ignore it, I presume I should retrieve that node solely based on the node name. That's what I'm after. Is my understanding correct? I'll post a new series that is still compatible both with old HW descriptions and the device-tree bindings. In that way, I'll not touch the docs. However, given that the compatible string is unnecessary to describe the hardware, and after we modify the code to disregard it, it is awkward for the binding documentation to request a compatible string that serves no purpose. Shouldn't we consider updating this requirement at some point? Regards, Luiz
On Thu, Dec 07, 2023 at 11:46:46PM -0300, Luiz Angelo Daros de Luca wrote: > The device-tree bindings should delineate the hardware characteristics > rather than specifying the implementation details of a particular > driver. The requirement of an "mdio" node with a compatible string > such as "realtek,smi-mdio" may be misleading, implying a potential > correlation between the host-switch interface (SMI, SPI, or MDIO) and > a specific user MDIO it describes. It's important to note that how we > describe the user mdio could vary for other future switch families, > but not with a distinct management interface. Agree, "realtek,smi-mdio" is not a great compatible string. But it is an established compatible string. > I am currently conducting tests using the same user MDIO driver for > both realtek-smi and realtek-mdio. However, it's noteworthy that > unlike realtek-smi, the current user MDIO for realtek-mdio does not > require a compatible string; only a node named "mdio". Realtek-mdio is > presently utilizing the generic DSA user MDIO, but you mentioned it's > not considered a "core functionality." I assume this implies I > shouldn't depend on it. That's the reason for my switch to the > existing user MDIO driver from realtek-smi. > > Regarding the absence of a compatible string for realtek-mdio, we have > a few options: introducing a new compatible string exclusively for > realtek-mdio, such as "realtek,mdio-mdio"; creating a new generic one > for both interfaces like "realtek,user-mdio" or "rtl836x-user-mdio"; The naming choice really looks like the secondary problem here. But what about "realtek,rtl8365mb-mdio" and "realtek,rtl8366rb-mdio" as a secondary compatible string for SMI, and primary compatible string for MDIO-connected switches? > or simply ignore the compatible string, as you suggested. However, if > I opt to ignore it, I presume I should retrieve that node solely based > on the node name. That's what I'm after. Is my understanding correct? You could do that. There's a very high chance that the node was named "mdio". The schema says it should be called "mdio", _and_ be compatible with realtek,smi-mdio. If anyone comes and complains (very unlikely), just say "hey, even Documentation/devicetree/bindings/net/dsa/realtek-smi.txt said you should name the node 'mdio'"! > I'll post a new series that is still compatible both with old HW > descriptions and the device-tree bindings. In that way, I'll not touch > the docs. > However, given that the compatible string is unnecessary to describe > the hardware, and after we modify the code to disregard it, it is > awkward for the binding documentation to request a compatible string > that serves no purpose. Shouldn't we consider updating this > requirement at some point? > > Regards, > > Luiz Not everything that is in the device tree has to be used. It is a description of the hardware, not a rigid set of instructions for what the OS has to do. The OS still does whatever it wants based on that info. You can ask anyone about this. See Thomas Petazzoni's slides as just one example. https://elinux.org/images/f/f9/Petazzoni-device-tree-dummies_0.pdf | The Device Tree is really a hardware description language. | It should describe the hardware layout, and how it works. | But it should not describe which particular hardware configuration you’re interested in. | As an example: | You may describe in the DT whether a particular piece of hardware supports DMA or not. | But you may not describe in the DT if you want to use DMA or not. It's a really weak argument for recommending users to remove the compatible string, thereby deliberately breaking ABI compatibility in one direction, to literally _no_ benefit. Compatible strings for MDIO controllers are, in essence, not strange. They are independent devices which could reasonably be bound to their own drivers. I don't see what's awkward about this.
diff --git a/drivers/net/dsa/realtek/realtek-common.c b/drivers/net/dsa/realtek/realtek-common.c index 1b733ac56560..cdd8d77c20d9 100644 --- a/drivers/net/dsa/realtek/realtek-common.c +++ b/drivers/net/dsa/realtek/realtek-common.c @@ -1,10 +1,72 @@ // SPDX-License-Identifier: GPL-2.0+ #include <linux/module.h> +#include <linux/of_device.h> #include "realtek.h" #include "realtek-common.h" +static LIST_HEAD(realtek_variants_list); +static DEFINE_MUTEX(realtek_variants_lock); + +void realtek_variant_register(struct realtek_variant_entry *variant_entry) +{ + mutex_lock(&realtek_variants_lock); + list_add_tail(&variant_entry->list, &realtek_variants_list); + mutex_unlock(&realtek_variants_lock); +} +EXPORT_SYMBOL_GPL(realtek_variant_register); + +void realtek_variant_unregister(struct realtek_variant_entry *variant_entry) +{ + mutex_lock(&realtek_variants_lock); + list_del(&variant_entry->list); + mutex_unlock(&realtek_variants_lock); +} +EXPORT_SYMBOL_GPL(realtek_variant_unregister); + +const struct realtek_variant *realtek_variant_get(const char *compatible) +{ + const struct realtek_variant *variant = ERR_PTR(-ENOENT); + struct realtek_variant_entry *variant_entry; + + request_module(compatible); + + mutex_lock(&realtek_variants_lock); + list_for_each_entry(variant_entry, &realtek_variants_list, list) { + if (strcmp(compatible, variant_entry->compatible)) + continue; + + if (!try_module_get(variant_entry->owner)) + break; + + variant = variant_entry->variant; + break; + } + mutex_unlock(&realtek_variants_lock); + + return variant; +} +EXPORT_SYMBOL_GPL(realtek_variant_get); + +void realtek_variant_put(const struct realtek_variant *var) +{ + struct realtek_variant_entry *variant_entry; + + mutex_lock(&realtek_variants_lock); + list_for_each_entry(variant_entry, &realtek_variants_list, list) { + if (variant_entry->variant != var) + continue; + + if (variant_entry->owner) + module_put(variant_entry->owner); + + break; + } + mutex_unlock(&realtek_variants_lock); +} +EXPORT_SYMBOL_GPL(realtek_variant_put); + void realtek_common_lock(void *ctx) { struct realtek_priv *priv = ctx; @@ -25,18 +87,30 @@ struct realtek_priv *realtek_common_probe(struct device *dev, struct regmap_config rc, struct regmap_config rc_nolock) { const struct realtek_variant *var; + const struct of_device_id *match; struct realtek_priv *priv; struct device_node *np; int ret; - var = of_device_get_match_data(dev); - if (!var) + match = of_match_device(dev->driver->of_match_table, dev); + if (!match) return ERR_PTR(-EINVAL); + var = realtek_variant_get(match->compatible); + if (IS_ERR(var)) { + ret = PTR_ERR(var); + dev_err_probe(dev, ret, + "failed to get module for alias '%s'", + match->compatible); + goto err_variant_put; + } + priv = devm_kzalloc(dev, size_add(sizeof(*priv), var->chip_data_sz), GFP_KERNEL); - if (!priv) - return ERR_PTR(-ENOMEM); + if (!priv) { + ret = -ENOMEM; + goto err_variant_put; + } mutex_init(&priv->map_lock); @@ -45,14 +119,14 @@ struct realtek_priv *realtek_common_probe(struct device *dev, if (IS_ERR(priv->map)) { ret = PTR_ERR(priv->map); dev_err(dev, "regmap init failed: %d\n", ret); - return ERR_PTR(ret); + goto err_variant_put; } priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc_nolock); if (IS_ERR(priv->map_nolock)) { ret = PTR_ERR(priv->map_nolock); dev_err(dev, "regmap init failed: %d\n", ret); - return ERR_PTR(ret); + goto err_variant_put; } /* Link forward and backward */ @@ -69,11 +143,11 @@ struct realtek_priv *realtek_common_probe(struct device *dev, priv->leds_disabled = of_property_read_bool(np, "realtek,disable-leds"); /* TODO: if power is software controlled, set up any regulators here */ - priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); if (IS_ERR(priv->reset)) { + ret = PTR_ERR(priv->reset); dev_err(dev, "failed to get RESET GPIO\n"); - return ERR_CAST(priv->reset); + goto err_variant_put; } if (priv->reset) { gpiod_set_value(priv->reset, 1); @@ -85,13 +159,20 @@ struct realtek_priv *realtek_common_probe(struct device *dev, } priv->ds = devm_kzalloc(dev, sizeof(*priv->ds), GFP_KERNEL); - if (!priv->ds) - return ERR_PTR(-ENOMEM); + if (!priv->ds) { + ret = -ENOMEM; + goto err_variant_put; + } priv->ds->dev = dev; priv->ds->priv = priv; return priv; + +err_variant_put: + realtek_variant_put(var); + + return ERR_PTR(ret); } EXPORT_SYMBOL(realtek_common_probe); @@ -104,6 +185,8 @@ void realtek_common_remove(struct realtek_priv *priv) if (priv->user_mii_bus) of_node_put(priv->user_mii_bus->dev.of_node); + realtek_variant_put(priv->variant); + /* leave the device reset asserted */ if (priv->reset) gpiod_set_value(priv->reset, 1); @@ -112,10 +195,10 @@ EXPORT_SYMBOL(realtek_common_remove); const struct of_device_id realtek_common_of_match[] = { #if IS_ENABLED(CONFIG_NET_DSA_REALTEK_RTL8366RB) - { .compatible = "realtek,rtl8366rb", .data = &rtl8366rb_variant, }, + { .compatible = "realtek,rtl8366rb", }, #endif #if IS_ENABLED(CONFIG_NET_DSA_REALTEK_RTL8365MB) - { .compatible = "realtek,rtl8365mb", .data = &rtl8365mb_variant, }, + { .compatible = "realtek,rtl8365mb", }, #endif { /* sentinel */ }, }; diff --git a/drivers/net/dsa/realtek/realtek-common.h b/drivers/net/dsa/realtek/realtek-common.h index 90a949386518..6de4991d8b5c 100644 --- a/drivers/net/dsa/realtek/realtek-common.h +++ b/drivers/net/dsa/realtek/realtek-common.h @@ -5,6 +5,37 @@ #include <linux/regmap.h> +struct realtek_variant_entry { + const struct realtek_variant *variant; + const char *compatible; + struct module *owner; + struct list_head list; +}; + +#define module_realtek_variant(__variant, __compatible) \ +static struct realtek_variant_entry __variant ## _entry = { \ + .compatible = __compatible, \ + .variant = &(__variant), \ + .owner = THIS_MODULE, \ +}; \ +static int __init realtek_variant_module_init(void) \ +{ \ + realtek_variant_register(&__variant ## _entry); \ + return 0; \ +} \ +module_init(realtek_variant_module_init) \ + \ +static void __exit realtek_variant_module_exit(void) \ +{ \ + realtek_variant_unregister(&__variant ## _entry); \ +} \ +module_exit(realtek_variant_module_exit); \ + \ +MODULE_ALIAS(__compatible) + +void realtek_variant_register(struct realtek_variant_entry *variant_entry); +void realtek_variant_unregister(struct realtek_variant_entry *variant_entry); + extern const struct of_device_id realtek_common_of_match[]; void realtek_common_lock(void *ctx); @@ -12,5 +43,7 @@ void realtek_common_unlock(void *ctx); struct realtek_priv *realtek_common_probe(struct device *dev, struct regmap_config rc, struct regmap_config rc_nolock); void realtek_common_remove(struct realtek_priv *priv); +const struct realtek_variant *realtek_variant_get(const char *compatible); +void realtek_variant_put(const struct realtek_variant *var); #endif diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c index b865e11955ca..c447dd815a59 100644 --- a/drivers/net/dsa/realtek/realtek-mdio.c +++ b/drivers/net/dsa/realtek/realtek-mdio.c @@ -145,7 +145,7 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev) ret = priv->ops->detect(priv); if (ret) { dev_err(dev, "unable to detect switch\n"); - return ret; + goto err_variant_put; } priv->ds->ops = priv->variant->ds_ops_mdio; @@ -154,10 +154,15 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev) ret = dsa_register_switch(priv->ds); if (ret) { dev_err_probe(dev, ret, "unable to register switch\n"); - return ret; + goto err_variant_put; } return 0; + +err_variant_put: + realtek_variant_put(priv->variant); + + return ret; } static void realtek_mdio_remove(struct mdio_device *mdiodev) diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c index 2aebcbe0425f..e50b3c6203e6 100644 --- a/drivers/net/dsa/realtek/realtek-smi.c +++ b/drivers/net/dsa/realtek/realtek-smi.c @@ -408,12 +408,16 @@ static int realtek_smi_probe(struct platform_device *pdev) /* Fetch MDIO pins */ priv->mdc = devm_gpiod_get_optional(dev, "mdc", GPIOD_OUT_LOW); - if (IS_ERR(priv->mdc)) - return PTR_ERR(priv->mdc); + if (IS_ERR(priv->mdc)) { + ret = PTR_ERR(priv->mdc); + goto err_variant_put; + } priv->mdio = devm_gpiod_get_optional(dev, "mdio", GPIOD_OUT_LOW); - if (IS_ERR(priv->mdio)) - return PTR_ERR(priv->mdio); + if (IS_ERR(priv->mdio)) { + ret = PTR_ERR(priv->mdc); + goto err_variant_put; + } priv->setup_interface = realtek_smi_setup_mdio; priv->write_reg_noack = realtek_smi_write_reg_noack; @@ -421,7 +425,7 @@ static int realtek_smi_probe(struct platform_device *pdev) ret = priv->ops->detect(priv); if (ret) { dev_err(dev, "unable to detect switch\n"); - return ret; + goto err_variant_put; } priv->ds->ops = priv->variant->ds_ops_smi; @@ -430,10 +434,15 @@ static int realtek_smi_probe(struct platform_device *pdev) ret = dsa_register_switch(priv->ds); if (ret) { dev_err_probe(dev, ret, "unable to register switch\n"); - return ret; + goto err_variant_put; } return 0; + +err_variant_put: + realtek_variant_put(priv->variant); + + return ret; } static void realtek_smi_remove(struct platform_device *pdev) diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h index fbbdf538908e..267a1dc02080 100644 --- a/drivers/net/dsa/realtek/realtek.h +++ b/drivers/net/dsa/realtek/realtek.h @@ -143,7 +143,4 @@ void rtl8366_get_strings(struct dsa_switch *ds, int port, u32 stringset, int rtl8366_get_sset_count(struct dsa_switch *ds, int port, int sset); void rtl8366_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *data); -extern const struct realtek_variant rtl8366rb_variant; -extern const struct realtek_variant rtl8365mb_variant; - #endif /* _REALTEK_H */ diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c index 9b18774e988c..fb214dd717f0 100644 --- a/drivers/net/dsa/realtek/rtl8365mb.c +++ b/drivers/net/dsa/realtek/rtl8365mb.c @@ -2164,7 +2164,7 @@ static const struct realtek_ops rtl8365mb_ops = { .phy_write = rtl8365mb_phy_write, }; -const struct realtek_variant rtl8365mb_variant = { +static const struct realtek_variant rtl8365mb_variant = { .ds_ops_smi = &rtl8365mb_switch_ops_smi, .ds_ops_mdio = &rtl8365mb_switch_ops_mdio, .ops = &rtl8365mb_ops, @@ -2173,7 +2173,7 @@ const struct realtek_variant rtl8365mb_variant = { .cmd_write = 0xb8, .chip_data_sz = sizeof(struct rtl8365mb), }; -EXPORT_SYMBOL_GPL(rtl8365mb_variant); +module_realtek_variant(rtl8365mb_variant, "realtek,rtl8365mb"); MODULE_AUTHOR("Alvin Šipraga <alsi@bang-olufsen.dk>"); MODULE_DESCRIPTION("Driver for RTL8365MB-VC ethernet switch"); diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c index 1ac2fd098242..143c57c69ace 100644 --- a/drivers/net/dsa/realtek/rtl8366rb.c +++ b/drivers/net/dsa/realtek/rtl8366rb.c @@ -1912,7 +1912,7 @@ static const struct realtek_ops rtl8366rb_ops = { .phy_write = rtl8366rb_phy_write, }; -const struct realtek_variant rtl8366rb_variant = { +static const struct realtek_variant rtl8366rb_variant = { .ds_ops_smi = &rtl8366rb_switch_ops_smi, .ds_ops_mdio = &rtl8366rb_switch_ops_mdio, .ops = &rtl8366rb_ops, @@ -1921,7 +1921,7 @@ const struct realtek_variant rtl8366rb_variant = { .cmd_write = 0xa8, .chip_data_sz = sizeof(struct rtl8366rb), }; -EXPORT_SYMBOL_GPL(rtl8366rb_variant); +module_realtek_variant(rtl8366rb_variant, "realtek,rtl8366rb"); MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>"); MODULE_DESCRIPTION("Driver for RTL8366RB ethernet switch");
realtek-common had a hard dependency on both switch variants. As a result, it was not possible to selectively load only one model at runtime. Now, variants are registered in the realtek-common module, and interface modules look for a variant using the compatible string. The variant modules use the same compatible string as the module alias. This way, an interface module can use the matching compatible string to both load the module and get the variant reference. Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> --- drivers/net/dsa/realtek/realtek-common.c | 107 ++++++++++++++++++++--- drivers/net/dsa/realtek/realtek-common.h | 33 +++++++ drivers/net/dsa/realtek/realtek-mdio.c | 9 +- drivers/net/dsa/realtek/realtek-smi.c | 21 +++-- drivers/net/dsa/realtek/realtek.h | 3 - drivers/net/dsa/realtek/rtl8365mb.c | 4 +- drivers/net/dsa/realtek/rtl8366rb.c | 4 +- 7 files changed, 154 insertions(+), 27 deletions(-)