| Message ID | 20151106195903.0d55d819@bbrezillon |
|---|---|
| State | RFC |
| Headers | show |
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
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
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
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;