Hang on reboot in nand_get_device()

Hang on reboot in nand_get_device()

[Andrew E. Mileski]

I'm experiencing a hang on reboot with a Freescale P1022 PowerPC system, with a
dual chip-select NAND part (specified in the device tree as two separate 
devices), and kernel v4.0.6.

It appears to be a hang in mtd/nand/nand_base.c:nand_get_device() waiting on
chip->controller->active.

Shouldn't nand_shutdown(), or perhaps a special case in nand_get_device() for
FL_SHUTDOWN, set chip->controller->active = NULL before returning?

This seems to fix the problem for me, but I don't know all the code well enough
to know whether doing so is appropriate, or sufficient.

~~Andrew E. Mileski

Re: Hang on reboot in nand_get_device()

[Brian Norris]

+ others

Hi Andrew,

Sorry for the delay here. I overlooked this.

On Thu, Jul 02, 2015 at 03:21:48PM -0400, Andrew E. Mileski wrote:
> I'm experiencing a hang on reboot with a Freescale P1022 PowerPC system, with a
> dual chip-select NAND part (specified in the device tree as two
> separate devices), and kernel v4.0.6.

Which driver?

> It appears to be a hang in mtd/nand/nand_base.c:nand_get_device() waiting on
> chip->controller->active.
> 
> Shouldn't nand_shutdown(), or perhaps a special case in nand_get_device() for
> FL_SHUTDOWN, set chip->controller->active = NULL before returning?

I don't think that's exactly the right solution, but you're in the right
ballpark I expect.

> This seems to fix the problem for me, but I don't know all the code well enough
> to know whether doing so is appropriate, or sufficient.

I'm going to guess you're seeing two reboot handlers trying to 'get' the
same controller structure. We could probably confirm that if we see your
driver.

But I see this could be a problem with a wide class of drivers.
Basically, any NAND driver that has multiple NAND chips attached will
see multiple reboot handlers that point at the same controller lock.
This will obviously deadlock, since only one of the chips will make it
through the nand_shutdown() function successfully.

And now that I begin describing the problem and grepping through the
source logs... I see that this problem was already resolved back in
2009, except for the FL_PM_SUSPENDED mode:

commit 6b0d9a84124937f048bcb8b21313152b23063978
Author: Li Yang <leoli@freescale.com>
Date:   Tue Nov 17 14:45:49 2009 -0800

    mtd: nand: fix multi-chip suspend problem

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=6b0d9a84124937f048bcb8b21313152b23063978
    
I actually don't see why we can't just use
nand_get_device(FL_PM_SUSPENDED) for the shutdown/reboot case, like
this:

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index cc74142938b0..ece544efccc3 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3110,7 +3110,7 @@ static void nand_resume(struct mtd_info *mtd)
  */
 static void nand_shutdown(struct mtd_info *mtd)
 {
-	nand_get_device(mtd, FL_SHUTDOWN);
+	nand_get_device(mtd, FL_PM_SUSPENDED);
 }
 
 /* Set default functions */


It's also possible that this could be better solved in a proper
refactor/rewrite of the NAND subsystem using a better controller/chip
split, so there's only one reboot handler per NAND controller. Boris has
been looking at that.

Anyway, if the above looks OK, I can send a proper patch and get your
'Tested-by's.

Brian

Re: Hang on reboot in nand_get_device()

[Boris Brezillon]

Hi Brian,

On Fri, 6 Nov 2015 10:00:52 -0800
Brian Norris <computersforpeace@gmail.com> wrote:

> + others
> 
> Hi Andrew,
> 
> Sorry for the delay here. I overlooked this.
> 
> On Thu, Jul 02, 2015 at 03:21:48PM -0400, Andrew E. Mileski wrote:
> > I'm experiencing a hang on reboot with a Freescale P1022 PowerPC system, with a
> > dual chip-select NAND part (specified in the device tree as two
> > separate devices), and kernel v4.0.6.
> 
> Which driver?
> 
> > It appears to be a hang in mtd/nand/nand_base.c:nand_get_device() waiting on
> > chip->controller->active.
> > 
> > Shouldn't nand_shutdown(), or perhaps a special case in nand_get_device() for
> > FL_SHUTDOWN, set chip->controller->active = NULL before returning?
> 
> I don't think that's exactly the right solution, but you're in the right
> ballpark I expect.

Hm, actually I find the idea of releasing the NAND controller when the
flash is set in FL_SHUTDOWN state not that bad. Is there any reason
we would want the NAND chip to stay active when we're shutting the
system down.
I understand that this is needed for the suspended case because we want
the system to restore the correct state when resuming (ie marking the
right device as active), but shutdown should be simpler.

> 
> > This seems to fix the problem for me, but I don't know all the code well enough
> > to know whether doing so is appropriate, or sufficient.
> 
> I'm going to guess you're seeing two reboot handlers trying to 'get' the
> same controller structure. We could probably confirm that if we see your
> driver.

Yes, that's probably what's happening here.

> 
> But I see this could be a problem with a wide class of drivers.
> Basically, any NAND driver that has multiple NAND chips attached will
> see multiple reboot handlers that point at the same controller lock.
> This will obviously deadlock, since only one of the chips will make it
> through the nand_shutdown() function successfully.

Indeed.

> 
> And now that I begin describing the problem and grepping through the
> source logs... I see that this problem was already resolved back in
> 2009, except for the FL_PM_SUSPENDED mode:
> 
> commit 6b0d9a84124937f048bcb8b21313152b23063978
> Author: Li Yang <leoli@freescale.com>
> Date:   Tue Nov 17 14:45:49 2009 -0800
> 
>     mtd: nand: fix multi-chip suspend problem
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=6b0d9a84124937f048bcb8b21313152b23063978
>     
> I actually don't see why we can't just use
> nand_get_device(FL_PM_SUSPENDED) for the shutdown/reboot case, like
> this:

That should work, but shouldn't we keep the appropriate state (FL_SHUTDOWN)
and patch the nand_get_device() code instead?

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index ceb68ca..884caf0 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -839,9 +839,9 @@ retry:
                spin_unlock(lock);
                return 0;
        }
-       if (new_state == FL_PM_SUSPENDED) {
-               if (chip->controller->active->state == FL_PM_SUSPENDED) {
-                       chip->state = FL_PM_SUSPENDED;
+       if (new_state == FL_PM_SUSPENDED || new_state == FL_SHUTDOWN) {
+               if (chip->controller->active->state == new_state) {
+                       chip->state = new_state;
                        spin_unlock(lock);
                        return 0;
                }

or

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index ceb68ca..812b8b1 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -830,6 +830,20 @@ nand_get_device(struct mtd_info *mtd, int new_state)
 retry:
        spin_lock(lock);
 
+       /* putting the NAND chip in shutdown state should always succeed. */
+       if (new_state == FL_SHUTDOWN) {
+               /*
+                * release the controller if the chip put in shutdown state
+                * is the current active device.
+                */
+               if (chip->controller->active == chip)
+                       chip->controller->active = NULL;
+
+               chip->state = new_state;
+               spin_unlock(lock);
+               return 0;
+       }
+
        /* Hardware controller shared among independent devices */
        if (!chip->controller->active)
                chip->controller->active = chip;

> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index cc74142938b0..ece544efccc3 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -3110,7 +3110,7 @@ static void nand_resume(struct mtd_info *mtd)
>   */
>  static void nand_shutdown(struct mtd_info *mtd)
>  {
> -	nand_get_device(mtd, FL_SHUTDOWN);
> +	nand_get_device(mtd, FL_PM_SUSPENDED);
>  }
>  
>  /* Set default functions */
> 
> 
> It's also possible that this could be better solved in a proper
> refactor/rewrite of the NAND subsystem using a better controller/chip
> split, so there's only one reboot handler per NAND controller. Boris has
> been looking at that.

Yes, but I haven't considered reworking the locking or the reboot notifier
stuff :-). BTW, if you want to have a look at this work, it is in my
nand/layering-rework branch (starting at commit
9da4cc22857c103010141bca4d403915ee103dbb).

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: Hang on reboot in nand_get_device()

[Andrew E. Mileski]

On 2015-11-06 13:00, Brian Norris wrote:
>
> On Thu, Jul 02, 2015 at 03:21:48PM -0400, Andrew E. Mileski wrote:
>> I'm experiencing a hang on reboot with a Freescale P1022 PowerPC system, with a
>> dual chip-select NAND part (specified in the device tree as two
>> separate devices), and kernel v4.0.6.
>
> Which driver?

Sorry, I wasn't specific enough:  fsl_elbc_nand.c

Currently using the 4.2.0 Linux kernel version, and the problem persists.

I've actually made a small hack to it to enable software ECC, but it isn't fit 
for distribution.

The stock driver is rather ugly in my opinion, and makes some overly broad 
assumptions about the hardware; the driver really is only intended to support 
hardware ECC.  I've taken a few stabs at cleaning-up the stock driver, but they 
all end-up being complete re-writes (for multi-die support), which is a bit more 
than I can chew.

The driver otherwise is usable as-is, albeit with only hardware ECC (that is 
insufficient for many modern NAND devices), and exhibits the same problem.

>> It appears to be a hang in mtd/nand/nand_base.c:nand_get_device() waiting on
>> chip->controller->active.
>
> And now that I begin describing the problem and grepping through the
> source logs... I see that this problem was already resolved back in
> 2009, except for the FL_PM_SUSPENDED mode:
>
> commit 6b0d9a84124937f048bcb8b21313152b23063978
> Author: Li Yang <leoli@freescale.com>
> Date:   Tue Nov 17 14:45:49 2009 -0800
>
>      mtd: nand: fix multi-chip suspend problem
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=6b0d9a84124937f048bcb8b21313152b23063978
>
> I actually don't see why we can't just use
> nand_get_device(FL_PM_SUSPENDED) for the shutdown/reboot case, like
> this:
>
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index cc74142938b0..ece544efccc3 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -3110,7 +3110,7 @@ static void nand_resume(struct mtd_info *mtd)
>    */
>   static void nand_shutdown(struct mtd_info *mtd)
>   {
> -	nand_get_device(mtd, FL_SHUTDOWN);
> +	nand_get_device(mtd, FL_PM_SUSPENDED);
>   }
>
>   /* Set default functions */
>
>
> It's also possible that this could be better solved in a proper
> refactor/rewrite of the NAND subsystem using a better controller/chip
> split, so there's only one reboot handler per NAND controller. Boris has
> been looking at that.
>
> Anyway, if the above looks OK, I can send a proper patch and get your
> 'Tested-by's.
>
> Brian

That seems to work too!  Thanks!

~~Andrew E. Mileski

Re: Hang on reboot in nand_get_device()

[Brian Norris]

On Mon, Nov 09, 2015 at 01:43:41PM -0500, Andrew E. Mileski wrote:
> On 2015-11-06 13:00, Brian Norris wrote:
> >On Thu, Jul 02, 2015 at 03:21:48PM -0400, Andrew E. Mileski wrote:
> >>I'm experiencing a hang on reboot with a Freescale P1022 PowerPC system, with a
> >>dual chip-select NAND part (specified in the device tree as two
> >>separate devices), and kernel v4.0.6.
> >
> >Which driver?
> 
> Sorry, I wasn't specific enough:  fsl_elbc_nand.c
> 
> Currently using the 4.2.0 Linux kernel version, and the problem persists.
> 
> I've actually made a small hack to it to enable software ECC, but it
> isn't fit for distribution.
> 
> The stock driver is rather ugly in my opinion, and makes some overly
> broad assumptions about the hardware; the driver really is only
> intended to support hardware ECC.  I've taken a few stabs at
> cleaning-up the stock driver, but they all end-up being complete
> re-writes (for multi-die support), which is a bit more than I can
> chew.
> 
> The driver otherwise is usable as-is, albeit with only hardware ECC
> (that is insufficient for many modern NAND devices), and exhibits
> the same problem.

Yikes, I wish I hadn't actually taken a closer look at that driver :(
It could really use some rewrites for proper multi-chip/multi-die
support.

...

> >Anyway, if the above looks OK, I can send a proper patch and get your
> >'Tested-by's.
> >
> >Brian
> 
> That seems to work too!  Thanks!

Great! I'll review Boris's suggestions to see if there's a better
option.

Brian

Re: Hang on reboot in nand_get_device()

[Brian Norris]

On Fri, Nov 06, 2015 at 07:59:03PM +0100, Boris Brezillon wrote:
> On Fri, 6 Nov 2015 10:00:52 -0800
> Brian Norris <computersforpeace@gmail.com> wrote:
> > On Thu, Jul 02, 2015 at 03:21:48PM -0400, Andrew E. Mileski wrote:
> > > I'm experiencing a hang on reboot with a Freescale P1022 PowerPC system, with a
> > > dual chip-select NAND part (specified in the device tree as two
> > > separate devices), and kernel v4.0.6.
> > 
> > Which driver?
> > 
> > > It appears to be a hang in mtd/nand/nand_base.c:nand_get_device() waiting on
> > > chip->controller->active.
> > > 
> > > Shouldn't nand_shutdown(), or perhaps a special case in nand_get_device() for
> > > FL_SHUTDOWN, set chip->controller->active = NULL before returning?
> > 
> > I don't think that's exactly the right solution, but you're in the right
> > ballpark I expect.
> 
> Hm, actually I find the idea of releasing the NAND controller when the
> flash is set in FL_SHUTDOWN state not that bad. Is there any reason
> we would want the NAND chip to stay active when we're shutting the
> system down.

No, and that's the point of "getting" the device, without actually
releasing it. (Andrew's suggestion is essentially a
nand_release_device().) We don't want any other process picking up any
I/O at this point. I suppose there really is no way to begin any new
I/O, but it does seem possible for the last operation to still be in
flight, at least according to Scott's reports.

> I understand that this is needed for the suspended case because we want
> the system to restore the correct state when resuming (ie marking the
> right device as active), but shutdown should be simpler.

Is the SUSPENDED code really that complex? It's just allowing all
FL_PM_SUSPENDED requests after the first one. It doesn't even do much
fancy for the release path (and in fact, it seems slightly buggy, now
that I'm looking at it; because we violated mutual exclusion, there can
now be many chips that are releasing the same chip. So nand_get_device()
won't really work right again until all chips have called nand_resume().)

> > I actually don't see why we can't just use
> > nand_get_device(FL_PM_SUSPENDED) for the shutdown/reboot case, like
> > this:
> 
> That should work, but shouldn't we keep the appropriate state (FL_SHUTDOWN)
> and patch the nand_get_device() code instead?

The states mean only as much as we ascribe to them. If we really need to
treat shutdown differently than suspend, then I suppose yes. But
otherwise, I see no need.

> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index ceb68ca..884caf0 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -839,9 +839,9 @@ retry:
>                 spin_unlock(lock);
>                 return 0;
>         }
> -       if (new_state == FL_PM_SUSPENDED) {
> -               if (chip->controller->active->state == FL_PM_SUSPENDED) {
> -                       chip->state = FL_PM_SUSPENDED;
> +       if (new_state == FL_PM_SUSPENDED || new_state == FL_SHUTDOWN) {
> +               if (chip->controller->active->state == new_state) {
> +                       chip->state = new_state;
>                         spin_unlock(lock);
>                         return 0;
>                 }

I suppose that works, but I don't see it buying us much.

(Although, I suppose it makes it a little more obvious if we somehow
mixed the SUSPENDED state with the SHUTDOWN state.)

> or
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index ceb68ca..812b8b1 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -830,6 +830,20 @@ nand_get_device(struct mtd_info *mtd, int new_state)
>  retry:
>         spin_lock(lock);
>  
> +       /* putting the NAND chip in shutdown state should always succeed. */
> +       if (new_state == FL_SHUTDOWN) {
> +               /*
> +                * release the controller if the chip put in shutdown state
> +                * is the current active device.
> +                */
> +               if (chip->controller->active == chip)
> +                       chip->controller->active = NULL;
> +
> +               chip->state = new_state;
> +               spin_unlock(lock);
> +               return 0;
> +       }
> +
>         /* Hardware controller shared among independent devices */
>         if (!chip->controller->active)
>                 chip->controller->active = chip;
> 

This looks a lot more subtle and potentially wrong. What exactly is the
rationale here? It appears you're kind of unlocking the controller (any
other flash on the same controller can still go ahead) but at the same
time forcing no further users of this particular flash. In the end, I
guess it accomplishes mostly the same thing as the SUSPENDED case,
except that it's written differently, and has slightly different
behaviors in corner cases (e.g., what if another nand_chip gets a call
to nand_get_device(FL_WRITING)? with this patch, they can still write to
the flash; with your first patch or with mine, they are locked out).

IOW, I'd rather share the implementation if they're really that similar,
unless you really have a good reason for this one.

> > It's also possible that this could be better solved in a proper
> > refactor/rewrite of the NAND subsystem using a better controller/chip
> > split, so there's only one reboot handler per NAND controller. Boris has
> > been looking at that.
> 
> Yes, but I haven't considered reworking the locking or the reboot notifier
> stuff :-).

Right, but once they're split, it might be easier to have more generic
per-controller features (rather than per-flash). But maybe not. I'm
definitely not suggesting we do that now; let's just fix the problem
with the code as it stands now.

Brian

Re: Hang on reboot in nand_get_device()

[Andrew E. Mileski]

On 2015-11-09 14:46, Brian Norris wrote:
>
> No, and that's the point of "getting" the device, without actually
> releasing it. (Andrew's suggestion is essentially a
> nand_release_device().) We don't want any other process picking up any
> I/O at this point. I suppose there really is no way to begin any new
> I/O, but it does seem possible for the last operation to still be in
> flight, at least according to Scott's reports.

That right there. is the very reason I questioned whether my approach was 
sufficient.

~~Andrew E. Mileski

Re: Hang on reboot in nand_get_device()

[Scott Branden]

On 15-11-09 11:56 AM, Andrew E. Mileski wrote:
> On 2015-11-09 14:46, Brian Norris wrote:
>>
>> No, and that's the point of "getting" the device, without actually
>> releasing it. (Andrew's suggestion is essentially a
>> nand_release_device().) We don't want any other process picking up any
>> I/O at this point. I suppose there really is no way to begin any new
>> I/O, but it does seem possible for the last operation to still be in
>> flight, at least according to Scott's reports.
>
> That right there. is the very reason I questioned whether my approach
> was sufficient.

Yes, the last operation can still be in flight - there are async cleanup 
threads running in the UBI/UBIFS layers which recreated this problem.
>
> ~~Andrew E. Mileski

Re: Hang on reboot in nand_get_device()

[Boris Brezillon]

Hi Brian,

On Mon, 9 Nov 2015 11:46:51 -0800
Brian Norris <computersforpeace@gmail.com> wrote:

> On Fri, Nov 06, 2015 at 07:59:03PM +0100, Boris Brezillon wrote:
> > On Fri, 6 Nov 2015 10:00:52 -0800
> > Brian Norris <computersforpeace@gmail.com> wrote:
> > > On Thu, Jul 02, 2015 at 03:21:48PM -0400, Andrew E. Mileski wrote:
> > > > I'm experiencing a hang on reboot with a Freescale P1022 PowerPC system, with a
> > > > dual chip-select NAND part (specified in the device tree as two
> > > > separate devices), and kernel v4.0.6.
> > > 
> > > Which driver?
> > > 
> > > > It appears to be a hang in mtd/nand/nand_base.c:nand_get_device() waiting on
> > > > chip->controller->active.
> > > > 
> > > > Shouldn't nand_shutdown(), or perhaps a special case in nand_get_device() for
> > > > FL_SHUTDOWN, set chip->controller->active = NULL before returning?
> > > 
> > > I don't think that's exactly the right solution, but you're in the right
> > > ballpark I expect.
> > 
> > Hm, actually I find the idea of releasing the NAND controller when the
> > flash is set in FL_SHUTDOWN state not that bad. Is there any reason
> > we would want the NAND chip to stay active when we're shutting the
> > system down.
> 
> No, and that's the point of "getting" the device, without actually
> releasing it. (Andrew's suggestion is essentially a
> nand_release_device().) We don't want any other process picking up any
> I/O at this point. I suppose there really is no way to begin any new
> I/O, but it does seem possible for the last operation to still be in
> flight, at least according to Scott's reports.

Hm, if we really want all I/O going through the NAND controller to be
stopped, then that should definitely be handled at the controller level
(as you suggested in your previous email).
My understanding was that the shutdown and suspended states were only
attached to the device itself, not the controller behind this device
(even if at some point all the devices will be marked as suspended or
shut down and the controller will remain inactive).

> 
> > I understand that this is needed for the suspended case because we want
> > the system to restore the correct state when resuming (ie marking the
> > right device as active), but shutdown should be simpler.
> 
> Is the SUSPENDED code really that complex? It's just allowing all
> FL_PM_SUSPENDED requests after the first one. It doesn't even do much
> fancy for the release path (and in fact, it seems slightly buggy, now
> that I'm looking at it; because we violated mutual exclusion, there can
> now be many chips that are releasing the same chip. So nand_get_device()
> won't really work right again until all chips have called nand_resume().)

No it's not that complex, but I find it weird to keep the lock on the
controller even if the NAND chip attached to this controller has been
suspended. And the problem you pointed out just shows how tricky it
can be when you play this kind of game (keep the lock on the controller
assigned to one NAND chip but consider other nand_get_device() calls as
successful even if the controller is already taken by another chip in
some specific cases).

BTW, I guess you already have the fix, but in case you haven't done it
yet, this should do the trick:

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index f1ddacf..9f0ea48 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -136,9 +136,11 @@ static void nand_release_device(struct mtd_info *mtd)
 
        /* Release the controller and the chip */
        spin_lock(&chip->controller->lock);
-       chip->controller->active = NULL;
        chip->state = FL_READY;
-       wake_up(&chip->controller->wq);
+       if (chip->controller->active == chip) {
+               chip->controller->active = NULL;
+               wake_up(&chip->controller->wq);
+       }
        spin_unlock(&chip->controller->lock);
 }
 


> 
> > > I actually don't see why we can't just use
> > > nand_get_device(FL_PM_SUSPENDED) for the shutdown/reboot case, like
> > > this:
> > 
> > That should work, but shouldn't we keep the appropriate state (FL_SHUTDOWN)
> > and patch the nand_get_device() code instead?
> 
> The states mean only as much as we ascribe to them. If we really need to
> treat shutdown differently than suspend, then I suppose yes. But
> otherwise, I see no need.
> 
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index ceb68ca..884caf0 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -839,9 +839,9 @@ retry:
> >                 spin_unlock(lock);
> >                 return 0;
> >         }
> > -       if (new_state == FL_PM_SUSPENDED) {
> > -               if (chip->controller->active->state == FL_PM_SUSPENDED) {
> > -                       chip->state = FL_PM_SUSPENDED;
> > +       if (new_state == FL_PM_SUSPENDED || new_state == FL_SHUTDOWN) {
> > +               if (chip->controller->active->state == new_state) {
> > +                       chip->state = new_state;
> >                         spin_unlock(lock);
> >                         return 0;
> >                 }
> 
> I suppose that works, but I don't see it buying us much.
> 
> (Although, I suppose it makes it a little more obvious if we somehow
> mixed the SUSPENDED state with the SHUTDOWN state.)

Well, it's more about using the proper state in case we have to propagate it to
NAND controller drivers someday.

> 
> > or
> > 
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index ceb68ca..812b8b1 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -830,6 +830,20 @@ nand_get_device(struct mtd_info *mtd, int new_state)
> >  retry:
> >         spin_lock(lock);
> >  
> > +       /* putting the NAND chip in shutdown state should always succeed. */
> > +       if (new_state == FL_SHUTDOWN) {
> > +               /*
> > +                * release the controller if the chip put in shutdown state
> > +                * is the current active device.
> > +                */
> > +               if (chip->controller->active == chip)
> > +                       chip->controller->active = NULL;
> > +
> > +               chip->state = new_state;
> > +               spin_unlock(lock);
> > +               return 0;
> > +       }
> > +
> >         /* Hardware controller shared among independent devices */
> >         if (!chip->controller->active)
> >                 chip->controller->active = chip;
> > 
> 
> This looks a lot more subtle and potentially wrong. What exactly is the
> rationale here? It appears you're kind of unlocking the controller (any
> other flash on the same controller can still go ahead) but at the same
> time forcing no further users of this particular flash. In the end, I
> guess it accomplishes mostly the same thing as the SUSPENDED case,
> except that it's written differently, and has slightly different
> behaviors in corner cases (e.g., what if another nand_chip gets a call
> to nand_get_device(FL_WRITING)? with this patch, they can still write to
> the flash; with your first patch or with mine, they are locked out).

Yes, that's pretty much was I said above: I was seeing FL_XXX states as attached
to the NAND chips not to the controller used to access those chips.
Not sure this is possible, but what if we had a way to use runtime PM at the
chip level, not the controller level. That would mean preventing every other
NAND chip under the same controller to work until this chip enter the
resumed/ready state.

> 
> IOW, I'd rather share the implementation if they're really that similar,
> unless you really have a good reason for this one.



> 
> > > It's also possible that this could be better solved in a proper
> > > refactor/rewrite of the NAND subsystem using a better controller/chip
> > > split, so there's only one reboot handler per NAND controller. Boris has
> > > been looking at that.
> > 
> > Yes, but I haven't considered reworking the locking or the reboot notifier
> > stuff :-).
> 
> Right, but once they're split, it might be easier to have more generic
> per-controller features (rather than per-flash). But maybe not. I'm
> definitely not suggesting we do that now; let's just fix the problem
> with the code as it stands now.

Yep.

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: Hang on reboot in nand_get_device()

[Boris Brezillon]

Hi again,

Just want to add that this discussion shouldn't prevent your fix from
being applied. The main reason I'm arguing here is because I want to
understand the rationale behind the current handling of FL_PM_SUSPENDED
and FL_SHUTDOWN.

On Mon, 9 Nov 2015 21:55:08 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> > > 
> > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > > index ceb68ca..812b8b1 100644
> > > --- a/drivers/mtd/nand/nand_base.c
> > > +++ b/drivers/mtd/nand/nand_base.c
> > > @@ -830,6 +830,20 @@ nand_get_device(struct mtd_info *mtd, int new_state)
> > >  retry:
> > >         spin_lock(lock);
> > >  
> > > +       /* putting the NAND chip in shutdown state should always succeed. */
> > > +       if (new_state == FL_SHUTDOWN) {
> > > +               /*
> > > +                * release the controller if the chip put in shutdown state
> > > +                * is the current active device.
> > > +                */
> > > +               if (chip->controller->active == chip)
> > > +                       chip->controller->active = NULL;
> > > +
> > > +               chip->state = new_state;
> > > +               spin_unlock(lock);
> > > +               return 0;
> > > +       }
> > > +
> > >         /* Hardware controller shared among independent devices */
> > >         if (!chip->controller->active)
> > >                 chip->controller->active = chip;
> > > 
> > 
> > This looks a lot more subtle and potentially wrong. What exactly is the
> > rationale here? It appears you're kind of unlocking the controller (any
> > other flash on the same controller can still go ahead) but at the same
> > time forcing no further users of this particular flash.

It's even worst: I'm not waiting for the chip to become ready, so I'm
potentially re-introducing the bug Scott was trying to solve with his
reboot notifier.



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: Hang on reboot in nand_get_device()

[Brian Norris]

On Mon, Nov 09, 2015 at 10:36:13PM +0100, Boris Brezillon wrote:
> Just want to add that this discussion shouldn't prevent your fix from
> being applied. The main reason I'm arguing here is because I want to
> understand the rationale behind the current handling of FL_PM_SUSPENDED
> and FL_SHUTDOWN.

Sure, that's reasonable. I'd also like to touch this code only once (or
very close to that) in the near future, so it's best if we get to a good
understanding.

I'll send this as a proper patch, if that sounds OK:

http://patchwork.ozlabs.org/patch/541065/

> On Mon, 9 Nov 2015 21:55:08 +0100
> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> 
> > > > 
> > > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > > > index ceb68ca..812b8b1 100644
> > > > --- a/drivers/mtd/nand/nand_base.c
> > > > +++ b/drivers/mtd/nand/nand_base.c
> > > > @@ -830,6 +830,20 @@ nand_get_device(struct mtd_info *mtd, int new_state)
> > > >  retry:
> > > >         spin_lock(lock);
> > > >  
> > > > +       /* putting the NAND chip in shutdown state should always succeed. */
> > > > +       if (new_state == FL_SHUTDOWN) {
> > > > +               /*
> > > > +                * release the controller if the chip put in shutdown state
> > > > +                * is the current active device.
> > > > +                */
> > > > +               if (chip->controller->active == chip)
> > > > +                       chip->controller->active = NULL;
> > > > +
> > > > +               chip->state = new_state;
> > > > +               spin_unlock(lock);
> > > > +               return 0;
> > > > +       }
> > > > +
> > > >         /* Hardware controller shared among independent devices */
> > > >         if (!chip->controller->active)
> > > >                 chip->controller->active = chip;
> > > > 
> > > 
> > > This looks a lot more subtle and potentially wrong. What exactly is the
> > > rationale here? It appears you're kind of unlocking the controller (any
> > > other flash on the same controller can still go ahead) but at the same
> > > time forcing no further users of this particular flash.
> 
> It's even worst: I'm not waiting for the chip to become ready, so I'm
> potentially re-introducing the bug Scott was trying to solve with his
> reboot notifier.

Ah, I see! Good catch. My distaste for duplication pays off, then :)

Brian

Re: Hang on reboot in nand_get_device()

[Scott Branden]

Hi Brian,

I'm confused as to what the outcome is here and what the final patch is. 
  Will this affect the previous fixes we made such that shutdown is 
called on reboot so that MTD operations to the controller are not in 
progress on reboot?

Thanks,
  Scott

On 15-11-09 01:44 PM, Brian Norris wrote:
> On Mon, Nov 09, 2015 at 10:36:13PM +0100, Boris Brezillon wrote:
>> Just want to add that this discussion shouldn't prevent your fix from
>> being applied. The main reason I'm arguing here is because I want to
>> understand the rationale behind the current handling of FL_PM_SUSPENDED
>> and FL_SHUTDOWN.
>
> Sure, that's reasonable. I'd also like to touch this code only once (or
> very close to that) in the near future, so it's best if we get to a good
> understanding.
>
> I'll send this as a proper patch, if that sounds OK:
>
> http://patchwork.ozlabs.org/patch/541065/
>
>> On Mon, 9 Nov 2015 21:55:08 +0100
>> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
>>
>>>>>
>>>>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>>>>> index ceb68ca..812b8b1 100644
>>>>> --- a/drivers/mtd/nand/nand_base.c
>>>>> +++ b/drivers/mtd/nand/nand_base.c
>>>>> @@ -830,6 +830,20 @@ nand_get_device(struct mtd_info *mtd, int new_state)
>>>>>   retry:
>>>>>          spin_lock(lock);
>>>>>
>>>>> +       /* putting the NAND chip in shutdown state should always succeed. */
>>>>> +       if (new_state == FL_SHUTDOWN) {
>>>>> +               /*
>>>>> +                * release the controller if the chip put in shutdown state
>>>>> +                * is the current active device.
>>>>> +                */
>>>>> +               if (chip->controller->active == chip)
>>>>> +                       chip->controller->active = NULL;
>>>>> +
>>>>> +               chip->state = new_state;
>>>>> +               spin_unlock(lock);
>>>>> +               return 0;
>>>>> +       }
>>>>> +
>>>>>          /* Hardware controller shared among independent devices */
>>>>>          if (!chip->controller->active)
>>>>>                  chip->controller->active = chip;
>>>>>
>>>>
>>>> This looks a lot more subtle and potentially wrong. What exactly is the
>>>> rationale here? It appears you're kind of unlocking the controller (any
>>>> other flash on the same controller can still go ahead) but at the same
>>>> time forcing no further users of this particular flash.
>>
>> It's even worst: I'm not waiting for the chip to become ready, so I'm
>> potentially re-introducing the bug Scott was trying to solve with his
>> reboot notifier.
>
> Ah, I see! Good catch. My distaste for duplication pays off, then :)
>
> Brian
>

Re: Hang on reboot in nand_get_device()

[Brian Norris]

Hi Scott,

On Mon, Nov 09, 2015 at 01:51:53PM -0800, Scott Branden wrote:
> I'm confused as to what the outcome is here and what the final patch
> is.  Will this affect the previous fixes we made such that shutdown
> is called on reboot so that MTD operations to the controller are not
> in progress on reboot?

There is no "final outcome" yet, but the plan is below:

> On 15-11-09 01:44 PM, Brian Norris wrote:
> >On Mon, Nov 09, 2015 at 10:36:13PM +0100, Boris Brezillon wrote:
> >>Just want to add that this discussion shouldn't prevent your fix from
> >>being applied.
[...]
> >I'll send this as a proper patch, if that sounds OK:
> >
> >http://patchwork.ozlabs.org/patch/541065/

^^ I plan to resend that patch as an independent thread, and give people
a chance to test/ack/nak anything there. I'll CC you, so you can ensure
it doesn't break anything you did previously. (I don't see how it
would.)

> >>On Mon, 9 Nov 2015 21:55:08 +0100
> >>Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> >>
> >>It's even worst: I'm not waiting for the chip to become ready, so I'm
> >>potentially re-introducing the bug Scott was trying to solve with his
> >>reboot notifier.
> >
> >Ah, I see! Good catch. My distaste for duplication pays off, then :)

In case this hunk was confusing: Boris was providing another reason to
reject one of his suggested alternative patches, and I was agreeing with
him.

Stay tuned.

Brian

P.S. Richard and I had some discussion on IRC, and I think there was a
rough agreement that the whole reboot handler dance really doesn't
belong (exclusively) in the MTD layer. Your patch was inspired by
problems with UBI, and (at least for non-initrd cases) we think UBI
should probably learn how to clean up after itself before we reboot. So
in the long term, there may be an attempt to fix up UBI and drop the MTD
reboot handlers. But that's probably not going to happen today.