* [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 related [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, other threads:[~2020-10-07 5:49 UTC | newest]
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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).