On 2023/09/20 0:30, Niklas Cassel wrote:
> On Wed, Sep 20, 2023 at 09:21:14AM +0200, Niklas Cassel wrote:
>> On Tue, Sep 19, 2023 at 09:31:04AM -0700, Damien Le Moal wrote:
>>> 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@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?
>>>
>>> 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.
>>
>> We are checking if ATA_PFLAG_PM_PENDING is set, if it is, we do
>> ata_port_wait_eh(), which will wait until both ATA_PFLAG_EH_PENDING and
>> ATA_PFLAG_EH_IN_PROGRESS is cleared.
>>
>> Note that ATA_PFLAG_PM_PENDING and ATA_PFLAG_EH_PENDING have very similar
>> names... I really think we should rename ATA_PFLAG_PM_PENDING to something
>> like ATA_PFLAG_EH_PM_PENDING (the PM is performed by EH), in order to make
>> it harder to mix them up.
>
> Perhaps ATA_PFLAG_POWER_STATE_PENDING is a better name?
That could be confused with a power state called "pending". Something like
ATA_PFLAG_EH_PM_REQUEST_PENDING would be more descriptive and different enough
from ATA_PFLAG_EH_PENDING.
>
> That way we make it even harder to mix them up, since my previous
> suggestion ATA_PFLAG_EH_PM_PENDING, people might still miss the _PM_ part
> when reading quickly and could still confuse it with ATA_PFLAG_EH_PENDING.
>
>
> Kind regards,
> Niklas
--
Damien Le Moal
Western Digital Research
