On 8/6/23 11:55, Linus Torvalds wrote: > On Sat, 5 Aug 2023 at 19:26, Linus Torvalds > <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote: >> >> (a) split manage_start_stop into three bits for the three actual >> cases it deals with: (stop_on_suspend, stop_on_shutdown, >> start_on_resume). >> >> (b) then not set the "start_on_resume" bit > > Side note: the ATA layer already has > > - ATA_FLAG_NO_POWEROFF_SPINDOWN > > which seems to basically be very close to "don't send stop on shutdown" > > And in fact, it looks like > > - ATA_FLAG_NO_HIBERNATE_SPINDOWN > > is in turn very close to "don't send stop on suspend". So if that > manage_start_stop had been just split into those three actions, it > sounds like those ATA_FLAG flags we have would basically have > translated to setting those bits too. Indeed. > And the whole "start" seems to be translated to "ATA_CMD_VERIFY for > the first sector", which would seem to be literally just a random > command intentionally chosen to not return any actual data. This likely was added to force the drive to spinup, to avoid subsequent commands to fail. The start process doing a port reset and then revalidating the drive, which starts by issuing an IDENTIFY command, definitely will wake up the drive. There is no need for relying on scsi to do it. However, I am still not convinced that the revalidate should be done at all, unless the scsi layer does do it too. As you said, this may be causing unwarranted spinup for cases when the drive is not used after resuming. But again here, scsi and ATA revalidate are not linked either. One does not trigger the other. > The *only* effect of doing that 'start' is to cause extra disk IO > early that is then ignored. Logically it doesn't actually do anything > useful, and it would seem like it might actually be an actively bad > thing (ie spinning up a disk on a laptop for potentially no actual > good reason, and waiting for this all to happen). > > End result: it really smells like ATA fundamentally doesn't want the > whole 'manage_start_stop' noise AT ALL. Hannes suggested doing all ATA start/stop processing solely relying on libata, that is, not using manage_start_stop at all. This is another option I am considering. > > You just removed the nasty early start with that 'no_start_on_resume' > bit, and the spindown seems to be questionable on at least some > machines too. Yes it is. I have seen issues with the shutdown side, most of the time showing as a synchronize cache failure. But until now, I had no clear understanding how/why that happened. I understand it now and will be fixing that. > So I wonder: did somebody test just removing the setting of that flag entirely? > > I guess ATA is legacy enough these days that people want to make > minimal changes, although that 'no_start_on_resume' really doesn't > smell all that much more minimal to me than not sending spindown > commands. I will revisit everything around PM operations. ATA is indeed legacy-ish for most desktop/laptop users as NVMe has taken over most systems. But there are still a lot of enterprise deployments using ATA drives, and a lot of users running old hardware. The former is rather easy to deal with as the hardware is most of the time very recent, and so better than old devices. The former is a constant source of headaches due to the sheer amount of buggy hardware out there. The potential for regressions when changing something is high, so I try to be prudent with changes. Thank you for your comments. -- Damien Le Moal Western Digital Research
