From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Date: Fri, 01 May 2015 10:03:42 +0200 Subject: [U-Boot] [PATCH 1/8] dm: usb: Copy over usb_device values from usb_scan_device() to final usb_device In-Reply-To: <55432993.6000006@redhat.com> References: <1430404524-31413-1-git-send-email-hdegoede@redhat.com> <1430404524-31413-2-git-send-email-hdegoede@redhat.com> <55432993.6000006@redhat.com> Message-ID: <5543335E.9080001@redhat.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi, On 01-05-15 09:21, Hans de Goede wrote: > Hi, > > On 01-05-15 06:11, Simon Glass wrote: >> Hi Hans, >> >> On 30 April 2015 at 08:35, Hans de Goede wrote: >>> Currently we copy over a number of usb_device values stored in the on stack >>> struct usb_device probed in usb_scan_device() to the final driver-model managed >>> struct usb_device in usb_child_pre_probe() through usb_device_platdata, and >>> then call usb_select_config() to fill in the rest. >>> >>> There are 2 problems with this approach: >>> >>> 1) It does not fill in enough fields before calling usb_select_config(), >>> specifically it does not fill in ep0's maxpacketsize causing a div by zero >>> exception in the ehci driver. >> >> Didn't you have another patch that fixes that? > > Yes, but as explained in the coverletter of that patch I believe that > this version is better. It would seem we disagree on that though :) > >>> 2) It unnecessarily redoes a number of usb requests making usb probing slower, >>> and potentially upsetting some devices. >> >> Does it actually upset anything? > > Not to my knowledge, but I'm afraid that with some devices it may. > >> The extra requests are in the second call to usb_select_config(). > > Correct. > >> Do you think we could put the things we want to copy in a struct, and >> copy just those? > > We would end up pretty much duplicating usb_device AFAICT, since we need > the descriptors, the max packet sizes per endpoint parsed from them, etc. > > Anyways since you clearly dislike this patch I'll drop it for v2, replacing > it with my original fix for the ep0 maxpacket not being set issue, assuming > that doing so does not cause any regressions during my testing. Ok, so my first test, which is hooking up a sunxi device to my dvi/usb kvm switch fails immediately when I use the maxpacketsize fix instead of my fix to avoid calling get_config twice. This is actually a pretty though test-case as the kvm presents itself + the keyboard and mouse as a complex hub hierarchy with both usb-2 and usb-1 hubs in there, which is what makes it a great test-case. Now I could spend a couple of hours debugging this and maybe find a different fix, but this really shows that there is a reason why all usb stacks do the device probing / descriptor reading all in the exact same sequence, because usb devices are cheap and there qa consists of plug it into $random windows version running machine, does it work? Yes -> ship it. So I really believe that my original fix for this is best. As for trying to pass all the bits we need through platdata rather then passing the struct usb_device itself. I can see value in that as part of a patch-set to get rid of usb_device, iow as part of a larger patchset but until then it just feels like make work to me. So I'm going to stick with my original approach for v1 of this patch. Regards, Hans