On Fri, Sep 15, 2023 at 05:14:45PM +0900, Damien Le Moal wrote:
> The function ata_port_request_pm() checks the port flag
> ATA_PFLAG_PM_PENDING and calls ata_port_wait_eh() if this flag is set to
> ensure that power management operations for a port are not secheduled
s/secheduled/scheduled/
> simultaneously. However, this flag check is done without holding the
> port lock.
>
> Fix this by taking the port lock on entry to the function and checking
> the flag under this lock. The lock is released and re-taken if
> ata_port_wait_eh() needs to be called.
>
> Fixes: 5ef41082912b ("ata: add ata port system PM callbacks")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Damien Le Moal <dlemoal@xxxxxxxxxx>
> Reviewed-by: Hannes Reinecke <hare@xxxxxxx>
> Tested-by: Chia-Lin Kao (AceLan) <acelan.kao@xxxxxxxxxxxxx>
> ---
> drivers/ata/libata-core.c | 17 +++++++++--------
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 74314311295f..c4898483d716 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5040,17 +5040,20 @@ static void ata_port_request_pm(struct ata_port *ap, pm_message_t mesg,
> struct ata_link *link;
> unsigned long flags;
>
> - /* Previous resume operation might still be in
> - * progress. Wait for PM_PENDING to clear.
> + spin_lock_irqsave(ap->lock, flags);
> +
> + /*
> + * A previous PM operation might still be in progress. Wait for
> + * ATA_PFLAG_PM_PENDING to clear.
> */
> if (ap->pflags & ATA_PFLAG_PM_PENDING) {
> + spin_unlock_irqrestore(ap->lock, flags);
> ata_port_wait_eh(ap);
> + spin_lock_irqsave(ap->lock, flags);
> WARN_ON(ap->pflags & ATA_PFLAG_PM_PENDING);
> }
>
> - /* request PM ops to EH */
> - spin_lock_irqsave(ap->lock, flags);
> -
> + /* Request PM operation to EH */
> ap->pm_mesg = mesg;
> ap->pflags |= ATA_PFLAG_PM_PENDING;
> ata_for_each_link(link, ap, HOST_FIRST) {
> @@ -5062,10 +5065,8 @@ static void ata_port_request_pm(struct ata_port *ap, pm_message_t mesg,
>
> spin_unlock_irqrestore(ap->lock, flags);
>
> - if (!async) {
> + if (!async)
> ata_port_wait_eh(ap);
> - WARN_ON(ap->pflags & ATA_PFLAG_PM_PENDING);
Perhaps you should mention why this WARN_ON() is removed in the commit
message.
I don't understand why you keep the WARN_ON() higher up in this function,
but remove this WARN_ON(). They seem to have equal worth to me.
Perhaps just take and release the lock around the WARN_ON() here as well?
