On Sat, Apr 25, 2015 at 11:34:29AM +0200, Gabriele Mazzotta wrote: > When the LPM policy is set to ATA_LPM_MAX_POWER, the device might > generate a spurious PHY event that might cause errors on the link. > Ignore this event if it occurred within 10s after the policy change. > > The timeout was chosen observing that on a Dell XPS13 9333 these > spurious events can occur up to roughly 6s after the policy change. Just a couple things. Can you please add the following tag? Link: http://lkml.kernel.org/g/<MSG-ID of the first message in this thread> > diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c > index 61a9c07..59a2517 100644 > --- a/drivers/ata/libahci.c > +++ b/drivers/ata/libahci.c > @@ -1700,6 +1700,9 @@ static void ahci_handle_port_interrupt(struct ata_port *ap, > struct ahci_port_priv *pp = ap->private_data; > struct ahci_host_priv *hpriv = ap->host->private_data; > int resetting = !!(ap->pflags & ATA_PFLAG_RESETTING); > + unsigned long lpm_timeout = ap->link.last_lpm_change + > + msecs_to_jiffies(ATA_TMOUT_SPURIOUS_PHY); > + bool ignore_event = false; > u32 qc_active = 0; > int rc; > > @@ -1707,8 +1710,16 @@ static void ahci_handle_port_interrupt(struct ata_port *ap, > if (unlikely(resetting)) > status &= ~PORT_IRQ_BAD_PMP; > > + /* ignore the first PHY event after the LPM policy changed > + * as it is might be spurious > + */ > + if ((ap->link.flags & ATA_LFLAG_CHANGED) && > + time_before(jiffies, lpm_timeout)) > + ignore_event = true; Maybe the following is better? ignore_event = (ap->link.flags & ATA_LFLAG_CHANGED) && time_before(jiffies, lpm_timeout); > + > /* if LPM is enabled, PHYRDY doesn't mean anything */ > - if (ap->link.lpm_policy > ATA_LPM_MAX_POWER) { > + if (ap->link.lpm_policy > ATA_LPM_MAX_POWER || ignore_event) { > + ap->link.flags &= ~ATA_LFLAG_CHANGED; > status &= ~PORT_IRQ_PHYRDY; > ahci_scr_write(&ap->link, SCR_ERROR, SERR_PHYRDY_CHG); > } I thought more about it and it's weird to have ATA_LFLAG_CHANGED and last_lpm_change timestamp in libata core side and then open-code the actual logic in ahci. How about implementing a generic helper, say, bool sata_lpm_ignore_phy_events(link) in libata core and use it in specific drivers? It can test both the current lpm_policy and the timeout (preferably a separate patch to introduce the function and factor out lpm policy testing). Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
