Re: [PATCH v3 01/23] ata: libata-core: Fix ata_port_request_pm() locking

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

On 2023/09/19 6:21, Niklas Cassel wrote:
> 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@vger.kernel.org
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>> Reviewed-by: Hannes Reinecke <hare@suse.de>
>> Tested-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com>
>> ---
>>  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?

Yes, they have the same worth == not super useful... I kept the one higher up as
it is OK because we hold the lock, but removed the second one as checking pflags
without the lock is just plain wrong. Thinking of it, the first WRN_ON() is also
wrong I think because EH could be rescheduled right after wait_eh and before we
take the lock. In that case, the warn on would be a flase positive. I will
remove it as well.

-- 
Damien Le Moal
Western Digital Research