From: Tom Rini Date: Wed, 27 Sep 2017 21:39:51 -0400 Subject: [U-Boot] [U-Boot, 5/5] dm: usb: storage: Fix broken read/write when both EHCD and xHCD are enabled In-Reply-To: References: <1504790001-24715-5-git-send-email-bmeng.cn@gmail.com> <20170927163443.GY3112@bill-the-cat> Message-ID: <20170928013951.GG3112@bill-the-cat> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Thu, Sep 28, 2017 at 09:34:23AM +0800, Bin Meng wrote: > Hi Tom, > > On Thu, Sep 28, 2017 at 12:34 AM, Tom Rini wrote: > > On Thu, Sep 07, 2017 at 06:13:21AM -0700, Bin Meng wrote: > > > >> When EHCD and xHCD are enabled at the same time, USB storage device > >> driver will fail to read/write from/to the storage device attached > >> to the xHCI interface, due to its transfer blocks exceeds the xHCD > >> driver limitation. > >> > >> With driver model, we have an API to get the controller's maximum > >> transfer size and we can use that to determine the storage driver's > >> capability of read/write. > >> > >> Note: the non-DM version driver is still broken with xHCD and the > >> intent here is not to fix the non-DM one, since the xHCD itself is > >> already broken in places like 3.0 hub support, etc. > >> > >> Signed-off-by: Bin Meng > >> Tested-by: Stefan Roese > > [snip] > >> @@ -953,6 +957,17 @@ static void usb_stor_set_max_xfer_blk(struct us_data *us) > >> #else > >> blk = 20; > >> #endif > >> +#else > >> + ret = usb_get_max_xfer_size(udev, (size_t *)&size); > >> + if (ret < 0) { > >> + /* unimplemented, let's use default 20 */ > >> + blk = 20; > >> + } else { > >> + if (size > USHRT_MAX * 512) > >> + blk = USHRT_MAX; > >> + blk = size / 512; > >> + } > >> +#endif > > > > So, Coverity saw this and found an issue (CID 167250), and I was going > > to just fix it, but I'm not sure. The problem is that we check size > > > (USHRT_MAX * 512), and then assign blk. Then, we always assign blk, so > > the test above isn't used. But my background recollection is that > > there's a real issue that's being addressed here. Can we really just > > always say blk = size / 512 in this case, or did we want to be shifting > > size not blk under the if? Thanks! > > Did this patch applied to anywhere? I see it is in the usb tree, but > not in the u-boot/master. > > The fix should be: > > if (size > USHRT_MAX * 512) > size = USHRT_MAX * 512; It's in usb/master, and will be in master itself shortly. Please submit a patch that corrects this and has the Reported-by for Coverity. Marek, do you want to take it via your tree or should I just grab it? Thanks! -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: