Message-ID: <qaijsywlvxewssd2nxsfowvlzrua3ipgu3l7iesdq3lci7upd7@t73e2tsc3mhy>
Date: Tue, 14 Nov 2023 12:17:23 +0000
From: Alvin Šipraga <ALSI@...g-olufsen.dk>
To: Luiz Angelo Daros de Luca <luizluca@...il.com>
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... ;)

> +	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.

> +
> +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.

Powered by blists - more mailing lists