Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/2] net: dsa: microchip: Do not reinit mutexes on KSZ87xx
@ 2019-10-10 18:25 Marek Vasut
  2019-10-10 18:25 ` [PATCH 2/2] net: dsa: microchip: Add shared regmap mutex Marek Vasut
  2019-10-13  0:20 ` [PATCH 1/2] net: dsa: microchip: Do not reinit mutexes on KSZ87xx David Miller
  0 siblings, 2 replies; 7+ messages in thread
From: Marek Vasut @ 2019-10-10 18:25 UTC (permalink / raw)
  To: netdev
  Cc: Marek Vasut, Andrew Lunn, David S . Miller, Florian Fainelli,
	George McCollister, Tristram Ha, Woojung Huh

The KSZ87xx driver calls mutex_init() on mutexes already inited in
ksz_common.c ksz_switch_register(). Do not do it twice, drop the
reinitialization.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: David S. Miller <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: George McCollister <george.mccollister@gmail.com>
Cc: Tristram Ha <Tristram.Ha@microchip.com>
Cc: Woojung Huh <woojung.huh@microchip.com>
---
 drivers/net/dsa/microchip/ksz8795.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index a23d3ffdf0c4..24a5e99f7fd5 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -1224,10 +1224,6 @@ static int ksz8795_switch_init(struct ksz_device *dev)
 {
 	int i;
 
-	mutex_init(&dev->stats_mutex);
-	mutex_init(&dev->alu_mutex);
-	mutex_init(&dev->vlan_mutex);
-
 	dev->ds->ops = &ksz8795_switch_ops;
 
 	for (i = 0; i < ARRAY_SIZE(ksz8795_switch_chips); i++) {
-- 
2.23.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 2/2] net: dsa: microchip: Add shared regmap mutex
  2019-10-10 18:25 [PATCH 1/2] net: dsa: microchip: Do not reinit mutexes on KSZ87xx Marek Vasut
@ 2019-10-10 18:25 ` Marek Vasut
  2019-10-13  0:20   ` David Miller
  2019-10-13  0:20 ` [PATCH 1/2] net: dsa: microchip: Do not reinit mutexes on KSZ87xx David Miller
  1 sibling, 1 reply; 7+ messages in thread
From: Marek Vasut @ 2019-10-10 18:25 UTC (permalink / raw)
  To: netdev
  Cc: Marek Vasut, Andrew Lunn, David S . Miller, Florian Fainelli,
	George McCollister, Tristram Ha, Woojung Huh

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.

Note that the status_mutex was unused and only initied, hence it was
renamed and repurposed as the regmap mutex.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: David S. Miller <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: George McCollister <george.mccollister@gmail.com>
Cc: Tristram Ha <Tristram.Ha@microchip.com>
Cc: Woojung Huh <woojung.huh@microchip.com>
---
 drivers/net/dsa/microchip/ksz8795_spi.c |  7 ++++---
 drivers/net/dsa/microchip/ksz9477_i2c.c |  6 ++++--
 drivers/net/dsa/microchip/ksz9477_spi.c |  6 ++++--
 drivers/net/dsa/microchip/ksz_common.c  | 14 +++++++++++++-
 drivers/net/dsa/microchip/ksz_common.h  |  7 ++++++-
 5 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795_spi.c b/drivers/net/dsa/microchip/ksz8795_spi.c
index d0f8153e86b7..614404c40cba 100644
--- a/drivers/net/dsa/microchip/ksz8795_spi.c
+++ b/drivers/net/dsa/microchip/ksz8795_spi.c
@@ -26,6 +26,7 @@ KSZ_REGMAP_TABLE(ksz8795, 16, SPI_ADDR_SHIFT,
 static int ksz8795_spi_probe(struct spi_device *spi)
 {
 	struct ksz_device *dev;
+	struct regmap_config rc;
 	int i, ret;
 
 	dev = ksz_switch_alloc(&spi->dev, spi);
@@ -33,9 +34,9 @@ static int ksz8795_spi_probe(struct spi_device *spi)
 		return -ENOMEM;
 
 	for (i = 0; i < ARRAY_SIZE(ksz8795_regmap_config); i++) {
-		dev->regmap[i] = devm_regmap_init_spi(spi,
-						      &ksz8795_regmap_config
-						      [i]);
+		rc = ksz8795_regmap_config[i];
+		rc.lock_arg = &dev->regmap_mutex;
+		dev->regmap[i] = devm_regmap_init_spi(spi, &rc);
 		if (IS_ERR(dev->regmap[i])) {
 			ret = PTR_ERR(dev->regmap[i]);
 			dev_err(&spi->dev,
diff --git a/drivers/net/dsa/microchip/ksz9477_i2c.c b/drivers/net/dsa/microchip/ksz9477_i2c.c
index b0a1595d780d..2cb178675b83 100644
--- a/drivers/net/dsa/microchip/ksz9477_i2c.c
+++ b/drivers/net/dsa/microchip/ksz9477_i2c.c
@@ -18,6 +18,7 @@ static int ksz9477_i2c_probe(struct i2c_client *i2c,
 			     const struct i2c_device_id *i2c_id)
 {
 	struct ksz_device *dev;
+	struct regmap_config rc;
 	int i, ret;
 
 	dev = ksz_switch_alloc(&i2c->dev, i2c);
@@ -25,8 +26,9 @@ static int ksz9477_i2c_probe(struct i2c_client *i2c,
 		return -ENOMEM;
 
 	for (i = 0; i < ARRAY_SIZE(ksz9477_regmap_config); i++) {
-		dev->regmap[i] = devm_regmap_init_i2c(i2c,
-					&ksz9477_regmap_config[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])) {
 			ret = PTR_ERR(dev->regmap[i]);
 			dev_err(&i2c->dev,
diff --git a/drivers/net/dsa/microchip/ksz9477_spi.c b/drivers/net/dsa/microchip/ksz9477_spi.c
index f4198d6f72be..a39d2373eb3a 100644
--- a/drivers/net/dsa/microchip/ksz9477_spi.c
+++ b/drivers/net/dsa/microchip/ksz9477_spi.c
@@ -25,6 +25,7 @@ KSZ_REGMAP_TABLE(ksz9477, 32, SPI_ADDR_SHIFT,
 static int ksz9477_spi_probe(struct spi_device *spi)
 {
 	struct ksz_device *dev;
+	struct regmap_config rc;
 	int i, ret;
 
 	dev = ksz_switch_alloc(&spi->dev, spi);
@@ -32,8 +33,9 @@ static int ksz9477_spi_probe(struct spi_device *spi)
 		return -ENOMEM;
 
 	for (i = 0; i < ARRAY_SIZE(ksz9477_regmap_config); i++) {
-		dev->regmap[i] = devm_regmap_init_spi(spi,
-					&ksz9477_regmap_config[i]);
+		rc = ksz9477_regmap_config[i];
+		rc.lock_arg = &dev->regmap_mutex;
+		dev->regmap[i] = devm_regmap_init_spi(spi, &rc);
 		if (IS_ERR(dev->regmap[i])) {
 			ret = PTR_ERR(dev->regmap[i]);
 			dev_err(&spi->dev,
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index b0b870f0c252..21e483f6e786 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -416,6 +416,18 @@ struct ksz_device *ksz_switch_alloc(struct device *base, void *priv)
 }
 EXPORT_SYMBOL(ksz_switch_alloc);
 
+void ksz_regmap_lock(void *__mtx)
+{
+	struct mutex *mtx = __mtx;
+	mutex_lock(mtx);
+}
+
+void ksz_regmap_unlock(void *__mtx)
+{
+	struct mutex *mtx = __mtx;
+	mutex_unlock(mtx);
+}
+
 int ksz_switch_register(struct ksz_device *dev,
 			const struct ksz_dev_ops *ops)
 {
@@ -436,7 +448,7 @@ int ksz_switch_register(struct ksz_device *dev,
 	}
 
 	mutex_init(&dev->dev_mutex);
-	mutex_init(&dev->stats_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 dd60d0837fc6..c21410353ea8 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -47,7 +47,7 @@ struct ksz_device {
 	const char *name;
 
 	struct mutex dev_mutex;		/* device access */
-	struct mutex stats_mutex;	/* status 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;
@@ -145,6 +145,9 @@ struct ksz_dev_ops {
 	void (*exit)(struct ksz_device *dev);
 };
 
+void ksz_regmap_lock(void *__mtx);
+void ksz_regmap_unlock(void *__mtx);
+
 struct ksz_device *ksz_switch_alloc(struct device *base, void *priv);
 int ksz_switch_register(struct ksz_device *dev,
 			const struct ksz_dev_ops *ops);
@@ -314,6 +317,8 @@ static inline void ksz_pwrite32(struct ksz_device *dev, int port, int offset,
 		.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			\
 	}
-- 
2.23.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] net: dsa: microchip: Do not reinit mutexes on KSZ87xx
  2019-10-10 18:25 [PATCH 1/2] net: dsa: microchip: Do not reinit mutexes on KSZ87xx Marek Vasut
  2019-10-10 18:25 ` [PATCH 2/2] net: dsa: microchip: Add shared regmap mutex Marek Vasut
@ 2019-10-13  0:20 ` David Miller
  1 sibling, 0 replies; 7+ messages in thread
From: David Miller @ 2019-10-13  0:20 UTC (permalink / raw)
  To: marex
  Cc: netdev, andrew, f.fainelli, george.mccollister, Tristram.Ha, woojung.huh

From: Marek Vasut <marex@denx.de>
Date: Thu, 10 Oct 2019 20:25:07 +0200

> The KSZ87xx driver calls mutex_init() on mutexes already inited in
> ksz_common.c ksz_switch_register(). Do not do it twice, drop the
> reinitialization.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>

Applied.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] net: dsa: microchip: Add shared regmap mutex
  2019-10-10 18:25 ` [PATCH 2/2] net: dsa: microchip: Add shared regmap mutex Marek Vasut
@ 2019-10-13  0:20   ` David Miller
  2019-10-13  0:21     ` David Miller
  0 siblings, 1 reply; 7+ messages in thread
From: David Miller @ 2019-10-13  0:20 UTC (permalink / raw)
  To: marex
  Cc: netdev, andrew, f.fainelli, george.mccollister, Tristram.Ha, woojung.huh

From: Marek Vasut <marex@denx.de>
Date: Thu, 10 Oct 2019 20:25:08 +0200

> 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.
> 
> Note that the status_mutex was unused and only initied, hence it was
> renamed and repurposed as the regmap mutex.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>

Applied.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] net: dsa: microchip: Add shared regmap mutex
  2019-10-13  0:20   ` David Miller
@ 2019-10-13  0:21     ` David Miller
  2019-10-13 10:50       ` Marek Vasut
  0 siblings, 1 reply; 7+ messages in thread
From: David Miller @ 2019-10-13  0:21 UTC (permalink / raw)
  To: marex
  Cc: netdev, andrew, f.fainelli, george.mccollister, Tristram.Ha, woojung.huh

From: David Miller <davem@davemloft.net>
Date: Sat, 12 Oct 2019 17:20:55 -0700 (PDT)

> From: Marek Vasut <marex@denx.de>
> Date: Thu, 10 Oct 2019 20:25:08 +0200
> 
>> 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.
>> 
>> Note that the status_mutex was unused and only initied, hence it was
>> renamed and repurposed as the regmap mutex.
>> 
>> Signed-off-by: Marek Vasut <marex@denx.de>
> 
> Applied.

Actually, both patches reverted.  Please test your changes properly:

ERROR: "ksz_regmap_unlock" [drivers/net/dsa/microchip/ksz8795_spi.ko] undefined!
ERROR: "ksz_regmap_lock" [drivers/net/dsa/microchip/ksz8795_spi.ko] undefined!
ERROR: "ksz_regmap_unlock" [drivers/net/dsa/microchip/ksz9477_spi.ko] undefined!
ERROR: "ksz_regmap_lock" [drivers/net/dsa/microchip/ksz9477_spi.ko] undefined!
ERROR: "ksz_regmap_unlock" [drivers/net/dsa/microchip/ksz9477_i2c.ko] undefined!
ERROR: "ksz_regmap_lock" [drivers/net/dsa/microchip/ksz9477_i2c.ko] undefined!

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] net: dsa: microchip: Add shared regmap mutex
  2019-10-13  0:21     ` David Miller
@ 2019-10-13 10:50       ` Marek Vasut
  2019-10-13 16:26         ` David Miller
  0 siblings, 1 reply; 7+ messages in thread
From: Marek Vasut @ 2019-10-13 10:50 UTC (permalink / raw)
  To: David Miller
  Cc: netdev, andrew, f.fainelli, george.mccollister, Tristram.Ha, woojung.huh

On 10/13/19 2:21 AM, David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Sat, 12 Oct 2019 17:20:55 -0700 (PDT)
> 
>> From: Marek Vasut <marex@denx.de>
>> Date: Thu, 10 Oct 2019 20:25:08 +0200
>>
>>> 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.
>>>
>>> Note that the status_mutex was unused and only initied, hence it was
>>> renamed and repurposed as the regmap mutex.
>>>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>
>> Applied.
> 
> Actually, both patches reverted.  Please test your changes properly:
> 
> ERROR: "ksz_regmap_unlock" [drivers/net/dsa/microchip/ksz8795_spi.ko] undefined!
> ERROR: "ksz_regmap_lock" [drivers/net/dsa/microchip/ksz8795_spi.ko] undefined!
> ERROR: "ksz_regmap_unlock" [drivers/net/dsa/microchip/ksz9477_spi.ko] undefined!
> ERROR: "ksz_regmap_lock" [drivers/net/dsa/microchip/ksz9477_spi.ko] undefined!
> ERROR: "ksz_regmap_unlock" [drivers/net/dsa/microchip/ksz9477_i2c.ko] undefined!
> ERROR: "ksz_regmap_lock" [drivers/net/dsa/microchip/ksz9477_i2c.ko] undefined!

So the test is to compile it as a module ?

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] net: dsa: microchip: Add shared regmap mutex
  2019-10-13 10:50       ` Marek Vasut
@ 2019-10-13 16:26         ` David Miller
  0 siblings, 0 replies; 7+ messages in thread
From: David Miller @ 2019-10-13 16:26 UTC (permalink / raw)
  To: marex
  Cc: netdev, andrew, f.fainelli, george.mccollister, Tristram.Ha, woojung.huh

From: Marek Vasut <marex@denx.de>
Date: Sun, 13 Oct 2019 12:50:15 +0200

> On 10/13/19 2:21 AM, David Miller wrote:
>> From: David Miller <davem@davemloft.net>
>> Date: Sat, 12 Oct 2019 17:20:55 -0700 (PDT)
>> 
>>> From: Marek Vasut <marex@denx.de>
>>> Date: Thu, 10 Oct 2019 20:25:08 +0200
>>>
>>>> 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.
>>>>
>>>> Note that the status_mutex was unused and only initied, hence it was
>>>> renamed and repurposed as the regmap mutex.
>>>>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>
>>> Applied.
>> 
>> Actually, both patches reverted.  Please test your changes properly:
>> 
>> ERROR: "ksz_regmap_unlock" [drivers/net/dsa/microchip/ksz8795_spi.ko] undefined!
>> ERROR: "ksz_regmap_lock" [drivers/net/dsa/microchip/ksz8795_spi.ko] undefined!
>> ERROR: "ksz_regmap_unlock" [drivers/net/dsa/microchip/ksz9477_spi.ko] undefined!
>> ERROR: "ksz_regmap_lock" [drivers/net/dsa/microchip/ksz9477_spi.ko] undefined!
>> ERROR: "ksz_regmap_unlock" [drivers/net/dsa/microchip/ksz9477_i2c.ko] undefined!
>> ERROR: "ksz_regmap_lock" [drivers/net/dsa/microchip/ksz9477_i2c.ko] undefined!
> 
> So the test is to compile it as a module ?

The test is to compile it in all relevant possible configurations.

As a module, statically, and with dependent modules both static and
modular as is relevant.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-10 18:25 [PATCH 1/2] net: dsa: microchip: Do not reinit mutexes on KSZ87xx Marek Vasut
2019-10-10 18:25 ` [PATCH 2/2] net: dsa: microchip: Add shared regmap mutex Marek Vasut
2019-10-13  0:20   ` David Miller
2019-10-13  0:21     ` David Miller
2019-10-13 10:50       ` Marek Vasut
2019-10-13 16:26         ` David Miller
2019-10-13  0:20 ` [PATCH 1/2] net: dsa: microchip: Do not reinit mutexes on KSZ87xx David Miller

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git