| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJq09z6319jY6oB_t09fBjhEhWxyoV4To7hESyPm=GkSH1KO0w@mail.gmail.com>
Date: Fri, 17 Nov 2023 18:14:19 -0300
From: Luiz Angelo Daros de Luca <luizluca@...il.com>
To: Alvin Šipraga <ALSI@...g-olufsen.dk>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linus.walleij@...aro.org" <linus.walleij@...aro.org>, "andrew@...n.ch" <andrew@...n.ch>,
"vivien.didelot@...il.com" <vivien.didelot@...il.com>, "f.fainelli@...il.com" <f.fainelli@...il.com>,
"olteanv@...il.com" <olteanv@...il.com>, "davem@...emloft.net" <davem@...emloft.net>,
"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
"robh+dt@...nel.org" <robh+dt@...nel.org>, "krzk+dt@...nel.org" <krzk+dt@...nel.org>,
"arinc.unal@...nc9.com" <arinc.unal@...nc9.com>
Subject: Re: [RFC net-next 4/5] net: dsa: realtek: load switch variants on demand
> On Sat, Nov 11, 2023 at 06:51:07PM -0300, Luiz Angelo Daros de Luca wrote:
> > realtek-common had a hard dependency on both switch variants. That way,
> > it was not possible to selectively load only one model at runtime. Now
> > variants are registered at the realtek-common module and interface
> > modules look for a variant using the compatible string.
> >
> > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@...il.com>
> > ---
> > drivers/net/dsa/realtek/realtek-common.c | 125 ++++++++++++++++++++---
> > drivers/net/dsa/realtek/realtek-common.h | 3 +
> > drivers/net/dsa/realtek/realtek-mdio.c | 9 +-
> > drivers/net/dsa/realtek/realtek-smi.c | 9 +-
> > drivers/net/dsa/realtek/realtek.h | 36 ++++++-
> > drivers/net/dsa/realtek/rtl8365mb.c | 4 +-
> > drivers/net/dsa/realtek/rtl8366rb.c | 4 +-
> > 7 files changed, 162 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/net/dsa/realtek/realtek-common.c b/drivers/net/dsa/realtek/realtek-common.c
> > index 36f8b60771be..e383db21c776 100644
> > --- a/drivers/net/dsa/realtek/realtek-common.c
> > +++ b/drivers/net/dsa/realtek/realtek-common.c
> > @@ -1,10 +1,76 @@
> > // 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 *var_ent)
> > +{
> > + mutex_lock(&realtek_variants_lock);
> > + list_add_tail(&var_ent->list, &realtek_variants_list);
> > + mutex_unlock(&realtek_variants_lock);
> > +}
> > +EXPORT_SYMBOL_GPL(realtek_variant_register);
> > +
> > +void realtek_variant_unregister(struct realtek_variant_entry *var_ent)
> > +{
> > + mutex_lock(&realtek_variants_lock);
> > + list_del(&var_ent->list);
> > + mutex_unlock(&realtek_variants_lock);
> > +}
> > +EXPORT_SYMBOL_GPL(realtek_variant_unregister);
> > +
> > +const struct realtek_variant *realtek_variant_get(
> > + const struct of_device_id *match)
> > +{
> > + const struct realtek_variant *var = ERR_PTR(-ENOENT);
> > + struct realtek_variant_entry *var_ent;
> > + const char *modname = match->data;
> > +
> > + request_module(modname);
> > +
> > + mutex_lock(&realtek_variants_lock);
> > + list_for_each_entry(var_ent, &realtek_variants_list, list) {
> > + const struct realtek_variant *tmp = var_ent->variant;
> > +
> > + if (strcmp(match->compatible, var_ent->compatible))
> > + continue;
> > +
> > + if (!try_module_get(var_ent->owner))
> > + break;
> > +
> > + var = tmp;
> > + break;
> > + }
>
> Why not:
>
> list_for_each_entry(...) {
> if (strcmp(...))
> continue;
>
> if (!try_module_get(...))
> break;
>
> var = var_ent->variant;
> break;
> }
>
> no need for tmp.
>
> Maybe also rename var to variant? tmp, var, foo, etc. have somewhat throw-away
> variable connotations... ;)
You are right. That tmp came from dsa tag.c. At first, it had some use
as the match was using fields from the variant. However, once I opted
to use the compatible string, variant became just the result.
> > + mutex_unlock(&realtek_variants_lock);
> > +
> > + return var;
> > +}
> > +EXPORT_SYMBOL_GPL(realtek_variant_get);
> > +
> > +void realtek_variant_put(const struct realtek_variant *var)
> > +{
> > + struct realtek_variant_entry *var_ent;
> > +
> > + mutex_lock(&realtek_variants_lock);
> > + list_for_each_entry(var_ent, &realtek_variants_list, list) {
> > + if (var_ent->variant != var)
> > + continue;
> > +
> > + if (var_ent->owner)
> > + module_put(var_ent->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 +91,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 || !match->data)
> > return ERR_PTR(-EINVAL);
> >
> > + var = realtek_variant_get(match);
> > + if (IS_ERR(var)) {
> > + ret = PTR_ERR(var);
> > + dev_err_probe(dev, ret,
> > + "failed to get module for '%s'. Is '%s' loaded?",
> > + match->compatible, match->data);
> > + 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 +123,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,23 +147,27 @@ struct realtek_priv *realtek_common_probe(struct device *dev,
> >
> > /* Fetch MDIO pins */
> > priv->mdc = devm_gpiod_get_optional(dev, "mdc", GPIOD_OUT_LOW);
> > - if (IS_ERR(priv->mdc))
> > - return ERR_CAST(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 ERR_CAST(priv->mdio);
> > + if (IS_ERR(priv->mdio)) {
> > + ret = PTR_ERR(priv->mdio);
> > + goto err_variant_put;
> > + }
> >
> > np = dev->of_node;
> > -
> > 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);
> > @@ -97,13 +179,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);
> >
> > @@ -116,6 +205,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);
> > @@ -124,10 +215,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", .data = REALTEK_RTL8366RB_MODNAME, },
> > #endif
> > #if IS_ENABLED(CONFIG_NET_DSA_REALTEK_RTL8365MB)
> > - { .compatible = "realtek,rtl8365mb", .data = &rtl8366rb_variant, },
> > + { .compatible = "realtek,rtl8365mb", .data = REALTEK_RTL8365MB_MODNAME, },
> > #endif
> > { /* sentinel */ },
> > };
> > diff --git a/drivers/net/dsa/realtek/realtek-common.h b/drivers/net/dsa/realtek/realtek-common.h
> > index 90a949386518..089fda2d4fa9 100644
> > --- a/drivers/net/dsa/realtek/realtek-common.h
> > +++ b/drivers/net/dsa/realtek/realtek-common.h
> > @@ -12,5 +12,8 @@ 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 struct of_device_id *match);
> > +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 6f610386c977..6d81cd88dbe6 100644
> > --- a/drivers/net/dsa/realtek/realtek-mdio.c
> > +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> > @@ -146,7 +146,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->num_ports = priv->num_ports;
> > @@ -155,10 +155,15 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev)
> > if (ret) {
> > dev_err_probe(dev, ret, "unable to register switch ret = %pe\n",
> > ERR_PTR(ret));
> > - 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 0cf89f9db99e..a772bb7dbe35 100644
> > --- a/drivers/net/dsa/realtek/realtek-smi.c
> > +++ b/drivers/net/dsa/realtek/realtek-smi.c
> > @@ -413,7 +413,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->num_ports = priv->num_ports;
> > @@ -422,10 +422,15 @@ static int realtek_smi_probe(struct platform_device *pdev)
> > if (ret) {
> > dev_err_probe(dev, ret, "unable to register switch ret = %pe\n",
> > ERR_PTR(ret));
> > - 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 8d9d546bf5f5..f9bd6678e3bd 100644
> > --- a/drivers/net/dsa/realtek/realtek.h
> > +++ b/drivers/net/dsa/realtek/realtek.h
> > @@ -16,6 +16,38 @@
> > #define REALTEK_HW_STOP_DELAY 25 /* msecs */
> > #define REALTEK_HW_START_DELAY 100 /* msecs */
> >
> > +#define REALTEK_RTL8365MB_MODNAME "rtl8365mb"
> > +#define REALTEK_RTL8366RB_MODNAME "rtl8366"
>
> The solution is a little baroque but I see the benefit. This however seems
> rather brittle. I don't have a good idea of how to improve this but maybe
> somebody else will chime in.
I need some way to map "this CHIP requires module XXX" in order to let
it request the module. DSA tags, for example, have a module format
based on the tag name. Here, rtl8365mb matches the compatible string
but rtl8366rb doesn't. We discussed in the past to keep a single
compatible string for each driver when we dropped the "rtl8367s"
string. We could migrate the rtl8366-core to realtek-common and rename
the module from rtl8366 to rtl8366rb.
Anyway, I'll try another solution for now. How about
MODULE_ALIAS("realtek,rtl8365mb")? It seems to work nicely.
> > +
> > +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)
> > +
> > +void realtek_variant_register(struct realtek_variant_entry *var_ent);
> > +void realtek_variant_unregister(struct realtek_variant_entry *var_ent);
> > +
> > struct realtek_ops;
> > struct dentry;
> > struct inode;
> > @@ -120,6 +152,7 @@ struct realtek_ops {
> > struct realtek_variant {
> > const struct dsa_switch_ops *ds_ops_smi;
> > const struct dsa_switch_ops *ds_ops_mdio;
> > + const struct realtek_variant_info *info;
>
> Unused member variable.
Removed.
Thanks Alvin, I might send a new series with 3/5 and 4/5 soon with the
changes after some more tests.
Regards,
Luiz
Powered by blists - more mailing lists