[v3,02/10] mtd: powernv_flash: Lock around concurrent access to OPAL

Message ID 20170712042304.19745-3-cyrilbur@gmail.com
State Superseded
Headers show

Commit Message

Cyril Bur July 12, 2017, 4:22 a.m.
OPAL can only manage one flash access at a time and will return an
OPAL_BUSY error for each concurrent access to the flash. The simplest
way to prevent this from happening is with a mutex.

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 drivers/mtd/devices/powernv_flash.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

Comments

Balbir Singh July 17, 2017, 7:34 a.m. | #1
On Wed, 2017-07-12 at 14:22 +1000, Cyril Bur wrote:
> OPAL can only manage one flash access at a time and will return an
> OPAL_BUSY error for each concurrent access to the flash. The simplest
> way to prevent this from happening is with a mutex.
> 
> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> ---

Should the mutex_lock() be mutex_lock_interruptible()? Are we OK waiting on
the mutex while other operations with the lock are busy?

Balbir Singh.
Cyril Bur July 17, 2017, 7:55 a.m. | #2
On Mon, 2017-07-17 at 17:34 +1000, Balbir Singh wrote:
> On Wed, 2017-07-12 at 14:22 +1000, Cyril Bur wrote:
> > OPAL can only manage one flash access at a time and will return an
> > OPAL_BUSY error for each concurrent access to the flash. The simplest
> > way to prevent this from happening is with a mutex.
> > 
> > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> > ---
> 
> Should the mutex_lock() be mutex_lock_interruptible()? Are we OK waiting on
> the mutex while other operations with the lock are busy?
> 

This is a good question. My best interpretation is that
_interruptible() should be used when you'll only be coming from a user
context. Which is mostly true for this driver, however, MTD does
provide kernel interfaces, so I was hesitant, there isn't a great deal
of use of _interruptible() in drivers/mtd. 

Thoughts?

Cyril

> Balbir Singh.
>
Balbir Singh July 17, 2017, 9:29 a.m. | #3
On Mon, 2017-07-17 at 17:55 +1000, Cyril Bur wrote:
> On Mon, 2017-07-17 at 17:34 +1000, Balbir Singh wrote:
> > On Wed, 2017-07-12 at 14:22 +1000, Cyril Bur wrote:
> > > OPAL can only manage one flash access at a time and will return an
> > > OPAL_BUSY error for each concurrent access to the flash. The simplest
> > > way to prevent this from happening is with a mutex.
> > > 
> > > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> > > ---
> > 
> > Should the mutex_lock() be mutex_lock_interruptible()? Are we OK waiting on
> > the mutex while other operations with the lock are busy?
> > 
> 
> This is a good question. My best interpretation is that
> _interruptible() should be used when you'll only be coming from a user
> context. Which is mostly true for this driver, however, MTD does
> provide kernel interfaces, so I was hesitant, there isn't a great deal
> of use of _interruptible() in drivers/mtd. 
> 
> Thoughts?

What are the kernel interfaces (I have not read through mtd in detail)?
I would still like to see us not blocked in mutex_lock() across threads
for parallel calls, one option is to use mutex_trylock() and return if
someone already holds the mutex with -EBUSY, but you'll need to evaluate
what that means for every call.

Balbir Singh.
Cyril Bur July 18, 2017, 1:14 a.m. | #4
On Mon, 2017-07-17 at 19:29 +1000, Balbir Singh wrote:
> On Mon, 2017-07-17 at 17:55 +1000, Cyril Bur wrote:
> > On Mon, 2017-07-17 at 17:34 +1000, Balbir Singh wrote:
> > > On Wed, 2017-07-12 at 14:22 +1000, Cyril Bur wrote:
> > > > OPAL can only manage one flash access at a time and will return an
> > > > OPAL_BUSY error for each concurrent access to the flash. The simplest
> > > > way to prevent this from happening is with a mutex.
> > > > 
> > > > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> > > > ---
> > > 
> > > Should the mutex_lock() be mutex_lock_interruptible()? Are we OK waiting on
> > > the mutex while other operations with the lock are busy?
> > > 
> > 
> > This is a good question. My best interpretation is that
> > _interruptible() should be used when you'll only be coming from a user
> > context. Which is mostly true for this driver, however, MTD does
> > provide kernel interfaces, so I was hesitant, there isn't a great deal
> > of use of _interruptible() in drivers/mtd. 
> > 
> > Thoughts?
> 
> What are the kernel interfaces (I have not read through mtd in detail)?
> I would still like to see us not blocked in mutex_lock() across threads
> for parallel calls, one option is to use mutex_trylock() and return if
> someone already holds the mutex with -EBUSY, but you'll need to evaluate
> what that means for every call.
> 

Yeah maybe mutex_trylock() is the way to go, thinking quickly, I don't
see how it could be a problem for userspace using powernv_flash. I'm
honestly not too sure about the depths of the mtd kernel interfaces but
I've seen a tonne of cool stuff you could do, hence my reluctance to go
with _interruptible()

Cyril
> Balbir Singh.
>
Michael Ellerman July 18, 2017, 3:12 a.m. | #5
Cyril Bur <cyrilbur@gmail.com> writes:

> On Mon, 2017-07-17 at 19:29 +1000, Balbir Singh wrote:
>> On Mon, 2017-07-17 at 17:55 +1000, Cyril Bur wrote:
>> > On Mon, 2017-07-17 at 17:34 +1000, Balbir Singh wrote:
>> > > On Wed, 2017-07-12 at 14:22 +1000, Cyril Bur wrote:
>> > > > OPAL can only manage one flash access at a time and will return an
>> > > > OPAL_BUSY error for each concurrent access to the flash. The simplest
>> > > > way to prevent this from happening is with a mutex.
>> > > > 
>> > > > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
>> > > > ---
>> > > 
>> > > Should the mutex_lock() be mutex_lock_interruptible()? Are we OK waiting on
>> > > the mutex while other operations with the lock are busy?
>> > > 
>> > 
>> > This is a good question. My best interpretation is that
>> > _interruptible() should be used when you'll only be coming from a user
>> > context. Which is mostly true for this driver, however, MTD does
>> > provide kernel interfaces, so I was hesitant, there isn't a great deal
>> > of use of _interruptible() in drivers/mtd. 
>> > 
>> > Thoughts?
>> 
>> What are the kernel interfaces (I have not read through mtd in detail)?
>> I would still like to see us not blocked in mutex_lock() across threads
>> for parallel calls, one option is to use mutex_trylock() and return if
>> someone already holds the mutex with -EBUSY, but you'll need to evaluate
>> what that means for every call.
>
> Yeah maybe mutex_trylock() is the way to go, thinking quickly, I don't
> see how it could be a problem for userspace using powernv_flash. I'm
> honestly not too sure about the depths of the mtd kernel interfaces but
> I've seen a tonne of cool stuff you could do, hence my reluctance to go
> with _interruptible()

If you use trylock that means all your callers now need to handle EBUSY,
which I doubt they do. Which means it goes up to userspace, which most
users will just treat as a hard error.

So that sounds like a bad plan to me.

cheers

Patch

diff --git a/drivers/mtd/devices/powernv_flash.c b/drivers/mtd/devices/powernv_flash.c
index a9a20c00687c..7b41af06f4fe 100644
--- a/drivers/mtd/devices/powernv_flash.c
+++ b/drivers/mtd/devices/powernv_flash.c
@@ -38,6 +38,7 @@ 
 
 struct powernv_flash {
 	struct mtd_info	mtd;
+	struct mutex lock;
 	u32 id;
 };
 
@@ -59,12 +60,15 @@  static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op,
 	dev_dbg(dev, "%s(op=%d, offset=0x%llx, len=%zu)\n",
 			__func__, op, offset, len);
 
+	mutex_lock(&info->lock);
+
 	token = opal_async_get_token_interruptible();
 	if (token < 0) {
 		if (token != -ERESTARTSYS)
 			dev_err(dev, "Failed to get an async token\n");
 
-		return token;
+		rc = token;
+		goto out;
 	}
 
 	switch (op) {
@@ -79,18 +83,21 @@  static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op,
 		break;
 	default:
 		WARN_ON_ONCE(1);
-		return -EIO;
+		rc = -EIO;
+		goto out;
 	}
 
 	if (rc != OPAL_ASYNC_COMPLETION) {
 		dev_err(dev, "opal_flash_async_op(op=%d) failed (rc %d)\n",
 				op, rc);
 		opal_async_release_token(token);
-		return -EIO;
+		rc = -EIO;
+		goto out;
 	}
 
 	rc = opal_async_wait_response(token, &msg);
 	opal_async_release_token(token);
+	mutex_unlock(&info->lock);
 	if (rc) {
 		dev_err(dev, "opal async wait failed (rc %d)\n", rc);
 		return -EIO;
@@ -106,6 +113,9 @@  static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op,
 	}
 
 	return rc;
+out:
+	mutex_unlock(&info->lock);
+	return rc;
 }
 
 /**
@@ -237,6 +247,8 @@  static int powernv_flash_probe(struct platform_device *pdev)
 	if (ret)
 		goto out;
 
+	mutex_init(&data->lock);
+
 	dev_set_drvdata(dev, data);
 
 	/*