Re: [PATCH v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Kamal Dasu <kdasu.kdev@xxxxxxxxx>]

"At least one problem I suspect will happen here is that if a command is
in progress on the SPI NOR and the controller suspends, resumes and
rescans, it is well possible the SPI NOR will be in undefined state."

I don't think suspend would happen in middle of a command or transfer.
On suspend filesystem drivers do a sync and it is my understanding
that the mtd layer, spi masters will stop the queue before the m25p80
suspend will get called. And on resume new transfers continue after
the spi-nor is setup in its probed state.

Kamal

On Fri, Jan 20, 2017 at 3:42 PM, Marek Vasut <marex@xxxxxxx> wrote:
> On 01/20/2017 09:23 PM, Florian Fainelli wrote:
>> On 01/20/2017 12:13 PM, Marek Vasut wrote:
>>> On 01/20/2017 09:07 PM, Florian Fainelli wrote:
>>>> On 01/20/2017 12:02 PM, Marek Vasut wrote:
>>>>> On 01/20/2017 07:50 PM, Kamal Dasu wrote:
>>>>>> On Fri, Jan 20, 2017 at 12:22 PM, Marek Vasut <marex@xxxxxxx> wrote:
>>>>>>> On 01/20/2017 05:33 PM, Kamal Dasu wrote:
>>>>>>>> Marek,
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> OK, so I'm fine with leaving the patch as is for now but I would like Marek
>>>>>>>>>> review just to be sure we didn't miss something: Marek, any comments?
>>>>>>>>>>
>>>>>>>>>> I just have one more comment below but it's only a detail.
>>>>>>>>>
>>>>>>>>> What would happen if you have a FS attached on this SPI NOR (like UBIFS)
>>>>>>>>> and you suspend with this patch ? Will it survive ?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>> I have tried with both jffs2 rw and ubifs rw rootfs on spi-nor
>>>>>>>> booting from spi-nor partitions while  active filesystem operations
>>>>>>>> were going on over suspend/resume cycles. And it survives.
>>>>>>>
>>>>>>> Can you elaborate on your test a bit ?
>>>>>>>
>>>>>>
>>>>>> The power management test  puts  board in standby mode,  during
>>>>>> standby the DRAM is in refresh and power to the spi-nor is removed.
>>>>>> After a set timeout the board wakes up again and resumes from where it
>>>>>> left off. This pm test is run while a cp command is invoked where a
>>>>>> file is transferred to the rw rootfs. The test loops through said
>>>>>> number of iterations of suspend and resumes cycles. The copy
>>>>>> operations is eventually done over a few cycles. Once the test is
>>>>>> completed  filesystem should have no corruptions, the copied files
>>>>>> should compare and rootfs should be intact.  This test was repeated
>>>>>> with jffs2 as well as ubifs rootfs flashed on spi-nor device.
>>>>>
>>>>> Would be much nicer if you could just share the script you used.
>>>>
>>>> Not sure if this is even possible, but maybe we could. What part of
>>>> Kamal's description of the test is not clear here?
>>>
>>> I cannot check the test and see what was really done, the code is the
>>> best description. If someone tells me a script (which is likely about
>>> 20 lines) cannot be shared, that's fishy.
>>
>> The reason why I mentioned that is that it's not like our test suite is
>> somewhere out there on Github and we could just point you to it, it
>> could take some time and effort to extract the script, period. It's not
>> fishy, it's just not ideal, and it is a consequence of working in a
>> corporate environment.
>
> Somehow, it feels like you're digging yourself in deeper and deeper ...
> now I'm seriously curious how this was tested. Esp. since this patch
> has potentially massive impact and could annoy a lot of people if it
> breaks things, so a lot of testing will have to happen on this one.
> Having a testcase would help ...
>
>>>> Even without doing actual copy across suspend/resume cycles, a device
>>>> which has a power domain architecture where the SPI-NOR flash gets
>>>> powered down during a suspend state, and the root filesystem is on that
>>>> flash would expose issues without Kamal's patch here.
>>>
>>> At least one problem I suspect will happen here is that if a command is
>>> in progress on the SPI NOR and the controller suspends, resumes and
>>> rescans, it is well possible the SPI NOR will be in undefined state.
>>>
>>
>> Fair enough.
>>
>
>
> --
> Best regards,
> Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html