Openbmc archive at lore.kernel.org
 help / color / Atom feed
* [PATCH linux dev-5.8] fsi: Aspeed: Add mutex to protect HW access
@ 2020-09-29 18:49 Eddie James
  2020-09-30 22:34 ` Milton Miller II
  0 siblings, 1 reply; 3+ messages in thread
From: Eddie James @ 2020-09-29 18:49 UTC (permalink / raw)
  To: openbmc; +Cc: Eddie James

There is nothing to prevent multiple commands being executed
simultaneously. Add a mutex to prevent this.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/fsi/fsi-master-aspeed.c | 48 ++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 18 deletions(-)

diff --git a/drivers/fsi/fsi-master-aspeed.c b/drivers/fsi/fsi-master-aspeed.c
index c006ec008a1a..c71d7e9a32b0 100644
--- a/drivers/fsi/fsi-master-aspeed.c
+++ b/drivers/fsi/fsi-master-aspeed.c
@@ -8,6 +8,7 @@
 #include <linux/io.h>
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
@@ -19,6 +20,7 @@
 
 struct fsi_master_aspeed {
 	struct fsi_master	master;
+	struct mutex		lock;	/* protect HW access */
 	struct device		*dev;
 	void __iomem		*base;
 	struct clk		*clk;
@@ -254,6 +256,8 @@ static int aspeed_master_read(struct fsi_master *master, int link,
 	addr |= id << 21;
 	addr += link * FSI_HUB_LINK_SIZE;
 
+	mutex_lock(&aspeed->lock);
+
 	switch (size) {
 	case 1:
 		ret = opb_readb(aspeed, fsi_base + addr, val);
@@ -265,14 +269,14 @@ static int aspeed_master_read(struct fsi_master *master, int link,
 		ret = opb_readl(aspeed, fsi_base + addr, val);
 		break;
 	default:
-		return -EINVAL;
+		ret = -EINVAL;
+		goto done;
 	}
 
 	ret = check_errors(aspeed, ret);
-	if (ret)
-		return ret;
-
-	return 0;
+done:
+	mutex_unlock(&aspeed->lock);
+	return ret;
 }
 
 static int aspeed_master_write(struct fsi_master *master, int link,
@@ -287,6 +291,8 @@ static int aspeed_master_write(struct fsi_master *master, int link,
 	addr |= id << 21;
 	addr += link * FSI_HUB_LINK_SIZE;
 
+	mutex_lock(&aspeed->lock);
+
 	switch (size) {
 	case 1:
 		ret = opb_writeb(aspeed, fsi_base + addr, *(u8 *)val);
@@ -298,14 +304,14 @@ static int aspeed_master_write(struct fsi_master *master, int link,
 		ret = opb_writel(aspeed, fsi_base + addr, *(__be32 *)val);
 		break;
 	default:
-		return -EINVAL;
+		ret = -EINVAL;
+		goto done;
 	}
 
 	ret = check_errors(aspeed, ret);
-	if (ret)
-		return ret;
-
-	return 0;
+done:
+	mutex_unlock(&aspeed->lock);
+	return ret;
 }
 
 static int aspeed_master_link_enable(struct fsi_master *master, int link,
@@ -317,20 +323,23 @@ static int aspeed_master_link_enable(struct fsi_master *master, int link,
 
 	idx = link / 32;
 	bit = link % 32;
-
 	reg = cpu_to_be32(0x80000000 >> bit);
 
-	if (!enable)
-		return opb_writel(aspeed, ctrl_base + FSI_MCENP0 + (4 * idx),
-				  reg);
+	mutex_lock(&aspeed->lock);
 
-	ret = opb_writel(aspeed, ctrl_base + FSI_MSENP0 + (4 * idx), reg);
+	if (!enable)
+		ret = opb_writel(aspeed, ctrl_base + FSI_MCENP0 + (4 * idx),
+				 reg);
+	else
+		ret = opb_writel(aspeed, ctrl_base + FSI_MSENP0 + (4 * idx),
+				 reg);
 	if (ret)
-		return ret;
+		goto done;
 
 	mdelay(FSI_LINK_ENABLE_SETUP_TIME);
-
-	return 0;
+done:
+	mutex_unlock(&aspeed->lock);
+	return ret;
 }
 
 static int aspeed_master_term(struct fsi_master *master, int link, uint8_t id)
@@ -431,9 +440,11 @@ static ssize_t cfam_reset_store(struct device *dev, struct device_attribute *att
 {
 	struct fsi_master_aspeed *aspeed = dev_get_drvdata(dev);
 
+	mutex_lock(&aspeed->lock);
 	gpiod_set_value(aspeed->cfam_reset_gpio, 1);
 	usleep_range(900, 1000);
 	gpiod_set_value(aspeed->cfam_reset_gpio, 0);
+	mutex_unlock(&aspeed->lock);
 
 	return count;
 }
@@ -597,6 +608,7 @@ static int fsi_master_aspeed_probe(struct platform_device *pdev)
 
 	dev_set_drvdata(&pdev->dev, aspeed);
 
+	mutex_init(&aspeed->lock);
 	aspeed_master_init(aspeed);
 
 	rc = fsi_master_register(&aspeed->master);
-- 
2.26.2


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

* Re: [PATCH linux dev-5.8] fsi: Aspeed: Add mutex to protect HW access
  2020-09-29 18:49 [PATCH linux dev-5.8] fsi: Aspeed: Add mutex to protect HW access Eddie James
@ 2020-09-30 22:34 ` Milton Miller II
  2020-10-07  5:47   ` Joel Stanley
  0 siblings, 1 reply; 3+ messages in thread
From: Milton Miller II @ 2020-09-30 22:34 UTC (permalink / raw)
  To: Eddie James, joel; +Cc: openbmc

On September 29, 2020 around 1:50PM in some timezone, Eddie James wrote:
>
>There is nothing to prevent multiple commands being executed
>simultaneously. Add a mutex to prevent this.
>
>Signed-off-by: Eddie James <eajames@linux.ibm.com>
>---
> drivers/fsi/fsi-master-aspeed.c | 48
>++++++++++++++++++++-------------
> 1 file changed, 30 insertions(+), 18 deletions(-)
>
>diff --git a/drivers/fsi/fsi-master-aspeed.c
>b/drivers/fsi/fsi-master-aspeed.c
>index c006ec008a1a..c71d7e9a32b0 100644
>--- a/drivers/fsi/fsi-master-aspeed.c
>+++ b/drivers/fsi/fsi-master-aspeed.c
>@@ -8,6 +8,7 @@
> #include <linux/io.h>
> #include <linux/mfd/syscon.h>
> #include <linux/module.h>
>+#include <linux/mutex.h>
> #include <linux/of.h>
> #include <linux/platform_device.h>
> #include <linux/regmap.h>
>@@ -19,6 +20,7 @@
> 
> struct fsi_master_aspeed {
> 	struct fsi_master	master;
>+	struct mutex		lock;	/* protect HW access */
> 	struct device		*dev;
> 	void __iomem		*base;
> 	struct clk		*clk;
>@@ -254,6 +256,8 @@ static int aspeed_master_read(struct fsi_master
>*master, int link,
> 	addr |= id << 21;
> 	addr += link * FSI_HUB_LINK_SIZE;
> 
>+	mutex_lock(&aspeed->lock);
>+
> 	switch (size) {
> 	case 1:
> 		ret = opb_readb(aspeed, fsi_base + addr, val);
>@@ -265,14 +269,14 @@ static int aspeed_master_read(struct fsi_master
>*master, int link,
> 		ret = opb_readl(aspeed, fsi_base + addr, val);
> 		break;
> 	default:
>-		return -EINVAL;
>+		ret = -EINVAL;
>+		goto done;
> 	}
> 
> 	ret = check_errors(aspeed, ret);
>-	if (ret)
>-		return ret;
>-
>-	return 0;
>+done:
>+	mutex_unlock(&aspeed->lock);
>+	return ret;
> }
> 
> static int aspeed_master_write(struct fsi_master *master, int link,
>@@ -287,6 +291,8 @@ static int aspeed_master_write(struct fsi_master
>*master, int link,
> 	addr |= id << 21;
> 	addr += link * FSI_HUB_LINK_SIZE;
> 
>+	mutex_lock(&aspeed->lock);
>+
> 	switch (size) {
> 	case 1:
> 		ret = opb_writeb(aspeed, fsi_base + addr, *(u8 *)val);
>@@ -298,14 +304,14 @@ static int aspeed_master_write(struct
>fsi_master *master, int link,
> 		ret = opb_writel(aspeed, fsi_base + addr, *(__be32 *)val);
> 		break;
> 	default:
>-		return -EINVAL;
>+		ret = -EINVAL;
>+		goto done;
> 	}
> 
> 	ret = check_errors(aspeed, ret);
>-	if (ret)
>-		return ret;
>-
>-	return 0;
>+done:
>+	mutex_unlock(&aspeed->lock);
>+	return ret;
> }
> 
> static int aspeed_master_link_enable(struct fsi_master *master, int
>link,
>@@ -317,20 +323,23 @@ static int aspeed_master_link_enable(struct
>fsi_master *master, int link,
> 
> 	idx = link / 32;
> 	bit = link % 32;
>-
> 	reg = cpu_to_be32(0x80000000 >> bit);
> 
>-	if (!enable)
>-		return opb_writel(aspeed, ctrl_base + FSI_MCENP0 + (4 * idx),
>-				  reg);
>+	mutex_lock(&aspeed->lock);
> 
>-	ret = opb_writel(aspeed, ctrl_base + FSI_MSENP0 + (4 * idx), reg);
>+	if (!enable)
>+		ret = opb_writel(aspeed, ctrl_base + FSI_MCENP0 + (4 * idx),
>+				 reg);
>+	else
>+		ret = opb_writel(aspeed, ctrl_base + FSI_MSENP0 + (4 * idx),
>+				 reg);
> 	if (ret)
>-		return ret;
>+		goto done;
> 
> 	mdelay(FSI_LINK_ENABLE_SETUP_TIME);
>-
>-	return 0;
>+done:
>+	mutex_unlock(&aspeed->lock);
>+	return ret;
> }
> 
> static int aspeed_master_term(struct fsi_master *master, int link,
>uint8_t id)
>@@ -431,9 +440,11 @@ static ssize_t cfam_reset_store(struct device
>*dev, struct device_attribute *att
> {
> 	struct fsi_master_aspeed *aspeed = dev_get_drvdata(dev);
> 
>+	mutex_lock(&aspeed->lock);
> 	gpiod_set_value(aspeed->cfam_reset_gpio, 1);
> 	usleep_range(900, 1000);
> 	gpiod_set_value(aspeed->cfam_reset_gpio, 0);
>+	mutex_unlock(&aspeed->lock);
> 
> 	return count;
> }
>@@ -597,6 +608,7 @@ static int fsi_master_aspeed_probe(struct
>platform_device *pdev)
> 
> 	dev_set_drvdata(&pdev->dev, aspeed);
> 
>+	mutex_init(&aspeed->lock);
> 	aspeed_master_init(aspeed);

So you initialize the lock here in the probe function before its 
used, good.  I notice its not taken in aspeed_master_init nor over 
the opb_read for the version register.  Both are called from the 
probe function and presumably are therefore the sole context 
available, but having it taken could be considered a good for 
consistency.

Are there any concerns that this is part of the fsi master 
context if we wanted to use both fsi buses in the future?

Reviewed-by: Milton Miller <miltonm@us.ibm.com>

> 
> 	rc = fsi_master_register(&aspeed->master);
>-- 
>2.26.2
>
>


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

* Re: [PATCH linux dev-5.8] fsi: Aspeed: Add mutex to protect HW access
  2020-09-30 22:34 ` Milton Miller II
@ 2020-10-07  5:47   ` Joel Stanley
  0 siblings, 0 replies; 3+ messages in thread
From: Joel Stanley @ 2020-10-07  5:47 UTC (permalink / raw)
  To: Milton Miller II, Eddie James; +Cc: OpenBMC Maillist

On Wed, 30 Sep 2020 at 22:34, Milton Miller II <miltonm@us.ibm.com> wrote:
>
> On September 29, 2020 around 1:50PM in some timezone, Eddie James wrote:
> >
> >There is nothing to prevent multiple commands being executed
> >simultaneously. Add a mutex to prevent this.
> >
> >Signed-off-by: Eddie James <eajames@linux.ibm.com>

> >@@ -597,6 +608,7 @@ static int fsi_master_aspeed_probe(struct
> >platform_device *pdev)
> >
> >       dev_set_drvdata(&pdev->dev, aspeed);
> >
> >+      mutex_init(&aspeed->lock);
> >       aspeed_master_init(aspeed);
>
> So you initialize the lock here in the probe function before its
> used, good.  I notice its not taken in aspeed_master_init nor over
> the opb_read for the version register.  Both are called from the
> probe function and presumably are therefore the sole context
> available, but having it taken could be considered a good for
> consistency.
>
> Are there any concerns that this is part of the fsi master
> context if we wanted to use both fsi buses in the future?

If we use the other FSI master, then it would have a second instance
of the driver so we would be fine.

If/when we add support for the second OPB bus the driver will need a
overhaul, so reworking the locking will form part of that work.

> Reviewed-by: Milton Miller <miltonm@us.ibm.com>

Reviewed-by: Joel Stanley <joel@jms.id.au>
Fixes: 606397d67f41 ("fsi: Add ast2600 master driver")

I have merged this to the openbmc tree.

Eddie, I know this was written to fix the sbe fifo usage. Did you have
a (simplified) test workload that showed the bug that you could share?

Please send this to the upstream lists with the r-b and fixes tags.

Cheers,

Joel

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

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-29 18:49 [PATCH linux dev-5.8] fsi: Aspeed: Add mutex to protect HW access Eddie James
2020-09-30 22:34 ` Milton Miller II
2020-10-07  5:47   ` Joel Stanley

Openbmc archive at lore.kernel.org

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

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

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.ozlabs.lists.openbmc


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