| Message ID | 20170629065413.19845-6-cyrilbur@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, Jun 29, 2017 at 04:54:13PM +1000, Cyril Bur wrote: > The OPAL calls performed in this driver shouldn't be using > opal_async_wait_response() as this performs a wait_event() which, on > long running OPAL calls could result in hung task warnings. wait_event() > also prevents timely signal delivery which is also undesirable. > > This patch also attempts to quieten down the use of dev_err() when > errors haven't actually occurred and also to return better information up > the stack rather than always -EIO. > > Signed-off-by: Cyril Bur <cyrilbur@gmail.com> > --- > drivers/mtd/devices/powernv_flash.c | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) Seems OK: Acked-by: Brian Norris <computersforpeace@gmail.com>
On Thu, 2017-06-29 at 16:54 +1000, Cyril Bur wrote: > The OPAL calls performed in this driver shouldn't be using > opal_async_wait_response() as this performs a wait_event() which, on > long running OPAL calls could result in hung task warnings. wait_event() > also prevents timely signal delivery which is also undesirable. > > This patch also attempts to quieten down the use of dev_err() when > errors haven't actually occurred and also to return better information up > the stack rather than always -EIO. So what happens when you get a signal then ? The async op is still in progress, will the callers properly handle that ? For example, for a read, OPAL will potentially still be writing to the destination buffer. Are the callers expecting this ? That sounds very broken to me .... You should simply limit the amount of flash that can be affected by a single call to avoid long hung tasks. > Signed-off-by: Cyril Bur <cyrilbur@gmail.com> > --- > drivers/mtd/devices/powernv_flash.c | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/drivers/mtd/devices/powernv_flash.c b/drivers/mtd/devices/powernv_flash.c > index 7b41af06f4fe..a7c5ae2b2898 100644 > --- a/drivers/mtd/devices/powernv_flash.c > +++ b/drivers/mtd/devices/powernv_flash.c > @@ -88,19 +88,23 @@ static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op, > } > > if (rc != OPAL_ASYNC_COMPLETION) { > - dev_err(dev, "opal_flash_async_op(op=%d) failed (rc %d)\n", > - op, rc); > + if (rc != OPAL_BUSY) > + dev_err(dev, "opal_flash_async_op(op=%d) failed (rc %d)\n", > + op, rc); > opal_async_release_token(token); > - rc = -EIO; > + rc = opal_error_code(rc); > goto out; > } > > - rc = opal_async_wait_response(token, &msg); > + rc = opal_async_wait_response_interruptible(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; > + if (rc == -ERESTARTSYS) > + rc = -EINTR; > + else > + dev_err(dev, "opal async wait failed (rc %d)\n", rc); > + return rc; > } > > rc = opal_get_async_rc(msg); > @@ -109,7 +113,7 @@ static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op, > if (retlen) > *retlen = len; > } else { > - rc = -EIO; > + rc = opal_error_code(rc); > } > > return rc;
On Sat, 2017-07-08 at 14:59 -0500, Benjamin Herrenschmidt wrote: > On Thu, 2017-06-29 at 16:54 +1000, Cyril Bur wrote: > > The OPAL calls performed in this driver shouldn't be using > > opal_async_wait_response() as this performs a wait_event() which, on > > long running OPAL calls could result in hung task warnings. wait_event() > > also prevents timely signal delivery which is also undesirable. > > > > This patch also attempts to quieten down the use of dev_err() when > > errors haven't actually occurred and also to return better information up > > the stack rather than always -EIO. > > So what happens when you get a signal then ? The async op is still in > progress, will the callers properly handle that ? > > For example, for a read, OPAL will potentially still be writing to > the destination buffer. Are the callers expecting this ? > > That sounds very broken to me .... > Correct - I have a v2 that I let stew in my head over the weekend before sending. I think its good. Thanks, Cyril > You should simply limit the amount of flash that can be affected by > a single call to avoid long hung tasks. > > > Signed-off-by: Cyril Bur <cyrilbur@gmail.com> > > --- > > drivers/mtd/devices/powernv_flash.c | 18 +++++++++++------- > > 1 file changed, 11 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/mtd/devices/powernv_flash.c b/drivers/mtd/devices/powernv_flash.c > > index 7b41af06f4fe..a7c5ae2b2898 100644 > > --- a/drivers/mtd/devices/powernv_flash.c > > +++ b/drivers/mtd/devices/powernv_flash.c > > @@ -88,19 +88,23 @@ static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op, > > } > > > > if (rc != OPAL_ASYNC_COMPLETION) { > > - dev_err(dev, "opal_flash_async_op(op=%d) failed (rc %d)\n", > > - op, rc); > > + if (rc != OPAL_BUSY) > > + dev_err(dev, "opal_flash_async_op(op=%d) failed (rc %d)\n", > > + op, rc); > > opal_async_release_token(token); > > - rc = -EIO; > > + rc = opal_error_code(rc); > > goto out; > > } > > > > - rc = opal_async_wait_response(token, &msg); > > + rc = opal_async_wait_response_interruptible(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; > > + if (rc == -ERESTARTSYS) > > + rc = -EINTR; > > + else > > + dev_err(dev, "opal async wait failed (rc %d)\n", rc); > > + return rc; > > } > > > > rc = opal_get_async_rc(msg); > > @@ -109,7 +113,7 @@ static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op, > > if (retlen) > > *retlen = len; > > } else { > > - rc = -EIO; > > + rc = opal_error_code(rc); > > } > > > > return rc;
diff --git a/drivers/mtd/devices/powernv_flash.c b/drivers/mtd/devices/powernv_flash.c index 7b41af06f4fe..a7c5ae2b2898 100644 --- a/drivers/mtd/devices/powernv_flash.c +++ b/drivers/mtd/devices/powernv_flash.c @@ -88,19 +88,23 @@ static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op, } if (rc != OPAL_ASYNC_COMPLETION) { - dev_err(dev, "opal_flash_async_op(op=%d) failed (rc %d)\n", - op, rc); + if (rc != OPAL_BUSY) + dev_err(dev, "opal_flash_async_op(op=%d) failed (rc %d)\n", + op, rc); opal_async_release_token(token); - rc = -EIO; + rc = opal_error_code(rc); goto out; } - rc = opal_async_wait_response(token, &msg); + rc = opal_async_wait_response_interruptible(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; + if (rc == -ERESTARTSYS) + rc = -EINTR; + else + dev_err(dev, "opal async wait failed (rc %d)\n", rc); + return rc; } rc = opal_get_async_rc(msg); @@ -109,7 +113,7 @@ static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op, if (retlen) *retlen = len; } else { - rc = -EIO; + rc = opal_error_code(rc); } return rc;
The OPAL calls performed in this driver shouldn't be using opal_async_wait_response() as this performs a wait_event() which, on long running OPAL calls could result in hung task warnings. wait_event() also prevents timely signal delivery which is also undesirable. This patch also attempts to quieten down the use of dev_err() when errors haven't actually occurred and also to return better information up the stack rather than always -EIO. Signed-off-by: Cyril Bur <cyrilbur@gmail.com> --- drivers/mtd/devices/powernv_flash.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)