RE: Regression caused by: usb: add a flag to skip PHY initialization to struct usb_hcd

2018-06-04 Thread Peter Chen
 
> >>>
> >> 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.

Peter


Re: [PATCH 1/3] usb: gadget: f_fs: Only return delayed status when len is 0

2018-06-04 Thread Jerry Zhang
Hi Felipe,

I noticed this wasn't queued up for 4.18. Do you think there is
anything I need to do to get this patch set into 4.19? Also, can we at
least add just this patch ('usb: gadget: f_fs: Only return delayed
status when len is 0') to 4.18 as functionfs control requests won't
work without it?

Thanks,

Jerry
On Mon, Apr 16, 2018 at 6:18 PM Jerry Zhang  wrote:
>
> Commit 1b9ba000 ("Allow function drivers to pause control
> transfers") states that USB_GADGET_DELAYED_STATUS is only
> supported if data phase is 0 bytes.
>
> It seems that when the length is not 0 bytes, there is no
> need to explicitly delay the data stage since the transfer
> is not completed until the user responds. However, when the
> length is 0, there is no data stage and the transfer is
> finished once setup() returns, hence there is a need to
> explicitly delay completion.
>
> This manifests as the following bugs:
>
> Prior to 946ef68ad4e4 ('Let setup() return
> USB_GADGET_DELAYED_STATUS'), when setup is 0 bytes, ffs
> would require user to queue a 0 byte request in order to
> clear setup state. However, that 0 byte request was actually
> not needed and would hang and cause errors in other setup
> requests.
>
> After the above commit, 0 byte setups work since the gadget
> now accepts empty queues to ep0 to clear the delay, but all
> other setups hang.
>
> Fixes: 946ef68ad4e4 ("Let setup() return USB_GADGET_DELAYED_STATUS")
> Signed-off-by: Jerry Zhang 
> ---
>  drivers/usb/gadget/function/f_fs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/function/f_fs.c 
> b/drivers/usb/gadget/function/f_fs.c
> index 7d5b1c448eb2..4b2cb9d93176 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -3271,7 +3271,7 @@ static int ffs_func_setup(struct usb_function *f,
> __ffs_event_add(ffs, FUNCTIONFS_SETUP);
> spin_unlock_irqrestore(>ev.waitq.lock, flags);
>
> -   return USB_GADGET_DELAYED_STATUS;
> +   return creq->wLength == 0 ? USB_GADGET_DELAYED_STATUS : 0;
>  }
>
>  static bool ffs_func_req_match(struct usb_function *f,
> --
> 2.17.0.484.g0c8726318c-goog
>
--
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: External USB fuzzing

2018-06-04 Thread Alan Stern
On Mon, 4 Jun 2018, Andrey Konovalov wrote:

> Hi Greg and Alan!
> 
> As you might know I've been working on adding external USB fuzzing
> support to syzkaller.

Yes.  It's an ambitious goal.

> At this point a have a prototype, which is able to emulate USB devices
> from userspace via a custom written userspace interface for the gadget
> subsystem and CONFIG_USB_DUMMY_HCD. The patches and some details are
> here [1].
> 
> This is just a prototype and there are a few things we need to support
> USB fuzzing properly.
> 
> 1. A way to collect coverage from USB code.
> 
> Syzkaller uses kcov (CONFIG_KCOV) to collect coverage from the kernel.
> The issue is that kcov collects coverage in a process context (from
> the kernel task that corresponds to the userspace process that created
> the kcov device). But AFAIU USB uses a kernel worker to process USB
> hub events.

That's right.  And other parts of the USB stack do this too, not just 
the hub driver.  Some drivers create their own helper threads, some 
rely on work queues, some rely on threaded interrupt handlers, and so 
on.  Not to mention non-process stuff, like timer or softirq callbacks.

> The approach I tried to deal with this is to change hub_event() to put
> events into a global queue and wait instead of processing them right
> away. Then I added an ioctl to the hub device that takes an event from
> the queue and processes it the same way hub_event() used to. This
> results in USB events being processed in a process context. The patch
> is here [2].
> 
> Is there a less hacky way to make processing USB events synchronously
> (by an ioctl call from userspace for example), if that's feasible at
> all?

Sounds exactly like what you have already done for the hub driver.  It 
might be okay in that setting, and maybe in a few other drivers, but 
it isn't a general-purpose solution.

In general, I don't think that kind of approach will work for all
drivers.  There are too many different asynchronous execution
mechanisms in use.  Not to mention execution pathways that originate
completely outside the USB stack (Power Management callbacks).

> 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.

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.

> 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.

>  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.

> There's another issue with getting coverage after USB device has been
> initialized. For example some device drivers create new threads that
> communicate with that device. I don't see an easy way to collect
> coverage from those threads.

Right.  This is the problem I mentioned above.

> 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.

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.

> Basically we need a way to run a test program (that connects a USB
> device and works 

Re: Regression caused by: usb: add a flag to skip PHY initialization to struct usb_hcd

2018-06-04 Thread Mats Karrman

On 2018-06-04 03:14, Peter Chen wrote:

  

causes a regression for me on my SolidRun Hummingboard (NXP i.MX6 DL
SoC with chipidea controller).

Example:
-- Cold boot of system.
-- Plug in USB memory stick --> Attached OK.
-- Mount disk --> OK, "ls" works
-- Unmount disk --> OK
-- Unplug USB stick --> Nothing happens. "udevadm monitor" see no events.
-- Plug in/unplug stick again --> Still nothing.
-- Plug in stick and try mounting --> Somehow causes both detach and
attach
       of the stick (mounting fails but mounting again now works).

-- All tested USB devices are working in a similar way. I.e first
attach
   after boot works and then trouble starts.
-- Tested using default device tree from kernel tree
       (arch/arm/boot/dts/imx6dl-hummingboard.dts).

Please ask if you think I can help in any way to track down the issue.


Hi Mats,

I have tested the latest usb-next tree, the hot plug works at my imx6sx-sdb 
board.
Would you please check the value of hcd->skip_phy_initialization at the end of

function host_start?

hcd->skip_phy_initialization=1

BR // Mats


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.

// Mats



External USB fuzzing

2018-06-04 Thread Andrey Konovalov
Hi Greg and Alan!

As you might know I've been working on adding external USB fuzzing
support to syzkaller.

At this point a have a prototype, which is able to emulate USB devices
from userspace via a custom written userspace interface for the gadget
subsystem and CONFIG_USB_DUMMY_HCD. The patches and some details are
here [1].

This is just a prototype and there are a few things we need to support
USB fuzzing properly.

1. A way to collect coverage from USB code.

Syzkaller uses kcov (CONFIG_KCOV) to collect coverage from the kernel.
The issue is that kcov collects coverage in a process context (from
the kernel task that corresponds to the userspace process that created
the kcov device). But AFAIU USB uses a kernel worker to process USB
hub events.

The approach I tried to deal with this is to change hub_event() to put
events into a global queue and wait instead of processing them right
away. Then I added an ioctl to the hub device that takes an event from
the queue and processes it the same way hub_event() used to. This
results in USB events being processed in a process context. The patch
is here [2].

Is there a less hacky way to make processing USB events synchronously
(by an ioctl call from userspace for example), if that's feasible at
all?

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).

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. 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).

There's another issue with getting coverage after USB device has been
initialized. For example some device drivers create new threads that
communicate with that device. I don't see an easy way to collect
coverage from those threads.

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?

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.

Thanks!

[1] 
https://github.com/google/syzkaller/blob/usb-fuzzer/docs/linux/external_fuzzing_usb.md

[2] 
https://github.com/google/syzkaller/blob/usb-fuzzer/tools/usb/0001-usb-fuzzer-add-hub-hijacking-ioctl.patch
--
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: usb HC busted?

2018-06-04 Thread Sudip Mukherjee
Hi Mathias,

On Thu, May 24, 2018 at 04:35:34PM +0300, Mathias Nyman wrote:
> Hi
> 
> On 24.05.2018 00:29, Sudip Mukherjee wrote:
> > Hi Mathias,
> > 
> > On Fri, May 18, 2018 at 04:19:02PM +0300, Mathias Nyman wrote:
> > > On 18.05.2018 16:04, Sudip Mukherjee wrote:

> > > > 
> > We have finally reproduced the error while traces were on. The trace and
> > the relevant part of the dmesg (when the error starts) are in:
> > https://drive.google.com/open?id=1PbcYwL1a9ndtHw1MNjE6uVqb0fFX9jV8
> > 
> > Will request you to have a look and suggest what might be going wrong here.
> > 
> 
> Log show two rings having the same TRB segment dma address, this will 
> completely mess up the transfer:
> 
> While allocating rigs the enque pointers for the two rings are the same:
> 
> 461.859315: xhci_ring_alloc: ISOC efa4e580: enq 
> 0x33386000(0x33386000) deq 
> 0x33386000(0x33386000) segs 2 stream 0 ...bs
> 461.859320: xhci_ring_alloc: ISOC f0ce1f00: enq 
> 0x33386000(0x33386000) deq 
> 0x33386000(0x33386000) segs 2 stream 0 ...
> 
> URBs for ISOC IN transfers are queued on EP3 at enqueue address (33386000 to 
> 33386140)
> 
> 461.859998: xhci_urb_enqueue: ep3in-isoc: urb f0ec0e00 pipe 4294528 slot 8 
> length 0/170 sgs 0/0 stream 0 flags 00010302
> 461.860004: xhci_queue_trb: ISOC: Buffer 2b480240 length 17 TD size 0 
> intr 0 type 'Isoch' flags b:i:I:c:s:I:e:c
> 461.860006: xhci_inc_enq: ISOC f0ce1f00: enq 
> 0x33386010(0x33386000) deq 
> 0x33386000(0x33386000
> 
> Later URBs for ISOC OUT transfers are queued at the same address, this should 
> not happen:
> 
> 461.901175: xhci_urb_enqueue: ep3out-isoc: urb ecec2600 pipe 100096 slot 8 
> length 0/51 sgs 0/0 stream 0 flags 00010002
> 461.901180: xhci_queue_trb: ISOC: Buffer 2d9fa805 length 17 TD size 0 
> intr 0 type 'Isoch' flags b:i:I:c:s:i:e:c
> 461.901181: xhci_inc_enq: ISOC efa4e580: enq 
> 0x33386010(0x33386000) deq 
> 0x33386000(0x33386000)
> 
> So something goes really wrong when allocating or setting up the rings in one 
> of these functions:
> xhci_ring_alloc()
> xhci_alloc_segments_for_ring()
> xhci_initialize_ring_info()
> xhci_segment_alloc()
> xhci_link_segments()
> dma_pool_zalloc()
> 
> To verify and rule out dma_pool_zalloc(), could you apply the attached patch 
> and reproduce with new logs?

I spoke too soon in my yesterday's mail. We were able to reproduce it
on the automated tests. The log and the trace is at:
https://drive.google.com/open?id=1h-3r-1lfjg8oblBGkzdRIq8z3ZNgGZx-

Will request you to have a look at it.

--
Regards
Sudip
--
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: Fwd: usb: uas: device reset most the time while enumeration- usb3.0

2018-06-04 Thread Oliver Neukum
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 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.
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


Re: Since Linux 4.13 tlp or powertop usage cause "xHCI host controller not responding, assume dead" on Dell 5855

2018-06-04 Thread russianneuromancer
Hello!

Thank you for fixing this issue! 

> If you want to have a "tested-by:" flag with your name added to the
> patch 

It doesn't matter to me.

04/06/2018 10:33 +0300, Mathias Nyman:
> On 02.06.2018 15:32, russianneuroman...@ya.ru wrote:
> > Hi!
> > 
> > I tested for-usb-linus branch on Dell 5855 and my issue is no
> > longer reproducible with it (at least for latest two days, which
> > should be more than enough to reproduce it).
> > 
> 
> Great, Thanks.
> 
> If you want to have a "tested-by:" flag with your name added to the
> patch I'll need
> a real name to go with the email address.
> 
> -Mathias
--
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: Since Linux 4.13 tlp or powertop usage cause "xHCI host controller not responding, assume dead" on Dell 5855

2018-06-04 Thread Mathias Nyman

On 02.06.2018 15:32, russianneuroman...@ya.ru wrote:

Hi!

I tested for-usb-linus branch on Dell 5855 and my issue is no longer 
reproducible with it (at least for latest two days, which should be more than 
enough to reproduce it).



Great, Thanks.

If you want to have a "tested-by:" flag with your name added to the patch I'll 
need
a real name to go with the email address.

-Mathias
--
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