Hotplug issue with Marvell 6121

Hotplug issue with Marvell 6121

[Justin Maggard]

I've been experiencing some issues with disk hotplugging using the
AHCI driver with a Marvell 6121 controller.  It varies a bit when the
driver gives up, but it's almost always within the first three
remove/add attempts.  I was using 2.6.27.6 for quite some time and
never had any issues with hotplug, even after many hotplugs.  The
problems started when I updated to 2.6.29, and they still exist today
on 2.6.33.  I'm running a regular i686 non-SMP kernel.  Anyone have
any ideas?

Below are some relevant portions of the kernel log from 2.6.27 and
2.6.33.  If anyone needs any more information from me, please let me
know.

Thanks,
-Justin

=== Removing disk, 2.6.27.6 ===
ata3: exception Emask 0x10 SAct 0x0 SErr 0x0 action 0xe frozen
ata3: irq_stat 0x04400000, PHY RDY changed
ata3: hard resetting link
ata3: SATA link down (SStatus 0 SControl 300)
ata3: hard resetting link
ata3: SATA link down (SStatus 0 SControl 300)
ata3: hard resetting link
ata3: SATA link down (SStatus 0 SControl 300)
ata3.00: disabled
ata3: EH complete
ata3.00: detaching (SCSI 2:0:0:0)
sd 2:0:0:0: [sdc] Synchronizing SCSI cache
sd 2:0:0:0: [sdc] Result: hostbyte=DID_BAD_TARGET
driverbyte=DRIVER_OK,SUGGEST_OK
sd 2:0:0:0: [sdc] Stopping disk
sd 2:0:0:0: [sdc] START_STOP FAILED
sd 2:0:0:0: [sdc] Result: hostbyte=DID_BAD_TARGET
driverbyte=DRIVER_OK,SUGGEST_OK

=== Removing disk, 2.6.33 ===
ata3: exception Emask 0x10 SAct 0x0 SErr 0x0 action 0xe frozen
ata3: irq_stat 0x04400000, PHY RDY changed
ata3: hard resetting link
ata3: SATA link down (SStatus 0 SControl 300)
ata3: hard resetting link
ata3: SATA link down (SStatus 0 SControl 300)
ata3: limiting SATA link speed to 1.5 Gbps
ata3: hard resetting link
ata3: SATA link down (SStatus 0 SControl 310)
ata3.00: disabled
ata3: EH complete
ata3.00: detaching (SCSI 2:0:0:0)
sd 2:0:0:0: [sdc] Synchronizing SCSI cache
sd 2:0:0:0: [sdc] Result: hostbyte=DID_BAD_TARGET driverbyte=DRIVER_OK
sd 2:0:0:0: [sdc] Stopping disk
sd 2:0:0:0: [sdc] START_STOP FAILED
sd 2:0:0:0: [sdc] Result: hostbyte=DID_BAD_TARGET driverbyte=DRIVER_OK

=== Adding disk, 2.6.27.6 ===
ata3: exception Emask 0x10 SAct 0x0 SErr 0x0 action 0xe frozen
ata3: irq_stat 0x02400000, PHY RDY changed
ata3: hard resetting link
ata3: link is slow to respond, please be patient (ready=0)
ata3: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
ata3.00: ATA-8: ST3750330AS, SD15, max UDMA/133
ata3.00: 1465149168 sectors, multi 0: LBA48 NCQ (depth 0/32)
ata3.00: configured for UDMA/133
ata3: EH complete
scsi 2:0:0:0: Direct-Access     ATA      ST3750330AS      SD15 PQ: 0 ANSI: 5
sd 2:0:0:0: [sdc] 1465149168 512-byte hardware sectors (750156 MB)
sd 2:0:0:0: [sdc] Write Protect is off
sd 2:0:0:0: [sdc] Mode Sense: 00 3a 00 00
sd 2:0:0:0: [sdc] Write cache: enabled, read cache: enabled, doesn't
support DPO or FUA
sd 2:0:0:0: [sdc] 1465149168 512-byte hardware sectors (750156 MB)
sd 2:0:0:0: [sdc] Write Protect is off
sd 2:0:0:0: [sdc] Mode Sense: 00 3a 00 00
sd 2:0:0:0: [sdc] Write cache: enabled, read cache: enabled, doesn't
support DPO or FUA
 sdc: sdc1 sdc2 sdc4 < sdc5 >
sd 2:0:0:0: [sdc] Attached SCSI disk
sd 2:0:0:0: Attached scsi generic sg2 type 0

=== Adding disk, 2.6.33 ===
ata3: exception Emask 0x10 SAct 0x0 SErr 0x0 action 0xe frozen
ata3: irq_stat 0x02400000, PHY RDY changed
ata3: hard resetting link
ata3: link is slow to respond, please be patient (ready=0)
ata3: COMRESET failed (errno=-16)
ata3: hard resetting link
ata3: link is slow to respond, please be patient (ready=0)
ata3: COMRESET failed (errno=-16)
ata3: hard resetting link
ata3: link is slow to respond, please be patient (ready=0)
ata3: COMRESET failed (errno=-16)
ata3: limiting SATA link speed to 1.5 Gbps
ata3: hard resetting link
ata3: COMRESET failed (errno=-16)
ata3: reset failed, giving up
ata3: EH complete

Re: Hotplug issue with Marvell 6121

[Justin Maggard]

On Tue, Mar 9, 2010 at 4:51 PM, Justin Maggard <jmaggard10@gmail.com> wrote:
> I've been experiencing some issues with disk hotplugging using the
> AHCI driver with a Marvell 6121 controller.  It varies a bit when the
> driver gives up, but it's almost always within the first three
> remove/add attempts.  I was using 2.6.27.6 for quite some time and
> never had any issues with hotplug, even after many hotplugs.  The
> problems started when I updated to 2.6.29, and they still exist today
> on 2.6.33.  I'm running a regular i686 non-SMP kernel.  Anyone have
> any ideas?
>
Okay, so I did some more investigation and narrowed down the issue.
It seems the Marvell controller (at least the 6121) doesn't like when
you limit the link speed.  So when the libata error handler slows down
the link speed on the last hard reset after the disk is removed, there
is a good chance that a newly-added disk on that channel will not be
recognized when you plug it in.

I'm sure there's a much better way to fix it, as this patch feels
terribly wrong... but it fixes the issue for me.  Any chance of this
getting addressed upstream?

--- linux-2.6.33.orig/drivers/ata/libata-eh.c	2010-02-24
10:52:17.000000000 -0800
+++ linux-2.6.33/drivers/ata/libata-eh.c	2010-03-10 13:13:37.000000000 -0800
@@ -3300,9 +3305,11 @@ static int ata_eh_schedule_probe(struct
 	return 1;
 }

+#define AHCI_HFLAG_MV_PATA (1 << 4)
 static int ata_eh_handle_dev_fail(struct ata_device *dev, int err)
 {
 	struct ata_eh_context *ehc = &dev->link->eh_context;
+	unsigned int *hflags = dev->link->ap->host->private_data;

 	/* -EAGAIN from EH routine indicates retry without prejudice.
 	 * The requester is responsible for ensuring forward progress.
@@ -3322,7 +3329,12 @@ static int ata_eh_handle_dev_fail(struct
 			/* This is the last chance, better to slow
 			 * down than lose it.
 			 */
-			sata_down_spd_limit(ata_dev_phys_link(dev), 0);
+			if (*hflags & AHCI_HFLAG_MV_PATA) {
+				ata_dev_printk(dev, KERN_WARNING,
+					"Marvell controller detected,"
+					" not slowing down link\n");
+			} else
+				sata_down_spd_limit(ata_dev_phys_link(dev), 0);
 			if (dev->pio_mode > XFER_PIO_0)
 				ata_down_xfermask_limit(dev, ATA_DNXFER_PIO);
 		}

Re: Hotplug issue with Marvell 6121

[Robert Hancock]

On 03/10/2010 04:46 PM, Justin Maggard wrote:
> On Tue, Mar 9, 2010 at 4:51 PM, Justin Maggard<jmaggard10@gmail.com>  wrote:
>> I've been experiencing some issues with disk hotplugging using the
>> AHCI driver with a Marvell 6121 controller.  It varies a bit when the
>> driver gives up, but it's almost always within the first three
>> remove/add attempts.  I was using 2.6.27.6 for quite some time and
>> never had any issues with hotplug, even after many hotplugs.  The
>> problems started when I updated to 2.6.29, and they still exist today
>> on 2.6.33.  I'm running a regular i686 non-SMP kernel.  Anyone have
>> any ideas?
>>
> Okay, so I did some more investigation and narrowed down the issue.
> It seems the Marvell controller (at least the 6121) doesn't like when
> you limit the link speed.  So when the libata error handler slows down
> the link speed on the last hard reset after the disk is removed, there
> is a good chance that a newly-added disk on that channel will not be
> recognized when you plug it in.
>
> I'm sure there's a much better way to fix it, as this patch feels
> terribly wrong... but it fixes the issue for me.  Any chance of this
> getting addressed upstream?
>
> --- linux-2.6.33.orig/drivers/ata/libata-eh.c	2010-02-24
> 10:52:17.000000000 -0800
> +++ linux-2.6.33/drivers/ata/libata-eh.c	2010-03-10 13:13:37.000000000 -0800
> @@ -3300,9 +3305,11 @@ static int ata_eh_schedule_probe(struct
>   	return 1;
>   }
>
> +#define AHCI_HFLAG_MV_PATA (1<<  4)
>   static int ata_eh_handle_dev_fail(struct ata_device *dev, int err)
>   {
>   	struct ata_eh_context *ehc =&dev->link->eh_context;
> +	unsigned int *hflags = dev->link->ap->host->private_data;
>
>   	/* -EAGAIN from EH routine indicates retry without prejudice.
>   	 * The requester is responsible for ensuring forward progress.
> @@ -3322,7 +3329,12 @@ static int ata_eh_handle_dev_fail(struct
>   			/* This is the last chance, better to slow
>   			 * down than lose it.
>   			 */
> -			sata_down_spd_limit(ata_dev_phys_link(dev), 0);
> +			if (*hflags&  AHCI_HFLAG_MV_PATA) {
> +				ata_dev_printk(dev, KERN_WARNING,
> +					"Marvell controller detected,"
> +					" not slowing down link\n");
> +			} else
> +				sata_down_spd_limit(ata_dev_phys_link(dev), 0);
>   			if (dev->pio_mode>  XFER_PIO_0)
>   				ata_down_xfermask_limit(dev, ATA_DNXFER_PIO);
>   		}

That doesn't sound like a great solution. I'd think that the speed 
limiting should be reset upon disabling the device so that we start over 
at 3Gbps again when you reconnect..

Re: Hotplug issue with Marvell 6121

[Tejun Heo]

Hello,

On 03/11/2010 09:53 AM, Robert Hancock wrote:
> That doesn't sound like a great solution. I'd think that the speed
> limiting should be reset upon disabling the device so that we start over
> at 3Gbps again when you reconnect..

libata EH already does that.  It resets the speed limit mask after the
device is disabled and when the *next* reset happens on device
hotplug, the highest speed will be applied.  It's just that there
usually is no need to do an extra reset right after disabling a
device.

Does 1.5Gbps work at all on those marvell controllers?  It's *very*
strange to miss hotplug under 1.5Gbps.  Those signals are OOB and
don't even follow the usual signal frequency.

Thanks.

-- 
tejun

Re: Hotplug issue with Marvell 6121

[Justin Maggard]

On Wed, Mar 10, 2010 at 5:01 PM, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> On 03/11/2010 09:53 AM, Robert Hancock wrote:
>> That doesn't sound like a great solution. I'd think that the speed
>> limiting should be reset upon disabling the device so that we start over
>> at 3Gbps again when you reconnect..
>
> libata EH already does that.  It resets the speed limit mask after the
> device is disabled and when the *next* reset happens on device
> hotplug, the highest speed will be applied.  It's just that there
> usually is no need to do an extra reset right after disabling a
> device.
>
> Does 1.5Gbps work at all on those marvell controllers?  It's *very*
> strange to miss hotplug under 1.5Gbps.  Those signals are OOB and
> don't even follow the usual signal frequency.

It's not the 1.5Gbps that is causing the problem.  It appears to be
the act of changing speeds.  I added "libata.force=3:1.5Gbps" to my
kernel command line, and hotplugs seemed to work fine on that port
(presumably due to the fact that libata EH won't change the link speed
limit if it's already at 1.5Gbps).  The other port on the controller
still had hotplug problems.  So everything seems to be pointing to the
Marvell controller having problems when you change speeds.

-Justin

Re: Hotplug issue with Marvell 6121

[Mark Lord]

On 03/10/10 21:32, Justin Maggard wrote:
>
> It's not the 1.5Gbps that is causing the problem.  It appears to be
> the act of changing speeds.  I added "libata.force=3:1.5Gbps" to my
> kernel command line, and hotplugs seemed to work fine on that port
> (presumably due to the fact that libata EH won't change the link speed
> limit if it's already at 1.5Gbps).  The other port on the controller
> still had hotplug problems.  So everything seems to be pointing to the
> Marvell controller having problems when you change speeds.
...

If it's like their non-AHCI controllers (sata_mv), then the chipset/phy
could be very particular about the sequence/timing used when changing speeds.

-ml

Re: Hotplug issue with Marvell 6121

[Tejun Heo]

Hello,

On 03/11/2010 12:33 PM, Mark Lord wrote:
> On 03/10/10 21:32, Justin Maggard wrote:
>>
>> It's not the 1.5Gbps that is causing the problem.  It appears to be
>> the act of changing speeds.  I added "libata.force=3:1.5Gbps" to my
>> kernel command line, and hotplugs seemed to work fine on that port
>> (presumably due to the fact that libata EH won't change the link speed
>> limit if it's already at 1.5Gbps).  The other port on the controller
>> still had hotplug problems.  So everything seems to be pointing to the
>> Marvell controller having problems when you change speeds.
> ...
> 
> If it's like their non-AHCI controllers (sata_mv), then the chipset/phy
> could be very particular about the sequence/timing used when changing
> speeds.

BTW, if not allowing PHY speed adjustment is necessary, the correct
way to implement that is in ->port_start() by modifying
link->hw_sata_spd_limit.  But I really hope there's some other way to
solve this.

Thanks.

-- 
tejun

Re: Hotplug issue with Marvell 6121

[Justin Maggard]

On Wed, Mar 10, 2010 at 7:45 PM, Tejun Heo <tj@kernel.org> wrote:
> On 03/11/2010 12:33 PM, Mark Lord wrote:
>> If it's like their non-AHCI controllers (sata_mv), then the chipset/phy
>> could be very particular about the sequence/timing used when changing
>> speeds.

This is certainly possible.  The 6121 has a PATA port, and can be used
with the sata_mv driver also.

> BTW, if not allowing PHY speed adjustment is necessary, the correct
> way to implement that is in ->port_start() by modifying
> link->hw_sata_spd_limit.  But I really hope there's some other way to
> solve this.

I assume you're talking about setting hw_sata_spd_limit to 1.5Gbps so
that sata_down_spd_limit() doesn't do anything?  That will work, but
it would still be nice to be able to run at 3.0Gbps on these ports.

If anyone has any more elegant solutions they would like me to test,
just let me know.

Thanks,
-Justin

Re: Hotplug issue with Marvell 6121

[Tejun Heo]

On 03/12/2010 03:42 AM, Justin Maggard wrote:
>> BTW, if not allowing PHY speed adjustment is necessary, the correct
>> way to implement that is in ->port_start() by modifying
>> link->hw_sata_spd_limit.  But I really hope there's some other way to
>> solve this.
> 
> I assume you're talking about setting hw_sata_spd_limit to 1.5Gbps so
> that sata_down_spd_limit() doesn't do anything?  That will work, but
> it would still be nice to be able to run at 3.0Gbps on these ports.

Hmmm... you can peg it at 3 Gbps.  That should work, I think.

Thanks.

-- 
tejun