* [PATCH 0/2] Clause-22/Clause-45 MDIO regmap support fixups @ 2021-06-09 11:46 Sander Vanheule 2021-06-09 11:46 ` [PATCH 1/2] Revert "regmap: mdio: Add clause-45 support" Sander Vanheule 2021-06-09 11:46 ` [PATCH 2/2] regmap: mdio: Reject invalid clause-22 addresses Sander Vanheule 0 siblings, 2 replies; 6+ messages in thread From: Sander Vanheule @ 2021-06-09 11:46 UTC (permalink / raw) To: Mark Brown, Greg Kroah-Hartman, Rafael J . Wysocki, linux-kernel Cc: Andy Shevchenko, Adrew Lunn, 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] Revert "regmap: mdio: Add clause-45 support" 2021-06-09 11:46 [PATCH 0/2] Clause-22/Clause-45 MDIO regmap support fixups Sander Vanheule @ 2021-06-09 11:46 ` Sander Vanheule 2021-06-09 12:24 ` Mark Brown 2021-06-09 11:46 ` [PATCH 2/2] regmap: mdio: Reject invalid clause-22 addresses Sander Vanheule 1 sibling, 1 reply; 6+ messages in thread From: Sander Vanheule @ 2021-06-09 11:46 UTC (permalink / raw) To: Mark Brown, Greg Kroah-Hartman, Rafael J . Wysocki, linux-kernel Cc: Andy Shevchenko, Adrew Lunn, 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 = ®map_mdio_c22_bus; - else if (config->reg_bits == 21 && config->val_bits == 16) - bus = ®map_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, ®map_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 = ®map_mdio_c22_bus; - else if (config->reg_bits == 21 && config->val_bits == 16) - bus = ®map_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, ®map_mdio_bus, mdio_dev, + config, lock_key, lock_name); } EXPORT_SYMBOL_GPL(__devm_regmap_init_mdio); -- 2.31.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] Revert "regmap: mdio: Add clause-45 support" 2021-06-09 11:46 ` [PATCH 1/2] Revert "regmap: mdio: Add clause-45 support" Sander Vanheule @ 2021-06-09 12:24 ` Mark Brown 2021-06-09 12:42 ` Sander Vanheule 0 siblings, 1 reply; 6+ messages in thread From: Mark Brown @ 2021-06-09 12:24 UTC (permalink / raw) To: Sander Vanheule Cc: Greg Kroah-Hartman, Rafael J . Wysocki, linux-kernel, Andy Shevchenko, Adrew Lunn [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] Revert "regmap: mdio: Add clause-45 support" 2021-06-09 12:24 ` Mark Brown @ 2021-06-09 12:42 ` Sander Vanheule 2021-06-09 13:23 ` Mark Brown 0 siblings, 1 reply; 6+ messages in thread From: Sander Vanheule @ 2021-06-09 12:42 UTC (permalink / raw) To: Mark Brown Cc: Greg Kroah-Hartman, Rafael J . Wysocki, linux-kernel, Andy Shevchenko, Adrew Lunn 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] Revert "regmap: mdio: Add clause-45 support" 2021-06-09 12:42 ` Sander Vanheule @ 2021-06-09 13:23 ` Mark Brown 0 siblings, 0 replies; 6+ messages in thread From: Mark Brown @ 2021-06-09 13:23 UTC (permalink / raw) To: Sander Vanheule Cc: Greg Kroah-Hartman, Rafael J . Wysocki, linux-kernel, Andy Shevchenko, Adrew Lunn [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] regmap: mdio: Reject invalid clause-22 addresses 2021-06-09 11:46 [PATCH 0/2] Clause-22/Clause-45 MDIO regmap support fixups Sander Vanheule 2021-06-09 11:46 ` [PATCH 1/2] Revert "regmap: mdio: Add clause-45 support" Sander Vanheule @ 2021-06-09 11:46 ` Sander Vanheule 1 sibling, 0 replies; 6+ messages in thread From: Sander Vanheule @ 2021-06-09 11:46 UTC (permalink / raw) To: Mark Brown, Greg Kroah-Hartman, Rafael J . Wysocki, linux-kernel Cc: Andy Shevchenko, Adrew Lunn, 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-06-09 13:23 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-09 11:46 [PATCH 0/2] Clause-22/Clause-45 MDIO regmap support fixups Sander Vanheule 2021-06-09 11:46 ` [PATCH 1/2] Revert "regmap: mdio: Add clause-45 support" Sander Vanheule 2021-06-09 12:24 ` Mark Brown 2021-06-09 12:42 ` Sander Vanheule 2021-06-09 13:23 ` Mark Brown 2021-06-09 11:46 ` [PATCH 2/2] regmap: mdio: Reject invalid clause-22 addresses Sander Vanheule
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.