| Message ID | 20230915081507.761711-2-dlemoal@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix libata suspend/resume handling and code cleanup | expand |
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?
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.
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. Since the only place that sets ATA_PFLAG_PM_PENDING is ata_port_request_pm() and since PM core holds the device lock (device_lock()), I don't think that ATA_PFLAG_PM_PENDING can get set while inside ata_port_request_pm(). And since we wait for EH to complete, and since both ata_eh_handle_port_suspend() and ata_eh_handle_port_resume() are called unconditionally by EH, they will only return if ATA_PFLAG_PM_PENDING is not set, and since these functions both clear ATA_PFLAG_PM_PENDING unconditionally, I would agree with you, that these two WARN_ON() seem superfluous. (Yes, EH could trigger again if we got an error IRQ before ata_port_request_pm() takes the lock the second time, but that can only set ATA_PFLAG_EH_PENDING, it can not set ATA_PFLAG_PM_PENDING.) Kind regards, Niklas
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
On 2023/09/20 0:21, 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. Let's do the renaming in a followup patch, not in this fix patch. > > Since the only place that sets ATA_PFLAG_PM_PENDING is ata_port_request_pm() > and since PM core holds the device lock (device_lock()), I don't think that > ATA_PFLAG_PM_PENDING can get set while inside ata_port_request_pm(). > And since we wait for EH to complete, and since both > ata_eh_handle_port_suspend() and ata_eh_handle_port_resume() are called > unconditionally by EH, they will only return if ATA_PFLAG_PM_PENDING is not > set, and since these functions both clear ATA_PFLAG_PM_PENDING unconditionally, > I would agree with you, that these two WARN_ON() seem superfluous. > > (Yes, EH could trigger again if we got an error IRQ before ata_port_request_pm() > takes the lock the second time, but that can only set ATA_PFLAG_EH_PENDING, > it can not set ATA_PFLAG_PM_PENDING.) > > > Kind regards, > Niklas
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@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 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
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); - } } /*
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 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(-)