RE: Regression caused by: usb: add a flag to skip PHY initialization to struct usb_hcd
> > Admittedly without really understanding everything that is going on, I put > back the > deleted lines from this patch chunk and it started working again: > > diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c index > 19d60ed..af45aa32 100644 > --- a/drivers/usb/chipidea/host.c > +++ b/drivers/usb/chipidea/host.c > @@ -124,10 +124,8 @@ static int host_start(struct ci_hdrc *ci) > > hcd->power_budget = ci->platdata->power_budget; > hcd->tpl_support = ci->platdata->tpl_support; > - if (ci->phy) > - hcd->phy = ci->phy; > - else > - hcd->usb_phy = ci->usb_phy; > + if (ci->phy || ci->usb_phy) > + hcd->skip_phy_initialization = 1; > > ehci = hcd_to_ehci(hcd); > ehci->caps = ci->hw_bank.cap; > > Without a value in hcd->usb_phy, the call to usb_phy_notify_disconnect() in > hub_port_connect() (usb/core/hub.c) never fires but that is only part of the > problem. > Hope this helps. > Thanks, Mat. I posted a patch for this fix, help to test please. Peter
[PATCH 1/1] usb: chipidea: host: fix disconnection detect issue
The commit 4e88d4c08301 ("usb: add a flag to skip PHY
initialization to struct usb_hcd") delete the assignment
for hcd->usb_phy, it causes usb_phy_notify_connect{disconnect)
are not called, the USB PHY driver is not notified of hot plug
event, then the disconnection will not be detected by hardware.
Fixes: 4e88d4c08301 ("usb: add a flag to skip PHY initialization
to struct usb_hcd")
Cc: Martin Blumenstingl
Reported-by: Mats Karrman
Signed-off-by: Peter Chen
---
drivers/usb/chipidea/host.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index af45aa3222b5..4638d9b066be 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -124,8 +124,11 @@ static int host_start(struct ci_hdrc *ci)
hcd->power_budget = ci->platdata->power_budget;
hcd->tpl_support = ci->platdata->tpl_support;
- if (ci->phy || ci->usb_phy)
+ if (ci->phy || ci->usb_phy) {
hcd->skip_phy_initialization = 1;
+ if (ci->usb_phy)
+ hcd->usb_phy = ci->usb_phy;
+ }
ehci = hcd_to_ehci(hcd);
ehci->caps = ci->hw_bank.cap;
--
2.14.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Regression caused by: usb: add a flag to skip PHY initialization to struct usb_hcd
On 2018-06-05 02:54, Peter Chen wrote:
And this is what the "decompiled" device tree entry for the USB
controller and phy look like:
usb@2184200 {
compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
reg = <0x2184200 0x200>;
interrupts = <0x0 0x28 0x4>;
clocks = <0x4 0xa2>;
fsl,usbphy = <0x2c>;
fsl,usbmisc = <0x29 0x1>;
dr_mode = "host";
ahb-burst-config = <0x0>;
tx-burst-size-dword = <0x10>;
rx-burst-size-dword = <0x10>;
status = "okay";
disable-over-current;
vbus-supply = <0x2d>;
};
usbphy@20ca000 {
compatible = "fsl,imx6q-usbphy", "fsl,imx23-usbphy";
reg = <0x20ca000 0x1000>;
interrupts = <0x0 0x2d 0x4>;
clocks = <0x4 0xb7>;
fsl,anatop = <0x2>;
phandle = <0x2c>;
};
So, using deprecated? "fsl,usbphy" instead of "phys", in case that matters.
It is ok.
Check two things:
- ci->usb_phy is non-NULL, and ci->phy is NULL
That is correct
- phy_roothub is NULL at the functions of drivers/usb/core/phy.c
I put a trace at the beginning of each of the functions of that file but none
of them is
ever called.
It is so strange. Please double confirm your git bisect is correct, if it is,
try to find which
line causes your regression.
Bisect confirmed.
Admittedly without really understanding everything that is going on, I put
back the deleted lines from this patch chunk and it started working again:
diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index 19d60ed..af45aa32 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -124,10 +124,8 @@ static int host_start(struct ci_hdrc *ci)
hcd->power_budget = ci->platdata->power_budget;
hcd->tpl_support = ci->platdata->tpl_support;
- if (ci->phy)
- hcd->phy = ci->phy;
- else
- hcd->usb_phy = ci->usb_phy;
+ if (ci->phy || ci->usb_phy)
+ hcd->skip_phy_initialization = 1;
ehci = hcd_to_ehci(hcd);
ehci->caps = ci->hw_bank.cap;
Without a value in hcd->usb_phy, the call to usb_phy_notify_disconnect()
in hub_port_connect() (usb/core/hub.c) never fires but that is only part of the
problem. Hope this helps.
BR // Mats
Re: External USB fuzzing
On Tue, 5 Jun 2018, Andrey Konovalov wrote: > On Mon, Jun 4, 2018 at 9:37 PM, Alan Stern wrote: > > On Mon, 4 Jun 2018, Andrey Konovalov wrote: > > Hi, Alan! Hi! > [...] > > >> The perfect solution would be to have something like /dev/tun for USB, > >> where you can write USB packets and the kernel would synchronously > >> process them. I'm not sure whether this is possible (highly > >> asynchronous USB core subsystem architecture + all communication is > >> initiated by the host). > > > > gadgetfs is a little like that already, except of course that the > > processing is not synchronous. > > Yes, I started with GadgetFS initially, but actually ended up writing > my own interface that provides userspace interface to the gadget API > instead. Once I figure out what exactly I need from it, it might be a > good idea to merge it with GadgetFS as some special mode. Or not, not > sure yet. What do you need that's different from what gadgetfs provides? > > Another aspect of this would be fuzzing USB host controller drivers. > > In theory you could start doing this now, although you would have to > > use a real UDC instead of dummy-hcd. However, this may create more > > complications that you want to deal with at the moment. > > Could you elaborate on this? AFAIU the dummy device contains both a > virtual UDC and a virtual HCD that are connected to each other and > allow to plug a gadget device into usb core within the kernel. How do > you test a driver for an actual host controller? Is there a way to > connect a dummy UDC with an actual HCD? You can't connect a dummy UDC to an actual HCD. But you can connect an actual UDC to an actual HCD -- people do it all the time. Normally the UDC and the HCD reside on separate computers. For example, mobile devices typically have UDCs, and you can connect them to PCs. But there is also UDC hardware available for Linux on Intel systems, so you can have the UDC and the HCD in the same computer if that's what you want. Or two separate PCs, both running Linux. Or even a Linux PC gadget connected to a Windows or OS X host! ... > The main question is: suppose we started a test program that emulates > a virtual USB device. How do we know that the test is over? That depends on what you are testing. > Once all > the hub events are processed we can tell that the device is probably > either connected successfully or failed to connect. But then the > device driver can start it's own thread for example or register a > timer that does more work with the device. Right. For example, usb-storage uses a work-queue routine for SCSI scanning when a new device is probed. And in that routine, the SCSI layer often delegates some of the scanning work to async helper threads, depending on the how the system is configured. > There are probably some > ways to tell what the device driver does at this moment for some > standard USB classes. Yes, I imagine so. But it will be different for each driver. > I guess I'll start with a timeout for now, until I figure out how to > properly collect coverage. That's probably the best thing to do. ... > > In fact, dummy-hcd is capable of creating more than one virtual root > > hub (see the "num" module parameter), so in theory you could run > > multiple tests at the same time. I don't think this capability has > > been tested very much. > > OK, that's nice. I just tried it and it kind of works, but there are two > issues: > > 1. There's a hardcoded limit on the maximum number of virtual UDCs > (#define MAX_NUM_UDC 2). Increasing this parameters seems to work just > fine. > > 2. All the virtual UDCs get the same name "dummy_udc". And since the > process of UDC lookup to bind a gadget driver is based on the UDC name > (see usb_gadget_probe_driver()), we can't bind different gadget > drivers to different UDCs (and therefore connect to different hubs). That can be changed pretty easily. For virtual UDCs beyond the first, we could append "-2", "-3", and so on to the "dummy_udc" name, for example. > >> Basically we need a way to run a test program (that connects a USB > >> device and works with it from userspace for example) and then destroy > >> all the accumulated state before running the next test. > > > > You can do that with dummy-hcd simply by removing the USB gadget > > (closing the gadgetfs files, for example). > > Do I understand correctly that calling usb_gadget_unregister_driver() > should get rid of all of the device state? (That's what gadgetfs seems > to do when you close the file). It should. If it doesn't, there's a bug in the UDC driver or UDC core. > > If you want to be extra > > thorough, unload the gadget driver and dummy-hcd modules after the end > > of the test, and then reload them before starting the next test. That > > should get rid of any important state. > > What state is kept if we don't reload the module? There shouldn't be anything important. Perhaps just some trivial leftovers (like
Re: External USB fuzzing
On Mon, Jun 4, 2018 at 9:37 PM, Alan Stern wrote: > On Mon, 4 Jun 2018, Andrey Konovalov wrote: Hi, Alan! [...] >> The perfect solution would be to have something like /dev/tun for USB, >> where you can write USB packets and the kernel would synchronously >> process them. I'm not sure whether this is possible (highly >> asynchronous USB core subsystem architecture + all communication is >> initiated by the host). > > gadgetfs is a little like that already, except of course that the > processing is not synchronous. Yes, I started with GadgetFS initially, but actually ended up writing my own interface that provides userspace interface to the gadget API instead. Once I figure out what exactly I need from it, it might be a good idea to merge it with GadgetFS as some special mode. Or not, not sure yet. > > Another aspect of this would be fuzzing USB host controller drivers. > In theory you could start doing this now, although you would have to > use a real UDC instead of dummy-hcd. However, this may create more > complications that you want to deal with at the moment. Could you elaborate on this? AFAIU the dummy device contains both a virtual UDC and a virtual HCD that are connected to each other and allow to plug a gadget device into usb core within the kernel. How do you test a driver for an actual host controller? Is there a way to connect a dummy UDC with an actual HCD? > >> Another thing we could do is to teach kcov to collect coverage from >> arbitrary kernel threads. In this case we could collect coverage from >> the thread that processes the USB events. > > That seems like a more general solution, something which could be > applied to other settings too. One difficulty you would face is > telling the tool which threads to work on. This is particularly tricky > when dealing with work queues, since a single work-queue thread can run > code for many different drivers and purposes. > > Perhaps it would be better to collect coverage information based on the > location of the code being executed rather than the thread ID. Yes, that is what I was thinking about. I have a prototype patch for kcov that allows to annotate a block of the kernel code and collect coverage from that block. I think this is the way I'll try to move forward with. > >> However we would still need >> some signal to understand whether the device finished initialization, >> etc. (with the first approach we could tell that some stage of device >> initialization is finished by seeing that hub_event() returned). > > Why initialization particularly? A device goes through a number of > stages in its life cycle. > > I get the impression you would like to have a mechanism whereby you can > send messages from arbitrary places in the kernel to your testing > process. A little like the function tracer or perf, perhaps. Yes, we need to see all the stages, initialization was just an example. The main question is: suppose we started a test program that emulates a virtual USB device. How do we know that the test is over? Once all the hub events are processed we can tell that the device is probably either connected successfully or failed to connect. But then the device driver can start it's own thread for example or register a timer that does more work with the device. There are probably some ways to tell what the device driver does at this moment for some standard USB classes. I guess I'll start with a timeout for now, until I figure out how to properly collect coverage. [...] >> 2. A way to isolate independent test programs. >> >> Right now CONFIG_USB_DUMMY_HCD creates one global hub to which all >> emulated USB devices connect. Would it be possible to create one >> virtual hub per test (or at least one per test process) by say calling >> some ioctl? What changes would that require? > > Are you planning to run multiple test processes concurrently? If you > aren't then this isn't an issue. Yes, I believe that would increase fuzzing speed. > > In fact, dummy-hcd is capable of creating more than one virtual root > hub (see the "num" module parameter), so in theory you could run > multiple tests at the same time. I don't think this capability has > been tested very much. OK, that's nice. I just tried it and it kind of works, but there are two issues: 1. There's a hardcoded limit on the maximum number of virtual UDCs (#define MAX_NUM_UDC 2). Increasing this parameters seems to work just fine. 2. All the virtual UDCs get the same name "dummy_udc". And since the process of UDC lookup to bind a gadget driver is based on the UDC name (see usb_gadget_probe_driver()), we can't bind different gadget drivers to different UDCs (and therefore connect to different hubs). > >> Basically we need a way to run a test program (that connects a USB >> device and works with it from userspace for example) and then destroy >> all the accumulated state before running the next test. > > You can do that with dummy-hcd simply by removing the USB gadget
Re: Fwd: usb: uas: device reset most the time while enumeration- usb3.0
Hi, On 2018-06-04 19:57, Oliver Neukum wrote: Am Dienstag, den 29.05.2018, 14:32 +0530 schrieb Tushar Nimkar: Hi, On 2018-05-28 18:12, Oliver Neukum wrote: > Am Donnerstag, den 24.05.2018, 20:13 +0530 schrieb Tushar Nimkar: > > We have built SCSI as module will it cause any problem to enable > > CONFIG_SCSI_SCAN_ASYNC ? > > No, that should work. But the failure is bizzare. You say yes there is no problem! I have run some test over night. > synchronous scanning fails, but async scan works? yes! Odd. Extremely odd. Does this show on all architectures? I have tried with different SOC version with same architecture(ARM)and Synopsys host controller. I am asking because that is the crucial question. I see two possibilities 1. the sync probing code has a bug that shows only on some architectures 2. architectures were a coincidence - the drive is broken We absolutely need to know what is happening. Please let me know if I could contribute on this issue. I would always like to work on it :) I am afraid this will have to be tested on another architecture. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- Best Regards, Tushar R Nimkar QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
