Use new suspend lock handling for this special case for concatenated
MTD devices.
Fixes: 013e6292aaf5 ("mtd: rawnand: Simplify the locking")
Signed-off-by: Sean Nyekjaer <[email protected]>
---
drivers/mtd/mtdconcat.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c
index f685a581df48..c497c851481f 100644
--- a/drivers/mtd/mtdconcat.c
+++ b/drivers/mtd/mtdconcat.c
@@ -561,25 +561,32 @@ static void concat_sync(struct mtd_info *mtd)
static int concat_suspend(struct mtd_info *mtd)
{
+ struct mtd_info *master = mtd_get_master(mtd);
struct mtd_concat *concat = CONCAT(mtd);
int i, rc = 0;
for (i = 0; i < concat->num_subdev; i++) {
struct mtd_info *subdev = concat->subdev[i];
- if ((rc = mtd_suspend(subdev)) < 0)
+
+ down_write(&master->master.suspend_lock);
+ if ((rc = __mtd_suspend(subdev)) < 0)
return rc;
+ up_write(&master->master.suspend_lock);
}
return rc;
}
static void concat_resume(struct mtd_info *mtd)
{
+ struct mtd_info *master = mtd_get_master(mtd);
struct mtd_concat *concat = CONCAT(mtd);
int i;
for (i = 0; i < concat->num_subdev; i++) {
struct mtd_info *subdev = concat->subdev[i];
- mtd_resume(subdev);
+ down_write(&master->master.suspend_lock);
+ __mtd_resume(subdev);
+ up_write(&master->master.suspend_lock);
}
}
--
2.33.0
On Mon, 11 Oct 2021 13:52:53 +0200
Sean Nyekjaer <[email protected]> wrote:
> Use new suspend lock handling for this special case for concatenated
> MTD devices.
>
> Fixes: 013e6292aaf5 ("mtd: rawnand: Simplify the locking")
> Signed-off-by: Sean Nyekjaer <[email protected]>
> ---
> drivers/mtd/mtdconcat.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c
> index f685a581df48..c497c851481f 100644
> --- a/drivers/mtd/mtdconcat.c
> +++ b/drivers/mtd/mtdconcat.c
> @@ -561,25 +561,32 @@ static void concat_sync(struct mtd_info *mtd)
>
> static int concat_suspend(struct mtd_info *mtd)
> {
> + struct mtd_info *master = mtd_get_master(mtd);
> struct mtd_concat *concat = CONCAT(mtd);
> int i, rc = 0;
>
> for (i = 0; i < concat->num_subdev; i++) {
> struct mtd_info *subdev = concat->subdev[i];
> - if ((rc = mtd_suspend(subdev)) < 0)
> +
> + down_write(&master->master.suspend_lock);
> + if ((rc = __mtd_suspend(subdev)) < 0)
> return rc;
> + up_write(&master->master.suspend_lock);
> }
> return rc;
> }
>
> static void concat_resume(struct mtd_info *mtd)
> {
> + struct mtd_info *master = mtd_get_master(mtd);
> struct mtd_concat *concat = CONCAT(mtd);
> int i;
>
> for (i = 0; i < concat->num_subdev; i++) {
> struct mtd_info *subdev = concat->subdev[i];
> - mtd_resume(subdev);
> + down_write(&master->master.suspend_lock);
> + __mtd_resume(subdev);
> + up_write(&master->master.suspend_lock);
> }
> }
>
Why do we need to implement the _suspend/_resume() hooks here? The
underlying MTD devices should be suspended at some point (when the
class ->suspend() method is called on those device), and there's
nothing mtdconcat-specific to do here. Looks like implementing this
suspend-all-subdevs loop results in calling mtd->_suspend()/_resume()
twice, which is useless. The only issue I see is if the subdevices
haven't been registered to the device model, but that happens, I
believe we have bigger issues (those devices won't be suspended when
mtdconcat is not used).
On Mon, 11 Oct 2021 15:15:01 +0200
Boris Brezillon <[email protected]> wrote:
> On Mon, 11 Oct 2021 13:52:53 +0200
> Sean Nyekjaer <[email protected]> wrote:
>
> > Use new suspend lock handling for this special case for concatenated
> > MTD devices.
> >
> > Fixes: 013e6292aaf5 ("mtd: rawnand: Simplify the locking")
> > Signed-off-by: Sean Nyekjaer <[email protected]>
> > ---
> > drivers/mtd/mtdconcat.c | 11 +++++++++--
> > 1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c
> > index f685a581df48..c497c851481f 100644
> > --- a/drivers/mtd/mtdconcat.c
> > +++ b/drivers/mtd/mtdconcat.c
> > @@ -561,25 +561,32 @@ static void concat_sync(struct mtd_info *mtd)
> >
> > static int concat_suspend(struct mtd_info *mtd)
> > {
> > + struct mtd_info *master = mtd_get_master(mtd);
> > struct mtd_concat *concat = CONCAT(mtd);
> > int i, rc = 0;
> >
> > for (i = 0; i < concat->num_subdev; i++) {
> > struct mtd_info *subdev = concat->subdev[i];
> > - if ((rc = mtd_suspend(subdev)) < 0)
> > +
> > + down_write(&master->master.suspend_lock);
> > + if ((rc = __mtd_suspend(subdev)) < 0)
> > return rc;
> > + up_write(&master->master.suspend_lock);
> > }
> > return rc;
> > }
> >
> > static void concat_resume(struct mtd_info *mtd)
> > {
> > + struct mtd_info *master = mtd_get_master(mtd);
> > struct mtd_concat *concat = CONCAT(mtd);
> > int i;
> >
> > for (i = 0; i < concat->num_subdev; i++) {
> > struct mtd_info *subdev = concat->subdev[i];
> > - mtd_resume(subdev);
> > + down_write(&master->master.suspend_lock);
> > + __mtd_resume(subdev);
> > + up_write(&master->master.suspend_lock);
> > }
> > }
> >
>
> Why do we need to implement the _suspend/_resume() hooks here? The
> underlying MTD devices should be suspended at some point (when the
> class ->suspend() method is called on those device), and there's
> nothing mtdconcat-specific to do here. Looks like implementing this
> suspend-all-subdevs loop results in calling mtd->_suspend()/_resume()
> twice, which is useless. The only issue I see is if the subdevices
> haven't been registered to the device model, but that happens, I
> believe we have bigger issues (those devices won't be suspended when
> mtdconcat is not used).
Uh, just had a look at mtd_concat_create() callers, and they indeed
don't register the subdevices, so I guess the suspend-all-subdevs loop
is needed. I really thought mtdconcat was something more generic
aggregating already registered devices...
On Mon, Oct 11, 2021 at 03:27:03PM +0200, Boris Brezillon wrote:
> On Mon, 11 Oct 2021 15:15:01 +0200
> Boris Brezillon <[email protected]> wrote:
>
> > On Mon, 11 Oct 2021 13:52:53 +0200
> > Sean Nyekjaer <[email protected]> wrote:
> >
> > > Use new suspend lock handling for this special case for concatenated
> > > MTD devices.
> > >
> > > Fixes: 013e6292aaf5 ("mtd: rawnand: Simplify the locking")
> > > Signed-off-by: Sean Nyekjaer <[email protected]>
> > > ---
> > > drivers/mtd/mtdconcat.c | 11 +++++++++--
> > > 1 file changed, 9 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c
> > > index f685a581df48..c497c851481f 100644
> > > --- a/drivers/mtd/mtdconcat.c
> > > +++ b/drivers/mtd/mtdconcat.c
> > > @@ -561,25 +561,32 @@ static void concat_sync(struct mtd_info *mtd)
> > >
> > > static int concat_suspend(struct mtd_info *mtd)
> > > {
> > > + struct mtd_info *master = mtd_get_master(mtd);
> > > struct mtd_concat *concat = CONCAT(mtd);
> > > int i, rc = 0;
> > >
> > > for (i = 0; i < concat->num_subdev; i++) {
> > > struct mtd_info *subdev = concat->subdev[i];
> > > - if ((rc = mtd_suspend(subdev)) < 0)
> > > +
> > > + down_write(&master->master.suspend_lock);
> > > + if ((rc = __mtd_suspend(subdev)) < 0)
> > > return rc;
> > > + up_write(&master->master.suspend_lock);
> > > }
> > > return rc;
> > > }
> > >
> > > static void concat_resume(struct mtd_info *mtd)
> > > {
> > > + struct mtd_info *master = mtd_get_master(mtd);
> > > struct mtd_concat *concat = CONCAT(mtd);
> > > int i;
> > >
> > > for (i = 0; i < concat->num_subdev; i++) {
> > > struct mtd_info *subdev = concat->subdev[i];
> > > - mtd_resume(subdev);
> > > + down_write(&master->master.suspend_lock);
> > > + __mtd_resume(subdev);
> > > + up_write(&master->master.suspend_lock);
> > > }
> > > }
> > >
> >
> > Why do we need to implement the _suspend/_resume() hooks here? The
> > underlying MTD devices should be suspended at some point (when the
> > class ->suspend() method is called on those device), and there's
> > nothing mtdconcat-specific to do here. Looks like implementing this
> > suspend-all-subdevs loop results in calling mtd->_suspend()/_resume()
> > twice, which is useless. The only issue I see is if the subdevices
> > haven't been registered to the device model, but that happens, I
> > believe we have bigger issues (those devices won't be suspended when
> > mtdconcat is not used).
>
>
> Uh, just had a look at mtd_concat_create() callers, and they indeed
> don't register the subdevices, so I guess the suspend-all-subdevs loop
> is needed. I really thought mtdconcat was something more generic
> aggregating already registered devices...
Hi Boris,
Cool, mtd_concat should be seen as mtd devices concatenated? Could be
spi-nors and rawnand. So _suspend() needs to be called for every device
layer?
From what I see here, mtd_suspend()/mtd_resume() is called for every mtd
device. Before this patch mtd_suspend() would only have effect on the
first device as master->master.suspended is set and then calls to
device specific _suspend() is skipped.
Correct?
/Sean
On Mon, 11 Oct 2021 15:35:56 +0200
Sean Nyekjaer <[email protected]> wrote:
> On Mon, Oct 11, 2021 at 03:27:03PM +0200, Boris Brezillon wrote:
> > On Mon, 11 Oct 2021 15:15:01 +0200
> > Boris Brezillon <[email protected]> wrote:
> >
> > > On Mon, 11 Oct 2021 13:52:53 +0200
> > > Sean Nyekjaer <[email protected]> wrote:
> > >
> > > > Use new suspend lock handling for this special case for concatenated
> > > > MTD devices.
> > > >
> > > > Fixes: 013e6292aaf5 ("mtd: rawnand: Simplify the locking")
> > > > Signed-off-by: Sean Nyekjaer <[email protected]>
> > > > ---
> > > > drivers/mtd/mtdconcat.c | 11 +++++++++--
> > > > 1 file changed, 9 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c
> > > > index f685a581df48..c497c851481f 100644
> > > > --- a/drivers/mtd/mtdconcat.c
> > > > +++ b/drivers/mtd/mtdconcat.c
> > > > @@ -561,25 +561,32 @@ static void concat_sync(struct mtd_info *mtd)
> > > >
> > > > static int concat_suspend(struct mtd_info *mtd)
> > > > {
> > > > + struct mtd_info *master = mtd_get_master(mtd);
> > > > struct mtd_concat *concat = CONCAT(mtd);
> > > > int i, rc = 0;
> > > >
> > > > for (i = 0; i < concat->num_subdev; i++) {
> > > > struct mtd_info *subdev = concat->subdev[i];
> > > > - if ((rc = mtd_suspend(subdev)) < 0)
> > > > +
> > > > + down_write(&master->master.suspend_lock);
> > > > + if ((rc = __mtd_suspend(subdev)) < 0)
> > > > return rc;
> > > > + up_write(&master->master.suspend_lock);
> > > > }
> > > > return rc;
> > > > }
> > > >
> > > > static void concat_resume(struct mtd_info *mtd)
> > > > {
> > > > + struct mtd_info *master = mtd_get_master(mtd);
> > > > struct mtd_concat *concat = CONCAT(mtd);
> > > > int i;
> > > >
> > > > for (i = 0; i < concat->num_subdev; i++) {
> > > > struct mtd_info *subdev = concat->subdev[i];
> > > > - mtd_resume(subdev);
> > > > + down_write(&master->master.suspend_lock);
> > > > + __mtd_resume(subdev);
> > > > + up_write(&master->master.suspend_lock);
> > > > }
> > > > }
> > > >
> > >
> > > Why do we need to implement the _suspend/_resume() hooks here? The
> > > underlying MTD devices should be suspended at some point (when the
> > > class ->suspend() method is called on those device), and there's
> > > nothing mtdconcat-specific to do here. Looks like implementing this
> > > suspend-all-subdevs loop results in calling mtd->_suspend()/_resume()
> > > twice, which is useless. The only issue I see is if the subdevices
> > > haven't been registered to the device model, but that happens, I
> > > believe we have bigger issues (those devices won't be suspended when
> > > mtdconcat is not used).
> >
> >
> > Uh, just had a look at mtd_concat_create() callers, and they indeed
> > don't register the subdevices, so I guess the suspend-all-subdevs loop
> > is needed. I really thought mtdconcat was something more generic
> > aggregating already registered devices...
>
> Hi Boris,
>
> Cool, mtd_concat should be seen as mtd devices concatenated? Could be
> spi-nors and rawnand. So _suspend() needs to be called for every device
> layer?
If those subdevices were registered as MTD devices too, you wouldn't
have to call ->_suspend()/_resume() on the subdevices (the core does it
for you for every registered 'struct device' object).
>
> From what I see here, mtd_suspend()/mtd_resume() is called for every mtd
> device. Before this patch mtd_suspend() would only have effect on the
> first device as master->master.suspended is set and then calls to
> device specific _suspend() is skipped.
Unfortunately, subdevices are not exposed to the MTD framework, only the
aggregate is, so suspend is called once in that case, and the mtdconcat
layer has to propagate it to all its subdevices. This makes mtdconcat
unsuitable for a generic MTD aggregation layer BTW (which I remember
discussing with someone on the MTD ML in the past).
On Mon, 11 Oct 2021 13:52:53 +0200
Sean Nyekjaer <[email protected]> wrote:
> Use new suspend lock handling for this special case for concatenated
> MTD devices.
>
> Fixes: 013e6292aaf5 ("mtd: rawnand: Simplify the locking")
> Signed-off-by: Sean Nyekjaer <[email protected]>
> ---
> drivers/mtd/mtdconcat.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c
> index f685a581df48..c497c851481f 100644
> --- a/drivers/mtd/mtdconcat.c
> +++ b/drivers/mtd/mtdconcat.c
> @@ -561,25 +561,32 @@ static void concat_sync(struct mtd_info *mtd)
>
> static int concat_suspend(struct mtd_info *mtd)
> {
> + struct mtd_info *master = mtd_get_master(mtd);
> struct mtd_concat *concat = CONCAT(mtd);
> int i, rc = 0;
>
> for (i = 0; i < concat->num_subdev; i++) {
> struct mtd_info *subdev = concat->subdev[i];
> - if ((rc = mtd_suspend(subdev)) < 0)
> +
> + down_write(&master->master.suspend_lock);
You should definitely not take the concat lock here, the framework does
it for you, so all you'll get is a deadlock.
> + if ((rc = __mtd_suspend(subdev)) < 0)
> return rc;
You're returning with the lock held => DEADLOCK next time you try to
acquire it.
Anyway, as mentioned in my review of patch 1, I'd go for this ad-hoc
solution:
for (i = 0; i < concat->num_subdev; i++) {
rc = subdev->_suspend ? subdev->_suspend(subdev) : 0;
if (rc < 0)
return rc;
}
return 0;
> + up_write(&master->master.suspend_lock);
> }
> return rc;
> }
>
> static void concat_resume(struct mtd_info *mtd)
> {
> + struct mtd_info *master = mtd_get_master(mtd);
> struct mtd_concat *concat = CONCAT(mtd);
> int i;
>
> for (i = 0; i < concat->num_subdev; i++) {
> struct mtd_info *subdev = concat->subdev[i];
> - mtd_resume(subdev);
> + down_write(&master->master.suspend_lock);
> + __mtd_resume(subdev);
> + up_write(&master->master.suspend_lock);
> }
No down/up_write() needed:
for (i = 0; i < concat->num_subdev; i++) {
struct mtd_info *subdev = concat->subdev[i];
if (subdev->_resume)
subdev->_resume(subdev);
}
> }
>