Re: [PATCH v2] usb: uas: quiesce SCSI before stopping endpoints on unbind

[Jiayi Li]

Hi Oliver,

Sorry for the delayed reply, and thank you for the feedback.

> Why this specific trigger?

I initially chose -ENODEV and -ESHUTDOWN to handle reported device
removal or transport shutdown. Earlier errors such as -EPROTO can also
be transient, so I did not include them in the same transport-dead
handling.

However, I agree that we cannot assume an URB will necessarily fail
with -ENODEV during physical disconnection. I think earlier errors
such as -EPROTO may also need additional handling, and I am looking
into this.

> In the context of which work?

I mean the existing per-device retry work, devinfo->work, handled by
uas_do_work().

In the current local candidate, the transport-dead state is set under
devinfo->lock and the existing work item is queued:

static void uas_mark_transport_dead_locked(struct uas_dev_info *devinfo)
{
	lockdep_assert_held(&devinfo->lock);
	if (devinfo->resetting || devinfo->transport_dead)
		return;

	devinfo->transport_dead = 1;
	queue_work(workqueue, &devinfo->work);
}

The relevant part of uas_do_work() is:

	if (devinfo->resetting)
		goto out;
	if (devinfo->transport_dead) {
		spin_unlock_irqrestore(&devinfo->lock, flags);
		usb_kill_anchored_urbs(&devinfo->cmd_urbs);
		usb_kill_anchored_urbs(&devinfo->sense_urbs);
		usb_kill_anchored_urbs(&devinfo->data_urbs);
		uas_zap_pending(devinfo, DID_NO_CONNECT);
		return;
	}

This moves the URB draining into work context, outside devinfo->lock.

> I wonder what is to be done if error handling has already started
> on the SCSI level.

In the current local candidate, after usb_lock_device_for_reset()
succeeds, uas_eh_device_reset_handler() sets resetting and calls
cancel_work_sync(&devinfo->work) before draining the URBs.

I will run more tests with SCSI EH already in progress to check for
races between the cleanup worker and the abort/reset paths.

I am trying to refine the patch.

Thanks,
Jiayi Li