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 at 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