SATA link power management issues

Hi,

It seems that the following patch prevents errors when the policy is
changed. Could anybody explain why?

Thanks,
Gabriele

--
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 Sunday 22 February 2015 21:53:35 Gabriele Mazzotta wrote:
> Hi,
> 
> It seems that the following patch prevents errors when the policy is
> changed. Could anybody explain why?
> 
> Thanks,
> Gabriele
> 
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 61a9c07..38d39f7 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -1708,7 +1708,10 @@ static void ahci_handle_port_interrupt(struct ata_port *ap,
>  		status &= ~PORT_IRQ_BAD_PMP;
>  
>  	/* 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 ||
> +	    ap->link.flags & ATA_LFLAG_CHANGED) {
> +		if (ap->link.flags & ATA_LFLAG_CHANGED)
> +			ap->link.flags &= ~ATA_LFLAG_CHANGED;
>  		status &= ~PORT_IRQ_PHYRDY;
>  		ahci_scr_write(&ap->link, SCR_ERROR, SERR_PHYRDY_CHG);
>  	}
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index d2029a4..e8f965c 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -3489,6 +3489,8 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
>  		}
>  	}
>  
> +	link->flags |= ATA_LFLAG_CHANGED;
> +
>  	return 0;
>  
>  fail:
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index fc03efa..5abf5f2 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -205,6 +205,7 @@ enum {
>  	ATA_LFLAG_SW_ACTIVITY	= (1 << 7), /* keep activity stats */
>  	ATA_LFLAG_NO_LPM	= (1 << 8), /* disable LPM on this link */
>  	ATA_LFLAG_RST_ONCE	= (1 << 9), /* limit recovery to one reset */
> +	ATA_LFLAG_CHANGED	= (1 << 10),
>  
>  	/* struct ata_port flags */
>  	ATA_FLAG_SLAVE_POSS	= (1 << 0), /* host supports slave dev */

Hi,

did anyone looked at the above change?

Thanks,
Gabriele
--
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 Mon, Apr 20, 2015 at 10:02:12PM +0200, Gabriele Mazzotta wrote:
> On Sunday 22 February 2015 21:53:35 Gabriele Mazzotta wrote:
> > It seems that the following patch prevents errors when the policy is
> > changed. Could anybody explain why?

The device is generating a spurious PHY event after LPM state change
and the patch is ignoring the event once after state change.  It's a
bit too specific.  Maybe we should do this timeout-based - e.g. ignore
phy events for 1s after LPM state change.  Gabriele, would be
interested in making such changes?

Thanks.

On Tuesday 21 April 2015 11:31:21 Tejun Heo wrote:
> On Mon, Apr 20, 2015 at 10:02:12PM +0200, Gabriele Mazzotta wrote:
> > On Sunday 22 February 2015 21:53:35 Gabriele Mazzotta wrote:
> > > It seems that the following patch prevents errors when the policy is
> > > changed. Could anybody explain why?
> 
> The device is generating a spurious PHY event after LPM state change
> and the patch is ignoring the event once after state change.  It's a
> bit too specific.  Maybe we should do this timeout-based - e.g. ignore
> phy events for 1s after LPM state change.  Gabriele, would be
> interested in making such changes?

Thanks for the explanation and yes, I am interested.

Doing some quick tests I found that in some cases it takes 5 or 6
seconds for the first interrupt to arrive, so I'd have to use a quite
long interval to completely prevent errors.

I am wondering if it would be better using my original solution
(i.e. ignore first event), but make it device specific given that it
might make no sense on other systems and seems to be more reliable than
the time-based one.

Gabriele
--
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

Hello,

On Tue, Apr 21, 2015 at 10:29:38PM +0200, Gabriele Mazzotta wrote:
> Doing some quick tests I found that in some cases it takes 5 or 6
> seconds for the first interrupt to arrive, so I'd have to use a quite
> long interval to completely prevent errors.

Hmm...

> I am wondering if it would be better using my original solution
> (i.e. ignore first event), but make it device specific given that it
> might make no sense on other systems and seems to be more reliable than
> the time-based one.

I'm not sure.  What if the extra PHY event isn't that reliable.  It'd
be pretty confusing if it ends up ignoring a legitimate PHY event
after, say, two hours, so one way or the other, we'd need to cap how
long we're gonna be ignoring the event.  Ignore the first PHY event
for 10s after LPM state change?

Thanks.