Patchwork sata_sil24: external raid storage mistaken as port multiplier

login
register
mail settings
Submitter Tejun Heo
Date Nov. 26, 2010, 5:19 p.m.
Message ID <4CEFEC17.6060503@kernel.org>
Download mbox | patch
Permalink /patch/73207/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Tejun Heo - Nov. 26, 2010, 5:19 p.m.
On 11/24/2010 05:23 AM, Tobias Karnat wrote:
> Hi,
> 
> I got it fixed by removing ATA_FLAG_PMP from the SIL24_COMMON_FLAGS.
> 
> Could someone turn this into a module option?
> 
> The external raid case might in fact has a built-in port multiplier,
> but the case can only be configured as raid0 and raid1.
> 
> I suspect that Linux tries to to access the drives separately, which fails.

Hmmm... well, libata is just sending SRST w/ the port number set to 15
and the device is reporting that it is a port multipler to that.
Depending on configuration these devices don't work too well when
commanded as a PMP device.  If you put it into JBOD mode, it will
probably work fine.  I have no idea why it still reports as a PMP
device when configured as a virtual device.

That said, yeah, it probably would be a good idea to add a
libata.force param.

Can you please apply the following patch and verify that the device
doesn't work without any parameter but it does with
"libata.force=nopmp"?

Thanks.

--
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
Mark Lord - Nov. 26, 2010, 8:13 p.m.
On 10-11-26 12:19 PM, Tejun Heo wrote:
> On 11/24/2010 05:23 AM, Tobias Karnat wrote:
>> Hi,
>>
>> I got it fixed by removing ATA_FLAG_PMP from the SIL24_COMMON_FLAGS.
>>
>> Could someone turn this into a module option?
>>
>> The external raid case might in fact has a built-in port multiplier,
>> but the case can only be configured as raid0 and raid1.
>>
>> I suspect that Linux tries to to access the drives separately, which fails.
>
> Hmmm... well, libata is just sending SRST w/ the port number set to 15
> and the device is reporting that it is a port multipler to that.
> Depending on configuration these devices don't work too well when
> commanded as a PMP device.  If you put it into JBOD mode, it will
> probably work fine.  I have no idea why it still reports as a PMP
> device when configured as a virtual device.
>
> That said, yeah, it probably would be a good idea to add a
> libata.force param.

How about some form of auto-detection instead?

-ml
--
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
Tejun Heo - Nov. 26, 2010, 8:16 p.m.
Hello,

On 11/26/2010 09:13 PM, Mark Lord wrote:
>> That said, yeah, it probably would be a good idea to add a
>> libata.force param.
> 
> How about some form of auto-detection instead?

That's a device which reports that it's a PMP but fails to behave
itself as one.  The sucky thing is that depending on the current mode
and firmware revision, the same hardware can actually bahave as a
pretty decent PMP.  I frankly have no idea how to work it around.
Mark Lord - Nov. 27, 2010, 2:57 a.m.
On 10-11-26 03:16 PM, Tejun Heo wrote:
> Hello,
>
> On 11/26/2010 09:13 PM, Mark Lord wrote:
>>> That said, yeah, it probably would be a good idea to add a
>>> libata.force param.
>>
>> How about some form of auto-detection instead?
>
> That's a device which reports that it's a PMP but fails to behave
> itself as one.  The sucky thing is that depending on the current mode
> and firmware revision, the same hardware can actually bahave as a
> pretty decent PMP.

Yuck!  :)

I suppose *if* we knew the exact fwrev that requires the workaround,
then we could make it automatic for that, and still have the boot flag
for cases we don't know about.

Tobias?  Got the IDENTIFY info from that device?
Something like "hdparm --istdout /dev/sdX"  ?
--
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
Tobias Karnat - Nov. 28, 2010, 10:51 a.m.
Am Freitag, den 26.11.2010, 18:19 +0100 schrieb Tejun Heo:
> That said, yeah, it probably would be a good idea to add a
> libata.force param.
> 
> Can you please apply the following patch and verify that the device
> doesn't work without any parameter but it does with
> "libata.force=nopmp"?

Sorry, but I am currently happy with the generic Ubuntu Kernel
with the module recompiled.

Maybe I am just lazy, but I have scheduled to compile my first Kernel
on Ubuntu 10.10 when 2.6.37 will be out.

Am Freitag, den 26.11.2010, 21:57 -0500 schrieb Mark Lord:
> I suppose *if* we knew the exact fwrev that requires the workaround,
> then we could make it automatic for that, and still have the boot flag
> for cases we don't know about.
> 
> Tobias?  Got the IDENTIFY info from that device?
> Something like "hdparm --istdout /dev/sdX"  ?

Yes, hdparm --istdout /dev/sdh gives,

/dev/sdh:
0040 3fff c837 0010 0000 0000 003f 0000
0000 0000 3037 3043 3035 465f 3837 3933
3931 5f5f 5f30 5f38 0003 3e00 0004 5247
4c31 3034 3033 4578 7465 726e 616c 2044
6973 6b20 3020 2020 2020 2020 2020 2020
2020 2020 2020 2020 2020 2020 2020 8001
0000 2f00 4000 0200 0000 0006 0000 0000
0000 0000 0000 0101 ffff 0fff 0000 0407
0003 0078 0078 0078 0078 0000 0000 0000
0000 0000 0000 0000 0201 0000 0000 0000
007e 001b 0069 7460 4040 0028 3460 4040
207f 0000 0000 0000 fffe 0000 c0fe 0000
0000 0000 0000 0000 c060 7470 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0001 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0017 2040
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 a3a5

Tobias


--
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
Tobias Karnat - Dec. 3, 2010, 12:45 p.m.
Am Sonntag, den 28.11.2010, 11:51 +0100 schrieb Tobias Karnat:
> Am Freitag, den 26.11.2010, 18:19 +0100 schrieb Tejun Heo:
> > That said, yeah, it probably would be a good idea to add a
> > libata.force param.
> > 
> > Can you please apply the following patch and verify that the device
> > doesn't work without any parameter but it does with
> > "libata.force=nopmp"?
> 
> Sorry, but I am currently happy with the generic Ubuntu Kernel
> with the module recompiled.
> 
> Maybe I am just lazy, but I have scheduled to compile my first Kernel
> on Ubuntu 10.10 when 2.6.37 will be out.

Well, I'm now on 2.6.36.1, because I had an problem with Kaffeine and
2.6.37-rc4. But I don't like the idea to force to not use pmp on every
controller. I don't have an pmp which I use, but maybe someone else does
and also has the problem with the external case.

> Am Freitag, den 26.11.2010, 21:57 -0500 schrieb Mark Lord:
> > I suppose *if* we knew the exact fwrev that requires the workaround,
> > then we could make it automatic for that, and still have the boot flag
> > for cases we don't know about.
> > 
> > Tobias?  Got the IDENTIFY info from that device?
> > Something like "hdparm --istdout /dev/sdX"  ?
> 
> Yes, hdparm --istdout /dev/sdh gives,
> 
> /dev/sdh:
> 0040 3fff c837 0010 0000 0000 003f 0000
> 0000 0000 3037 3043 3035 465f 3837 3933
> 3931 5f5f 5f30 5f38 0003 3e00 0004 5247
> 4c31 3034 3033 4578 7465 726e 616c 2044
> 6973 6b20 3020 2020 2020 2020 2020 2020
> 2020 2020 2020 2020 2020 2020 2020 8001
> 0000 2f00 4000 0200 0000 0006 0000 0000
> 0000 0000 0000 0101 ffff 0fff 0000 0407
> 0003 0078 0078 0078 0078 0000 0000 0000
> 0000 0000 0000 0000 0201 0000 0000 0000
> 007e 001b 0069 7460 4040 0028 3460 4040
> 207f 0000 0000 0000 fffe 0000 c0fe 0000
> 0000 0000 0000 0000 c060 7470 0000 0000
> 0000 0000 0000 0000 0000 0000 0000 0000
> 0000 0000 0000 0000 0000 0000 0000 0000
> 0000 0000 0000 0000 0000 0000 0000 0000
> 0001 0000 0000 0000 0000 0000 0000 0000
> 0000 0000 0000 0000 0000 0000 0000 0000
> 0000 0000 0000 0000 0000 0000 0017 2040
> 0000 0000 0000 0000 0000 0000 0000 0000
> 0000 0000 0000 0000 0000 0000 0000 0000
> 0000 0000 0000 0000 0000 0000 0000 0000
> 0000 0000 0000 0000 0000 0000 0000 0000
> 0000 0000 0000 0000 0000 0000 0000 0000
> 0000 0000 0000 0000 0000 0000 0000 0000
> 0000 0000 0000 0000 0000 0000 0000 0000
> 0000 0000 0000 0000 0000 0000 0000 0000
> 0000 0000 0000 0000 0000 0000 0000 0000
> 0000 0000 0000 0000 0000 0000 0000 0000
> 0000 0000 0000 0000 0000 0000 0000 0000
> 0000 0000 0000 0000 0000 0000 0000 0000
> 0000 0000 0000 0000 0000 0000 0000 a3a5

I hope this is enough info?

-Tobias

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

Patch

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 7f77c67..7423265 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6325,6 +6325,7 @@  static int __init ata_parse_force_one(char **cur,
 		{ "nohrst",	.lflags		= ATA_LFLAG_NO_HRST },
 		{ "nosrst",	.lflags		= ATA_LFLAG_NO_SRST },
 		{ "norst",	.lflags		= ATA_LFLAG_NO_HRST | ATA_LFLAG_NO_SRST },
+		{ "nopmp",	.lflags		= ATA_LFLAG_NO_PMP },
 	};
 	char *start = *cur, *p = *cur;
 	char *id, *val, *endp;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index d947b12..6102ba2 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -174,6 +174,7 @@  enum {
 	ATA_LFLAG_DISABLED	= (1 << 6), /* link is disabled */
 	ATA_LFLAG_SW_ACTIVITY	= (1 << 7), /* keep activity stats */
 	ATA_LFLAG_NO_LPM	= (1 << 8), /* disable LPM on this link */
+	ATA_LFLAG_NO_PMP	= (1 << 9), /* disable PMP support */

 	/* struct ata_port flags */
 	ATA_FLAG_SLAVE_POSS	= (1 << 0), /* host supports slave dev */
@@ -1210,7 +1211,8 @@  extern struct device_attribute *ata_common_sdev_attrs[];
 #ifdef CONFIG_SATA_PMP
 static inline bool sata_pmp_supported(struct ata_port *ap)
 {
-	return ap->flags & ATA_FLAG_PMP;
+	return (ap->flags & ATA_FLAG_PMP) &&
+		!(ap->link.flags & ATA_LFLAG_NO_PMP);
 }

 static inline bool sata_pmp_attached(struct ata_port *ap)