With uas over usb-3 the tags inside the uas iu-s must match the usb-3 stream
ids, and those go from 1 - qdepth.
Before blk-mq calling scsi_activate_tcq(sdev, qdepth) guaranteed that we would
only get cmnd->request->tag from 0 - (qdepth - 1), and we used those as
uas-tags / stream-ids.
With blk-mq however we are guaranteed to never get more then qdepth commands
queued at the same time, but the cmnd->request->tag values may be much larger,
which breaks uas.
This commit fixes this by generating uas tags in the 1 - qdepth range ourselves
instead of using cmnd->request->tag.
While touching all involved code anyways also rename the uas_cmd_info stream
field to uas_tag, because when using uas over usb-2 streams are not used.
Cc: Christoph Hellwig <hch@infradead.org>
Reported-by: Douglas Gilbert <dgilbert@interlog.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
--
Changes in v2:
-Remove ".disable_blk_mq = true" from uas_host_template
Changes in v3:
-Rebased on top of Linus' current master branch
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Here's the big USB patchset for 3.18-rc1. Also in here is the PHY tree,
as it seems to fit well with the USB tree for various reasons...
Anyway, lots of little changes in here, all over the place, full details
in the changelog below.
All have been in the linux-next tree for a while with no issues.
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
iEYEABECAAYFAlQ0aLYACgkQMUfUDdst+ylBvwCgs9fGRj0RQkLyGhQdEpzdZtTU
ZcwAoMPBImnaA1ZeSl7ZnoO8vC/WE4bR
=tfpj
-----END PGP SIGNATURE-----
Merge tag 'usb-3.18-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb
Pull USB updates from Greg KH:
"Here's the big USB patchset for 3.18-rc1. Also in here is the PHY
tree, as it seems to fit well with the USB tree for various reasons...
Anyway, lots of little changes in here, all over the place, full
details in the changelog
All have been in the linux-next tree for a while with no issues"
* tag 'usb-3.18-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb: (244 commits)
USB: host: st: fix typo 'CONFIG_USB_EHCI_HCD_ST'
uas: Reduce number of function arguments for uas_alloc_foo functions
xhci: Allow xHCI drivers to be built as separate modules
xhci: Export symbols used by host-controller drivers
xhci: Check for XHCI_COMP_MODE_QUIRK when disabling D3cold
xhci: Introduce xhci_init_driver()
usb: hcd: add generic PHY support
usb: rename phy to usb_phy in HCD
usb: gadget: uvc: fix up uvcg_v4l2_get_unmapped_area typo
USB: host: st: fix ehci/ohci driver selection
usb: host: ehci-exynos: Remove unnecessary usb-phy support
usb: core: return -ENOTSUPP for all targeted hosts
USB: Remove .owner field for driver
usb: core: log higher level message on malformed LANGID descriptor
usb: Add LED triggers for USB activity
usb: Rename usb-common.c
usb: gadget: Refactor request completion
usb: gadget: Introduce usb_gadget_giveback_request()
usb: dwc2/gadget: move phy bus legth initialization
phy: remove .owner field for drivers using module_platform_driver
...
The stream_id and pipe are already present in uas_cmd_info resp uas_dev_info,
so there is no need to pass a copy along.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
The uas driver uses the block layer tag for USB3 stream IDs. With
blk-mq we can get larger tag numbers that the queue depth, which breaks
this assumption. A fix is under way for 3.18, but sits on top of
large changes so can't easily be backported. Set the disable_blk_mq
path so that a uas device can't easily crash the system when using
blk-mq for SCSI.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Acked-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: James Bottomley <JBottomley@Parallels.com>
If something goes wrong in our communication with an uas device we may get
a response iu in reaction to a cmnd, rather then a status iu. In this case
propagate an error upwards, rather then logging a bogus iu message.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Instead of doing:
uas_log_cmd_state(cmnd, __func__)
scmd_printk(KERN_ERR, cmnd, "error doing foo %d\n", err)
On error, resulting in 2 log calls for a single error, make uas_log_cmd_state
take a status code, and change calls like the above to:
uas_log_cmd_state(cmnd, "error doing foo", err)
Also change various sanity checks (which should never trigger) from:
"scmd_printk(KERN_ERR, cmnd, "sanity foo failed\n")" to calling the new
uas_log_cmd_state(), so that when they do trigger we get more info.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
We've removed all hack from the driver for pre-production hardware.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
I've access to a number of different uas devices now, and none of them use
old style sense urbs. The only case where these code-paths trigger is with
the asm1051 and there they do the wrong thing, as the asm1051 sends 8 bytes
status iu-s when it does not have any sense data, but uses new style
sense iu-s regardless, as can be seen for scsi cmnds where there is sense
data.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
It was only used to sanity check against completing the same cmnd twice,
but that is the case we're likely operating on free-ed memory, and doing
sanity checks on free-ed memory is not really helpful.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Use scsi_print_command to print commands during errors, rather then printing
the rather meaningless pointer to the command.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Check for both type of cancellation codes for sense and data urbs.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Limit the no-streams case to speeds less then USB_SPEED_SUPER.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
The purpose of uas_pre_reset is to:
1) Stop any new commands from being submitted while an externally triggered
usb-device-reset is running
2) Wait for any pending commands to finish before allowing the usb-device-reset
to continue
The purpose of uas_suspend is to:
2) Wait for any pending commands to finish before suspending
This commit fixes races in both paths:
1) For 1) we use scsi_block_requests, but the scsi midlayer calls queuecommand
without holding any locks, so a queuecommand may already past the midlayer
scsi_block_requests checks when we call it, add a check to uas_queuecommand
to fix this
2) For 2) we were waiting for all sense-urbs to complete, there are 2 problems
with this approach:
a) data-urbs may complete after the sense urb, so we need to check for those
too
b) if a sense-urb completes with a iu id of READ/WRITE_READY a command is not
yet done. We submit a new sense-urb immediately in this case, but that
submit may fail (in which case it will get retried by uas_do_work), if this
happens the sense_urbs anchor may become empty while the cmnd is not yet
done
Also unblock requests on timeout, to avoid things getting stuck in that case.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Not all urbs we've allocated are necessarily also submitted, non-submitted
urbs will not be free-ed by their completion handler. So we need to free
them manually.
There are 2 scenarios where this can happen:
1) We have failed to submit some urbs at abort / disconnect
2) When running over usb-2 we may have never tried to submit the data urbs
when completing the scsi cmnd, because we never got a READ/WRITE_READY iu
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Do not keep references around to a cmnd which is under error handling.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
It is not strictly necessary for the cmd urb to have a reference to the
cmnd, and without this reference it becomes easier to drop all references to
a cmnd on an abort.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
We've the same info doubled in both the inflight list and the cmnd array,
drop the list.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
The data urbs are all killed before calling zap_pending, and their completion
handler should have cleared their inflight flag.
Do not 0 the data inflight flags, and add a check for try_complete succeeding,
as it should always succeed when called from zap_pending.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Drop the whole dance with first moving cmnds to a dead-list. The resetting
flag ensures that no new cmds / urbs will be submitted, and that any urb
completions are short-circuited without trying to complete the scsi cmnd.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Now that we no longer drop our lock to unlink the data urbs, we can simply
free them on completion, making their handling consistent with the other urbs.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
There is no need for all the trickery with dropping the lock, we can
simply reference the urbs while we hold the lock to ensure the urbs don't
disappear beneath us, and do the actual unlink (+ unreference) after we've
dropped the lock.
This also fixes a race where we may loose of cmnd ownership to the scsi
midlayer without holding the lock due to the midlayer re-claiming ownership
through an abort (which will be handled by a future patch in this series).
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
The status urb should not complete before the command has been submitted, nor
should we get a second status urb for the same tag after a IU_ID_STATUS.
Data urbs should not complete before the command has been submitted, but may
complete after the IU_ID_STATUS.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Using scsi_host_find_tag with tags returned by the device is unsafe for
multiple reasons:
1) It returns tags->rqs[tag], which may be non NULL even when the cmnd is
not owned by us
2) It returns tags->rqs[tag], without holding any locks protecting it
3) It returns tags->rqs[tag], without doing any boundary checking
Instead keep our own list which maps tags -> inflight cmnds.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Factor out the mapping of scsi-tags -> uas-tags/stream-ids to a helper function
so that there is a single place where this "magic" happens.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
- Make sure we always hold the lock when setting / checking resetting
- Check resetting before checking urb->status
- Add missing check for resetting to uas_data_cmplt
- Add missing check for resetting to uas_do_work
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
There are various bug reports about oopses / hangs with the uas driver,
which all point to the abort-command and logical-unit-reset (task-management)
error handling paths.
Getting these right is very hard, there are quite a few corner cases, and
testing is almost impossible since under normal operation these code paths
are not used at all.
Another problem is that there are also some cases where it simply is not clear
what to do at all. E.g. over usb-2 multiple outstanding commands share the same
endpoint. What if a command gets aborted while its sense urb is half way
through completing (so some data has been transfered but not all). Since the
urb is not yet complete we don't know if the sense urb is actually for this
command, or for one of the other oustanding commands. If it is for one of the
other commands and we cancel it, then we end up in an undefined state. But if
it is actually for the command we're aborting, and the abort succeeds, then it
may never complete...
This exact same problem applies to logical unit resets too, if there are
multiple luns, then commands outstanding on both luns share the sense
endpoint. If there is only a single lun, then doing a logical unit reset is
little better then doing a full usb device reset.
So summarizing because:
1) abort / lun-reset is very tricky to get right
2) Not being able to test the tricky code, which means it will have bugs
3) This being a code path which under normal operation will never happen,
so being slow / sub-optimal here is not really an issue
4) Under error conditions we will still be able to recover through usb
device resets.
5) This may be a bit slower in some cases, but this is actually faster in
cases where the bridge ship has locked up, which seems to be the most
common error case sofar.
This commit removes the abort / lun-reset error handling paths, and also the
taks-mgmt code since those are the only 2 task-mgmt users. Leaving only the
(tested and testable) usb-device-reset error handling path in place.
Note I realize that this is somewhat of a big hammer, but currently people
are seeing very hard to debug oopses with uas. First let focus on making uas
work reliable, then we can later look into adding more fine grained error
handling.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Besides the ASM1051 (*) needing sdev->no_report_opcodes = 1, it turns out that
the JMicron JMS567 also needs it to work properly with uas (usb-storage always
sets it). Since some of the scsi devs were not to keen on the idea to
outrightly set sdev->no_report_opcodes = 1 for all uas devices, so add a quirk
for this, and set it for the JMS567.
*) Which has become a non-issue since we've completely blacklisted uas on
the ASM1051 for other reasons
Cc: stable@vger.kernel.org
Reported-and-tested-by: Claudio Bizzarri <claudio.bizzarri@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
And set this quirk for the Seagate Expansion Desk (0bc2:2312), as that one
seems to hang upon receiving an ATA_12 or ATA_16 command.
https://bugzilla.kernel.org/show_bug.cgi?id=79511https://bbs.archlinux.org/viewtopic.php?id=183190
While at it also add missing documentation for the u value for usb-storage
quirks.
Cc: stable@vger.kernel.org # 3.16, 3.17
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
--
Changes in v2: Add documentation for new t and u usb-storage.quirks flags
Changes in v3: Fix typo in documentation
Changes in v4: Also apply the quirk to (0bc2:3312)
Changes in v5: Rebased on 3.17-rc5, drop u documentation, already upstream
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
on some architecture spin_is_locked() always return false in
uniprocessor configuration and therefore it would be advise
to replace with lockdep_assert_held().
Signed-off-by: Sanjeev Sharma <Sanjeev_Sharma@mentor.com>
Acked-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Some jmicron uas chipsets act up (they disconnect from the bus) when sending
more then 32 commands to them at once.
Rather then building an ever growing list with usb-id based quirks for
devices using this chipset, simply reduce the qdepth to 32 when connected
over usb-2. 32 should be plenty to keep things close to maximum
possible throughput on usb-2.
Cc: stable@vger.kernel.org
Tested-and-reported-by: Laszlo T. <tlacix@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
There are also two allocations with GFP_KERNEL in the pre-/post_reset
code paths. That is no good because that is a part of the SCSI error handler.
Signed-off-by: Oliver Neukum <oliver@neukum.org>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
intfdata is set only after scsi_scan(). uas_pre_reset() however
needs intfdata to be valid and will follow the NULL pointer
killing khubd. intfdata must be preemptively set before the
host is registered and undone in the error case.
Signed-off-by: Oliver Neukum <oliver@neukum.org>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Quote Dan:
The patch e36e64930c: "uas: Use GFP_NOIO rather then GFP_ATOMIC
where possible" from Nov 7, 2013, leads to the following static
checker warning:
drivers/usb/storage/uas.c:806 uas_eh_task_mgmt()
error: scheduling with locks held: 'spin_lock:lock'
Some other allocations under spinlock are not caught.
The fix essentially reverts e36e64930c
Signed-off-by: Oliver Neukum <oliver@neukum.org>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Although an interesting concept, I don't think that this is a good idea:
-This will result in lots of "virtual" scsi controllers confusing users
-If we get a scsi-bus-reset we will now need to do a usb-device-reset of all
uas devices on the same usb bus, which is something to avoid if possible
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>
At the kernel-summit Sarah Sharp asked me if I was willing to become the
uas maintainer. I said yes, and here is a patch to make this official.
Also remove Matthew Wilcox and Sarah Sharp as maintainers at their request.
I've also added myself to the module's author tag, so that if people look there
rather then in maintainers they will know they should bug me about uas too.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>
Copy the sg alignment trick from the usb-storage driver, without this I'm
seeing intermittent errors when using uas devices with an ehci controller.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>
The scsi error handling path re-uses previously queued up (and errored-out)
cmds. If such a re-used cmd had a data-phase then cmdinfo will have
data_in_urb / data_out_urb still set to the free-ed urbs from the errored-out
cmd, and they will get free-ed a second time when the error handling cmd
completes, corrupting the kernel heap.
Clearing cmdinfo on command queue-ing fixes this, and seems like a good idea
in general.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>
The scsi-host structure is refcounted, scsi_remove_host tears down the
scsi-host but does not decrement the refcount, so we need to call
scsi_put_host on disconnect to get the underlying memory to be freed.
After calling scsi_remove_host, the scsi-core may still hold a reference to
the scsi-host, iow we may still get called after uas_disconnect, but we
do our own life cycle management of uas_devinfo, freeing it on disconnect,
and thus may end up using devinfo after it has been freed. Switch to letting
scsi_host_alloc allocate and manage the memory for us.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>
cmds are either on the inflight list or on the dead list, never both, so
we only need one list head.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>
Before this commit the uas driver would keep track of scsi commands which still
need to have some urbs submitted to the device, and complete this with an
ABORT result code on bus-reset or disconnect, but in flight scsi commands
which have all their urbs submitted, and thus are not part of the work list,
would never get their done callback called.
The problem is killed sense urbs don't have any tag info, so it is impossible
to tell which scsi cmd they belong to, so merely making sure all the urbs
have completed one way or the other is not enough.
This commit fixes this by changing the work list to an inflight list, which
keeps tracks of all inflight scsi cmnds, using the IS_IN_WORK_LIST flag to
determine if actual work needs to be done in uas_do_work(), and by moving
marking all inflight scsi commands as aborted and moving them to the dead list
on bus-reset or disconnect.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>
uas_alloc_data_urb always gets called with a stream_id value of 0 when not
using streams. Removing the check makes it consistent with uas_alloc_sense_urb.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>
For USB-2 connections the stream-id must always be 0.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>
Some BIOS-es will hang on reboot when an uas device is attached and left in
uas mode on reboot.
This commit adds a shutdown handler which on reboot puts the device back into
usb-storage mode, fixing the hang on reboot on these systems.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>
