Move USB Storage definitions to their own header file
Subsystems (4)
PATCH 1/2 Move USB Storage definitions to their own header file
From: Matthew Wilcox <[email protected]>
The libusual header file is hard to use from code that isn't part of libusual. As the comment suggests, these definitions are moved to their own header file, paralleling other USB classes.
--- include/linux/usb/storage.h | 44 +++++++++++++++++++++++++++++++++++++++++++ include/linux/usb_usual.h | 37 +----------------------------------- 2 files changed, 45 insertions(+), 36 deletions(-) create mode 100644 include/linux/usb/storage.h
include/linux/usb/storage.h new file +44 -0
@@ -0,0 +1,44 @@ +/* +* linux/usb/storage.h +* +* Copyright Matthew Wilcox for Intel Corp, 2010 +* +* This file contains definitions taken from the +* USB Mass Storage Class Specification Overview +* +* Distributed under the terms of the GNU GPL, version two. +*/ + +/* Storage subclass codes */ + +#define US_SC_RBC 0x01/* Typically, flash devices */ +#define US_SC_8020 0x02/* CD-ROM */ +#define US_SC_QIC 0x03/* QIC-157 Tapes */ +#define US_SC_UFI 0x04/* Floppy */ +#define US_SC_8070 0x05/* Removable media */ +#define US_SC_SCSI 0x06/* Transparent */ +#define US_SC_LOCKABLE 0x07/* Password-protected */ + +#define US_SC_ISD200 0xf0/* ISD200 ATA */ +#define US_SC_CYP_ATACB 0xf1/* Cypress ATACB */ +#define US_SC_DEVICE 0xff/* Use device's value */ + +/* Storage protocol codes */ + +#define US_PR_CBI 0x00/* Control/Bulk/Interrupt */ +#define US_PR_CB 0x01/* Control/Bulk w/o interrupt */ +#define US_PR_BULK 0x50/* bulk only */ +#define US_PR_UAS 0x62/* USB Attached SCSI */ + +#define US_PR_USBAT 0x80/* SCM-ATAPI bridge */ +#define US_PR_EUSB_SDDR09 0x81/* SCM-SCSI bridge for SDDR-09 */ +#define US_PR_SDDR55 0x82/* SDDR-55 (made up) */ +#define US_PR_DPCM_USB 0xf0/* Combination CB/SDDR09 */ +#define US_PR_FREECOM 0xf1/* Freecom */ +#define US_PR_DATAFAB 0xf2/* Datafab chipsets */ +#define US_PR_JUMPSHOT 0xf3/* Lexar Jumpshot */ +#define US_PR_ALAUDA 0xf4/* Alauda chipsets */ +#define US_PR_KARMA 0xf5/* Rio Karma */ + +#define US_PR_DEVICE 0xff/* Use device's value */ +
include/linux/usb_usual.h +1 -36
@@ -74,42 +74,7 @@ enum { US_DO_ALL_FLAGS };#define USB_US_TYPE (flags) (((flags) >> 24) & 0xFF)#define USB_US_ORIG_FLAGS (flags) ((flags) & 0x00FFFFFF) -/* -* This is probably not the best place to keep these constants, conceptually. -* But it's the only header included into all places which need them. -*/ - -/* Sub Classes */ - -#define US_SC_RBC 0x01/* Typically, flash devices */ -#define US_SC_8020 0x02/* CD-ROM */ -#define US_SC_QIC 0x03/* QIC-157 Tapes */ -#define US_SC_UFI 0x04/* Floppy */ -#define US_SC_8070 0x05/* Removable media */ -#define US_SC_SCSI 0x06/* Transparent */ -#define US_SC_LOCKABLE 0x07/* Password-protected */ - -#define US_SC_ISD200 0xf0/* ISD200 ATA */ -#define US_SC_CYP_ATACB 0xf1/* Cypress ATACB */ -#define US_SC_DEVICE 0xff/* Use device's value */ - -/* Protocols */ - -#define US_PR_CBI 0x00/* Control/Bulk/Interrupt */ -#define US_PR_CB 0x01/* Control/Bulk w/o interrupt */ -#define US_PR_BULK 0x50/* bulk only */ - -#define US_PR_USBAT 0x80/* SCM-ATAPI bridge */ -#define US_PR_EUSB_SDDR09 0x81/* SCM-SCSI bridge for SDDR-09 */ -#define US_PR_SDDR55 0x82/* SDDR-55 (made up) */ -#define US_PR_DPCM_USB 0xf0/* Combination CB/SDDR09 */ -#define US_PR_FREECOM 0xf1/* Freecom */ -#define US_PR_DATAFAB 0xf2/* Datafab chipsets */ -#define US_PR_JUMPSHOT 0xf3/* Lexar Jumpshot */ -#define US_PR_ALAUDA 0xf4/* Alauda chipsets */ -#define US_PR_KARMA 0xf5/* Rio Karma */ - -#define US_PR_DEVICE 0xff/* Use device's value */ +#include <linux/usb/storage.h> /* */
Signature · 1 line
1.7.1
PATCH 2/2 Add UAS driver
From: Matthew Wilcox <[email protected]>
USB Attached SCSI is a new protocol specified jointly by the SCSI T10 committee and the USB Implementors Forum.
--- MAINTAINERS | 8 + drivers/usb/storage/Kconfig | 13 + drivers/usb/storage/Makefile | 1 + drivers/usb/storage/uas.c | 751 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 773 insertions(+), 0 deletions(-) create mode 100644 drivers/usb/storage/uas.c
MAINTAINERS +8 -0
@@ -5900,6 +5900,14 @@ S: Maintained F: Documentation/usb/acm.txt F: drivers/usb/class/cdc-acm.* +USB ATTACHED SCSI +M: Matthew Wilcox <[email protected]> +M: Sarah Sharp <[email protected]> +L: [email protected] +L: [email protected] +S: Supported +F: drivers/usb/storage/uas.c + USB BLOCK DRIVER (UB ub) M: Pete Zaitcev <[email protected]> L: [email protected]
drivers/usb/storage/Kconfig +13 -0
@@ -172,6 +172,19 @@ config USB_STORAGE_CYPRESS_ATACB If this driver is compiled as a module, it will be named ums-cypress. +config USB_UAS + tristate "USB Attached SCSI" + depends on USB && SCSI + help + The USB Attached SCSI protocol is supported by some USB + storage devices. It permits higher performance by supporting + multiple outstanding commands. + + If you don't know whether you have a UAS device, it is safe to + say 'Y' or 'M' here and the kernel will use the right driver. + + If you compile this driver as a module, it will be named uas. + config USB_LIBUSUAL bool "The shared table of common (or usual) storage devices" depends on USB
drivers/usb/storage/Makefile +1 -0
@@ -7,6 +7,7 @@ EXTRA_CFLAGS := -Idrivers/scsi +obj-$(CONFIG_USB_UAS) += uas.o obj-$(CONFIG_USB_STORAGE) += usb-storage.o usb-storage-obj-$(CONFIG_USB_STORAGE_DEBUG) += debug.o
drivers/usb/storage/uas.c new file +751 -0
751 lines not on this page · raw message
Signature · 1 line
1.7.1
A message that is not in the archive (sent off-list or not yet received): 1285668896-6356-2-git-send-email-willy-VuQAYsv1563Yd54FQh9/[email protected]
Re: [PATCH 2/2] Add UAS driver
Am Dienstag, 28. September 2010, 12:14:56 schrieb Matthew Wilcox:
Quoted 206 lines
From: Matthew Wilcox <[email protected]>
USB Attached SCSI is a new protocol specified jointly by the SCSI T10 committee and the USB Implementors Forum.
--- MAINTAINERS | 8 + drivers/usb/storage/Kconfig | 13 + drivers/usb/storage/Makefile | 1 + drivers/usb/storage/uas.c | 751 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 773 insertions(+), 0 deletions(-) create mode 100644 drivers/usb/storage/uas.cMAINTAINERS +8
-0@@ -5900,6 +5900,14 @@ S: Maintained F: Documentation/usb/acm.txt F: drivers/usb/class/cdc-acm.* +USB ATTACHED SCSI +M: Matthew Wilcox <willy-VuQAYsv1563Yd54FQh9/[email protected]> +M: Sarah Sharp <sarah.a.sharp-VuQAYsv1563Yd54FQh9/[email protected]> +L: [email protected] +L: [email protected] +S: Supported +F: drivers/usb/storage/uas.c + USB BLOCK DRIVER (UB ub) M: Pete Zaitcev <[email protected]> L: [email protected]drivers/usb/storage/Kconfig +13
-0@@ -172,6 +172,19 @@ config USB_STORAGE_CYPRESS_ATACB If this driver is compiled as a module, it will be named ums-cypress. +config USB_UAS + tristate "USB Attached SCSI" + depends on USB && SCSI + help + The USB Attached SCSI protocol is supported by some USB + storage devices. It permits higher performance by supporting + multiple outstanding commands. + + If you don't know whether you have a UAS device, it is safe to + say 'Y' or 'M' here and the kernel will use the right driver. + + If you compile this driver as a module, it will be named uas. + config USB_LIBUSUAL bool "The shared table of common (or usual) storage devices" depends on USBdrivers/usb/storage/Makefile +1
-0@@ -7,6 +7,7 @@ EXTRA_CFLAGS := -Idrivers/scsi +obj-$(CONFIG_USB_UAS) += uas.o obj-$(CONFIG_USB_STORAGE) += usb-storage.o usb-storage-obj-$(CONFIG_USB_STORAGE_DEBUG) += debug.odrivers/usb/storage/uas.c new file +131
-0@@ -0,0 +1,751 @@ +/* +* USB Attached SCSI +* Note that this is not the same as the USB Mass Storage driver +* +* Copyright Matthew Wilcox for Intel Corp, 2010 +* Copyright Sarah Sharp for Intel Corp, 2010 +* +* Distributed under the terms of the GNU GPL, version two. +*/ + +#include <linux/blkdev.h> +#include <linux/slab.h> +#include <linux/types.h> +#include <linux/usb.h> +#include <linux/usb/storage.h> + +#include <scsi/scsi.h> +#include <scsi/scsi_dbg.h> +#include <scsi/scsi_cmnd.h> +#include <scsi/scsi_device.h> +#include <scsi/scsi_host.h> +#include <scsi/scsi_tcq.h> + +/* Common header for all IUs */ +struct iu { +__u8 iu_id; +__u8 rsvd1; +__be16 tag; +}; + +enum { +IU_ID_COMMAND =0x01 , +IU_ID_STATUS =0x03 , +IU_ID_RESPONSE =0x04 , +IU_ID_TASK_MGMT =0x05 , +IU_ID_READ_READY =0x06 , +IU_ID_WRITE_READY =0x07 , +}; + +struct command_iu { +__u8 iu_id; +__u8 rsvd1; +__be16 tag; +__u8 prio_attr; +__u8 rsvd5; +__u8 len; +__u8 rsvd7; +struct scsi_lun lun; +__u8 cdb[16 ];/* XXX: Overflow-checking tools may misunderstand */ +}; + +struct sense_iu { +__u8 iu_id; +__u8 rsvd1; +__be16 tag; +__be16 status_qual; +__u8 status; +__u8 service_response; +__u8 rsvd8[6 ]; +__be16 len; +__u8 sense[SCSI_SENSE_BUFFERSIZE ]; +}; + +/* +* The r00-r01c specs define this version of the SENSE IU data structure. +* It's still in use by several different firmware releases. +*/ +struct sense_iu_old { +__u8 iu_id; +__u8 rsvd1; +__be16 tag; +__be16 len; +__u8 status; +__u8 service_response; +__u8 sense[SCSI_SENSE_BUFFERSIZE ]; +}; + +enum { +CMD_PIPE_ID =1 , +STATUS_PIPE_ID =2 , +DATA_IN_PIPE_ID =3 , +DATA_OUT_PIPE_ID =4 , + +UAS_SIMPLE_TAG =0 , +UAS_HEAD_TAG =1 , +UAS_ORDERED_TAG =2 , +UAS_ACA =4 , +}; + +struct uas_dev_info { +struct usb_interface *intf; +struct usb_device *udev; +int qdepth; +unsigned cmd_pipe, status_pipe, data_in_pipe, data_out_pipe; +unsigned use_streams:1 ; +unsigned uas_sense_old:1 ; +}; + +enum { +ALLOC_SENSE_URB = (1 <<0 ), +SUBMIT_SENSE_URB = (1 <<1 ), +ALLOC_DATA_IN_URB = (1 <<2 ), +SUBMIT_DATA_IN_URB = (1 <<3 ), +ALLOC_DATA_OUT_URB = (1 <<4 ), +SUBMIT_DATA_OUT_URB = (1 <<5 ), +ALLOC_CMD_URB = (1 <<6 ), +SUBMIT_CMD_URB = (1 <<7 ), +}; + +/* Overrides scsi_pointer */ +struct uas_cmd_info { +unsigned int state; +unsigned int stream; +struct urb *cmd_urb; +struct urb *sense_urb; +struct urb *data_in_urb; +struct urb *data_out_urb; +struct list_head list; +}; + +/* I hate forward declarations, but I actually have a loop */ +static int uas_submit_urbs (struct scsi_cmnd *cmnd, +struct uas_dev_info *devinfo,gfp_t gfp); + +static DEFINE_SPINLOCK (uas_work_lock); +static LIST_HEAD (uas_work_list); + +static void uas_do_work (struct work_struct *work) +{ +struct uas_cmd_info *cmdinfo; +struct list_head list;
drivers/usb/storage/uas.c line 141
Quoted 10 lines
drivers/usb/storage/uas.c new file +10
-0+ +spin_lock_irq (&uas_work_lock); +list_replace_init (&uas_work_list, &list); +spin_unlock_irq (&uas_work_lock); + +list_for_each_entry (cmdinfo, &list, list) { +struct scsi_pointer *scp = (void *)cmdinfo; +struct scsi_cmnd *cmnd =container_of (scp, + struct scsi_cmnd, SCp); +uas_submit_urbs (cmnd, cmnd->device->hostdata,GFP_KERNEL );
Deadlocky, use GFP_NOIO
Quoted 18 lines
drivers/usb/storage/uas.c new file +18
-0+ } +} + +static DECLARE_WORK(uas_work, uas_do_work); + +static void uas_sense (struct urb *urb,struct scsi_cmnd *cmnd) +{ +struct sense_iu *sense_iu = urb->transfer_buffer; +struct scsi_device *sdev = cmnd->device; + +if (urb->actual_length >16 ) { +unsigned len =be16_to_cpup (&sense_iu->len); +if (len +16 != urb->actual_length) { +int newlen =min (len +16 , urb->actual_length) -16 ; +if (newlen <0 ) + newlen =0 ; +sdev_printk (KERN_INFO , sdev,"%s: urb length %d " + "disagrees with IU sense data length %d, "
drivers/usb/storage/uas.c line 169
Quoted 10 lines
drivers/usb/storage/uas.c new file +10
-0+"using %d bytes of sense data\n" , __func__, + urb->actual_length, len, newlen); + len = newlen; + } +memcpy (cmnd->sense_buffer, sense_iu->sense, len); + } + + cmnd->result = sense_iu->status; +if (sdev->current_cmnd) + sdev->current_cmnd =NULL ;
the condition is absolutely senseless
Quoted 58 lines
drivers/usb/storage/uas.c new file +58
-0+ cmnd->scsi_done (cmnd); +usb_free_urb (urb); +} + +static void uas_sense_old (struct urb *urb,struct scsi_cmnd *cmnd) +{ +struct sense_iu_old *sense_iu = urb->transfer_buffer; +struct scsi_device *sdev = cmnd->device; + +if (urb->actual_length >8 ) { +unsigned len =be16_to_cpup (&sense_iu->len) -2 ; +if (len +8 != urb->actual_length) { +int newlen =min (len +8 , urb->actual_length) -8 ; +if (newlen <0 ) + newlen =0 ; +sdev_printk (KERN_INFO , sdev,"%s: urb length %d " +"disagrees with IU sense data length %d, " +"using %d bytes of sense data\n" , __func__, + urb->actual_length, len, newlen); + len = newlen; + } +memcpy (cmnd->sense_buffer, sense_iu->sense, len); + } + + cmnd->result = sense_iu->status; +if (sdev->current_cmnd) + sdev->current_cmnd =NULL ; + cmnd->scsi_done (cmnd); +usb_free_urb (urb); +} + +static void uas_xfer_data (struct urb *urb,struct scsi_cmnd *cmnd, +unsigned direction) +{ +struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp; +int err; + + cmdinfo->state = direction |SUBMIT_SENSE_URB ; + err =uas_submit_urbs (cmnd, cmnd->device->hostdata,GFP_ATOMIC ); +if (err) { +spin_lock (&uas_work_lock); +list_add_tail (&cmdinfo->list, &uas_work_list); +spin_unlock (&uas_work_lock); +schedule_work (&uas_work); + } +} + +static void uas_stat_cmplt (struct urb *urb) +{ +struct iu *iu = urb->transfer_buffer; +struct scsi_device *sdev = urb->context; +struct uas_dev_info *devinfo = sdev->hostdata; +struct scsi_cmnd *cmnd; +u16 tag; + +if (urb->status) { +dev_err (&urb->dev->dev,"URB BAD STATUS %d\n" , urb->status); +usb_free_urb (urb);
drivers/usb/storage/uas.c line 237
Quoted 10 lines
drivers/usb/storage/uas.c new file +10
-0+return ; + } + + tag =be16_to_cpup (&iu->tag) -1 ; +if (sdev->current_cmnd) + cmnd = sdev->current_cmnd; +else + cmnd =scsi_find_tag (sdev, tag); +if (!cmnd) +return ;
Where is the URB freed?
Quoted 17 lines
drivers/usb/storage/uas.c new file +17
-0+switch (iu->iu_id) { +case IU_ID_STATUS : +if (urb->actual_length <16 ) + devinfo->uas_sense_old =1 ; +if (devinfo->uas_sense_old) +uas_sense_old (urb, cmnd); +else +uas_sense (urb, cmnd); +break ; +case IU_ID_READ_READY : +uas_xfer_data (urb, cmnd,SUBMIT_DATA_IN_URB ); +break ; +case IU_ID_WRITE_READY : +uas_xfer_data (urb, cmnd,SUBMIT_DATA_OUT_URB ); +break ; +default : + scmd_printk(KERN_ERR, cmnd,
drivers/usb/storage/uas.c line 265
Quoted 10 lines
drivers/usb/storage/uas.c new file +9
-0+ "Bogus IU (%d) received on status pipe\n", iu->iu_id); + } +} + +static int uas_pre_reset (struct usb_interface *intf) +{ +/* XXX: Need to return 1 if it's not our device in error handling */ +return 0 ; +}
You should probably make sure that no further commands are queued from this point on.
Quoted 9 lines
drivers/usb/storage/uas.c new file +9
-0+static int uas_post_reset (struct usb_interface *intf) +{ +/* XXX: Need to return 1 if it's not our device in error handling */ +return 0 ; +} + +static void uas_disconnect (struct usb_interface *intf) +{ +struct usb_device *udev =interface_to_usbdev (intf);
drivers/usb/storage/uas.c line 719
Quoted 10 lines
drivers/usb/storage/uas.c new file +10
-0+struct usb_host_endpoint *eps[3 ]; +struct Scsi_Host *shost =usb_get_intfdata (intf); +struct uas_dev_info *devinfo = (void *)shost->hostdata[0 ]; + +scsi_remove_host (shost); + + eps[0 ] =usb_pipe_endpoint (udev, devinfo->status_pipe); + eps[1 ] =usb_pipe_endpoint (udev, devinfo->data_in_pipe); + eps[2 ] =usb_pipe_endpoint (udev, devinfo->data_out_pipe); +usb_free_streams (intf, eps,3 ,GFP_KERNEL );
This implies that this can fail due to a lack of memory. Then what?
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] Add UAS driver
[Could you trim a little when replying?]
Quoted 17 lines · Oliver Neukum
Am Dienstag, 28. September 2010, 12:14:56 schrieb Matthew Wilcox:
Quoted 4 lines
drivers/usb/storage/uas.c new file +4
-0+static void uas_do_work (struct work_struct *work) +{ +struct uas_cmd_info *cmdinfo; +struct list_head list;drivers/usb/storage/uas.c line 141
Quoted 10 lines
drivers/usb/storage/uas.c new file +10
-0+ +spin_lock_irq (&uas_work_lock); +list_replace_init (&uas_work_list, &list); +spin_unlock_irq (&uas_work_lock); + +list_for_each_entry (cmdinfo, &list, list) { +struct scsi_pointer *scp = (void *)cmdinfo; +struct scsi_cmnd *cmnd =container_of (scp, + struct scsi_cmnd, SCp); +uas_submit_urbs (cmnd, cmnd->device->hostdata,GFP_KERNEL );Deadlocky, use
GFP_NOIO
ACK. I thought I'd fixed this already.
Quoted 17 lines
Quoted 17 lines
drivers/usb/storage/uas.c new file +17
-0+static void uas_sense (struct urb *urb,struct scsi_cmnd *cmnd) +{ +struct sense_iu *sense_iu = urb->transfer_buffer; +struct scsi_device *sdev = cmnd->device; + +if (urb->actual_length >16 ) { +unsigned len =be16_to_cpup (&sense_iu->len); +if (len +16 != urb->actual_length) { +int newlen =min (len +16 , urb->actual_length) -16 ; +if (newlen <0 ) + newlen =0 ; +sdev_printk (KERN_INFO , sdev,"%s: urb length %d " +"disagrees with IU sense data length %d, " +"using %d bytes of sense data\n" , __func__, + urb->actual_length, len, newlen); + len = newlen; + }
Quoted 8 lines
drivers/usb/storage/uas.c line 169
Quoted 6 lines
drivers/usb/storage/uas.c new file +6
-0+memcpy (cmnd->sense_buffer, sense_iu->sense, len); + } + + cmnd->result = sense_iu->status; +if (sdev->current_cmnd) + sdev->current_cmnd =NULL ;the condition is absolutely senseless
That's not true. Avoiding dirtying a cacheline is important for performance.
Quoted 15 lines
Quoted 15 lines
drivers/usb/storage/uas.c new file +15
-0+static void uas_stat_cmplt (struct urb *urb) +{ +struct iu *iu = urb->transfer_buffer; +struct scsi_device *sdev = urb->context; +struct uas_dev_info *devinfo = sdev->hostdata; +struct scsi_cmnd *cmnd; +u16 tag; + +if (urb->status) { +dev_err (&urb->dev->dev,"URB BAD STATUS %d\n" , urb->status); +usb_free_urb (urb); +return ; + } + + tag =be16_to_cpup (&iu->tag) -1 ;
Quoted 8 lines
drivers/usb/storage/uas.c line 237
Quoted 6 lines
drivers/usb/storage/uas.c new file +6
-0+if (sdev->current_cmnd) + cmnd = sdev->current_cmnd; +else + cmnd =scsi_find_tag (sdev, tag); +if (!cmnd) +return ;Where is the URB freed?
Good point. Will fix.
Quoted 8 lines
drivers/usb/storage/uas.c line 699
Quoted 5 lines
drivers/usb/storage/uas.c new file +5
-0+static int uas_pre_reset (struct usb_interface *intf) +{ +/* XXX: Need to return 1 if it's not our device in error handling */ +return 0 ; +}You should probably make sure that no further commands are queued from this point on.
Yeah, there's a whole bunch of problems around error handling.
Quoted 8 lines
Quoted 8 lines
drivers/usb/storage/uas.c new file +8
-0+static void uas_disconnect (struct usb_interface *intf) +{ +struct usb_device *udev =interface_to_usbdev (intf); +struct usb_host_endpoint *eps[3 ]; +struct Scsi_Host *shost =usb_get_intfdata (intf); +struct uas_dev_info *devinfo = (void *)shost->hostdata[0 ]; + +scsi_remove_host (shost);
Quoted 8 lines
drivers/usb/storage/uas.c line 719
Quoted 5 lines
drivers/usb/storage/uas.c new file +5
-0+ + eps[0 ] =usb_pipe_endpoint (udev, devinfo->status_pipe); + eps[1 ] =usb_pipe_endpoint (udev, devinfo->data_in_pipe); + eps[2 ] =usb_pipe_endpoint (udev, devinfo->data_out_pipe); +usb_free_streams (intf, eps,3 ,GFP_KERNEL );This implies that this can fail due to a lack of memory. Then what?
Uhh. I think I'll have to get Sarah to answer that question. We probably end up resetting the endpoints instead of telling the endpoint to stop doing streams.
Thanks for the review.
Re: [PATCH 2/2] Add UAS driver
Quoted 5 lines · Matthew Wilcox
[Could you trim a little when replying?]
Quoted 1 line · Oliver Neukum
Am Dienstag, 28. September 2010, 12:14:56 schrieb Matthew Wilcox:
Quoted 8 lines
Quoted 4 lines
drivers/usb/storage/uas.c line 719
Quoted 1 line
drivers/usb/storage/uas.c new file +1
-0+usb_free_streams (intf, eps,3 ,GFP_KERNEL );This implies that this can fail due to a lack of memory. Then what?
Uhh. I think I'll have to get Sarah to answer that question. We probably end up resetting the endpoints instead of telling the endpoint to stop doing streams.
Sorry Oliver, the usb_free_streams() calls into the xHCI driver, which doesn't actually do anything with the memory flags. Alan Stern already noted it should be removed. Freeing streams doesn't rely on memory allocation (a command is pre-allocated when streams are enabled, and a TRB is reserved on the command ring).
Sarah Sharp
A message that is not in the archive (sent off-list or not yet received): 20100929005413.GB4689-VuQAYsv1563Yd54FQh9/[email protected]
Re: [PATCH 2/2] Add UAS driver
Am Mittwoch, 29. September 2010, 02:54:13 schrieb Matthew Wilcox:
Quoted 2 lines
[Could you trim a little when replying?]
Sorry.
Quoted 20 lines · Oliver Neukum
Quoted 17 lines
Am Dienstag, 28. September 2010, 12:14:56 schrieb Matthew Wilcox:
Quoted 4 lines
drivers/usb/storage/uas.c new file +4
-0+static void uas_do_work (struct work_struct *work) +{ +struct uas_cmd_info *cmdinfo; +struct list_head list;drivers/usb/storage/uas.c line 141
Quoted 10 lines
drivers/usb/storage/uas.c new file +10
-0+ +spin_lock_irq (&uas_work_lock); +list_replace_init (&uas_work_list, &list); +spin_unlock_irq (&uas_work_lock); + +list_for_each_entry (cmdinfo, &list, list) { +struct scsi_pointer *scp = (void *)cmdinfo; +struct scsi_cmnd *cmnd =container_of (scp, + struct scsi_cmnd, SCp); +uas_submit_urbs (cmnd, cmnd->device->hostdata,GFP_KERNEL );Deadlocky, use
GFP_NOIO
Quoted 1 line
ACK. I thought I'd fixed this already.
Upon firther thought I think you need a dedicated work queue. Not only need you use GFP_NOIO, but you need to make sure that no work scheduled before your work on the same queue uses GFP_KERNEL.
Quoted 8 lines
Quoted 5 lines
drivers/usb/storage/uas.c line 169
Quoted 3 lines
drivers/usb/storage/uas.c new file +3
-0+ cmnd->result = sense_iu->status; +if (sdev->current_cmnd) + sdev->current_cmnd =NULL ;the condition is absolutely senseless
That's not true. Avoiding dirtying a cacheline is important for performance.
This is subtle. Do you need to play compiler tricks here?
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] Add UAS driver
In reply to Matthew WilcoxQuoted 11 lines · Matthew Wilcox
Quoted 7 lines · Oliver Neukum
Am Dienstag, 28. September 2010, 12:14:56 schrieb Matthew Wilcox:
drivers/usb/storage/uas.c line 169
Quoted 3 lines
drivers/usb/storage/uas.c new file +3
-0+ cmnd->result = sense_iu->status; +if (sdev->current_cmnd) + sdev->current_cmnd =NULL ;the condition is absolutely senseless
That's not true. Avoiding dirtying a cacheline is important for performance.
Go and tell your processor division to only mark a cacheline as dirty if it was actually changed by a write. They are throwing so much resources on things like SSE9 and the hell that they can probably afford doing that sort of optimizations. Or they already do and this condition is absolutely senseless ;)
Eike
Re: [PATCH 2/2] Add UAS driver
In reply to Matthew WilcoxQuoted 3 lines · Matthew Wilcox
+ * Copyright Matthew Wilcox for Intel Corp, 2010
+ * Copyright Sarah Sharp for Intel Corp, 2010
It's not clear who the copyright holder is. Is it you (as individuals) or Intel Corp?
David
Signature · 6 lines
David Vrabel, Senior Software Engineer, Drivers CSR, Churchill House, Cambridge Business Park, Tel: +44 (0)1223 692562 Cowley Road, Cambridge, CB4 0WZ http://www.csr.com/ Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
Re: [PATCH 2/2] Add UAS driver
Quoted 7 lines · David Vrabel
Quoted 3 lines · Matthew Wilcox
+ * Copyright Matthew Wilcox for Intel Corp, 2010
+ * Copyright Sarah Sharp for Intel Corp, 2010It's not clear who the copyright holder is. Is it you (as individuals) or Intel Corp?
Intel owns the copyright. Let me see if there's preferred wording here.
Re: [PATCH 2/2] Add UAS driver
Quoted 5 lines
Quoted 4 lines
Quoted 2 lines · Matthew Wilcox
+ * Copyright Matthew Wilcox for Intel Corp,
2010
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
+ * Copyright Sarah Sharp for Intel Corp, 2010
It's not clear who the copyright holder is. Is
it you (as individuals)
Quoted 1 line
or Intel Corp?
Intel owns the copyright. Let me see if there's
preferred wording here.
For copyright, I'd suggest the standard
Copyright 2010 by Intel Corp.
For authorship, "Written by ... etc.
Don't confuse legalese (copyright) with credit.
- Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] Add UAS driver
In reply to Matthew WilcoxQuoted 5 lines · Matthew Wilcox
From: Matthew Wilcox <[email protected]>
USB Attached SCSI is a new protocol specified jointly by the SCSI T10 committee and the USB Implementors Forum.
Quoted 8 lines
Signed-off-by Matthew Wilcox <[email protected]>--- MAINTAINERS | 8 + drivers/usb/storage/Kconfig | 13 + drivers/usb/storage/Makefile | 1 + drivers/usb/storage/uas.c | 751 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 773 insertions(+), 0 deletions(-) create mode 100644 drivers/usb/storage/uas.c
Given that this is distinctly not usb-storage, and it is entirely contained within a single file, does it really belong in the drivers/usb/storage directory?
That just seems like a plan for confusion. The fact that there is a big "this is not usb-storage" message in the comments of uas.c would seem to support this position.
Given that it is a single file, I would put it in drivers/usb directly. If you wanted your own directory for possible future refactoring into multiple files or addid other files (like usb-storage did to support oddball devices), then maybe create a drivers/usb/uas directory.
Matt
Signature · 6 lines
Matthew Dharm Home: [email protected] Maintainer, Linux USB Mass Storage Driver It was a new hope. -- Dust Puppy User Friendly, 12/25/1998
Re: [PATCH 2/2] Add UAS driver
Quoted 19 lines · Matthew Dharm
Quoted 5 lines · Matthew Wilcox
From: Matthew Wilcox <[email protected]>
USB Attached SCSI is a new protocol specified jointly by the SCSI T10 committee and the USB Implementors Forum.
Quoted 8 lines
Signed-off-by Matthew Wilcox <[email protected]>--- MAINTAINERS | 8 + drivers/usb/storage/Kconfig | 13 + drivers/usb/storage/Makefile | 1 + drivers/usb/storage/uas.c | 751 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 773 insertions(+), 0 deletions(-) create mode 100644 drivers/usb/storage/uas.cGiven that this is distinctly not usb-storage, and it is entirely contained within a single file, does it really belong in the
drivers/usb/storagedirectory?
Quoted 8 lines
That just seems like a plan for confusion. The fact that there is a big "this is not usb-storage" message in the comments of
uas.cwould seem to support this position.Given that it is a single file, I would put it in
drivers/usbdirectly. If you wanted your own directory for possible future refactoring into multiple files or addid other files (like usb-storage did to support oddball devices), then maybe create adrivers/usb/uasdirectory.
What about drivers/usb/class/ where a number of other USB class drivers live. That would make more sense, right?
thanks,
greg k-h
Re: [PATCH 2/2] Add UAS driver
Quoted 24 lines · Greg KH
Quoted 23 lines · Matthew Dharm
Quoted 5 lines · Matthew Wilcox
From: Matthew Wilcox <[email protected]>
USB Attached SCSI is a new protocol specified jointly by the SCSI T10 committee and the USB Implementors Forum.
Quoted 8 lines
Signed-off-by Matthew Wilcox <[email protected]>--- MAINTAINERS | 8 + drivers/usb/storage/Kconfig | 13 + drivers/usb/storage/Makefile | 1 + drivers/usb/storage/uas.c | 751 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 773 insertions(+), 0 deletions(-) create mode 100644 drivers/usb/storage/uas.cGiven that this is distinctly not usb-storage, and it is entirely contained within a single file, does it really belong in the
drivers/usb/storagedirectory?That just seems like a plan for confusion. The fact that there is a big "this is not usb-storage" message in the comments of
uas.cwould seem to support this position.
Quoted 7 lines
Quoted 4 lines
Given that it is a single file, I would put it in
drivers/usbdirectly. If you wanted your own directory for possible future refactoring into multiple files or addid other files (like usb-storage did to support oddball devices), then maybe create adrivers/usb/uasdirectory.What about
drivers/usb/class/where a number of other USB class drivers live. That would make more sense, right?
No argument from me. Heck, maybe someone wants to move usb-storage into that directory also?
My main point is that it doesn't belong in the same directory as usb-storage. There are lots of good alternatives.
Matt
Signature · 7 lines
Matthew Dharm Home: [email protected] Maintainer, Linux USB Mass Storage Driver It's not that hard. No matter what the problem is, tell the customer to reinstall Windows. -- Nurse User Friendly, 3/22/1998
Re: [PATCH 2/2] Add UAS driver
Quoted 30 lines · Matthew Dharm
Quoted 29 lines · Greg KH
Quoted 27 lines · Matthew Dharm
Quoted 5 lines · Matthew Wilcox
From: Matthew Wilcox <[email protected]>
USB Attached SCSI is a new protocol specified jointly by the SCSI T10 committee and the USB Implementors Forum.
Quoted 8 lines
Signed-off-by Matthew Wilcox <[email protected]>--- MAINTAINERS | 8 + drivers/usb/storage/Kconfig | 13 + drivers/usb/storage/Makefile | 1 + drivers/usb/storage/uas.c | 751 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 773 insertions(+), 0 deletions(-) create mode 100644 drivers/usb/storage/uas.cGiven that this is distinctly not usb-storage, and it is entirely contained within a single file, does it really belong in the
drivers/usb/storagedirectory?That just seems like a plan for confusion. The fact that there is a big "this is not usb-storage" message in the comments of
uas.cwould seem to support this position.Given that it is a single file, I would put it in
drivers/usbdirectly. If you wanted your own directory for possible future refactoring into multiple files or addid other files (like usb-storage did to support oddball devices), then maybe create adrivers/usb/uasdirectory.
Quoted 5 lines
Quoted 2 lines
What about
drivers/usb/class/where a number of other USB class drivers live. That would make more sense, right?No argument from me. Heck, maybe someone wants to move usb-storage into that directory also?
Heh, nah, that's not needed :)
Quoted 2 lines
My main point is that it doesn't belong in the same directory as usb-storage. There are lots of good alternatives.
Actually it will need to tie into libusual as the driver is going to need to unbind the "older" protocol on the device to get usb-storage to not control it anymore and switch to the uas driver. So I think it will need to be in this directory.
thanks,
greg k-h
A message that is not in the archive (sent off-list or not yet received): 20100928175122.GE25677-JGfshJpz5UybPZpvUQj5UqxOck334EZe@public.gmane.org
Re: [PATCH 2/2] Add UAS driver
Quoted 15 lines · Matthew Dharm
Quoted 5 lines · Matthew Wilcox
From: Matthew Wilcox <[email protected]>
USB Attached SCSI is a new protocol specified jointly by the SCSI T10 committee and the USB Implementors Forum.
Quoted 8 lines
--- MAINTAINERS | 8 + drivers/usb/storage/Kconfig | 13 + drivers/usb/storage/Makefile | 1 + drivers/usb/storage/uas.c | 751 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 773 insertions(+), 0 deletions(-) create mode 100644 drivers/usb/storage/uas.c
Quoted 3 lines
Given that this is distinctly not usb-storage, and it is entirely contained within a single file, does it really belong in the
drivers/usb/storagedirectory?
Well. It's USB Storage, specified as part of the Mass Storage Class Specification Overview 1.4. It's not part of the Linux usb-storage driver (yet?), but it's really closely related. For one thing, UAS devices come up in BBB (aka BOT) mode and have to be switched to UAS mode. That might be an argument for integration with libusual, or it might be an argument for integration with usb-storage. I have no idea yet, I'm still experimenting.
Either way, there's got to be close cooperation, and being in the same directory seems like a minimum requirement.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] Add UAS driver
In reply to Matthew WilcoxQuoted 43 lines · Matthew Wilcox
drivers/usb/storage/uas.c new file +43
-0+static int uas_queuecommand (struct scsi_cmnd *cmnd, +void (*done)(struct scsi_cmnd *)) +{ +struct scsi_device *sdev = cmnd->device; +struct uas_dev_info *devinfo = sdev->hostdata; +struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp; +int err; + +BUILD_BUG_ON (sizeof (struct uas_cmd_info ) >sizeof (struct scsi_pointer )); + +if (!cmdinfo->sense_urb && sdev->current_cmnd) +return SCSI_MLQUEUE_DEVICE_BUSY ; + +if (blk_rq_tagged (cmnd->request)) { + cmdinfo->stream = cmnd->request->tag +1 ; + }else { + sdev->current_cmnd = cmnd; + cmdinfo->stream =1 ; + } + + cmnd->scsi_done = done; + + cmdinfo->state =ALLOC_SENSE_URB |SUBMIT_SENSE_URB | +ALLOC_CMD_URB |SUBMIT_CMD_URB ; + +switch (cmnd->sc_data_direction) { +case DMA_FROM_DEVICE : + cmdinfo->state |=ALLOC_DATA_IN_URB |SUBMIT_DATA_IN_URB ; +break ; +case DMA_BIDIRECTIONAL : + cmdinfo->state |=ALLOC_DATA_IN_URB |SUBMIT_DATA_IN_URB ; +case DMA_TO_DEVICE : + cmdinfo->state |=ALLOC_DATA_OUT_URB |SUBMIT_DATA_OUT_URB ; +case DMA_NONE : +break ; + } + +if (!devinfo->use_streams) { + cmdinfo->state &= ~(SUBMIT_DATA_IN_URB |SUBMIT_DATA_OUT_URB ); + cmdinfo->stream =0 ; + } + + err =uas_submit_urbs (cmnd, devinfo,GFP_ATOMIC );
drivers/usb/storage/uas.c line 488
Quoted 10 lines
drivers/usb/storage/uas.c new file +10
-0+if (err) { +/* If we did nothing, give up now */ +if (cmdinfo->state &SUBMIT_SENSE_URB ) { +usb_free_urb (cmdinfo->sense_urb); +return SCSI_MLQUEUE_DEVICE_BUSY ; + } +spin_lock (&uas_work_lock); +list_add_tail (&cmdinfo->list, &uas_work_list); +spin_unlock (&uas_work_lock); +schedule_work (&uas_work);
So, as I read this, you defer to a workqueue if allocation fails? You can't do that in all cases because the system will lock up if we do this on a dirty page clearing path. In order to avoid this lockup, you have to guarantee at least some forward progress. That usually (for SCSI) means that we guarantee at least one command can be issued per device, so we use mempools to guarantee this single free command.
James
Quoted 5 lines
drivers/usb/storage/uas.c new file +5
-0+ } + +return 0 ; +} +