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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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