| Message ID | CAA9_cmfzgNkep_ZBKwyvW3iUXnKyNjpbB1uGtDLonbD1wCM10g@mail.gmail.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512 On 01/10/2014 05:26 PM, Dan Williams wrote: > I was going to comment that this leaves us open to a race to > submit new i/o before recovery starts, but scsi_schedule_eh already > blocks new i/o, so I think we're good. I think it deserves a > comment here about why it's safe to not care. I don't follow, could you explain? >> >> static int ata_port_resume(struct device *dev) { int rc; >> >> + if (pm_runtime_suspended(dev)) + return 0; > > Why? If we dpm_resume() a port that was rpm_suspend()ed the state > needs to be reset to active. I think this check also forces the > port to resume twice, once for system resume and again for the > eventual runtime_resume. It stops the first resume by returning early, so only the second one happens. > ...so, the pm_runtime_suspended() check should go and something > like this folded in: > > diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c > index 92d7797223be..a6cc107c9434 100644 --- > a/drivers/ata/libata-eh.c +++ b/drivers/ata/libata-eh.c @@ -4143,5 > +4143,11 @@ static void ata_eh_handle_port_resume(struct ata_port > *ap) ap->pm_result = NULL; } spin_unlock_irqrestore(ap->lock, > flags); + + if (rc == 0) { + > pm_runtime_disable(dev); + > pm_runtime_set_active(dev); + > pm_runtime_enable(dev); + } } #endif /* CONFIG_PM */ Ahh, and that will stop the second resume, rather than the previous change to stop the first? Don't we want to stop the first rather than the second? Otherwise if the port is runtime suspended at system suspend time ( maybe no drive connected to it? ) then there is no sense in resuming it at system resume time. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.14 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCgAGBQJS0KujAAoJEI5FoCIzSKrwhQsH/0f4AywqDM1y1BOPSFNuiF/X 5SRz1BaXCs7AZhSBcBrS5H9lZJLUQoKshUFXHtV9A4DuRhIghl+cu7JsJt3aPlmM u2yx3xETTj/iLqnL4shHVta/y9gXCfXhO7qZNtcDyPmiSgPNlFM4ZFTnnXtKaVgj PxZPzLqLA+Oh/iTgOXLsC/CECrLdWm6paTE6ii04QiabUjbK9vKk7EqazRnrlnsC I3MzulTu8jDLxMtwpdPPYUvi93BQr2P1Q11u/Rt8za+Y19h3UXpb2ATEiuHeYcNK F/HpXR+zDDSKSDnE591pF5q6gsC1MHt4+Zq0g7ZfpparO+2TciQo0hLs09hVEhI= =Cu86 -----END PGP SIGNATURE----- -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 10, 2014 at 6:25 PM, Phillip Susi <psusi@ubuntu.com> wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA512 > > On 01/10/2014 05:26 PM, Dan Williams wrote: >> I was going to comment that this leaves us open to a race to >> submit new i/o before recovery starts, but scsi_schedule_eh already >> blocks new i/o, so I think we're good. I think it deserves a >> comment here about why it's safe to not care. > > I don't follow, could you explain? The question I have when reading "__ata_port_resume_common(ap, mesg, &dontcare);" is what makes it safe for the port to not actually be resumed upon return. The answer I believe is that the host is guaranteed to be in the SHOST_RECOVERY state upon return and no new i/o submissions will occur until the error handler has a chance to run. >>> static int ata_port_resume(struct device *dev) { int rc; >>> >>> + if (pm_runtime_suspended(dev)) + return 0; >> >> Why? If we dpm_resume() a port that was rpm_suspend()ed the state >> needs to be reset to active. I think this check also forces the >> port to resume twice, once for system resume and again for the >> eventual runtime_resume. > > It stops the first resume by returning early, so only the second one > happens. Got it, but it looks odd. > >> ...so, the pm_runtime_suspended() check should go and something >> like this folded in: >> >> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c >> index 92d7797223be..a6cc107c9434 100644 --- >> a/drivers/ata/libata-eh.c +++ b/drivers/ata/libata-eh.c @@ -4143,5 >> +4143,11 @@ static void ata_eh_handle_port_resume(struct ata_port >> *ap) ap->pm_result = NULL; } spin_unlock_irqrestore(ap->lock, >> flags); + + if (rc == 0) { + >> pm_runtime_disable(dev); + >> pm_runtime_set_active(dev); + >> pm_runtime_enable(dev); + } } #endif /* CONFIG_PM */ > > Ahh, and that will stop the second resume, rather than the previous > change to stop the first? > > Don't we want to stop the first rather than the second? Otherwise if > the port is runtime suspended at system suspend time ( maybe no drive > connected to it? ) then there is no sense in resuming it at system > resume time. Hmm, if the drive disconnected during suspend I think we want to find that out sooner rather than later. The changelog should really read "Don't block the resume path waiting for the disk re-establish its link". Given it's async do we gain that much by deferring a one time check of the ports at resume? At the very least I think tying rpm_resume to dpm_resume like that is an additional change to defer to a separate patch. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c index 92d7797223be..a6cc107c9434 100644 --- a/drivers/ata/libata-eh.c +++ b/drivers/ata/libata-eh.c @@ -4143,5 +4143,11 @@ static void ata_eh_handle_port_resume(struct ata_port *ap) ap->pm_result = NULL; } spin_unlock_irqrestore(ap->lock, flags); + + if (rc == 0) { + pm_runtime_disable(dev); + pm_runtime_set_active(dev); + pm_runtime_enable(dev); + } }