| 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: <CAJq09z7deOf1qt+4eK5d3ZX0oGjwgn0VGnCeHbLtin3oDFvxiw@mail.gmail.com>
Date: Thu, 11 Jan 2024 23:15:56 -0300
From: Luiz Angelo Daros de Luca <luizluca@...il.com>
To: Vladimir Oltean <olteanv@...il.com>
Cc: netdev@...r.kernel.org, linus.walleij@...aro.org, alsi@...g-olufsen.dk,
andrew@...n.ch, f.fainelli@...il.com, davem@...emloft.net,
edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
arinc.unal@...nc9.com
Subject: Re: [PATCH net-next v3 3/8] net: dsa: realtek: common realtek-dsa module
Em qui., 11 de jan. de 2024 às 06:41, Vladimir Oltean
<olteanv@...il.com> escreveu:
>
> On Thu, Jan 11, 2024 at 03:20:10AM -0300, Luiz Angelo Daros de Luca wrote:
> > IMHO, the constant regmap_config looks cleaner than a sequence of
> > assignments. However, we don't actually need 4 of them.
> > As we already have a writable regmap_config in stack (to assign
> > lock_arg), we can reuse the same struct and simply set
> > disable_locking.
> > It makes the regmap ignore all locking fields and we don't even need
> > to unset them for map_nolock. Something like this:
> >
> > realtek_common_probe(struct device *dev, const struct regmap_config *rc_base)
> > {
> >
> > (...)
> >
> > rc = *rc_base;
> > rc.lock_arg = priv;
> > priv->map = devm_regmap_init(dev, NULL, priv, &rc);
> > if (IS_ERR(priv->map)) {
> > ret = PTR_ERR(priv->map);
> > dev_err(dev, "regmap init failed: %d\n", ret);
> > return ERR_PTR(ret);
> > }
> >
> > rc.disable_locking = true;
> > priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc);
> > 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);
> > }
> >
> > It has a cleaner function signature and we can remove the _nolock
> > constants as well.
> >
> > The regmap configs still have some room for improvement, like moving
> > from interfaces to variants, but this series is already too big. We
> > can leave that as it is.
>
> I was thinking something like this, does it look bad?
>
> From 2e462507171ed0fd8393598842dc0f7e6c50d499 Mon Sep 17 00:00:00 2001
> From: Vladimir Oltean <vladimir.oltean@....com>
> Date: Thu, 11 Jan 2024 11:38:17 +0200
> Subject: [PATCH] realtek_common_info
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
> drivers/net/dsa/realtek/realtek-common.c | 35 ++++++++++++++++++------
> drivers/net/dsa/realtek/realtek-common.h | 9 ++++--
> drivers/net/dsa/realtek/realtek-mdio.c | 27 ++----------------
> drivers/net/dsa/realtek/realtek-smi.c | 35 ++++--------------------
> 4 files changed, 41 insertions(+), 65 deletions(-)
>
> diff --git a/drivers/net/dsa/realtek/realtek-common.c b/drivers/net/dsa/realtek/realtek-common.c
> index 80b37e5fe780..bd6b04922b6d 100644
> --- a/drivers/net/dsa/realtek/realtek-common.c
> +++ b/drivers/net/dsa/realtek/realtek-common.c
> @@ -22,10 +22,21 @@ void realtek_common_unlock(void *ctx)
> EXPORT_SYMBOL_GPL(realtek_common_unlock);
>
> struct realtek_priv *
> -realtek_common_probe(struct device *dev, struct regmap_config rc,
> - struct regmap_config rc_nolock)
> +realtek_common_probe(struct device *dev,
> + const struct realtek_common_info *info)
> {
> const struct realtek_variant *var;
> + struct regmap_config rc = {
> + .reg_bits = 10, /* A4..A0 R4..R0 */
> + .val_bits = 16,
> + .reg_stride = 1,
> + /* PHY regs are at 0x8000 */
> + .max_register = 0xffff,
> + .reg_format_endian = REGMAP_ENDIAN_BIG,
> + .reg_read = info->reg_read,
> + .reg_write = info->reg_write,
> + .cache_type = REGCACHE_NONE,
> + };
> struct realtek_priv *priv;
> int ret;
>
> @@ -40,17 +51,23 @@ realtek_common_probe(struct device *dev, struct regmap_config rc,
>
> mutex_init(&priv->map_lock);
>
> - rc.lock_arg = priv;
> - priv->map = devm_regmap_init(dev, NULL, priv, &rc);
> - if (IS_ERR(priv->map)) {
> - ret = PTR_ERR(priv->map);
> + /* Initialize the non-locking regmap first */
> + rc.disable_locking = true;
> + priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc);
> + 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);
> }
>
> - priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc_nolock);
> - if (IS_ERR(priv->map_nolock)) {
> - ret = PTR_ERR(priv->map_nolock);
> + /* Then the locking regmap */
> + rc.disable_locking = false;
> + rc.lock = realtek_common_lock;
> + rc.unlock = realtek_common_unlock;
> + rc.lock_arg = priv;
> + priv->map = devm_regmap_init(dev, NULL, priv, &rc);
> + if (IS_ERR(priv->map)) {
> + ret = PTR_ERR(priv->map);
> dev_err(dev, "regmap init failed: %d\n", ret);
> return ERR_PTR(ret);
> }
> diff --git a/drivers/net/dsa/realtek/realtek-common.h b/drivers/net/dsa/realtek/realtek-common.h
> index 518d091ff496..71fc43d8d90a 100644
> --- a/drivers/net/dsa/realtek/realtek-common.h
> +++ b/drivers/net/dsa/realtek/realtek-common.h
> @@ -5,11 +5,16 @@
>
> #include <linux/regmap.h>
>
> +struct realtek_common_info {
> + int (*reg_read)(void *ctx, u32 reg, u32 *val);
> + int (*reg_write)(void *ctx, u32 reg, u32 val);
> +};
> +
> void realtek_common_lock(void *ctx);
> void realtek_common_unlock(void *ctx);
> struct realtek_priv *
> -realtek_common_probe(struct device *dev, struct regmap_config rc,
> - struct regmap_config rc_nolock);
> +realtek_common_probe(struct device *dev,
> + const struct realtek_common_info *info);
> int realtek_common_register_switch(struct realtek_priv *priv);
> void realtek_common_remove(struct realtek_priv *priv);
>
> diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> index 1eed09ab3aa1..8725cd1b027b 100644
> --- a/drivers/net/dsa/realtek/realtek-mdio.c
> +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> @@ -101,31 +101,9 @@ static int realtek_mdio_read(void *ctx, u32 reg, u32 *val)
> return ret;
> }
>
> -static const struct regmap_config realtek_mdio_regmap_config = {
> - .reg_bits = 10, /* A4..A0 R4..R0 */
> - .val_bits = 16,
> - .reg_stride = 1,
> - /* PHY regs are at 0x8000 */
> - .max_register = 0xffff,
> - .reg_format_endian = REGMAP_ENDIAN_BIG,
> +static const struct realtek_common_info realtek_mdio_info = {
> .reg_read = realtek_mdio_read,
> .reg_write = realtek_mdio_write,
> - .cache_type = REGCACHE_NONE,
> - .lock = realtek_common_lock,
> - .unlock = realtek_common_unlock,
> -};
> -
> -static const struct regmap_config realtek_mdio_nolock_regmap_config = {
> - .reg_bits = 10, /* A4..A0 R4..R0 */
> - .val_bits = 16,
> - .reg_stride = 1,
> - /* PHY regs are at 0x8000 */
> - .max_register = 0xffff,
> - .reg_format_endian = REGMAP_ENDIAN_BIG,
> - .reg_read = realtek_mdio_read,
> - .reg_write = realtek_mdio_write,
> - .cache_type = REGCACHE_NONE,
> - .disable_locking = true,
> };
>
> int realtek_mdio_probe(struct mdio_device *mdiodev)
> @@ -134,8 +112,7 @@ int realtek_mdio_probe(struct mdio_device *mdiodev)
> struct realtek_priv *priv;
> int ret;
>
> - priv = realtek_common_probe(dev, realtek_mdio_regmap_config,
> - realtek_mdio_nolock_regmap_config);
> + priv = realtek_common_probe(dev, &realtek_mdio_info);
> if (IS_ERR(priv))
> return PTR_ERR(priv);
>
> diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
> index fc54190839cf..7697dc66e5e8 100644
> --- a/drivers/net/dsa/realtek/realtek-smi.c
> +++ b/drivers/net/dsa/realtek/realtek-smi.c
> @@ -312,33 +312,6 @@ static int realtek_smi_read(void *ctx, u32 reg, u32 *val)
> return realtek_smi_read_reg(priv, reg, val);
> }
>
> -static const struct regmap_config realtek_smi_regmap_config = {
> - .reg_bits = 10, /* A4..A0 R4..R0 */
> - .val_bits = 16,
> - .reg_stride = 1,
> - /* PHY regs are at 0x8000 */
> - .max_register = 0xffff,
> - .reg_format_endian = REGMAP_ENDIAN_BIG,
> - .reg_read = realtek_smi_read,
> - .reg_write = realtek_smi_write,
> - .cache_type = REGCACHE_NONE,
> - .lock = realtek_common_lock,
> - .unlock = realtek_common_unlock,
> -};
> -
> -static const struct regmap_config realtek_smi_nolock_regmap_config = {
> - .reg_bits = 10, /* A4..A0 R4..R0 */
> - .val_bits = 16,
> - .reg_stride = 1,
> - /* PHY regs are at 0x8000 */
> - .max_register = 0xffff,
> - .reg_format_endian = REGMAP_ENDIAN_BIG,
> - .reg_read = realtek_smi_read,
> - .reg_write = realtek_smi_write,
> - .cache_type = REGCACHE_NONE,
> - .disable_locking = true,
> -};
> -
> static int realtek_smi_mdio_read(struct mii_bus *bus, int addr, int regnum)
> {
> struct realtek_priv *priv = bus->priv;
> @@ -396,14 +369,18 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
> return ret;
> }
>
> +static const struct realtek_common_info realtek_smi_info = {
> + .reg_read = realtek_smi_read,
> + .reg_write = realtek_smi_write,
> +};
> +
> int realtek_smi_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> struct realtek_priv *priv;
> int ret;
>
> - priv = realtek_common_probe(dev, realtek_smi_regmap_config,
> - realtek_smi_nolock_regmap_config);
> + priv = realtek_common_probe(dev, &realtek_smi_info);
> if (IS_ERR(priv))
> return PTR_ERR(priv);
>
> --
> 2.34.1
>
I don't have a strong opinion on how the regmap config is prepared.
I'll adopt your suggestion, with some minor changes.
Regmap config for realtek dsa driver should be composed of values that
(should) depend on the variant, the management interface (read/write)
and some common values. Today it is only dependent on the interface
and common values (with register range being a forced coincidence).
However, that is a topic for another series. We do need to stop this
series at some point. It already has 11 patches (and not counting the
2 bindings + 1 code that are actually my target). We already have some
nice features for delivery.
Regards,
Luiz
Powered by blists - more mailing lists