This driver allows you to connect to UAS devices
and use them as SCSI devices.
Signed-off-by: Luben Tuikov <[email protected]>
---
* The name "uas" was taken, so I used "uasp".
* The idea is to have this in Greg's tree, everyone submits
patches to this CC list, it gets ACKed and goes right into
Greg's tree.
* Before submitting patches, please test _both_ High-Speed and
SuperSpeed mode.
* Vendors whose solution is still in development wanting to
utilize this driver, can use dynamic ids to test their
implementation, although there is no reason to not have the
correct descriptors.
* Vendors: I don't know /when/ this will make it into the kernel.
CC this thread when asking.
drivers/usb/storage/Kconfig | 19 +
drivers/usb/storage/Makefile | 5 +
drivers/usb/storage/uasp.c | 1117 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 1141 insertions(+), 0 deletions(-)
create mode 100644 drivers/usb/storage/uasp.c
diff --git a/drivers/usb/storage/Kconfig b/drivers/usb/storage/Kconfig
index eb9f6af..1f58cd0 100644
--- a/drivers/usb/storage/Kconfig
+++ b/drivers/usb/storage/Kconfig
@@ -195,6 +195,25 @@ config USB_UAS_DEBUG
If unsure, say 'N'.
+config USB_UASP
+ tristate "USB Attached SCSI (UAS) protocol driver (uasp)"
+ depends on USB && SCSI
+ help
+ USB Attached SCSI (UAS) protocol driver. This driver allows you
+ to connect to UAS devices and use them as SCSI devices.
+
+ If unusure say 'Y' or 'M'.
+
+ If compiled as a module, this driver will be named uasp.
+
+config USB_UASP_DEBUG
+ bool "Compile the uasp driver in debug mode"
+ depends on USB_UASP
+ help
+ Compiles the uasp driver in debug mode.
+
+ If unsure say 'N'.
+
config USB_LIBUSUAL
bool "The shared table of common (or usual) storage devices"
depends on USB
diff --git a/drivers/usb/storage/Makefile b/drivers/usb/storage/Makefile
index 16715ab..d148bdc 100644
--- a/drivers/usb/storage/Makefile
+++ b/drivers/usb/storage/Makefile
@@ -8,12 +8,17 @@
ccflags-y := -Idrivers/scsi
obj-$(CONFIG_USB_UAS) += uas.o
+obj-$(CONFIG_USB_UASP) += uasp.o
obj-$(CONFIG_USB_STORAGE) += usb-storage.o
ifeq ($(CONFIG_USB_UAS_DEBUG),y)
EXTRA_CFLAGS += -DUAS_DEBUG
endif
+ifeq ($(CONFIG_USB_UASP_DEBUG),y)
+ EXTRA_CFLAGS += -DUASP_DEBUG
+endif
+
usb-storage-y := scsiglue.o protocol.o transport.o usb.o
usb-storage-y += initializers.o sierra_ms.o option_ms.o
diff --git a/drivers/usb/storage/uasp.c b/drivers/usb/storage/uasp.c
new file mode 100644
index 0000000..1d2b2e3
--- /dev/null
+++ b/drivers/usb/storage/uasp.c
@@ -0,0 +1,1117 @@
+/*
+ * USB Attached SCSI (UAS) protocol driver
+ * Copyright Luben Tuikov, 2010
+ *
+ * This driver allows you to connect to UAS devices and use them as
+ * SCSI devices.
+ *
+ * Distributed under the terms of the GNU GPL version 2.
+ */
+
+#ifdef UASP_DEBUG
+#if !defined(DEBUG) && !defined(CONFIG_DYNAMIC_DEBUG)
+#define DEBUG
+#endif
+#endif /* UASP_DEBUG */
+
+#define DRIVER_NAME "uasp"
+
+#include <linux/blkdev.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/usb.h>
+#include <linux/usb/storage.h>
+#include <linux/completion.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>
+
+/* Information unit types
+ */
+#define IU_CMD 1
+#define IU_SENSE 3
+#define IU_RESP 4
+#define IU_TMF 5
+#define IU_RRDY 6
+#define IU_WRDY 7
+
+#define IU_CMD_LEN 32
+#define IU_SENSE_LEN 16
+#define IU_RESP_LEN 8
+#define IU_TMF_LEN 16
+#define IU_RRDY_LEN 4
+#define IU_WRDY_LEN 4
+
+#define MAX_SENSE_DATA_LEN 252
+
+#define STAT_IU_LEN (IU_SENSE_LEN + MAX_SENSE_DATA_LEN)
+
+#define GET_IU_ID(__Ptr) ((__Ptr)[0])
+#define GET_IU_TAG(__Ptr) ((__Ptr)[2] << 8 | (__Ptr)[3])
+
+/* SENSE IU
+ */
+#define GET_IU_STATQUAL(__Ptr) ((__Ptr)[4] << 8 | (__Ptr)[5])
+#define GET_IU_STATUS(__Ptr) ((__Ptr)[6])
+#define GET_IU_LENGTH(__Ptr) ((__Ptr)[14] << 8 | (__Ptr)[15])
+
+/* RESPONSE IU
+ */
+#define GET_IU_RESPONSE(__Ptr) ((__Ptr)[4] << 24 | (__Ptr)[5] << 16 | \
+ (__Ptr)[6] << 8 | (__Ptr)[7])
+
+/* Task management
+ */
+#define TMF_ABORT_TASK 1
+#define TMF_ABORT_TASK_SET 2
+#define TMF_CLEAR_TASK_SET 4
+#define TMF_LU_RESET 8
+#define TMF_IT_NEXUS_RESET 0x10
+#define TMF_CLEAR_ACA 0x40
+#define TMF_QUERY_TASK 0x80
+#define TMF_QUERY_TASK_SET 0x81
+#define TMF_QUERY_ASYNC_EVENT 0x82
+
+#define TMR_RESPONSE_CODE_MASK 0xFF
+#define TMR_RESPONSE_CODE_SHIFT 0
+#define TMR_RESPONSE_INFO_MASK 0xFFFFFF00
+#define TMR_RESPONSE_INFO_SHIFT 8
+
+#define TMR_RESPONSE_CODE(__Val) \
+ (((__Val) & TMR_RESPONSE_CODE_MASK) >> TMR_RESPONSE_CODE_SHIFT)
+
+#define TMR_RESPONSE_INFO(__Val) \
+ (((__Val) & TMR_RESPONSE_INFO_MASK) >> TMR_RESPONSE_INFO_SHIFT)
+
+#define TMR_COMPLETE 0
+#define TMR_IIU 2
+#define TMR_UNSUPP 4
+#define TMR_FAILED 5
+#define TMR_SUCC 8
+#define TMR_ILUN 9
+#define TMR_OLAP 0xA
+
+/* Pipe types
+ */
+#define CMD_PIPE_ID 1
+#define STAT_PIPE_ID 2
+#define DATAIN_PIPE_ID 3
+#define DATAOUT_PIPE_ID 4
+
+/* Task attribute
+ */
+#define UASP_TASK_SIMPLE 0
+#define UASP_TASK_HOQ 1
+#define UASP_TASK_ORDERED 2
+#define UASP_TASK_ACA 4
+
+/* Target device port information
+ */
+struct uasp_tport_info {
+ struct usb_interface *iface;
+ struct usb_device *udev;
+ unsigned cmd_pipe, status_pipe, datain_pipe, dataout_pipe;
+ struct usb_host_endpoint *eps[4];
+ unsigned use_streams:1;
+ int max_streams; /* streams supported */
+ int num_streams; /* usable streams [1, num_streams] */
+ int max_cmds; /* max cmds we can queue */
+};
+
+/* Current task management information
+ */
+struct uasp_lu_info {
+ struct completion tmf_completion;
+ unsigned int tmf_resp;
+};
+
+/* Command information
+ */
+struct uasp_cmd_info {
+ int tag;
+ struct urb *cmd_urb;
+ struct urb *status_urb;
+ struct urb *datain_urb;
+ struct urb *dataout_urb;
+};
+
+#define UASP_CMD_INFO(__Scmd) ((struct uasp_cmd_info *)(&(__Scmd)->SCp))
+#define UASP_TPORT_INFO(__Scmd) \
+ ((struct uasp_tport_info *)((__Scmd)->device->host->hostdata[0]))
+#define SDEV_TPORT_INFO(__Sdev) \
+ ((struct uasp_tport_info *)((__Sdev)->host->hostdata[0]))
+#define SDEV_LU_INFO(__Sdev) ((struct uasp_lu_info *)((__Sdev)->hostdata))
+
+/* ---------- IU processors ---------- */
+
+static void uasp_sense(struct urb *urb, struct scsi_cmnd *cmd, u16 tag)
+{
+ unsigned char *iu = urb->transfer_buffer;
+ struct scsi_device *sdev = cmd->device;
+
+ cmd->result = GET_IU_STATUS(iu);
+
+ if (urb->actual_length > IU_SENSE_LEN) {
+ unsigned slen = GET_IU_LENGTH(iu);
+
+ if (urb->actual_length >= IU_SENSE_LEN + slen) {
+ slen = min(slen, (unsigned) SCSI_SENSE_BUFFERSIZE);
+ } else {
+ unsigned dlen = slen;
+
+ slen = min(urb->actual_length - IU_SENSE_LEN,
+ (unsigned) SCSI_SENSE_BUFFERSIZE);
+
+ dev_err(&SDEV_TPORT_INFO(sdev)->udev->dev,
+ "%s: short SENSE IU by %d bytes, "
+ "using %d/%d bytes of sense data\n",
+ __func__,
+ IU_SENSE_LEN + slen - urb->actual_length,
+ slen, dlen);
+ }
+ memcpy(cmd->sense_buffer, iu + IU_SENSE_LEN, slen);
+ }
+
+ if (tag == 1)
+ sdev->current_cmnd = NULL;
+ cmd->scsi_done(cmd);
+ usb_free_urb(urb);
+}
+
+/* High-Speed devices only
+ */
+static void uasp_xfer_data(struct urb *urb, struct scsi_cmnd *cmd,
+ struct uasp_tport_info *tpinfo,
+ enum dma_data_direction dir, int tag)
+{
+ struct uasp_cmd_info *cmdinfo = UASP_CMD_INFO(cmd);
+ int res = 0;
+
+ res = usb_submit_urb(urb, GFP_ATOMIC);
+
+ if (res == 0) {
+ if (dir == DMA_FROM_DEVICE)
+ res = usb_submit_urb(cmdinfo->datain_urb, GFP_ATOMIC);
+ else
+ res = usb_submit_urb(cmdinfo->dataout_urb,GFP_ATOMIC);
+ }
+
+ dev_dbg(&tpinfo->udev->dev, "%s: cmd:%p tag:%d res:%d\n",
+ __func__, cmd, cmdinfo->tag, res);
+}
+
+#ifdef UASP_DEBUG
+
+static const char *id_to_str[] = {
+ [0 ... 0xFF] = "Unknown",
+ [1] = "Command",
+ [3] = "Sense",
+ [4] = "Response",
+ [5] = "Task Management",
+ [6] = "RRDY",
+ [7] = "WRDY",
+};
+
+#endif /* UASP_DEBUG */
+
+/**
+ * uasp_stat_cmplt -- Status pipe urb completion
+ * @urb: the URB that completed
+ *
+ * Anything we expect to come back on the status pipe
+ * comes here.
+ */
+static void uasp_stat_done(struct urb *urb)
+{
+ unsigned char *iu = urb->transfer_buffer;
+ struct scsi_device *sdev = urb->context;
+ struct uasp_tport_info *tpinfo = SDEV_TPORT_INFO(sdev);
+ struct scsi_cmnd *cmd = NULL;
+ int id = GET_IU_ID(iu);
+ int tag = GET_IU_TAG(iu);
+
+ dev_dbg(&urb->dev->dev, "%s: %s IU (0x%02x) tag:%d urb:%p\n",
+ __func__, id_to_str[id], id, tag, urb);
+
+ if (urb->status) {
+ dev_err(&urb->dev->dev, "%s: URB BAD STATUS %d\n",
+ __func__, urb->status);
+ usb_free_urb(urb);
+ return;
+ }
+
+ if (id == IU_RESP) {
+ struct uasp_lu_info *luinfo = SDEV_LU_INFO(sdev);
+ unsigned char *riu = urb->transfer_buffer;
+
+ luinfo->tmf_resp = GET_IU_RESPONSE(riu);
+ complete(&luinfo->tmf_completion);
+ usb_free_urb(urb);
+ return;
+ }
+
+ if (tag == 1)
+ cmd = sdev->current_cmnd;
+ else if (tag > 1)
+ cmd = scsi_find_tag(sdev, tag-2);
+
+ if (cmd == NULL)
+ goto Out_no_cmd;
+
+ switch (id) {
+ case IU_SENSE:
+ uasp_sense(urb, cmd, tag);
+ break;
+ case IU_RRDY:
+ uasp_xfer_data(urb, cmd, tpinfo, DMA_FROM_DEVICE, tag);
+ break;
+ case IU_WRDY:
+ uasp_xfer_data(urb, cmd, tpinfo, DMA_TO_DEVICE, tag);
+ break;
+ default:
+ usb_free_urb(urb);
+ break;
+ }
+
+ return;
+
+ Out_no_cmd:
+ dev_dbg(&urb->dev->dev, "%s: No command!?\n", __func__);
+ usb_free_urb(urb);
+}
+
+static void uasp_data_done(struct urb *urb)
+{
+ struct scsi_data_buffer *sdb = urb->context;
+
+ dev_dbg(&urb->dev->dev, "%s: urb:%p\n", __func__, urb);
+
+ if (urb->status == 0)
+ sdb->resid = sdb->length - urb->actual_length;
+ else
+ dev_err(&urb->dev->dev, "%s: URB BAD STATUS %d\n",
+ __func__, urb->status);
+
+ usb_free_urb(urb);
+}
+
+/* ---------- URB allocators and submission ---------- */
+
+/**
+ * uasp_fill_cmdp_urb -- Fill in a command pipe urb
+ * @urb: the urb
+ * @tpinfo: the UAS device info struct
+ * @transfer_buffer: the IU
+ * @buffer_length: the size of the IU
+ *
+ * A unified place to initialize a command pipe urb so that
+ * all command pipe urbs are freed in the same manner.
+ */
+static void uasp_fill_cmdp_urb(struct urb *urb, struct uasp_tport_info *tpinfo,
+ void *transfer_buffer, int buffer_length)
+{
+ usb_fill_bulk_urb(urb, tpinfo->udev, tpinfo->cmd_pipe,
+ transfer_buffer, buffer_length,
+ usb_free_urb, NULL);
+ urb->transfer_flags |= URB_FREE_BUFFER;
+}
+
+/**
+ * uasp_fill_statp_urb -- Fill in a status pipe urb
+ * @urb: the urb
+ * @dev: the scsi device
+ * @transfer_buffer: the transfer buffer of size STAT_IU_LEN
+ * @tag: the tag
+ *
+ * The callback is responsible to free/recycle the urb and the
+ * transfer buffer.
+ */
+static void uasp_fill_statp_urb(struct urb *urb, struct scsi_device *sdev,
+ void *transfer_buffer, int tag)
+{
+ struct uasp_tport_info *tpinfo = SDEV_TPORT_INFO(sdev);
+
+ usb_fill_bulk_urb(urb, tpinfo->udev, tpinfo->status_pipe,
+ transfer_buffer, STAT_IU_LEN,
+ uasp_stat_done, sdev);
+ urb->transfer_flags |= URB_FREE_BUFFER;
+ if (tpinfo->use_streams)
+ urb->stream_id = tag;
+}
+
+static struct urb *uasp_alloc_data_urb(struct uasp_tport_info *tpinfo,
+ gfp_t gfp, unsigned int data_pipe,
+ u16 tag, struct scsi_data_buffer *sdb)
+{
+ struct usb_device *udev = tpinfo->udev;
+ struct urb *urb;
+
+ urb = usb_alloc_urb(0, gfp);
+ if (urb == NULL)
+ goto Out;
+
+ usb_fill_bulk_urb(urb, udev, data_pipe, NULL, sdb->length,
+ uasp_data_done, sdb);
+ if (tpinfo->use_streams)
+ urb->stream_id = tag;
+ urb->num_sgs = udev->bus->sg_tablesize ? sdb->table.nents : 0;
+ urb->sg = sdb->table.sgl;
+ Out:
+ return urb;
+}
+
+static struct urb *uasp_alloc_status_urb(struct scsi_cmnd *cmd, gfp_t gfp)
+{
+ unsigned char *siu;
+ struct urb *urb;
+
+ urb = usb_alloc_urb(0, gfp);
+ if (urb == NULL)
+ return NULL;
+
+ siu = kzalloc(STAT_IU_LEN, gfp);
+ if (siu == NULL)
+ goto Out_free;
+
+ uasp_fill_statp_urb(urb, cmd->device, siu, UASP_CMD_INFO(cmd)->tag);
+
+ return urb;
+ Out_free:
+ usb_free_urb(urb);
+ return NULL;
+}
+
+static struct urb *uasp_alloc_cmd_urb(struct scsi_cmnd *cmd, gfp_t gfp)
+{
+ struct uasp_cmd_info *cmdinfo = UASP_CMD_INFO(cmd);
+ struct uasp_tport_info *tpinfo = UASP_TPORT_INFO(cmd);
+ struct usb_device *udev = tpinfo->udev;
+ struct scsi_device *sdev = cmd->device;
+ struct urb *urb;
+ struct scsi_lun slun;
+ unsigned char *ciu;
+ int len;
+
+ urb = usb_alloc_urb(0, gfp);
+ if (urb == NULL)
+ return NULL;
+
+ len = cmd->cmd_len + 16;
+ if (len < IU_CMD_LEN)
+ len = IU_CMD_LEN;
+ else if (len > IU_CMD_LEN)
+ len = ALIGN(len, 4);
+
+ ciu = kzalloc(len, gfp);
+ if (ciu == NULL)
+ goto Free;
+
+ ciu[0] = IU_CMD;
+ ciu[2] = cmdinfo->tag >> 8;
+ ciu[3] = cmdinfo->tag;
+ if (sdev->ordered_tags && cmd->request->cmd_flags & REQ_HARDBARRIER)
+ ciu[4] = UASP_TASK_ORDERED;
+ ciu[6] = len - IU_CMD_LEN;
+ int_to_scsilun(sdev->lun, &slun);
+ memcpy(&ciu[8], slun.scsi_lun, 8);
+ memcpy(&ciu[16], cmd->cmnd, cmd->cmd_len);
+
+ usb_fill_bulk_urb(urb, udev, tpinfo->cmd_pipe, ciu, len, usb_free_urb,
+ NULL);
+ urb->transfer_flags |= URB_FREE_BUFFER;
+ return urb;
+ Free:
+ usb_free_urb(urb);
+ return NULL;
+}
+
+static int uasp_alloc_urbs(struct scsi_cmnd *cmd, gfp_t gfp)
+{
+ struct uasp_cmd_info *cmdinfo = UASP_CMD_INFO(cmd);
+ struct uasp_tport_info *tpinfo = UASP_TPORT_INFO(cmd);
+
+ cmdinfo->status_urb = NULL;
+ cmdinfo->datain_urb = NULL;
+ cmdinfo->dataout_urb = NULL;
+ cmdinfo->cmd_urb = NULL;
+
+ cmdinfo->status_urb = uasp_alloc_status_urb(cmd, gfp);
+ cmdinfo->cmd_urb = uasp_alloc_cmd_urb(cmd, gfp);
+ if (cmdinfo->cmd_urb == NULL || cmdinfo->status_urb == NULL)
+ goto Out_err1;
+
+ switch (cmd->sc_data_direction) {
+ case DMA_BIDIRECTIONAL:
+ case DMA_TO_DEVICE:
+ cmdinfo->dataout_urb =
+ uasp_alloc_data_urb(tpinfo, gfp,
+ tpinfo->dataout_pipe,
+ cmdinfo->tag,
+ scsi_out(cmd));
+ if (cmdinfo->dataout_urb == NULL)
+ goto Out_err2;
+ if (cmd->sc_data_direction != DMA_BIDIRECTIONAL)
+ break;
+ else {
+ case DMA_FROM_DEVICE:
+ cmdinfo->datain_urb =
+ uasp_alloc_data_urb(tpinfo, gfp,
+ tpinfo->datain_pipe,
+ cmdinfo->tag,
+ scsi_in(cmd));
+ if (cmdinfo->datain_urb == NULL)
+ goto Out_err2;
+ }
+ break;
+ case DMA_NONE:
+ break;
+ }
+
+ return 0;
+
+ Out_err2:
+ usb_free_urb(cmdinfo->datain_urb);
+ usb_free_urb(cmdinfo->dataout_urb);
+ Out_err1:
+ usb_free_urb(cmdinfo->cmd_urb);
+ usb_free_urb(cmdinfo->status_urb);
+ return -ENOMEM;
+}
+
+static int uasp_submit_urbs(struct scsi_cmnd *cmd, gfp_t gfp)
+{
+ struct uasp_cmd_info *cmdinfo = UASP_CMD_INFO(cmd);
+ struct uasp_tport_info *tpinfo = UASP_TPORT_INFO(cmd);
+ int res;
+
+ dev_dbg(&tpinfo->udev->dev, "%s: cmd:%p (0x%02x) tag:%d\n",
+ __func__, cmd, cmd->cmnd[0], cmdinfo->tag);
+
+ res = usb_submit_urb(cmdinfo->status_urb, gfp);
+ if (res) {
+ dev_err(&tpinfo->udev->dev,
+ "error submitting status urb (%d)\n", res);
+ return res;
+ }
+
+ if (cmdinfo->datain_urb && tpinfo->use_streams) {
+ res = usb_submit_urb(cmdinfo->datain_urb, gfp);
+ if (res) {
+ dev_err(&tpinfo->udev->dev,
+ "error submitting datain urb (%d)\n", res);
+ return res;
+ }
+ }
+
+ if (cmdinfo->dataout_urb && tpinfo->use_streams) {
+ res = usb_submit_urb(cmdinfo->dataout_urb, gfp);
+ if (res) {
+ dev_err(&tpinfo->udev->dev,
+ "error submitting dataout urb (%d)\n", res);
+ return res;
+ }
+ }
+
+ res = usb_submit_urb(cmdinfo->cmd_urb, gfp);
+ if (res) {
+ dev_err(&tpinfo->udev->dev,
+ "error submitting cmd urb (%d)\n", res);
+ return res;
+ }
+
+ return 0;
+}
+
+static void uasp_free_urbs(struct scsi_cmnd *cmd)
+{
+ struct uasp_cmd_info *cmdinfo = UASP_CMD_INFO(cmd);
+ int res;
+
+ if (cmdinfo->cmd_urb) {
+ res = usb_unlink_urb(cmdinfo->cmd_urb);
+ if (res != -EINPROGRESS)
+ usb_free_urb(cmdinfo->cmd_urb);
+ }
+ if (cmdinfo->status_urb) {
+ res = usb_unlink_urb(cmdinfo->status_urb);
+ if (res != -EINPROGRESS)
+ usb_free_urb(cmdinfo->status_urb);
+ }
+ if (cmdinfo->datain_urb) {
+ res = usb_unlink_urb(cmdinfo->datain_urb);
+ if (res != -EINPROGRESS)
+ usb_free_urb(cmdinfo->datain_urb);
+ }
+ if (cmdinfo->dataout_urb) {
+ res = usb_unlink_urb(cmdinfo->dataout_urb);
+ if (res != -EINPROGRESS)
+ usb_free_urb(cmdinfo->dataout_urb);
+ }
+}
+
+static int uasp_queuecommand(struct scsi_cmnd *cmd,
+ void (*done)(struct scsi_cmnd *))
+{
+ struct scsi_device *sdev = cmd->device;
+ struct uasp_cmd_info *cmdinfo = UASP_CMD_INFO(cmd);
+ int res;
+
+ BUILD_BUG_ON(sizeof(struct uasp_cmd_info) > sizeof(struct scsi_pointer));
+
+ /* If LLDDs are NOT to maintain their own tags, (but the block
+ * layer would), THEN ANY AND ALL scsi_cmnds passed to the
+ * queuecommand entry of a LLDD MUST HAVE A VALID,
+ * REVERSE-MAPPABLE tag, REGARDLESS of where the command came
+ * from, regardless of whether the device supports tags, and
+ * regardless of how many tags it supports.
+ */
+ if (blk_rq_tagged(cmd->request)) {
+ cmdinfo->tag = cmd->request->tag + 2;
+ } else if (sdev->current_cmnd == NULL) {
+ sdev->current_cmnd = cmd;
+ cmdinfo->tag = 1;
+ } else {
+ cmd->result = DID_ABORT << 16;
+ goto Out_err;
+ }
+
+ cmd->scsi_done = done;
+
+ res = uasp_alloc_urbs(cmd, GFP_ATOMIC);
+ if (res) {
+ cmd->result = DID_ABORT << 16;
+ goto Out_err_null;
+ }
+
+ res = uasp_submit_urbs(cmd, GFP_ATOMIC);
+ if (res) {
+ cmd->result = DID_NO_CONNECT << 16;
+ uasp_free_urbs(cmd);
+ goto Out_err_null;
+ }
+
+ dev_dbg(&UASP_TPORT_INFO(cmd)->udev->dev,
+ "%s: cmd:%p (0x%02x) tag:%d lun:%d res:%d\n",
+ __func__, cmd, cmd->cmnd[0], cmdinfo->tag,
+ cmd->device->lun, res);
+
+ return 0;
+ Out_err_null:
+ if (sdev->current_cmnd == cmd)
+ sdev->current_cmnd = NULL;
+ Out_err:
+ done(cmd);
+ return 0;
+}
+
+/* ---------- Error Recovery ---------- */
+
+static int uasp_alloc_tmf_urb(struct urb **urb, struct uasp_tport_info *tpinfo,
+ u8 tmf, u16 ttbm, unsigned char *lun)
+{
+ unsigned char *tiu;
+ int tag;
+
+ *urb = usb_alloc_urb(0, GFP_KERNEL);
+ if (*urb == NULL)
+ return -ENOMEM;
+
+ tiu = kzalloc(IU_TMF_LEN, GFP_KERNEL);
+ if (tiu == NULL)
+ goto Out_err;
+
+ /* If LLDDs are NOT to maintain their own tags, (but the block
+ * layer would), THEN LLDDs must be able to call a function of
+ * some sort and reserve a tag from the same pool and obtain
+ * it for their own use, as well as being able to free it back
+ * later. See the comment in uasp_set_max_cmds().
+ */
+ tag = tpinfo->max_cmds + 2;
+
+ tiu[0] = IU_TMF;
+ tiu[2] = tag >> 8;
+ tiu[3] = tag;
+ tiu[4] = tmf;
+ tiu[6] = ttbm >> 8;
+ tiu[7] = ttbm;
+ memcpy(&tiu[8], lun, 8);
+
+ uasp_fill_cmdp_urb(*urb, tpinfo, tiu, IU_TMF_LEN);
+
+ return 0;
+ Out_err:
+ usb_free_urb(*urb);
+ return -ENOMEM;
+}
+
+static int uasp_alloc_resp_urb(struct urb **urb, struct scsi_device *sdev,
+ int tag)
+{
+ unsigned char *resp;
+
+ *urb = usb_alloc_urb(0, GFP_KERNEL);
+ if (*urb == NULL)
+ return -ENOMEM;
+
+ resp = kzalloc(STAT_IU_LEN, GFP_KERNEL);
+ if (resp == NULL)
+ goto Out_free;
+
+ uasp_fill_statp_urb(*urb, sdev, resp, tag);
+
+ return 0;
+ Out_free:
+ usb_free_urb(*urb);
+ return -ENOMEM;
+}
+
+#define UASP_TMF_TIMEOUT (5*HZ)
+
+/**
+ * uasp_do_tmf -- Execute the desired TMF
+ * @sdev: the device to which we should send the TMF
+ * @tmf: the task management function to execute
+ * @ttbm: the tag of task to be managed
+ *
+ * This function returns a negative value on error (-ENOMEM, etc), or
+ * an integer with bytes 3, 2 and 1 being the high to low byte of the
+ * Additional Response Information field and byte 0 being the Response
+ * code of the Response IU.
+ *
+ * If the response code is 0xFF, then the TMF timed out.
+ */
+static int uasp_do_tmf(struct scsi_cmnd *cmd, u8 tmf, u8 ttbm)
+{
+ struct scsi_device *sdev = cmd->device;
+ struct uasp_tport_info *tpinfo = SDEV_TPORT_INFO(sdev);
+ struct uasp_lu_info *luinfo = SDEV_LU_INFO(sdev);
+ struct urb *tmf_urb = NULL;
+ struct urb *resp_urb = NULL;
+ unsigned char *tiu;
+ struct scsi_lun slun;
+ int res;
+
+ /* scsi_dev should contain u8[8] for a LUN, not an unsigned int!
+ */
+ int_to_scsilun(sdev->lun, &slun);
+ res = uasp_alloc_tmf_urb(&tmf_urb, tpinfo, tmf, ttbm, slun.scsi_lun);
+ if (res)
+ return -ENOMEM;
+
+ tiu = tmf_urb->transfer_buffer;
+ res = uasp_alloc_resp_urb(&resp_urb, sdev, GET_IU_TAG(tiu));
+ if (res) {
+ usb_free_urb(tmf_urb);
+ return -ENOMEM;
+ }
+
+ init_completion(&luinfo->tmf_completion);
+ luinfo->tmf_resp = 0xFFFFFFFF;
+
+ res = usb_submit_urb(resp_urb, GFP_KERNEL);
+ if (res) {
+ complete(&luinfo->tmf_completion);
+ usb_free_urb(tmf_urb);
+ res = usb_unlink_urb(resp_urb);
+ if (res != -EINPROGRESS)
+ usb_free_urb(resp_urb);
+ return res;
+ }
+
+ res = usb_submit_urb(tmf_urb, GFP_KERNEL);
+ if (res) {
+ complete(&luinfo->tmf_completion);
+ res = usb_unlink_urb(tmf_urb);
+ if (res != -EINPROGRESS)
+ usb_free_urb(tmf_urb);
+ res = usb_unlink_urb(resp_urb);
+ if (res != -EINPROGRESS)
+ usb_free_urb(resp_urb);
+ return res;
+ }
+
+ wait_for_completion_timeout(&luinfo->tmf_completion, UASP_TMF_TIMEOUT);
+
+ if (luinfo->tmf_resp != 0xFFFFFFFF) {
+ res = luinfo->tmf_resp;
+ } else {
+ res = 0xFF;
+ }
+
+ return res;
+}
+
+static int uasp_er_tmf(struct scsi_cmnd *cmd, u8 tmf)
+{
+ struct scsi_device *sdev = cmd->device;
+ int tag;
+ int res;
+
+ if (sdev->current_cmnd == cmd)
+ tag = 1;
+ else
+ tag = cmd->request->tag + 2;
+
+ res = uasp_do_tmf(cmd, tmf, tag);
+
+ dev_dbg(&SDEV_TPORT_INFO(sdev)->udev->dev,
+ "%s: cmd:%p (0x%02x) tag:%d tmf:0x%02x resp:0x%08x\n",
+ __func__, cmd, cmd->cmnd[0], tag, tmf, res);
+
+ switch (TMR_RESPONSE_CODE(res)) {
+ case TMR_COMPLETE:
+ case TMR_SUCC:
+ return SUCCESS;
+ default:
+ return FAILED;
+ }
+}
+
+static int uasp_abort_cmd(struct scsi_cmnd *cmd)
+{
+ return uasp_er_tmf(cmd, TMF_ABORT_TASK);
+}
+
+static int uasp_device_reset(struct scsi_cmnd *cmd)
+{
+ return uasp_er_tmf(cmd, TMF_LU_RESET);
+}
+
+static int uasp_target_reset(struct scsi_cmnd *cmd)
+{
+ return uasp_er_tmf(cmd, TMF_IT_NEXUS_RESET);
+}
+
+static int uasp_bus_reset(struct scsi_cmnd *cmd)
+{
+ struct scsi_device *sdev = cmd->device;
+ struct uasp_tport_info *tpinfo = SDEV_TPORT_INFO(sdev);
+ struct usb_device *udev = tpinfo->udev;
+ int res;
+
+ res = usb_lock_device_for_reset(udev, tpinfo->iface);
+ if (res == 0) {
+ res = usb_reset_device(udev);
+ } else {
+ dev_err(&udev->dev, "%s: cmd:%p (0x%02x) failed to "
+ "lock dev (%d)\n",
+ __func__, cmd, cmd->cmnd[0], res);
+ return FAILED;
+ }
+
+ dev_dbg(&udev->dev, "%s: cmd:%p (0x%02x) (%d)\n",
+ __func__, cmd, cmd->cmnd[0], res);
+
+ if (res == 0)
+ return SUCCESS;
+
+ return FAILED;
+}
+
+/* ---------- SCSI Host support ---------- */
+
+static int uasp_slave_alloc(struct scsi_device *sdev)
+{
+ sdev->hostdata = kzalloc(sizeof(struct uasp_lu_info), GFP_KERNEL);
+ if (sdev->hostdata == NULL)
+ return -ENOMEM;
+
+ return 0;
+}
+
+static int uasp_slave_configure(struct scsi_device *sdev)
+{
+ struct uasp_tport_info *tpinfo = SDEV_TPORT_INFO(sdev);
+
+ scsi_set_tag_type(sdev, MSG_ORDERED_TAG);
+ scsi_activate_tcq(sdev, tpinfo->max_cmds);
+
+ return 0;
+}
+
+static void uasp_slave_destroy(struct scsi_device *sdev)
+{
+ kfree(sdev->hostdata);
+}
+
+static struct scsi_host_template uasp_host_template = {
+ .module = THIS_MODULE,
+ .name = DRIVER_NAME,
+ .queuecommand = uasp_queuecommand,
+ .slave_alloc = uasp_slave_alloc,
+ .slave_configure = uasp_slave_configure,
+ .slave_destroy = uasp_slave_destroy,
+ .eh_abort_handler = uasp_abort_cmd,
+ .eh_device_reset_handler = uasp_device_reset,
+ .eh_target_reset_handler = uasp_target_reset,
+ .eh_bus_reset_handler = uasp_bus_reset,
+ .can_queue = 1,
+ .cmd_per_lun = 1,
+ .this_id = -1,
+ .sg_tablesize = SG_NONE,
+ .max_sectors = SCSI_DEFAULT_MAX_SECTORS,
+ .use_clustering = ENABLE_CLUSTERING,
+ .skip_settle_delay = 1,
+ .ordered_tag = 1,
+};
+
+/* ---------- USB related ---------- */
+
+static const struct usb_device_id uasp_usb_ids[] = {
+ { USB_INTERFACE_INFO(USB_CLASS_MASS_STORAGE, USB_SC_SCSI, USB_PR_BULK)},
+ { USB_INTERFACE_INFO(USB_CLASS_MASS_STORAGE, USB_SC_SCSI, USB_PR_UAS) },
+ { }
+};
+MODULE_DEVICE_TABLE(usb, uasp_usb_ids);
+
+/* We don't have to do this here if Linux allowed tag usage for
+ * anything other than commands, i.e. TMFs which also use tags.
+ */
+static int uasp_set_max_cmds(struct uasp_tport_info *tpinfo)
+{
+ int mc;
+
+ /* The range of tags generated by the block layer would be
+ * [0, max_cmds-1], which is [0, num_streams-3]. Now reserve
+ * stream 1 for untagged commands submitted to us and the last
+ * usable stream id for a TMF to get the following stream id
+ * assignment:
+ * [1:untagged, [2, num_streams-1]:tagged, num_streams:TMF].
+ */
+ mc = tpinfo->num_streams - 2;
+ if (mc <= 0) {
+ /* Pathological case--perhaps fail discovery?
+ */
+ dev_notice(&tpinfo->udev->dev,
+ "device supports too few streams (%d)\n",
+ tpinfo->num_streams);
+ mc = max(1, tpinfo->num_streams - 1);
+ }
+
+ tpinfo->max_cmds = mc;
+
+ return 0;
+}
+
+static int uasp_ep_conf(struct uasp_tport_info *tpinfo)
+{
+ struct usb_device *udev = tpinfo->udev;
+ struct usb_interface *iface = tpinfo->iface;
+ struct usb_host_endpoint *epa = iface->cur_altsetting->endpoint;
+ int numep = iface->cur_altsetting->desc.bNumEndpoints;
+ int i;
+
+ for (i = 0; i < numep; i++) {
+ unsigned char *desc = epa[i].extra;
+ int len = epa[i].extralen;
+
+ for ( ; len > 1; len -= desc[0], desc += desc[0]) {
+ unsigned pid = desc[2];
+
+ if (desc[0] != 4 || desc[1] != USB_DT_PIPE_USAGE)
+ continue;
+ else if (CMD_PIPE_ID <= pid && pid <= DATAOUT_PIPE_ID) {
+ tpinfo->eps[pid - 1] = &epa[i];
+ break;
+ }
+ }
+ }
+
+ if (tpinfo->eps[0] == NULL || tpinfo->eps[1] == NULL ||
+ tpinfo->eps[2] == NULL || tpinfo->eps[3] == NULL) {
+ dev_err(&udev->dev, "%s: one or more endpoints are missing\n",
+ __func__);
+ return -1;
+ }
+
+ tpinfo->cmd_pipe = usb_sndbulkpipe(udev,
+ tpinfo->eps[0]->desc.bEndpointAddress);
+ tpinfo->status_pipe = usb_rcvbulkpipe(udev,
+ tpinfo->eps[1]->desc.bEndpointAddress);
+ tpinfo->datain_pipe = usb_rcvbulkpipe(udev,
+ tpinfo->eps[2]->desc.bEndpointAddress);
+ tpinfo->dataout_pipe = usb_sndbulkpipe(udev,
+ tpinfo->eps[3]->desc.bEndpointAddress);
+
+ if (udev->speed == USB_SPEED_SUPER) {
+ for (i = 1; i < 4; i++) {
+ if (tpinfo->max_streams == 0)
+ tpinfo->max_streams = USB_SS_MAX_STREAMS(tpinfo->eps[i]->ss_ep_comp.bmAttributes);
+ else
+ tpinfo->max_streams = min(tpinfo->max_streams,
+ USB_SS_MAX_STREAMS(tpinfo->eps[i]->ss_ep_comp.bmAttributes));
+ }
+
+ if (tpinfo->max_streams <= 1) {
+ dev_err(&udev->dev, "%s: no streams\n", __func__);
+ return -1;
+ }
+
+ tpinfo->use_streams = 1;
+ tpinfo->num_streams = usb_alloc_streams(iface,
+ &tpinfo->eps[1], 3,
+ tpinfo->max_streams,
+ GFP_KERNEL);
+ if (tpinfo->num_streams <= 0) {
+ dev_err(&udev->dev,
+ "%s: Couldn't allocate %d streams (%d)\n",
+ __func__, tpinfo->max_streams,
+ tpinfo->num_streams);
+ return -1;
+ }
+
+ uasp_set_max_cmds(tpinfo);
+ dev_info(&udev->dev, "streams:%d allocated streams:%d\n",
+ tpinfo->max_streams, tpinfo->num_streams);
+ } else {
+ tpinfo->use_streams = 0;
+ tpinfo->max_cmds = 32; /* Be conservative */
+ }
+
+ return 0;
+}
+
+static int uasp_set_alternate(struct usb_interface *iface)
+{
+ struct usb_device *udev = interface_to_usbdev(iface);
+ int i, res;
+
+ for (i = 0; i < iface->num_altsetting; i++) {
+ struct usb_host_interface *hi = &iface->altsetting[i];
+
+ if (hi->desc.bInterfaceProtocol == USB_PR_UAS) {
+ int ifnum = iface->cur_altsetting->
+ desc.bInterfaceNumber;
+ int alt = hi->desc.bAlternateSetting;
+
+ res = usb_set_interface(udev, ifnum, alt);
+ if (res) {
+ dev_err(&udev->dev, "%s: Couldn't "
+ "set alternate %d iface (%d)\n",
+ __func__, alt, res);
+ return -ENODEV;
+ }
+ return 0;
+ }
+ }
+ dev_err(&udev->dev, "%s: Match, but no suitable alt "
+ "iface setting\n", __func__);
+ return -ENODEV;
+}
+
+/* TODO: We should ideally register a SCSI Target port with the SCSI
+ * subsystem and let the SCSI Core perform, in that order, REPORT LUN,
+ * INQUIRY, TUR, etc, for each LU, and register each LU as a SCSI
+ * Device.
+ *
+ * We should modify/change/remove the struct scsi_host concept, as we
+ * now see SCSI over a myriad of other protocols, where the host
+ * controller is NOT a SCSI controller at all. This however deserves
+ * it's own patch.
+ */
+static int uasp_probe(struct usb_interface *iface,
+ const struct usb_device_id *id)
+{
+ int res;
+ struct Scsi_Host *shost = NULL;
+ struct uasp_tport_info *tpinfo;
+ struct usb_device *udev = interface_to_usbdev(iface);
+
+ if (id->bInterfaceProtocol == USB_PR_BULK) {
+ res = uasp_set_alternate(iface);
+ if (res)
+ return res;
+ }
+
+ tpinfo = kzalloc(sizeof(struct uasp_tport_info), GFP_KERNEL);
+ if (tpinfo == NULL)
+ return -ENOMEM;
+
+ tpinfo->iface = iface;
+ tpinfo->udev = udev;
+
+ res = uasp_ep_conf(tpinfo);
+ if (res)
+ goto Free;
+
+ res = -ENOMEM;
+ shost = scsi_host_alloc(&uasp_host_template, sizeof(void *));
+ if (shost == NULL)
+ goto Free;
+
+ /* The protocol supports XCDB, but that won't fit unsigned short,
+ * so claim variable length CDB.
+ */
+ shost->max_cmd_len = 260;
+ shost->max_id = 1;
+ shost->sg_tablesize = udev->bus->sg_tablesize;
+ shost->can_queue = tpinfo->max_cmds;
+ shost->cmd_per_lun = tpinfo->max_cmds;
+
+ dev_info(&shost->shost_gendev, "max commands: %d\n", tpinfo->max_cmds);
+
+ res = scsi_add_host(shost, &iface->dev);
+ if (res)
+ goto Free;
+
+ shost->hostdata[0] = (unsigned long)tpinfo;
+ scsi_scan_host(shost);
+ usb_set_intfdata(iface, shost);
+
+ return res;
+ Free:
+ kfree(tpinfo);
+ if (shost)
+ scsi_host_put(shost);
+ return res;
+}
+
+static int uasp_pre_reset(struct usb_interface *iface)
+{
+ return 0;
+}
+
+static int uasp_post_reset(struct usb_interface *iface)
+{
+ return 0;
+}
+
+static void uasp_disconnect(struct usb_interface *iface)
+{
+ struct Scsi_Host *shost = usb_get_intfdata(iface);
+ struct uasp_tport_info *tpinfo = (void *)shost->hostdata[0];
+
+ scsi_remove_host(shost);
+ usb_free_streams(iface, &tpinfo->eps[1], 3, GFP_KERNEL);
+
+ kfree(tpinfo);
+}
+
+static struct usb_driver uasp_driver = {
+ .name = DRIVER_NAME,
+ .probe = uasp_probe,
+ .disconnect = uasp_disconnect,
+ .pre_reset = uasp_pre_reset,
+ .post_reset = uasp_post_reset,
+ .id_table = uasp_usb_ids,
+};
+
+static int uasp_init(void)
+{
+ return usb_register(&uasp_driver);
+}
+
+static void uasp_exit(void)
+{
+ usb_deregister(&uasp_driver);
+}
+
+module_init(uasp_init);
+module_exit(uasp_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Luben Tuikov");
--
1.7.2.2.165.gbc382
On Mon, Dec 06, 2010 at 09:05:11AM -0800, Luben Tuikov wrote:
> This driver allows you to connect to UAS devices
> and use them as SCSI devices.
We already have this support in the kernel tree, why do you want and
need another driver that does the same exact thing?
thanks,
greg k-h
--- On Mon, 12/6/10, Greg KH <[email protected]> wrote:
>
> We already have this support in the kernel tree,
Greg, this isn't an effort for "linux kernel code turf".? This is a well-implemented driver according to the respective specifications (SAM, SPC, USB 2/3). It reflects correct implementation, naming, etc. It shows understanding of the abstraction layers and underlying technology. It's what you'd want to have in /YOUR/ kernel.
> why do you want and need another driver that does the same exact thing?
Nothing can be further from the truth. The current uas.c driver doesn't do the "exact same thing", far from it.
You responded 4 minutes after my post. Did you have to time to verify that the driver I posted does "the exact same thing" as the one in the kernel?
"The exact same thing", let's see:
1) uasp.c doesn't assume the alternate setting of the interface and discovers its setting. uas.c assumes it's 1.
2) uas.c doesn't do any error checking of the endpoints, and goes ahead
and registers a scsi host first, and then "configures" the endpoints, while end point configuration/discovery may fail or be incorrect.
3) uasp.c correctly parses the pipe usage endpoint descriptors and
saves them in a target port abstraction. If any were not present, discovery of the device fails.
4) uasp.c discovers the maximum number of streams we can use of the streaming endpoints (their minimum) of the interface, uasp blindly requests 256.
5) uasp.c fails discovery if no streaming is supported (bad configuration) for a SuperSpeed device. uas.c doesn't do any checking and doesn't discern between SS and HS (it relies on usb_alloc_streams() to fail, thus it cannot discover the correct/valid number of streams of the target interface).
6) uasp.c correctly sets the number of commands the block/SCSI layer can enqueue, reserving tags for TMF and untagged commands. uas.c has no concept of TMFs and doesn't do this.
7) uas.c incorrectly sets some members of shost. uasp.c does this correctly.
8) uas.c is filled with incorrect comments showing the unfamiliarity of the author(s) with SAM, SPC, USB, etc. For example,
SCSI:
> .can_queue = 65536,??? /* Is there a limit on the _host_ ? */
Unfamiliarity with USB or SAM or both:
> /* XXX: Can we reset just the one USB interface?
>? * Would calling usb_set_interface() have the right effect?
>? */
and
> /* XXX: Need to return 1 if it's not our device in error handling */
Unfamiliarity with UAS/SS (scary commend follows, read at your own risk):
> /*
>? * Why should I request the Status IU before sending the Command IU? Spec
>? * says to, but also says the device may receive them in any order.? Seems
> * daft to me.
> */
Unfamiliarity with SAM, SCSI, etc: the driver trying to re-queue
failed Execute Command procedure with the transport. The driver abusing SCSI_MLQUEUE_DEVICE_BUSY and its usage of a work queue to both requeue URBs and data (wrong in transport protocol driver).? These result in infinite loops in SCSI/block layer when the device goes out to lunch (I can test quite a few possibilities), or errors out, or errors out in the middle of a HighSpeed transaction requiring RRDY and WRDY.
9) uasp.c provides correct (extensible) abstraction of underlying (represented) objects (Target port, LU and command, a la I_T_L_Q nexus). uas.c lacks this.
10) uasp.c implements TMF for both SS and HS modes of operation (directly related to 9 above.) uas.c cannot do this as it has no representation of the I_T_L_Q nexus.
11) uasp.c provides correct naming of structs, constants, etc. It is what you want in a protocol driver. uas.c lacks this.
12) uasp.c does NOT implement packed structs to represent the information units, nor does it use be_to_cpu[p]() and cpu_to_be() to read/write values from said units. It is a portability nightmare (alignment, flexibility, etc). Instead uasp.c uses byte access, shift and or logical operations to extract values from the information units.? uas.c does implement packed structs, and does use be_to_cpu[p]() and cpu_to_be(), again a portability nightmare.
13) uas.c uses infantile non-value comments like:
> /* I hate forward declarations, but I actually have a loop */
14) uasp.c correctly uses dev_dbg()/dev_err()/dev_info()/etc, to report interesting information at each level of debugging. Thus it supports dynamic debugging.
15) uasp.c correctly implements the protocol and doesn't allow for non-conforming devices to be used (which are by default NON-UAS), such as devices that don't implement the required pipes, or early (out of protocol) information units. Such devices should never see the light of day, as they will cause interoperability problems. Dynamic ids are also supported by uasp.c.
16) uasp.c implement a module parameter limiting the number of streams to request from XHCI HCD to allocate. This is due to the fact that some HC misreport the number of streams they support by a factor of two since they filled in the HCCPARAMS field MaxPSASize. That is, they solved 2^v = streams, instead of 2^(v+1) = streams. Of course when the device tries to send back status, the host says ACK(NumP=0) and the device goes into flow control... etc. This module parameter allows to limit the streams allocated.
17) uasp.c implements correct tag assignment for TMFs such that should we get into 16 above, a TMF status _can_ be returned to the host. uas.c has no concept of TMFs to begin with.
18) uasp.c correctly checks the device configuration and fails discovery as it correctly reports that to USB.
There is perhaps more.
Luben
On Wed, Dec 08, 2010 at 04:16:06PM -0800, Luben Tuikov wrote:
> --- On Mon, 12/6/10, Greg KH <[email protected]> wrote:
> >
> > We already have this support in the kernel tree,
>
> Greg, this isn't an effort for "linux kernel code turf".? This is a
> well-implemented driver according to the respective specifications
> (SAM, SPC, USB 2/3). It reflects correct implementation, naming, etc.
> It shows understanding of the abstraction layers and underlying
> technology. It's what you'd want to have in /YOUR/ kernel.
What I don't want in _anyones_ kernel is multiple drivers wanting to
attach to the same device.
> > why do you want and need another driver that does the same exact thing?
>
> Nothing can be further from the truth. The current uas.c driver
> doesn't do the "exact same thing", far from it.
>
> You responded 4 minutes after my post. Did you have to time to verify
> that the driver I posted does "the exact same thing" as the one in the
> kernel?
By "same thing" I mean "binds to the same device."
I have lived through the hell of this in the past and I will not let
that happen again, sorry.
>
> "The exact same thing", let's see:
<snip>
I'm not saying that the in-kernel driver is perfect at all, and it's
author is getting annoying by refusing to answer my emails, but still,
please work with him to get the in-kernel driver to work up to your
needs.
If you have problems with the maintainer, please let me know and I'll
see what I can do.
But again, this driver is not acceptable as-is, sorry.
thanks,
greg k-h
On Wed, Dec 08, 2010 at 05:52:04PM -0800, Greg KH wrote:
> I'm not saying that the in-kernel driver is perfect at all, and it's
> author is getting annoying by refusing to answer my emails, but still,
> please work with him to get the in-kernel driver to work up to your
> needs.
>
> If you have problems with the maintainer, please let me know and I'll
> see what I can do.
>
> But again, this driver is not acceptable as-is, sorry.
Greg, I completely appreciate your position on this. I would just like to
add one thought for your consideration: If, in fact, Luben's driver is
somehow "better", it would be a shame to penalize users with a
lower-quality driver because they got to the submission first.
Now, for that to apply, several things would have to be true. I do not
know if they are true here, and I leave it to your discression to
determine. But, your stated problems getting hold of the maintainer make
me wonder....
That said, unless the current driver is really bad, and Luben's is
tremendously better, it seems to make more sense for everyone to focus on
improving the in-kernel driver, even if that means slowly re-writing it
over time. Lord knows that is basically what I did with the usb-storage
driver...
Matt
--
Matthew Dharm Home: [email protected]
Maintainer, Linux USB Mass Storage Driver
I want my GPFs!!!
-- Stef
User Friendly, 11/9/1998
Before I begin, I'd like everyone to note that Greg has completely ignored (and removed from the quotation) the 18 points of technical matter in my email and instead decided to write this, to which I'm responding here:
--- On Wed, 12/8/10, Greg KH <[email protected]> wrote:
>
> What I don't want in _anyones_ kernel is multiple drivers
> wanting to
> attach to the same device.
>
> By "same thing" I mean "binds to the same device."
Thanks for clarifying, Greg. For a moment everyone thought that by "the exact same thing" you meant "same functionality, same quality, identical, etc".
Now, if you don't want the still-born uas.c to bind to the same device, please simply remove the ids from that driver.
> I have lived through the hell of this in the past and I
> will not let
> that happen again, sorry.
So, according to your statement above "binding to the same device" is all it takes. The solution is very simple--see above.
So to expose the obvious, hypothetically, one of the resident "linux kernel engineers/maintainers" could've submitted AN EMPTY DRIVER that "binds to" the UAS id, and from then on, any other improvements would have to go through them, even if they don't work with the technology and are not very (at all?) knowledgeable about the technologies/standards/methodologies/termninology involved.
> I'm not saying that the in-kernel driver is perfect at all,
I think you did say "the exact same thing" ;-) in your previous email in this thread saying it "*does* the exact same thing".
So I guess, you didn't clarify what you meant by "does" when you said "does the exact same thing". Thanks for clarifying that by "does" you mean "binds to the same ids".
> and it's
> author is getting annoying by refusing to answer my emails,
> but still,
> please work with him to get the in-kernel driver to work up
> to your
> needs.
Why are you pushing him to this when he's getting annoying to you? When he's not responding to your emails? Is he your friend? (perhaps more?--none of my business) Do you guys have beers at LinuxCon or other Linux conferences? Is he in the technology sector? Or is he just a "linux kernel engineer"? A guru in USB? Or maybe SCSI? Is his employer, Intel, pressuring you? Because when you removed all possibilities, however improbable, must be the truth. (after all they did write the XHCI HCD) All I see is a lot of nepotism, an inferior coding, inferior terminology used, inferior naming convention, incompetence in USB and SCSI protocols, etc, etc, etc.
Were you not part of the same thread where YOU RESPONDED addressing its author about his cowardly and lacking-professional-integrity ways of changing my patches including changing the commit messages without reporting what's been changed, how it's been changed and where it's been committed to? To refresh everyone's memory, here:
http://marc.info/?l=linux-usb&m=129133499714636&w=2
I guess you had to do that to be released of some kind of binding?
Do you not see HOW DIFFERENT the two drivers are? Do you not see the difference in quality, presentation, etc, etc.
Linux is truly open source, but closed community. You have shown in this thread that it's not about the code or the technology, but the turf. After I listed the technical differences between the two drivers in this thread (18 and counting), after you've seen the code and leading on with this thread despite the problems you're having with him and his not responding to your emails.? Why you are protecting his interests so blatantly despite the problems you're having with him is beyond me. (other than, again, if his employers is involved and pressuring you)
"...whatever is left, however improbable, must be the truth."
> If you have problems with the maintainer, please let me
> know and I'll
> see what I can do.
What? Were you not on the CC list of the patches I posted, did you not state in those threads you're having problems with him too? You yourself admitted above that you also have problems with him in those very threads. You are protecting a person, not code.
Here are the threads:
http://marc.info/?t=128821112700001&r=1&w=2
http://marc.info/?t=128829652800022&r=1&w=2
http://marc.info/?t=128839891500022&r=1&w=2
http://marc.info/?t=128901886000001&r=1&w=2
> But again, this driver is not acceptable as-is, sorry.
"as-is"? You are giving me a clue, Greg! (Since you're NOT specifying any technical reasons.)
Greg, (to expose the obvious agenda and interests being protected) how about if I added his name as an author to my driver? Will you accept it then? Or perhaps I can send in a patch ripping out everything from the still-born uas.c other than the copyright and MODULE_AUTHOR() and substituting in the contents of my uasp.c (removing my name altogether)?
After all, people and the industry are learning a lot from this thread.? They are learning that the open source Linux kernel has been institutionalized between a few chosen people, regardless of capability, thus creating a closed community whose interests are highly protected. It's not about the code or the technology any more.
I think people need to know.
? ???Luben
> I think people need to know.
I think they will judge themselves from the full discussion. I'm not sure
they'll vote in your favour however.
"So to expose the obvious, hypothetically, one of the resident "linux
kernel engineers/maintainers" could've submitted AN EMPTY DRIVER that
"binds to" the UAS id, and from then on, any other improvements would
have to go through them"
Strangely that doesn't work because the community isn't full of idiots,
in fact we actively select against them because anyone who is maintaining
a driver needs to be able to deal with other people (at least other
people who wish to be constructive).
"Do you not see HOW DIFFERENT the two drivers are? Do you not see the
difference in quality, presentation, etc, etc."
I find the presentation *very* different. I'm rather worried about the
manner in which it is being presented. Your driver may be the best on the
planet - who knows - but if your response to needing to work with people
is to accuse them of being part of some giant conspiracy or make offensive
comments then that's rapidly going to outweight the quality of the code
and you'll simply run out of people to talk to.
It's actually pretty hard to get in GregKH's kill file, and I suspect by
now you are in a few others as well. So perhaps you can find someone
working with you who has people skills and can sit between you and Greg
and other maintainers and make progress ? It won't be the first time a
user interface problem has been fixed that way in the Linux world.
But basically it's really simple. When we have an existing driver we work
from it - step by step from where we are, to where we want to be. That
series of steps should tell a story so anyone reading the patch series can
understand how it unfolded.
We also have a process for dealing with people not responding, and
irritating them enough they killfile you and refuse to deal with you
isn't that process, especially when you then get yourself killfiled by
the next maintainer up as well.
When you come along late and say "I've got this great other driver", then
sorry you missed the party - you could have submitted yours earlier and
the question becomes "how do I make the existing driver at least as cool
as the one that was too late" not "how do I replace it"
Alan
Alan, don't you work for the same company as do the authors listed in "uas.c"?
--- On Fri, 12/10/10, Alan Cox <[email protected]> wrote:
>
> I think they will judge themselves from the full
> discussion. I'm not sure
> they'll vote in your favour however.
Alan, you're priming up here what people should think.
> "So to expose the obvious, hypothetically, one of the
> resident "linux
> kernel engineers/maintainers" could've submitted AN EMPTY
> DRIVER that
> "binds to" the UAS id, and from then on, any other
> improvements would
> have to go through them"
>
> Strangely that doesn't work because the community isn't
> full of idiots,
I didn't say it is.
> in fact we actively select against them because anyone who
> is maintaining
> a driver needs to be able to deal with other people (at
> least other
> people who wish to be constructive).
Did you not see the problems Greg has had with Matthew, your co-worker, and stating so? Let me refresh your memory:
http://marc.info/?l=linux-kernel&m=129133499814639&w=2
> "Do you not see HOW DIFFERENT the two drivers are? Do you
> not see the
> difference in quality, presentation, etc, etc."
>
> I find the presentation *very* different. I'm rather
> worried about the
> manner in which it is being presented.
Wait a minute... So a commit patch is not enough any more? Code is not enough anymore? Quick and knowledgeable responses are not enough anymore?
What was the "uas.c" presentation? ("uas.c" is written by a co-employee of yours.) Can you point me to it?
But you make a very good point: "presentation" is all there is to anything. If you've got the right presentation you can convince people of *anything*, literally anything.
For example, your response to which I'm replying is one such "presentation".
> Your driver may be
> the best on the
> planet
Well, I don't know Alan? Is it? You can read code, right. No ONE has commented on the code, on technical matter at all. Instead Greg snipped it all out.
> - who knows - but if your response to needing to
> work with people
> is to accuse them of being part of some giant conspiracy or
> make offensive
> comments
Wait a minute here... Is it about code and functionality then? Because if it is I didn't see any technical comments on the drivers.
> then that's rapidly going to outweight the quality
> of the code
> and you'll simply run out of people to talk to.
Here you're involving intangible things, but i realize it's part of your presentation.
> It's actually pretty hard to get in GregKH's kill file, and
It's just a move on his part to show he doesn't want deal with this anymore. Clearly he's expressed an opinion of having hard time working with uas.c's author, co-employee of yours. Clearly he's shown he doesn't want to comment on the technical matters, code, etc, despite all this he's shown to protect defunct code, written by a co-employee of yours, with comments in it like this:
> /* I hate forward declarations, but I actually have a loop */
and:
> /*
> * Why should I request the Status IU before sending the Command IU? Spec
> * says to, but also says the device may receive them in any order. Seems
> * daft to me.
> */
Anyway, I've noted these and other technical matter in the 18 major differences between the two drivers here:
http://marc.info/?l=linux-usb&m=129185378612218&w=2
> I suspect by
> now you are in a few others as well. So perhaps you can
You are telling people here what they should think and do.
> find someone
> working with you who has people skills and can sit between
> you and Greg
> and other maintainers and make progress ?
There is no agenda for Linux and there has never been. My submission was purely to help others and give them something useful they can use to test and develop their UAS devices, an alternative to Windows. It took me two days to write this driver. I decided to help people because I could. There never has been an agenda to have a UAS driver (let alone mine) in Linux. But it was interesting to see the dynamics involved.
> It won't be the
> first time a
> user interface problem has been fixed that way in the Linux
> world.
Or the last. Linux is open source but closed community.
> But basically it's really simple. When we have an existing
> driver we work
> from it - step by step from where we are, to where we want
> to be.
The existing driver shows lack of knowledge of UAS and SCSI. It falls short of almost everything. For the details see the 18 points here:
http://marc.info/?l=linux-usb&m=129185378612218&w=2
Why would you want to keep this in the kernel and "start from it", when we can rip it out, put the working and correct uasp.c in and then everyone including Matthew can contribute to uasp.c. It doesn't seem he works with the technology as I haven't seen any new patches to uas.c.
It's quick and constructive process. Out with the defunct and and in with what works. It's all about getting things done.
> That
> series of steps should tell a story so anyone reading the
> patch series can
> understand how it unfolded.
But there would be NOTHING LEFT of the original driver. Just one patch like this I posted here to uas.c changed 86% of the code. A diff between uasp.c and uas.c shows 100% difference.
> We also have a process for dealing with people not
> responding, and
"Not responding" -- are you talking about your co-worker?
> irritating them enough they killfile you and refuse to deal
> with you
> isn't that process, especially when you then get yourself
> killfiled by
> the next maintainer up as well.
>
> When you come along late and say "I've got this great other
> driver", then
> sorry you missed the party - you could have submitted yours
> earlier and
> the question becomes "how do I make the existing driver at
> least as cool
> as the one that was too late" not "how do I replace it"
Alan, none of this is really important. As technologist, I thought it would be helpful and useful to write a functional UAS driver. Since the one in the kernel, was defunct, judging from how it behaved, how it locked up, how it went into an infinite loop, and the comments there-in. I did submit patches to uas.c, which got secretly modified, both the commit message and the patch, but never posted publicly as to what was modified. Even Greg commented on this not-so-good practice (see link above).
The uasp.c took two days to write.
But here is what I would've done had someone, especially a technologist, posted a working driver while mine didn't do the job: I would not have been quiet about it but simply asked to include the better working driver and I'd contributed whatever I could to it. It's a quick, practical and constructive thing to do. It's what gets things done. In the long run though "uas.c" would look exactly like "uasp.c" looks today. There is just so few ways to do the same thing.
However this isn't the first time this will happen or the last. rlt8139.c and 8139too.c come to mind, although the former had been around for a long time and there were plenty of devices out there for it. There maybe other instances like this.
Luben
On Fri, Dec 10, 2010 at 06:26:56PM -0800, Luben Tuikov wrote:
[Reflowed your text into 80 columns, you might want to look at your MUA
configuration here.]
> --- On Fri, 12/10/10, Alan Cox <[email protected]> wrote:
> > "Do you not see HOW DIFFERENT the two drivers are? Do you
> > not see the
> > difference in quality, presentation, etc, etc."
> >
> > I find the presentation *very* different. I'm rather
> > worried about the
> > manner in which it is being presented.
> Wait a minute... So a commit patch is not enough any more? Code is not
> enough anymore? Quick and knowledgeable responses are not enough
> anymore?
The issue here is with the kernel change and risk management processes
rather than the code.
Your new code adds a driver which replicates the functionality of an
existing driver. We've had multiple implementations of the same
functionality in the past. Usually what happens is that users and
distros get confused and end up swapping randomly between the two
different implemenations trading off the different bug and feature sets,
which doesn't make anyone happy and there's a general idea that we
should try to avoid that.
This means that the 1000 foot review comment is what Greg has been
telling you - the standard approach is to work on the existing code in
place, incrementally making it better. This avoids the problem with bug
tradeoff (as there's only ever one version in the kernel at once) and
makes it much easier to isolate any new problems if they are introduced.
Sometimes this isn't possible or a good idea for some reason, in which
case the change should really explain that in the changelog (usually
everyone involved will have some awareness of the issues already but a
summary is useful for people picking up a new kernel release or
similar). At the very least proposing such changes needs to involve
some discussion of why a rewrite is required, and there needs to be some
sort of plan for how everything should converge back onto a single
implementation again.
> http://marc.info/?l=linux-usb&m=129185378612218&w=2
This is a good summary of what improvements the new driver brings
(ideally more of it would have gone in the changelog), the missing bit
is an explanation of why these issues can't be addressed with the usual
process of incremental improvements to the existing code, discussion
of how the existence of the two separate implementations would be
resolved and discussion of the user visible impact of swapping to a new
implementation.
Bear in mind that all your changelog said originally was:
| UASP: USB Attached SCSI (UAS) protocol driver
| This driver allows you to connect to UAS devices
| and use them as SCSI devices
which doesn't say much more than that there's a new implementation of
this.
On 13.12.2010 18:40, Mark Brown wrote:
> On Fri, Dec 10, 2010 at 06:26:56PM -0800, Luben Tuikov wrote:
>
> [Reflowed your text into 80 columns, you might want to look at your MUA
> configuration here.]
>
> > --- On Fri, 12/10/10, Alan Cox <[email protected]> wrote:
>
> > > "Do you not see HOW DIFFERENT the two drivers are? Do you
> > > not see the
> > > difference in quality, presentation, etc, etc."
> > >
> > > I find the presentation *very* different. I'm rather
> > > worried about the
> > > manner in which it is being presented.
>
> > Wait a minute... So a commit patch is not enough any more? Code is not
> > enough anymore? Quick and knowledgeable responses are not enough
> > anymore?
>
> The issue here is with the kernel change and risk management processes
> rather than the code.
>
> Your new code adds a driver which replicates the functionality of an
> existing driver. We've had multiple implementations of the same
> functionality in the past. Usually what happens is that users and
> distros get confused and end up swapping randomly between the two
> different implemenations trading off the different bug and feature sets,
> which doesn't make anyone happy and there's a general idea that we
> should try to avoid that.
>
> This means that the 1000 foot review comment is what Greg has been
> telling you - the standard approach is to work on the existing code in
> place, incrementally making it better. This avoids the problem with bug
> tradeoff (as there's only ever one version in the kernel at once) and
> makes it much easier to isolate any new problems if they are introduced.
>
> Sometimes this isn't possible or a good idea for some reason, in which
> case the change should really explain that in the changelog (usually
> everyone involved will have some awareness of the issues already but a
> summary is useful for people picking up a new kernel release or
> similar). At the very least proposing such changes needs to involve
> some discussion of why a rewrite is required, and there needs to be some
> sort of plan for how everything should converge back onto a single
> implementation again.
>
> > http://marc.info/?l=linux-usb&m=129185378612218&w=2
>
> This is a good summary of what improvements the new driver brings
> (ideally more of it would have gone in the changelog), the missing bit
> is an explanation of why these issues can't be addressed with the usual
> process of incremental improvements to the existing code, discussion
> of how the existence of the two separate implementations would be
> resolved and discussion of the user visible impact of swapping to a new
> implementation.
When done immediatly there won't have been a "user visible impact" as
uas was only accepted a few weeks earlier than uasp, to be releases with
2.6.37.
As uas has not been in a release-kernel, by definition, there is no
impact either way. No regressions or anything else.
If uas is reverted (or disabled like fanotify was for 2.6.36) NOW, there
is time to resolve the matter for 2.6.38.
If uas is of the quality that Luben describes, it isn't fit for release
with 2.6.37 anyway (or only with an EXPERIMENTAL tag, which it hasn't
and doesn't depend on).
As a future user of UASP (The protocol) i'm happy to spend a few more
weeks with UMS if it means the driver for UASP is really ready for
transferring TBs of data when released.
(Altough i'm guessing that i will spend some more time with UMS anyway,
as neither the packaging nor the website for my USB 3.0 enclosures
mention UASP)
Bis denn
--
Real Programmers consider "what you see is what you get" to be just as
bad a concept in Text Editors as it is in women. No, the Real Programmer
wants a "you asked for it, you got it" text editor -- complicated,
cryptic, powerful, unforgiving, dangerous.
--- On Mon, 12/13/10, Mark Brown <[email protected]> wrote:
> > --- On Fri, 12/10/10, Alan Cox <[email protected]>
> wrote:
>
> > > "Do you not see HOW DIFFERENT the two drivers
> are? Do you
> > > not see the
> > > difference in quality, presentation, etc, etc."
> > >
> > > I find the presentation *very* different. I'm
> rather
> > > worried about the
> > > manner in which it is being presented.
>
> > Wait a minute... So a commit patch is not enough any
> more? Code is not
> > enough anymore? Quick and knowledgeable responses are
> not enough
> > anymore?
>
> The issue here is with the kernel change and risk
> management processes
> rather than the code.
There are no UAS devices in the market. Risk 0. The in-kernel UAS driver is defunct, both in the way it is written, the terminology it uses and in the fact that it simply does not work. See the technological treatment here:
http://marc.info/?l=linux-usb&m=129185378612218&w=2
> Your new code adds a driver which replicates the
> functionality of an
> existing driver.
"eplicates the functionality of an existing driver": Now read this again:
http://marc.info/?l=linux-usb&m=129185378612218&w=2
> We've had multiple implementations
> of the same
> functionality in the past.
I've not followed the kernel lately, but I do remember what you speak of: rtl8139.c and 8139too.c. And this duplicate functionality was in fact a re-write (as the notice in the latter driver explains) WHILE there were hundreds of thousands devices out there. Why re-write instead of "contribute to the one in the kernel, patch by patch" as Greg and Alan would like you to believe? And at the same time WHILE there are (were) so many RealTek chips out there...
>? Usually what happens is
> that users and
> distros get confused and end up swapping randomly between
> the two
> different implemenations trading off the different bug and
> feature sets,
> which doesn't make anyone happy and there's a general idea
> that we
> should try to avoid that.
Ah, yes, I see: you're trying to _protect_ distros and end users. But the fact is that users and distros are smart enough to figure out what works and what doesn't work. People working at distros are smart enough to test and see what works. And often enough, distros would contain, new and/or rewritten drivers from the one in the mainline, especially for new or rare devices which their customers want support. So, please don't try to pull this bs of "we're protecting the end user and distros".
> This means that the 1000 foot review comment is what Greg
> has been
> telling you - the standard approach is to work on the
> existing code in
> place, incrementally making it better.
Just one patch I submitted changed 86% of the driver:
http://marc.info/?l=linux-kernel&m=128901883520391&w=2
Adding them all up, it'll be 100%. uas.c should be ripped out altogether. Do it now, while there are still no devices out there.
> This avoids
> the problem with bug
> tradeoff (as there's only ever one version in the kernel at
> once) and
> makes it much easier to isolate any new problems if they
> are introduced.
The only problems I've seen so far are with uas.c.
> Sometimes this isn't possible or a good idea for some
> reason, in which
> case the change should really explain that in the changelog
> (usually
> everyone involved will have some awareness of the issues
> already but a
> summary is useful for people picking up a new kernel
> release or
> similar).
See this: http://marc.info/?l=linux-usb&m=129185378612218&w=2
> At the very least proposing such changes
> needs to involve
> some discussion of why a rewrite is required, and there
> needs to be some
> sort of plan for how everything should converge back onto a
> single
> implementation again.
uas.c doesn't work and is very badly written piece of code. uasp.c works and complies with USB, UAS, SCSI and Linux kernel code programming guidlines. Again see this:
http://marc.info/?l=linux-usb&m=129185378612218&w=2
> > http://marc.info/?l=linux-usb&m=129185378612218&w=2
Ah, I see, so you have it.
> This is a good summary of what improvements the new driver
> brings
> (ideally more of it would have gone in the changelog), the
> missing bit
> is an explanation of why these issues can't be addressed
> with the usual
> process of incremental improvements to the existing code,
> discussion
> of how the existence of the two separate implementations
> would be
> resolved and discussion of the user visible impact of
> swapping to a new
> implementation.
I'm not sure what needs to be resolved: rip out the defunct uas.c and put in the working uasp.c. Then move on with more interesting things (like one's real job).
> Bear in mind that all your changelog said originally was:
>
> | UASP: USB Attached SCSI (UAS) protocol driver
>
> | This driver allows you to connect to UAS devices
> | and use them as SCSI devices
>
> which doesn't say much more than that there's a new
> implementation of
> this.
It doesn't need to say any more, if the defunct uas.c is ripped out. This isn't a "why uasp.c should be in and uas.c out"--it's "here is a working UAS driver".
Luben
--- On Sat, 12/11/10, Alan Cox <[email protected]> wrote:
>
> > Alan, don't you work for the same company as do the
> authors listed in "uas.c"?
>
> If you've got nothing better to do than insinuate further
> unpleasant
> things when people try and help then don't expect any help
> from anyone.
>
> *plonk*
Alan,
When in doubt, follow the money.
The problem isn't in that you're trying to do what is convenient for the corporation which has hired you and pays your salary. The problem is that you're still pretending that you have the interests of _free_ open source *as you once had*.
Did you review my driver? Did anyone else in this thread? No, of course not. You didn't even do it the second time I called your bluff in this thread. Why not? Simple, it's not what you're getting paid to do now.
Can you imagine Greg pulling out a defunct driver written by a co-worker of yours, (working for the same large corporation as you are), in favor of some no-name working driver? I mean imagine the sponsorships, the free equipment, etc your company provides to you, Greg and the "Linux community"? Your company is the single largest sponsor of Linux conferences and anything Linux.
After all, who do you think got Greg a UAS device? (not me ;-) )
Luben
--- On Mon, 12/13/10, Matthias Schniedermeyer <[email protected]> wrote:
> As a future user of UASP (The protocol) i'm happy to spend
> a few more
> weeks with UMS if it means the driver for UASP is really
> ready for
> transferring TBs of data when released.
> (Altough i'm guessing that i will spend some more time with
> UMS anyway,
> as neither the packaging nor the website for my USB 3.0
> enclosures
> mention UASP)
Yes, it is ready. If you have a UAS device, give it a try. The uasp.c driver is in the free-linux branch off of master in my github repo. See the announcement here:
http://marc.info/?l=linux-scsi&m=129227488508299&w=2
Luben
I was just chilling down late in the evening by reading a short LKML
overview, and then inadvertently managed to hit this bomb shell ;)
> As uas has not been in a release-kernel, by definition, there is no
> impact either way. No regressions or anything else.
Since I've actually just been oldconfig'ing uas driver yesterday on a new
installation, I can relate to UAS kernel support being quite new
in its entirety.
If this is the case, then I'd also tend to believe the situation
is very different from other more painful (since longstanding) driver conflicts
(e.g. 8139, some raid drivers, and also e100 as witnessed by myself).
The author of UASP might want to adjust some of his writings
(especially the more personal parts),
however several parts of his criticism (e.g. no focus on code review)
seem valid from my POV.
If the new driver indeed is a whole lot better than the other newly
submitted/unreleased driver, then I'd fully welcome a reevaluation of the
situation.
For the author of the original submission this might of course be a
"less than positive" (to put it extreemely mildly) situation.
Still, I'd hope that there could be sufficient agreement on how to proceed
and how to make sure to have the most fitting code get into the kernel
and be maintained properly, ideally by continued interest by _both_
authors (or perhaps via "personality firewalls" ;), obviously.
Alas, I'm feeling like I'm stating obvious blathering here.
Anyway, by now you know which direction I'm tending in, despite the
not terribly warm writings by a certain author ;)
Disclaimer: no code review (comparison) performed, sorry.
Disclaimer #2: as the author of a still-outstanding kernel patch
(uhm - hi Greg ;) I'm in a sufficiently unfavourable position to comment
on this...
Andreas Mohr
On Mon, Dec 13, 2010 at 01:46:58PM -0800, Luben Tuikov wrote:
> --- On Sat, 12/11/10, Alan Cox <[email protected]> wrote:
> >
> > > Alan, don't you work for the same company as do the
> > authors listed in "uas.c"?
> >
> > If you've got nothing better to do than insinuate further
> > unpleasant
> > things when people try and help then don't expect any help
> > from anyone.
> >
> > *plonk*
>
> When in doubt, follow the money.
Luben, let me congratulate you. You are the first person ever to be put
into my killfile. Ever. This is a first, and I have personal e-mail
archives stretching back almost 2 decades. This will be my last message to
you; I would have to look to see if it was the only one.
Your insinuations against the character of many long-standing Linux
developers, based on their refusal to accept your submission, are simply
beyond the pale. I could just as easily make every one of these
accusations against you -- who pays your salary, for example? What are
their interests? You make claims about your motivations, but how am I to
verify any of those?
The linux community works because it is exactly that -- a community. We
have, just like any other community, standards of decorum. You have
violated those standards, repeatedly, despite several people politely
telling you the proper manner in which to behave.
> Did you review my driver? Did anyone else in this thread? No, of course
> not. You didn't even do it the second time I called your bluff in this
> thread. Why not? Simple, it's not what you're getting paid to do now.
*bzzt* Wrong. I did review your driver. I found it interesting, but upon
first submission I found no compelling reason to prefer it over the old.
Your initial submission gave no such reasons.
It was only until, after your submission was refused, that you addressed
the technical merits of your driver over the existing one. I will admit,
you have some valid technical points. But, they are completely overridden
by your inability to work within the standards of behavior of the community.
If, at any time, you had decided to stop using ad-hominem attacks and
instead made a calm, composed, and well-reasoned statement about why the
work to fixup the existing driver would be worse than doing a drop-in
replacement, we would have listened. You came pretty close to doing this,
but seemed completely unable to resist personal attacks against members of
this community. And you never really addressed, beyond a 1-line statement
stating that to fixup the existing driver you would need to replace it
wholesale, why working to improve the existing driver is a bad choice.
We look to submitters to maintain their work. Yes, there are apparently
some difficulties getting Matt W. to push the ball forward on his UAS
driver. However, dealing with you is comparable to drinking napalm; I
would much rather deal with someone who is less-than-responsive rather than
someone who is abrasive.
Matt
--
Matthew Dharm Home: [email protected]
Maintainer, Linux USB Mass Storage Driver
We can customize our colonels.
-- Tux
User Friendly, 12/1/1998
On Mon, Dec 13, 2010 at 10:53:25PM +0100, Andreas Mohr wrote:
> I was just chilling down late in the evening by reading a short LKML
> overview, and then inadvertently managed to hit this bomb shell ;)
>
> > As uas has not been in a release-kernel, by definition, there is no
> > impact either way. No regressions or anything else.
>
> Since I've actually just been oldconfig'ing uas driver yesterday on a new
> installation, I can relate to UAS kernel support being quite new
> in its entirety.
>
> If this is the case, then I'd also tend to believe the situation
> is very different from other more painful (since longstanding) driver conflicts
> (e.g. 8139, some raid drivers, and also e100 as witnessed by myself).
>
> The author of UASP might want to adjust some of his writings
> (especially the more personal parts),
> however several parts of his criticism (e.g. no focus on code review)
> seem valid from my POV.
I think you may be missing some code review on the linux-usb mailing
list. Luben posted his original patch to the uas.c driver there, but he
included behavior changes, code style modification, and variable
renaming in one giant patch. It was very difficult to wade through, so
I asked him to break the changes into a series of patches (perhaps 18,
since he has 18 points?).
I did attempt to give a partial code review of Luben's original patches,
and he has yet to respond to any of my points:
http://marc.info/?l=linux-kernel&m=129021816023869&w=2
I also asked him to change a different patch for his driver, to move a
work-around into a better place:
http://marc.info/?l=linux-kernel&m=129192831709856&w=2
I have yet to see an updated patch set, only personal attacks.
Sarah Sharp
--- On Mon, 12/13/10, Sarah Sharp <[email protected]> wrote:
> I think you may be missing some code review on the
> linux-usb mailing
> list.
There was never a review of uasp.c. Let's be clear on that. Because the way you've written the above sentence, someone may misread it to understand that your co-workers and Greg had written extensive review of uasp and had rejected it based on some tangible virtue. Greg's NAK came 4 minutes after I posted it and I mentioned that in my response (in this thread).
> Luben posted his original patch to the uas.c
> driver there, but he
> included behavior changes, code style modification, and
> variable
> renaming in one giant patch.
I wanted to rip the whole thing out. That patch changed a modest 86% of the code and I did mention that in the patch:
http://marc.info/?l=linux-usb&m=128901883420388&w=2
> It was very difficult to
> wade through,
That's why it's better to rip the whole thing out and put in something properly done.
> so
> I asked him to break the changes into a series of patches
> (perhaps 18,
> since he has 18 points?).
I'm not going to generate 18+ patches that changes 100% of that driver into a completely different and working driver.
First, I've no time for this. Second, it's much better, quicker and error-proof to present a whole _new_ piece (and show how it's done), moreover the fact that there are no UAS devices out there at the moment.
You and anyone else can certainly contribute to uasp.c. What's the point in converting the defunct uas.c to look like the working uasp.c (two different drivers)? The proper thing is to pull uas.c out and put uasp.c in (working and properly implemented (see my original editorial and patches to uas.c)).
> I did attempt to give a partial code review of Luben's
> original patches,
> and he has yet to respond to any of my points:
>
> http://marc.info/?l=linux-kernel&m=129021816023869&w=2
Now that you asked, I just did respond to this. The reason I didn't respond to it before is because I didn't think there was any point in responding then, as I do now. Back then I would've said: the whole thing should be ripped out and re-written. Now I'd say: refer to uasp.c on how it's done. It's available on github.
> I also asked him to change a different patch for his
> driver, to move a
> work-around into a better place:
>
> http://marc.info/?l=linux-kernel&m=129192831709856&w=2
And I responded to this in the next entry in the thread in the same link. The parameter still has value. Most likely it wouldn't have to be set (but it still has value in the very very rare case...).
> I have yet to see an updated patch set, only personal
> attacks.
A patch set for what? UAS? See uasp.c on how it's done. For the host that mis-calculates the number of streams it reports in HCCPARAMS? I still haven't had time to plug in a newer version of this HC. When I do and if it still misbehaves, I'll send you a dump of the PCI config space and you can decide what you want to do.
Luben
--- On Mon, 12/13/10, Matthew Dharm <[email protected]> wrote:
> Your insinuations against the character of many
> long-standing Linux
> developers, based on their refusal to accept your
> submission,
No, not submission. The fact is that no one bothered to review the code. It was simply flat out NAK-ed it in just four minutes, fronting one of their co-workers with the defunct driver already in.
It would've been a different story if Greg, Alan and you and anyone else willing, had provided a technical review, and /then/ reject it--the rejection reason would've been clearer to everyone, than taking all this thread to get to.
Greg didn't review it and rejected it 4 minutes after posting. Alan didn't review it and after I posted the 18 technical reasons (my 2nd post in this thread replying to Greg's) said this:
> Your driver may be the best on the planet - who knows -
Obviously he didn't review it.
You, after I posted the driver and the 18 technical reasons, said this:
> If, in fact, Luben's driver is somehow "better", ...
and
> ... several things would have to be true.? I do not know if they are true here, ...
and
> That said, unless the current driver is really bad, and Luben's is tremendously better,
These are from this very thread.
> are simply
> beyond the pale.? I could just as easily make every
> one of these
> accusations against you -- who pays your salary, for
> example?? What are
> their interests?? You make claims about your
> motivations, but how am I to
> verify any of those?
I've already stated that there had never been an agenda about uasp.c and Linux. The uasp.c driver was written over most of a Saturday (when I had time and uas.c showed itself to be defunct).? Since uasp.c is a working driver and correctly implemented in terms of UAS, USB, SCSI, the kernel and it's layers, I thought it would be *helpful* and *useful* to people--that's all.
The driver was rejected flat out only after 4 minutes after submission claiming that there is a driver in the kernel that "does the exact same thing".? I then provided 18 technical reasons as to why uas.c does NOT in fact do the exact same thing to which Greg responded with:
> But again, this driver is not acceptable as-is, sorry.
Why would he have to apologize? Anyway, to which you responded with not knowing which driver is good.
> The linux community works because it is exactly that -- a
> community.? We
> have, just like any other community, standards of
> decorum.? You have
> violated those standards, repeatedly, despite several
> people politely
> telling you the proper manner in which to behave.
When people read "the community" responses in these mailing lists, they think "Wow what a benevolent bunch of people" and "look, they just wanna help". But what this mailing list doesn't show is that this is a very close knit of people who work for the same company (or two) and who go to Linux conferences religiously and are very close buddies. Obviously uasp.c didn't stand a chance _and_ was rejected in just 4 minutes, without a technical review. As you can see in this thread the three of you kept fronting the uas.c driver despite the 18 technical reasons I posted and the fact that it's badly written, and needs to be replaced with something that works, as people will find out when UAS devices make it to market. It's all in the beginning of this thread, start reading here:
http://marc.info/?t=129165521000011&r=2&w=2
So after such a blatant refusal to even give a technical review of the driver, it had me thinking: why would that be?
> > Did you review my driver? Did anyone else in this
> thread? No, of course
> > not. You didn't even do it the second time I called
> your bluff in this
> > thread. Why not? Simple, it's not what you're getting
> paid to do now.
>
> *bzzt*? Wrong.? I did review your driver.? I
> found it interesting, but upon
> first submission I found no compelling reason to prefer it
> over the old.
> Your initial submission gave no such reasons.
If you reviewed it, then why did you write that you had no idea if the driver were good or not? Here is what you wrote in this very thread:
> If, in fact, Luben's driver is somehow "better", ...
and
> ... several things would have to be true.? I do not know if they are true here, ...
and
> That said, unless the current driver is really bad, and Luben's is tremendously better,
Your full message can be found here: http://marc.info/?l=linux-kernel&m=129186163519728&w=2.
But now you say that you had reviewed it!? Now you say you found the driver "interesting" but "no compelling reason to prefer it over the old". How about that it works? How about the 18 technical reasons I posted *before* you posted your response quoted above (which reasons you had read)?? You didn't address any of them.
> It was only until, after your submission was refused,
It was flat out refused only 4 minutes after I submitted it.
> that
> you addressed
> the technical merits of your driver over the existing
> one.
The reason I posted the 18 technical reasons, is because Greg claimed that uas.c "does the exact same thing". His response is here: http://marc.info/?l=linux-usb&m=129165544600441&w=2, he posted this only 4 minutes after I posted the driver. I therefore decided to clarify that uas.c does NOT in fact do the "exact same thing", and backed that up with the 18 technical points.
> I will admit,
> you have some valid technical points.? But, they are
> completely overridden
> by your inability to work within the standards of behavior
> of the community.
What does this actually mean? I just posted my uasp.c driver in this very thread and it got rejected 4 minutes afterwards without technical review. When I followed up with a technical review you say "your inability to work within the standards of behavior of the community".
Does providing no technical review and flat out rejecting a driver fall into your "inability to work within the standards of behavior of the community" definition?
> If, at any time, you had decided to stop using ad-hominem
> attacks and
> instead made a calm, composed, and well-reasoned statement
> about why the
> work to fixup the existing driver would be worse than doing
1. The whole thing should be ripped out. Any good work would change 100% of the driver as uasp.c shows (essentially getting a new driver in).
2. I did make that "statement" in the 18 technical reasons of why uasp.c was better than uas.c, other than "it works" of course.
> a drop-in
> replacement, we would have listened.
I did. In the 18 technical reasons I listed in this very thread. Here is the link: http://marc.info/?l=linux-kernel&m=129185378712222&w=2
> You came pretty
> close to doing this,
> but seemed completely unable to resist personal attacks
> against members of
> this community.
And those are? I think you're priming up here what people should think.
> And you never really addressed,
> beyond a 1-line statement
> stating that to fixup the existing driver you would need to
> replace it
> wholesale, why working to improve the existing driver is a
> bad choice.
I said that in the 18 technical reasons I listed, link above.? The existing driver is badly written, the infrastructure is just off.
> We look to submitters to maintain their work.? Yes,
> there are apparently
> some difficulties getting Matt W. to push the ball forward
> on his UAS
> driver.
It's simple, rip out uas.c and put in uasp.c and give people something working and let everyone contribute to something with good foundation.
> However, dealing with you is comparable to
> drinking napalm; I
> would much rather deal with someone who is
> less-than-responsive rather than
> someone who is abrasive.
Actually, it's very easy to deal with me. When people come to me, I say "What can I do to help you?" and then I provide them a solution and always go the extra mile, and give them something constructive they can use, and I say, "here it is, and I did this extra thing and let me know what else I can do for you."? Most of the time I don't wait for people to come to ask me, but proactively contribute to the end goal, surprising people with "it's already done" when they ask for it.
???Luben
On Tue, 14 Dec 2010 09:25:12 -0800 (PST), Luben Tuikov <[email protected]> wrote:
> --- On Mon, 12/13/10, Matthew Dharm <[email protected]> wrote:
> > Your insinuations against the character of many
> > long-standing Linux
> > developers, based on their refusal to accept your
> > submission,
>
> No, not submission. The fact is that no one bothered to review the
> code. It was simply flat out NAK-ed it in just four minutes, fronting
> one of their co-workers with the defunct driver already in.
>
> It would've been a different story if Greg, Alan and you and anyone
> else willing, had provided a technical review, and /then/ reject
> it--the rejection reason would've been clearer to everyone, than
> taking all this thread to get to.
>
> Greg didn't review it and rejected it 4 minutes after posting. Alan
> didn't review it and after I posted the 18 technical reasons (my 2nd
> post in this thread replying to Greg's) said this:
>
> > Your driver may be the best on the planet - who knows -
> Obviously he didn't review it.
This is may be true, perhaps he didn't look at the code. This is not
because he doesn't have confidence that your driver is well-written or
holds a grudge against you. The reason for this NAK is policy: a driver
already exists. In the kernel we work with what already exists, even if
it may be more difficult than the alternative.
I looked at your code; from my uninformed perspective it looks very
nice. You clearly do have a good understanding of the stack and your
code looks refreshingly clean. That being said, a driver covering the
same devices already exists in the tree. There may be good technical
reasons why this driver is inferior, but these are merely reasons to fix
the existing code, not rip it out to be replaced wholesale.
At this point you have two options: You can continue to argue that the
world is against you, which as we seen makes neither side happy and only
makes your life as a contributor harder, or you can try to work what you
have into something that will fit with the existing codebase. I suggest
the latter. Further arguing is not going to change this policy. That
being said, your expertise is certainly needed and it is in everyone's
best interest to ensure that the UAS driver in the kernel is the best
that it can be.
> I've already stated that there had never been an agenda about uasp.c
> and Linux. The uasp.c driver was written over most of a Saturday (when
> I had time and uas.c showed itself to be defunct). Since uasp.c is a
> working driver and correctly implemented in terms of UAS, USB, SCSI,
> the kernel and it's layers, I thought it would be *helpful* and
> *useful* to people--that's all.
>
Improving code is very useful. History has taught us that flat-out
rewriting components just makes things messier.
> The driver was rejected flat out only after 4 minutes after submission
> claiming that there is a driver in the kernel that "does the exact
> same thing". I then provided 18 technical reasons as to why uas.c
> does NOT in fact do the exact same thing to which Greg responded with:
>
> > But again, this driver is not acceptable as-is, sorry.
>
> Why would he have to apologize? Anyway, to which you responded with
> not knowing which driver is good.
>
He apologized because he knows you put a lot of time, effort, and
thought into a piece of code which we cannot accept. It is unfortunate
that this is the case but such is the reality of large-scale software
development.
> So after such a blatant refusal to even give a technical review of the
> driver, it had me thinking: why would that be?
>
Easy question. See above. In short, there is no sense to give a
technical review of code which we ultimately won't be able use.
Sometimes this sort of thing happens. If I were you, I would accept what
people have been telling you and try to integrate portions of your
patch into the existing UAS driver. This will cause the least pain/most
benefit to all involved.
- Ben
--- On Tue, 12/14/10, Ben Gamari <[email protected]> wrote:
> > > Your driver may be the best on the planet - who
> knows -
> > Obviously he didn't review it.
>
> This is may be true, perhaps he didn't look at the code.
"may be true". It is clear he didn't. What he wrote is quoted above.
> This is not
> because he doesn't have confidence that your driver is
> well-written or
> holds a grudge against you.
Have you been authorized to speak for him?
> The reason for this NAK is
> policy: a driver
> already exists. In the kernel we work with what already
> exists, even if
> it may be more difficult than the alternative.
Nothing can be further from the truth. If you have been long enough around Linux, you'd know that many times there would be two perfectly working drivers for the same device with hundreds of thousands of those devices out in the wild, in the kernel. You'd also know that sometimes a driver is submitted and accepted and in the kernel, but it is so badly written that it gets re-written by one of the resident "linux kernel engineers" and then the latter driver replaces the one already existing, etc, etc. Add to this that it is sometimes good to offer people a choice between a defunct and unmaintained driver and a working and maintained driver.
Now is the time to do the right thing and get a working and technically correct UAS driver, when there are no UAS devices out there.
> I looked at your code; from my uninformed perspective it
> looks very
> nice. You clearly do have a good understanding of the stack
> and your
> code looks refreshingly clean. That being said, a driver
> covering the
> same devices already exists in the tree. There may be good
> technical
> reasons why this driver is inferior, but these are merely
> reasons to fix
> the existing code, not rip it out to be replaced
> wholesale.
It's much easier, faster and foolproof to rip out a defunct driver than to submit 30+ patches that change 100% of the driver. Moreover when there are no devices for it yet.
> At this point you have two options: You can continue to
> argue that the
> world is against you,
I don't argue that nor do I think that.
> which as we seen makes neither side
> happy and only
> makes your life as a contributor harder, or you can try to
> work what you
> have into something that will fit with the existing
> codebase. I suggest
> the latter. Further arguing is not going to change this
> policy. That
> being said, your expertise is certainly needed and it is in
> everyone's
> best interest to ensure that the UAS driver in the kernel
> is the best
> that it can be.
The situation is as follows: the Linux band is more than happy to review patches to /their/ code, but when you submit a driver that replaces a defunct driver of theirs in the kernel, then all of a sudden no one can read code to give a technical review of your driver. So they say "we're happy to accept patches to /our/ code even if it replaces 100% of it, but your driver which does this in one go is rejected, sorry" and "I've no idea if it is good or bad".
> Improving code is very useful.
Improving code can be useful, but not to the extent of replacing it 100% in one go in 30+ patches.
A patch is supposed to fix a single bug or a feature missed, etc. For example see my recent patches posted as well as examine git history, as well as examine my patches from before there was git (starting with the SCSI layer using the slab cache for struct scsi_cmnd allocation).
> History has taught us that
> flat-out
> rewriting components just makes things messier.
Nothing can be further from the truth. This isn't a whimsical rewrite. I tried to "fix" uas.c but it's just horrible and needs to be ripped out. Just one patch of mine changed 86% of the code and that's NOT right. But with that defunct driver, there was just no other way. The terminology the driver uses show the writer is not familiar with the technology. The logistical flow of the code shows the writer isn't familiar with the way a SCSI Delivery Subsystem works, UAS and USB in general. I've outlined that in my 18 technical point review earlier in this thread.
The right thing was to write a UAS driver that is correct and that it works, thus uasp.c.
> He apologized because he knows you put a lot of time,
> effort, and
Again, you speak for other people. This isn't a good thing.
> thought into a piece of code which we cannot accept.
"we"? See when you write emails here, and pretend to be this benevolent selfless creature lacking all self-interest, you have to be more subtle and present an unbiased opinion. Using "we" isn't the way to do it.
> It is
> unfortunate
> that this is the case but such is the reality of
> large-scale software
> development.
uasp.c is 1141 lines, a featherweight in drivers class. There is no "large-scale software development". uasp.c is self-contained driver. It doesn't change or modify any of the rest of the kernel. So now you can see how there is NO "large-scale software development". It doesn't affect the system other than when uasp.c is used people will notice that they can now use a UAS device.
> Easy question. See above. In short, there is no sense to
> give a
> technical review of code which we ultimately won't be able
> use.
You said it: "ultimately won't be able to use". Basically a decision has already been made. And here you have shown the problem: the biased opinion of the Linux band: you're welcome to improve /our/ code even it you completely replace it in one go in 30+ patches, but we're not accepting your driver, under any circumstances. How about give people a choice?
> Sometimes this sort of thing happens. If I were you, I
> would accept what
> people have been telling you and try to integrate portions
> of your
People? You mean a two people who have known each other for over 10 years are close knit friends who work for the same company whose company sponsors their conferences and gives them equipment.
> patch into the existing UAS driver. This will cause the
> least pain/most
> benefit to all involved.
The least pain/most benefit is to replace the defunct uas.c with uasp.c, so people can test the UAS devices they are developing and when those devices are released on the market, people can use them. See here: http://marc.info/?l=linux-scsi&m=129227488508299&w=2
Luben
Hi,
I had preferred to subsequently calmly stay on the sidelines
after trying to argue for a beneficial compromise,
but given this lack of realism I'm choosing to reply again:
> On Tue, 14 Dec 2010 09:25:12 -0800 (PST), Luben Tuikov
> <[email protected]> wrote:
> > --- On Mon, 12/13/10, Matthew Dharm <[email protected]>
> > wrote:> > Your driver may be the best on the planet - who knows -
> > Obviously he didn't review it.
>
> This is may be true, perhaps he didn't look at the code. This is not
> because he doesn't have confidence that your driver is well-written or
> holds a grudge against you. The reason for this NAK is policy: a driver
> already exists. In the kernel we work with what already exists, even if
> it may be more difficult than the alternative.
One word: HOG-WASH. At least the e100 vs. eepro100 case is a clear
counter-example to that. That was an eepro100 driver which has been in active
mainline service for years and had many diagnostic features,
to then be replaced (one could argue that it _may_ be better indeed
to replace a possibly less-maintained driver with a better-maintained one
by the original hardware manufacturer).
The result, even from my tiny involvement (one card where I had lots
of trouble getting a make-it-work-again kind of patch into e100 - one
YEAR to have mainline actually support the hardware again),
is known as several hardware variants failing to work with the new driver
(and I just found another hardware example in the very first semi-related
Google search I did, so there must have been many more).
Not sure about current status, but I'm nevertheless very positive that it's much better now.
Witness this very problematic case as opposed to the current conflict case
where there's a _new_ hardware/driver which has been hardly even used by anyone,
to potentially be replaced by a (supposedly?) much better driver
which has almost identical amount of history (read: _weeks_ or at most months,
and both have not even ever been released yet).
Guys, please argue healthily and don't keep making up straw-man
arguments (as I'm tending to believe when reading some parts of the
discussion).
The way I see it there is this (likely incomplete) list of reasons to
prefer the "old", "challenged"(?) new driver:
. there's the "old", "timely submitted"
driver, and the entire thing may be an understandably much-less-than-positive situation for its author
. the author of the new one is a d**k to argue with
(may easily be true given several cases of his awfully "personal" arguing,
but in his position facing such often unbased opposition
other people might have lost some temper, too),
causing uncertainty about proper continued maintenance of the driver
. for some certain reason, it's had "more testing", its situation is "better", ...
. suddenly accepting an entirely different driver after another can
legitimately be seen as some kind of unhealthy disruption of the
development advancement
. at this point in time we've lost enough additional time arguing
that a change of drivers might now really be inconvenient
from a kernel release continuity/planning POV.
Let me tell you that I'm somewhat unconvinced of this, though.
Perhaps at this time the only thing to be said is that due to "policy" (yeah, whatever)
a more timely submitted driver will remain in and the supposedly better
driver will stay out, but given the history of Linux kernel drivers this
is more than a bit unconvincing, as outlined above.
Of course one could argue that this policy simply is quite _new_
and didn't exist yet at the time of the other conflicts (e100, 8139, RAID adapters),
and possibly has been established exactly _due_ to the many difficulties that turned
up in these cases.
However it's still questionable whether it's appropriate
to apply such a policy _in this case_
given that _both_ candidates aren't entrenched (in active service) at all.
Still, I hope that even with a "negative" decision the best elements of the
drivers will eventually find their way into a combined driver,
with appropriate amounts of maintenance.
Andreas Mohr
--- On Wed, 12/15/10, Andreas Mohr <[email protected]> wrote:
> Hi,
>
> I had preferred to subsequently calmly stay on the
> sidelines
> after trying to argue for a beneficial compromise,
> but given this lack of realism I'm choosing to reply
> again:
>
> Ben Gamari wrote this:
> > This is may be true, perhaps he didn't look at the
> code. This is not
> > because he doesn't have confidence that your driver is
> well-written or
> > holds a grudge against you. The reason for this NAK is
> policy: a driver
> > already exists. In the kernel we work with what
> already exists, even if
> > it may be more difficult than the alternative.
>
> One word: HOG-WASH. At least the e100 vs. eepro100 case is
> a clear
Andreas, the way you've cut out the quote above, it seems that you're replying to something I've written, when in fact you're replying to Ben Gamari.
Above, I've corrected the quote, not to confuse people of course.
> counter-example to that. That was an eepro100 driver which
> has been in active
> mainline service for years and had many diagnostic
> features,
> to then be replaced (one could argue that it _may_ be
> better indeed
> to replace a possibly less-maintained driver with a
> better-maintained one
> by the original hardware manufacturer).
So eepro100.c was written by Don (as was rtl8139.c) and was replaced by e100.c written by its manufacturer (same place where uas.c is coming from)? Interesting...
As I recall rtl8139.c was also replaced by 8139too.c after many years with many devices in the field, instead of "dividing the code into patches for the driver which is already in the kernel and submitting it for review". The author of 8139too.c used to submit patches to rtl8139.c (as can be seen in the source of that driver) but then decided to write his own 8139too.c and submit it and it got accepted and now that's the only driver in the kernel for that chip. The copyright comment of 8139too.c says this.
Hmm... Interesting and insightful stuff. So who are those people that can rewrite whole drivers and get theirs in and the old ones out?
> The way I see it there is this (likely incomplete) list of
> reasons to
> prefer the "old", "challenged"(?) new driver:
> . there's the "old", "timely submitted"
> ? driver, and the entire thing may be an
> understandably much-less-than-positive situation for its
> author
> . the author of the new one is a d**k to argue with
> ? (may easily be true given several cases of his
> awfully "personal" arguing,
> ???but in his position facing such often
> unbased opposition
> ???other people might have lost some temper,
> too),
> ? causing uncertainty about proper continued
> maintenance of the driver
Within the last two months, I've submitted more patches to uasp.c (4-5) than the original author to uas.c (none). As well as I've submitted patches to uas.c itself (three, first two accepted, third rejected since it changed 86% of the uas.c code), lsusb, sd.c, block/blk-tag.c, etc.
Also, uasp.c is a protocol driver. The protocol will change a bit but not by that much. When this happens I'll submit new patches to update it, unless someone submits them before that. Even if no patches are submitted to support latter version of the protocol, UAS devices will be backward compatible.
In effect, this driver doesn't warrant much maintenance by virtue of its nature, but support is there if need be.
> . for some certain reason, it's had "more testing", its
> situation is "better", ...
"more testing" -- do you mean uasp.c or uas.c? If you have a UAS device, try both drivers (do I/O) and see for yourself. uas.c also doesn't support the UAS protocol by definition as it doesn't support TMF and other features present in uasp.c.
> . suddenly accepting an entirely different driver after
> another can
> ? legitimately be seen as some kind of unhealthy
> disruption of the
> development advancement
> . at this point in time we've lost enough additional time
> arguing
> ? that a change of drivers might now really be
> inconvenient
> ? from a kernel release continuity/planning POV.
> ? Let me tell you that I'm somewhat unconvinced of
> this, though.
>
> Perhaps at this time the only thing to be said is that due
> to "policy" (yeah, whatever)
> a more timely submitted driver will remain in and the
> supposedly better
> driver will stay out, but given the history of Linux kernel
> drivers this
> is more than a bit unconvincing, as outlined above.
> Of course one could argue that this policy simply is quite
> _new_
> and didn't exist yet at the time of the other conflicts
> (e100, 8139, RAID adapters),
> and possibly has been established exactly _due_ to the many
> difficulties that turned
> up in these cases.
> However it's still questionable whether it's appropriate
> to apply such a policy _in this case_
> given that _both_ candidates aren't entrenched (in active
> service) at all.
>
>
> Still, I hope that even with a "negative" decision the best
> elements of the
> drivers will eventually find their way into a combined
> driver,
> with appropriate amounts of maintenance.
You make some very good points, however it's a Goliath vs. David situation but this time David loses. While this may seem similar to rtl8139.c vs. 8139too.c and eepro100.c vs. e100.c, the difference is that the backing behind 8139too.c and e100.c is with with uas.c. Working code or the quality of the code is irrelevant here. uasp.c got rejected 4 minutes after submission.
uasp.c wouldn't even make it in the kernel alongside uas.c, to give people a choice at least. (and we've heard the excuse for that one before, while there had been two drivers for the same device in the kernel before)
In the immediate future distros may include uasp.c since it provides UAS support. Independent Linux vendors may do the same for the same reason. HDD/ASIC manufacturers may use uasp.c to test their UAS devices (an alternative to the Windows tools).
In the future though, I expect that someone will study uasp.c and submit patches to uas.c and eventually you'll see uas.c look exactly as uasp.c does now (sans the credits, similarly to what happened to the SAS code five years ago).
However, people are free to use whatever they want.
If you have a UAS device and would like to play with it, uasp.c is available on github: http://marc.info/?t=129227496000002&r=1&w=2
Luben
On Thu, Dec 16, 2010 at 01:46:50AM -0800, Luben Tuikov wrote:
> Andreas, the way you've cut out the quote above, it seems that you're replying to something I've written, when in fact you're replying to Ben Gamari.
Indeed, I had already realized that; that was a victim of me
_manually_ replying to external archive content in the case of LKML
(while preserving proper In-Reply-To ID).
> Hmm... Interesting and insightful stuff. So who are those people that can rewrite whole drivers and get theirs in and the old ones out?
<rhetorical question> :-P
I....
would like to say that I'm not really certain about this ;)
> Within the last two months, I've submitted more patches to uasp.c (4-5) than the original author to uas.c (none). As well as I've submitted patches to uas.c itself (three, first two accepted, third rejected since it changed 86% of the uas.c code), lsusb, sd.c, block/blk-tag.c, etc.
"two months". So it's probably a longer time than what I thought ("weeks"),
which probably is a large part of the reason why it got rejected
outright without its fair share of chance of review,
within the current kernel release interval.
Still, it's then not entirely PC if some people - admittedly people
other than the ones whose actual business it is/was to reject drivers :)) -
then claim that the reason the driver got rejected is firm "policy"
to always reject conflicting drivers.
> Also, uasp.c is a protocol driver. The protocol will change a bit but not by that much. When this happens I'll submit new patches to update it, unless someone submits them before that. Even if no patches are submitted to support latter version of the protocol, UAS devices will be backward compatible.
>
> In effect, this driver doesn't warrant much maintenance by virtue of its nature, but support is there if need be.
*noted*
> > . for some certain reason, it's had "more testing", its
> > situation is "better", ...
>
> "more testing" -- do you mean uasp.c or uas.c? If you have a UAS device, try both drivers (do I/O) and see for yourself. uas.c also doesn't support the UAS protocol by definition as it doesn't support TMF and other features present in uasp.c.
Since I indicated this compiled list to be about the "original" submission, it's hopefully obvious.
> uasp.c wouldn't even make it in the kernel alongside uas.c, to give people a choice at least. (and we've heard the excuse for that one before, while there had been two drivers for the same device in the kernel before)
The situation of two drivers in direct conflict can get messy indeed
(especially if people don't know which they are using),
thus I can easily understand that.
> In the immediate future distros may include uasp.c since it provides UAS support. Independent Linux vendors may do the same for the same reason. HDD/ASIC manufacturers may use uasp.c to test their UAS devices (an alternative to the Windows tools).
And that's one thing that I'm worrying about.
Let's say one does the "proper", "good" way to achieve driver
improvements:
have the earlier submission get into released kernels
and then submit patches to it in (let's say) third-steps
to have it get to what a driver with combined
features/functionality/stability would provide.
This would mean to have an original driver implementation in kernels,
which would not last long, get updated significantly ("third-steps" above),
then re-released until we ultimately hit the full combined and
full-featured driver in further kernel releases.
Depending on how important API / user space stability/architecture
effects may become, we could IMNSUTVHO see a situation where it can get
moderately problematic on which distribution / version is using which
driver development version.
All this as opposed to doing a (admittedly not really painless) emergency-yank now
and cleanly releasing with UASP as the _first_ driver version (which probably
is much less likely to change drastically) to ever hit a kernel release.
> If you have a UAS device and would like to play with it, uasp.c is available on github: http://marc.info/?t=129227496000002&r=1&w=2
Given current mass-market unavailability of hardware (as indicated in
the discussion): sorry, nope.
Andreas Mohr