| Message ID | 20240822-net-dsa-microchip-regmap-locking-v1-1-d557073b883c@kernel.org (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: microchip: Use standard regmap locking | expand |
On 8/22/24 21:53, Mark Brown wrote: > For unclear reasons the ksz drivers use custom regmap locking which is > simply a wrapper around a standard mutex. According to the commit introducing such lock, 013572a236ef53cbd1e315e0acf2ee632cc816d4 the ksz driver family one regmap per register width (8/16/32), accessing the same set of registers. The locking implemented with the code removed here allows serializing operations using different register widths. Thanks, Paolo
On Tue, Aug 27, 2024 at 02:54:52PM +0200, Paolo Abeni wrote: > On 8/22/24 21:53, Mark Brown wrote: > > For unclear reasons the ksz drivers use custom regmap locking which is > > simply a wrapper around a standard mutex. > > According to the commit introducing such lock, > 013572a236ef53cbd1e315e0acf2ee632cc816d4 > the ksz driver family one regmap per register width (8/16/32), accessing the > same set of registers. > > The locking implemented with the code removed here allows serializing > operations using different register widths. Just expanding on that, here is the full commit message: commit 013572a236ef53cbd1e315e0acf2ee632cc816d4 Author: Marek Vasut <marex@denx.de> Date: Wed Oct 16 15:33:24 2019 +0200 net: dsa: microchip: Add shared regmap mutex The KSZ driver uses one regmap per register width (8/16/32), each with it's own lock, but accessing the same set of registers. In theory, it is possible to create a race condition between these regmaps, although the underlying bus (SPI or I2C) locking should assure nothing bad will really happen and the accesses would be correct. To make the driver do the right thing, add one single shared mutex for all the regmaps used by the driver instead. This assures that even if some future hardware is on a bus which does not serialize the accesses the same way SPI or I2C does, nothing bad will happen. Also, adding Marek to Cc: Andrew
On Tue, Aug 27, 2024 at 02:54:52PM +0200, Paolo Abeni wrote: > On 8/22/24 21:53, Mark Brown wrote: > > For unclear reasons the ksz drivers use custom regmap locking which is > > simply a wrapper around a standard mutex. > > According to the commit introducing such lock, > 013572a236ef53cbd1e315e0acf2ee632cc816d4 > the ksz driver family one regmap per register width (8/16/32), accessing the > same set of registers. > > The locking implemented with the code removed here allows serializing > operations using different register widths. This really could use a comment somewhere.
diff --git a/drivers/net/dsa/microchip/ksz8863_smi.c b/drivers/net/dsa/microchip/ksz8863_smi.c index 5711a59e2ac9..582e744f7b68 100644 --- a/drivers/net/dsa/microchip/ksz8863_smi.c +++ b/drivers/net/dsa/microchip/ksz8863_smi.c @@ -105,8 +105,6 @@ static const struct regmap_config ksz8863_regmap_config[] = { .pad_bits = 24, .val_bits = 8, .cache_type = REGCACHE_NONE, - .lock = ksz_regmap_lock, - .unlock = ksz_regmap_unlock, .max_register = U8_MAX, }, { @@ -115,8 +113,6 @@ static const struct regmap_config ksz8863_regmap_config[] = { .pad_bits = 24, .val_bits = 16, .cache_type = REGCACHE_NONE, - .lock = ksz_regmap_lock, - .unlock = ksz_regmap_unlock, .max_register = U8_MAX, }, { @@ -125,8 +121,6 @@ static const struct regmap_config ksz8863_regmap_config[] = { .pad_bits = 24, .val_bits = 32, .cache_type = REGCACHE_NONE, - .lock = ksz_regmap_lock, - .unlock = ksz_regmap_unlock, .max_register = U8_MAX, } }; @@ -150,7 +144,6 @@ static int ksz8863_smi_probe(struct mdio_device *mdiodev) for (i = 0; i < __KSZ_NUM_REGMAPS; i++) { rc = ksz8863_regmap_config[i]; - rc.lock_arg = &dev->regmap_mutex; rc.wr_table = chip->wr_table; rc.rd_table = chip->rd_table; dev->regmap[i] = devm_regmap_init(&mdiodev->dev, diff --git a/drivers/net/dsa/microchip/ksz9477_i2c.c b/drivers/net/dsa/microchip/ksz9477_i2c.c index 7d7560f23a73..59deb390e605 100644 --- a/drivers/net/dsa/microchip/ksz9477_i2c.c +++ b/drivers/net/dsa/microchip/ksz9477_i2c.c @@ -26,7 +26,6 @@ static int ksz9477_i2c_probe(struct i2c_client *i2c) for (i = 0; i < __KSZ_NUM_REGMAPS; i++) { rc = ksz9477_regmap_config[i]; - rc.lock_arg = &dev->regmap_mutex; dev->regmap[i] = devm_regmap_init_i2c(i2c, &rc); if (IS_ERR(dev->regmap[i])) { return dev_err_probe(&i2c->dev, PTR_ERR(dev->regmap[i]), diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c index 1491099528be..4a383c87acf8 100644 --- a/drivers/net/dsa/microchip/ksz_common.c +++ b/drivers/net/dsa/microchip/ksz_common.c @@ -4381,7 +4381,6 @@ int ksz_switch_register(struct ksz_device *dev) } mutex_init(&dev->dev_mutex); - mutex_init(&dev->regmap_mutex); mutex_init(&dev->alu_mutex); mutex_init(&dev->vlan_mutex); diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h index 5f0a628b9849..b9d4e0e9460d 100644 --- a/drivers/net/dsa/microchip/ksz_common.h +++ b/drivers/net/dsa/microchip/ksz_common.h @@ -152,7 +152,6 @@ struct ksz_device { const struct ksz_chip_data *info; struct mutex dev_mutex; /* device access */ - struct mutex regmap_mutex; /* regmap access */ struct mutex alu_mutex; /* ALU access */ struct mutex vlan_mutex; /* vlan access */ const struct ksz_dev_ops *dev_ops; @@ -600,18 +599,6 @@ static inline int ksz_prmw32(struct ksz_device *dev, int port, int offset, mask, val); } -static inline void ksz_regmap_lock(void *__mtx) -{ - struct mutex *mtx = __mtx; - mutex_lock(mtx); -} - -static inline void ksz_regmap_unlock(void *__mtx) -{ - struct mutex *mtx = __mtx; - mutex_unlock(mtx); -} - static inline bool ksz_is_ksz87xx(struct ksz_device *dev) { return dev->chip_id == KSZ8795_CHIP_ID || @@ -781,8 +768,6 @@ static inline bool is_lan937x_tx_phy(struct ksz_device *dev, int port) .write_flag_mask = \ KSZ_SPI_OP_FLAG_MASK(KSZ_SPI_OP_WR, swp, \ regbits, regpad), \ - .lock = ksz_regmap_lock, \ - .unlock = ksz_regmap_unlock, \ .reg_format_endian = REGMAP_ENDIAN_BIG, \ .val_format_endian = REGMAP_ENDIAN_BIG \ } diff --git a/drivers/net/dsa/microchip/ksz_spi.c b/drivers/net/dsa/microchip/ksz_spi.c index 8e8d83213b04..06a9e648df0c 100644 --- a/drivers/net/dsa/microchip/ksz_spi.c +++ b/drivers/net/dsa/microchip/ksz_spi.c @@ -65,7 +65,6 @@ static int ksz_spi_probe(struct spi_device *spi) for (i = 0; i < __KSZ_NUM_REGMAPS; i++) { rc = regmap_config[i]; - rc.lock_arg = &dev->regmap_mutex; rc.wr_table = chip->wr_table; rc.rd_table = chip->rd_table; dev->regmap[i] = devm_regmap_init_spi(spi, &rc);
For unclear reasons the ksz drivers use custom regmap locking which is simply a wrapper around a standard mutex. This mutex is not used outside of the regmap operations so the result is simply to replicate the standard mutex based locking provided by the regmap core. Remove the redundant code and rely on the regmap core instead. Signed-off-by: Mark Brown <broonie@kernel.org> --- drivers/net/dsa/microchip/ksz8863_smi.c | 7 ------- drivers/net/dsa/microchip/ksz9477_i2c.c | 1 - drivers/net/dsa/microchip/ksz_common.c | 1 - drivers/net/dsa/microchip/ksz_common.h | 15 --------------- drivers/net/dsa/microchip/ksz_spi.c | 1 - 5 files changed, 25 deletions(-) --- base-commit: 7c626ce4bae1ac14f60076d00eafe71af30450ba change-id: 20240822-net-dsa-microchip-regmap-locking-9e79d9314e1f Best regards,