| 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: <20240111094148.jltccq4r6b42wbgq@skbuf>
Date: Thu, 11 Jan 2024 11:41:48 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Luiz Angelo Daros de Luca <luizluca@...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
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
> This case in particular might be hard to justify in the commit message
> other than "preliminary work". I'll split it as it makes review much
> easier. this series will grow from 7 to 10 patches, even after
> dropping the revert patch.
Preliminary work is fine if you explain a bit in advance why it will be
needed.
Powered by blists - more mailing lists