Received: by 2002:a05:6a10:f347:0:0:0:0 with SMTP id d7csp3605356pxu; Mon, 30 Nov 2020 06:44:05 -0800 (PST) X-Google-Smtp-Source: ABdhPJylyY6dGBEfmBmS4LjLkv6jlvgEF4Ub9PuoJqcpup9fcoHG1XmGuq0npebu5F6+1LLwliVS X-Received: by 2002:aa7:cdd3:: with SMTP id h19mr23084594edw.330.1606747445639; Mon, 30 Nov 2020 06:44:05 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1606747445; cv=none; d=google.com; s=arc-20160816; b=DA1srAHvIDcgatPEo9hsu2pjRrYQy0TY/8mNy4YS/EyUts94AoaxhL1dvG0IIHQJCX SG2kfr6GY5rwOUVXrLcXCcUvdBtSOmupFBiCr44CH59mZVrPnmrMH+g0Uu69vAI8/17F YUHrLaPbglg5pfeQtEohGLQyd/Nd+Elf2/OIuLq3pjr1eNXcCxhFA9sg/HSvE5pVOo9C WqocfOETQhUeoLVo68DQCSzLr9BKKWMEmr1JZUE3SaLPyjiJkX1hZmqFVmxA+x10zGNL tjffc2ThX0t0mONSYrfY5wTOp3MZftNSI+xzDdZU4jMKDKbJuyXFfncaeiu1nH54baMx ipKA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:subject:message-id:date:from:in-reply-to :references:mime-version:dkim-signature; bh=JM65qVWHe/D3jqo1oAErAS5mplJkY3vB0A9hWCwSLIE=; b=zG3yQulJX2VNdWhJh5Fjizxm5RIbbA8Jan+ugftkTnDnclsmBSQ4ugquv1DLVYlk+d 9nRgHJrnui3zw6aJ1oiqd8720vCKi+4HAU93RfMvs7+depb04UtNSZSVqfXp85p/QKdc N8xN9kjTfCpzaPIfKENc81lyvprgg+QDTzDCkRXno6lpcfJWqnLIRYJXnF/7LQI/bKhF Yp6astK4cU+dolwLgqKsb6gEU4OixeRMCjqG11gPoaAfubytvlOJIHYU8k1jKg0vGLn2 TtK3YuUjHJMyM5R6NAFkBQOhs7imXL7f0S2XBnLbAQ5aTfF0hTh6E4bR4f+S3MTaLYOI GYHA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=ZDxilJ5H; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id x18si12042163ejd.397.2020.11.30.06.43.42; Mon, 30 Nov 2020 06:44:05 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=ZDxilJ5H; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727199AbgK3Okx (ORCPT + 99 others); Mon, 30 Nov 2020 09:40:53 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49122 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726103AbgK3Okw (ORCPT ); Mon, 30 Nov 2020 09:40:52 -0500 Received: from mail-ej1-x642.google.com (mail-ej1-x642.google.com [IPv6:2a00:1450:4864:20::642]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id CE35BC0613CF; Mon, 30 Nov 2020 06:40:11 -0800 (PST) Received: by mail-ej1-x642.google.com with SMTP id f9so20044593ejw.4; Mon, 30 Nov 2020 06:40:11 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=JM65qVWHe/D3jqo1oAErAS5mplJkY3vB0A9hWCwSLIE=; b=ZDxilJ5H+NWCTSRBaHN4e2Xxpg2H1Esz2x/qwj0fYu5/+xeOCXtAA7h2mlHlsKRV7/ 587biUBHiQgoefo71N0/WWnVTtLZ3fIQxrq1+00PyuLzP2rR74s++C0WOuhO+ydZ3yJM 3AvAo4GkTK+7x1c9rPE83XLItVjfpreX4i80ARRSb0bxqasHpnza5JwIYcTByzbGL3bO 7h8dbqXZe+vqE1QJyw6MovPcWwGTfe+BaksipSGERipBohK3Z1LduS3Jxkxx4IH+kPTN HYAL/AoPd0eBzlJEd5UhfmgVIZu7VDAn3L8YhJWU3NwT0d8+AJQbM05c/CMpnLrHRcxY pcHQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=JM65qVWHe/D3jqo1oAErAS5mplJkY3vB0A9hWCwSLIE=; b=HN9m3nPZJoYZZFaUhZiScGXhZ5g77t+HIqIQAJadWVssbEg7FocZVHXcmhOhuHQZh6 OiHQrT4tiuoQVC3Vmlvmoapy268UH2VRM01OSMhiTAIbpVt28uwrxN44oe+i/plfcNdf hh5JbS/uhQlCAmPXHkBjFFPRkA9L3ITyb8NCDeTnwfyuJfMF29AMj8zBVKLXNDridIsz 6VI5k/fnd+F3CyJ8Xlxp3gNxO0q27wuRD9eqtz34CB2uuehIQm9GoCGhsNwr1CLeSFDX 8ski0FM/rWdlVOnoZ2KwQOXJRbbVOS7O7+ejQz526BhX+Wc4NRof5RyRhb4KnfsdLew2 8vBw== X-Gm-Message-State: AOAM533952xm6sUtcPObG8XdVN053poTGaLT1UfqMzSnzOaDIEAnHrwf Gtx15QG/D1bQgv55qCZk91XdE32nyxn4RA0/Rs4= X-Received: by 2002:a17:906:a195:: with SMTP id s21mr21312326ejy.146.1606747210492; Mon, 30 Nov 2020 06:40:10 -0800 (PST) MIME-Version: 1.0 References: <09992cec-65e4-2757-aae6-8fb02a42f961@redhat.com> <20201128154849.3193-1-tom.ty89@gmail.com> <20201128154849.3193-2-tom.ty89@gmail.com> <5e62c383-22ea-6df6-5acc-5e9f381d4632@redhat.com> <186eb035-4bc4-ff72-ee41-aeb6d81888e3@redhat.com> In-Reply-To: <186eb035-4bc4-ff72-ee41-aeb6d81888e3@redhat.com> From: Tom Yan Date: Mon, 30 Nov 2020 22:39:58 +0800 Message-ID: Subject: Re: [PATCH 2/2] usb-storage: revert from scsi_add_host_with_dma() to scsi_add_host() To: Hans de Goede Cc: Alan Stern , Christoph Hellwig , Greg KH , linux-usb , Mathias Nyman , Linux Kernel Mailing List , linux-pci@vger.kernel.org, Lu Baolu Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 30 Nov 2020 at 21:23, Hans de Goede wrote: > > Hi, > > On 11/30/20 1:58 PM, Tom Yan wrote: > > 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. They are not "other/new" changes. The same thing was done in the earlier version of the problematic commits, before I was given the idea that we can/should set dma_dev to the "chosen device". With the dma_dev setting approach the exact same clamping will be applied twice at different points so we don't have to invalidate the earlier one. But now since we no longer do so, the two clamping are / can be different, so we need to invalidate the earlier one when we are not overriding the default max_sectors in each case. > > p.s. Why did you not send this patch-series to Alan Stern, the maintainer of > the usb-storage driver? Either I accidentally missed him or the list I copied from did that already. Sorry. > > > Similar has been done in the equivalent > > patch for the UAS driver (and the reason is stated there). > > In the UAS driver the code setting max-hw-sectors was already moved to its > new place and another patch was added on top, so that is different. If you are referring to the alloc/configure move in the problematic commit, it was a trivial / code consistency change. It has nothing to do with what I'm talking about. What I'm talking about is the else-clause add in the first patch of the current series. I don't know if you simply missed it or it just seemed much trivial to you, either way it was "simple" there merely because the uas driver doesn't set its own "default" max_sectors (and hence has no comment for it) but use the one set in the SCSI layer. Most importantly, it's an *adapation* that makes these patches change *nothing* from the current behaviour but only switches back to scsi_add_host(). > > Regards, > > Hans > > > > > > On Mon, 30 Nov 2020 at 17:50, Hans de Goede wrote: > >> > >> Hi, > >> > >> On 11/28/20 4:48 PM, Tom Yan wrote: > >>> While the change only seemed to have caused issue on uas drives, we > >>> probably want to avoid it in the usb-storage driver as well, until > >>> we are sure it is the right thing to do. > >>> > >>> Signed-off-by: Tom Yan > >> > >> This seems to do a whole lot more then just dropping back from > >> scsi_add_host_with_dma() to scsi_add_host(). This has way more > >> lines then the orginal commit. > >> > >> IMHO it would be best to just revert commit 0154012f8018bba4d9971d1007c12ffd48539ddb > >> and then submit these changes as a separate patch (which would be > >> 5.11 material then). > >> > >> That separate patch could then also have a proper commit message > >> explaining the other changes you are making, which is also not > >> unimportant. > >> > >> Regards, > >> > >> Hans > >> > >> > >> > >> > >>> --- > >>> drivers/usb/storage/scsiglue.c | 40 +++++++++++++++++----------------- > >>> drivers/usb/storage/usb.c | 3 +-- > >>> 2 files changed, 21 insertions(+), 22 deletions(-) > >>> > >>> diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c > >>> index 560efd1479ba..6539bae1c188 100644 > >>> --- a/drivers/usb/storage/scsiglue.c > >>> +++ b/drivers/usb/storage/scsiglue.c > >>> @@ -92,7 +92,7 @@ static int slave_alloc (struct scsi_device *sdev) > >>> static int slave_configure(struct scsi_device *sdev) > >>> { > >>> struct us_data *us = host_to_us(sdev->host); > >>> - struct device *dev = sdev->host->dma_dev; > >>> + struct device *dev = us->pusb_dev->bus->sysdev; > >>> > >>> /* > >>> * Many devices have trouble transferring more than 32KB at a time, > >>> @@ -120,6 +120,25 @@ static int slave_configure(struct scsi_device *sdev) > >>> * better throughput on most devices. > >>> */ > >>> blk_queue_max_hw_sectors(sdev->request_queue, 2048); > >>> + } else { > >>> + /* > >>> + * Limit the total size of a transfer to 120 KB. > >>> + * > >>> + * Some devices are known to choke with anything larger. It seems like > >>> + * the problem stems from the fact that original IDE controllers had > >>> + * only an 8-bit register to hold the number of sectors in one transfer > >>> + * and even those couldn't handle a full 256 sectors. > >>> + * > >>> + * Because we want to make sure we interoperate with as many devices as > >>> + * possible, we will maintain a 240 sector transfer size limit for USB > >>> + * Mass Storage devices. > >>> + * > >>> + * Tests show that other operating have similar limits with Microsoft > >>> + * Windows 7 limiting transfers to 128 sectors for both USB2 and USB3 > >>> + * and Apple Mac OS X 10.11 limiting transfers to 256 sectors for USB2 > >>> + * and 2048 for USB3 devices. > >>> + */ > >>> + blk_queue_max_hw_sectors(sdev->request_queue, 240); > >>> } > >>> > >>> /* > >>> @@ -627,25 +646,6 @@ static const struct scsi_host_template usb_stor_host_template = { > >>> .sg_tablesize = SG_MAX_SEGMENTS, > >>> > >>> > >>> - /* > >>> - * Limit the total size of a transfer to 120 KB. > >>> - * > >>> - * Some devices are known to choke with anything larger. It seems like > >>> - * the problem stems from the fact that original IDE controllers had > >>> - * only an 8-bit register to hold the number of sectors in one transfer > >>> - * and even those couldn't handle a full 256 sectors. > >>> - * > >>> - * Because we want to make sure we interoperate with as many devices as > >>> - * possible, we will maintain a 240 sector transfer size limit for USB > >>> - * Mass Storage devices. > >>> - * > >>> - * Tests show that other operating have similar limits with Microsoft > >>> - * Windows 7 limiting transfers to 128 sectors for both USB2 and USB3 > >>> - * and Apple Mac OS X 10.11 limiting transfers to 256 sectors for USB2 > >>> - * and 2048 for USB3 devices. > >>> - */ > >>> - .max_sectors = 240, > >>> - > >>> /* emulated HBA */ > >>> .emulated = 1, > >>> > >>> diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c > >>> index c2ef367cf257..f177da4ff1bc 100644 > >>> --- a/drivers/usb/storage/usb.c > >>> +++ b/drivers/usb/storage/usb.c > >>> @@ -1050,8 +1050,7 @@ int usb_stor_probe2(struct us_data *us) > >>> usb_autopm_get_interface_no_resume(us->pusb_intf); > >>> snprintf(us->scsi_name, sizeof(us->scsi_name), "usb-storage %s", > >>> dev_name(dev)); > >>> - result = scsi_add_host_with_dma(us_to_host(us), dev, > >>> - us->pusb_dev->bus->sysdev); > >>> + result = scsi_add_host(us_to_host(us), dev); > >>> if (result) { > >>> dev_warn(dev, > >>> "Unable to add the scsi host\n"); > >>> > >> > > >