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

[Niklas Cassel <Niklas.Cassel@wdc.com>]

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@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.
> 
> 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 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