[PATCH linux dev-5.8] fsi: Aspeed: Add mutex to protect HW access

[Milton Miller II]

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 at 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 at us.ibm.com>

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