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