Re: [PATCH 1/1] libata: only wake a drive once on system resume

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

On 12/31/23 03:21, Phillip Susi wrote:
> In the event that more than one pass of EH is needed during system resume,
> only request the drive be started once.

This is not an improvement... What if the verify command that wakes-up the drive
fails to be issued, or EH does not reach the call to ata_dev_power_set_active()
on the first run ? You would want to retry it but your patch will prevent that.

I do not really see any fundamental issue here given that calling
ata_dev_power_set_active() is indeed useless if the drive is already active, but
that does not hurt either. The only overhead is issuing a check power mode
command (see the call to ata_dev_power_is_active() in ata_dev_power_set_active()).

Are you seeing different behavior with your system ? Any error ?

> ---
>  drivers/ata/libata-core.c | 4 ++--
>  drivers/ata/libata-eh.c   | 4 ----
>  2 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index d1389ce6943e..ca3ca8107a3e 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5223,7 +5223,7 @@ static void ata_port_request_pm(struct ata_port *ap, pm_message_t mesg,
>  	/* on system resume, don't wake PuiS drives */
>  	if (mesg.event == PM_EVENT_RESUME)
>  		ehi_flags |= ATA_EHI_NOSTART;
> -	
> +
>  	/* Request PM operation to EH */
>  	ap->pm_mesg = mesg;
>  	ap->pflags |= ATA_PFLAG_PM_PENDING;
> @@ -5297,7 +5297,7 @@ static int ata_port_pm_poweroff(struct device *dev)
>  static void ata_port_resume(struct ata_port *ap, pm_message_t mesg,
>  			    bool async)
>  {
> -	ata_port_request_pm(ap, mesg, ATA_EH_RESET,
> +	ata_port_request_pm(ap, mesg, ATA_EH_RESET | ATA_EH_SET_ACTIVE,
>  			    ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET,
>  			    async);
>  }
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 120f7d7fb450..f44ce7068a8b 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -711,10 +711,6 @@ void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap)
>  			ehc->saved_xfer_mode[devno] = dev->xfer_mode;
>  			if (ata_ncq_enabled(dev))
>  				ehc->saved_ncq_enabled |= 1 << devno;
> -
> -			/* If we are resuming, wake up the device */
> -			if (ap->pflags & ATA_PFLAG_RESUMING)
> -				ehc->i.dev_action[devno] |= ATA_EH_SET_ACTIVE;
>  		}
>  	}
>  

-- 
Damien Le Moal
Western Digital Research