Message ID | 20151109215508.7b14f5f3@bbrezillon |
---|---|
State | RFC |
Headers | show |
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.
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
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 >
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.
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); }