* [PATCH] libsas: User mode app failed to send ioctl commands to SATA drive. @ 2009-01-12 10:28 Ke Wei 2009-01-12 15:33 ` James Bottomley 0 siblings, 1 reply; 6+ messages in thread From: Ke Wei @ 2009-01-12 10:28 UTC (permalink / raw) To: linux-scsi, James Bottomley, Jeff Garzik, djwong, Jason Chu, Michael 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); ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] libsas: User mode app failed to send ioctl commands to SATA drive. 2009-01-12 10:28 [PATCH] libsas: User mode app failed to send ioctl commands to SATA drive Ke Wei @ 2009-01-12 15:33 ` James Bottomley 2009-01-16 12:20 ` Jeff Garzik 0 siblings, 1 reply; 6+ messages in thread From: James Bottomley @ 2009-01-12 15:33 UTC (permalink / raw) To: kewei; +Cc: linux-scsi, Jeff Garzik, djwong, Jason Chu, Michael Wang, Jacky Feng 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libsas: User mode app failed to send ioctl commands to SATA drive. 2009-01-12 15:33 ` James Bottomley @ 2009-01-16 12:20 ` Jeff Garzik 2009-01-16 14:21 ` James Bottomley 2009-01-20 20:18 ` problems resuming with some SATA drives Stuart_Hayes 0 siblings, 2 replies; 6+ messages in thread From: Jeff Garzik @ 2009-01-16 12:20 UTC (permalink / raw) To: James Bottomley Cc: kewei, linux-scsi, djwong, Jason Chu, Michael Wang, Jacky Feng [-- 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 *, ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] libsas: User mode app failed to send ioctl commands to SATA drive. 2009-01-16 12:20 ` Jeff Garzik @ 2009-01-16 14:21 ` James Bottomley 2009-01-16 14:33 ` Jeff Garzik 2009-01-20 20:18 ` problems resuming with some SATA drives Stuart_Hayes 1 sibling, 1 reply; 6+ messages in thread From: James Bottomley @ 2009-01-16 14:21 UTC (permalink / raw) To: Jeff Garzik Cc: kewei, linux-scsi, djwong, Jason Chu, Michael Wang, Jacky Feng 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libsas: User mode app failed to send ioctl commands to SATA drive. 2009-01-16 14:21 ` James Bottomley @ 2009-01-16 14:33 ` Jeff Garzik 0 siblings, 0 replies; 6+ messages in thread From: Jeff Garzik @ 2009-01-16 14:33 UTC (permalink / raw) To: James Bottomley Cc: kewei, linux-scsi, djwong, Jason Chu, Michael Wang, Jacky Feng 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* problems resuming with some SATA drives 2009-01-16 12:20 ` Jeff Garzik 2009-01-16 14:21 ` James Bottomley @ 2009-01-20 20:18 ` Stuart_Hayes 1 sibling, 0 replies; 6+ messages in thread From: Stuart_Hayes @ 2009-01-20 20:18 UTC (permalink / raw) To: linux-scsi 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-01-20 20:19 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-01-12 10:28 [PATCH] libsas: User mode app failed to send ioctl commands to SATA drive Ke Wei 2009-01-12 15:33 ` James Bottomley 2009-01-16 12:20 ` Jeff Garzik 2009-01-16 14:21 ` James Bottomley 2009-01-16 14:33 ` Jeff Garzik 2009-01-20 20:18 ` problems resuming with some SATA drives Stuart_Hayes
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox