[PATCH] libsas: User mode app failed to send ioctl commands to SATA drive.

[PATCH] libsas: User mode app failed to send ioctl commands to SATA drive.

[Ke Wei]

The context of scsi_host's hostdata points to a sas ha structure with libsas
module, but the libata casts this pointer to a ata_port structure when ioctl
sends HDIO_GET_IDENTITY.

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 9e92107..e6acc55 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -412,20 +412,9 @@ int ata_std_bios_param(struct scsi_device *sdev, struct
block_device *bdev,
 	return 0;
 }

-/**
- *	ata_get_identity - Handler for HDIO_GET_IDENTITY ioctl
- *	@sdev: SCSI device to get identify data for
- *	@arg: User buffer area for identify data
- *
- *	LOCKING:
- *	Defined by the SCSI layer.  We don't really care.
- *
- *	RETURNS:
- *	Zero on success, negative errno on error.
- */
-static int ata_get_identity(struct scsi_device *sdev, void __user *arg)
+static int __ata_get_identity(struct scsi_device *sdev,
+			struct ata_port *ap, void __user *arg)
 {
-	struct ata_port *ap = ata_shost_to_port(sdev->host);
 	struct ata_device *dev = ata_scsi_find_dev(ap, sdev);
 	u16 __user *dst = arg;
 	char buf[40];
@@ -452,6 +441,23 @@ static int ata_get_identity(struct scsi_device *sdev, void
__user *arg)
 }

 /**
+ *	ata_get_identity - Handler for HDIO_GET_IDENTITY ioctl
+ *	@sdev: SCSI device to get identify data for
+ *	@arg: User buffer area for identify data
+ *
+ *	LOCKING:
+ *	Defined by the SCSI layer.  We don't really care.
+ *
+ *	RETURNS:
+ *	Zero on success, negative errno on error.
+ */
+static int ata_get_identity(struct scsi_device *sdev, void __user *arg)
+{
+	struct ata_port *ap = ata_shost_to_port(sdev->host);
+	return __ata_get_identity(sdev, ap, arg);
+}
+
+/**
  *	ata_cmd_ioctl - Handler for HDIO_DRIVE_CMD ioctl
  *	@scsidev: Device to which we are issuing command
  *	@arg: User provided data for issuing command
@@ -3714,3 +3720,10 @@ int ata_sas_queuecmd(struct scsi_cmnd *cmd, void
(*done)(struct scsi_cmnd *),
 	return rc;
 }
 EXPORT_SYMBOL_GPL(ata_sas_queuecmd);
+
+int ata_sas_get_identity(struct scsi_device *scsidev,
+			struct ata_port *ap, void __user *arg)
+{
+	return __ata_get_identity(scsidev, ap, arg);
+}
+EXPORT_SYMBOL_GPL(ata_sas_get_identity);

diff --git a/drivers/scsi/libsas/sas_scsi_host.c
b/drivers/scsi/libsas/sas_scsi_host.c
index 7448387..30c419c 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -46,6 +46,7 @@
 #include <linux/freezer.h>
 #include <linux/scatterlist.h>
 #include <linux/libata.h>
+#include <linux/hdreg.h>

 /* ---------- SCSI Host glue ---------- */

@@ -716,8 +717,13 @@ int sas_ioctl(struct scsi_device *sdev, int cmd, void
__user *arg)
 {
 	struct domain_device *dev = sdev_to_domain_dev(sdev);

-	if (dev_is_sata(dev))
-		return ata_scsi_ioctl(sdev, cmd, arg);
+	if (dev_is_sata(dev)) {
+		if (cmd == HDIO_GET_IDENTITY)
+			return ata_sas_get_identity(sdev, dev->sata_dev.ap,
+						arg);
+		else
+			return ata_scsi_ioctl(sdev, cmd, arg);
+	}

 	return -EINVAL;
 }

diff --git a/include/linux/libata.h b/include/linux/libata.h
index b6b8a7f..8c1c5e1 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -936,6 +936,8 @@ extern void ata_sas_port_stop(struct ata_port *ap);
 extern int ata_sas_slave_configure(struct scsi_device *, struct ata_port *);
 extern int ata_sas_queuecmd(struct scsi_cmnd *cmd, void (*done)(struct
scsi_cmnd *),
 			    struct ata_port *ap);
+extern int ata_sas_get_identity(struct scsi_device *scsidev,
+			struct ata_port *ap, void __user *arg);
 extern int sata_scr_valid(struct ata_link *link);
 extern int sata_scr_read(struct ata_link *link, int reg, u32 *val);
 extern int sata_scr_write(struct ata_link *link, int reg, u32 val);

Re: [PATCH] libsas: User mode app failed to send ioctl commands to SATA drive.

[James Bottomley]

On Mon, 2009-01-12 at 18:28 +0800, Ke Wei wrote:
> The context of scsi_host's hostdata points to a sas ha structure with libsas
> module, but the libata casts this pointer to a ata_port structure when ioctl
> sends HDIO_GET_IDENTITY.

This is a good find thanks.  I think there's a small problem in the way
the layering is done.

I think ata_scsi_ioctl needs to be expanded to pass the port in all the
time.   That way other implementors don't have to remember to special
case HDIO_GET_IDENTITY.

When you redo this, ipr will need altering as well (rather than also
having the identity exception added).

James

Re: [PATCH] libsas: User mode app failed to send ioctl commands to SATA drive.

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 870 bytes --]

James Bottomley wrote:
> On Mon, 2009-01-12 at 18:28 +0800, Ke Wei wrote:
>> The context of scsi_host's hostdata points to a sas ha structure with libsas
>> module, but the libata casts this pointer to a ata_port structure when ioctl
>> sends HDIO_GET_IDENTITY.
> 
> This is a good find thanks.  I think there's a small problem in the way
> the layering is done.
> 
> I think ata_scsi_ioctl needs to be expanded to pass the port in all the
> time.   That way other implementors don't have to remember to special
> case HDIO_GET_IDENTITY.
> 
> When you redo this, ipr will need altering as well (rather than also
> having the identity exception added).

The attached appears to be what is needed.  We cannot change 
ata_scsi_ioctl() because every libata driver uses that function directly 
in its SCSI template as the driver's ioctl hook.

Ack, and I'll apply?

	Jeff




[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 3881 bytes --]

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 71218d7..552ecae 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6638,7 +6638,6 @@ EXPORT_SYMBOL_GPL(ata_dev_pair);
 EXPORT_SYMBOL_GPL(ata_port_disable);
 EXPORT_SYMBOL_GPL(ata_ratelimit);
 EXPORT_SYMBOL_GPL(ata_wait_register);
-EXPORT_SYMBOL_GPL(ata_scsi_ioctl);
 EXPORT_SYMBOL_GPL(ata_scsi_queuecmd);
 EXPORT_SYMBOL_GPL(ata_scsi_slave_config);
 EXPORT_SYMBOL_GPL(ata_scsi_slave_destroy);
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 9e92107..fbc2953 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -423,9 +423,9 @@ int ata_std_bios_param(struct scsi_device *sdev, struct block_device *bdev,
  *	RETURNS:
  *	Zero on success, negative errno on error.
  */
-static int ata_get_identity(struct scsi_device *sdev, void __user *arg)
+static int ata_get_identity(struct ata_port *ap, struct scsi_device *sdev,
+			    void __user *arg)
 {
-	struct ata_port *ap = ata_shost_to_port(sdev->host);
 	struct ata_device *dev = ata_scsi_find_dev(ap, sdev);
 	u16 __user *dst = arg;
 	char buf[40];
@@ -645,7 +645,8 @@ int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg)
 	return rc;
 }
 
-int ata_scsi_ioctl(struct scsi_device *scsidev, int cmd, void __user *arg)
+int __ata_scsi_ioctl(struct ata_port *ap, struct scsi_device *scsidev,
+		     int cmd, void __user *arg)
 {
 	int val = -EINVAL, rc = -EINVAL;
 
@@ -663,7 +664,7 @@ int ata_scsi_ioctl(struct scsi_device *scsidev, int cmd, void __user *arg)
 		return 0;
 
 	case HDIO_GET_IDENTITY:
-		return ata_get_identity(scsidev, arg);
+		return ata_get_identity(ap, scsidev, arg);
 
 	case HDIO_DRIVE_CMD:
 		if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
@@ -682,6 +683,14 @@ int ata_scsi_ioctl(struct scsi_device *scsidev, int cmd, void __user *arg)
 
 	return rc;
 }
+EXPORT_SYMBOL_GPL(__ata_scsi_ioctl);
+
+int ata_scsi_ioctl(struct scsi_device *scsidev, int cmd, void __user *arg)
+{
+	return __ata_scsi_ioctl(ata_shost_to_port(scsidev->host),
+				scsidev, cmd, arg);
+}
+EXPORT_SYMBOL_GPL(ata_scsi_ioctl);
 
 /**
  *	ata_scsi_qc_new - acquire new ata_queued_cmd reference
diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index 841f460..464e566 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -4912,7 +4912,7 @@ static int ipr_ioctl(struct scsi_device *sdev, int cmd, void __user *arg)
 	if (res && ipr_is_gata(res)) {
 		if (cmd == HDIO_GET_IDENTITY)
 			return -ENOTTY;
-		return ata_scsi_ioctl(sdev, cmd, arg);
+		return __ata_scsi_ioctl(res->sata_port->ap, sdev, cmd, arg);
 	}
 
 	return -EINVAL;
diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index 7448387..93aa393 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -717,7 +717,7 @@ int sas_ioctl(struct scsi_device *sdev, int cmd, void __user *arg)
 	struct domain_device *dev = sdev_to_domain_dev(sdev);
 
 	if (dev_is_sata(dev))
-		return ata_scsi_ioctl(sdev, cmd, arg);
+		return __ata_scsi_ioctl(dev->sata_dev.ap, sdev, cmd, arg);
 
 	return -EINVAL;
 }
diff --git a/include/linux/libata.h b/include/linux/libata.h
index b6b8a7f..fb3e5f6 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -926,6 +926,8 @@ extern void ata_host_init(struct ata_host *, struct device *,
 			  unsigned long, struct ata_port_operations *);
 extern int ata_scsi_detect(struct scsi_host_template *sht);
 extern int ata_scsi_ioctl(struct scsi_device *dev, int cmd, void __user *arg);
+extern int __ata_scsi_ioctl(struct ata_port *ap, struct scsi_device *dev,
+			    int cmd, void __user *arg);
 extern int ata_scsi_queuecmd(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *));
 extern void ata_sas_port_destroy(struct ata_port *);
 extern struct ata_port *ata_sas_port_alloc(struct ata_host *,

Re: [PATCH] libsas: User mode app failed to send ioctl commands to SATA drive.

[James Bottomley]

On Fri, 2009-01-16 at 07:20 -0500, Jeff Garzik wrote:
> James Bottomley wrote:
> > On Mon, 2009-01-12 at 18:28 +0800, Ke Wei wrote:
> >> The context of scsi_host's hostdata points to a sas ha structure with libsas
> >> module, but the libata casts this pointer to a ata_port structure when ioctl
> >> sends HDIO_GET_IDENTITY.
> > 
> > This is a good find thanks.  I think there's a small problem in the way
> > the layering is done.
> > 
> > I think ata_scsi_ioctl needs to be expanded to pass the port in all the
> > time.   That way other implementors don't have to remember to special
> > case HDIO_GET_IDENTITY.
> > 
> > When you redo this, ipr will need altering as well (rather than also
> > having the identity exception added).
> 
> The attached appears to be what is needed.  We cannot change 
> ata_scsi_ioctl() because every libata driver uses that function directly 
> in its SCSI template as the driver's ioctl hook.
> 
> Ack, and I'll apply?

That's about precisely what I was thinking ... except I'd call the API
ata_sas_scsi_ioctl() ... since the ata_sas prefix is what we use for all
the SAS inputs to libata (and __ usually implies hidden or unlocked).

I'll ack either way.

James

Re: [PATCH] libsas: User mode app failed to send ioctl commands to SATA drive.

[Jeff Garzik]

James Bottomley wrote:
> On Fri, 2009-01-16 at 07:20 -0500, Jeff Garzik wrote:
>> James Bottomley wrote:
>>> On Mon, 2009-01-12 at 18:28 +0800, Ke Wei wrote:
>>>> The context of scsi_host's hostdata points to a sas ha structure with libsas
>>>> module, but the libata casts this pointer to a ata_port structure when ioctl
>>>> sends HDIO_GET_IDENTITY.
>>> This is a good find thanks.  I think there's a small problem in the way
>>> the layering is done.
>>>
>>> I think ata_scsi_ioctl needs to be expanded to pass the port in all the
>>> time.   That way other implementors don't have to remember to special
>>> case HDIO_GET_IDENTITY.
>>>
>>> When you redo this, ipr will need altering as well (rather than also
>>> having the identity exception added).
>> The attached appears to be what is needed.  We cannot change 
>> ata_scsi_ioctl() because every libata driver uses that function directly 
>> in its SCSI template as the driver's ioctl hook.
>>
>> Ack, and I'll apply?
> 
> That's about precisely what I was thinking ... except I'd call the API
> ata_sas_scsi_ioctl() ... since the ata_sas prefix is what we use for all
> the SAS inputs to libata (and __ usually implies hidden or unlocked).

That's fine with me.  I was on the fence, tossing between __ and 
ata_sas_ in my head.

	Jeff

problems resuming with some SATA drives

[Stuart_Hayes]

When I resume a system that is suspended (it's a Dell studio xps
portable), I'm seeing ugly messages and a slow resume with certain SATA
hard drives --I've pasted the messages below.  This is with an ahci
controller.

I've dug into it, and see what is happening.  During resume, the port is
first issued a hard reset, then a soft reset.  This is done in
ata_eh_reset().  What's happening is that the hard reset is issued in
sata_link_hardreset(), and the code waits for the drive to be
ready--BUT, after ATA_TMOUT_PMP_SRST_WAIT (one second), the code stops
waiting for the drive to be ready, and everything proceeds as if it were
ready (!).  The soft reset is then sent to the drive, but, because the
drive isn't ready, it doesn't see the soft reset.  This times out after
10 seconds (ata_eh_reset_timeouts[0]), and then everything again
proceeds as if the drive was finished with the soft reset.  At that
point an IDENTIFY command is sent, which times out, and it goes into the
error handler and the whole process starts over with another hard reset,
which works this time because the drive has been powered up for a while
at this point.

The main problem is that the code in sata_link_hardreset() is timing out
after 1 second (ATA_TMOUT_PMP_SRST_WAIT), apparently as a workaround
because certain PMPs don't come back and say they are ready if their
first port is empty.  If I get rid of the ATA_TMOUT_PMP_SRST_WAIT one
second timeout, sata_link_hardreset() will wait until the drive is ready
(about 3 seconds--the ATA-6 spec seems to allow up to 5 seconds), then
the soft reset is successful, and the whole system resumes in about 3.5
seconds (instead of 17-18 seconds).

I saw this with an Ubuntu kernel based on 2.6.27, but the relevant code
appears to be unchanged in 2.6.29-rc2.

My best guess as to how to fix this is to change ATA_TMOUT_PMP_SRST_WAIT
to 5 seconds, since that's how long the ATA-6 spec gives drives to be
ready, but I'm not sure how badly that would impact users with the PMPs
that don't say they are ready after a hard reset.

Any suggestions?

Thanks!
Stuart

[181220.664100] ata1: link is slow to respond, please be patient
(ready=0)
[181224.136100] ata1: softreset failed (device not ready)
[181224.136106] ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
[181229.136103] ata1.00: qc timeout (cmd 0xec)
[181229.136108] ata1.00: failed to IDENTIFY (I/O error, err_mask=0x4)
[181229.136110] ata1.00: revalidation failed (errno=-5)
[181229.620102] ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
[181229.629530] ata1.00: configured for UDMA/133
[181229.629539] ata1: exception Emask 0x10 SAct 0x0 SErr 0x0 action 0x9
t4
[181229.629541] ata1: irq_stat 0x00400040, connection status changed
[181229.640113] ata1.00: configured for UDMA/133
[181229.640116] ata1: EH complete
[181229.640159] sd 0:0:0:0: [sda] 390721968 512-byte hardware sectors
(200050 MB)
[181229.640172] sd 0:0:0:0: [sda] Write Protect is off
[181229.640174] sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
[181229.640193] sd 0:0:0:0: [sda] Write cache: enabled, read cache:
enabled, doesn't support DPO or FUA
[181229.640212] sd 0:0:0:0: [sda] 390721968 512-byte hardware sectors
(200050 MB)
[181229.640223] sd 0:0:0:0: [sda] Write Protect is off
[181229.640225] sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
[181229.640243] sd 0:0:0:0: [sda] Write cache: enabled, read cache:
enabled, doesn't support DPO or FUA
[181230.480106] usb 4-3: reset high speed USB device using ehci_hcd and
address 2
[181230.620234] PM: resume devices took 17.376 seconds
[181230.620240] ------------[ cut here ]------------
[181230.620241] WARNING: at
/build/buildd/linux-2.6.27/kernel/power/main.c:176
suspend_test_finish+0x74/0x80()
[181230.620243] Modules linked in: nls_iso8859_1 nls_cp437 vfat fat
mmc_block af_packet binfmt_misc rfcomm bridge stp bnep sco l2cap
bluetooth vboxnetflt vboxdrv ppdev ipv6 acpi_cpufreq
cpufreq_conservative cpufreq_stats cpufreq_powersave cpufreq_userspace
cpufreq_ondemand freq_table pci_slot container sbs sbshc iptable_filter
ip_tables x_tables sbp2 parport_pc lp parport joydev psmouse dcdbas
uvcvideo nvidia(P) serio_raw evdev ieee80211_crypt_tkip compat_ioctl32
pcspkr agpgart wl(P) videodev v4l1_compat ieee80211_crypt i2c_core ac
battery sdhci_pci sdhci mmc_core ricoh_mmc video output snd_hda_intel
snd_pcm_oss snd_mixer_oss snd_pcm snd_seq_dummy snd_seq_oss snd_seq_midi
snd_rawmidi snd_seq_midi_event snd_seq wmi shpchp button snd_timer
snd_seq_device snd pci_hotplug soundcore snd_page_alloc ext3 jbd mbcache
sd_mod crc_t10dif sr_mod cdrom sg ohci_hcd ehci_hcd ahci libata scsi_mod
dock ohci1394 forcedeth ieee1394 usbcore thermal processor fan fbcon
tileblit font bitblit softcursor fuse
[181230.620293] Pid: 4199, comm: pm-suspend Tainted: P        W
2.6.27-11-generic #1
[181230.620296]  [<c037d386>] ? printk+0x1d/0x1f
[181230.620299]  [<c0131e99>] warn_on_slowpath+0x59/0x90
[181230.620303]  [<c014d305>] ? sched_clock_cpu+0xd5/0x170
[181230.620306]  [<c014c15f>] ? down_trylock+0x2f/0x40
[181230.620308]  [<c0132572>] ? try_acquire_console_sem+0x12/0x40
[181230.620311]  [<c024ed00>] ? kobject_put+0x20/0x50
[181230.620314]  [<c015d2b4>] suspend_test_finish+0x74/0x80
[181230.620317]  [<c015d3a6>] suspend_devices_and_enter+0xe6/0x190
[181230.620319]  [<c015d621>] enter_state+0xd1/0x100
[181230.620321]  [<c015d6d5>] state_store+0x85/0xd0
[181230.620323]  [<c015d650>] ? state_store+0x0/0xd0
[181230.620325]  [<c024ebc4>] kobj_attr_store+0x24/0x30
[181230.620328]  [<c01ffac7>] sysfs_write_file+0x97/0x100
[181230.620330]  [<c01b2760>] vfs_write+0xa0/0x110
[181230.620333]  [<c01ffa30>] ? sysfs_write_file+0x0/0x100
[181230.620335]  [<c01b28a2>] sys_write+0x42/0x70
[181230.620337]  [<c0103f7b>] sysenter_do_call+0x12/0x2f
[181230.620340]  [<c0370000>] ? init_memory_mapping+0x230/0x320
[181230.620343]  =======================
[181230.620345] ---[ end trace cefb6a77a684b9aa ]---
[181230.620805] PM: Finishing wakeup.
[181230.620807] Restarting tasks ... done.