Re: [PATCH 1/2] ata: libata-eh: Spinup disk on resume after revalidation

[Niklas Cassel <Niklas.Cassel@wdc.com>]

On Thu, Oct 12, 2023 at 04:17:59PM +0900, Damien Le Moal wrote:
> Move the call to ata_dev_power_set_active() to transition a disk in
> standby power mode to the active power mode from
> ata_eh_revalidate_and_attach() before doing revalidation to the end of
> ata_eh_recover(), after the link speed for the device is reconfigured
> (if that was necessary). This is safer as this ensure that the VERIFY
> command executed to spinup the disk is executed with the drive properly
> reconfigured first.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/ata/libata-eh.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 945675f6b822..b0d6e69c4a5b 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -3051,15 +3051,6 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
>  		if (ehc->i.flags & ATA_EHI_DID_RESET)
>  			readid_flags |= ATA_READID_POSTRESET;
>  
> -		/*
> -		 * When resuming, before executing any command, make sure to
> -		 * transition the device to the active power mode.
> -		 */
> -		if ((action & ATA_EH_SET_ACTIVE) && ata_dev_enabled(dev)) {
> -			ata_dev_power_set_active(dev);
> -			ata_eh_done(link, dev, ATA_EH_SET_ACTIVE);
> -		}
> -
>  		if ((action & ATA_EH_REVALIDATE) && ata_dev_enabled(dev)) {
>  			WARN_ON(dev->class == ATA_DEV_PMP);
>  
> @@ -3856,6 +3847,17 @@ int ata_eh_recover(struct ata_port *ap, ata_prereset_fn_t prereset,
>  			}
>  		}
>  
> +		/*
> +		 * Make sure to transition devices to the active power mode
> +		 * if needed (e.g. if we were scheduled on system resume).
> +		 */
> +		ata_for_each_dev(dev, link, ENABLED) {
> +			if (ehc->i.dev_action[dev->devno] & ATA_EH_SET_ACTIVE) {
> +				ata_dev_power_set_active(dev);
> +				ata_eh_done(link, dev, ATA_EH_SET_ACTIVE);
> +			}
> +		}
> +
>  		/* retry flush if necessary */
>  		ata_for_each_dev(dev, link, ALL) {
>  			if (dev->class != ATA_DEV_ATA &&
> -- 
> 2.41.0
> 

Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>