Re: [bug report] scsi: SATA devices missing after FLR is triggered during HBA suspended

[Damien Le Moal <dlemoal@kernel.org>]

On 7/2/24 05:39, Bjorn Helgaas wrote:
> [+cc Alex]
> 
> On Thu, Jun 27, 2024 at 09:56:02AM +0900, Damien Le Moal wrote:
>> On 6/27/24 00:15, Bjorn Helgaas wrote:
>>>>> Yes, I am talking about the PCI "Function Level Reset"
>>>>>
>>>>>> FLR and disk/controller suspend execution timing are unrelated.
>>>>>> FLR can be triggered at any time through sysfs. So please give
>>>>>> details here. Why is FLR done when the system is being
>>>>>> suspended ?
>>>>>
>>>>> Yes, it is because FLR can be triggered at any time that we are
>>>>> testing the reliability of executing FLR commands after
>>>>> disk/controller suspended.
>>>>
>>>> "can be triggered" ? FLR is not a random asynchronous event. It
>>>> is an action that is *issued* by a user with sys admin rights.
>>>> And such users can do a lot of things that can break a machine...
>>>>
>>>> I fail to see the point of doing a function reset while the
>>>> device is suspended. But granted, I guess the device should
>>>> comeback up in such case, though I would like to hear what the
>>>> PCI guys have to say about this.
>>>>
>>>> Bjorn,
>>>>
>>>> Is reseting a suspended PCI device something that should be/is
>>>> supported ?
>>>
>>> I doubt it.  The PCI core should be preserving all the generic PCI
>>> state across suspend/resume.  The driver should only need to
>>> save/restore device-specific things the PCI core doesn't know about.
>>>
>>> A reset will clear out most state, and the driver doesn't know the
>>> reset happened, so it will expect most device state to have been
>>> preserved.
>>
>> That is what I suspected. However, checking the code, reset_store() in
>> pci-sysfs.c does:
>>
>> 	pm_runtime_get_sync(dev);
>> 	result = pci_reset_function(pdev);
>> 	pm_runtime_put(dev);
>>
>> and pm_runtime_get_sync() calls __pm_runtime_resume() which will
>> resume a suspended device.
>>
>> So while I still think it is not a good idea to reset a suspended
>> device, things should still work as execpected and not cause any
>> problem with the device state, right ?
> 
> The reset will clear almost all state, including both the generic PCI
> part that pci_reset_function() saves/restores *and* any
> device-specific state the PCI core doesn't know about.
> 
> That device-specific state isn't saved and restored anywhere in the
> sysfs reset path, and the driver doesn't know this reset happened, so
> I think all bets are off and we shouldn't expect the driver to work
> afterwards.
> 
> A user-space reset might make sense if there's no driver bound to the
> device, but I don't think it does if there is a driver (except maybe a
> trivial stub driver that doesn't actually operate the device).

OK, makes sense.

I amstill looking into this though because I did find a nasty issue: if the HBA
is reset while all the drives connected to it are suspended (spun down), the
drives are never woken up and the drive re-scan trigerred by the PCI reset fails
with command timeouts. And even worse, I hit a deadlock when unloading the
driver after that happens.

All of that should not be happening: the HBA reset should simply result in
either all drives coming back or the drives (scsi devices) being dropped and
re-scan creating new ones. But that I think is not a PCI issue but rather a HBA
driver issue, or a problem with libsas/scsi/libata power management.

Thanks for the comments.

-- 
Damien Le Moal
Western Digital Research