linux-kernel - Re: [PATCH 2/2] usb-storage: revert from scsi_add_host_with_dma() to scsi_add

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 1 Dec 2020 03:01:56 +0800
From:   Tom Yan <tom.ty89@...il.com>
To:     Hans de Goede <hdegoede@...hat.com>
Cc:     Alan Stern <stern@...land.harvard.edu>,
        Greg KH <gregkh@...uxfoundation.org>,
        Christoph Hellwig <hch@....de>,
        linux-usb <linux-usb@...r.kernel.org>,
        Mathias Nyman <mathias.nyman@...el.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-pci@...r.kernel.org, Lu Baolu <baolu.lu@...ux.intel.com>
Subject: Re: [PATCH 2/2] usb-storage: revert from scsi_add_host_with_dma() to scsi_add_host()

For the record,
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/scsi/scsi_host.h?h=v5.10-rc6#n753

On Tue, 1 Dec 2020 at 02:57, Tom Yan <tom.ty89@...il.com> wrote:
>
> This maybe? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/scsi_lib.c?h=v5.10-rc6#n1816
>
> UAS:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/storage/uas.c?h=v5.10-rc6#n918
> BOT (AFAICT):
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/hosts.c?h=v5.10-rc6#n466
>
> It would explain why the issue is only triggered with UAS drives.
>
> The questions (from me) are:
> 1. From the scsi layer POV (as per what __scsi_init_queue() does),
> what/which should we use as dma_dev?
> 2. Do we really need to set dma_boundary in the UAS host template (to
> PAGE_SIZE - 1)?
> 3. Kind of the same question as #1: when we clamp hw_max_sectors to
> dma max mapping size, should the size actually be "the smaller one
> among dev and sysdev"? Or is one of the two sizes *always* the smaller
> one?
>
>
> On Tue, 1 Dec 2020 at 02:19, Hans de Goede <hdegoede@...hat.com> wrote:
> >
> > 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
> >