Fwd: usb: uas: device reset most the time while enumeration- usb3.0

[Tushar Nimkar <tnimkar@codeaurora.org>]

Hi All,

On 2018-04-24 14:42, Oliver Neukum wrote:
> Am Montag, den 23.04.2018, 18:35 +0530 schrieb Tushar Nimkar:
>> hi,
>> 
>> On 2018-04-23 18:20, Oliver Neukum wrote:
>> > Am Donnerstag, den 19.04.2018, 20:18 +0530 schrieb Tushar Nimkar:
> 
>> > No. But for testing we don't need to. Can you confirm that
>> > the problem is triggered by specific commands?
>> 
>> Observed with REPORT_LUN or READ_CAP16 or INQUIRY command.
> 
> Well, that is not the same thing. It is possible that these commands
> generally fail, but x86_64 has a working error handling but ARM does
> not.
> 
>> Are you planning to test it with dwc3?
> 
> No, I am sorry. I would need to acquire some very specific hardware.
> 
> 	Regards
> 		Oliver
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Please have look on the proposed fix for the slow enumeration and reset 
issue
for UAS devices(after SD_TIMEOUT).
Fix for :
...
[  123.672358] usb 2-1: new SuperSpeed USB device number 2 using 
xhci-hcd
[  123.711940] scsi host0: uas
[  123.867378] scsi 0:0:0:0: Direct-Access     Samsung  Portable SSD T3  
0    PQ: 0 ANSI: 6
[  155.132319] sd 0:0:0:0: tag#1 uas_eh_abort_handler 0 uas-tag 2 
inflight: CMD IN
[  155.132359] sd 0:0:0:0: tag#1 CDB: opcode=0x9e, sa=0x10 9e 10 00 00 
00 00 00 00 00 00 00 00 00 20 00 00
[  155.138868] scsi host0: uas_eh_device_reset_handler start
...

If changes seems fine will push this patch to upstream : )

Issue is mostly occurring while we issue the READ_CAPACITY_16 or 
REPORT_LUNS or INQ commands.
I have confirm this with USB analyzer too. But issue is not in those 
commands(they are supported)
Also which mean there is no problem in scanning lun with REPORT_LUN or 
older sequential scan,
which I was suspecting the issue cause.
Those commands issued from different layers say: sd.. uas.. scsi..
so making them to go one after other. Once REPORT_LUN done go with 
READ_CAPACITY_16.
This is only for the UAS devices. I believe no disturb to BOT behavior.

so far tested with two UAS SSDs  dwc3 as host controller with 3.00a as 
version
	1) StoreJet TS256GESD400K
	2) Samsung  Portable SSD T3

Draft Patch :
---
  drivers/scsi/sd.c              | 8 ++++++++
  drivers/usb/storage/scsiglue.c | 1 +
  drivers/usb/storage/uas.c      | 6 ++++++
  include/scsi/scsi_host.h       | 6 ++++++
  4 files changed, 21 insertions(+)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 78430ef..38c23ad 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2979,6 +2979,14 @@ static void sd_probe_async(void *data, 
async_cookie_t cookie)
  	struct device *dev;

  	sdp = sdkp->device;
+
+	/*
+	 *  Continue async probe once host scanning is finished
+	 *  this is only for uas devices
+	 */
+	if (sdp->host->hostt->is_uas)
+		wait_for_completion_interruptible_timeout(&sdp->host->hscan_done,
+						msecs_to_jiffies(500));
  	gd = sdkp->disk;
  	index = sdkp->index;
  	dev = &sdp->sdev_gendev;
diff --git a/drivers/usb/storage/scsiglue.c 
b/drivers/usb/storage/scsiglue.c
index dba5136..fbd397a 100644
--- a/drivers/usb/storage/scsiglue.c
+++ b/drivers/usb/storage/scsiglue.c
@@ -594,6 +594,7 @@ void usb_stor_host_template_init(struct 
scsi_host_template *sht,
  	sht->name = name;
  	sht->proc_name = name;
  	sht->module = owner;
+	sht->is_uas = false;    /* BOT device */
  }
  EXPORT_SYMBOL_GPL(usb_stor_host_template_init);

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index f952635..ead73d0 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -44,6 +44,7 @@ struct uas_dev_info {
  	unsigned use_streams:1;
  	unsigned shutdown:1;
  	struct scsi_cmnd *cmnd[MAX_CMNDS];
+	struct Scsi_Host *shost;
  	spinlock_t lock;
  	struct work_struct work;
  };
@@ -828,6 +829,7 @@ static struct scsi_host_template uas_host_template = 
{
  	.this_id = -1,
  	.sg_tablesize = SG_NONE,
  	.skip_settle_delay = 1,
+	.is_uas = true,
  };

  #define UNUSUAL_DEV(id_vendor, id_product, bcdDeviceMin, bcdDeviceMax, 
\
@@ -934,11 +936,13 @@ static int uas_probe(struct usb_interface *intf, 
const struct usb_device_id *id)
  	devinfo->resetting = 0;
  	devinfo->shutdown = 0;
  	devinfo->flags = dev_flags;
+	devinfo->shost = shost;
  	init_usb_anchor(&devinfo->cmd_urbs);
  	init_usb_anchor(&devinfo->sense_urbs);
  	init_usb_anchor(&devinfo->data_urbs);
  	spin_lock_init(&devinfo->lock);
  	INIT_WORK(&devinfo->work, uas_do_work);
+	init_completion(&shost->hscan_done);

  	result = uas_configure_endpoints(devinfo);
  	if (result)
@@ -956,6 +960,8 @@ static int uas_probe(struct usb_interface *intf, 
const struct usb_device_id *id)
  		goto free_streams;

  	scsi_scan_host(shost);
+	/* once scsi_host_scan done, let others to do their stuffs */
+	complete(&shost->hscan_done);
  	return result;

  free_streams:
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index fcfa3d7..b7d27a8 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -498,6 +498,9 @@ struct scsi_host_template {

  	/* temporary flag to disable blk-mq I/O path */
  	bool disable_blk_mq;
+
+	/* let the host know  the device is BOT or UAS */
+	bool is_uas;
  };

  /*
@@ -564,6 +567,9 @@ struct Scsi_Host {
  	struct scsi_host_template *hostt;
  	struct scsi_transport_template *transportt;

+	/* tell other's that host scan is done */
+	struct completion	hscan_done;
+
  	/*
  	 * Area to keep a shared tag map (if needed, will be
  	 * NULL if not).