Re: Hang on reboot in nand_get_device()

[Scott Branden <sbranden@broadcom.com>]

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
>