On 1/6/24 02:03, Phillip Susi wrote: > Damien Le Moal <dlemoal@xxxxxxxxxx> writes: > >>> - return; >>> + return AC_ERR_OTHER; >> >> Nope. This is wrong. ata_dev_power_init_tf() returns a bool, not an error. The >> bool indicates if the drive supports power management. > > Are you saying it should return AC_ERR_OK? If the drive doesn't support > power management at all, isn't it an error to try to change its power > management state? That is why the function returns doing nothing if ata_dev_power_init_tf() returns false, meaning "do not do power management". See that function. It includes ATAPI devices (e.g. CD/DVD) which do not have power management. >> But beside this, I still do not understand what this fixes... Calling again >> ata_dev_power_set_active() will do nothing but issue a check power mode command >> if the drive is already active. So I do not see the need for this added complexity. > > If the drive is NOT active because it is PuiS, it would try to start the drive > anyhow, despite the PuiS actively clearing the ATA_EH_SET_ACTVE flag. > Then the VERIFY command fails if the drive requires the SET FEATURES > command to leave PuiS. Define a device flag to indicate that the drive has PUIS "on" and so woke up in standby, e.g. ATA_DFLAG_PUIS_STANDBY. Check that flag in ata_dev_power_set_active(), similarly to the ATA_DFLAG_SLEEPING flag and return doing nothing if the puis flag is set. Way easier than playing games with the return value. > My goal here was to make sure that when the PuiS patch clears > ATA_EH_SET_ACTIVE, that it does not get turned back on during a second > round of EH. Then you pointed out that it SHOULD try on the second > round, if the first attempt failed, so I changed the return to be able > to detect the error, and turn ATA_EH_SET_ACTIVE on for the second round. See above. > It occurs to me now that I only ran into this issue once I changed to > actually giving the drive the SLEEP command instead of just setting the > sleeping flag. Once the drive actually went to sleep, it shut down the > sata link, which caused an error interrupt, which triggered a second > round of EH, which then issued the failing VERIFY command until all 5 > rounds were used and it gave up. So if I go back to just setting the > sleeping flag instead of actually putting the drive to sleep, I would > not get the error interrupt and would be fine without this patch. But I > can imagine some other cause for a second round of EH on some system > that would still run into this problem. Please separate handling of sleep and puis. The former can be used even if puis is off. -- Damien Le Moal Western Digital Research
