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

[Marek Vasut <marex@xxxxxxx>]

On 01/20/2017 10:15 PM, Kamal Dasu wrote:
> "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.

Who does guarantee that ?

> And on resume new transfers continue after
> the spi-nor is setup in its probed state.

Also please respect the ML netiquette and do NOT top-post.

> 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


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