[PATCH 0/2] Clause-22/Clause-45 MDIO regmap support fixups

[PATCH 0/2] Clause-22/Clause-45 MDIO regmap support fixups

[Sander Vanheule]

A proposed patch to make C22 access more strict [1], was posted in reply to an
RFC series which also added C45 register access [2]. It appears that as a
result, the original RFC patches got merged instead.

Since there are currently no (planned) C45 regmap users, this patch is
reverted. C22 access functions are corrected to return -ENXIO, instead of
silently ignoring any invalid high bits in the register offset.

[1] Proposed C22 patch:
https://lore.kernel.org/lkml/20210605083116.12786-1-sander@svanheule.net/

[2] RFC series:
https://lore.kernel.org/lkml/cover.1622743333.git.sander@svanheule.net/

Sander Vanheule (2):
  Revert "regmap: mdio: Add clause-45 support"
  regmap: mdio: Reject invalid clause-22 addresses

 drivers/base/regmap/regmap-mdio.c | 72 ++++++++-----------------------
 1 file changed, 18 insertions(+), 54 deletions(-)

-- 
2.31.1

[PATCH 1/2] Revert "regmap: mdio: Add clause-45 support"

[Sander Vanheule]

This reverts commit f083be9db060fbac09123d80bdffb2c001ac0e2b.

There are currently no (planned) regmap users for C45 register access.
Remove support for now, to reduce dead code.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
 drivers/base/regmap/regmap-mdio.c | 70 +++++++------------------------
 1 file changed, 14 insertions(+), 56 deletions(-)

diff --git a/drivers/base/regmap/regmap-mdio.c b/drivers/base/regmap/regmap-mdio.c
index cfb23afb19eb..aee34bf2400e 100644
--- a/drivers/base/regmap/regmap-mdio.c
+++ b/drivers/base/regmap/regmap-mdio.c
@@ -7,14 +7,13 @@
 
 #define REGVAL_MASK		GENMASK(15, 0)
 #define REGNUM_C22_MASK		GENMASK(4, 0)
-/* Clause-45 mask includes the device type (5 bit) and actual register number (16 bit) */
-#define REGNUM_C45_MASK		GENMASK(20, 0)
 
-static int regmap_mdio_read(struct mdio_device *mdio_dev, u32 reg, unsigned int *val)
+static int regmap_mdio_read(void *context, unsigned int reg, unsigned int *val)
 {
+	struct mdio_device *mdio_dev = context;
 	int ret;
 
-	ret = mdiobus_read(mdio_dev->bus, mdio_dev->addr, reg);
+	ret = mdiobus_read(mdio_dev->bus, mdio_dev->addr, reg & REGNUM_C22_MASK);
 	if (ret < 0)
 		return ret;
 
@@ -22,63 +21,27 @@ static int regmap_mdio_read(struct mdio_device *mdio_dev, u32 reg, unsigned int
 	return 0;
 }
 
-static int regmap_mdio_write(struct mdio_device *mdio_dev, u32 reg, unsigned int val)
-{
-	return mdiobus_write(mdio_dev->bus, mdio_dev->addr, reg, val);
-}
-
-static int regmap_mdio_c22_read(void *context, unsigned int reg, unsigned int *val)
-{
-	struct mdio_device *mdio_dev = context;
-
-	return regmap_mdio_read(mdio_dev, reg & REGNUM_C22_MASK, val);
-}
-
-static int regmap_mdio_c22_write(void *context, unsigned int reg, unsigned int val)
+static int regmap_mdio_write(void *context, unsigned int reg, unsigned int val)
 {
 	struct mdio_device *mdio_dev = context;
 
-	return regmap_mdio_write(mdio_dev, reg & REGNUM_C22_MASK, val);
+	return mdiobus_write(mdio_dev->bus, mdio_dev->addr, reg & REGNUM_C22_MASK, val);
 }
 
-static const struct regmap_bus regmap_mdio_c22_bus = {
-	.reg_write = regmap_mdio_c22_write,
-	.reg_read = regmap_mdio_c22_read,
-};
-
-static int regmap_mdio_c45_read(void *context, unsigned int reg, unsigned int *val)
-{
-	struct mdio_device *mdio_dev = context;
-
-	return regmap_mdio_read(mdio_dev, MII_ADDR_C45 | (reg & REGNUM_C45_MASK), val);
-}
-
-static int regmap_mdio_c45_write(void *context, unsigned int reg, unsigned int val)
-{
-	struct mdio_device *mdio_dev = context;
-
-	return regmap_mdio_write(mdio_dev, MII_ADDR_C45 | (reg & REGNUM_C45_MASK), val);
-}
-
-static const struct regmap_bus regmap_mdio_c45_bus = {
-	.reg_write = regmap_mdio_c45_write,
-	.reg_read = regmap_mdio_c45_read,
+static const struct regmap_bus regmap_mdio_bus = {
+	.reg_write = regmap_mdio_write,
+	.reg_read = regmap_mdio_read,
 };
 
 struct regmap *__regmap_init_mdio(struct mdio_device *mdio_dev,
 	const struct regmap_config *config, struct lock_class_key *lock_key,
 	const char *lock_name)
 {
-	struct regmap_bus *bus;
-
-	if (config->reg_bits == 5 && config->val_bits == 16)
-		bus = &regmap_mdio_c22_bus;
-	else if (config->reg_bits == 21 && config->val_bits == 16)
-		bus = &regmap_mdio_c45_bus;
-	else
+	if (config->reg_bits != 5 || config->val_bits != 16)
 		return ERR_PTR(-EOPNOTSUPP);
 
-	return __regmap_init(&mdio_dev->dev, bus, mdio_dev, config, lock_key, lock_name);
+	return __regmap_init(&mdio_dev->dev, &regmap_mdio_bus, mdio_dev, config,
+		lock_key, lock_name);
 }
 EXPORT_SYMBOL_GPL(__regmap_init_mdio);
 
@@ -86,16 +49,11 @@ struct regmap *__devm_regmap_init_mdio(struct mdio_device *mdio_dev,
 	const struct regmap_config *config, struct lock_class_key *lock_key,
 	const char *lock_name)
 {
-	const struct regmap_bus *bus;
-
-	if (config->reg_bits == 5 && config->val_bits == 16)
-		bus = &regmap_mdio_c22_bus;
-	else if (config->reg_bits == 21 && config->val_bits == 16)
-		bus = &regmap_mdio_c45_bus;
-	else
+	if (config->reg_bits != 5 || config->val_bits != 16)
 		return ERR_PTR(-EOPNOTSUPP);
 
-	return __devm_regmap_init(&mdio_dev->dev, bus, mdio_dev, config, lock_key, lock_name);
+	return __devm_regmap_init(&mdio_dev->dev, &regmap_mdio_bus, mdio_dev,
+		config, lock_key, lock_name);
 }
 EXPORT_SYMBOL_GPL(__devm_regmap_init_mdio);
 
-- 
2.31.1

[PATCH 2/2] regmap: mdio: Reject invalid clause-22 addresses

[Sander Vanheule]

When an invalid register offset is provided, the upper bits are silently
discarded. Change this to return -ENXIO instead, to help catch potential
bugs.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
 drivers/base/regmap/regmap-mdio.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/base/regmap/regmap-mdio.c b/drivers/base/regmap/regmap-mdio.c
index aee34bf2400e..9adfb82be8f1 100644
--- a/drivers/base/regmap/regmap-mdio.c
+++ b/drivers/base/regmap/regmap-mdio.c
@@ -13,7 +13,10 @@ static int regmap_mdio_read(void *context, unsigned int reg, unsigned int *val)
 	struct mdio_device *mdio_dev = context;
 	int ret;
 
-	ret = mdiobus_read(mdio_dev->bus, mdio_dev->addr, reg & REGNUM_C22_MASK);
+	if (unlikely(reg & ~REGNUM_C22_MASK))
+		return -ENXIO;
+
+	ret = mdiobus_read(mdio_dev->bus, mdio_dev->addr, reg);
 	if (ret < 0)
 		return ret;
 
@@ -25,7 +28,10 @@ static int regmap_mdio_write(void *context, unsigned int reg, unsigned int val)
 {
 	struct mdio_device *mdio_dev = context;
 
-	return mdiobus_write(mdio_dev->bus, mdio_dev->addr, reg & REGNUM_C22_MASK, val);
+	if (unlikely(reg & ~REGNUM_C22_MASK))
+		return -ENXIO;
+
+	return mdiobus_write(mdio_dev->bus, mdio_dev->addr, reg, val);
 }
 
 static const struct regmap_bus regmap_mdio_bus = {
-- 
2.31.1

Re: [PATCH 1/2] Revert "regmap: mdio: Add clause-45 support"

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 722 bytes --]

On Wed, Jun 09, 2021 at 01:46:05PM +0200, Sander Vanheule wrote:
> This reverts commit f083be9db060fbac09123d80bdffb2c001ac0e2b.

Please submit patches using subject lines reflecting the style for the
subsystem, this makes it easier for people to identify relevant patches.
Look at what existing commits in the area you're changing are doing and
make sure your subject lines visually resemble what they're doing.
There's no need to resubmit to fix this alone.

> There are currently no (planned) regmap users for C45 register access.
> Remove support for now, to reduce dead code.

This then creates a bootstrapping issue for anyone who does need it - I
can't see any way in which this causes problems or gets in the way?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] Revert "regmap: mdio: Add clause-45 support"

[Sander Vanheule]

On Wed, 2021-06-09 at 13:24 +0100, Mark Brown wrote:
> On Wed, Jun 09, 2021 at 01:46:05PM +0200, Sander Vanheule wrote:
> > This reverts commit f083be9db060fbac09123d80bdffb2c001ac0e2b.
> 
> Please submit patches using subject lines reflecting the style for the
> subsystem, this makes it easier for people to identify relevant patches.
> Look at what existing commits in the area you're changing are doing and
> make sure your subject lines visually resemble what they're doing.
> There's no need to resubmit to fix this alone.

I had grepped the commit log for other reverting patches, which also appear to
use this style, but I didn't check the regmap-specific ones.


> > There are currently no (planned) regmap users for C45 register access.
> > Remove support for now, to reduce dead code.
> 
> This then creates a bootstrapping issue for anyone who does need it - I
> can't see any way in which this causes problems or gets in the way?

If you would rather keep this, I should modify the other patch (regmap: mdio:
Reject invalid clause-22 addresses) to also cover C45 addresses.
Furthermore, there's an issue with a pointer const-ness in __regmap_init_mdio
that needs to be fixed if this code is staying.

I'll submit a v2 that fixes __regmap_init_mdio, and also applies the address
checks to C45 access.

Best,
Sander

Re: [PATCH 1/2] Revert "regmap: mdio: Add clause-45 support"

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 925 bytes --]

On Wed, Jun 09, 2021 at 02:42:06PM +0200, Sander Vanheule wrote:
> On Wed, 2021-06-09 at 13:24 +0100, Mark Brown wrote:

> > Please submit patches using subject lines reflecting the style for the
> > subsystem, this makes it easier for people to identify relevant patches.
> > Look at what existing commits in the area you're changing are doing and
> > make sure your subject lines visually resemble what they're doing.
> > There's no need to resubmit to fix this alone.

> I had grepped the commit log for other reverting patches, which also appear to
> use this style, but I didn't check the regmap-specific ones.

There's nothing in submitting-patches.rst which says to skip the usual
process for reverts - the fact that people sometimes apply things with
non-ideal subject lines doesn't mean it's best practice.

> I'll submit a v2 that fixes __regmap_init_mdio, and also applies the address
> checks to C45 access.

OK.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]