| Message ID | 20211011115253.38497-4-sean@geanix.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | mtd: core: protect access to mtd devices while in suspend | expand |
On Mon, 11 Oct 2021 13:52:53 +0200 Sean Nyekjaer <sean@geanix.com> 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 <sean@geanix.com> > --- > 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 <boris.brezillon@collabora.com> wrote: > On Mon, 11 Oct 2021 13:52:53 +0200 > Sean Nyekjaer <sean@geanix.com> 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 <sean@geanix.com> > > --- > > 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 <boris.brezillon@collabora.com> wrote: > > > On Mon, 11 Oct 2021 13:52:53 +0200 > > Sean Nyekjaer <sean@geanix.com> 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 <sean@geanix.com> > > > --- > > > 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 <sean@geanix.com> 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 <boris.brezillon@collabora.com> wrote: > > > > > On Mon, 11 Oct 2021 13:52:53 +0200 > > > Sean Nyekjaer <sean@geanix.com> 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 <sean@geanix.com> > > > > --- > > > > 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 <sean@geanix.com> 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 <sean@geanix.com> > --- > 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); } > } >
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); } }
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 <sean@geanix.com> --- drivers/mtd/mtdconcat.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)