| Message ID | 20220221184631.252308-2-alvin@pqrs.dk (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 907e772f6f6debb610ea28298ab57b31019a4edb |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: realtek: fix PHY register read corruption | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | warning | CHECK: struct mutex definition without comment |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Mon, Feb 21, 2022 at 07:46:30PM +0100, Alvin Šipraga wrote: > From: Alvin Šipraga <alsi@bang-olufsen.dk> > > Currently there is no way for Realtek DSA subdrivers to serialize > consecutive regmap accesses. In preparation for a bugfix relating to > indirect PHY register access - which involves a series of regmap > reads and writes - add a facility for subdrivers to serialize their > regmap access. > > Specifically, a mutex is added to the driver private data structure and > the standard regmap is initialized with custom lock/unlock ops which use > this mutex. Then, a "nolock" variant of the regmap is added, which is > functionally equivalent to the existing regmap except that regmap > locking is disabled. Functions that wish to serialize a sequence of > regmap accesses may then lock the newly introduced driver-owned mutex > before using the nolock regmap. > > Doing things this way means that subdriver code that doesn't care about > serialized register access - i.e. the vast majority of code - needn't > worry about synchronizing register access with an external lock: it can > just continue to use the original regmap. > > Another advantage of this design is that, while regmaps with locking > disabled do not expose a debugfs interface for obvious reasons, there > still exists the original regmap which does expose this interface. This > interface remains safe to use even combined with driver codepaths that > use the nolock regmap, because said codepaths will use the same mutex > to synchronize access. > > With respect to disadvantages, it can be argued that having > near-duplicate regmaps is confusing. However, the naming is rather > explicit, and examples will abound. > > Finally, while we are at it, rename realtek_smi_mdio_regmap_config to > realtek_smi_regmap_config. This makes it consistent with the naming > realtek_mdio_regmap_config in realtek-mdio.c. > > Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk> > --- Reviewed-by: Vladimir Oltean <olteanv@gmail.com> > drivers/net/dsa/realtek/realtek-mdio.c | 46 ++++++++++++++++++++++-- > drivers/net/dsa/realtek/realtek-smi.c | 48 ++++++++++++++++++++++++-- > drivers/net/dsa/realtek/realtek.h | 2 ++ > 3 files changed, 91 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c > index 0308be95d00a..31e1f100e48e 100644 > --- a/drivers/net/dsa/realtek/realtek-mdio.c > +++ b/drivers/net/dsa/realtek/realtek-mdio.c > @@ -98,6 +98,20 @@ static int realtek_mdio_read(void *ctx, u32 reg, u32 *val) > return ret; > } > > +static void realtek_mdio_lock(void *ctx) I looked even at the build results for v1 and I don't know exactly why sparse doesn't complain about the lack of __acquires() and __releases() annotations, to avoid context imbalance warnings. https://patchwork.kernel.org/project/netdevbpf/patch/20220216160500.2341255-2-alvin@pqrs.dk/ Anyway, those aren't of much use IMO (you can't get it to do anything but very trivial checks), and if sparse doesn't complain for whatever reason, I certainly won't request you to add them. I see other implementations don't have them either - like vexpress_config_lock.
diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c index 0308be95d00a..31e1f100e48e 100644 --- a/drivers/net/dsa/realtek/realtek-mdio.c +++ b/drivers/net/dsa/realtek/realtek-mdio.c @@ -98,6 +98,20 @@ static int realtek_mdio_read(void *ctx, u32 reg, u32 *val) return ret; } +static void realtek_mdio_lock(void *ctx) +{ + struct realtek_priv *priv = ctx; + + mutex_lock(&priv->map_lock); +} + +static void realtek_mdio_unlock(void *ctx) +{ + struct realtek_priv *priv = ctx; + + mutex_unlock(&priv->map_lock); +} + static const struct regmap_config realtek_mdio_regmap_config = { .reg_bits = 10, /* A4..A0 R4..R0 */ .val_bits = 16, @@ -108,6 +122,21 @@ static const struct regmap_config realtek_mdio_regmap_config = { .reg_read = realtek_mdio_read, .reg_write = realtek_mdio_write, .cache_type = REGCACHE_NONE, + .lock = realtek_mdio_lock, + .unlock = realtek_mdio_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, }; static int realtek_mdio_probe(struct mdio_device *mdiodev) @@ -115,8 +144,9 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev) struct realtek_priv *priv; struct device *dev = &mdiodev->dev; const struct realtek_variant *var; - int ret; + struct regmap_config rc; struct device_node *np; + int ret; var = of_device_get_match_data(dev); if (!var) @@ -126,13 +156,25 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev) if (!priv) return -ENOMEM; - priv->map = devm_regmap_init(dev, NULL, priv, &realtek_mdio_regmap_config); + mutex_init(&priv->map_lock); + + rc = realtek_mdio_regmap_config; + 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 ret; } + rc = realtek_mdio_nolock_regmap_config; + 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 ret; + } + priv->mdio_addr = mdiodev->addr; priv->bus = mdiodev->bus; priv->dev = &mdiodev->dev; diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c index 8806b74bd7a8..2243d3da55b2 100644 --- a/drivers/net/dsa/realtek/realtek-smi.c +++ b/drivers/net/dsa/realtek/realtek-smi.c @@ -311,7 +311,21 @@ 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_mdio_regmap_config = { +static void realtek_smi_lock(void *ctx) +{ + struct realtek_priv *priv = ctx; + + mutex_lock(&priv->map_lock); +} + +static void realtek_smi_unlock(void *ctx) +{ + struct realtek_priv *priv = ctx; + + mutex_unlock(&priv->map_lock); +} + +static const struct regmap_config realtek_smi_regmap_config = { .reg_bits = 10, /* A4..A0 R4..R0 */ .val_bits = 16, .reg_stride = 1, @@ -321,6 +335,21 @@ static const struct regmap_config realtek_smi_mdio_regmap_config = { .reg_read = realtek_smi_read, .reg_write = realtek_smi_write, .cache_type = REGCACHE_NONE, + .lock = realtek_smi_lock, + .unlock = realtek_smi_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) @@ -385,6 +414,7 @@ static int realtek_smi_probe(struct platform_device *pdev) const struct realtek_variant *var; struct device *dev = &pdev->dev; struct realtek_priv *priv; + struct regmap_config rc; struct device_node *np; int ret; @@ -395,14 +425,26 @@ static int realtek_smi_probe(struct platform_device *pdev) if (!priv) return -ENOMEM; priv->chip_data = (void *)priv + sizeof(*priv); - priv->map = devm_regmap_init(dev, NULL, priv, - &realtek_smi_mdio_regmap_config); + + mutex_init(&priv->map_lock); + + rc = realtek_smi_regmap_config; + 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 ret; } + rc = realtek_smi_nolock_regmap_config; + 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 ret; + } + /* Link forward and backward */ priv->dev = dev; priv->clk_delay = var->clk_delay; diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h index e7d3e1bcf8b8..4fa7c6ba874a 100644 --- a/drivers/net/dsa/realtek/realtek.h +++ b/drivers/net/dsa/realtek/realtek.h @@ -52,6 +52,8 @@ struct realtek_priv { struct gpio_desc *mdc; struct gpio_desc *mdio; struct regmap *map; + struct regmap *map_nolock; + struct mutex map_lock; struct mii_bus *slave_mii_bus; struct mii_bus *bus; int mdio_addr;