lkml.org 
[lkml]   [2020]   [Nov]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 2/2] usb-storage: revert from scsi_add_host_with_dma() to scsi_add_host()
    From
    Date
    Hi,

    On 11/30/20 6:20 PM, Alan Stern wrote:
    > On Mon, Nov 30, 2020 at 02:36:38PM +0100, Hans de Goede wrote:
    >> Hi,
    >>
    >> On 11/30/20 2:30 PM, Greg KH wrote:
    >>> On Mon, Nov 30, 2020 at 02:23:48PM +0100, Hans de Goede wrote:
    >>>> Hi,
    >>>>
    >>>> On 11/30/20 1:58 PM, Tom Yan wrote:
    >>>>> It's merely a moving of comment moving for/and a no-behavioral-change
    >>>>> adaptation for the reversion.>
    >>>>
    >>>> IMHO the revert of the troublesome commit and the other/new changes really
    >>>> should be 2 separate commits. But I will let Alan and Greg have the final
    >>>> verdict on this.
    >>>
    >>> I would prefer to just revert the commits and not do anything
    >>> different/special here so late in the release cycle.
    >>>
    >>> So, if Alan agrees, I'll be glad to do them on my end, I just need the
    >>> commit ids for them.
    >>
    >> The troublesome commit are (in reverse, so revert, order):
    >>
    >> 5df7ef7d32fe ("uas: bump hw_max_sectors to 2048 blocks for SS or faster drives")
    >> 558033c2828f ("uas: fix sdev->host->dma_dev")
    >> 0154012f8018 ("usb-storage: fix sdev->host->dma_dev")
    >>
    >> Alan, the reason for reverting these is that using scsi_add_host_with_dma() as the
    >> last 2 patches do, with the dmadev argument of that call pointing to the device
    >> for the XHCI controller is causing changes to the DMA settings of the XHCI controller
    >> itself which is causing regressions in 5.10, see this email thread:
    >>
    >> https://lore.kernel.org/linux-usb/fde7e11f-5dfc-8348-c134-a21cb1116285@redhat.com/T/#t
    >
    > It's hard to go wrong with reverting, so it's okay with me.
    >
    > Still, Hans, have you checked out the difference between the
    > scsi_add_host() and scsi_add_host_with_dma() calls? It's just a matter
    > of using dev vs. sysdev. In particular, have you checked to see what
    > those two devices are on your system?

    Its not just dev vs sysdev, its iface->dev vs bus->sysdev, and I assume
    that the latter is actually the XHCI controller.

    my vote goes to reverting to avoid the regression for 5.10, esp. since
    this is a clean revert of 3 patches with nothing depending / building
    on top of the reverted commits.

    Then for 5.11 we can retry to introduce similar changes. I would be happy
    to try a new patch-set for 5.11.

    > It seems likely that if one of those calls messes up some DMA settings,
    > the other one does too -- just maybe not settings that matter much.

    I'm not very familiar with all the DMA mapping / mask code, but AFAIK making
    changes to the DMA settings of a child will not influence the parent.

    Where as when passing bus->sysdev, then changes are made to a device
    which is shared with other devices on the bus, which is why we see
    a regression in an USB NIC driver being triggered by the UAS driver
    binding to a device (on the same bus).

    At least that is my interpretation of this. I bisected the regression
    and that pointed at the UAS DMA change and reverting it fixes things,
    confirming that I did not make any mistakes during the bisect.

    Regards,

    Hans

    \
     
     \ /
      Last update: 2020-11-30 19:21    [W:4.046 / U:0.176 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site