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
