Re: [PATCH 36/37] usb: host: xhci: add xhci_virt_device tracer

[Lu Baolu]

Hi,

On 12/29/2016 07:01 PM, Felipe Balbi wrote:
> Let's start tracing at least part of an xhci_virt_device lifetime. We
> might want to extend this tracepoint class later, but for now it already
> exposes quite a bit of valuable information.
>
> Signed-off-by: Felipe Balbi 
> ---
>  drivers/usb/host/xhci-hub.c   |  2 ++
>  drivers/usb/host/xhci-mem.c   |  7 ++
>  drivers/usb/host/xhci-trace.h | 57 
> +++
>  drivers/usb/host/xhci.c   |  1 +
>  4 files changed, 67 insertions(+)
>
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index b99f06f4c421..6fdea9e5e3a5 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -389,6 +389,8 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int 
> slot_id, int suspend)
>   if (!virt_dev)
>   return -ENODEV;
>  
> + trace_xhci_stop_device(virt_dev);
> +

I'd suggest move the trace down until all stop endpoint commands complete.
The state of a virt_device affects only after the command completes.

>   cmd = xhci_alloc_command(xhci, false, true, GFP_NOIO);
>   if (!cmd) {
>   xhci_dbg(xhci, "Couldn't allocate command structure.\n");
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index b12631ef160b..2b7c3504a95a 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -936,6 +936,9 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, int 
> slot_id)
>   return;
>  
>   dev = xhci->devs[slot_id];
> +
> + trace_xhci_free_virt_device(dev);
> +
>   xhci->dcbaa->dev_context_ptrs[slot_id] = 0;
>   if (!dev)
>   return;
> @@ -1041,6 +1044,8 @@ int xhci_alloc_virt_device(struct xhci_hcd *xhci, int 
> slot_id,
>>dcbaa->dev_context_ptrs[slot_id],
>le64_to_cpu(xhci->dcbaa->dev_context_ptrs[slot_id]));
>  
> + trace_xhci_alloc_virt_device(dev);
> +
>   return 1;
>  fail:
>   xhci_free_virt_device(xhci, slot_id);
> @@ -1215,6 +1220,8 @@ int xhci_setup_addressable_virt_dev(struct xhci_hcd 
> *xhci, struct usb_device *ud
>   ep0_ctx->deq = cpu_to_le64(dev->eps[0].ring->first_seg->dma |
>  dev->eps[0].ring->cycle_state);
>  
> + trace_xhci_setup_addressable_virt_device(dev);
> +
>   /* Steps 7 and 8 were done in xhci_alloc_virt_device() */
>  
>   return 0;
> diff --git a/drivers/usb/host/xhci-trace.h b/drivers/usb/host/xhci-trace.h
> index 08bed2f07e50..1e85c893247d 100644
> --- a/drivers/usb/host/xhci-trace.h
> +++ b/drivers/usb/host/xhci-trace.h
> @@ -158,6 +158,63 @@ DEFINE_EVENT(xhci_log_trb, xhci_queue_trb,
>   TP_ARGS(ring, trb)
>  );
>  
> +DECLARE_EVENT_CLASS(xhci_log_virt_dev,
> + TP_PROTO(struct xhci_virt_device *vdev),
> + TP_ARGS(vdev),
> + TP_STRUCT__entry(
> + __field(void *, vdev)
> + __field(unsigned long long, out_ctx)
> + __field(unsigned long long, in_ctx)
> + __field(int, devnum)
> + __field(int, state)
> + __field(int, speed)
> + __field(u8, portnum)
> + __field(u8, level)
> + __field(int, slot_id)
> + ),
> + TP_fast_assign(
> + __entry->vdev = vdev;
> + __entry->in_ctx = (unsigned long long) vdev->in_ctx->dma;
> + __entry->out_ctx = (unsigned long long) vdev->out_ctx->dma;
> + __entry->devnum = vdev->udev->devnum;
> + __entry->state = vdev->udev->state;
> + __entry->speed = vdev->udev->speed;
> + __entry->portnum = vdev->udev->portnum;
> + __entry->level = vdev->udev->level;
> + __entry->slot_id = vdev->udev->slot_id;
> + ),
> + TP_printk("vdev %p ctx %llx | %llx num %d state %d speed %d port %d 
> level %d slot %d",
> + __entry->vdev, __entry->in_ctx, __entry->out_ctx, 
> __entry->devnum,
> + __entry->state, __entry->speed, __entry->portnum, 
> __entry->level,
> + __entry->slot_id
> + )
> +);
> +
> +DEFINE_EVENT(xhci_log_virt_dev, xhci_alloc_virt_device,
> + TP_PROTO(struct xhci_virt_device *vdev),
> + TP_ARGS(vdev)
> +);
> +
> +DEFINE_EVENT(xhci_log_virt_dev, xhci_free_virt_device,
> + TP_PROTO(struct xhci_virt_device *vdev),
> + TP_ARGS(vdev)
> +);
> +
> +DEFINE_EVENT(xhci_log_virt_dev, xhci_setup_device,
> + TP_PROTO(struct xhci_virt_device *vdev),
> + TP_ARGS(vdev)
> +);
> +
> +DEFINE_EVENT(xhci_log_virt_dev, xhci_setup_addressable_virt_device,
> + TP_PROTO(struct xhci_virt_device *vdev),
> + TP_ARGS(vdev)
> +);
> +
> +DEFINE_EVENT(xhci_log_virt_dev, xhci_stop_device,
> + TP_PROTO(struct xhci_virt_device *vdev),
> + TP_ARGS(vdev)
> +);

The lifetime of a USB virtual device in xhci host is defined by
"Figure 10: Slot State Diagram" on spec page 83. These events
are partial of the 

RE: [PATCH v3 2/3] USB3/DWC3: Add property "snps, incr-burst-type-adjustment" for INCR burst type

[Felipe Balbi]

Hi,

Jerry Huang  writes:
>> > On Thu, Dec 22, 2016 at 8:52 PM, Jerry Huang 
>> wrote:
>> > > Hi, Rob,
>> > >> -Original Message-
>> > >> From: Rob Herring [mailto:r...@kernel.org]
>> > >> Sent: Friday, December 23, 2016 2:45 AM
>> > >> To: Jerry Huang 
>> > >> Cc: ba...@kernel.org; mark.rutl...@arm.com;
>> > >> catalin.mari...@arm.com; will.dea...@arm.com;
>> > >> li...@armlinux.org.uk; devicet...@vger.kernel.org;
>> > >> linux-usb@vger.kernel.org; linux-ker...@vger.kernel.org; linux-arm-
>> > >> ker...@lists.infradead.org
>> > >> Subject: Re: [PATCH v3 2/3] USB3/DWC3: Add property "snps,
>> > >> incr-burst- type-adjustment" for INCR burst type
>> > >>
>> > >> On Mon, Dec 19, 2016 at 05:25:53PM +0800, Changming Huang wrote:
>> > >> > New property "snps,incr-burst-type-adjustment = , " for
>> > >> > USB3.0
>> > >> DWC3.
>> > >> > Field "x": 1/0 - undefined length INCR burst type enable or not;
>> > >> > Field
>> > >> > "y": INCR4/INCR8/INCR16/INCR32/INCR64/INCR128/INCR256 burst
>> type.
>> > >> >
>> > >> > While enabling undefined length INCR burst type and INCR16 burst
>> > >> > type, get better write performance on NXP Layerscape platform:
>> > >> > around 3% improvement (from 364MB/s to 375MB/s).
>> > >> >
>> > >> > Signed-off-by: Changming Huang 
>> > >> > ---
>> > >> > Changes in v3:
>> > >> >   - add new property for INCR burst in usb node.
>> > >> >
>> > >> >  Documentation/devicetree/bindings/usb/dwc3.txt |5 +
>> > >> >  arch/arm/boot/dts/ls1021a.dtsi |1 +
>> > >> >  arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi |3 +++
>> > >> >  arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi |2 ++
>> > >> >  4 files changed, 11 insertions(+)
>> > >> >
>> > >> > diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
>> > >> > b/Documentation/devicetree/bindings/usb/dwc3.txt
>> > >> > index e3e6983..8c405a3 100644
>> > >> > --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>> > >> > +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>> > >> > @@ -55,6 +55,10 @@ Optional properties:
>> > >> > fladj_30mhz_sdbnd signal is invalid or incorrect.
>> > >> >
>> > >> >   -  tx-fifo-resize: determines if the FIFO *has* to
>> > >> > be
>> > >> reallocated.
>> > >> > + - snps,incr-burst-type-adjustment: Value for INCR burst type of
>> > >> GSBUSCFG0
>> > >> > +   register, undefined length INCR burst type enable and INCRx type.
>> > >> > +   First field is for undefined length INCR burst type enable or not.
>> > >> > +   Second field is for largest INCRx type enabled.
>> > >>
>> > >> Why do you need the first field? Is the 2nd field used if the 1st is 0?
>> > >> If not, then just use the presence of the property to enable or not.
>> > > The first field is one switch.
>> > > When it is 1, means undefined length INCR burst type enabled, we can
>> > > use
>> > any length less than or equal to the largest-enabled burst length of
>> > INCR4/8/16/32/64/128/256.
>> > > When it is zero, means INCRx burst mode enabled, we can use one
>> > > fixed
>> > burst length of 1/4/8/16/32/64/128/256 byte.
>> > > So, the 2nd field is used if the 1st is 0, we need to select one
>> > > largest burst
>> > length the USB controller can support.
>> > > If we don't want to change the value of this register (use the
>> > > default value),
>> > we don't need to add this property to usb node.
>> >
>> > Just make this a single value with 0 meaning INCR and 4/8/16/etc being
>> INCRx.
>> Maybe, I didn't describe it clearly.
>> According to DWC3 spec, the value "0" of field INCRBrstEna means INCRx
>> burst mode, 1 means INCR burst mode.
>> Regardless of the value of INCRBrstEna [bit0], we need to modify the other
>> field bit[1,2,3,4,5,6,7] to one INCR burst type  for the platform supported.
>> Ad you mentioned, if we just use a single value with 0 meaning INCR and
>> 4/8/16/etc being INCRx.
>> I understand totally that when it is none-zero, we can use it for INCR burst
>> mode.
>> Then, when it is 0, how to select the INCRx value?
>> 
>> So, I think we still need two vaue to specify INCRBrstEna and INCRx burst
>> type.
> Hi, Balbi, 
> It seems there is no feedback for my comment, so these patches can be 
> accepted?

probably not, we need to really understand what information we need so
it can be described properly. The last thing we want is unnecessary DT
properties.

It seems to me that we can extrapolate INCRBrstEna based on which burst
modes are enabled. If only 0 is passed, then that bit should be 1, if 0
and any other size is passed, then that bit should be 0, no?

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH v3 2/3] USB3/DWC3: Add property "snps, incr-burst-type-adjustment" for INCR burst type

[Jerry Huang]

> -Original Message-
> From: Jerry Huang
> Sent: Wednesday, January 04, 2017 10:25 AM
> To: 'Rob Herring' 
> Cc: ba...@kernel.org; mark.rutl...@arm.com; catalin.mari...@arm.com;
> will.dea...@arm.com; li...@armlinux.org.uk; devicet...@vger.kernel.org;
> linux-usb@vger.kernel.org; linux-ker...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org
> Subject: RE: [PATCH v3 2/3] USB3/DWC3: Add property "snps, incr-burst-
> type-adjustment" for INCR burst type
> 
> Hi, Rob,
> 
> > -Original Message-
> > From: Rob Herring [mailto:r...@kernel.org]
> > Sent: Wednesday, January 04, 2017 5:24 AM
> > To: Jerry Huang 
> > Cc: ba...@kernel.org; mark.rutl...@arm.com; catalin.mari...@arm.com;
> > will.dea...@arm.com; li...@armlinux.org.uk;
> > devicet...@vger.kernel.org; linux-usb@vger.kernel.org;
> > linux-ker...@vger.kernel.org; linux-arm- ker...@lists.infradead.org
> > Subject: Re: [PATCH v3 2/3] USB3/DWC3: Add property "snps, incr-burst-
> > type-adjustment" for INCR burst type
> >
> > On Thu, Dec 22, 2016 at 8:52 PM, Jerry Huang 
> wrote:
> > > Hi, Rob,
> > >> -Original Message-
> > >> From: Rob Herring [mailto:r...@kernel.org]
> > >> Sent: Friday, December 23, 2016 2:45 AM
> > >> To: Jerry Huang 
> > >> Cc: ba...@kernel.org; mark.rutl...@arm.com;
> > >> catalin.mari...@arm.com; will.dea...@arm.com;
> > >> li...@armlinux.org.uk; devicet...@vger.kernel.org;
> > >> linux-usb@vger.kernel.org; linux-ker...@vger.kernel.org; linux-arm-
> > >> ker...@lists.infradead.org
> > >> Subject: Re: [PATCH v3 2/3] USB3/DWC3: Add property "snps,
> > >> incr-burst- type-adjustment" for INCR burst type
> > >>
> > >> On Mon, Dec 19, 2016 at 05:25:53PM +0800, Changming Huang wrote:
> > >> > New property "snps,incr-burst-type-adjustment = , " for
> > >> > USB3.0
> > >> DWC3.
> > >> > Field "x": 1/0 - undefined length INCR burst type enable or not;
> > >> > Field
> > >> > "y": INCR4/INCR8/INCR16/INCR32/INCR64/INCR128/INCR256 burst
> type.
> > >> >
> > >> > While enabling undefined length INCR burst type and INCR16 burst
> > >> > type, get better write performance on NXP Layerscape platform:
> > >> > around 3% improvement (from 364MB/s to 375MB/s).
> > >> >
> > >> > Signed-off-by: Changming Huang 
> > >> > ---
> > >> > Changes in v3:
> > >> >   - add new property for INCR burst in usb node.
> > >> >
> > >> >  Documentation/devicetree/bindings/usb/dwc3.txt |5 +
> > >> >  arch/arm/boot/dts/ls1021a.dtsi |1 +
> > >> >  arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi |3 +++
> > >> >  arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi |2 ++
> > >> >  4 files changed, 11 insertions(+)
> > >> >
> > >> > diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
> > >> > b/Documentation/devicetree/bindings/usb/dwc3.txt
> > >> > index e3e6983..8c405a3 100644
> > >> > --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> > >> > +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> > >> > @@ -55,6 +55,10 @@ Optional properties:
> > >> > fladj_30mhz_sdbnd signal is invalid or incorrect.
> > >> >
> > >> >   -  tx-fifo-resize: determines if the FIFO *has* to
> > >> > be
> > >> reallocated.
> > >> > + - snps,incr-burst-type-adjustment: Value for INCR burst type of
> > >> GSBUSCFG0
> > >> > +   register, undefined length INCR burst type enable and INCRx type.
> > >> > +   First field is for undefined length INCR burst type enable or not.
> > >> > +   Second field is for largest INCRx type enabled.
> > >>
> > >> Why do you need the first field? Is the 2nd field used if the 1st is 0?
> > >> If not, then just use the presence of the property to enable or not.
> > > The first field is one switch.
> > > When it is 1, means undefined length INCR burst type enabled, we can
> > > use
> > any length less than or equal to the largest-enabled burst length of
> > INCR4/8/16/32/64/128/256.
> > > When it is zero, means INCRx burst mode enabled, we can use one
> > > fixed
> > burst length of 1/4/8/16/32/64/128/256 byte.
> > > So, the 2nd field is used if the 1st is 0, we need to select one
> > > largest burst
> > length the USB controller can support.
> > > If we don't want to change the value of this register (use the
> > > default value),
> > we don't need to add this property to usb node.
> >
> > Just make this a single value with 0 meaning INCR and 4/8/16/etc being
> INCRx.
> Maybe, I didn't describe it clearly.
> According to DWC3 spec, the value "0" of field INCRBrstEna means INCRx
> burst mode, 1 means INCR burst mode.
> Regardless of the value of INCRBrstEna [bit0], we need to modify the other
> field bit[1,2,3,4,5,6,7] to one INCR burst type  for the platform supported.
> Ad you mentioned, if we just use a single value with 0 meaning INCR and
> 4/8/16/etc being INCRx.
> I understand totally that when it is none-zero, we can use it for INCR burst
> mode.
> Then, when it is 0, how to select the INCRx 

Re: [PATCH 1/9] usb: dwc2: Cleanup some checkpatch issues

[Felipe Balbi]

John Youn  writes:
> This commmit is the result of running checkpatch --fix.
>
> The results were verified for correctness. Some of the fixes result in
> line over 80 char which we will fix manually later.
>
> The following is a summary of what was done by checkpatch:
> * Remove externs on function prototypes.
> * Replace symbolic permissions with octal.
> * Align code to open parens.
> * Replace 'unsigned' with 'unsigned int'.
> * Remove unneccessary blank lines.
> * Add blank lines after declarations.
> * Add spaces around operators.
> * Remove unnecessary spaces after casts.
> * Replace 'x == NULL' with '!x'.
> * Replace kzalloc() with kcalloc().
> * Concatenate multi-line strings.
> * Use the BIT() macro.
>
> Signed-off-by: John Youn 

doesn't apply to testing/next, please rebase.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 30/37] usb: host: xhci: make a generic TRB tracer

[Felipe Balbi]

Hi,

Lu Baolu  writes:
 +  __entry->type = ring->type;
 +  __entry->field0 = le32_to_cpu(trb->field[0]);
 +  __entry->field1 = le32_to_cpu(trb->field[1]);
 +  __entry->field2 = le32_to_cpu(trb->field[2]);
 +  __entry->field3 = le32_to_cpu(trb->field[3]);
),
 -  TP_printk("\ntrb_dma=@%llx, trb_va=@%p, status=%08x, flags=%08x",
 -  (unsigned long long) __entry->dma, __entry->va,
 -  __entry->status, __entry->flags
 +  TP_printk("%s: %s", xhci_ring_type_string(__entry->type),
>>> How about moving the common fields of a TRB (like TRB type and
>>> the cycle bit) to the place between ring type and trb decoding string?
>>> And remove type and cycle decoding in xhci_decode_trb() as well.
>>>
>>> Something like:
>>>
>>> diff --git a/drivers/usb/host/xhci-trace.h b/drivers/usb/host/xhci-trace.h
>>> index d01524b..f8ef7b8 100644
>>> --- a/drivers/usb/host/xhci-trace.h
>>> +++ b/drivers/usb/host/xhci-trace.h
>>> @@ -132,9 +132,11 @@ DECLARE_EVENT_CLASS(xhci_log_trb,
>>> __entry->field2 = le32_to_cpu(trb->field[2]);
>>> __entry->field3 = le32_to_cpu(trb->field[3]);
>>> ),
>>> -   TP_printk("%s: %s", xhci_ring_type_string(__entry->type),
>>> -   xhci_decode_trb(__entry->field0, __entry->field1,
>>> -   __entry->field2, __entry->field3)
>>> +   TP_printk("%s: %s: %c: %s", xhci_ring_type_string(__entry->type),
>>> + xhci_trb_type_string(TRB_FIELD_TO_TYPE(__entry->field3)),
>>> + __entry->field3 & TRB_CYCLE ? 'C' : 'c',
>>> + xhci_decode_trb(__entry->field0, __entry->field1,
>>> + __entry->field2, __entry->field3)
>> and what do I get with that? What's the actual benefit?
>
> I thought that it could make xhci_decode_trb() a bit simpler.

It'll have a very marginal simplification of a single function that's
only called from tracepoints. I wonder if there's an actual benefit
there.

 +DEFINE_EVENT(xhci_log_trb, xhci_handle_event,
 +  TP_PROTO(struct xhci_ring *ring, struct xhci_generic_trb *trb),
 +  TP_ARGS(ring, trb)
 +);
 +
 +DEFINE_EVENT(xhci_log_trb, xhci_handle_command,
 +  TP_PROTO(struct xhci_ring *ring, struct xhci_generic_trb *trb),
 +  TP_ARGS(ring, trb)
 +);
 +
 +DEFINE_EVENT(xhci_log_trb, xhci_handle_transfer,
 +  TP_PROTO(struct xhci_ring *ring, struct xhci_generic_trb *trb),
 +  TP_ARGS(ring, trb)
 +);
 +
 +DEFINE_EVENT(xhci_log_trb, xhci_queue_trb,
 +  TP_PROTO(struct xhci_ring *ring, struct xhci_generic_trb *trb),
 +  TP_ARGS(ring, trb)
  );
>>> xhci_handle_command and xhci_handle_transfer are duplications
>>> of xhci_handle_event. I suggest to remove them.
>> no, they are not. They give us more granularity into which events we
>> want to enable. 
>
> Fair enough.
>
> But why not defining events for all possible event types as well
> and dropping the all-in-one xhci_handle_event switch.
>
> A single event TRB will be traced twice in the same execution
> path if xhci_handle_event and xhci_handle_command/transfer
> are both enabled.

no, it won't. Commands sit in the Command Ring. Events sit in the Event
Ring. Transfers sit in the various Endpoint Rings.

Compile the branch, enable the tracers and look at the output.

>> They also make it clear where the even is coming
>> from. Imagine how the trace would look like if I had a single event and
>> just called trace_xhci_handle_event() from all locations. How would you
>> differentiate from all possible call sites?
>
> These events happen only in interrupt handler. There are no other
> possible call sites.

you misunderstand. Look at the output of the tracepoints and imagine how
they would look if we had a single event for the TRB class of
tracepoints.

>>> How about adding an event for the link TRBs. Something like,
>>>
>>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>>> index 4bad432..6dc8efb 100644
>>> --- a/drivers/usb/host/xhci-ring.c
>>> +++ b/drivers/usb/host/xhci-ring.c
>>> @@ -173,6 +173,7 @@ static void inc_deq(struct xhci_hcd *xhci, struct 
>>> xhci_ring *ring)
>>> ring->num_trbs_free++;
>>> }
>>> while (trb_is_link(ring->dequeue)) {
>>> +   trace_xhci_link_trb(ring, ring->dequeue);
>>> ring->deq_seg = ring->deq_seg->next;
>>> ring->dequeue = ring->deq_seg->trbs;
>>> }
>>> @@ -211,6 +212,7 @@ static void inc_enq(struct xhci_hcd *xhci, struct 
>>> xhci_ring *ring,
>>> ring->enq_updates++;
>>> /* Update the dequeue pointer further if that was a link TRB */
>>> while (trb_is_link(next)) {
>>> +   trace_xhci_link_trb(ring, next);
>>>  
>>> /*
>>>  * If the caller doesn't plan on 

Re: [PATCH 31/37] usb: host: xhci: add urb_enqueue/dequeue/giveback tracers

[Felipe Balbi]

Hi,

Lu Baolu  writes:
> Hi,
>
> On 12/29/2016 07:01 PM, Felipe Balbi wrote:
>> These three new tracers will help us tie TRBs into URBs by *also*
>> looking into URB lifetime.
>
> I didn't see anything related to TRBs in the patch.
> Anything I missed?

yes. Ordering of events. Seriously, you ought to compile my xhci-cleanup
patches and run with tracers enabled. Here's what I'm talking about:

before $subject, we would only see TRBs being queued to HW and
completed, etc. After $subject, we see URBs being queued by class
drivers, which in turn triggers xHCI driver to prepare TRBs which will
be queued to HW. Once a completion IRQ fires, we see TRBs being
"dequeued" from HW and URBs being given back.

IOW, after $subject we know that a particular TRB (or a set of them)
were queued because an URB was queued. We know the size of the URB, a
pointer to it, etc. We can actually match urb->transfer_length to
sum_of_trb_sizes().

>> Signed-off-by: Felipe Balbi 
>> ---
>>  drivers/usb/host/xhci-ring.c  |  1 +
>>  drivers/usb/host/xhci-trace.h | 70 
>> +++
>>  drivers/usb/host/xhci.c   |  5 
>>  3 files changed, 76 insertions(+)
>>
>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>> index 0ee7d358b812..1431e0651e78 100644
>> --- a/drivers/usb/host/xhci-ring.c
>> +++ b/drivers/usb/host/xhci-ring.c
>> @@ -627,6 +627,7 @@ static void xhci_giveback_urb_in_irq(struct xhci_hcd 
>> *xhci,
>>  usb_hcd_unlink_urb_from_ep(hcd, urb);
>>  spin_unlock(>lock);
>>  usb_hcd_giveback_urb(hcd, urb, status);
>> +trace_xhci_urb_giveback(urb);
>
> There is another trace point in xhci_urb_dequeue().
>
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index b49588f..ee46877 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -1535,6 +1535,7 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb 
> *urb, int status)
>  
> usb_hcd_unlink_urb_from_ep(hcd, urb);
> spin_unlock_irqrestore(>lock, flags);
> +   trace_xhci_urb_giveback(urb);

yes, so? I want to know that the URB *was* indeed given back.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 1/4] phy: sun4i-usb: add support for V3s USB PHY

[Kishon Vijay Abraham I]

On Friday 06 January 2017 07:26 PM, Maxime Ripard wrote:
> On Tue, Jan 03, 2017 at 11:25:31PM +0800, Icenowy Zheng wrote:
>> Allwinner V3s come with a USB PHY controller slightly different to other
>> SoCs, with only one PHY.
>>
>> Add support for it.
>>
>> Signed-off-by: Icenowy Zheng 
> 
> Acked-by: Maxime Ripard 

merged, thanks!

-Kishon
> 
> Thanks,
> Maxime
> 
--
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: [PATCH 36/37] usb: host: xhci: add xhci_virt_device tracer

[Felipe Balbi]

Hi,

Lu Baolu  writes:
> Hi,
>
> On 12/29/2016 07:01 PM, Felipe Balbi wrote:
>> Let's start tracing at least part of an xhci_virt_device lifetime. We
>> might want to extend this tracepoint class later, but for now it already
>> exposes quite a bit of valuable information.
>>
>> Signed-off-by: Felipe Balbi 
>> ---
>>  drivers/usb/host/xhci-hub.c   |  2 ++
>>  drivers/usb/host/xhci-mem.c   |  7 ++
>>  drivers/usb/host/xhci-trace.h | 57 
>> +++
>>  drivers/usb/host/xhci.c   |  1 +
>>  4 files changed, 67 insertions(+)
>>
>> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
>> index b99f06f4c421..6fdea9e5e3a5 100644
>> --- a/drivers/usb/host/xhci-hub.c
>> +++ b/drivers/usb/host/xhci-hub.c
>> @@ -389,6 +389,8 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int 
>> slot_id, int suspend)
>>  if (!virt_dev)
>>  return -ENODEV;
>>  
>> +trace_xhci_stop_device(virt_dev);
>> +
>
> I'd suggest move the trace down until all stop endpoint commands complete.
> The state of a virt_device affects only after the command completes.

no, that's the wrong thing to do. We're trying to track the *lifetime*
of things. xhci_stop_device() prepares a command to stop the
device. There's another command tracer which will trigger on command
completion IRQ. That command will also be traced.

Again, compile the branch and look at the output.

>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
>> index b12631ef160b..2b7c3504a95a 100644
>> --- a/drivers/usb/host/xhci-mem.c
>> +++ b/drivers/usb/host/xhci-mem.c
>> @@ -936,6 +936,9 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, int 
>> slot_id)
>>  return;
>>  
>>  dev = xhci->devs[slot_id];
>> +
>> +trace_xhci_free_virt_device(dev);
>> +
>>  xhci->dcbaa->dev_context_ptrs[slot_id] = 0;
>>  if (!dev)
>>  return;
>> @@ -1041,6 +1044,8 @@ int xhci_alloc_virt_device(struct xhci_hcd *xhci, int 
>> slot_id,
>>   >dcbaa->dev_context_ptrs[slot_id],
>>   le64_to_cpu(xhci->dcbaa->dev_context_ptrs[slot_id]));
>>  
>> +trace_xhci_alloc_virt_device(dev);
>> +
>>  return 1;
>>  fail:
>>  xhci_free_virt_device(xhci, slot_id);
>> @@ -1215,6 +1220,8 @@ int xhci_setup_addressable_virt_dev(struct xhci_hcd 
>> *xhci, struct usb_device *ud
>>  ep0_ctx->deq = cpu_to_le64(dev->eps[0].ring->first_seg->dma |
>> dev->eps[0].ring->cycle_state);
>>  
>> +trace_xhci_setup_addressable_virt_device(dev);
>> +
>>  /* Steps 7 and 8 were done in xhci_alloc_virt_device() */
>>  
>>  return 0;
>> diff --git a/drivers/usb/host/xhci-trace.h b/drivers/usb/host/xhci-trace.h
>> index 08bed2f07e50..1e85c893247d 100644
>> --- a/drivers/usb/host/xhci-trace.h
>> +++ b/drivers/usb/host/xhci-trace.h
>> @@ -158,6 +158,63 @@ DEFINE_EVENT(xhci_log_trb, xhci_queue_trb,
>>  TP_ARGS(ring, trb)
>>  );
>>  
>> +DECLARE_EVENT_CLASS(xhci_log_virt_dev,
>> +TP_PROTO(struct xhci_virt_device *vdev),
>> +TP_ARGS(vdev),
>> +TP_STRUCT__entry(
>> +__field(void *, vdev)
>> +__field(unsigned long long, out_ctx)
>> +__field(unsigned long long, in_ctx)
>> +__field(int, devnum)
>> +__field(int, state)
>> +__field(int, speed)
>> +__field(u8, portnum)
>> +__field(u8, level)
>> +__field(int, slot_id)
>> +),
>> +TP_fast_assign(
>> +__entry->vdev = vdev;
>> +__entry->in_ctx = (unsigned long long) vdev->in_ctx->dma;
>> +__entry->out_ctx = (unsigned long long) vdev->out_ctx->dma;
>> +__entry->devnum = vdev->udev->devnum;
>> +__entry->state = vdev->udev->state;
>> +__entry->speed = vdev->udev->speed;
>> +__entry->portnum = vdev->udev->portnum;
>> +__entry->level = vdev->udev->level;
>> +__entry->slot_id = vdev->udev->slot_id;
>> +),
>> +TP_printk("vdev %p ctx %llx | %llx num %d state %d speed %d port %d 
>> level %d slot %d",
>> +__entry->vdev, __entry->in_ctx, __entry->out_ctx, 
>> __entry->devnum,
>> +__entry->state, __entry->speed, __entry->portnum, 
>> __entry->level,
>> +__entry->slot_id
>> +)
>> +);
>> +
>> +DEFINE_EVENT(xhci_log_virt_dev, xhci_alloc_virt_device,
>> +TP_PROTO(struct xhci_virt_device *vdev),
>> +TP_ARGS(vdev)
>> +);
>> +
>> +DEFINE_EVENT(xhci_log_virt_dev, xhci_free_virt_device,
>> +TP_PROTO(struct xhci_virt_device *vdev),
>> +TP_ARGS(vdev)
>> +);
>> +
>> +DEFINE_EVENT(xhci_log_virt_dev, xhci_setup_device,
>> +TP_PROTO(struct xhci_virt_device *vdev),
>> +TP_ARGS(vdev)
>> +);
>> +
>> +DEFINE_EVENT(xhci_log_virt_dev, xhci_setup_addressable_virt_device,
>> +TP_PROTO(struct xhci_virt_device *vdev),
>> +TP_ARGS(vdev)
>> +);
>> +
>> 

Re: [PATCH v2 1/6] usb: dwc3: omap: Replace the extcon API

[Felipe Balbi]

Hi,

Chanwoo Choi  writes:
> This patch uses the resource-managed extcon API for extcon_register_notifier()
> and replaces the deprecated extcon API as following:
> - extcon_get_cable_state_() -> extcon_get_state()
>
> Signed-off-by: Chanwoo Choi 
> Acked-by: Felipe Balbi 

doesn't apply to testing/next. Please rebase

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 01/21] usb: dwc2: Cleanup some checkpatch issues

[Felipe Balbi]

John Youn  writes:
> This commmit is the result of running checkpatch --fix.
>
> The results were verified for correctness. Some of the fixes result in
> line over 80 char which we will fix manually later.
>
> The following is a summary of what was done by checkpatch:
> * Remove externs on function prototypes.
> * Replace symbolic permissions with octal.
> * Align code to open parens.
> * Replace 'unsigned' with 'unsigned int'.
> * Remove unneccessary blank lines.
> * Add blank lines after declarations.
> * Add spaces around operators.
> * Remove unnecessary spaces after casts.
> * Replace 'x == NULL' with '!x'.
> * Replace kzalloc() with kcalloc().
> * Concatenate multi-line strings.
> * Use the BIT() macro.
>
> Signed-off-by: John Youn 

Unfortunately, doesn't apply to testing/next, please rebase.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v2 1/2] dt-bindings: usb: add DT binding for s3c2410 USB device controller

[Felipe Balbi]

Hi,

Sergio Prado  writes:
> Adds the device tree bindings description for Samsung S3C2410 and
> compatible USB device controller.
>
> Signed-off-by: Sergio Prado 

without Ack from DT folks, I can't take this.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 1/4 v3] usb: dwc2: Avoid sleeping while holding hsotg->lock

[Felipe Balbi]

Hi,

John Stultz  writes:
> Basically when plugging in various cables in different orders, I'm
> occasionally seeing the following BUG splat:
>
> [   86.215403] BUG: scheduling while atomic: kworker/u16:2/53/0x0002
> [   86.219164] usb 1-1: USB disconnect, device number 9
> [   86.226845] Preemption disabled at:[   86.230218]
> [] dwc2_conn_id_status_change+0x120/0x250
> [   86.236894] CPU: 0 PID: 53 Comm: kworker/u16:2 Tainted: GW
>  4.9.0-rc8-00051-gd5a7979-dirty #1702
> [   86.246836] Hardware name: HiKey Development Board (DT)
> [   86.252100] Workqueue: dwc2 dwc2_conn_id_status_change
> [   86.257279] Call trace:
> [   86.259771] [] dump_backtrace+0x0/0x1a0
> [   86.265210] [] show_stack+0x14/0x20
> [   86.270308] [] dump_stack+0x90/0xb0
> [   86.275401] [] __schedule_bug+0x6c/0xb8
> [   86.280841] [] __schedule+0x4f8/0x5b0
> [   86.286099] [] schedule+0x38/0xa0
> [   86.291017] [] schedule_hrtimeout_range_clock+0x8c/0xf0
> [   86.297846] [] schedule_hrtimeout_range+0x10/0x18
> [   86.304150] [] usleep_range+0x50/0x58
> [   86.309418] [] dwc2_wait_for_mode.isra.4+0x54/0xd0
> [   86.315815] [] dwc2_core_reset+0xe0/0x168
> [   86.321431] [] 
> dwc2_hsotg_core_init_disconnected+0x2c/0x310
> [   86.328602] [] dwc2_conn_id_status_change+0x130/0x250
> [   86.335254] [] process_one_work+0x118/0x370
> [   86.341035] [] worker_thread+0x48/0x498
> [   86.346473] [] kthread+0xd0/0xe8
> [   86.351299] [] ret_from_fork+0x10/0x50
>
> This seems to be caused by the dwc2_wait_for_mode() calling
> usleep_range() while the hstog->lock spinlock is held, since
> we take that before calling dwc2_hsotg_core_init_disconnected().
>
> This patch avoids the issue by adding an extra argument to
> dwc2_core_reset(), as suggested by John Youn, which allows us to
> skip the waiting, which should be unnecessary when calling from
> dwc2_hsotg_core_init_disconnected().
>
> Cc: Wei Xu 
> Cc: Guodong Xu 
> Cc: Amit Pundir 
> Cc: Rob Herring 
> Cc: John Youn 
> Cc: Douglas Anderson 
> Cc: Chen Yu 
> Cc: Vardan Mikayelyan 
> Cc: Kishon Vijay Abraham I 
> Cc: Felipe Balbi 
> Cc: Greg Kroah-Hartman 
> Cc: linux-usb@vger.kernel.org
> Signed-off-by: John Stultz 
> ---

doesn't apply to my testing/next. Please rebase

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v5 1/2] usb: host: plat: Enable xhci plat runtime PM

[Baolin Wang]

Hi Mathias,

On 13 December 2016 at 15:49, Baolin Wang  wrote:
> Enable the xhci plat runtime PM for parent device to suspend/resume xhci.
> Also call pm_runtime_get_noresume() in probe() function in case the parent
> device doesn't call suspend/resume callback by runtime PM now.
>
> Signed-off-by: Baolin Wang 
> ---
> Changes since v4:
>  - No updates.
>
> Changes since v3:
>  - Fix kbuild error.
>
> Changes since v2:
>  - Add pm_runtime_get_noresume() in probe() function.
>  - Add pm_runtime_set_suspended()/pm_runtime_put_noidle() in remove() 
> function.
>
> Changes since v1:
>  - No updates.
> ---

Do you have any comments about this patch? Thanks.

>  drivers/usb/host/xhci-plat.c |   41 -
>  1 file changed, 36 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index ed56bf9..5805c6a 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -246,6 +246,10 @@ static int xhci_plat_probe(struct platform_device *pdev)
> if (ret)
> goto dealloc_usb2_hcd;
>
> +   pm_runtime_get_noresume(>dev);
> +   pm_runtime_set_active(>dev);
> +   pm_runtime_enable(>dev);
> +
> return 0;
>
>
> @@ -274,6 +278,10 @@ static int xhci_plat_remove(struct platform_device *dev)
> struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> struct clk *clk = xhci->clk;
>
> +   pm_runtime_set_suspended(>dev);
> +   pm_runtime_put_noidle(>dev);
> +   pm_runtime_disable(>dev);
> +
> usb_remove_hcd(xhci->shared_hcd);
> usb_phy_shutdown(hcd->usb_phy);
>
> @@ -311,14 +319,37 @@ static int xhci_plat_resume(struct device *dev)
>
> return xhci_resume(xhci, 0);
>  }
> +#endif /* CONFIG_PM_SLEEP */
> +
> +#ifdef CONFIG_PM
> +static int xhci_plat_runtime_suspend(struct device *dev)
> +{
> +   struct usb_hcd  *hcd = dev_get_drvdata(dev);
> +   struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> +
> +   return xhci_suspend(xhci, device_may_wakeup(dev));
> +}
> +
> +static int xhci_plat_runtime_resume(struct device *dev)
> +{
> +   struct usb_hcd  *hcd = dev_get_drvdata(dev);
> +   struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> +
> +   return xhci_resume(xhci, 0);
> +}
> +
> +static int xhci_plat_runtime_idle(struct device *dev)
> +{
> +   return 0;
> +}
> +#endif /* CONFIG_PM */
>
>  static const struct dev_pm_ops xhci_plat_pm_ops = {
> SET_SYSTEM_SLEEP_PM_OPS(xhci_plat_suspend, xhci_plat_resume)
> +
> +   SET_RUNTIME_PM_OPS(xhci_plat_runtime_suspend, 
> xhci_plat_runtime_resume,
> +  xhci_plat_runtime_idle)
>  };
> -#define DEV_PM_OPS (_plat_pm_ops)
> -#else
> -#define DEV_PM_OPS NULL
> -#endif /* CONFIG_PM */
>
>  static const struct acpi_device_id usb_xhci_acpi_match[] = {
> /* XHCI-compliant USB Controller */
> @@ -332,7 +363,7 @@ static int xhci_plat_resume(struct device *dev)
> .remove = xhci_plat_remove,
> .driver = {
> .name = "xhci-hcd",
> -   .pm = DEV_PM_OPS,
> +   .pm = _plat_pm_ops,
> .of_match_table = of_match_ptr(usb_xhci_of_match),
> .acpi_match_table = ACPI_PTR(usb_xhci_acpi_match),
> },
> --
> 1.7.9.5
>



-- 
Baolin.wang
Best Regards
--
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: [RFC][PATCH 0/5 v2] Fixes and workarounds for dwc2 on HiKey board

[Felipe Balbi]

Hi,

John Stultz  writes:
> I just wanted to re-send my current queue of patches for dwc2
> controller on the HiKey board, as my last patchset ended up
> colliding with a number of changes that landed in the 4.10-rc
> merge window. 
>
> I've fixed things up and validated these against HiKey. I also
> made a small change to one of the patches as suggested by Vardan.
>
> This does exclude my patchset[1] to add extcon support to dwc2,
> which John Youn suspects a pending rework of the dwc2 fifo init
> logic might make unnecssary (however, no such changes appeared
> to have landed in 4.10-rc, so I'm still using that patchset in
> my testing).
>
> I'm also out for the holidays after today, and while I suspect
> others are likely going to be out as well, I figured I'd send
> these out for a chance at review, rather then sit on them for
> two weeks. I'll resend after the new year addressing any
> feedback I do get though.
>
> Feedback would be greatly appreciated!

John Y, any comments?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v5 2/2] usb: dwc3: core: Support the dwc3 host suspend/resume

[Felipe Balbi]

Hi,

Baolin Wang  writes:
> For some mobile devices with strict power management, we also want to suspend
> the host when the slave is detached for power saving. Thus we add the host
> suspend/resume functions to support this requirement.
>
> Signed-off-by: Baolin Wang 
> ---
> Changes since v4:
>  - Remove Kconfig and just enable host suspend/resume.
>  - Simplify the dwc3_host_suspend/resume() function.
>
> Changes since v3:
>  - No updates.
>
> Changes since v2:
>  - Remove pm_children_suspended() and other unused macros.
>
> Changes since v1:
>  - Add pm_runtime.h head file to avoid kbuild error.
> ---
>  drivers/usb/dwc3/core.c |   26 +-
>  drivers/usb/dwc3/core.h |7 +++
>  drivers/usb/dwc3/host.c |   21 +
>  3 files changed, 53 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 9a4a5e4..7ad4bc3 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1091,6 +1091,7 @@ static int dwc3_probe(struct platform_device *pdev)
>   pm_runtime_use_autosuspend(dev);
>   pm_runtime_set_autosuspend_delay(dev, DWC3_DEFAULT_AUTOSUSPEND_DELAY);
>   pm_runtime_enable(dev);
> + pm_suspend_ignore_children(dev, true);
>   ret = pm_runtime_get_sync(dev);
>   if (ret < 0)
>   goto err1;
> @@ -1215,15 +1216,27 @@ static int dwc3_remove(struct platform_device *pdev)
>  static int dwc3_suspend_common(struct dwc3 *dwc)
>  {
>   unsigned long   flags;
> + int ret;
>  
>   switch (dwc->dr_mode) {
>   case USB_DR_MODE_PERIPHERAL:
> + spin_lock_irqsave(>lock, flags);
> + dwc3_gadget_suspend(dwc);
> + spin_unlock_irqrestore(>lock, flags);
> + break;
>   case USB_DR_MODE_OTG:
> + ret = dwc3_host_suspend(dwc);
> + if (ret)
> + return ret;
> +
>   spin_lock_irqsave(>lock, flags);
>   dwc3_gadget_suspend(dwc);
>   spin_unlock_irqrestore(>lock, flags);
>   break;
>   case USB_DR_MODE_HOST:
> + ret = dwc3_host_suspend(dwc);
> + if (ret)
> + return ret;
>   default:
>   /* do nothing */
>   break;
> @@ -1245,12 +1258,23 @@ static int dwc3_resume_common(struct dwc3 *dwc)
>  
>   switch (dwc->dr_mode) {
>   case USB_DR_MODE_PERIPHERAL:
> + spin_lock_irqsave(>lock, flags);
> + dwc3_gadget_resume(dwc);
> + spin_unlock_irqrestore(>lock, flags);
> + break;
>   case USB_DR_MODE_OTG:
> + ret = dwc3_host_resume(dwc);
> + if (ret)
> + return ret;
> +
>   spin_lock_irqsave(>lock, flags);
>   dwc3_gadget_resume(dwc);
>   spin_unlock_irqrestore(>lock, flags);
> - /* FALLTHROUGH */
> + break;
>   case USB_DR_MODE_HOST:
> + ret = dwc3_host_resume(dwc);
> + if (ret)
> + return ret;
>   default:
>   /* do nothing */
>   break;
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index b585a30..1099168 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1158,11 +1158,18 @@ static inline bool dwc3_is_usb31(struct dwc3 *dwc)
>  #if IS_ENABLED(CONFIG_USB_DWC3_HOST) || IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)
>  int dwc3_host_init(struct dwc3 *dwc);
>  void dwc3_host_exit(struct dwc3 *dwc);
> +int dwc3_host_suspend(struct dwc3 *dwc);
> +int dwc3_host_resume(struct dwc3 *dwc);
>  #else
>  static inline int dwc3_host_init(struct dwc3 *dwc)
>  { return 0; }
>  static inline void dwc3_host_exit(struct dwc3 *dwc)
>  { }
> +
> +static inline int dwc3_host_suspend(struct dwc3 *dwc)
> +{ return 0; }
> +static inline int dwc3_host_resume(struct dwc3 *dwc)
> +{ return 0; }
>  #endif
>  
>  #if IS_ENABLED(CONFIG_USB_DWC3_GADGET) || 
> IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)
> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
> index ed82464..7959ef0 100644
> --- a/drivers/usb/dwc3/host.c
> +++ b/drivers/usb/dwc3/host.c
> @@ -16,6 +16,7 @@
>   */
>  
>  #include 
> +#include 
>  
>  #include "core.h"
>  
> @@ -130,3 +131,23 @@ void dwc3_host_exit(struct dwc3 *dwc)
> dev_name(>xhci->dev));
>   platform_device_unregister(dwc->xhci);
>  }
> +
> +int dwc3_host_suspend(struct dwc3 *dwc)
> +{
> + struct device *xhci = >xhci->dev;
> +
> + /*
> +  * Note: if we get the -EBUSY, which means the xHCI children devices are
> +  * not in suspend state yet, the glue layer need to wait for a while and
> +  * try to suspend xHCI device again.
> +  */
> + return pm_runtime_put_sync(xhci);
> +}
> +
> +int dwc3_host_resume(struct dwc3 *dwc)
> +{
> + struct device *xhci = >xhci->dev;
> +
> + /* 

Re: [PATCH v5 2/2] usb: dwc3: core: Support the dwc3 host suspend/resume

[Baolin Wang]

Hi,

On 16 January 2017 at 18:28, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  writes:
>> For some mobile devices with strict power management, we also want to suspend
>> the host when the slave is detached for power saving. Thus we add the host
>> suspend/resume functions to support this requirement.
>>
>> Signed-off-by: Baolin Wang 
>> ---
>> Changes since v4:
>>  - Remove Kconfig and just enable host suspend/resume.
>>  - Simplify the dwc3_host_suspend/resume() function.
>>
>> Changes since v3:
>>  - No updates.
>>
>> Changes since v2:
>>  - Remove pm_children_suspended() and other unused macros.
>>
>> Changes since v1:
>>  - Add pm_runtime.h head file to avoid kbuild error.
>> ---
>>  drivers/usb/dwc3/core.c |   26 +-
>>  drivers/usb/dwc3/core.h |7 +++
>>  drivers/usb/dwc3/host.c |   21 +
>>  3 files changed, 53 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 9a4a5e4..7ad4bc3 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -1091,6 +1091,7 @@ static int dwc3_probe(struct platform_device *pdev)
>>   pm_runtime_use_autosuspend(dev);
>>   pm_runtime_set_autosuspend_delay(dev, DWC3_DEFAULT_AUTOSUSPEND_DELAY);
>>   pm_runtime_enable(dev);
>> + pm_suspend_ignore_children(dev, true);
>>   ret = pm_runtime_get_sync(dev);
>>   if (ret < 0)
>>   goto err1;
>> @@ -1215,15 +1216,27 @@ static int dwc3_remove(struct platform_device *pdev)
>>  static int dwc3_suspend_common(struct dwc3 *dwc)
>>  {
>>   unsigned long   flags;
>> + int ret;
>>
>>   switch (dwc->dr_mode) {
>>   case USB_DR_MODE_PERIPHERAL:
>> + spin_lock_irqsave(>lock, flags);
>> + dwc3_gadget_suspend(dwc);
>> + spin_unlock_irqrestore(>lock, flags);
>> + break;
>>   case USB_DR_MODE_OTG:
>> + ret = dwc3_host_suspend(dwc);
>> + if (ret)
>> + return ret;
>> +
>>   spin_lock_irqsave(>lock, flags);
>>   dwc3_gadget_suspend(dwc);
>>   spin_unlock_irqrestore(>lock, flags);
>>   break;
>>   case USB_DR_MODE_HOST:
>> + ret = dwc3_host_suspend(dwc);
>> + if (ret)
>> + return ret;
>>   default:
>>   /* do nothing */
>>   break;
>> @@ -1245,12 +1258,23 @@ static int dwc3_resume_common(struct dwc3 *dwc)
>>
>>   switch (dwc->dr_mode) {
>>   case USB_DR_MODE_PERIPHERAL:
>> + spin_lock_irqsave(>lock, flags);
>> + dwc3_gadget_resume(dwc);
>> + spin_unlock_irqrestore(>lock, flags);
>> + break;
>>   case USB_DR_MODE_OTG:
>> + ret = dwc3_host_resume(dwc);
>> + if (ret)
>> + return ret;
>> +
>>   spin_lock_irqsave(>lock, flags);
>>   dwc3_gadget_resume(dwc);
>>   spin_unlock_irqrestore(>lock, flags);
>> - /* FALLTHROUGH */
>> + break;
>>   case USB_DR_MODE_HOST:
>> + ret = dwc3_host_resume(dwc);
>> + if (ret)
>> + return ret;
>>   default:
>>   /* do nothing */
>>   break;
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index b585a30..1099168 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -1158,11 +1158,18 @@ static inline bool dwc3_is_usb31(struct dwc3 *dwc)
>>  #if IS_ENABLED(CONFIG_USB_DWC3_HOST) || 
>> IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)
>>  int dwc3_host_init(struct dwc3 *dwc);
>>  void dwc3_host_exit(struct dwc3 *dwc);
>> +int dwc3_host_suspend(struct dwc3 *dwc);
>> +int dwc3_host_resume(struct dwc3 *dwc);
>>  #else
>>  static inline int dwc3_host_init(struct dwc3 *dwc)
>>  { return 0; }
>>  static inline void dwc3_host_exit(struct dwc3 *dwc)
>>  { }
>> +
>> +static inline int dwc3_host_suspend(struct dwc3 *dwc)
>> +{ return 0; }
>> +static inline int dwc3_host_resume(struct dwc3 *dwc)
>> +{ return 0; }
>>  #endif
>>
>>  #if IS_ENABLED(CONFIG_USB_DWC3_GADGET) || 
>> IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)
>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
>> index ed82464..7959ef0 100644
>> --- a/drivers/usb/dwc3/host.c
>> +++ b/drivers/usb/dwc3/host.c
>> @@ -16,6 +16,7 @@
>>   */
>>
>>  #include 
>> +#include 
>>
>>  #include "core.h"
>>
>> @@ -130,3 +131,23 @@ void dwc3_host_exit(struct dwc3 *dwc)
>> dev_name(>xhci->dev));
>>   platform_device_unregister(dwc->xhci);
>>  }
>> +
>> +int dwc3_host_suspend(struct dwc3 *dwc)
>> +{
>> + struct device *xhci = >xhci->dev;
>> +
>> + /*
>> +  * Note: if we get the -EBUSY, which means the xHCI children devices 
>> are
>> +  * not in suspend state yet, the glue layer need to wait for a while 
>> 

Re: [PATCH] usb: gadget: udc: atmel: used managed kasprintf

[Alexandre Belloni]

On 16/01/2017 at 12:27:04 +0200, Felipe Balbi wrote :
> 
> Hi,
> 
> David Laight  writes:
> > From: Alexandre Belloni
> >> Sent: 02 December 2016 16:19
> >> On 02/12/2016 at 15:59:57 +, David Laight wrote :
> >> > From: Alexandre Belloni
> >> > > Sent: 01 December 2016 10:27
> >> > > Use devm_kasprintf instead of simple kasprintf to free the allocated 
> >> > > memory
> >> > > when needed.
> >> >
> >> > s/when needed/when the device is freed/
> >> >
> >> > > Suggested-by: Peter Rosin 
> >> > > Signed-off-by: Alexandre Belloni 
> >> > > ---
> >> > >  drivers/usb/gadget/udc/atmel_usba_udc.c | 3 ++-
> >> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> >> > >
> >> > > diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c 
> >> > > b/drivers/usb/gadget/udc/atmel_usba_udc.c
> >> > > index 45bc997d0711..aec72fe8273c 100644
> >> > > --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
> >> > > +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
> >> > > @@ -1978,7 +1978,8 @@ static struct usba_ep * atmel_udc_of_init(struct 
> >> > > platform_device *pdev,
> >> > >dev_err(>dev, "of_probe: name 
> >> > > error(%d)\n", ret);
> >> > >goto err;
> >> > >}
> >> > > -  ep->ep.name = kasprintf(GFP_KERNEL, "ep%d", ep->index);
> >> > > +  ep->ep.name = devm_kasprintf(>dev, GFP_KERNEL, 
> >> > > "ep%d",
> >> > > +   ep->index);
> >> >
> >> > Acually why bother mallocing such a small string at all.
> >> > The maximum length is 12 bytes even if 'index' are unrestricted.
> >> >
> >> 
> >> IIRC, using statically allocated string is failing somewhere is the USB
> >> core but I don't remember all the details.
> >
> > I can't imagine that changing ep->ep.name from 'char *' to 'char [12]' would
> > make any difference.
> 
> the actual name is managed by the UDC. Meaning, ep->ep.name should be a
> pointer, but it could very well just point to ep->name which would be
> char [12].
> 

Yeah, I sent a patch that did just that.
http://lists.infradead.org/pipermail/linux-arm-kernel/2017-January/478602.html

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
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: [PATCH] usb: gadget: udc: atmel: used managed kasprintf

[Felipe Balbi]

Hi,

Alexandre Belloni  writes:
>> David Laight  writes:
>> > From: Alexandre Belloni
>> >> Sent: 02 December 2016 16:19
>> >> On 02/12/2016 at 15:59:57 +, David Laight wrote :
>> >> > From: Alexandre Belloni
>> >> > > Sent: 01 December 2016 10:27
>> >> > > Use devm_kasprintf instead of simple kasprintf to free the allocated 
>> >> > > memory
>> >> > > when needed.
>> >> >
>> >> > s/when needed/when the device is freed/
>> >> >
>> >> > > Suggested-by: Peter Rosin 
>> >> > > Signed-off-by: Alexandre Belloni 
>> >> > > 
>> >> > > ---
>> >> > >  drivers/usb/gadget/udc/atmel_usba_udc.c | 3 ++-
>> >> > >  1 file changed, 2 insertions(+), 1 deletion(-)
>> >> > >
>> >> > > diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c 
>> >> > > b/drivers/usb/gadget/udc/atmel_usba_udc.c
>> >> > > index 45bc997d0711..aec72fe8273c 100644
>> >> > > --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
>> >> > > +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
>> >> > > @@ -1978,7 +1978,8 @@ static struct usba_ep * 
>> >> > > atmel_udc_of_init(struct platform_device *pdev,
>> >> > >   dev_err(>dev, "of_probe: name 
>> >> > > error(%d)\n", ret);
>> >> > >   goto err;
>> >> > >   }
>> >> > > - ep->ep.name = kasprintf(GFP_KERNEL, "ep%d", ep->index);
>> >> > > + ep->ep.name = devm_kasprintf(>dev, GFP_KERNEL, 
>> >> > > "ep%d",
>> >> > > +  ep->index);
>> >> >
>> >> > Acually why bother mallocing such a small string at all.
>> >> > The maximum length is 12 bytes even if 'index' are unrestricted.
>> >> >
>> >> 
>> >> IIRC, using statically allocated string is failing somewhere is the USB
>> >> core but I don't remember all the details.
>> >
>> > I can't imagine that changing ep->ep.name from 'char *' to 'char [12]' 
>> > would
>> > make any difference.
>> 
>> the actual name is managed by the UDC. Meaning, ep->ep.name should be a
>> pointer, but it could very well just point to ep->name which would be
>> char [12].
>> 
>
> Yeah, I sent a patch that did just that.
> http://lists.infradead.org/pipermail/linux-arm-kernel/2017-January/478602.html

it's in my fixes now :-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: ep0: Fix the possible missed request for handling delay STATUS phase

[Baolin Wang]

Hi,

On 16 January 2017 at 19:29, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  writes:
>> Hi,
>>
>> On 16 January 2017 at 18:56, Felipe Balbi  wrote:
>>>
>>> Hi,
>>>
>>> Baolin Wang  writes:
 When handing the SETUP packet by composite_setup(), we will release the
 dwc->lock. If we get the 'USB_GADGET_DELAYED_STATUS' result from setup
 function, which means we need to delay handling the STATUS phase.
>>>
>>> this sentence needs a little work. Seems like it's missing some
>>> information.
>>>
>>> anyway, I get that we release the lock but...
>>>
 But during the lock release period, maybe the request for handling delay
>>>
>>> execution of ->setup() itself should be locked. I can see that it's only
>>> locked for set_config() which is rather peculiar.
>>>
>>> What exact request you had when you triggered this? (Hint: dwc3
>>> tracepoints print out ctrl request bytes). IIRC, only set_config() or
>>> f->set_alt() can actually return USB_GADGET_DELAYED_STATUS.
>>
>> Yes, when host set configuration for mass storage driver, it can
>> produce this issue.
>>
>>>
>>> Which gadget driver were you using when you triggered this?
>>
>> mass storage driver. When host issues the setting config request, we
>> will get USB_GADGET_DELAYED_STATUS result from
>> set_config()--->fsg_set_alt(). Then the mass storage driver will issue
>> one thread to complete the status stage by ep0_queue() (this thread
>> may be running on another core), then if the thread issues ep0_queue()
>> too fast before we get the dwc->lock in dwc3_ep0_delegate_req() or
>> before we get into the STATUS stage, then we can not handle this
>> request for the delay STATUS stage in dwc3_gadget_ep0_queue().
>>
>>>
>>> Another point here is that the really robust way of fixing this is to
>>> get rid of USB_GADGET_DELAYED_STATUS altogether and just make sure
>>> gadget drivers know how to queue requests for all three phases of a
>>> Control Transfer.
>>>
>>> A lot of code will be removed from all gadget drivers and UDC drivers
>>> while combining all of it in a single place in composite.c.
>>>
>>> The reason I'm saying this is that other UDC drivers might have similar
>>> races already but they just haven't triggered.
>>
>> Yes, maybe.
>>
>>>
 STATUS phase has been queued into list before we set 'dwc->delayed_status'
 flag or entering 'EP0_STATUS_PHASE' phase, then we will miss the chance
 to handle the STATUS phase. Thus we should check if the request for delay
 STATUS phase has been enqueued when entering 'EP0_STATUS_PHASE' phase in
 dwc3_ep0_xfernotready(), if so, we should handle it.

 Signed-off-by: Baolin Wang 
 ---
  drivers/usb/dwc3/ep0.c |   14 ++
  1 file changed, 14 insertions(+)

 diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
 index 9bb1f85..e689ced 100644
 --- a/drivers/usb/dwc3/ep0.c
 +++ b/drivers/usb/dwc3/ep0.c
 @@ -1123,7 +1123,21 @@ static void dwc3_ep0_xfernotready(struct dwc3 *dwc,
   dwc->ep0state = EP0_STATUS_PHASE;

   if (dwc->delayed_status) {
 + struct dwc3_ep *dep = dwc->eps[0];
 +
   WARN_ON_ONCE(event->endpoint_number != 1);
 + /*
 +  * We should handle the delay STATUS phase here if 
 the
 +  * request for handling delay STATUS has been queued
 +  * into the list.
 +  */
 + if (!list_empty(>pending_list)) {
 + dwc->delayed_status = false;
 + usb_gadget_set_state(>gadget,
 +  USB_STATE_CONFIGURED);
>>>
>>> Isn't this patch also changing the normal case when usb_ep_queue() comes
>>> later? I guess list_empty() protects against that...
>>
>> I think it will not change other cases, we only handle the delayed
>> status and I've tested it for a while and I did not find any problem.
>
> Alright, it's important enough to fix this bug. Can you also have a look
> into dropping USB_GADGET_DELAYED_STATUS altogether? If you're too busy,
> no issues. It'll stay in my queue.

Okay, I will have a look at f_mass_storage driver to see if we can
drop USB_GADGET_DELAYED_STATUS. Thanks.

-- 
Baolin.wang
Best Regards
--
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: [PATCH] usb: dwc3: ep0: Fix the possible missed request for handling delay STATUS phase

[Felipe Balbi]

Hi,

Baolin Wang  writes:
> Hi,
>
> On 16 January 2017 at 19:29, Felipe Balbi  wrote:
>>
>> Hi,
>>
>> Baolin Wang  writes:
>>> Hi,
>>>
>>> On 16 January 2017 at 18:56, Felipe Balbi  wrote:

 Hi,

 Baolin Wang  writes:
> When handing the SETUP packet by composite_setup(), we will release the
> dwc->lock. If we get the 'USB_GADGET_DELAYED_STATUS' result from setup
> function, which means we need to delay handling the STATUS phase.

 this sentence needs a little work. Seems like it's missing some
 information.

 anyway, I get that we release the lock but...

> But during the lock release period, maybe the request for handling delay

 execution of ->setup() itself should be locked. I can see that it's only
 locked for set_config() which is rather peculiar.

 What exact request you had when you triggered this? (Hint: dwc3
 tracepoints print out ctrl request bytes). IIRC, only set_config() or
 f->set_alt() can actually return USB_GADGET_DELAYED_STATUS.
>>>
>>> Yes, when host set configuration for mass storage driver, it can
>>> produce this issue.
>>>

 Which gadget driver were you using when you triggered this?
>>>
>>> mass storage driver. When host issues the setting config request, we
>>> will get USB_GADGET_DELAYED_STATUS result from
>>> set_config()--->fsg_set_alt(). Then the mass storage driver will issue
>>> one thread to complete the status stage by ep0_queue() (this thread
>>> may be running on another core), then if the thread issues ep0_queue()
>>> too fast before we get the dwc->lock in dwc3_ep0_delegate_req() or
>>> before we get into the STATUS stage, then we can not handle this
>>> request for the delay STATUS stage in dwc3_gadget_ep0_queue().
>>>

 Another point here is that the really robust way of fixing this is to
 get rid of USB_GADGET_DELAYED_STATUS altogether and just make sure
 gadget drivers know how to queue requests for all three phases of a
 Control Transfer.

 A lot of code will be removed from all gadget drivers and UDC drivers
 while combining all of it in a single place in composite.c.

 The reason I'm saying this is that other UDC drivers might have similar
 races already but they just haven't triggered.
>>>
>>> Yes, maybe.
>>>

> STATUS phase has been queued into list before we set 'dwc->delayed_status'
> flag or entering 'EP0_STATUS_PHASE' phase, then we will miss the chance
> to handle the STATUS phase. Thus we should check if the request for delay
> STATUS phase has been enqueued when entering 'EP0_STATUS_PHASE' phase in
> dwc3_ep0_xfernotready(), if so, we should handle it.
>
> Signed-off-by: Baolin Wang 
> ---
>  drivers/usb/dwc3/ep0.c |   14 ++
>  1 file changed, 14 insertions(+)
>
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 9bb1f85..e689ced 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -1123,7 +1123,21 @@ static void dwc3_ep0_xfernotready(struct dwc3 *dwc,
>   dwc->ep0state = EP0_STATUS_PHASE;
>
>   if (dwc->delayed_status) {
> + struct dwc3_ep *dep = dwc->eps[0];
> +
>   WARN_ON_ONCE(event->endpoint_number != 1);
> + /*
> +  * We should handle the delay STATUS phase here if 
> the
> +  * request for handling delay STATUS has been queued
> +  * into the list.
> +  */
> + if (!list_empty(>pending_list)) {
> + dwc->delayed_status = false;
> + usb_gadget_set_state(>gadget,
> +  USB_STATE_CONFIGURED);

 Isn't this patch also changing the normal case when usb_ep_queue() comes
 later? I guess list_empty() protects against that...
>>>
>>> I think it will not change other cases, we only handle the delayed
>>> status and I've tested it for a while and I did not find any problem.
>>
>> Alright, it's important enough to fix this bug. Can you also have a look
>> into dropping USB_GADGET_DELAYED_STATUS altogether? If you're too busy,
>> no issues. It'll stay in my queue.
>
> Okay, I will have a look at f_mass_storage driver to see if we can
> drop USB_GADGET_DELAYED_STATUS. Thanks.

not only mass storage. It needs to be done for all drivers. The way to
do that is to teach functions that control transfers are composed of two
or three phases. If you look at UDC drivers today, they all have
peculiarities about control transfers to handle stuff that *maybe*
gadget drivers won't handle.

What we should do here is make sure that *all* 3 

RE: [PATCH] usb: gadget: udc: atmel: used managed kasprintf

[Felipe Balbi]

Hi,

David Laight  writes:
> From: Alexandre Belloni
>> Sent: 02 December 2016 16:19
>> On 02/12/2016 at 15:59:57 +, David Laight wrote :
>> > From: Alexandre Belloni
>> > > Sent: 01 December 2016 10:27
>> > > Use devm_kasprintf instead of simple kasprintf to free the allocated 
>> > > memory
>> > > when needed.
>> >
>> > s/when needed/when the device is freed/
>> >
>> > > Suggested-by: Peter Rosin 
>> > > Signed-off-by: Alexandre Belloni 
>> > > ---
>> > >  drivers/usb/gadget/udc/atmel_usba_udc.c | 3 ++-
>> > >  1 file changed, 2 insertions(+), 1 deletion(-)
>> > >
>> > > diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c 
>> > > b/drivers/usb/gadget/udc/atmel_usba_udc.c
>> > > index 45bc997d0711..aec72fe8273c 100644
>> > > --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
>> > > +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
>> > > @@ -1978,7 +1978,8 @@ static struct usba_ep * atmel_udc_of_init(struct 
>> > > platform_device *pdev,
>> > >  dev_err(>dev, "of_probe: name 
>> > > error(%d)\n", ret);
>> > >  goto err;
>> > >  }
>> > > -ep->ep.name = kasprintf(GFP_KERNEL, "ep%d", ep->index);
>> > > +ep->ep.name = devm_kasprintf(>dev, GFP_KERNEL, 
>> > > "ep%d",
>> > > + ep->index);
>> >
>> > Acually why bother mallocing such a small string at all.
>> > The maximum length is 12 bytes even if 'index' are unrestricted.
>> >
>> 
>> IIRC, using statically allocated string is failing somewhere is the USB
>> core but I don't remember all the details.
>
> I can't imagine that changing ep->ep.name from 'char *' to 'char [12]' would
> make any difference.

the actual name is managed by the UDC. Meaning, ep->ep.name should be a
pointer, but it could very well just point to ep->name which would be
char [12].

-- 
balbi


signature.asc
Description: PGP signature

Re: Standard USB Descriptor question

[Greg KH]

On Sun, Jan 15, 2017 at 07:03:34PM -0800, bruce m beach wrote:
> On 1/13/17, Greg KH  wrote:
> > On Fri, Jan 13, 2017 at 07:24:10PM -0800, bruce m beach wrote:
> >> Hello everybody
> >>
> >>   I have been stuck for ages trying to figure out something
> >> in the Standard USB Descriptor during the enumeration. It
> >> looks like it should should be quite simple but I am not
> >> getting it because maybe some mind block, maybe becuase the
> >> documentation is unclear, or maybe because I'm brain-dead.
> >> Here is the problem:
> >>
> >> I think the host starts out requesting the device descriptor
> >> This is okay and works no problem. At some point the host
> >> requests the configuration descriptor and I get stuck. The
> >> usb 2.0 spec. says:
> >>
> >> "When the host requests the configuration descriptor, all
> >> related interface and endpoint descriptors are returned"
> >>
> >> How is the host supposed to know how much data is coming in
> >> or how much data to request until it reads wTotalLength?
> >>
> >> I would assume the host would request the 9 bytes of the
> >> configuration descriptor, then make subsequent requests to
> >> get the interfaces and endpoints but for instance in the
> >> section on the interface descriptor the spec says
> >>
> >> " An interface descriptor is always returned as part of a
> >> configuration descriptor.
> >>
> >> Again how is this supposed happen if the host is only
> >> requesting 9 bytes in the configuration descriptor stage
> >
> > Have you looked at the data on the wire?  The host asks for 9 bytes, and
> > then it uses the information there to get the "real" size of the
> > descriptor and then it asks for the full descriptor with all of the
> > information in it.
> >
> > hope this helps,
> >
> > greg k-h
> >
> 
>   Yes it helps. Makes sense. I made some changes and can get the
>   descriptors from a userland program so I know its working (i.e.
>   from userland I can upload firmware and have control over the
>   chip includeing the usb subsystem) but at this point after
>   a reset there is no re-unumeration. It justs lists the original
>   configuration.(using lsusb). Notice I am using ioctl's.

I don't understand how you can be using ioctls to enumerate a USB
device, that's the kernel's job, not userspace.

And what do you mean by "reset"?  What reset?  How?

What exactly are you trying to do here that the kernel is not already
providing for you?  What problem are you trying to solve?

thanks,

greg k-h
--
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: [PATCH 2/2] usb: gadget: uac2: add req_number as parameter

[Felipe Balbi]

Hi,

Peter Chen  writes:
> There are only two requests for uac2, it may not be enough at high
> loading system which usb interrupt handler can't be serviced on
> time, then the data will be lost since it is isoc transfer for audio.
>
> In this patch, we introduce a parameter for the number for usb request,
> and the user can override it if current number for request is not enough
> for his/her use case.
>
> Besides, update this parameter for legacy audio gadget and documentation.

I would rather just drop pre-allocation of requests. Every time Audio
wants transmit data you allocate and queue a request there and free()
the request on completion.

All of these functions already have a ton of parameters :-s

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v2] USB: cypress_m8: remove unused variable

[Johan Hovold]

On Tue, Jan 03, 2017 at 09:01:10PM +, Sudip Mukherjee wrote:
> From: Sudip Mukherjee 
> 
> The variable havedata was only being set but never used afterwards.
> 
> Signed-off-by: Sudip Mukherjee 
> ---
> 
> v2: changed the from line

Now applied, thanks.

Johan
--
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: [PATCH] usb: dwc3: ep0: Fix the possible missed request for handling delay STATUS phase

[Felipe Balbi]

Hi,

Baolin Wang  writes:
> Hi,
>
> On 16 January 2017 at 18:56, Felipe Balbi  wrote:
>>
>> Hi,
>>
>> Baolin Wang  writes:
>>> When handing the SETUP packet by composite_setup(), we will release the
>>> dwc->lock. If we get the 'USB_GADGET_DELAYED_STATUS' result from setup
>>> function, which means we need to delay handling the STATUS phase.
>>
>> this sentence needs a little work. Seems like it's missing some
>> information.
>>
>> anyway, I get that we release the lock but...
>>
>>> But during the lock release period, maybe the request for handling delay
>>
>> execution of ->setup() itself should be locked. I can see that it's only
>> locked for set_config() which is rather peculiar.
>>
>> What exact request you had when you triggered this? (Hint: dwc3
>> tracepoints print out ctrl request bytes). IIRC, only set_config() or
>> f->set_alt() can actually return USB_GADGET_DELAYED_STATUS.
>
> Yes, when host set configuration for mass storage driver, it can
> produce this issue.
>
>>
>> Which gadget driver were you using when you triggered this?
>
> mass storage driver. When host issues the setting config request, we
> will get USB_GADGET_DELAYED_STATUS result from
> set_config()--->fsg_set_alt(). Then the mass storage driver will issue
> one thread to complete the status stage by ep0_queue() (this thread
> may be running on another core), then if the thread issues ep0_queue()
> too fast before we get the dwc->lock in dwc3_ep0_delegate_req() or
> before we get into the STATUS stage, then we can not handle this
> request for the delay STATUS stage in dwc3_gadget_ep0_queue().
>
>>
>> Another point here is that the really robust way of fixing this is to
>> get rid of USB_GADGET_DELAYED_STATUS altogether and just make sure
>> gadget drivers know how to queue requests for all three phases of a
>> Control Transfer.
>>
>> A lot of code will be removed from all gadget drivers and UDC drivers
>> while combining all of it in a single place in composite.c.
>>
>> The reason I'm saying this is that other UDC drivers might have similar
>> races already but they just haven't triggered.
>
> Yes, maybe.
>
>>
>>> STATUS phase has been queued into list before we set 'dwc->delayed_status'
>>> flag or entering 'EP0_STATUS_PHASE' phase, then we will miss the chance
>>> to handle the STATUS phase. Thus we should check if the request for delay
>>> STATUS phase has been enqueued when entering 'EP0_STATUS_PHASE' phase in
>>> dwc3_ep0_xfernotready(), if so, we should handle it.
>>>
>>> Signed-off-by: Baolin Wang 
>>> ---
>>>  drivers/usb/dwc3/ep0.c |   14 ++
>>>  1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>>> index 9bb1f85..e689ced 100644
>>> --- a/drivers/usb/dwc3/ep0.c
>>> +++ b/drivers/usb/dwc3/ep0.c
>>> @@ -1123,7 +1123,21 @@ static void dwc3_ep0_xfernotready(struct dwc3 *dwc,
>>>   dwc->ep0state = EP0_STATUS_PHASE;
>>>
>>>   if (dwc->delayed_status) {
>>> + struct dwc3_ep *dep = dwc->eps[0];
>>> +
>>>   WARN_ON_ONCE(event->endpoint_number != 1);
>>> + /*
>>> +  * We should handle the delay STATUS phase here if the
>>> +  * request for handling delay STATUS has been queued
>>> +  * into the list.
>>> +  */
>>> + if (!list_empty(>pending_list)) {
>>> + dwc->delayed_status = false;
>>> + usb_gadget_set_state(>gadget,
>>> +  USB_STATE_CONFIGURED);
>>
>> Isn't this patch also changing the normal case when usb_ep_queue() comes
>> later? I guess list_empty() protects against that...
>
> I think it will not change other cases, we only handle the delayed
> status and I've tested it for a while and I did not find any problem.

Alright, it's important enough to fix this bug. Can you also have a look
into dropping USB_GADGET_DELAYED_STATUS altogether? If you're too busy,
no issues. It'll stay in my queue.

-- 
balbi


signature.asc
Description: PGP signature

[PATCH 3/4] usb: dwc3: gadget: allocate bounce buffer for unaligned xfers

[Felipe Balbi]

Allocate a coherent buffer of 1024 bytes (size of a single superspeed
bulk packet) to serve as bounce buffer for an extra TRB needed to align
transfers to wMaxPacketSize.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/dwc3/core.h   |  3 +++
 drivers/usb/dwc3/gadget.c | 16 ++--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 14b760209680..9b4b79ae0beb 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -40,6 +40,7 @@
 /* Global constants */
 #define DWC3_PULL_UP_TIMEOUT   500 /* ms */
 #define DWC3_ZLP_BUF_SIZE  1024/* size of a superspeed bulk */
+#define DWC3_BOUNCE_SIZE   1024/* size of a superspeed bulk */
 #define DWC3_EP0_BOUNCE_SIZE   512
 #define DWC3_ENDPOINTS_NUM 32
 #define DWC3_XHCI_RESOURCES_NUM2
@@ -857,12 +858,14 @@ struct dwc3_scratchpad_array {
 struct dwc3 {
struct usb_ctrlrequest  *ctrl_req;
struct dwc3_trb *ep0_trb;
+   void*bounce;
void*ep0_bounce;
void*zlp_buf;
void*scratchbuf;
u8  *setup_buf;
dma_addr_t  ctrl_req_addr;
dma_addr_t  ep0_trb_addr;
+   dma_addr_t  bounce_addr;
dma_addr_t  ep0_bounce_addr;
dma_addr_t  scratch_addr;
struct dwc3_request ep0_usb_req;
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index f1ffbf789e0a..653251751e38 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -3021,6 +3021,13 @@ int dwc3_gadget_init(struct dwc3 *dwc)
goto err4;
}
 
+   dwc->bounce = dma_alloc_coherent(dwc->sysdev, DWC3_BOUNCE_SIZE,
+   >bounce_addr, GFP_KERNEL);
+   if (!dwc->bounce) {
+   ret = -ENOMEM;
+   goto err5;
+   }
+
init_completion(>ep0_in_setup);
 
dwc->gadget.ops = _gadget_ops;
@@ -3064,15 +3071,18 @@ int dwc3_gadget_init(struct dwc3 *dwc)
 
ret = dwc3_gadget_init_endpoints(dwc);
if (ret)
-   goto err5;
+   goto err6;
 
ret = usb_add_gadget_udc(dwc->dev, >gadget);
if (ret) {
dev_err(dwc->dev, "failed to register udc\n");
-   goto err5;
+   goto err6;
}
 
return 0;
+err6:
+   dma_free_coherent(dwc->sysdev, DWC3_BOUNCE_SIZE, dwc->bounce,
+   dwc->bounce_addr);
 
 err5:
kfree(dwc->zlp_buf);
@@ -3105,6 +3115,8 @@ void dwc3_gadget_exit(struct dwc3 *dwc)
 
dwc3_gadget_free_endpoints(dwc);
 
+   dma_free_coherent(dwc->sysdev, DWC3_BOUNCE_SIZE, dwc->bounce,
+   dwc->bounce_addr);
dma_free_coherent(dwc->sysdev, DWC3_EP0_BOUNCE_SIZE,
dwc->ep0_bounce, dwc->ep0_bounce_addr);
 
-- 
2.11.0.295.gd7dffce1ce

--
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: [PATCH v2 09/14] USB: serial: ch341: fix control-message error handling

[Johan Hovold]

On Wed, Jan 11, 2017 at 12:08:23PM +0100, Johan Hovold wrote:
> On Fri, Jan 06, 2017 at 07:15:18PM +0100, Johan Hovold wrote:
> > A short control transfer would currently fail to be detected, something
> > which could lead to stale buffer data being used as valid input.
> > 
> > Check for short transfers, and make sure to log any transfer errors.
> > 
> > Fixes: 6ce76104781a ("USB: Driver for CH341 USB-serial adaptor")
> > Signed-off-by: Johan Hovold 
> > ---
> 
> As this can also lead to bits of uninitialised heap buffers leaking to
> user space via the modem-status interface, or to a remote device through
> break control, I'll CC stable and add this one for inclusion -rc4 as
> well.

The rest of the series is now applied for -next.

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

[PATCH 4/4] usb: dwc3: gadget: align transfers to wMaxPacketSize

[Felipe Balbi]

Instead of passing quirk_ep_out_aligned_size, we can use one extra TRB
to align transfer to wMaxPacketSize.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/dwc3/core.h   |  2 ++
 drivers/usb/dwc3/gadget.c | 69 +--
 2 files changed, 63 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 9b4b79ae0beb..2b9e4ca3c932 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -725,6 +725,7 @@ struct dwc3_hwparams {
  * @epnum: endpoint number to which this request refers
  * @trb: pointer to struct dwc3_trb
  * @trb_dma: DMA address of @trb
+ * @unaligned: true for OUT endpoints with length not divisible by maxp
  * @direction: IN or OUT direction flag
  * @mapped: true when request has been dma-mapped
  * @queued: true when request has been queued to HW
@@ -741,6 +742,7 @@ struct dwc3_request {
struct dwc3_trb *trb;
dma_addr_t  trb_dma;
 
+   unsignedunaligned:1;
unsigneddirection:1;
unsignedmapped:1;
unsignedstarted:1;
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 653251751e38..6faf484e5dfc 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -992,12 +992,33 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
int i;
 
for_each_sg(sg, s, req->num_pending_sgs, i) {
+   unsigned int length = req->request.length;
+   unsigned int maxp = usb_endpoint_maxp(dep->endpoint.desc);
+   unsigned int rem = length % maxp;
unsigned chain = true;
 
if (sg_is_last(s))
chain = false;
 
-   dwc3_prepare_one_trb(dep, req, chain, i);
+   if (rem && usb_endpoint_dir_out(dep->endpoint.desc) && !chain) {
+   struct dwc3 *dwc = dep->dwc;
+   struct dwc3_trb *trb;
+
+   req->unaligned = true;
+
+   /* prepare normal TRB */
+   dwc3_prepare_one_trb(dep, req, true, i);
+
+   /* Now prepare one extra TRB to align transfer size */
+   trb = >trb_pool[dep->trb_enqueue];
+   __dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr,
+   maxp - rem, false, 0,
+   req->request.stream_id,
+   req->request.short_not_ok,
+   req->request.no_interrupt);
+   } else {
+   dwc3_prepare_one_trb(dep, req, chain, i);
+   }
 
if (!dwc3_calc_trbs_left(dep))
break;
@@ -1007,7 +1028,28 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
 static void dwc3_prepare_one_trb_linear(struct dwc3_ep *dep,
struct dwc3_request *req)
 {
-   dwc3_prepare_one_trb(dep, req, false, 0);
+   unsigned int length = req->request.length;
+   unsigned int maxp = usb_endpoint_maxp(dep->endpoint.desc);
+   unsigned int rem = length % maxp;
+
+   if (rem && usb_endpoint_dir_out(dep->endpoint.desc)) {
+   struct dwc3 *dwc = dep->dwc;
+   struct dwc3_trb *trb;
+
+   req->unaligned = true;
+
+   /* prepare normal TRB */
+   dwc3_prepare_one_trb(dep, req, true, 0);
+
+   /* Now prepare one extra TRB to align transfer size */
+   trb = >trb_pool[dep->trb_enqueue];
+   __dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, maxp - rem,
+   false, 0, req->request.stream_id,
+   req->request.short_not_ok,
+   req->request.no_interrupt);
+   } else {
+   dwc3_prepare_one_trb(dep, req, false, 0);
+   }
 }
 
 /*
@@ -2031,6 +2073,16 @@ static int __dwc3_cleanup_done_trbs(struct dwc3 *dwc, 
struct dwc3_ep *dep,
if (chain && (trb->ctrl & DWC3_TRB_CTRL_HWO))
trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
 
+   /*
+* If we're dealing with unaligned size OUT transfer, we will be left
+* with one TRB pending in the ring. We need to manually clear HWO bit
+* from that TRB.
+*/
+   if (req->unaligned && (trb->ctrl & DWC3_TRB_CTRL_HWO)) {
+   trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
+   return 1;
+   }
+
if ((trb->ctrl & DWC3_TRB_CTRL_HWO) && status != -ESHUTDOWN)
return 1;
 
@@ -2120,6 +2172,13 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, 
struct dwc3_ep *dep,
event, status, chain);
}
 
+   if (req->unaligned) {
+   trb = 

[PATCH 1/4] usb: dwc3: gadget: simplify dwc3_prepare_one_trb()

[Felipe Balbi]

We are already passing struct dwc3_request * to dwc3_prepare_one_trb(),
because of that there's no need to extract dma address and length in the
caller. We can let dwc3_prepare_one_trb() itself handle that part.

This simplifies the prototype of the function by removing two arguments.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/dwc3/gadget.c | 22 +-
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 204c754cc647..cd410a720aa2 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -839,13 +839,14 @@ static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep);
  * @req: dwc3_request pointer
  */
 static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
-   struct dwc3_request *req, dma_addr_t dma,
-   unsigned length, unsigned chain, unsigned node)
+   struct dwc3_request *req, unsigned chain, unsigned node)
 {
struct dwc3_trb *trb;
struct dwc3 *dwc = dep->dwc;
struct usb_gadget   *gadget = >gadget;
enum usb_device_speed   speed = gadget->speed;
+   unsignedlength = req->request.length;
+   dma_addr_t  dma = req->request.dma;
 
trb = >trb_pool[dep->trb_enqueue];
 
@@ -974,21 +975,15 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
 {
struct scatterlist *sg = req->sg;
struct scatterlist *s;
-   unsigned intlength;
-   dma_addr_t  dma;
int i;
 
for_each_sg(sg, s, req->num_pending_sgs, i) {
unsigned chain = true;
 
-   length = sg_dma_len(s);
-   dma = sg_dma_address(s);
-
if (sg_is_last(s))
chain = false;
 
-   dwc3_prepare_one_trb(dep, req, dma, length,
-   chain, i);
+   dwc3_prepare_one_trb(dep, req, chain, i);
 
if (!dwc3_calc_trbs_left(dep))
break;
@@ -998,14 +993,7 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
 static void dwc3_prepare_one_trb_linear(struct dwc3_ep *dep,
struct dwc3_request *req)
 {
-   unsigned intlength;
-   dma_addr_t  dma;
-
-   dma = req->request.dma;
-   length = req->request.length;
-
-   dwc3_prepare_one_trb(dep, req, dma, length,
-   false, 0);
+   dwc3_prepare_one_trb(dep, req, false, 0);
 }
 
 /*
-- 
2.11.0.295.gd7dffce1ce

--
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: [PATCH] usb: dwc2: host: use msleep() for long delays

[Felipe Balbi]

Hi,

John Youn  writes:
> On 1/12/2017 7:52 AM, Nicholas Mc Guire wrote:
>> ulseep_range() uses hrtimers and provides no advantage over msleep()
>> for larger delays. Fix up the 20+ ms delays here passing the adjusted "min"
>> value to msleep(). This helps reduce the load on the hrtimer subsystem.
>> 
>> Signed-off-by: Nicholas Mc Guire 
>> ---
>> 
>> Patch was compile tested with: x86_64_defconfig + CONFIG_USB_DWC2
>> (1 sparse and 6 coccinelle warning - preparing separate patches for that)
>> 
>> Patch is against 4.10-rc3 (localversion-next is next-20170112)
>> 
>>  drivers/usb/dwc2/hcd.c | 12 ++--
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>> index 911c3b3..37ee72d 100644
>> --- a/drivers/usb/dwc2/hcd.c
>> +++ b/drivers/usb/dwc2/hcd.c
>> @@ -2150,7 +2150,7 @@ static int dwc2_hcd_endpoint_disable(struct dwc2_hsotg 
>> *hsotg,
>>  }
>>  
>>  spin_unlock_irqrestore(>lock, flags);
>> -usleep_range(2, 4);
>> +msleep(20);
>>  spin_lock_irqsave(>lock, flags);
>>  qh = ep->hcpriv;
>>  if (!qh) {
>> @@ -3240,7 +3240,7 @@ static void dwc2_conn_id_status_change(struct 
>> work_struct *work)
>>   "Waiting for Peripheral Mode, Mode=%s\n",
>>   dwc2_is_host_mode(hsotg) ? "Host" :
>>   "Peripheral");
>> -usleep_range(2, 4);
>> +msleep(20);
>>  if (++count > 250)
>>  break;
>>  }
>> @@ -3261,7 +3261,7 @@ static void dwc2_conn_id_status_change(struct 
>> work_struct *work)
>>  dev_info(hsotg->dev, "Waiting for Host Mode, Mode=%s\n",
>>   dwc2_is_host_mode(hsotg) ?
>>   "Host" : "Peripheral");
>> -usleep_range(2, 4);
>> +msleep(20);
>>  if (++count > 250)
>>  break;
>>  }
>> @@ -3354,7 +3354,7 @@ static void dwc2_port_suspend(struct dwc2_hsotg 
>> *hsotg, u16 windex)
>>  
>>  spin_unlock_irqrestore(>lock, flags);
>>  
>> -usleep_range(20, 25);
>> +msleep(200);
>>  } else {
>>  spin_unlock_irqrestore(>lock, flags);
>>  }
>> @@ -3378,7 +3378,7 @@ static void dwc2_port_resume(struct dwc2_hsotg *hsotg)
>>  pcgctl &= ~PCGCTL_STOPPCLK;
>>  dwc2_writel(pcgctl, hsotg->regs + PCGCTL);
>>  spin_unlock_irqrestore(>lock, flags);
>> -usleep_range(2, 4);
>> +msleep(20);
>>  spin_lock_irqsave(>lock, flags);
>>  }
>>  
>> @@ -3691,7 +3691,7 @@ static int dwc2_hcd_hub_control(struct dwc2_hsotg 
>> *hsotg, u16 typereq,
>>  }
>>  
>>  /* Clear reset bit in 10ms (FS/LS) or 50ms (HS) */
>> -usleep_range(5, 7);
>> +msleep(50);
>>  hprt0 &= ~HPRT0_RST;
>>  dwc2_writel(hprt0, hsotg->regs + HPRT0);
>>  hsotg->lx_state = DWC2_L0; /* Now back to On state */
>> 
>
> +Felipe
>
> Acked-by: John Youn 
>
> I've also fixed this up locally to apply against recent dwc2 patches
> for next.
>
> Hi Felipe,
>
> If/when you queue the pending dwc2 patches on testing/next I can
> resend this to you.

it actually applied just fine ;-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: core: Disable USB2.0 phy suspend when dwc3 acts as host role

[Felipe Balbi]

Hi,

John Youn  writes:
>> Baolin Wang  writes:
 Baolin Wang  writes:
> When dwc3 controller acts as host role with attaching slow speed device
> (like mouse or keypad). Then if we plugged out the slow speed device,
> it will timeout to run the deconfiguration endpoint command to drop the
> endpoint's resources. Some xHCI command timeout log as below when
> disconnecting one slow device:
>
> [   99.807739] c0 xhci-hcd.0.auto: Port Status Change Event for port 1
> [   99.814699] c0 xhci-hcd.0.auto: resume root hub
> [   99.819992] c0 xhci-hcd.0.auto: handle_port_status: starting port
>  polling.
> [   99.827808] c0 xhci-hcd.0.auto: get port status, actual port 0 status
>  = 0x202a0
> [   99.835903] c0 xhci-hcd.0.auto: Get port status returned 0x10100
> [   99.850052] c0 xhci-hcd.0.auto: clear port connect change, actual
>  port 0 status  = 0x2a0
> [   99.859313] c0 xhci-hcd.0.auto: Cancel URB ffc01ed6cd00, dev 1,
>  ep 0x81, starting at offset 0xc406d210
> [   99.869645] c0 xhci-hcd.0.auto: // Ding dong!
> [   99.874776] c0 xhci-hcd.0.auto: Stopped on Transfer TRB
> [   99.880713] c0 xhci-hcd.0.auto: Removing canceled TD starting at
>  0xc406d210 (dma).
> [   99.889012] c0 xhci-hcd.0.auto: Finding endpoint context
> [   99.895069] c0 xhci-hcd.0.auto: Cycle state = 0x1
> [   99.900519] c0 xhci-hcd.0.auto: New dequeue segment =
>  ffc1112f0880 (virtual)
> [   99.908655] c0 xhci-hcd.0.auto: New dequeue pointer = 0xc406d220 (DMA)
> [   99.915927] c0 xhci-hcd.0.auto: Set TR Deq Ptr cmd, new deq seg =
>  ffc1112f0880 (0xc406d000 dma),
>  new deq ptr = ff8002175220
>  (0xc406d220 dma), new cycle = 1
> [   99.931242] c0 xhci-hcd.0.auto: // Ding dong!
> [   99.936360] c0 xhci-hcd.0.auto: Successful Set TR Deq Ptr cmd,
>  deq = @c406d220
> [   99.944458] c0 xhci-hcd.0.auto: xhci_hub_status_data: stopping port
>  polling.
> [  100.047619] c0 xhci-hcd.0.auto: xhci_drop_endpoint called for udev
>  ffc01ae08800
> [  100.057002] c0 xhci-hcd.0.auto: drop ep 0x81, slot id 1, new drop
>  flags = 0x8, new add flags = 0x0
> [  100.067878] c0 xhci-hcd.0.auto: xhci_check_bandwidth called for udev
>  ffc01ae08800
> [  100.076868] c0 xhci-hcd.0.auto: New Input Control Context:
>
> ..
>
> [  100.427252] c0 xhci-hcd.0.auto: // Ding dong!
> [  105.430728] c0 xhci-hcd.0.auto: Command timeout
> [  105.436029] c0 xhci-hcd.0.auto: Abort command ring
> [  113.558223] c0 xhci-hcd.0.auto: Command completion event does not match
>  command
> [  113.569778] c0 xhci-hcd.0.auto: Timeout while waiting for configure
>  endpoint command
>
> The reason is it will suspend USB phy to disable phy clock when
> disconnecting the slow USB decice, that will hang on the xHCI commands
> executing which depends on the phy clock.
>
> Thus we should disable USB2.0 phy suspend feature when dwc3 acts as host
> role.
>
> Signed-off-by: Baolin Wang 
> ---
>  drivers/usb/dwc3/core.c |   14 ++
>  1 file changed, 14 insertions(+)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 9a4a5e4..0b646cf 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -565,6 +565,20 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
>   if (dwc->revision > DWC3_REVISION_194A)
>   reg |= DWC3_GUSB2PHYCFG_SUSPHY;
>
> + /*
> +  * When dwc3 controller acts as host role with attaching one slow 
> speed
> +  * device (like mouse or keypad). Then if we plugged out the slow 
> speed
> +  * device, it will timeout to run the deconfiguration endpoint 
> command.
> +  * The reason is it will suspend USB phy to disable phy clock when
> +  * disconnecting slow speed decice, which will affect the xHCI 
> commands
> +  * executing.
> +  *
> +  * Thus we should disable USB 2.0 phy suspend feature when dwc3 
> acts as
> +  * host role.
> +  */
> + if (dwc->dr_mode == USB_DR_MODE_HOST || dwc->dr_mode == 
> USB_DR_MODE_OTG)
> + reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;

 which version of 

Re: [PATCH] usb: dwc3: ep0: Fix the possible missed request for handling delay STATUS phase

[Felipe Balbi]

Hi,

Baolin Wang  writes:
> When handing the SETUP packet by composite_setup(), we will release the
> dwc->lock. If we get the 'USB_GADGET_DELAYED_STATUS' result from setup
> function, which means we need to delay handling the STATUS phase.

this sentence needs a little work. Seems like it's missing some
information.

anyway, I get that we release the lock but...

> But during the lock release period, maybe the request for handling delay

execution of ->setup() itself should be locked. I can see that it's only
locked for set_config() which is rather peculiar.

What exact request you had when you triggered this? (Hint: dwc3
tracepoints print out ctrl request bytes). IIRC, only set_config() or
f->set_alt() can actually return USB_GADGET_DELAYED_STATUS.

Which gadget driver were you using when you triggered this?

Another point here is that the really robust way of fixing this is to
get rid of USB_GADGET_DELAYED_STATUS altogether and just make sure
gadget drivers know how to queue requests for all three phases of a
Control Transfer.

A lot of code will be removed from all gadget drivers and UDC drivers
while combining all of it in a single place in composite.c.

The reason I'm saying this is that other UDC drivers might have similar
races already but they just haven't triggered.

> STATUS phase has been queued into list before we set 'dwc->delayed_status'
> flag or entering 'EP0_STATUS_PHASE' phase, then we will miss the chance
> to handle the STATUS phase. Thus we should check if the request for delay
> STATUS phase has been enqueued when entering 'EP0_STATUS_PHASE' phase in
> dwc3_ep0_xfernotready(), if so, we should handle it.
>
> Signed-off-by: Baolin Wang 
> ---
>  drivers/usb/dwc3/ep0.c |   14 ++
>  1 file changed, 14 insertions(+)
>
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 9bb1f85..e689ced 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -1123,7 +1123,21 @@ static void dwc3_ep0_xfernotready(struct dwc3 *dwc,
>   dwc->ep0state = EP0_STATUS_PHASE;
>  
>   if (dwc->delayed_status) {
> + struct dwc3_ep *dep = dwc->eps[0];
> +
>   WARN_ON_ONCE(event->endpoint_number != 1);
> + /*
> +  * We should handle the delay STATUS phase here if the
> +  * request for handling delay STATUS has been queued
> +  * into the list.
> +  */
> + if (!list_empty(>pending_list)) {
> + dwc->delayed_status = false;
> + usb_gadget_set_state(>gadget,
> +  USB_STATE_CONFIGURED);

Isn't this patch also changing the normal case when usb_ep_queue() comes
later? I guess list_empty() protects against that...

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: ep0: Fix the possible missed request for handling delay STATUS phase

[Baolin Wang]

Hi,

On 16 January 2017 at 18:56, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  writes:
>> When handing the SETUP packet by composite_setup(), we will release the
>> dwc->lock. If we get the 'USB_GADGET_DELAYED_STATUS' result from setup
>> function, which means we need to delay handling the STATUS phase.
>
> this sentence needs a little work. Seems like it's missing some
> information.
>
> anyway, I get that we release the lock but...
>
>> But during the lock release period, maybe the request for handling delay
>
> execution of ->setup() itself should be locked. I can see that it's only
> locked for set_config() which is rather peculiar.
>
> What exact request you had when you triggered this? (Hint: dwc3
> tracepoints print out ctrl request bytes). IIRC, only set_config() or
> f->set_alt() can actually return USB_GADGET_DELAYED_STATUS.

Yes, when host set configuration for mass storage driver, it can
produce this issue.

>
> Which gadget driver were you using when you triggered this?

mass storage driver. When host issues the setting config request, we
will get USB_GADGET_DELAYED_STATUS result from
set_config()--->fsg_set_alt(). Then the mass storage driver will issue
one thread to complete the status stage by ep0_queue() (this thread
may be running on another core), then if the thread issues ep0_queue()
too fast before we get the dwc->lock in dwc3_ep0_delegate_req() or
before we get into the STATUS stage, then we can not handle this
request for the delay STATUS stage in dwc3_gadget_ep0_queue().

>
> Another point here is that the really robust way of fixing this is to
> get rid of USB_GADGET_DELAYED_STATUS altogether and just make sure
> gadget drivers know how to queue requests for all three phases of a
> Control Transfer.
>
> A lot of code will be removed from all gadget drivers and UDC drivers
> while combining all of it in a single place in composite.c.
>
> The reason I'm saying this is that other UDC drivers might have similar
> races already but they just haven't triggered.

Yes, maybe.

>
>> STATUS phase has been queued into list before we set 'dwc->delayed_status'
>> flag or entering 'EP0_STATUS_PHASE' phase, then we will miss the chance
>> to handle the STATUS phase. Thus we should check if the request for delay
>> STATUS phase has been enqueued when entering 'EP0_STATUS_PHASE' phase in
>> dwc3_ep0_xfernotready(), if so, we should handle it.
>>
>> Signed-off-by: Baolin Wang 
>> ---
>>  drivers/usb/dwc3/ep0.c |   14 ++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>> index 9bb1f85..e689ced 100644
>> --- a/drivers/usb/dwc3/ep0.c
>> +++ b/drivers/usb/dwc3/ep0.c
>> @@ -1123,7 +1123,21 @@ static void dwc3_ep0_xfernotready(struct dwc3 *dwc,
>>   dwc->ep0state = EP0_STATUS_PHASE;
>>
>>   if (dwc->delayed_status) {
>> + struct dwc3_ep *dep = dwc->eps[0];
>> +
>>   WARN_ON_ONCE(event->endpoint_number != 1);
>> + /*
>> +  * We should handle the delay STATUS phase here if the
>> +  * request for handling delay STATUS has been queued
>> +  * into the list.
>> +  */
>> + if (!list_empty(>pending_list)) {
>> + dwc->delayed_status = false;
>> + usb_gadget_set_state(>gadget,
>> +  USB_STATE_CONFIGURED);
>
> Isn't this patch also changing the normal case when usb_ep_queue() comes
> later? I guess list_empty() protects against that...

I think it will not change other cases, we only handle the delayed
status and I've tested it for a while and I did not find any problem.

-- 
Baolin.wang
Best Regards
--
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

[PATCH 2/4] usb: dwc3: gadget: extract __dwc3_prepare_one_trb()

[Felipe Balbi]

This new internal function will be used to solve a minor issue with dwc3
which exists in regards to short packets with OUT endpoints. Currently
we're asking gadget driver to *always* send us aligned requests; however
if we have enough TRBs we can easily append one extra TRB chained to the
previous and keep a throw away 1024 byte buffer around for that.

The actual fix will come in a separate patch, this is merely in
preparation for such fix.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/dwc3/gadget.c | 58 +--
 1 file changed, 36 insertions(+), 22 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index cd410a720aa2..f1ffbf789e0a 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -833,29 +833,13 @@ static void dwc3_gadget_ep_free_request(struct usb_ep *ep,
 
 static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep);
 
-/**
- * dwc3_prepare_one_trb - setup one TRB from one request
- * @dep: endpoint for which this request is prepared
- * @req: dwc3_request pointer
- */
-static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
-   struct dwc3_request *req, unsigned chain, unsigned node)
+static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb,
+   dma_addr_t dma, unsigned length, unsigned chain, unsigned node,
+   unsigned stream_id, unsigned short_not_ok, unsigned 
no_interrupt)
 {
-   struct dwc3_trb *trb;
struct dwc3 *dwc = dep->dwc;
struct usb_gadget   *gadget = >gadget;
enum usb_device_speed   speed = gadget->speed;
-   unsignedlength = req->request.length;
-   dma_addr_t  dma = req->request.dma;
-
-   trb = >trb_pool[dep->trb_enqueue];
-
-   if (!req->trb) {
-   dwc3_gadget_move_started_request(req);
-   req->trb = trb;
-   req->trb_dma = dwc3_trb_dma_offset(dep, trb);
-   dep->queued_requests++;
-   }
 
dwc3_ep_inc_enq(dep);
 
@@ -901,11 +885,11 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
if (usb_endpoint_dir_out(dep->endpoint.desc)) {
trb->ctrl |= DWC3_TRB_CTRL_CSP;
 
-   if (req->request.short_not_ok)
+   if (short_not_ok)
trb->ctrl |= DWC3_TRB_CTRL_ISP_IMI;
}
 
-   if ((!req->request.no_interrupt && !chain) ||
+   if ((!no_interrupt && !chain) ||
(dwc3_calc_trbs_left(dep) == 0))
trb->ctrl |= DWC3_TRB_CTRL_IOC;
 
@@ -913,7 +897,7 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
trb->ctrl |= DWC3_TRB_CTRL_CHN;
 
if (usb_endpoint_xfer_bulk(dep->endpoint.desc) && dep->stream_capable)
-   trb->ctrl |= DWC3_TRB_CTRL_SID_SOFN(req->request.stream_id);
+   trb->ctrl |= DWC3_TRB_CTRL_SID_SOFN(stream_id);
 
trb->ctrl |= DWC3_TRB_CTRL_HWO;
 
@@ -921,6 +905,36 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
 }
 
 /**
+ * dwc3_prepare_one_trb - setup one TRB from one request
+ * @dep: endpoint for which this request is prepared
+ * @req: dwc3_request pointer
+ * @chain: should this TRB be chained to the next?
+ * @node: only for isochronous endpoints. First TRB needs different type.
+ */
+static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
+   struct dwc3_request *req, unsigned chain, unsigned node)
+{
+   struct dwc3_trb *trb;
+   unsignedlength = req->request.length;
+   unsignedstream_id = req->request.stream_id;
+   unsignedshort_not_ok = req->request.short_not_ok;
+   unsignedno_interrupt = req->request.no_interrupt;
+   dma_addr_t  dma = req->request.dma;
+
+   trb = >trb_pool[dep->trb_enqueue];
+
+   if (!req->trb) {
+   dwc3_gadget_move_started_request(req);
+   req->trb = trb;
+   req->trb_dma = dwc3_trb_dma_offset(dep, trb);
+   dep->queued_requests++;
+   }
+
+   __dwc3_prepare_one_trb(dep, trb, dma, length, chain, node,
+   stream_id, short_not_ok, no_interrupt);
+}
+
+/**
  * dwc3_ep_prev_trb() - Returns the previous TRB in the ring
  * @dep: The endpoint with the TRB ring
  * @index: The index of the current TRB in the ring
-- 
2.11.0.295.gd7dffce1ce

--
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: [PATCH 6/6] USB: serial: kl5kusb105: fix port-settings error handling

[Johan Hovold]

On Tue, Jan 10, 2017 at 12:05:42PM +0100, Johan Hovold wrote:
> Fix port-settings error handling in order to detect a short transfer.
> 
> Note that this currently only implies that an error is logged.
> 
> Signed-off-by: Johan Hovold 
>
> ---
>  drivers/usb/serial/kl5kusb105.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/serial/kl5kusb105.c b/drivers/usb/serial/kl5kusb105.c
> index 595415e59d5d..9b27f3d6029f 100644
> --- a/drivers/usb/serial/kl5kusb105.c
> +++ b/drivers/usb/serial/kl5kusb105.c
> @@ -138,9 +138,11 @@ static int klsi_105_chg_port_settings(struct 
> usb_serial_port *port,
>   settings,
>   sizeof(struct klsi_105_port_settings),
>   KLSI_TIMEOUT);
> - if (rc < 0)
> - dev_err(>dev,
> - "Change port settings failed (error = %d)\n", rc);
> + if (rc < sizeof(*settings)) {
> + dev_err(>dev, "failed to change port settings: %d\n", rc);
> + if (rc >= 0)
> + rc = -EIO;
> + }

I'm dropping this one, since a short out transfer should return a proper
errno.

Rest applied for -next.

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

[PATCH] media/usb: Fix pctv452e

[Wolfgang Rohdewald]

Commit 73d5c5c864f40 made sure that media/usb drivers don't do DMA on stack.

That made pctv452e oops while initialising:

[...]
BUG: unable to handle kernel NULL pointer dereference at   (null)
[...]
[9.051527]  [] mutex_lock+0x17/0x30
[9.051531]  [] pctv452e_power_ctrl+0x85/0x190 
[dvb_usb_pctv452e]
[9.051536]  [] dvb_usb_device_power_ctrl+0x3f/0x50 
[dvb_usb]
[9.051541]  [] dvb_usb_device_init+0x225/0x620 [dvb_usb]
[9.051546]  [] pctv452e_usb_probe+0x51/0x60 
[dvb_usb_pctv452e]
[9.051550]  [] usb_probe_interface+0x159/0x2d0
[...]

Commit 7724325a1 fixed most drivers but not all - I guess that only
those using data_mutex for locking got fixes. But some gave the
mutex a different name, in pctv452e it was ca_mutex.

This patches only pctv452e but grep mutex_lock in usb/dvb-usb makes me
believe that more drivers need to be fixed.

Fixes: 73d5c5c8 ("[media] pctv452e: don't do DMA on stack")
Signed-off-by: Wolfgang Rohdewald 
CC: Mauro Carvalho Chehab 
---
 drivers/media/usb/dvb-usb/pctv452e.c | 31 +++
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/media/usb/dvb-usb/pctv452e.c 
b/drivers/media/usb/dvb-usb/pctv452e.c
index 07fa08b..5f9cd32 100644
--- a/drivers/media/usb/dvb-usb/pctv452e.c
+++ b/drivers/media/usb/dvb-usb/pctv452e.c
@@ -92,7 +92,6 @@ static struct stb0899_postproc pctv45e_postproc[] = {
  */
 struct pctv452e_state {
struct dvb_ca_en50221 ca;
-   struct mutex ca_mutex;
 
u8 c;  /* transaction counter, wraps around...  */
u8 initialized; /* set to 1 if 0x15 has been sent */
@@ -114,7 +113,7 @@ static int tt3650_ci_msg(struct dvb_usb_device *d, u8 cmd, 
u8 *data,
return -EIO;
}
 
-   mutex_lock(>ca_mutex);
+   mutex_lock(>data_mutex);
id = state->c++;
 
state->data[0] = SYNC_BYTE_OUT;
@@ -136,14 +135,14 @@ static int tt3650_ci_msg(struct dvb_usb_device *d, u8 
cmd, u8 *data,
 
memcpy(data, state->data + 4, read_len);
 
-   mutex_unlock(>ca_mutex);
+   mutex_unlock(>data_mutex);
return 0;
 
 failed:
err("CI error %d; %02X %02X %02X -> %*ph.",
 ret, SYNC_BYTE_OUT, id, cmd, 3, state->data);
 
-   mutex_unlock(>ca_mutex);
+   mutex_unlock(>data_mutex);
return ret;
 }
 
@@ -155,9 +154,9 @@ static int tt3650_ci_msg_locked(struct dvb_ca_en50221 *ca,
struct pctv452e_state *state = (struct pctv452e_state *)d->priv;
int ret;
 
-   mutex_lock(>ca_mutex);
+   mutex_lock(>data_mutex);
ret = tt3650_ci_msg(d, cmd, data, write_len, read_len);
-   mutex_unlock(>ca_mutex);
+   mutex_unlock(>data_mutex);
 
return ret;
 }
@@ -296,7 +295,7 @@ static int tt3650_ci_slot_reset(struct dvb_ca_en50221 *ca, 
int slot)
 
buf[0] = 0;
 
-   mutex_lock(>ca_mutex);
+   mutex_lock(>data_mutex);
 
ret = tt3650_ci_msg(d, TT3650_CMD_CI_RESET, buf, 1, 1);
if (0 != ret)
@@ -317,7 +316,7 @@ static int tt3650_ci_slot_reset(struct dvb_ca_en50221 *ca, 
int slot)
ret = tt3650_ci_msg(d, TT3650_CMD_CI_SET_VIDEO_PORT, buf, 1, 1);
 
  failed:
-   mutex_unlock(>ca_mutex);
+   mutex_unlock(>data_mutex);
 
return ret;
 }
@@ -376,7 +375,7 @@ static int tt3650_ci_init(struct dvb_usb_adapter *a)
 
ci_dbg("%s", __func__);
 
-   mutex_init(>ca_mutex);
+   mutex_init(>data_mutex);
 
state->ca.owner = THIS_MODULE;
state->ca.read_attribute_mem = tt3650_ci_read_attribute_mem;
@@ -413,7 +412,7 @@ static int pctv452e_i2c_msg(struct dvb_usb_device *d, u8 
addr,
u8 id;
int ret;
 
-   mutex_lock(>ca_mutex);
+   mutex_lock(>data_mutex);
id = state->c++;
 
ret = -EINVAL;
@@ -447,7 +446,7 @@ static int pctv452e_i2c_msg(struct dvb_usb_device *d, u8 
addr,
goto failed;
 
memcpy(rcv_buf, state->data + 7, rcv_len);
-   mutex_unlock(>ca_mutex);
+   mutex_unlock(>data_mutex);
 
return rcv_len;
 
@@ -456,7 +455,7 @@ static int pctv452e_i2c_msg(struct dvb_usb_device *d, u8 
addr,
 ret, SYNC_BYTE_OUT, id, addr << 1, snd_len, rcv_len,
 7, state->data);
 
-   mutex_unlock(>ca_mutex);
+   mutex_unlock(>data_mutex);
return ret;
 }
 
@@ -520,7 +519,7 @@ static int pctv452e_power_ctrl(struct dvb_usb_device *d, 
int i)
if (!rx)
return -ENOMEM;
 
-   mutex_lock(>ca_mutex);
+   mutex_lock(>data_mutex);
/* hmm where shoud this should go? */
ret = usb_set_interface(d->udev, 0, ISOC_INTERFACE_ALTERNATIVE);
if (ret != 0)
@@ -548,7 +547,7 @@ static int pctv452e_power_ctrl(struct dvb_usb_device *d, 
int i)
state->initialized = 1;
 
 ret:
-   mutex_unlock(>ca_mutex);
+   mutex_unlock(>data_mutex);
kfree(rx);
return ret;
 }
@@ -559,7 +558,7 @@ static int pctv452e_rc_query(struct 

[PATCH v3 3/6] usb: phy: omap-otg: Replace the extcon API

[Chanwoo Choi]

This patch uses the resource-managed extcon API for extcon_register_notifier()
and replaces the deprecated extcon API as following:
- extcon_get_cable_state_() -> extcon_get_state()

Cc: linux-o...@vger.kernel.org
Signed-off-by: Chanwoo Choi 
Acked-by: Felipe Balbi 
---
 drivers/usb/phy/phy-omap-otg.c | 24 ++--
 1 file changed, 6 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/phy/phy-omap-otg.c b/drivers/usb/phy/phy-omap-otg.c
index 6523af4f8f93..800d1d90753d 100644
--- a/drivers/usb/phy/phy-omap-otg.c
+++ b/drivers/usb/phy/phy-omap-otg.c
@@ -118,19 +118,19 @@ static int omap_otg_probe(struct platform_device *pdev)
otg_dev->id_nb.notifier_call = omap_otg_id_notifier;
otg_dev->vbus_nb.notifier_call = omap_otg_vbus_notifier;
 
-   ret = extcon_register_notifier(extcon, EXTCON_USB_HOST, 
_dev->id_nb);
+   ret = devm_extcon_register_notifier(>dev, extcon,
+   EXTCON_USB_HOST, _dev->id_nb);
if (ret)
return ret;
 
-   ret = extcon_register_notifier(extcon, EXTCON_USB, _dev->vbus_nb);
+   ret = devm_extcon_register_notifier(>dev, extcon,
+   EXTCON_USB, _dev->vbus_nb);
if (ret) {
-   extcon_unregister_notifier(extcon, EXTCON_USB_HOST,
-   _dev->id_nb);
return ret;
}
 
-   otg_dev->id = extcon_get_cable_state_(extcon, EXTCON_USB_HOST);
-   otg_dev->vbus = extcon_get_cable_state_(extcon, EXTCON_USB);
+   otg_dev->id = extcon_get_state(extcon, EXTCON_USB_HOST);
+   otg_dev->vbus = extcon_get_state(extcon, EXTCON_USB);
omap_otg_set_mode(otg_dev);
 
rev = readl(otg_dev->base);
@@ -145,20 +145,8 @@ static int omap_otg_probe(struct platform_device *pdev)
return 0;
 }
 
-static int omap_otg_remove(struct platform_device *pdev)
-{
-   struct otg_device *otg_dev = platform_get_drvdata(pdev);
-   struct extcon_dev *edev = otg_dev->extcon;
-
-   extcon_unregister_notifier(edev, EXTCON_USB_HOST, _dev->id_nb);
-   extcon_unregister_notifier(edev, EXTCON_USB, _dev->vbus_nb);
-
-   return 0;
-}
-
 static struct platform_driver omap_otg_driver = {
.probe  = omap_otg_probe,
-   .remove = omap_otg_remove,
.driver = {
.name   = "omap_otg",
},
-- 
1.9.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

[PATCH v3 1/6] usb: dwc3: omap: Replace the extcon API

[Chanwoo Choi]

This patch uses the resource-managed extcon API for extcon_register_notifier()
and replaces the deprecated extcon API as following:
- extcon_get_cable_state_() -> extcon_get_state()

Cc: linux-o...@vger.kernel.org
Signed-off-by: Chanwoo Choi 
Acked-by: Felipe Balbi 
---
 drivers/usb/dwc3/dwc3-omap.c | 20 +++-
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
index eb1b9cb3f9d1..2092e46b1380 100644
--- a/drivers/usb/dwc3/dwc3-omap.c
+++ b/drivers/usb/dwc3/dwc3-omap.c
@@ -426,20 +426,20 @@ static int dwc3_omap_extcon_register(struct dwc3_omap 
*omap)
}
 
omap->vbus_nb.notifier_call = dwc3_omap_vbus_notifier;
-   ret = extcon_register_notifier(edev, EXTCON_USB,
-   >vbus_nb);
+   ret = devm_extcon_register_notifier(omap->dev, edev,
+   EXTCON_USB, >vbus_nb);
if (ret < 0)
dev_vdbg(omap->dev, "failed to register notifier for 
USB\n");
 
omap->id_nb.notifier_call = dwc3_omap_id_notifier;
-   ret = extcon_register_notifier(edev, EXTCON_USB_HOST,
-   >id_nb);
+   ret = devm_extcon_register_notifier(omap->dev, edev,
+   EXTCON_USB_HOST, >id_nb);
if (ret < 0)
dev_vdbg(omap->dev, "failed to register notifier for 
USB-HOST\n");
 
-   if (extcon_get_cable_state_(edev, EXTCON_USB) == true)
+   if (extcon_get_state(edev, EXTCON_USB) == true)
dwc3_omap_set_mailbox(omap, OMAP_DWC3_VBUS_VALID);
-   if (extcon_get_cable_state_(edev, EXTCON_USB_HOST) == true)
+   if (extcon_get_state(edev, EXTCON_USB_HOST) == true)
dwc3_omap_set_mailbox(omap, OMAP_DWC3_ID_GROUND);
 
omap->edev = edev;
@@ -528,17 +528,13 @@ static int dwc3_omap_probe(struct platform_device *pdev)
ret = of_platform_populate(node, NULL, NULL, dev);
if (ret) {
dev_err(>dev, "failed to create dwc3 core\n");
-   goto err2;
+   goto err1;
}
 
dwc3_omap_enable_irqs(omap);
enable_irq(omap->irq);
return 0;
 
-err2:
-   extcon_unregister_notifier(omap->edev, EXTCON_USB, >vbus_nb);
-   extcon_unregister_notifier(omap->edev, EXTCON_USB_HOST, >id_nb);
-
 err1:
pm_runtime_put_sync(dev);
pm_runtime_disable(dev);
@@ -550,8 +546,6 @@ static int dwc3_omap_remove(struct platform_device *pdev)
 {
struct dwc3_omap*omap = platform_get_drvdata(pdev);
 
-   extcon_unregister_notifier(omap->edev, EXTCON_USB, >vbus_nb);
-   extcon_unregister_notifier(omap->edev, EXTCON_USB_HOST, >id_nb);
dwc3_omap_disable_irqs(omap);
disable_irq(omap->irq);
of_platform_depopulate(omap->dev);
-- 
1.9.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: [PATCH] usb: dwc3 dwc3_exynos_probe() change goto labels to meaningful names

[Felipe Balbi]

Hi,

Shuah Khan  writes:
> Change goto labels to meaningful names from a series of errNs.
>
> Signed-off-by: Shuah Khan 

doesn't apply to testing/next, please rebase.

-- 
balbi


signature.asc
Description: PGP signature

Re: Memory barrier needed with wake_up_process()?

[Felipe Balbi]

Hi,

Alan Stern  writes:
> On Tue, 20 Sep 2016, Felipe Balbi wrote:
>
>> And here's trace output (complete, scroll to bottom). It seems to me
>> like the thread didn't wake up at all. It didn't even try to
>> execute. I'll add some more traces and try to get better information
>> about what's going on.
>> 
>> 
>> 
>> # tracer: nop
>> #
>> # entries-in-buffer/entries-written: 2865/2865   #P:4
>> #
>> #  _-=> irqs-off
>> # / _=> need-resched
>> #| / _---=> hardirq/softirq
>> #|| / _--=> preempt-depth
>> #||| / delay
>> #   TASK-PID   CPU#  TIMESTAMP  FUNCTION
>> #  | |   |      | |
>
> Skipping to the end...
>
>>  irq/17-dwc3-2522  [002] d...43.504199: bulk_out_complete: 0, 31/31
>> file-storage-2521  [001] 43.504202: fsg_main_thread: 
>> get_next_command -> 0
>> file-storage-2521  [001] 43.504288: fsg_main_thread: 
>> do_scsi_command -> 0
>> file-storage-2521  [001] 43.504295: fsg_main_thread: 
>> finish_reply -> 0
>> file-storage-2521  [001] 43.504298: fsg_main_thread: send_status 
>> -> 0
>>  irq/17-dwc3-2522  [002] d...43.504347: bulk_in_complete: 0, 
>> 16384/16384
>>  irq/17-dwc3-2522  [002] d...43.504351: bulk_in_complete: 0, 
>> 16384/16384
>>  irq/17-dwc3-2522  [002] d...43.504434: bulk_in_complete: 0, 
>> 16384/16384
>>  irq/17-dwc3-2522  [002] d...43.504438: bulk_in_complete: 0, 
>> 16384/16384
>>  irq/17-dwc3-2522  [002] d...43.504535: bulk_in_complete: 0, 
>> 16384/16384
>>  irq/17-dwc3-2522  [002] d...43.504539: bulk_in_complete: 0, 
>> 16384/16384
>>  irq/17-dwc3-2522  [002] d...43.504618: bulk_in_complete: 0, 
>> 16384/16384
>>  irq/17-dwc3-2522  [002] d...43.504703: bulk_in_complete: 0, 
>> 16384/16384
>>  irq/17-dwc3-2522  [002] d...43.504794: bulk_in_complete: 0, 13/13
>>  irq/17-dwc3-2522  [002] d...43.504797: bulk_out_complete: 0, 31/31
>
> Like you say, it appears that the thread didn't get woken up at all.
> But this is inconsistent with your earlier results.  On Sep. 9, you 
> posted a message that ended with these lines:
>
>>  irq/17-dwc3-3579  [003] d..1 21167.729666: bulk_out_complete: compl: bh 
>> 880111e6aac0 state 1
>> file-storage-3578  [002]  21167.729670: fsg_main_thread: next: bh 
>> 880111e6aac0 state 1
>
> This indicates that in the earlier test, the thread did start running 
> and get_next_command should have returned.
>
> The trace you posted after this one doesn't seem to show anything new, 
> as far as I can tell.
>
> So I still can't tell what's happening.  Maybe the patch below will 
> help.  It concentrates on the critical area.

Sorry for the long delay, I finally have more information on this. All
this time I was doing something that I never considered to matter: I've
been running host and peripheral on the same machine. Now that I have
tracepoints on xHCI as well, I could see that these 30 seconds of
"nothing" is actuall full of xHCI activity and I can see that for the
duration of these 30 seconds preempt depth on the CPU that (eventually)
queues a request on dwc3, is always > 1 (sometimes 2, most of the time
1). My conclusion from that is that xHCI (or usbcore ?!?) locks the CPU
and g_mass_storage is spinning for over 30 seconds at which point
storage.ko (host side class driver) dequeues the request.

I'll see if I can capture a fresh trace with both xHCI and dwc3 with
this happening, but probably not today (testing stuff for -rc).

-- 
balbi


signature.asc
Description: PGP signature

[PATCH v3 4/6] usb: phy: qcom-8x16-usb: Replace the extcon API

[Chanwoo Choi]

This patch uses the resource-managed extcon API for extcon_register_notifier()
and replaces the deprecated extcon API as following:
- extcon_get_cable_state_() -> extcon_get_state()

Signed-off-by: Chanwoo Choi 
Acked-by: Felipe Balbi 
---
 drivers/usb/phy/phy-qcom-8x16-usb.c | 13 -
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/phy/phy-qcom-8x16-usb.c 
b/drivers/usb/phy/phy-qcom-8x16-usb.c
index d8593adb3621..fdf686398772 100644
--- a/drivers/usb/phy/phy-qcom-8x16-usb.c
+++ b/drivers/usb/phy/phy-qcom-8x16-usb.c
@@ -187,7 +187,7 @@ static int phy_8x16_init(struct usb_phy *phy)
val = ULPI_PWR_OTG_COMP_DISABLE;
usb_phy_io_write(phy, val, ULPI_SET(ULPI_PWR_CLK_MNG_REG));
 
-   state = extcon_get_cable_state_(qphy->vbus_edev, EXTCON_USB);
+   state = extcon_get_state(qphy->vbus_edev, EXTCON_USB);
if (state)
phy_8x16_vbus_on(qphy);
else
@@ -316,23 +316,20 @@ static int phy_8x16_probe(struct platform_device *pdev)
goto off_clks;
 
qphy->vbus_notify.notifier_call = phy_8x16_vbus_notify;
-   ret = extcon_register_notifier(qphy->vbus_edev, EXTCON_USB,
-  >vbus_notify);
+   ret = devm_extcon_register_notifier(>dev, qphy->vbus_edev,
+   EXTCON_USB, >vbus_notify);
if (ret < 0)
goto off_power;
 
ret = usb_add_phy_dev(>phy);
if (ret)
-   goto off_extcon;
+   goto off_power;
 
qphy->reboot_notify.notifier_call = phy_8x16_reboot_notify;
register_reboot_notifier(>reboot_notify);
 
return 0;
 
-off_extcon:
-   extcon_unregister_notifier(qphy->vbus_edev, EXTCON_USB,
-  >vbus_notify);
 off_power:
regulator_bulk_disable(ARRAY_SIZE(qphy->regulator), qphy->regulator);
 off_clks:
@@ -347,8 +344,6 @@ static int phy_8x16_remove(struct platform_device *pdev)
struct phy_8x16 *qphy = platform_get_drvdata(pdev);
 
unregister_reboot_notifier(>reboot_notify);
-   extcon_unregister_notifier(qphy->vbus_edev, EXTCON_USB,
-  >vbus_notify);
 
/*
 * Ensure that D+/D- lines are routed to uB connector, so
-- 
1.9.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

[PATCH v3 2/6] usb: phy: msm: Replace the extcon API

[Chanwoo Choi]

This patch uses the resource-managed extcon API for extcon_register_notifier()
and replaces the deprecated extcon API as following:
- extcon_get_cable_state_() -> extcon_get_state()

Signed-off-by: Chanwoo Choi 
Acked-by: Felipe Balbi 
---
 drivers/usb/phy/phy-msm-usb.c | 33 +++--
 1 file changed, 11 insertions(+), 22 deletions(-)

diff --git a/drivers/usb/phy/phy-msm-usb.c b/drivers/usb/phy/phy-msm-usb.c
index 8a34759727bb..a15a89d4235d 100644
--- a/drivers/usb/phy/phy-msm-usb.c
+++ b/drivers/usb/phy/phy-msm-usb.c
@@ -1742,14 +1742,14 @@ static int msm_otg_read_dt(struct platform_device 
*pdev, struct msm_otg *motg)
if (!IS_ERR(ext_vbus)) {
motg->vbus.extcon = ext_vbus;
motg->vbus.nb.notifier_call = msm_otg_vbus_notifier;
-   ret = extcon_register_notifier(ext_vbus, EXTCON_USB,
-   >vbus.nb);
+   ret = devm_extcon_register_notifier(>dev, ext_vbus,
+   EXTCON_USB, >vbus.nb);
if (ret < 0) {
dev_err(>dev, "register VBUS notifier failed\n");
return ret;
}
 
-   ret = extcon_get_cable_state_(ext_vbus, EXTCON_USB);
+   ret = extcon_get_state(ext_vbus, EXTCON_USB);
if (ret)
set_bit(B_SESS_VLD, >inputs);
else
@@ -1759,16 +1759,14 @@ static int msm_otg_read_dt(struct platform_device 
*pdev, struct msm_otg *motg)
if (!IS_ERR(ext_id)) {
motg->id.extcon = ext_id;
motg->id.nb.notifier_call = msm_otg_id_notifier;
-   ret = extcon_register_notifier(ext_id, EXTCON_USB_HOST,
-   >id.nb);
+   ret = devm_extcon_register_notifier(>dev, ext_id,
+   EXTCON_USB_HOST, >id.nb);
if (ret < 0) {
dev_err(>dev, "register ID notifier failed\n");
-   extcon_unregister_notifier(motg->vbus.extcon,
-  EXTCON_USB, >vbus.nb);
return ret;
}
 
-   ret = extcon_get_cable_state_(ext_id, EXTCON_USB_HOST);
+   ret = extcon_get_state(ext_id, EXTCON_USB_HOST);
if (ret)
clear_bit(ID, >inputs);
else
@@ -1883,10 +1881,9 @@ static int msm_otg_probe(struct platform_device *pdev)
 */
if (motg->phy_number) {
phy_select = devm_ioremap_nocache(>dev, USB2_PHY_SEL, 4);
-   if (!phy_select) {
-   ret = -ENOMEM;
-   goto unregister_extcon;
-   }
+   if (!phy_select)
+   return -ENOMEM;
+
/* Enable second PHY with the OTG port */
writel(0x1, phy_select);
}
@@ -1897,7 +1894,7 @@ static int msm_otg_probe(struct platform_device *pdev)
if (motg->irq < 0) {
dev_err(>dev, "platform_get_irq failed\n");
ret = motg->irq;
-   goto unregister_extcon;
+   return motg->irq;
}
 
regs[0].supply = "vddcx";
@@ -1906,7 +1903,7 @@ static int msm_otg_probe(struct platform_device *pdev)
 
ret = devm_regulator_bulk_get(motg->phy.dev, ARRAY_SIZE(regs), regs);
if (ret)
-   goto unregister_extcon;
+   return ret;
 
motg->vddcx = regs[0].consumer;
motg->v3p3  = regs[1].consumer;
@@ -2003,11 +2000,6 @@ static int msm_otg_probe(struct platform_device *pdev)
clk_disable_unprepare(motg->clk);
if (!IS_ERR(motg->core_clk))
clk_disable_unprepare(motg->core_clk);
-unregister_extcon:
-   extcon_unregister_notifier(motg->id.extcon,
-  EXTCON_USB_HOST, >id.nb);
-   extcon_unregister_notifier(motg->vbus.extcon,
-  EXTCON_USB, >vbus.nb);
 
return ret;
 }
@@ -2029,9 +2021,6 @@ static int msm_otg_remove(struct platform_device *pdev)
 */
gpiod_set_value_cansleep(motg->switch_gpio, 0);
 
-   extcon_unregister_notifier(motg->id.extcon, EXTCON_USB_HOST, 
>id.nb);
-   extcon_unregister_notifier(motg->vbus.extcon, EXTCON_USB, 
>vbus.nb);
-
msm_otg_debugfs_cleanup();
cancel_delayed_work_sync(>chg_work);
cancel_work_sync(>sm_work);
-- 
1.9.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

[PATCH v3 5/6] usb: phy: tahvo: Replace the deprecated extcon API

[Chanwoo Choi]

This patch replaces the deprecated extcon API as following:
- extcon_set_cable_state_() -> extcon_set_state_sync()

Signed-off-by: Chanwoo Choi 
Acked-by: Felipe Balbi 
---
 drivers/usb/phy/phy-tahvo.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/phy/phy-tahvo.c b/drivers/usb/phy/phy-tahvo.c
index ab5d364f6e8c..a31c8682e998 100644
--- a/drivers/usb/phy/phy-tahvo.c
+++ b/drivers/usb/phy/phy-tahvo.c
@@ -121,7 +121,7 @@ static void check_vbus_state(struct tahvo_usb *tu)
prev_state = tu->vbus_state;
tu->vbus_state = reg & TAHVO_STAT_VBUS;
if (prev_state != tu->vbus_state) {
-   extcon_set_cable_state_(tu->extcon, EXTCON_USB, tu->vbus_state);
+   extcon_set_state_sync(tu->extcon, EXTCON_USB, tu->vbus_state);
sysfs_notify(>pt_dev->dev.kobj, NULL, "vbus_state");
}
 }
@@ -130,7 +130,7 @@ static void tahvo_usb_become_host(struct tahvo_usb *tu)
 {
struct retu_dev *rdev = dev_get_drvdata(tu->pt_dev->dev.parent);
 
-   extcon_set_cable_state_(tu->extcon, EXTCON_USB_HOST, true);
+   extcon_set_state_sync(tu->extcon, EXTCON_USB_HOST, true);
 
/* Power up the transceiver in USB host mode */
retu_write(rdev, TAHVO_REG_USBR, USBR_REGOUT | USBR_NSUSPEND |
@@ -149,7 +149,7 @@ static void tahvo_usb_become_peripheral(struct tahvo_usb 
*tu)
 {
struct retu_dev *rdev = dev_get_drvdata(tu->pt_dev->dev.parent);
 
-   extcon_set_cable_state_(tu->extcon, EXTCON_USB_HOST, false);
+   extcon_set_state_sync(tu->extcon, EXTCON_USB_HOST, false);
 
/* Power up transceiver and set it in USB peripheral mode */
retu_write(rdev, TAHVO_REG_USBR, USBR_SLAVE_CONTROL | USBR_REGOUT |
@@ -379,9 +379,9 @@ static int tahvo_usb_probe(struct platform_device *pdev)
}
 
/* Set the initial cable state. */
-   extcon_set_cable_state_(tu->extcon, EXTCON_USB_HOST,
+   extcon_set_state_sync(tu->extcon, EXTCON_USB_HOST,
   tu->tahvo_mode == TAHVO_MODE_HOST);
-   extcon_set_cable_state_(tu->extcon, EXTCON_USB, tu->vbus_state);
+   extcon_set_state_sync(tu->extcon, EXTCON_USB, tu->vbus_state);
 
/* Create OTG interface */
tahvo_usb_power_off(tu);
-- 
1.9.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

[PATCH v3 6/6] usb: renesas_usbhs: Replace the deprecated extcon API

[Chanwoo Choi]

This patch replaces the deprecated extcon API as following:
- extcon_get_cable_state_() -> extcon_get_state()

Signed-off-by: Chanwoo Choi 
Acked-by: Felipe Balbi 
Acked-by: Yoshihiro Shimoda 
---
 drivers/usb/renesas_usbhs/common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/renesas_usbhs/common.c 
b/drivers/usb/renesas_usbhs/common.c
index 012a37aa3e0d..623c51300393 100644
--- a/drivers/usb/renesas_usbhs/common.c
+++ b/drivers/usb/renesas_usbhs/common.c
@@ -389,7 +389,7 @@ static void usbhsc_hotplug(struct usbhs_priv *priv)
 
if (enable && !mod) {
if (priv->edev) {
-   cable = extcon_get_cable_state_(priv->edev, 
EXTCON_USB_HOST);
+   cable = extcon_get_state(priv->edev, EXTCON_USB_HOST);
if ((cable > 0 && id != USBHS_HOST) ||
(!cable && id != USBHS_GADGET)) {
dev_info(>dev,
-- 
1.9.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: [RFC 0/1] Platform driver support for 'amd5536udc' driver

[Felipe Balbi]

Hi,

Arnd Bergmann  writes:
> On Friday, January 6, 2017 12:29:12 PM CET Raviteja Garimella wrote:
>> Hi Arnd,
>> 
>> On Fri, Jan 6, 2017 at 3:33 AM, Arnd Bergmann  wrote:
>> > On Thursday, January 5, 2017 1:53:16 PM CET Raviteja Garimella wrote:
>> >> The UDC is based on Synopsys Designware core USB (2.0) Device controller
>> >> IP.
>> > ...
>> >> This is a request for comments from maintainers/others regarding approach
>> >> on whether to have 2 different drivers (one each for AMD and Broadcom)
>> >> with a common library (3 files in total), or have a single driver like
>> >> it's done in this patch and have the driver filename changed to some
>> >> common name based on ther underlying IP, like snps_udc.c.
>> >
>> > I have not looked at the code at all, so sorry for my ignorance, but
>> > isn't the IP block you describe the one that drivers/usb/dwc2/ is for?
>> > Could you add support for the Broadcom hardware there instead?
>> 
>> The current driver I submitted is for a different Synopsys IP (USB
>> Device Controller IP,
>> not the HS OTG). It's confirmed by John Youn (from Synopsys) earlier.
>> 
>
> Ok, sounds fine the. I'd suggest taking the current driver than and
> splitting out the pci_driver front-end into a separate module that
> calls exported symbols of the main driver, with the new platform
> driver in a third file that also calls the same exported symbols.

right, that's the best idea.

-- 
balbi


signature.asc
Description: PGP signature

usb: gadger: f_fs: Do not copy past descriptor end.

[Vincent Pelletier]

Endpoint descriptors come in 2 sizes, struct usb_endpoint_descriptor being
the largest. Take bLength into account to not copy past the endpoint
descriptor end, which could be the next descriptor or past interface
descriptor (by 2 bytes).

Signed-off-by: Vincent Pelletier 
---
 drivers/usb/gadget/function/f_fs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_fs.c 
b/drivers/usb/gadget/function/f_fs.c
index 5490fc51638e..c573c4425f10 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -1230,7 +1230,8 @@ static long ffs_epfile_ioctl(struct file *file, unsigned 
code,
desc = epfile->ep->descs[desc_idx];
 
spin_unlock_irq(>ffs->eps_lock);
-   ret = copy_to_user((void *)value, desc, sizeof(*desc));
+   ret = copy_to_user((void *)value, desc,
+  min(sizeof(*desc), 
(size_t)desc->bLength));
if (ret)
ret = -EFAULT;
return ret;
-- 
2.11.0

--
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: [PATCH] usb: dwc2: fix "iomem 0x00000000" message by setting iomem parameters in usb_hcd

[Felipe Balbi]

Hi,

Heiner Kallweit  writes:
> Set the iomem parameters in the usb_hcd to fix this misleading
> message during driver load:
> dwc2 c910.usb: irq 22, io mem 0x
>
> Signed-off-by: Heiner Kallweit 
> ---
>  drivers/usb/dwc2/core.h | 3 ++-
>  drivers/usb/dwc2/hcd.c  | 5 -
>  drivers/usb/dwc2/hcd.h  | 3 ++-
>  drivers/usb/dwc2/platform.c | 2 +-
>  4 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 9548d3e0..b66eaeea 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -1229,7 +1229,8 @@ static inline void dwc2_hcd_connect(struct dwc2_hsotg 
> *hsotg) {}
>  static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force) 
> {}
>  static inline void dwc2_hcd_start(struct dwc2_hsotg *hsotg) {}
>  static inline void dwc2_hcd_remove(struct dwc2_hsotg *hsotg) {}
> -static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
> +static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
> + struct resource *res)
>  { return 0; }
>  static inline int dwc2_backup_host_registers(struct dwc2_hsotg *hsotg)
>  { return 0; }
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 911c3b36..2cfbd10e 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -4964,7 +4964,7 @@ static void dwc2_hcd_release(struct dwc2_hsotg *hsotg)
>   * USB bus with the core and calls the hc_driver->start() function. It 
> returns
>   * a negative error on failure.
>   */
> -int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
> +int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq, struct resource *res)
>  {
>   struct usb_hcd *hcd;
>   struct dwc2_host_chan *channel;
> @@ -5021,6 +5021,9 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
>  
>   hcd->has_tt = 1;
>  
> + hcd->rsrc_start = res->start;
> + hcd->rsrc_len = resource_size(res);
> +
>   ((struct wrapper_priv_data *) >hcd_priv)->hsotg = hsotg;
>   hsotg->priv = hcd;
>  
> diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h
> index 1ed5fa2b..2305b5fb 100644
> --- a/drivers/usb/dwc2/hcd.h
> +++ b/drivers/usb/dwc2/hcd.h
> @@ -521,7 +521,8 @@ static inline u8 dwc2_hcd_is_pipe_out(struct 
> dwc2_hcd_pipe_info *pipe)
>   return !dwc2_hcd_is_pipe_in(pipe);
>  }
>  
> -extern int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq);
> +extern int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
> +  struct resource *res);
>  extern void dwc2_hcd_remove(struct dwc2_hsotg *hsotg);
>  
>  /* Transaction Execution Functions */
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index 4fc8c603..5ddc8860 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -445,7 +445,7 @@ static int dwc2_driver_probe(struct platform_device *dev)
>   }
>  
>   if (hsotg->dr_mode != USB_DR_MODE_PERIPHERAL) {
> - retval = dwc2_hcd_init(hsotg, hsotg->irq);
> + retval = dwc2_hcd_init(hsotg, hsotg->irq, res);

This is a good idea, but there's more work that can come out of this, if
you're interested:

i) devm_ioremap_resource() could be called by the generic layer
ii) devm_request_irq() could be move to the generic layer
iii) dwc2_hcd_init() could also be called generically as long as dr_mode
 is set properly.
iv) dwc2_debugfs_init() could be called generically as well

IOW, dwc2_driver_probe() could be as minimal as:

static int dwc2_driver_probe(struct platform_device *dev)
{
struct dwc2_hsotg *hsotg;
struct resource *res;
int retval;

hsotg = devm_kzalloc(>dev, sizeof(*hsotg), GFP_KERNEL);
if (!hsotg)
return -ENOMEM;

hsotg->dev = >dev;

if (!dev->dev.dma_mask)
dev->dev.dma_mask = >dev.coherent_dma_mask;

retval = dma_set_coherent_mask(>dev, DMA_BIT_MASK(32));
if (retval)
return retval;

hsotg->res = platform_get_resource(dev, IORESOURCE_MEM, 0);
hsotg->irq = platform_get_irq(dev, 0);

retval = dwc2_get_dr_mode(hsotg);
if (retval)
return retval;

retval = dwc2_get_hwparams(hsotg);
if (retval)
return retval;

platform_set_drvdata(dev, hsotg);

retval = dwc2_core_init(hsotg);
if (retval)
return retval;

return 0;
}

dwc2_core_init() needs to implemented, of course, but it could hide all
details about what to do with IRQs and resources and what not. Assuming
you can properly initialize clocks, it could even handle clocks
generically for you.

-- 
balbi


signature.asc
Description: PGP signature

[PATCH v3 0/6] usb: Replace the deprecated extcon API

[Chanwoo Choi]

This patches just replace the deprecated extcon API without any change
of extcon operation and use the resource-managed function for
extcon_register_notifier().

The new extcon API instead of deprecated API.
- extcon_set_cable_state_() -> extcon_set_state_sync();
- extcon_get_cable_state_() -> extcon_get_state();

Changes from v2:
- Rebase these patches based on usb.git[1] (testing/next branch)

Changes from v1:
- Rebase these patches based on v4.10-rc1.
- Add acked-by tag from usb maintainer and reviewer.
- Drop the phy/power-supply/chipidea patches.

[1] 
https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/log/?h=testing/next

Chanwoo Choi (6):
  usb: dwc3: omap: Replace the extcon API
  usb: phy: msm: Replace the extcon API
  usb: phy: omap-otg: Replace the extcon API
  usb: phy: qcom-8x16-usb: Replace the extcon API
  usb: phy: tahvo: Replace the deprecated extcon API
  usb: renesas_usbhs: Replace the deprecated extcon API

 drivers/usb/dwc3/dwc3-omap.c| 20 +++-
 drivers/usb/phy/phy-msm-usb.c   | 33 +++--
 drivers/usb/phy/phy-omap-otg.c  | 24 ++--
 drivers/usb/phy/phy-qcom-8x16-usb.c | 13 -
 drivers/usb/phy/phy-tahvo.c | 10 +-
 drivers/usb/renesas_usbhs/common.c  |  2 +-
 6 files changed, 34 insertions(+), 68 deletions(-)

-- 
1.9.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: usb: gadger: f_fs: Do not copy past descriptor end.

[Felipe Balbi]

Hi,

Vincent Pelletier  writes:
> Endpoint descriptors come in 2 sizes, struct usb_endpoint_descriptor being
> the largest. Take bLength into account to not copy past the endpoint
> descriptor end, which could be the next descriptor or past interface
> descriptor (by 2 bytes).
>
> Signed-off-by: Vincent Pelletier 
> ---
>  drivers/usb/gadget/function/f_fs.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/function/f_fs.c 
> b/drivers/usb/gadget/function/f_fs.c
> index 5490fc51638e..c573c4425f10 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -1230,7 +1230,8 @@ static long ffs_epfile_ioctl(struct file *file, 
> unsigned code,
>   desc = epfile->ep->descs[desc_idx];
>  
>   spin_unlock_irq(>ffs->eps_lock);
> - ret = copy_to_user((void *)value, desc, sizeof(*desc));
> + ret = copy_to_user((void *)value, desc,
> +min(sizeof(*desc), 
> (size_t)desc->bLength));

so we need min() here? desc->bLength should always contain correct size.

-- 
balbi


signature.asc
Description: PGP signature

[PATCHv2] usb: dwc2: gadget: Fix GUSBCFG.USBTRDTIM value

[Amelie Delaunay]

USBTrdTim must be programmed to 0x5 when phy has a UTMI+ 16-bit wide
interface or 0x9 when it has a 8-bit wide interface.
GUSBCFG reset value (Value After Reset: 0x1400) sets USBTrdTim to 0x5.
In case of 8-bit UTMI+, without clearing GUSBCFG.USBTRDTIM mask, USBTrdTim
results in 0xD (0x5 | 0x9).
That's why we need to clear GUSBCFG.USBTRDTIM mask before setting USBTrdTim
value, to ensure USBTrdTim is correctly set in case of 8-bit UTMI+.

Fixes: ecd9a7ad453c ("usb: dwc2: do not override forced dr_mode in gadget 
setup")
Signed-off-by: Amelie Delaunay 
---
v2: add Fixes: in commit message

 drivers/usb/dwc2/gadget.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index c55db4a..86b2076 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -3169,7 +3169,7 @@ void dwc2_hsotg_core_init_disconnected(struct dwc2_hsotg 
*hsotg,
/* keep other bits untouched (so e.g. forced modes are not lost) */
usbcfg = dwc2_readl(hsotg->regs + GUSBCFG);
usbcfg &= ~(GUSBCFG_TOUTCAL_MASK | GUSBCFG_PHYIF16 | GUSBCFG_SRPCAP |
-   GUSBCFG_HNPCAP);
+   GUSBCFG_HNPCAP | GUSBCFG_USBTRDTIM_MASK);
 
if (hsotg->params.phy_type == DWC2_PHY_TYPE_PARAM_FS &&
(hsotg->params.speed == DWC2_SPEED_PARAM_FULL ||
@@ -4131,7 +4131,7 @@ static void dwc2_hsotg_init(struct dwc2_hsotg *hsotg)
/* keep other bits untouched (so e.g. forced modes are not lost) */
usbcfg = dwc2_readl(hsotg->regs + GUSBCFG);
usbcfg &= ~(GUSBCFG_TOUTCAL_MASK | GUSBCFG_PHYIF16 | GUSBCFG_SRPCAP |
-   GUSBCFG_HNPCAP);
+   GUSBCFG_HNPCAP | GUSBCFG_USBTRDTIM_MASK);
 
/* set the PLL on, remove the HNP/SRP and set the PHY */
trdtim = (hsotg->phyif == GUSBCFG_PHYIF8) ? 9 : 5;
-- 
1.9.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: [PATCHv15 2/3] usb: USB Type-C connector class

[Greg KH]

On Mon, Jan 16, 2017 at 05:56:13PM +0300, Heikki Krogerus wrote:
> The purpose of USB Type-C connector class is to provide
> unified interface for the user space to get the status and
> basic information about USB Type-C connectors on a system,
> control over data role swapping, and when the port supports
> USB Power Delivery, also control over power role swapping
> and Alternate Modes.
> 
> Signed-off-by: Heikki Krogerus 
> Reviewed-by: Mika Westerberg 

I asked for signed-off-by or reviewed-by from other Intel kernel
developers before I would look at this again.

You did run this through the Intel internal kernel developer mailing
list before sending this here, right?  If not, please go do that now,
we've reviewed this enough, you need to burn some Intel resources on
this before I am going to take the time again...

thanks,

greg k-h
--
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: [PATCH] USB: serial: opticon: fix CTS retrieval at open

[Johan Hovold]

On Fri, Jan 13, 2017 at 01:41:47PM +0100, Greg Kroah-Hartman wrote:
> On Fri, Jan 13, 2017 at 01:21:08PM +0100, Johan Hovold wrote:
> > The opticon driver used a control request at open to trigger a CTS
> > status notification to be sent over the bulk-in pipe. When the driver
> > was converted to using the generic read implementation, an inverted test
> > prevented this request from being sent, something which could lead to
> > TIOCMGET reporting an incorrect CTS state.
> > 
> > Reported-by: Dan Carpenter 
> > Fixes: 7a6ee2b02751 ("USB: opticon: switch to generic read
> > implementation")
> > Cc: stable 
> > Signed-off-by: Johan Hovold 
> > ---
> >  drivers/usb/serial/opticon.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> 
> Reviewed-by: Greg Kroah-Hartman 

Thanks for the review. Now applied for -next.

Johan
--
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: VL805 USB 3.0 does not see connected devices (only on x86_64) (x86 is ok)

[Mathias Nyman]

Obviously there is some sort of communications problem.  Either the
device stopped transferring data or else the xHCI host controller
stopped receiving it.  From the usbmon trace, there's no way to tell
which.  However, if the device works okay with an EHCI host
controller or on another computer, that would indicate the problem is
in the xHCI controller.

Maybe Mathias can offer some advice.



The "WARNING: Host System Error" means we got a interrupt with
with HSE status bit set, indicating a fatal xHIC error.
In the logs this is seen together with the DMA remapping errors.
(dmesg below)

Do you have the INTEL_IOMMU_DEFAULT_ON config option set,
or passing intel_iommu=on to the kernel? if so, try disabling
it

Device Drivers -> IOMMU hardware supports -> Enable Intel DMA remapping devices 
by default.

Also adding xhci debugging could show something,

echo -n 'module xhci_hcd =p' > /sys/kernel/debug/dynamic_debug/control

-Mathias



[  197.085680] PM: Adding info for No Bus:sdc1
[  197.086929] sd 7:0:0:0: [sdc] Attached SCSI removable disk
[  230.955006] FAT-fs (sdc1): Volume was not properly unmounted. Some
data may be corrupt. Please run fsck.
[  234.422942] DMAR: DRHD: handling fault status reg 2
[  234.422946] DMAR: [DMA Read] Request device [02:00.0] fault addr
fffbb000 [fault reason 06] PTE Read access is not set
[  234.422950] xhci_hcd :02:00.0: WARNING: Host System Error
[  234.446078] xhci_hcd :02:00.0: Host not halted after 16000 microseconds.
[  270.447035] xhci_hcd :02:00.0: xHCI host not responding to stop
endpoint command.
[  270.447040] xhci_hcd :02:00.0: Assuming host is dying, halting host.
[  270.470143] xhci_hcd :02:00.0: Host not halted after 16000 microseconds.
[  270.470153] xhci_hcd :02:00.0: Non-responsive xHCI host is not halting.
[  270.470154] xhci_hcd :02:00.0: Completing active URBs anyway.
[  270.470161] xhci_hcd :02:00.0: HC died; cleaning up


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

[PATCHv15 1/3] lib/string: add sysfs_match_string helper

[Heikki Krogerus]

Make a simple helper for matching strings with sysfs
attribute files. In most parts the same as match_string(),
except sysfs_match_string() uses sysfs_streq() instead of
strcmp() for matching. This is more convenient when used
with sysfs attributes.

Signed-off-by: Heikki Krogerus 
Reviewed-by: Mika Westerberg 
---
 include/linux/string.h | 10 ++
 lib/string.c   | 26 ++
 2 files changed, 36 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index 26b6f6a66f83..c4011b28f3d8 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -135,6 +135,16 @@ static inline int strtobool(const char *s, bool *res)
 }
 
 int match_string(const char * const *array, size_t n, const char *string);
+int __sysfs_match_string(const char * const *array, size_t n, const char *s);
+
+/**
+ * sysfs_match_string - matches given string in an array
+ * @_a: array of strings
+ * @_s: string to match with
+ *
+ * Helper for __sysfs_match_string(). Calculates the size of @a automatically.
+ */
+#define sysfs_match_string(_a, _s) __sysfs_match_string(_a, ARRAY_SIZE(_a), _s)
 
 #ifdef CONFIG_BINARY_PRINTF
 int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args);
diff --git a/lib/string.c b/lib/string.c
index ed83562a53ae..1a7d3fd52541 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -656,6 +656,32 @@ int match_string(const char * const *array, size_t n, 
const char *string)
 }
 EXPORT_SYMBOL(match_string);
 
+/**
+ * __sysfs_match_string - matches given string in an array
+ * @array: array of strings
+ * @n: number of strings in the array or -1 for NULL terminated arrays
+ * @str: string to match with
+ *
+ * Returns index of @str in the @array or -EINVAL, just like match_string().
+ * Uses sysfs_streq instead of strcmp for matching.
+ */
+int __sysfs_match_string(const char * const *array, size_t n, const char *str)
+{
+   const char *item;
+   int index;
+
+   for (index = 0; index < n; index++) {
+   item = array[index];
+   if (!item)
+   break;
+   if (sysfs_streq(item, str))
+   return index;
+   }
+
+   return -EINVAL;
+}
+EXPORT_SYMBOL(__sysfs_match_string);
+
 #ifndef __HAVE_ARCH_MEMSET
 /**
  * memset - Fill a region of memory with the given value
-- 
2.11.0

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

[PATCHv15 2/3] usb: USB Type-C connector class

[Heikki Krogerus]

The purpose of USB Type-C connector class is to provide
unified interface for the user space to get the status and
basic information about USB Type-C connectors on a system,
control over data role swapping, and when the port supports
USB Power Delivery, also control over power role swapping
and Alternate Modes.

Signed-off-by: Heikki Krogerus 
Reviewed-by: Mika Westerberg 
---
 Documentation/ABI/testing/sysfs-class-typec |  289 ++
 Documentation/usb/typec.rst |  185 
 MAINTAINERS |9 +
 drivers/usb/Kconfig |2 +
 drivers/usb/Makefile|2 +
 drivers/usb/typec/Kconfig   |7 +
 drivers/usb/typec/Makefile  |1 +
 drivers/usb/typec/typec.c   | 1263 +++
 include/linux/usb/typec.h   |  243 ++
 9 files changed, 2001 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-typec
 create mode 100644 Documentation/usb/typec.rst
 create mode 100644 drivers/usb/typec/Kconfig
 create mode 100644 drivers/usb/typec/Makefile
 create mode 100644 drivers/usb/typec/typec.c
 create mode 100644 include/linux/usb/typec.h

diff --git a/Documentation/ABI/testing/sysfs-class-typec 
b/Documentation/ABI/testing/sysfs-class-typec
new file mode 100644
index ..09eb439246b0
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-typec
@@ -0,0 +1,289 @@
+USB Type-C port devices (eg. /sys/class/typec/port0/)
+
+What:  /sys/class/typec//data_role
+Date:  February 2017
+Contact:   Heikki Krogerus 
+Description:
+   The supported USB data roles. This attribute can be used for
+   requesting data role swapping on the port. Swapping is supported
+   as synchronous operation, so write(2) to the attribute will not
+   return until the operation has finished. The attribute is
+   notified about role changes so that poll(2) on the attribute
+   wakes up. Change on the role will also generate uevent
+   KOBJ_CHANGE on the port. The current role is show in brackets,
+   for example "[host] device" when DRP port is in host mode.
+
+   Valid values:
+   - host
+   - device
+
+What:  /sys/class/typec//power_role
+Date:  February 2017
+Contact:   Heikki Krogerus 
+Description:
+   The supported power roles. This attribute can be used to request
+   power role swap on the port when the port supports USB Power
+   Delivery. Swapping is supported as synchronous operation, so
+   write(2) to the attribute will not return until the operation
+   has finished. The attribute is notified about role changes so
+   that poll(2) on the attribute wakes up. Change on the role will
+   also generate uevent KOBJ_CHANGE. The current role is show in
+   brackets, for example "[source] sink" when in source mode.
+
+   Valid values:
+   - source
+   - sink
+
+What:  /sys/class/typec//vconn_source
+Date:  February 2017
+Contact:   Heikki Krogerus 
+Description:
+   Shows is the port VCONN Source. This attribute can be used to
+   request VCONN swap to change the VCONN Source during connection
+   when both the port and the partner support USB Power Delivery.
+   Swapping is supported as synchronous operation, so write(2) to
+   the attribute will not return until the operation has finished.
+   The attribute is notified about VCONN source changes so that
+   poll(2) on the attribute wakes up. Change on VCONN source also
+   generates uevent KOBJ_CHANGE.
+
+   Valid values are:
+   - 0 when the port is not the VCONN Source
+   - 1 when the port is the VCONN Source
+
+What:  /sys/class/typec//power_operation_mode
+Date:  February 2017
+Contact:   Heikki Krogerus 
+Description:
+   Shows the current power operational mode the port is in. The
+   power operation mode means current level for VBUS. In case USB
+   Power Delivery communication is used for negotiating the levels,
+   power operation mode should show "usb_power_delivery".
+
+   Valid values:
+   - default
+   - 1.5A
+   - 3.0A
+   - usb_power_delivery
+
+What:  /sys/class/typec//preferred_role
+Date:  February 2017
+Contact:   Heikki Krogerus 
+Description:
+

[PATCHv15 3/3] usb: typec: add driver for Intel Whiskey Cove PMIC USB Type-C PHY

[Heikki Krogerus]

This adds driver for the USB Type-C PHY on Intel WhiskeyCove
PMIC which is available on some of the Intel Broxton SoC
based platforms.

Signed-off-by: Heikki Krogerus 
Reviewed-by: Mika Westerberg 
---
 drivers/usb/typec/Kconfig   |  14 ++
 drivers/usb/typec/Makefile  |   1 +
 drivers/usb/typec/typec_wcove.c | 377 
 3 files changed, 392 insertions(+)
 create mode 100644 drivers/usb/typec/typec_wcove.c

diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
index 17792f9114c6..2abbcb021d1b 100644
--- a/drivers/usb/typec/Kconfig
+++ b/drivers/usb/typec/Kconfig
@@ -4,4 +4,18 @@ menu "USB Power Delivery and Type-C drivers"
 config TYPEC
tristate
 
+config TYPEC_WCOVE
+   tristate "Intel WhiskeyCove PMIC USB Type-C PHY driver"
+   depends on ACPI
+   depends on INTEL_SOC_PMIC
+   depends on INTEL_PMC_IPC
+   select TYPEC
+   help
+ This driver adds support for USB Type-C detection on Intel Broxton
+ platforms that have Intel Whiskey Cove PMIC. The driver can detect the
+ role and cable orientation.
+
+ To compile this driver as module, choose M here: the module will be
+ called typec_wcove
+
 endmenu
diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
index 1012a8bed6d5..b9cb862221af 100644
--- a/drivers/usb/typec/Makefile
+++ b/drivers/usb/typec/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_TYPEC)+= typec.o
+obj-$(CONFIG_TYPEC_WCOVE)  += typec_wcove.o
diff --git a/drivers/usb/typec/typec_wcove.c b/drivers/usb/typec/typec_wcove.c
new file mode 100644
index ..d5a7b21fa3f1
--- /dev/null
+++ b/drivers/usb/typec/typec_wcove.c
@@ -0,0 +1,377 @@
+/**
+ * typec_wcove.c - WhiskeyCove PMIC USB Type-C PHY driver
+ *
+ * Copyright (C) 2017 Intel Corporation
+ * Author: Heikki Krogerus 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/* Register offsets */
+#define WCOVE_CHGRIRQ0 0x4e09
+#define WCOVE_PHYCTRL  0x5e07
+
+#define USBC_CONTROL1  0x7001
+#define USBC_CONTROL2  0x7002
+#define USBC_CONTROL3  0x7003
+#define USBC_CC1_CTRL  0x7004
+#define USBC_CC2_CTRL  0x7005
+#define USBC_STATUS1   0x7007
+#define USBC_STATUS2   0x7008
+#define USBC_STATUS3   0x7009
+#define USBC_IRQ1  0x7015
+#define USBC_IRQ2  0x7016
+#define USBC_IRQMASK1  0x7017
+#define USBC_IRQMASK2  0x7018
+
+/* Register bits */
+
+#define USBC_CONTROL1_MODE_DRP(r)  (((r) & ~0x7) | 4)
+
+#define USBC_CONTROL2_UNATT_SNKBIT(0)
+#define USBC_CONTROL2_UNATT_SRCBIT(1)
+#define USBC_CONTROL2_DIS_ST   BIT(2)
+
+#define USBC_CONTROL3_PD_DIS   BIT(1)
+
+#define USBC_CC_CTRL_VCONN_EN  BIT(1)
+
+#define USBC_STATUS1_DET_ONGOING   BIT(6)
+#define USBC_STATUS1_RSLT(r)   ((r) & 0xf)
+#define USBC_RSLT_NOTHING  0
+#define USBC_RSLT_SRC_DEFAULT  1
+#define USBC_RSLT_SRC_1_5A 2
+#define USBC_RSLT_SRC_3_0A 3
+#define USBC_RSLT_SNK  4
+#define USBC_RSLT_DEBUG_ACC5
+#define USBC_RSLT_AUDIO_ACC6
+#define USBC_RSLT_UNDEF15
+#define USBC_STATUS1_ORIENT(r) (((r) >> 4) & 0x3)
+#define USBC_ORIENT_NORMAL 1
+#define USBC_ORIENT_REVERSE2
+
+#define USBC_STATUS2_VBUS_REQ  BIT(5)
+
+#define USBC_IRQ1_ADCDONE1 BIT(2)
+#define USBC_IRQ1_OVERTEMP BIT(1)
+#define USBC_IRQ1_SHORTBIT(0)
+
+#define USBC_IRQ2_CC_CHANGEBIT(7)
+#define USBC_IRQ2_RX_PDBIT(6)
+#define USBC_IRQ2_RX_HRBIT(5)
+#define USBC_IRQ2_RX_CRBIT(4)
+#define USBC_IRQ2_TX_SUCCESS   BIT(3)
+#define USBC_IRQ2_TX_FAIL  BIT(2)
+
+#define USBC_IRQMASK1_ALL  (USBC_IRQ1_ADCDONE1 | USBC_IRQ1_OVERTEMP | \
+USBC_IRQ1_SHORT)
+
+#define USBC_IRQMASK2_ALL  (USBC_IRQ2_CC_CHANGE | USBC_IRQ2_RX_PD | \
+USBC_IRQ2_RX_HR | USBC_IRQ2_RX_CR | \
+USBC_IRQ2_TX_SUCCESS | USBC_IRQ2_TX_FAIL)
+
+struct wcove_typec {
+   struct mutex lock; /* device lock */
+   struct device *dev;
+   struct regmap *regmap;
+   struct typec_port *port;
+   struct typec_capability cap;
+   struct typec_partner *partner;
+};
+
+enum wcove_typec_func {
+   WCOVE_FUNC_DRIVE_VBUS = 1,
+   WCOVE_FUNC_ORIENTATION,
+   WCOVE_FUNC_ROLE,
+   WCOVE_FUNC_DRIVE_VCONN,
+};
+

[PATCHv15 0/3] USB Type-C Connector class

[Heikki Krogerus]

The USB Type-C class is meant to provide unified interface to the
userspace to present the USB Type-C ports in a system.

Changes since v14:
- Fixes proposed by Mika
- "identity" directory for all discover identity VDOs instead of "vdo" attribute
- alternate mode device names to just "svid-"

Changes since v13:
- New API. Everything is registered separately.

Changes since v12:
- Added prefer_role member to typec_capability structure as requested by Guenter

Changes since v11:
- The port drivers are responsible of removing the alternate
  modes (just like the documentation already said).

Changes since v10:
- Using ATTRIBUTE_GROUPS and DEVICE_ATTR marcos everywhere
- Moved sysfs_match_string to lib/string.c
- Rationalized uevents
- Calling ida_destroy

Changes since v9:
- Minor typec_wcove.c cleanup as proposed by Guenter Roeck. No
  function affect.

Changes since v8:
- checking sysfs_streq() result correctly in sysfs_strmatch
- fixed accessory check in supported_accessory_mode
- using "none" as the only string that can clear the preferred role

Changes since v7:
- Removed "type" attribute from partners
- Added supports_usb_power_delivery attribute for partner and cable

Changes since v6:
- current_vconn_role attr renamed to vconn_source (no API changes)
- Small documentation improvements proposed by Vincent Palatin

Changes since v5:
- Only updating the roles based on driver notifications
- Added MODULE_ALIAS for the WhiskeyCove module
- Including the patch that creates the actual platform device for the
  WhiskeyCove Type-C PHY in this series.

Changes since v4:
- Remove the port lock completely

Changes since v3:
- Documentation cleanup as proposed by Roger Quadros
- Setting partner altmodes member to NULL on removal and fixing a
  warning, as proposed by Guenter Roeck
- Added the following attributes for partners and cables:
  * supports_usb_power_delivery
  * id_header_vdo
- "id_header_vdo" is visible only when the partner or cable supports
  USB Power Delivery communication.
- Partner attribute "accessory" is hidden when the partner type is not
  "Accessory".

Changes since v2:
- Notification on role and alternate mode changes
- cleanups

Changes since v1:
- Completely rewrote alternate mode support
- Patners, cables and cable plugs presented as devices.


Heikki Krogerus (3):
  lib/string: add sysfs_match_string helper
  usb: USB Type-C connector class
  usb: typec: add driver for Intel Whiskey Cove PMIC USB Type-C PHY

 Documentation/ABI/testing/sysfs-class-typec |  289 ++
 Documentation/usb/typec.rst |  185 
 MAINTAINERS |9 +
 drivers/usb/Kconfig |2 +
 drivers/usb/Makefile|2 +
 drivers/usb/typec/Kconfig   |   21 +
 drivers/usb/typec/Makefile  |2 +
 drivers/usb/typec/typec.c   | 1263 +++
 drivers/usb/typec/typec_wcove.c |  377 
 include/linux/string.h  |   10 +
 include/linux/usb/typec.h   |  243 ++
 lib/string.c|   26 +
 12 files changed, 2429 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-typec
 create mode 100644 Documentation/usb/typec.rst
 create mode 100644 drivers/usb/typec/Kconfig
 create mode 100644 drivers/usb/typec/Makefile
 create mode 100644 drivers/usb/typec/typec.c
 create mode 100644 drivers/usb/typec/typec_wcove.c
 create mode 100644 include/linux/usb/typec.h

-- 
2.11.0

--
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: functionfs on dwc3, xhci host: endpoint cannot be used in both directions ?

[Vincent Pelletier]

On Mon, 16 Jan 2017 09:06:20 +0200, Felipe Balbi
 wrote:
> it's actually pretty important :-)

Will do then, although maybe not before this week-end.

There is something I do not get about this though: the protocol
analyser does find IN tokens directed at the right device address,
endpoint 1. So I think the host controller is doing its job just fine.
It is the device controller which somehow does not do whatever it is
supposed to do to pick the data buffer which is already sitting there,
waiting to be pushed on the bus...


I made a few more debugging attempts:
1) I made device.py arm ep1in stall, which did affect ep1in transfers
   (making them fail instantaneously, instead of letting host reach its
   5s timeout). ep1out and ep2in were working properly (ie, not
   stalled). So exactly one endpoint was affected, and the expected one.
   Looks like ep1in has issues with IN transfers (at least BULK ones, I
   did not try other types yet).
2) I replaced two INs, one OUT with just two INs: ep1in is silent,
   ep2in works.
   Likewise, with three INs, only ep1in is silent, 2in and 3in work.
3) I declared 4 endpoints (2 IN, 2 OUT). And I went one level deeper
   down the rabbit hole: now enumeration fails with this message on
   host:

Jan 16 13:37:04 localhost kernel: [268765.354184] usb 1-10: config 1 interface 
0 altsetting 0 has 3 endpoint descriptors, different from the interface 
descriptor's value: 4

  So, protocol analyser time it is again, and I found this beast of a
  configuration descriptor:

000:36.013'125"200n IN  @021.00 DATA146B ACK
000 09 02 2e 00 01 01 04 80  fa 07 05 01 02 00 02 00 

010 09 04 00 00 04 ff 00 00  05 07 05 81 02 00 02 00 

020 07 05 02 02 00 02 00 07  05 82 02 00 02 00   
..

000 09 02: configuration descriptor.
00a 07 05: endpoint descriptor (ep1out). <- WTF ?!
010 09 04: interface descriptor.
01a 07 05: endpoint descriptor (ep1in).
020 07 05: endpoint descriptor (ep2out).
027 07 05: endpoint descriptor (ep2in).

  I checked thrice that I am writing the descriptors in the correct
  order (interface before all endpoints), both on FS and HS
  configurations, in consistent order, and with consistent endpoint
  numbers. Also it is not a FS edpoint as their wMaxPacketSize is 64[1].
  The same happens with 4 IN endpoints, one (0x81) gets in front of the
  interface descriptor. Maybe f_fs.c reordering descriptors ? More
  debugging for another day...
  If something is reordering descriptors, then it means my assumption
  that initialising ep1in before or after ep1out does not make a
  difference may have been misguided.

[1] By the way, functionfs produces non-standard descriptors when no
full-speed descriptors are given: the first (in fs, hs, ss order) list
of endpoints gets passed to usb_ep_autoconfig[_ss](), which limits
wMaxPacketSize to 64. Took me a while to figure several weeks ago. If
that API allowed passing descriptor's speed around it would be
possible, but that's way over my head at the moment - and not a blocker
anyway.
-- 
Vincent Pelletier
--
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: OHCI: high softirq load

[Johan Hovold]

[ +CC: linux-usb ]

On Mon, Jan 16, 2017 at 12:14:03PM +0100, Boris Brezillon wrote:
> On Mon, 16 Jan 2017 11:54:23 +0100
> Antoine Aubert  wrote:
> 
> > Also, I made a big misunderstanding
> > 
> > With EHCI + OHCI = high level of softirq (USB2.0)
> 
> Well, the number of irqs and softirqs are likely to be related (you
> usually trigger a softirq after you received an hardirq).
> 
> > 
> > OHCI only = normal level
> 
> What about EHCI only? And what happens if you only plug 1 device?
> Please share the content of /proc/interrupts (and everything you think
> is relevant) for each of these cases.
> 
> > 
> > Le 16/01/2017 à 11:31, Antoine Aubert a écrit :
> > > Thx for your answer Boris
> > >
> > > Le 16/01/2017 à 10:02, Boris Brezillon a écrit :  
> > >> Hi Antoine,
> > >>
> > >> On Mon, 16 Jan 2017 08:45:58 +0100
> > >> Antoine Aubert  wrote:
> > >>  
> > >>> Hi,
> > >>>
> > >>> Im working on a AT91SAM9G25cu board
> > >>> (arch/arm/boot/dts/at91-kizboxmini.dts). We use linux-4.1.31, and when
> > >>> OHCI is enabled, I got some wired effects.  
> > >> Can you test on a more recent kernel (4.9 or 4.10-rc4)?  
> > > I'll give a try, just need little time ;)  
> > >>> eg with 3 FTDI pluged, interrupts: more than 3.5k/s, cpu softirq > 24%,
> > >>> loadavg > 0.5  
> > >> Can you check which interrupt is triggered (cat /proc/interrupts),  
> > > cat /proc/interrupts
> > >CPU0  
> > >  16:   2286  atmel-aic   1 Level pmc, at91_tick, at91_rtc, ttyS0
> > >  17:  0   PMC  17 Level main_rc_osc
> > >  18:  0   PMC   0 Level main_osc
> > >  19:  0   PMC  16 Level mainck
> > >  20:  0   PMC   1 Level clk-plla
> > >  21:  0   PMC   6 Level clk-utmi
> > >  22:  0   PMC   3 Level clk-master
> > >  23: 945527  atmel-aic  17 Level tc_clkevt
> > >  24:  21815  atmel-aic  20 Level at_hdmac
> > >  25:  0  atmel-aic  21 Level at_hdmac
> > >  30: 120299  atmel-aic  24 Level eth0
> > >  31:   22783651  atmel-aic  22 Level ehci_hcd:usb1, ohci_hcd:usb2
> > >  99:  0  GPIO  16 Edge  PB_RST
> > > 100:  0  GPIO  17 Edge  PB_PROG
> > > Err:  0

Note that the ftdi driver uses a low-latency setting by default which
implies that the device sends a status update every millisecond. Hence,
the 1k interrupts per second (per device) while the port is open is
expected.

You can disable the low-latency behaviour using setserial:

setserial /dev/ttyUSB0 ^low_latency

and see the number of interrupts drop to 1/16th. This can then be
reduced further by changing the latency_timer from its
(non-low-latency) default of 16 ms, for example:

echo 64 >/sys/bus/usb-serial/devices/ttyUSB0/latency_timer

Johan
--
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: gadger: f_fs: Do not copy past descriptor end.

[Vincent Pelletier]

On Mon, 16 Jan 2017 15:30:15 +0200, Felipe Balbi
 wrote:
> so we need min() here? desc->bLength should always contain correct size.

It should, yes, there is a check when parsing descriptors.
I was a bit worried at removing the "natural" maximum size (the one
declared as part of the ioctl code). But if it is fine for you I'll
remove it.

Regards,
-- 
Vincent Pelletier


pgp7Q1Q3jUjvQ.pgp
Description: Signature digitale OpenPGP

Re: [PATCHv15 2/3] usb: USB Type-C connector class

[Mika Westerberg]

On Mon, Jan 16, 2017 at 04:35:24PM +0100, Greg KH wrote:
> On Mon, Jan 16, 2017 at 05:56:13PM +0300, Heikki Krogerus wrote:
> > The purpose of USB Type-C connector class is to provide
> > unified interface for the user space to get the status and
> > basic information about USB Type-C connectors on a system,
> > control over data role swapping, and when the port supports
> > USB Power Delivery, also control over power role swapping
> > and Alternate Modes.
> > 
> > Signed-off-by: Heikki Krogerus 
> > Reviewed-by: Mika Westerberg 
> 
> I asked for signed-off-by or reviewed-by from other Intel kernel
> developers before I would look at this again.

Well, I reviewed this and I work for Intel as kernel developer :-)

> You did run this through the Intel internal kernel developer mailing
> list before sending this here, right?  If not, please go do that now,
> we've reviewed this enough, you need to burn some Intel resources on
> this before I am going to take the time again...

This and the previous version were submitted to the internal list and
was reviewed by me there.
--
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: [PATCH 32/37] usb: host: xhci: switch to running avg trb length

[Mathias Nyman]

On 29.12.2016 13:01, Felipe Balbi wrote:

It's unlikely that we will ever know the avg so
instead of assuming it'll be something really large,
we will calculate the avg as we go as mentioned in
XHCI specification section 4.14.1.1.

Signed-off-by: Felipe Balbi 
---
  drivers/usb/host/xhci-mem.c  | 34 --
  drivers/usb/host/xhci-ring.c | 20 
  drivers/usb/host/xhci.h  |  3 ++-
  3 files changed, 46 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index f7cfbab5ba4c..9e1ccfadf128 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1451,18 +1451,34 @@ int xhci_endpoint_init(struct xhci_hcd *xhci,

ring_type = usb_endpoint_type(>desc);

-   /*
-* Get values to fill the endpoint context, mostly from ep descriptor.
-* The average TRB buffer lengt for bulk endpoints is unclear as we
-* have no clue on scatter gather list entry size. For Isoc and Int,
-* set it to max available. See xHCI 1.1 spec 4.14.1.1 for details.
-*/
+   /* Get values to fill the endpoint context, mostly from ep descriptor. 
*/
max_esit_payload = xhci_get_max_esit_payload(udev, ep);
interval = xhci_get_endpoint_interval(udev, ep);
mult = xhci_get_endpoint_mult(udev, ep);
max_packet = usb_endpoint_maxp(>desc);
max_burst = xhci_get_endpoint_max_burst(udev, ep);
-   avg_trb_len = max_esit_payload;
+
+   /*
+* We are using a running avg for our endpoint's avg_trb_len. The reason
+* is that we have no clue about average transfer sizes for any
+* endpoints because the HCD does not know which USB Class is running on
+* the other end.
+*
+* See xHCI 1.1 spec 4.14.1.1 for details about initial avg_trb_len
+* setting.
+*/
+   switch (usb_endpoint_type(>desc)) {
+   case USB_ENDPOINT_XFER_CONTROL:
+   avg_trb_len = 8;
+   break;
+   case USB_ENDPOINT_XFER_INT:
+   avg_trb_len = 1024;
+   break;
+   case USB_ENDPOINT_XFER_ISOC:
+   case USB_ENDPOINT_XFER_BULK:
+   avg_trb_len = 3072;
+   break;
+   }

/* FIXME dig Mult and streams info out of ep companion desc */

@@ -1472,9 +1488,7 @@ int xhci_endpoint_init(struct xhci_hcd *xhci,
/* Some devices get this wrong */
if (usb_endpoint_xfer_bulk(>desc) && udev->speed == USB_SPEED_HIGH)
max_packet = 512;
-   /* xHCI 1.0 and 1.1 indicates that ctrl ep avg TRB Length should be 8 */
-   if (usb_endpoint_xfer_control(>desc) && xhci->hci_version >= 0x100)
-   avg_trb_len = 8;
+
/* xhci 1.1 with LEC support doesn't use mult field, use RsvdZ */
if ((xhci->hci_version > 0x100) && HCC2_LEC(xhci->hcc_params2))
mult = 0;
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 1431e0651e78..f5f999d8a72c 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2752,6 +2752,8 @@ static int prepare_transfer(struct xhci_hcd *xhci,
struct xhci_td  *td;
struct xhci_ring *ep_ring;
struct xhci_ep_ctx *ep_ctx = xhci_get_ep_ctx(xhci, xdev->out_ctx, 
ep_index);
+   unsigned int avg_trb_len;
+   unsigned int tx_info;

ep_ring = xhci_stream_id_to_ring(xdev, ep_index, stream_id);
if (!ep_ring) {
@@ -2760,6 +2762,24 @@ static int prepare_transfer(struct xhci_hcd *xhci,
return -EINVAL;
}

+   /*
+* Here we update avg_trb_len so that, over time, we get a better
+* representation of what the actual average length for this endpoint's
+* TRBs are going to be.
+*/
+   if (urb->transfer_buffer_length > 0) {
+   tx_info = le32_to_cpu(ep_ctx->tx_info);
+
+   avg_trb_len = EP_AVG_TRB_LENGTH(tx_info);
+   avg_trb_len += urb->transfer_buffer_length;
+   avg_trb_len /= 2;


This avg calculation will give 50% weight to the latest urb length, and only 
50% to all
the previous ones. If the lengths vary a lot then the running avg will also 
fluctuate a lot,
consider if URB1 1024bytes data, URB2 6bytes command, USB3 1024bytes data.

If I remember correctly we couldn't see any benefit from this change. Maybe a
more extensive avg length calculation could be implemented if we start seeing
bandwidth issues.

Skipping this patch

-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: Memory barrier needed with wake_up_process()?

[Alan Stern]

On Mon, 16 Jan 2017, Felipe Balbi wrote:

> Sorry for the long delay, I finally have more information on this. All
> this time I was doing something that I never considered to matter: I've
> been running host and peripheral on the same machine. Now that I have
> tracepoints on xHCI as well, I could see that these 30 seconds of
> "nothing" is actuall full of xHCI activity and I can see that for the
> duration of these 30 seconds preempt depth on the CPU that (eventually)
> queues a request on dwc3, is always > 1 (sometimes 2, most of the time
> 1). My conclusion from that is that xHCI (or usbcore ?!?) locks the CPU
> and g_mass_storage is spinning for over 30 seconds at which point
> storage.ko (host side class driver) dequeues the request.
> 
> I'll see if I can capture a fresh trace with both xHCI and dwc3 with
> this happening, but probably not today (testing stuff for -rc).

Does anything change if the host and peripheral are separate machines?

Alan Stern

--
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: [PATCH] usb: dwc3: ep0: Fix the possible missed request for handling delay STATUS phase

[Alan Stern]

On Mon, 16 Jan 2017, Felipe Balbi wrote:

>  Another point here is that the really robust way of fixing this is to
>  get rid of USB_GADGET_DELAYED_STATUS altogether and just make sure
>  gadget drivers know how to queue requests for all three phases of a
>  Control Transfer.
> 
>  A lot of code will be removed from all gadget drivers and UDC drivers
>  while combining all of it in a single place in composite.c.

Don't forget the legacy drivers.

> 
>  The reason I'm saying this is that other UDC drivers might have similar
>  races already but they just haven't triggered.

> >> Alright, it's important enough to fix this bug. Can you also have a look
> >> into dropping USB_GADGET_DELAYED_STATUS altogether? If you're too busy,
> >> no issues. It'll stay in my queue.
> >
> > Okay, I will have a look at f_mass_storage driver to see if we can
> > drop USB_GADGET_DELAYED_STATUS. Thanks.
> 
> not only mass storage. It needs to be done for all drivers. The way to
> do that is to teach functions that control transfers are composed of two
> or three phases. If you look at UDC drivers today, they all have
> peculiarities about control transfers to handle stuff that *maybe*
> gadget drivers won't handle.
> 
> What we should do here is make sure that *all* 3 phases always have a
> matching usb_ep_queue() coming from the upper layers. Whether
> composite.c or f_*.c handles it, that's an implementation detail. But
> just to illustrate the problem, we should be able to get rid of
> dwc3_ep0_out_start() and assume that the upper layer will call
> usb_ep_queue() when it wants to receive a new SETUP packet.
> 
> Likewise, we should be able to assume that STATUS phase will only start
> based on a usb_ep_queue() call. That way we can remove
> USB_GADGET_DELAYED_STATUS altogether, because that will *always* be the
> case. There will be no races to handle apart from the normal case where
> XferNotReady can come before or after usb_ep_queue(), but we already
> have proper handling for that too.

It sounds like you're talking about a major change in the way the 
gadget subsystem handles control requests.

We can distinguish three cases.  In the existing implementation, they 
work like this:

(1) Control-OUT with no data stage.  The gadget driver's setup
routine either queues a request on ep0, which the UDC driver 
uses for the status stage transfer (so it should be a length-0 
IN transfer) and returns 0, or else returns an error, in which
case the UDC driver sends a protocol STALL for the status 
stage.

(What the UDC driver should do if the setup routine queues a
request on ep0 and then returns an error is undefined.)

(2) Control-OUT with a data stage.  The gadget driver's setup 
routine either queues an OUT request on ep0, which the UDC
driver uses for the data stage transfer, or else returns an
error, in which case the UDC driver sends a protocol STALL for
the data stage.  In the first case, the UDC driver 
automatically queues a 0-length IN request for the status 
stage; the gadget driver does not get any chance to fail the
transfer after the host's data has been successfully received.
(IMO this is a bug in the design of the gadget subsystem.)

(3) Control-IN with a data stage.  The gadget driver's setup 
routine either queues an IN request on ep0, which the UDC
driver uses for the data stage transfer, or else returns an
error, in which case the UDC driver sends a protocol STALL for
the data stage.  In the first case, the UDC driver 
automatically queues a 0-length OUT request for the status 
stage; the gadget driver does not get any chance to fail the
transfer after its data has been successfully sent (and I can't 
think of any reason for doing this).

In the delayed-status or delayed-data case, the setup routine does not
queue a request on ep0 before returning 0; instead the gadget driver
queues this request at a later time in a separate thread.

The gadget driver never calls usb_ep_queue in order to receive the next
SETUP packet; the UDC driver takes care of SETUP handling
automatically.

You are suggesting that status stage requests should not be queued 
automatically by UDC drivers but instead queued explicitly by gadget 
drivers.  This would mean changing every UDC driver and every gadget 
driver.

Also, it won't fix the race that Baolin Wang found.  The setup routine
is always called in interrupt context, so it can't sleep.  Doing
anything non-trivial will require a separate task, and it's possible
that this task will try to enqueue the data-stage or status-stage
request before the UDC driver is ready to handle it (for example, 
before or shortly after the setup routine returns).

To work properly, the UDC driver must be able to accept a request for 
ep0 any time after it invokes the 

Re: [PATCH 36/37] usb: host: xhci: add xhci_virt_device tracer

[Lu Baolu]

Hi,

On 01/16/2017 05:08 PM, Felipe Balbi wrote:
> Hi,
>
> Lu Baolu  writes:
>> Hi,
>>
>> On 12/29/2016 07:01 PM, Felipe Balbi wrote:
>>> Let's start tracing at least part of an xhci_virt_device lifetime. We
>>> might want to extend this tracepoint class later, but for now it already
>>> exposes quite a bit of valuable information.
>>>
>>> Signed-off-by: Felipe Balbi 
>>> ---
>>>  drivers/usb/host/xhci-hub.c   |  2 ++
>>>  drivers/usb/host/xhci-mem.c   |  7 ++
>>>  drivers/usb/host/xhci-trace.h | 57 
>>> +++
>>>  drivers/usb/host/xhci.c   |  1 +
>>>  4 files changed, 67 insertions(+)
>>>
>>> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
>>> index b99f06f4c421..6fdea9e5e3a5 100644
>>> --- a/drivers/usb/host/xhci-hub.c
>>> +++ b/drivers/usb/host/xhci-hub.c
>>> @@ -389,6 +389,8 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int 
>>> slot_id, int suspend)
>>> if (!virt_dev)
>>> return -ENODEV;
>>>  
>>> +   trace_xhci_stop_device(virt_dev);
>>> +
>> I'd suggest move the trace down until all stop endpoint commands complete.
>> The state of a virt_device affects only after the command completes.
> no, that's the wrong thing to do. We're trying to track the *lifetime*
> of things. xhci_stop_device() prepares a command to stop the
> device. There's another command tracer which will trigger on command
> completion IRQ. That command will also be traced.

Got it, thanks.

>
> Again, compile the branch and look at the output.

Yes. I have an output.

>
>>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
>>> index b12631ef160b..2b7c3504a95a 100644
>>> --- a/drivers/usb/host/xhci-mem.c
>>> +++ b/drivers/usb/host/xhci-mem.c
>>> @@ -936,6 +936,9 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, int 
>>> slot_id)
>>> return;
>>>  
>>> dev = xhci->devs[slot_id];
>>> +
>>> +   trace_xhci_free_virt_device(dev);
>>> +
>>> xhci->dcbaa->dev_context_ptrs[slot_id] = 0;
>>> if (!dev)
>>> return;
>>> @@ -1041,6 +1044,8 @@ int xhci_alloc_virt_device(struct xhci_hcd *xhci, int 
>>> slot_id,
>>>  >dcbaa->dev_context_ptrs[slot_id],
>>>  le64_to_cpu(xhci->dcbaa->dev_context_ptrs[slot_id]));
>>>  
>>> +   trace_xhci_alloc_virt_device(dev);
>>> +
>>> return 1;
>>>  fail:
>>> xhci_free_virt_device(xhci, slot_id);
>>> @@ -1215,6 +1220,8 @@ int xhci_setup_addressable_virt_dev(struct xhci_hcd 
>>> *xhci, struct usb_device *ud
>>> ep0_ctx->deq = cpu_to_le64(dev->eps[0].ring->first_seg->dma |
>>>dev->eps[0].ring->cycle_state);
>>>  
>>> +   trace_xhci_setup_addressable_virt_device(dev);
>>> +
>>> /* Steps 7 and 8 were done in xhci_alloc_virt_device() */
>>>  
>>> return 0;
>>> diff --git a/drivers/usb/host/xhci-trace.h b/drivers/usb/host/xhci-trace.h
>>> index 08bed2f07e50..1e85c893247d 100644
>>> --- a/drivers/usb/host/xhci-trace.h
>>> +++ b/drivers/usb/host/xhci-trace.h
>>> @@ -158,6 +158,63 @@ DEFINE_EVENT(xhci_log_trb, xhci_queue_trb,
>>> TP_ARGS(ring, trb)
>>>  );
>>>  
>>> +DECLARE_EVENT_CLASS(xhci_log_virt_dev,
>>> +   TP_PROTO(struct xhci_virt_device *vdev),
>>> +   TP_ARGS(vdev),
>>> +   TP_STRUCT__entry(
>>> +   __field(void *, vdev)
>>> +   __field(unsigned long long, out_ctx)
>>> +   __field(unsigned long long, in_ctx)
>>> +   __field(int, devnum)
>>> +   __field(int, state)
>>> +   __field(int, speed)
>>> +   __field(u8, portnum)
>>> +   __field(u8, level)
>>> +   __field(int, slot_id)
>>> +   ),
>>> +   TP_fast_assign(
>>> +   __entry->vdev = vdev;
>>> +   __entry->in_ctx = (unsigned long long) vdev->in_ctx->dma;
>>> +   __entry->out_ctx = (unsigned long long) vdev->out_ctx->dma;
>>> +   __entry->devnum = vdev->udev->devnum;
>>> +   __entry->state = vdev->udev->state;
>>> +   __entry->speed = vdev->udev->speed;
>>> +   __entry->portnum = vdev->udev->portnum;
>>> +   __entry->level = vdev->udev->level;
>>> +   __entry->slot_id = vdev->udev->slot_id;
>>> +   ),
>>> +   TP_printk("vdev %p ctx %llx | %llx num %d state %d speed %d port %d 
>>> level %d slot %d",
>>> +   __entry->vdev, __entry->in_ctx, __entry->out_ctx, 
>>> __entry->devnum,
>>> +   __entry->state, __entry->speed, __entry->portnum, 
>>> __entry->level,
>>> +   __entry->slot_id
>>> +   )
>>> +);
>>> +
>>> +DEFINE_EVENT(xhci_log_virt_dev, xhci_alloc_virt_device,
>>> +   TP_PROTO(struct xhci_virt_device *vdev),
>>> +   TP_ARGS(vdev)
>>> +);
>>> +
>>> +DEFINE_EVENT(xhci_log_virt_dev, xhci_free_virt_device,
>>> +   TP_PROTO(struct xhci_virt_device *vdev),
>>> +   TP_ARGS(vdev)
>>> +);
>>> +
>>> +DEFINE_EVENT(xhci_log_virt_dev, xhci_setup_device,
>>> +   TP_PROTO(struct xhci_virt_device *vdev),
>>> +   TP_ARGS(vdev)
>>> 

Re: [PATCH] usb: dwc3: ep0: Fix the possible missed request for handling delay STATUS phase

[Baolin Wang]

Hi,

On 16 January 2017 at 20:06, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  writes:
>> Hi,
>>
>> On 16 January 2017 at 19:29, Felipe Balbi  wrote:
>>>
>>> Hi,
>>>
>>> Baolin Wang  writes:
 Hi,

 On 16 January 2017 at 18:56, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  writes:
>> When handing the SETUP packet by composite_setup(), we will release the
>> dwc->lock. If we get the 'USB_GADGET_DELAYED_STATUS' result from setup
>> function, which means we need to delay handling the STATUS phase.
>
> this sentence needs a little work. Seems like it's missing some
> information.
>
> anyway, I get that we release the lock but...
>
>> But during the lock release period, maybe the request for handling delay
>
> execution of ->setup() itself should be locked. I can see that it's only
> locked for set_config() which is rather peculiar.
>
> What exact request you had when you triggered this? (Hint: dwc3
> tracepoints print out ctrl request bytes). IIRC, only set_config() or
> f->set_alt() can actually return USB_GADGET_DELAYED_STATUS.

 Yes, when host set configuration for mass storage driver, it can
 produce this issue.

>
> Which gadget driver were you using when you triggered this?

 mass storage driver. When host issues the setting config request, we
 will get USB_GADGET_DELAYED_STATUS result from
 set_config()--->fsg_set_alt(). Then the mass storage driver will issue
 one thread to complete the status stage by ep0_queue() (this thread
 may be running on another core), then if the thread issues ep0_queue()
 too fast before we get the dwc->lock in dwc3_ep0_delegate_req() or
 before we get into the STATUS stage, then we can not handle this
 request for the delay STATUS stage in dwc3_gadget_ep0_queue().

>
> Another point here is that the really robust way of fixing this is to
> get rid of USB_GADGET_DELAYED_STATUS altogether and just make sure
> gadget drivers know how to queue requests for all three phases of a
> Control Transfer.
>
> A lot of code will be removed from all gadget drivers and UDC drivers
> while combining all of it in a single place in composite.c.
>
> The reason I'm saying this is that other UDC drivers might have similar
> races already but they just haven't triggered.

 Yes, maybe.

>
>> STATUS phase has been queued into list before we set 
>> 'dwc->delayed_status'
>> flag or entering 'EP0_STATUS_PHASE' phase, then we will miss the chance
>> to handle the STATUS phase. Thus we should check if the request for delay
>> STATUS phase has been enqueued when entering 'EP0_STATUS_PHASE' phase in
>> dwc3_ep0_xfernotready(), if so, we should handle it.
>>
>> Signed-off-by: Baolin Wang 
>> ---
>>  drivers/usb/dwc3/ep0.c |   14 ++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>> index 9bb1f85..e689ced 100644
>> --- a/drivers/usb/dwc3/ep0.c
>> +++ b/drivers/usb/dwc3/ep0.c
>> @@ -1123,7 +1123,21 @@ static void dwc3_ep0_xfernotready(struct dwc3 
>> *dwc,
>>   dwc->ep0state = EP0_STATUS_PHASE;
>>
>>   if (dwc->delayed_status) {
>> + struct dwc3_ep *dep = dwc->eps[0];
>> +
>>   WARN_ON_ONCE(event->endpoint_number != 1);
>> + /*
>> +  * We should handle the delay STATUS phase here if 
>> the
>> +  * request for handling delay STATUS has been 
>> queued
>> +  * into the list.
>> +  */
>> + if (!list_empty(>pending_list)) {
>> + dwc->delayed_status = false;
>> + usb_gadget_set_state(>gadget,
>> +  USB_STATE_CONFIGURED);
>
> Isn't this patch also changing the normal case when usb_ep_queue() comes
> later? I guess list_empty() protects against that...

 I think it will not change other cases, we only handle the delayed
 status and I've tested it for a while and I did not find any problem.
>>>
>>> Alright, it's important enough to fix this bug. Can you also have a look
>>> into dropping USB_GADGET_DELAYED_STATUS altogether? If you're too busy,
>>> no issues. It'll stay in my queue.
>>
>> Okay, I will have a look at f_mass_storage driver to see if we can
>> drop USB_GADGET_DELAYED_STATUS. Thanks.
>
> not only mass storage. It needs to be done for all drivers. The way to
> do that is to teach functions that control transfers are composed of 

Re: [PATCH 2/2] usb: gadget: uac2: add req_number as parameter

[Peter Chen]

On Mon, Jan 16, 2017 at 12:40:06PM +0200, Felipe Balbi wrote:
> 
> Hi,
> 
> Peter Chen  writes:
> > There are only two requests for uac2, it may not be enough at high
> > loading system which usb interrupt handler can't be serviced on
> > time, then the data will be lost since it is isoc transfer for audio.
> >
> > In this patch, we introduce a parameter for the number for usb request,
> > and the user can override it if current number for request is not enough
> > for his/her use case.
> >
> > Besides, update this parameter for legacy audio gadget and documentation.
> 
> I would rather just drop pre-allocation of requests. Every time Audio
> wants transmit data you allocate and queue a request there and free()
> the request on completion.
> 
> All of these functions already have a ton of parameters :-s
> 

At high loading system, how can we make sure the interrupt can be
serviced in one isoc time slice? We ran out this problem at one
customer project, and even at default mainline, the aplay will
meet underrun using UAC2.

-- 

Best Regards,
Peter Chen
--
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: [PATCH v14 02/10] usbip: exporting devices: modifications to host side libraries

[fx IWATA NOBUO]

Hello Shuah,

> Way too many changes in this one patch.

> This is not an accurate description. You are really changing look ups using
> bus-id instead bus-num and whether usbip_get_device() is used or not is
> irrelevant.

> Please don't include exporting bind_device() and unbind_device() and
> changing their names in this patch. Send a separate patch for that.

I will divide this patch into following 3 patches.
1) removing un used old usbip_get_device() as out of the series.
2) exporting bind/unbind() for new operations.
3) adding new find device by busid for new operations.

> It makes sense to move the functions you are exporting bind_device(),
> unbind_device(), and usbip_get_device() to libsrc - usbip_common.c
> might be a good choice.

usbip_common.c does not depend stub nor vhci.
bind/unbind depends stub.
And bind_device() and unbind_device() use local functions in 
usbip_bind.c and usbip_unbind.c. in this series

So I'd like to keep them in usbip_bind.c and usbip_unbind.c.

> > +   if (!strncmp(busid, edev->udev.busid,
> SYSFS_BUS_ID_SIZE))
> 
> Use strlen(edev->udev.busid) instead of SYSFS_BUS_ID_SIZE

I will use strcmp() because busid in refreshed edev list must be valid.

> The following isn't part of this patch, hence shouldn't be included in
> the change log.

I will remove.

Best Regards,

Nobuo Iwata
//
> -Original Message-
> From: Shuah Khan [mailto:shua...@osg.samsung.com]
> Sent: Friday, January 13, 2017 6:15 AM
> To: fx IWATA NOBUO ;
> valentina.mane...@gmail.com; sh...@kernel.org;
> gre...@linuxfoundation.org; linux-usb@vger.kernel.org
> Cc: fx MICHIMURA TADAO ; Shuah
> Khan ; Shuah Khan 
> Subject: Re: [PATCH v14 02/10] usbip: exporting devices: modifications to
> host side libraries
> 
> On 12/26/2016 12:08 AM, Nobuo Iwata wrote:
> > Modifications to host driver wrapper and its related operations (i.e.
> > bind/unbind).
> 
> Way too many changes in this one patch.
> 
> >
> > usbip_get_device() method was not used. The implementation of the
> > method searches a bound devices list by list index. The position in the
> > list is meaningless for any operations so it is assumed not to be used
> > in future.
> >
> > For this patch set, a method to find a bound device in the bound
> > devices list by bus-id is needed. usbip_get_device() is re-implemented
> > as a function to find a bound device by bus-id.
> 
> This is not an accurate description. You are really changing look ups using
> bus-id instead bus-num and whether usbip_get_device() is used or not is
> irrelevant.
> 
> Please make changes to find bound device by bus-id in a separate patch.
> You will have to change usbip_get_device() anyway because you are
> changing
> .get_device() interface. Include exporting usbip_get_debice() in a separate
> patch.
> 
> >
> > bind and unbind function are exported to reuse by new connect and
> > disconnect operation. Here, connect and disconnect is NEW-3 and
> NEW-4
> > respactively in diagram below.
> 
> Please don't include exporting bind_device() and unbind_device() and
> changing their names in this patch. Send a separate patch for that.
> 
> It makes sense to move the functions you are exporting bind_device(),
> unbind_device(), and usbip_get_device() to libsrc - usbip_common.c
> might be a good choice.
> 
> The following isn't part of this patch, hence shouldn't be included in
> the change log.
> 
> >
> > EXISTING) - invites devices from application(vhci)-side
> >  +--+
> +--+
> >  device--+ STUB |   | application/VHCI |
> >  +--+
> +--+
> >  (server)   (client)
> >  1) # usbipd ... start daemon
> >  = = =
> >  2) # usbip list --local
> >  3) # usbip bind
> >   <--- list bound devices ---  4) # usbip list --remote
> >   <--- import a device --  5) # usbip attach
> >  = = =
> >  X disconnected6) # usbip detach
> >  7) usbip unbind
> >
> > NEW) - dedicates devices from device(stub)-side
> >  +--+
> +--+
> >  device--+ STUB |   | application/VHCI |
> >  +--+
> +--+
> >  (client)   (server)
> > 1) # usbipa ... start
> daemon
> >  = = =
> >  2) # usbip list --local
> >  3) # usbip connect--- export a device -->
> >  = = =
> >  4) # usbip disconnect --- un-export a device --->
> >
> >  Bind and unbind are done in connect and disconnect internally.
> >
> > Signed-off-by: Nobuo Iwata 
> > ---
> >  tools/usb/usbip/libsrc/usbip_host_common.c | 6 ++
> >  tools/usb/usbip/libsrc/usbip_host_common.h | 8 
> >  tools/usb/usbip/src/usbip.h| 3 +++
> >  

[PATCH v2] usb: host: xhci: plat: check hcc_params after add hcd

[William Wu]

From: William wu 

The commit 4ac53087d6d4 ("usb: xhci: plat: Create both
HCDs before adding them") move add hcd to the end of
probe, this cause hcc_params uninitiated, because xHCI
driver sets hcc_params in xhci_gen_setup() called from
usb_add_hcd().

This patch checks the Maximum Primary Stream Array Size
in the hcc_params register after add primary hcd.

Signed-off-by: William wu 
---
 drivers/usb/host/xhci-plat.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index ddfab30..f96caeb 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -232,9 +232,6 @@ static int xhci_plat_probe(struct platform_device *pdev)
if (device_property_read_bool(>dev, "usb3-lpm-capable"))
xhci->quirks |= XHCI_LPM_SUPPORT;
 
-   if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
-   xhci->shared_hcd->can_do_streams = 1;
-
hcd->usb_phy = devm_usb_get_phy_by_phandle(>dev, "usb-phy", 0);
if (IS_ERR(hcd->usb_phy)) {
ret = PTR_ERR(hcd->usb_phy);
@@ -251,6 +248,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
if (ret)
goto disable_usb_phy;
 
+   if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
+   xhci->shared_hcd->can_do_streams = 1;
+
ret = usb_add_hcd(xhci->shared_hcd, irq, IRQF_SHARED);
if (ret)
goto dealloc_usb2_hcd;
-- 
2.7.4


--
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: [PATCH v14 02/10] usbip: exporting devices: modifications to host side libraries

[fx IWATA NOBUO]

Dear Krzysztof,

> Great. Now make this a separate patch and send separated from this
> series.

I'd like to separate a patch to remove unused old usbip_get_device() 
method from this series.

Exported bind/unbind() and new finding device method are used only 
by new operations.
So I'd like to divide them into 2 patches and keep them in this series.

> usbip: Make bind and unbind helpers global functions

I will use this phrase for the first patch.

> > +   if (!strncmp(busid, edev->udev.busid,
> SYSFS_BUS_ID_SIZE))
> 
> does it really make sense to use strncmp() instead of strcmp() here?

No.
Both busid and edev->udev.busid must be valid string here.

> s/respactively/respectively
Yes. Thanks.

Best Regards,

Nobuo Iwata
//
> -Original Message-
> From: Krzysztof Opasiak [mailto:k.opas...@samsung.com]
> Sent: Thursday, January 12, 2017 9:29 PM
> To: fx IWATA NOBUO ;
> valentina.mane...@gmail.com; sh...@kernel.org;
> gre...@linuxfoundation.org; linux-usb@vger.kernel.org
> Cc: fx MICHIMURA TADAO 
> Subject: Re: [PATCH v14 02/10] usbip: exporting devices: modifications to
> host side libraries
> 
> 
> 
> On 12/26/2016 08:08 AM, Nobuo Iwata wrote:
> > Modifications to host driver wrapper and its related operations (i.e.
> > bind/unbind).
> >
> > usbip_get_device() method was not used. The implementation of the
> > method searches a bound devices list by list index. The position in the
> > list is meaningless for any operations so it is assumed not to be used
> > in future.
> >
> > For this patch set, a method to find a bound device in the bound
> > devices list by bus-id is needed. usbip_get_device() is re-implemented
> > as a function to find a bound device by bus-id.
> 
> Great. Now make this a separate patch and send separated from this
> series.
> 
> PS. remember to fix title for this commit.
> 
> >
> > bind and unbind function are exported to reuse by new connect and
> > disconnect operation. Here, connect and disconnect is NEW-3 and
> NEW-4
> > respactively in diagram below.
> 
> s/respactively/respectively
> 
> and then this could be a commit titled:
> 
> usbip: Make bind and unbind helpers global functions
> 
> >
> > EXISTING) - invites devices from application(vhci)-side
> >  +--+
> +--+
> >  device--+ STUB |   | application/VHCI |
> >  +--+
> +--+
> >  (server)   (client)
> >  1) # usbipd ... start daemon
> >  = = =
> >  2) # usbip list --local
> >  3) # usbip bind
> >   <--- list bound devices ---  4) # usbip list --remote
> >   <--- import a device --  5) # usbip attach
> >  = = =
> >  X disconnected6) # usbip detach
> >  7) usbip unbind
> >
> > NEW) - dedicates devices from device(stub)-side
> >  +--+
> +--+
> >  device--+ STUB |   | application/VHCI |
> >  +--+
> +--+
> >  (client)   (server)
> > 1) # usbipa ... start
> daemon
> >  = = =
> >  2) # usbip list --local
> >  3) # usbip connect--- export a device -->
> >  = = =
> >  4) # usbip disconnect --- un-export a device --->
> >
> >  Bind and unbind are done in connect and disconnect internally.
> >
> > Signed-off-by: Nobuo Iwata 
> > ---
> >  tools/usb/usbip/libsrc/usbip_host_common.c | 6 ++
> >  tools/usb/usbip/libsrc/usbip_host_common.h | 8 
> >  tools/usb/usbip/src/usbip.h| 3 +++
> >  tools/usb/usbip/src/usbip_bind.c   | 4 ++--
> >  tools/usb/usbip/src/usbip_unbind.c | 4 ++--
> >  5 files changed, 13 insertions(+), 12 deletions(-)
> >
> > diff --git a/tools/usb/usbip/libsrc/usbip_host_common.c
> b/tools/usb/usbip/libsrc/usbip_host_common.c
> > index 9d41522..6a98d6c 100644
> > --- a/tools/usb/usbip/libsrc/usbip_host_common.c
> > +++ b/tools/usb/usbip/libsrc/usbip_host_common.c
> > @@ -256,17 +256,15 @@ int usbip_export_device(struct
> usbip_exported_device *edev, int sockfd)
> >  }
> >
> >  struct usbip_exported_device *usbip_generic_get_device(
> > -   struct usbip_host_driver *hdriver, int num)
> > +   struct usbip_host_driver *hdriver, char *busid)
> >  {
> > struct list_head *i;
> > struct usbip_exported_device *edev;
> > -   int cnt = 0;
> >
> > list_for_each(i, >edev_list) {
> > edev = list_entry(i, struct usbip_exported_device, node);
> > -   if (num == cnt)
> > +   if (!strncmp(busid, edev->udev.busid,
> SYSFS_BUS_ID_SIZE))
> 
> does it really make sense to use strncmp() instead of strcmp() here?
> 
> Best regards,
> --
> Krzysztof Opasiak
> Samsung R Institute Poland
> Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of 

Re: [PATCH 31/37] usb: host: xhci: add urb_enqueue/dequeue/giveback tracers

[Lu Baolu]

Hi,

On 01/16/2017 04:59 PM, Felipe Balbi wrote:
> Hi,
>
> Lu Baolu  writes:
>> Hi,
>>
>> On 12/29/2016 07:01 PM, Felipe Balbi wrote:
>>> These three new tracers will help us tie TRBs into URBs by *also*
>>> looking into URB lifetime.
>> I didn't see anything related to TRBs in the patch.
>> Anything I missed?
> yes. Ordering of events. Seriously, you ought to compile my xhci-cleanup
> patches and run with tracers enabled. Here's what I'm talking about:
>
> before $subject, we would only see TRBs being queued to HW and
> completed, etc. After $subject, we see URBs being queued by class
> drivers, which in turn triggers xHCI driver to prepare TRBs which will
> be queued to HW. Once a completion IRQ fires, we see TRBs being
> "dequeued" from HW and URBs being given back.
>
> IOW, after $subject we know that a particular TRB (or a set of them)
> were queued because an URB was queued. We know the size of the URB, a
> pointer to it, etc. We can actually match urb->transfer_length to
> sum_of_trb_sizes().

Yes. Get it now. Thanks for this comment.

>>> Signed-off-by: Felipe Balbi 
>>> ---
>>>  drivers/usb/host/xhci-ring.c  |  1 +
>>>  drivers/usb/host/xhci-trace.h | 70 
>>> +++
>>>  drivers/usb/host/xhci.c   |  5 
>>>  3 files changed, 76 insertions(+)
>>>
>>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>>> index 0ee7d358b812..1431e0651e78 100644
>>> --- a/drivers/usb/host/xhci-ring.c
>>> +++ b/drivers/usb/host/xhci-ring.c
>>> @@ -627,6 +627,7 @@ static void xhci_giveback_urb_in_irq(struct xhci_hcd 
>>> *xhci,
>>> usb_hcd_unlink_urb_from_ep(hcd, urb);
>>> spin_unlock(>lock);
>>> usb_hcd_giveback_urb(hcd, urb, status);
>>> +   trace_xhci_urb_giveback(urb);
>> There is another trace point in xhci_urb_dequeue().
>>
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> index b49588f..ee46877 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -1535,6 +1535,7 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb 
>> *urb, int status)
>>  
>> usb_hcd_unlink_urb_from_ep(hcd, urb);
>> spin_unlock_irqrestore(>lock, flags);
>> +   trace_xhci_urb_giveback(urb);
> yes, so? I want to know that the URB *was* indeed given back.
>

Got it. There is no need to trace a giveback which ties no real trbs.

I have no further questions about this patch. Thanks.

Best regards,
Lu Baolu
--
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: [PATCH 30/37] usb: host: xhci: make a generic TRB tracer

[Lu Baolu]

Hi,

On 01/16/2017 04:55 PM, Felipe Balbi wrote:
> Hi,
>
> Lu Baolu  writes:
> + __entry->type = ring->type;
> + __entry->field0 = le32_to_cpu(trb->field[0]);
> + __entry->field1 = le32_to_cpu(trb->field[1]);
> + __entry->field2 = le32_to_cpu(trb->field[2]);
> + __entry->field3 = le32_to_cpu(trb->field[3]);
>   ),
> - TP_printk("\ntrb_dma=@%llx, trb_va=@%p, status=%08x, flags=%08x",
> - (unsigned long long) __entry->dma, __entry->va,
> - __entry->status, __entry->flags
> + TP_printk("%s: %s", xhci_ring_type_string(__entry->type),
 How about moving the common fields of a TRB (like TRB type and
 the cycle bit) to the place between ring type and trb decoding string?
 And remove type and cycle decoding in xhci_decode_trb() as well.

 Something like:

 diff --git a/drivers/usb/host/xhci-trace.h b/drivers/usb/host/xhci-trace.h
 index d01524b..f8ef7b8 100644
 --- a/drivers/usb/host/xhci-trace.h
 +++ b/drivers/usb/host/xhci-trace.h
 @@ -132,9 +132,11 @@ DECLARE_EVENT_CLASS(xhci_log_trb,
 __entry->field2 = le32_to_cpu(trb->field[2]);
 __entry->field3 = le32_to_cpu(trb->field[3]);
 ),
 -   TP_printk("%s: %s", xhci_ring_type_string(__entry->type),
 -   xhci_decode_trb(__entry->field0, __entry->field1,
 -   __entry->field2, __entry->field3)
 +   TP_printk("%s: %s: %c: %s", xhci_ring_type_string(__entry->type),
 + xhci_trb_type_string(TRB_FIELD_TO_TYPE(__entry->field3)),
 + __entry->field3 & TRB_CYCLE ? 'C' : 'c',
 + xhci_decode_trb(__entry->field0, __entry->field1,
 + __entry->field2, __entry->field3)
>>> and what do I get with that? What's the actual benefit?
>> I thought that it could make xhci_decode_trb() a bit simpler.
> It'll have a very marginal simplification of a single function that's
> only called from tracepoints. I wonder if there's an actual benefit
> there.

Fair enough.

>
> +DEFINE_EVENT(xhci_log_trb, xhci_handle_event,
> + TP_PROTO(struct xhci_ring *ring, struct xhci_generic_trb *trb),
> + TP_ARGS(ring, trb)
> +);
> +
> +DEFINE_EVENT(xhci_log_trb, xhci_handle_command,
> + TP_PROTO(struct xhci_ring *ring, struct xhci_generic_trb *trb),
> + TP_ARGS(ring, trb)
> +);
> +
> +DEFINE_EVENT(xhci_log_trb, xhci_handle_transfer,
> + TP_PROTO(struct xhci_ring *ring, struct xhci_generic_trb *trb),
> + TP_ARGS(ring, trb)
> +);
> +
> +DEFINE_EVENT(xhci_log_trb, xhci_queue_trb,
> + TP_PROTO(struct xhci_ring *ring, struct xhci_generic_trb *trb),
> + TP_ARGS(ring, trb)
>  );
 xhci_handle_command and xhci_handle_transfer are duplications
 of xhci_handle_event. I suggest to remove them.
>>> no, they are not. They give us more granularity into which events we
>>> want to enable. 
>> Fair enough.
>>
>> But why not defining events for all possible event types as well
>> and dropping the all-in-one xhci_handle_event switch.
>>
>> A single event TRB will be traced twice in the same execution
>> path if xhci_handle_event and xhci_handle_command/transfer
>> are both enabled.
> no, it won't. Commands sit in the Command Ring. Events sit in the Event
> Ring. Transfers sit in the various Endpoint Rings.
>
> Compile the branch, enable the tracers and look at the output.
>
>>> They also make it clear where the even is coming
>>> from. Imagine how the trace would look like if I had a single event and
>>> just called trace_xhci_handle_event() from all locations. How would you
>>> differentiate from all possible call sites?
>> These events happen only in interrupt handler. There are no other
>> possible call sites.
> you misunderstand. Look at the output of the tracepoints and imagine how
> they would look if we had a single event for the TRB class of
> tracepoints.

Oh, yes, I totally misunderstood what events of xhci_handle_command
and xhci_handle_transfer do. I ever thought they are for command or
transfer completion event TRB. Sorry for that.

 How about adding an event for the link TRBs. Something like,

 diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
 index 4bad432..6dc8efb 100644
 --- a/drivers/usb/host/xhci-ring.c
 +++ b/drivers/usb/host/xhci-ring.c
 @@ -173,6 +173,7 @@ static void inc_deq(struct xhci_hcd *xhci, struct 
 xhci_ring *ring)
 ring->num_trbs_free++;
 }
 while (trb_is_link(ring->dequeue)) {
 +   trace_xhci_link_trb(ring, ring->dequeue);
 ring->deq_seg = ring->deq_seg->next;
 ring->dequeue = ring->deq_seg->trbs;
 }
 @@ -211,6 +212,7 @@ static void inc_enq(struct 

RE: [PATCH v14 01/10] usbip: exporting devices: modifications to network header

[fx IWATA NOBUO]

Hello,

> > This patch set implements operations to export and unexport device
> > using pre-defined but un-used export and un-export request and reply.
> 
> Get rid of the above from the changelog.

I will remove it.

> Please don't use file names, instead reference the structures.

I see.
I will not use file name to reference global structures.

> > They become empty struct. Other empty struct, 'op_devlist_request',
> > defined.
> 
> Does this go with this patch. Doesn't look like the above is accurate.
> Don't see op_devlist_request in this patch.

OK.

> As an example this change log would be lot clear as below:
> 
> "Add new status codes to network common header struct op_common.
>  Add corresponding user friendly string for each of the status
>  codes. Change dbg messages to use new status strings"
> 
> There is no need to describe code in the change log.

The revised change log is as below.
---
This patch adds new status codes which are needed for new operations of 
this series to network common header 'struct op_common'. Corresponding 
user friendly string for each of the status are added and a debug 
message is changed to use the new status strings.  
The 'returncode' of export and un-export reply are removed.
---

Best Regards,

Nobuo Iwata
//
> -Original Message-
> From: Shuah Khan [mailto:shua...@osg.samsung.com]
> Sent: Friday, January 13, 2017 2:44 AM
> To: fx IWATA NOBUO ;
> valentina.mane...@gmail.com; sh...@kernel.org;
> gre...@linuxfoundation.org; linux-usb@vger.kernel.org
> Cc: fx MICHIMURA TADAO ; Shuah
> Khan ; Shuah Khan 
> Subject: Re: [PATCH v14 01/10] usbip: exporting devices: modifications to
> network header
> 
> Hi Nobuo,
> 
> On 12/26/2016 12:08 AM, Nobuo Iwata wrote:
> > This patch set implements operations to export and unexport device
> > using pre-defined but un-used export and un-export request and reply.
> 
> Get rid of the above from the changelog.
> 
> >
> > This patch modifies export and un-export reply in
> > tools/usb/usbip/src/usbip_network.h. The 'returncode' of each
> response
> > is removed. The status is set in op_common.status.
> 
> Please don't use file names, instead reference the structures.
> 
> As an example this change log would be lot clear as below:
> 
> "Add new status codes to network common header struct op_common.
>  Add corresponding user friendly string for each of the status
>  codes. Change dbg messages to use new status strings"
> 
> There is no need to describe code in the change log.
> 
> >
> > Following status codes are added for this patch set.
> >   ST_NO_FREE_POR
> >   ST_NOT_FOUND
> >
> > The reason to use op_common.status:
> >   1) usbip_net_recv_op_common() hanles as error when the status is
> other
> >  than ST_OK.
> >   2) The status has a commnet "add more error code".
> >
> > They become empty struct. Other empty struct, 'op_devlist_request',
> > defined.
> 
> Does this go with this patch. Doesn't look like the above is accurate.
> Don't see op_devlist_request in this patch.
> 
> >
> > This patch also includes string translation of the status codes.
> >
> > Signed-off-by: Nobuo Iwata 
> > ---
> >  tools/usb/usbip/src/usbip_network.c | 26
> +-
> >  tools/usb/usbip/src/usbip_network.h |  8 
> >  2 files changed, 29 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/usb/usbip/src/usbip_network.c
> b/tools/usb/usbip/src/usbip_network.c
> > index b4c37e7..069ec54 100644
> > --- a/tools/usb/usbip/src/usbip_network.c
> > +++ b/tools/usb/usbip/src/usbip_network.c
> > @@ -163,6 +163,29 @@ int usbip_net_send_op_common(int sockfd,
> uint32_t code, uint32_t status)
> > return 0;
> >  }
> >
> > +struct op_common_error {
> > +   uint32_tstatus;
> > +   const char  *str;
> > +};
> > +
> > +struct op_common_error op_common_errors[] = {
> > +   {ST_NA, "not available"},
> > +   {ST_NO_FREE_PORT, "no free port"},
> > +   {ST_DEVICE_NOT_FOUND, "device not found"},
> > +   {0, NULL}
> > +};
> > +
> > +static const char *op_common_strerror(uint32_t status)
> > +{
> > +   struct op_common_error *err;
> > +
> > +   for (err = op_common_errors; err->str != NULL; err++) {
> > +   if (err->status == status)
> > +   return err->str;
> > +   }
> > +   return "unknown error";
> > +}
> 
> This loop is unnecessary. Do a range check on the passed in status
> value and index and return the string.
> 
> Move the op_common_errors[] and op_common_strerror()
> usbip_network.h
> This can easily be a macro.
> 
> Change other places ST_OK handling is done. I see some in usbipd.c
> 
> > +
> >  int usbip_net_recv_op_common(int sockfd, uint16_t *code)
> >  {
> > struct op_common op_common;
> > @@ -196,7 +219,8 @@ int usbip_net_recv_op_common(int sockfd,
> uint16_t *code)
> > }
> >
> > if (op_common.status != ST_OK) {
> > -   

Re: [PATCH] usb: dwc3: ep0: Fix the possible missed request for handling delay STATUS phase

[Felipe Balbi]

Hi,

Alan Stern  writes:
> On Mon, 16 Jan 2017, Felipe Balbi wrote:
>
>>  Another point here is that the really robust way of fixing this is to
>>  get rid of USB_GADGET_DELAYED_STATUS altogether and just make sure
>>  gadget drivers know how to queue requests for all three phases of a
>>  Control Transfer.
>> 
>>  A lot of code will be removed from all gadget drivers and UDC drivers
>>  while combining all of it in a single place in composite.c.
>
> Don't forget the legacy drivers.

right. I think EHCI Debug gadget is the only one not using composite.c
though. All others under drivers/usb/gadget/legacy are static
configurations of existing function drivers and all use composite.c

>>  The reason I'm saying this is that other UDC drivers might have similar
>>  races already but they just haven't triggered.
>
>> >> Alright, it's important enough to fix this bug. Can you also have a look
>> >> into dropping USB_GADGET_DELAYED_STATUS altogether? If you're too busy,
>> >> no issues. It'll stay in my queue.
>> >
>> > Okay, I will have a look at f_mass_storage driver to see if we can
>> > drop USB_GADGET_DELAYED_STATUS. Thanks.
>> 
>> not only mass storage. It needs to be done for all drivers. The way to
>> do that is to teach functions that control transfers are composed of two
>> or three phases. If you look at UDC drivers today, they all have
>> peculiarities about control transfers to handle stuff that *maybe*
>> gadget drivers won't handle.
>> 
>> What we should do here is make sure that *all* 3 phases always have a
>> matching usb_ep_queue() coming from the upper layers. Whether
>> composite.c or f_*.c handles it, that's an implementation detail. But
>> just to illustrate the problem, we should be able to get rid of
>> dwc3_ep0_out_start() and assume that the upper layer will call
>> usb_ep_queue() when it wants to receive a new SETUP packet.
>> 
>> Likewise, we should be able to assume that STATUS phase will only start
>> based on a usb_ep_queue() call. That way we can remove
>> USB_GADGET_DELAYED_STATUS altogether, because that will *always* be the
>> case. There will be no races to handle apart from the normal case where
>> XferNotReady can come before or after usb_ep_queue(), but we already
>> have proper handling for that too.
>
> It sounds like you're talking about a major change in the way the 
> gadget subsystem handles control requests.

yeah, not the first time :-)

> We can distinguish three cases.  In the existing implementation, they 
> work like this:
>
> (1) Control-OUT with no data stage.  The gadget driver's setup
>   routine either queues a request on ep0, which the UDC driver 
>   uses for the status stage transfer (so it should be a length-0 
>   IN transfer) and returns 0, or else returns an error, in which
>   case the UDC driver sends a protocol STALL for the status 
>   stage.
>
>   (What the UDC driver should do if the setup routine queues a
>   request on ep0 and then returns an error is undefined.)

correct

> (2) Control-OUT with a data stage.  The gadget driver's setup 
>   routine either queues an OUT request on ep0, which the UDC
>   driver uses for the data stage transfer, or else returns an
>   error, in which case the UDC driver sends a protocol STALL for
>   the data stage.  In the first case, the UDC driver 
>   automatically queues a 0-length IN request for the status 
>   stage; the gadget driver does not get any chance to fail the
>   transfer after the host's data has been successfully received.
>   (IMO this is a bug in the design of the gadget subsystem.)

exactly, what I'm proposing here would let us fix this detail, too.

> (3) Control-IN with a data stage.  The gadget driver's setup 
>   routine either queues an IN request on ep0, which the UDC
>   driver uses for the data stage transfer, or else returns an
>   error, in which case the UDC driver sends a protocol STALL for
>   the data stage.  In the first case, the UDC driver 
>   automatically queues a 0-length OUT request for the status 
>   stage; the gadget driver does not get any chance to fail the
>   transfer after its data has been successfully sent (and I can't 
>   think of any reason for doing this).

right

> In the delayed-status or delayed-data case, the setup routine does not
> queue a request on ep0 before returning 0; instead the gadget driver
> queues this request at a later time in a separate thread.
>
> The gadget driver never calls usb_ep_queue in order to receive the next
> SETUP packet; the UDC driver takes care of SETUP handling
> automatically.

yeah, that's another thing I'd like to change. Currently, we have no
means to either try to implement device-initiated LPM without adding a
ton of hacks to UDC drivers. If we require upper layers (composite.c,
most of the time) to usb_ep_queue() separate requests for all 3 phases

Re: [PATCH] usb: dwc2: fix "iomem 0x00000000" message by setting iomem parameters in usb_hcd

[Heiner Kallweit]

Am 16.01.2017 um 15:05 schrieb Felipe Balbi:
> 
> Hi,
> 
> Heiner Kallweit  writes:
>> Set the iomem parameters in the usb_hcd to fix this misleading
>> message during driver load:
>> dwc2 c910.usb: irq 22, io mem 0x
>>
>> Signed-off-by: Heiner Kallweit 
>> ---
>>  drivers/usb/dwc2/core.h | 3 ++-
>>  drivers/usb/dwc2/hcd.c  | 5 -
>>  drivers/usb/dwc2/hcd.h  | 3 ++-
>>  drivers/usb/dwc2/platform.c | 2 +-
>>  4 files changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>> index 9548d3e0..b66eaeea 100644
>> --- a/drivers/usb/dwc2/core.h
>> +++ b/drivers/usb/dwc2/core.h
>> @@ -1229,7 +1229,8 @@ static inline void dwc2_hcd_connect(struct dwc2_hsotg 
>> *hsotg) {}
>>  static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool 
>> force) {}
>>  static inline void dwc2_hcd_start(struct dwc2_hsotg *hsotg) {}
>>  static inline void dwc2_hcd_remove(struct dwc2_hsotg *hsotg) {}
>> -static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
>> +static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
>> +struct resource *res)
>>  { return 0; }
>>  static inline int dwc2_backup_host_registers(struct dwc2_hsotg *hsotg)
>>  { return 0; }
>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>> index 911c3b36..2cfbd10e 100644
>> --- a/drivers/usb/dwc2/hcd.c
>> +++ b/drivers/usb/dwc2/hcd.c
>> @@ -4964,7 +4964,7 @@ static void dwc2_hcd_release(struct dwc2_hsotg *hsotg)
>>   * USB bus with the core and calls the hc_driver->start() function. It 
>> returns
>>   * a negative error on failure.
>>   */
>> -int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
>> +int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq, struct resource *res)
>>  {
>>  struct usb_hcd *hcd;
>>  struct dwc2_host_chan *channel;
>> @@ -5021,6 +5021,9 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
>>  
>>  hcd->has_tt = 1;
>>  
>> +hcd->rsrc_start = res->start;
>> +hcd->rsrc_len = resource_size(res);
>> +
>>  ((struct wrapper_priv_data *) >hcd_priv)->hsotg = hsotg;
>>  hsotg->priv = hcd;
>>  
>> diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h
>> index 1ed5fa2b..2305b5fb 100644
>> --- a/drivers/usb/dwc2/hcd.h
>> +++ b/drivers/usb/dwc2/hcd.h
>> @@ -521,7 +521,8 @@ static inline u8 dwc2_hcd_is_pipe_out(struct 
>> dwc2_hcd_pipe_info *pipe)
>>  return !dwc2_hcd_is_pipe_in(pipe);
>>  }
>>  
>> -extern int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq);
>> +extern int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
>> + struct resource *res);
>>  extern void dwc2_hcd_remove(struct dwc2_hsotg *hsotg);
>>  
>>  /* Transaction Execution Functions */
>> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
>> index 4fc8c603..5ddc8860 100644
>> --- a/drivers/usb/dwc2/platform.c
>> +++ b/drivers/usb/dwc2/platform.c
>> @@ -445,7 +445,7 @@ static int dwc2_driver_probe(struct platform_device *dev)
>>  }
>>  
>>  if (hsotg->dr_mode != USB_DR_MODE_PERIPHERAL) {
>> -retval = dwc2_hcd_init(hsotg, hsotg->irq);
>> +retval = dwc2_hcd_init(hsotg, hsotg->irq, res);
> 
> This is a good idea, but there's more work that can come out of this, if
> you're interested:
> 
> i) devm_ioremap_resource() could be called by the generic layer
> ii) devm_request_irq() could be move to the generic layer
> iii) dwc2_hcd_init() could also be called generically as long as dr_mode
>  is set properly.
> iv) dwc2_debugfs_init() could be called generically as well
> 
> IOW, dwc2_driver_probe() could be as minimal as:
> 
> static int dwc2_driver_probe(struct platform_device *dev)
> {
>   struct dwc2_hsotg *hsotg;
>   struct resource *res;
>   int retval;
> 
>   hsotg = devm_kzalloc(>dev, sizeof(*hsotg), GFP_KERNEL);
>   if (!hsotg)
>   return -ENOMEM;
> 
>   hsotg->dev = >dev;
> 
>   if (!dev->dev.dma_mask)
>   dev->dev.dma_mask = >dev.coherent_dma_mask;
> 
>   retval = dma_set_coherent_mask(>dev, DMA_BIT_MASK(32));
>   if (retval)
>   return retval;
> 
>   hsotg->res = platform_get_resource(dev, IORESOURCE_MEM, 0);
>   hsotg->irq = platform_get_irq(dev, 0);
> 
>   retval = dwc2_get_dr_mode(hsotg);
>   if (retval)
>   return retval;
> 
>   retval = dwc2_get_hwparams(hsotg);
>   if (retval)
>   return retval;
> 
>   platform_set_drvdata(dev, hsotg);
> 
>   retval = dwc2_core_init(hsotg);
>   if (retval)
>   return retval;
> 
>   return 0;
> }
> 
> dwc2_core_init() needs to implemented, of course, but it could hide all
> details about what to do with IRQs and resources and what not. Assuming
> you can properly initialize clocks, it could even handle clocks
> generically for you.
> 
I see what you mean. I'm just a little confused 

Re: Memory barrier needed with wake_up_process()?

[Felipe Balbi]

Hi,

Alan Stern  writes:
> On Mon, 16 Jan 2017, Felipe Balbi wrote:
>
>> Sorry for the long delay, I finally have more information on this. All
>> this time I was doing something that I never considered to matter: I've
>> been running host and peripheral on the same machine. Now that I have
>> tracepoints on xHCI as well, I could see that these 30 seconds of
>> "nothing" is actuall full of xHCI activity and I can see that for the
>> duration of these 30 seconds preempt depth on the CPU that (eventually)
>> queues a request on dwc3, is always > 1 (sometimes 2, most of the time
>> 1). My conclusion from that is that xHCI (or usbcore ?!?) locks the CPU
>> and g_mass_storage is spinning for over 30 seconds at which point
>> storage.ko (host side class driver) dequeues the request.
>> 
>> I'll see if I can capture a fresh trace with both xHCI and dwc3 with
>> this happening, but probably not today (testing stuff for -rc).
>
> Does anything change if the host and peripheral are separate machines?

couldn't reproduce the problem yet ;-)

-- 
balbi
--
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: Memory barrier needed with wake_up_process()?

[Felipe Balbi]

Hi,

Felipe Balbi  writes:
> Alan Stern  writes:
>> On Mon, 16 Jan 2017, Felipe Balbi wrote:
>>
>>> Sorry for the long delay, I finally have more information on this. All
>>> this time I was doing something that I never considered to matter: I've
>>> been running host and peripheral on the same machine. Now that I have
>>> tracepoints on xHCI as well, I could see that these 30 seconds of
>>> "nothing" is actuall full of xHCI activity and I can see that for the
>>> duration of these 30 seconds preempt depth on the CPU that (eventually)
>>> queues a request on dwc3, is always > 1 (sometimes 2, most of the time
>>> 1). My conclusion from that is that xHCI (or usbcore ?!?) locks the CPU
>>> and g_mass_storage is spinning for over 30 seconds at which point
>>> storage.ko (host side class driver) dequeues the request.
>>> 
>>> I'll see if I can capture a fresh trace with both xHCI and dwc3 with
>>> this happening, but probably not today (testing stuff for -rc).
>>
>> Does anything change if the host and peripheral are separate machines?
>
> couldn't reproduce the problem yet ;-)

*yet* is a keyword here. I wouldn't call this problem "done" until I
 successfully run my test case for at least a week.

-- 
balbi
--
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: [RFC][PATCH 0/5 v2] Fixes and workarounds for dwc2 on HiKey board

[John Stultz]

On Mon, Jan 16, 2017 at 2:29 AM, Felipe Balbi
 wrote:
>
> Hi,
>
> John Stultz  writes:
>> I just wanted to re-send my current queue of patches for dwc2
>> controller on the HiKey board, as my last patchset ended up
>> colliding with a number of changes that landed in the 4.10-rc
>> merge window.
>>
>> I've fixed things up and validated these against HiKey. I also
>> made a small change to one of the patches as suggested by Vardan.
>>
>> This does exclude my patchset[1] to add extcon support to dwc2,
>> which John Youn suspects a pending rework of the dwc2 fifo init
>> logic might make unnecssary (however, no such changes appeared
>> to have landed in 4.10-rc, so I'm still using that patchset in
>> my testing).
>>
>> I'm also out for the holidays after today, and while I suspect
>> others are likely going to be out as well, I figured I'd send
>> these out for a chance at review, rather then sit on them for
>> two weeks. I'll resend after the new year addressing any
>> feedback I do get though.
>>
>> Feedback would be greatly appreciated!
>
> John Y, any comments?

So from this JohnY submitted one patch for 4.10-rc (which hasn't yet
landed) and suggested I re-spin the other 4 on top of his for-next
branch. However, that patchset you said doesn't apply to your tree.

Its not really clear who's tree I should be targeting here?

thanks
-john
--
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: [PATCH 1/4 v3] usb: dwc2: Avoid sleeping while holding hsotg->lock

[John Stultz]

On Mon, Jan 16, 2017 at 2:36 AM, Felipe Balbi
 wrote:
>
> Hi,
>
> John Stultz  writes:
>> Basically when plugging in various cables in different orders, I'm
>> occasionally seeing the following BUG splat:
>>
>> [   86.215403] BUG: scheduling while atomic: kworker/u16:2/53/0x0002
>> [   86.219164] usb 1-1: USB disconnect, device number 9
>> [   86.226845] Preemption disabled at:[   86.230218]
>> [] dwc2_conn_id_status_change+0x120/0x250
>> [   86.236894] CPU: 0 PID: 53 Comm: kworker/u16:2 Tainted: GW
>>  4.9.0-rc8-00051-gd5a7979-dirty #1702
>> [   86.246836] Hardware name: HiKey Development Board (DT)
>> [   86.252100] Workqueue: dwc2 dwc2_conn_id_status_change
>> [   86.257279] Call trace:
>> [   86.259771] [] dump_backtrace+0x0/0x1a0
>> [   86.265210] [] show_stack+0x14/0x20
>> [   86.270308] [] dump_stack+0x90/0xb0
>> [   86.275401] [] __schedule_bug+0x6c/0xb8
>> [   86.280841] [] __schedule+0x4f8/0x5b0
>> [   86.286099] [] schedule+0x38/0xa0
>> [   86.291017] [] schedule_hrtimeout_range_clock+0x8c/0xf0
>> [   86.297846] [] schedule_hrtimeout_range+0x10/0x18
>> [   86.304150] [] usleep_range+0x50/0x58
>> [   86.309418] [] dwc2_wait_for_mode.isra.4+0x54/0xd0
>> [   86.315815] [] dwc2_core_reset+0xe0/0x168
>> [   86.321431] [] 
>> dwc2_hsotg_core_init_disconnected+0x2c/0x310
>> [   86.328602] [] dwc2_conn_id_status_change+0x130/0x250
>> [   86.335254] [] process_one_work+0x118/0x370
>> [   86.341035] [] worker_thread+0x48/0x498
>> [   86.346473] [] kthread+0xd0/0xe8
>> [   86.351299] [] ret_from_fork+0x10/0x50
>>
>> This seems to be caused by the dwc2_wait_for_mode() calling
>> usleep_range() while the hstog->lock spinlock is held, since
>> we take that before calling dwc2_hsotg_core_init_disconnected().
>>
>> This patch avoids the issue by adding an extra argument to
>> dwc2_core_reset(), as suggested by John Youn, which allows us to
>> skip the waiting, which should be unnecessary when calling from
>> dwc2_hsotg_core_init_disconnected().
>>
>> Cc: Wei Xu 
>> Cc: Guodong Xu 
>> Cc: Amit Pundir 
>> Cc: Rob Herring 
>> Cc: John Youn 
>> Cc: Douglas Anderson 
>> Cc: Chen Yu 
>> Cc: Vardan Mikayelyan 
>> Cc: Kishon Vijay Abraham I 
>> Cc: Felipe Balbi 
>> Cc: Greg Kroah-Hartman 
>> Cc: linux-usb@vger.kernel.org
>> Signed-off-by: John Stultz 
>> ---
>
> doesn't apply to my testing/next. Please rebase

So these were rebased onto JohnY's tree here:
  https://github.com/synopsys-usb/linux.git next

And apparently have been merged there. I suspect he's going to submit
his entire tree there to you?

JohnY: Is this right?

thanks
-john
--
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: [PATCH 1/4 v3] usb: dwc2: Avoid sleeping while holding hsotg->lock

[John Stultz]

On Mon, Jan 16, 2017 at 12:57 PM, John Youn  wrote:
>
>
>> On Jan 16, 2017, at 12:37 PM, John Stultz  wrote:
>>
>> On Mon, Jan 16, 2017 at 2:36 AM, Felipe Balbi
>>  wrote:
>>>
>>> Hi,
>>>
>>> John Stultz  writes:
 Basically when plugging in various cables in different orders, I'm
 occasionally seeing the following BUG splat:

 [   86.215403] BUG: scheduling while atomic: kworker/u16:2/53/0x0002
 [   86.219164] usb 1-1: USB disconnect, device number 9
 [   86.226845] Preemption disabled at:[   86.230218]
 [] dwc2_conn_id_status_change+0x120/0x250
 [   86.236894] CPU: 0 PID: 53 Comm: kworker/u16:2 Tainted: GW
 4.9.0-rc8-00051-gd5a7979-dirty #1702
 [   86.246836] Hardware name: HiKey Development Board (DT)
 [   86.252100] Workqueue: dwc2 dwc2_conn_id_status_change
 [   86.257279] Call trace:
 [   86.259771] [] dump_backtrace+0x0/0x1a0
 [   86.265210] [] show_stack+0x14/0x20
 [   86.270308] [] dump_stack+0x90/0xb0
 [   86.275401] [] __schedule_bug+0x6c/0xb8
 [   86.280841] [] __schedule+0x4f8/0x5b0
 [   86.286099] [] schedule+0x38/0xa0
 [   86.291017] [] 
 schedule_hrtimeout_range_clock+0x8c/0xf0
 [   86.297846] [] schedule_hrtimeout_range+0x10/0x18
 [   86.304150] [] usleep_range+0x50/0x58
 [   86.309418] [] dwc2_wait_for_mode.isra.4+0x54/0xd0
 [   86.315815] [] dwc2_core_reset+0xe0/0x168
 [   86.321431] [] 
 dwc2_hsotg_core_init_disconnected+0x2c/0x310
 [   86.328602] [] dwc2_conn_id_status_change+0x130/0x250
 [   86.335254] [] process_one_work+0x118/0x370
 [   86.341035] [] worker_thread+0x48/0x498
 [   86.346473] [] kthread+0xd0/0xe8
 [   86.351299] [] ret_from_fork+0x10/0x50

 This seems to be caused by the dwc2_wait_for_mode() calling
 usleep_range() while the hstog->lock spinlock is held, since
 we take that before calling dwc2_hsotg_core_init_disconnected().

 This patch avoids the issue by adding an extra argument to
 dwc2_core_reset(), as suggested by John Youn, which allows us to
 skip the waiting, which should be unnecessary when calling from
 dwc2_hsotg_core_init_disconnected().

 Cc: Wei Xu 
 Cc: Guodong Xu 
 Cc: Amit Pundir 
 Cc: Rob Herring 
 Cc: John Youn 
 Cc: Douglas Anderson 
 Cc: Chen Yu 
 Cc: Vardan Mikayelyan 
 Cc: Kishon Vijay Abraham I 
 Cc: Felipe Balbi 
 Cc: Greg Kroah-Hartman 
 Cc: linux-usb@vger.kernel.org
 Signed-off-by: John Stultz 
 ---
>>>
>>> doesn't apply to my testing/next. Please rebase
>>
>> So these were rebased onto JohnY's tree here:
>>  
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_synopsys-2Dusb_linux.git=DwIBaQ=DPL6_X_6JkXFx7AXWqB0tg=U3o8uKoKhWme5_V9D-eeCkB11BFwt4KvWztBgdE9ZpA=eBJTBuJyU21iKJvHdy5FxqtxsVARo0iIqJGxMHrlbyQ=VIiAT32aG7s04G5NoOOthNdm2JX0eWjJpg62neY_-KI=
>>   next
>>
>> And apparently have been merged there. I suspect he's going to submit
>> his entire tree there to you?
>>
>> JohnY: Is this right?
>
>
> Yeah I'll get these issues sorted out with Felipe. Which may mean 
> resubmitting everything the the proper order.

Ok. Thanks for sorting this out. Please let me know if there's
anything else you need from me!

thanks again!
-john
--
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: [linux-sunxi] [PATCH 1/4] phy: sun4i-usb: support PHY0 on H3 in MUSB mode

[Ondřej Jirman]

Dne 16.1.2017 v 20:14 Icenowy Zheng napsal(a):
> The PHY0 on H3 can be wired either to MUSB controller or OHCI/EHCI
> controller.
> 
> The original driver wired it to OHCI/EHCI controller; however, as the
> code to use PHY0 as OHCI/EHCI is missing, it makes the PHY fully
> unusable.
> 
> Rename the register (according to its function and the name in BSP
> driver), and remove the code which wires the PHY0 to OHCI/EHCI, as MUSB
> can support both peripheral and host mode (although the host mode of
> MUSB is buggy).
> 
> The register that is renamed is now unused, as its initial value is just
> MUSB mode. However, when OHCI/EHCI mode support is added, the register
> can be used again.
> 
> Signed-off-by: Icenowy Zheng 
> ---
>  drivers/phy/phy-sun4i-usb.c | 25 +
>  1 file changed, 9 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c
> index bf28a0fdd569..6b193a635c6b 100644
> --- a/drivers/phy/phy-sun4i-usb.c
> +++ b/drivers/phy/phy-sun4i-usb.c
> @@ -49,7 +49,7 @@
>  #define REG_PHYBIST  0x08
>  #define REG_PHYTUNE  0x0c
>  #define REG_PHYCTL_A33   0x10
> -#define REG_PHY_UNK_H3   0x20
> +#define REG_PHY_OTGCTL   0x20

You have added REG_PHY_OTGCTL, but it is not used below.

regards,
  o.

>  #define REG_PMU_UNK1 0x10
>  
> @@ -269,23 +269,16 @@ static int sun4i_usb_phy_init(struct phy *_phy)
>   writel(val & ~2, phy->pmu + REG_PMU_UNK1);
>   }
>  
> - if (data->cfg->type == sun8i_h3_phy) {
> - if (phy->index == 0) {
> - val = readl(data->base + REG_PHY_UNK_H3);
> - writel(val & ~1, data->base + REG_PHY_UNK_H3);
> - }
> - } else {
> - /* Enable USB 45 Ohm resistor calibration */
> - if (phy->index == 0)
> - sun4i_usb_phy_write(phy, PHY_RES45_CAL_EN, 0x01, 1);
> + /* Enable USB 45 Ohm resistor calibration */
> + if (phy->index == 0)
> + sun4i_usb_phy_write(phy, PHY_RES45_CAL_EN, 0x01, 1);
>  
> - /* Adjust PHY's magnitude and rate */
> - sun4i_usb_phy_write(phy, PHY_TX_AMPLITUDE_TUNE, 0x14, 5);
> + /* Adjust PHY's magnitude and rate */
> + sun4i_usb_phy_write(phy, PHY_TX_AMPLITUDE_TUNE, 0x14, 5);
>  
> - /* Disconnect threshold adjustment */
> - sun4i_usb_phy_write(phy, PHY_DISCON_TH_SEL,
> - data->cfg->disc_thresh, 2);
> - }
> + /* Disconnect threshold adjustment */
> + sun4i_usb_phy_write(phy, PHY_DISCON_TH_SEL,
> + data->cfg->disc_thresh, 2);
>  
>   sun4i_usb_phy_passby(phy, 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: [PATCHv2] dmaengine: cppi41: Fix oops in cppi41_runtime_resume

[Tony Lindgren]

* Tony Lindgren  [170113 14:00]:
> * Grygorii Strashko  [170113 13:37]:
> > > Simplified diff with fix on top of your patch:
> > > 
> > > diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
> > > index ce37a1a..9e9403a 100644
> > > --- a/drivers/dma/cppi41.c
> > > +++ b/drivers/dma/cppi41.c
> > > @@ -319,12 +319,6 @@ static irqreturn_t cppi41_irq(int irq, void *data)
> > >  
> > >   while (val) {
> > >   u32 desc, len;
> > > - int error;
> > > -
> > > - error = pm_runtime_get(cdd->ddev.dev);
> > > - if ((error != -EINPROGRESS) && (error < 0))
> > > - dev_err(cdd->ddev.dev, "%s pm runtime get: 
> > > %i\n",
> > > - __func__, error);
> > >  
> > >   /* This warning should never trigger */
> > >   WARN_ON(cdd->is_suspended);
> > > @@ -500,7 +494,6 @@ static void cppi41_dma_issue_pending(struct dma_chan 
> > > *chan)
> > >   spin_unlock_irqrestore(>lock, flags);
> > >  
> > >   pm_runtime_mark_last_busy(cdd->ddev.dev);
> > > - pm_runtime_put_autosuspend(cdd->ddev.dev);
> > >  }
> > >  
> > >  static u32 get_host_pd0(u32 length)
> > > 
> > 
> > Ok. this is incorrect in case of USB hub and will just hide the problem
> > by incrementing PM runtime usage counter every time USB host is connected :(
> 
> Yeah if anything changes in those two nested loops, the pm_runtime counts
> get unbalanced.
> 
> > Once USB hub is connected the PM runtime usage counter will became 1 and 
> > stay
> > 1 after disconnection. Which means that some descriptor was pushed in Q, 
> > but IRQ
> > was not triggered.
> > 
> > Don't know how to proceed as I'm not so familiar with musb :(
> 
> I'm now playing with saving the queue manager value and kicking the
> PM runtime if we have transfers in progress. Looks like the dma
> registers are zero while there are transfers in progress, or at least
> I have not yet found any hardware registers that would tell that.

Looks like drivers/usb/musb/musb_cppi41.c is not properly terminating
dma transactions.. The following additional patch makes things behave
without warnings for me.

I'll fold the drivers/dma/cppi41.c changes to $subject patch and repost,
then will post a separate musb fix for drivers/usb/musb/musb_cppi41.c
that avoids the warning after some more investigating.

Luckily the warning is harmless in this case as musb and cppi41 are
in the same device so the common parent is powered and cppi41 is
getting clocked.

Regards,

Tony

8< ---
diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
--- a/drivers/dma/cppi41.c
+++ b/drivers/dma/cppi41.c
@@ -705,19 +705,31 @@ static int cppi41_stop_chan(struct dma_chan *chan)
u32 desc_phys;
int ret;
 
+   ret = pm_runtime_get(cdd->ddev.dev);
+   if ((ret != -EINPROGRESS) && (ret < 0))
+   dev_err(cdd->ddev.dev, "%s pm runtime get: %i\n",
+   __func__, ret);
+   WARN_ON(cdd->is_suspended);
+
desc_phys = lower_32_bits(c->desc_phys);
desc_num = (desc_phys - cdd->descs_phys) / sizeof(struct cppi41_desc);
-   if (!cdd->chan_busy[desc_num])
-   return 0;
+   if (!cdd->chan_busy[desc_num]) {
+   ret = 0;
+   goto out;
+   }
 
ret = cppi41_tear_down_chan(c);
if (ret)
-   return ret;
+   goto out;
 
WARN_ON(!cdd->chan_busy[desc_num]);
cdd->chan_busy[desc_num] = NULL;
 
-   return 0;
+out:
+   pm_runtime_mark_last_busy(cdd->ddev.dev);
+   pm_runtime_put_autosuspend(cdd->ddev.dev);
+
+   return ret;
 }
 
 static void cleanup_chans(struct cppi41_dd *cdd)
diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c
--- a/drivers/usb/musb/musb_cppi41.c
+++ b/drivers/usb/musb/musb_cppi41.c
@@ -445,9 +445,16 @@ static struct dma_channel 
*cppi41_dma_channel_allocate(struct dma_controller *c,
 static void cppi41_dma_channel_release(struct dma_channel *channel)
 {
struct cppi41_dma_channel *cppi41_channel = channel->private_data;
+   struct musb_hw_ep *hw_ep = cppi41_channel->hw_ep;
+   int error;
 
trace_musb_cppi41_free(cppi41_channel);
if (cppi41_channel->is_allocated) {
+   error = dmaengine_terminate_all(cppi41_channel->dc);
+   if (error)
+   dev_err(hw_ep->musb->controller,
+   "dma terminate failed: %i\n",
+   error);
cppi41_channel->is_allocated = 0;
channel->status = MUSB_DMA_STATUS_FREE;
channel->actual_len = 0;
--
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: OHCI: high softirq load

[Antoine Aubert]

Thanks Johan, you save my day.

Indeed, soft irq coming from ftdi_sio updates, with very high rate.
(dyndbg='file drivers/usb/* +p')

reducing latency timer = reducing sofirq.

Problem solved.

Antoine Aubert
a.aub...@overkiz.com

Le 16/01/2017 à 15:47, Johan Hovold a écrit :
> [ +CC: linux-usb ]
>
> On Mon, Jan 16, 2017 at 12:14:03PM +0100, Boris Brezillon wrote:
>> On Mon, 16 Jan 2017 11:54:23 +0100
>> Antoine Aubert  wrote:
>>
>>> Also, I made a big misunderstanding
>>>
>>> With EHCI + OHCI = high level of softirq (USB2.0)
>> Well, the number of irqs and softirqs are likely to be related (you
>> usually trigger a softirq after you received an hardirq).
>>
>>> OHCI only = normal level
>> What about EHCI only? And what happens if you only plug 1 device?
>> Please share the content of /proc/interrupts (and everything you think
>> is relevant) for each of these cases.
>>
>>> Le 16/01/2017 à 11:31, Antoine Aubert a écrit :
 Thx for your answer Boris

 Le 16/01/2017 à 10:02, Boris Brezillon a écrit :  
> Hi Antoine,
>
> On Mon, 16 Jan 2017 08:45:58 +0100
> Antoine Aubert  wrote:
>  
>> Hi,
>>
>> Im working on a AT91SAM9G25cu board
>> (arch/arm/boot/dts/at91-kizboxmini.dts). We use linux-4.1.31, and when
>> OHCI is enabled, I got some wired effects.  
> Can you test on a more recent kernel (4.9 or 4.10-rc4)?  
 I'll give a try, just need little time ;)  
>> eg with 3 FTDI pluged, interrupts: more than 3.5k/s, cpu softirq > 24%,
>> loadavg > 0.5  
> Can you check which interrupt is triggered (cat /proc/interrupts),  
 cat /proc/interrupts
CPU0  
  16:   2286  atmel-aic   1 Level pmc, at91_tick, at91_rtc, ttyS0
  17:  0   PMC  17 Level main_rc_osc
  18:  0   PMC   0 Level main_osc
  19:  0   PMC  16 Level mainck
  20:  0   PMC   1 Level clk-plla
  21:  0   PMC   6 Level clk-utmi
  22:  0   PMC   3 Level clk-master
  23: 945527  atmel-aic  17 Level tc_clkevt
  24:  21815  atmel-aic  20 Level at_hdmac
  25:  0  atmel-aic  21 Level at_hdmac
  30: 120299  atmel-aic  24 Level eth0
  31:   22783651  atmel-aic  22 Level ehci_hcd:usb1, ohci_hcd:usb2
  99:  0  GPIO  16 Edge  PB_RST
 100:  0  GPIO  17 Edge  PB_PROG
 Err:  0
> Note that the ftdi driver uses a low-latency setting by default which
> implies that the device sends a status update every millisecond. Hence,
> the 1k interrupts per second (per device) while the port is open is
> expected.
>
> You can disable the low-latency behaviour using setserial:
>
>   setserial /dev/ttyUSB0 ^low_latency
>
> and see the number of interrupts drop to 1/16th. This can then be
> reduced further by changing the latency_timer from its
> (non-low-latency) default of 16 ms, for example:
>
>   echo 64 >/sys/bus/usb-serial/devices/ttyUSB0/latency_timer
>
> Johan


--
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: [PATCH 1/4 v3] usb: dwc2: Avoid sleeping while holding hsotg->lock

[John Youn]

> On Jan 16, 2017, at 12:37 PM, John Stultz  wrote:
> 
> On Mon, Jan 16, 2017 at 2:36 AM, Felipe Balbi
>  wrote:
>> 
>> Hi,
>> 
>> John Stultz  writes:
>>> Basically when plugging in various cables in different orders, I'm
>>> occasionally seeing the following BUG splat:
>>> 
>>> [   86.215403] BUG: scheduling while atomic: kworker/u16:2/53/0x0002
>>> [   86.219164] usb 1-1: USB disconnect, device number 9
>>> [   86.226845] Preemption disabled at:[   86.230218]
>>> [] dwc2_conn_id_status_change+0x120/0x250
>>> [   86.236894] CPU: 0 PID: 53 Comm: kworker/u16:2 Tainted: GW
>>> 4.9.0-rc8-00051-gd5a7979-dirty #1702
>>> [   86.246836] Hardware name: HiKey Development Board (DT)
>>> [   86.252100] Workqueue: dwc2 dwc2_conn_id_status_change
>>> [   86.257279] Call trace:
>>> [   86.259771] [] dump_backtrace+0x0/0x1a0
>>> [   86.265210] [] show_stack+0x14/0x20
>>> [   86.270308] [] dump_stack+0x90/0xb0
>>> [   86.275401] [] __schedule_bug+0x6c/0xb8
>>> [   86.280841] [] __schedule+0x4f8/0x5b0
>>> [   86.286099] [] schedule+0x38/0xa0
>>> [   86.291017] [] schedule_hrtimeout_range_clock+0x8c/0xf0
>>> [   86.297846] [] schedule_hrtimeout_range+0x10/0x18
>>> [   86.304150] [] usleep_range+0x50/0x58
>>> [   86.309418] [] dwc2_wait_for_mode.isra.4+0x54/0xd0
>>> [   86.315815] [] dwc2_core_reset+0xe0/0x168
>>> [   86.321431] [] 
>>> dwc2_hsotg_core_init_disconnected+0x2c/0x310
>>> [   86.328602] [] dwc2_conn_id_status_change+0x130/0x250
>>> [   86.335254] [] process_one_work+0x118/0x370
>>> [   86.341035] [] worker_thread+0x48/0x498
>>> [   86.346473] [] kthread+0xd0/0xe8
>>> [   86.351299] [] ret_from_fork+0x10/0x50
>>> 
>>> This seems to be caused by the dwc2_wait_for_mode() calling
>>> usleep_range() while the hstog->lock spinlock is held, since
>>> we take that before calling dwc2_hsotg_core_init_disconnected().
>>> 
>>> This patch avoids the issue by adding an extra argument to
>>> dwc2_core_reset(), as suggested by John Youn, which allows us to
>>> skip the waiting, which should be unnecessary when calling from
>>> dwc2_hsotg_core_init_disconnected().
>>> 
>>> Cc: Wei Xu 
>>> Cc: Guodong Xu 
>>> Cc: Amit Pundir 
>>> Cc: Rob Herring 
>>> Cc: John Youn 
>>> Cc: Douglas Anderson 
>>> Cc: Chen Yu 
>>> Cc: Vardan Mikayelyan 
>>> Cc: Kishon Vijay Abraham I 
>>> Cc: Felipe Balbi 
>>> Cc: Greg Kroah-Hartman 
>>> Cc: linux-usb@vger.kernel.org
>>> Signed-off-by: John Stultz 
>>> ---
>> 
>> doesn't apply to my testing/next. Please rebase
> 
> So these were rebased onto JohnY's tree here:
>  
> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_synopsys-2Dusb_linux.git=DwIBaQ=DPL6_X_6JkXFx7AXWqB0tg=U3o8uKoKhWme5_V9D-eeCkB11BFwt4KvWztBgdE9ZpA=eBJTBuJyU21iKJvHdy5FxqtxsVARo0iIqJGxMHrlbyQ=VIiAT32aG7s04G5NoOOthNdm2JX0eWjJpg62neY_-KI=
>   next
> 
> And apparently have been merged there. I suspect he's going to submit
> his entire tree there to you?
> 
> JohnY: Is this right?


Yeah I'll get these issues sorted out with Felipe. Which may mean resubmitting 
everything the the proper order.

Thanks,
John

--
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: [PATCHv2] dmaengine: cppi41: Fix oops in cppi41_runtime_resume

[Tony Lindgren]

* Tony Lindgren  [170116 15:36]:
> * Tony Lindgren  [170113 14:00]:
> > * Grygorii Strashko  [170113 13:37]:
> > > > Simplified diff with fix on top of your patch:
> > > > 
> > > > diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
> > > > index ce37a1a..9e9403a 100644
> > > > --- a/drivers/dma/cppi41.c
> > > > +++ b/drivers/dma/cppi41.c
> > > > @@ -319,12 +319,6 @@ static irqreturn_t cppi41_irq(int irq, void *data)
> > > >  
> > > > while (val) {
> > > > u32 desc, len;
> > > > -   int error;
> > > > -
> > > > -   error = pm_runtime_get(cdd->ddev.dev);
> > > > -   if ((error != -EINPROGRESS) && (error < 0))
> > > > -   dev_err(cdd->ddev.dev, "%s pm runtime 
> > > > get: %i\n",
> > > > -   __func__, error);
> > > >  
> > > > /* This warning should never trigger */
> > > > WARN_ON(cdd->is_suspended);
> > > > @@ -500,7 +494,6 @@ static void cppi41_dma_issue_pending(struct 
> > > > dma_chan *chan)
> > > > spin_unlock_irqrestore(>lock, flags);
> > > >  
> > > > pm_runtime_mark_last_busy(cdd->ddev.dev);
> > > > -   pm_runtime_put_autosuspend(cdd->ddev.dev);
> > > >  }
> > > >  
> > > >  static u32 get_host_pd0(u32 length)
> > > > 
> > > 
> > > Ok. this is incorrect in case of USB hub and will just hide the problem
> > > by incrementing PM runtime usage counter every time USB host is connected 
> > > :(
> > 
> > Yeah if anything changes in those two nested loops, the pm_runtime counts
> > get unbalanced.
> > 
> > > Once USB hub is connected the PM runtime usage counter will became 1 and 
> > > stay
> > > 1 after disconnection. Which means that some descriptor was pushed in Q, 
> > > but IRQ
> > > was not triggered.
> > > 
> > > Don't know how to proceed as I'm not so familiar with musb :(
> > 
> > I'm now playing with saving the queue manager value and kicking the
> > PM runtime if we have transfers in progress. Looks like the dma
> > registers are zero while there are transfers in progress, or at least
> > I have not yet found any hardware registers that would tell that.
> 
> Looks like drivers/usb/musb/musb_cppi41.c is not properly terminating
> dma transactions.. The following additional patch makes things behave
> without warnings for me.
> 
> I'll fold the drivers/dma/cppi41.c changes to $subject patch and repost,
> then will post a separate musb fix for drivers/usb/musb/musb_cppi41.c
> that avoids the warning after some more investigating.
> 
> Luckily the warning is harmless in this case as musb and cppi41 are
> in the same device so the common parent is powered and cppi41 is
> getting clocked.

Sorry actually it's after these fixes when we still need to implement
something for cppi41 PM runtime usecount that makes sense as the
calls will finally be paired. For testing, reverting 098de42ad670
("dmaengine: cppi41: Fix unpaired pm runtime when only a USB hub
is connected") can be done. But I don't like that as it's still
unpaired pm_runtime_calls potentially if something goes wrong.

Anyways, for the -rc series oops, we can just leave out the WARN_ON
parts for now until drivers/usb/musb/musb_cppi41.c is fixed too.

Regards,

Tony


> 8< ---
> diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
> --- a/drivers/dma/cppi41.c
> +++ b/drivers/dma/cppi41.c
> @@ -705,19 +705,31 @@ static int cppi41_stop_chan(struct dma_chan *chan)
>   u32 desc_phys;
>   int ret;
>  
> + ret = pm_runtime_get(cdd->ddev.dev);
> + if ((ret != -EINPROGRESS) && (ret < 0))
> + dev_err(cdd->ddev.dev, "%s pm runtime get: %i\n",
> + __func__, ret);
> + WARN_ON(cdd->is_suspended);
> +
>   desc_phys = lower_32_bits(c->desc_phys);
>   desc_num = (desc_phys - cdd->descs_phys) / sizeof(struct cppi41_desc);
> - if (!cdd->chan_busy[desc_num])
> - return 0;
> + if (!cdd->chan_busy[desc_num]) {
> + ret = 0;
> + goto out;
> + }
>  
>   ret = cppi41_tear_down_chan(c);
>   if (ret)
> - return ret;
> + goto out;
>  
>   WARN_ON(!cdd->chan_busy[desc_num]);
>   cdd->chan_busy[desc_num] = NULL;
>  
> - return 0;
> +out:
> + pm_runtime_mark_last_busy(cdd->ddev.dev);
> + pm_runtime_put_autosuspend(cdd->ddev.dev);
> +
> + return ret;
>  }
>  
>  static void cleanup_chans(struct cppi41_dd *cdd)
> diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c
> --- a/drivers/usb/musb/musb_cppi41.c
> +++ b/drivers/usb/musb/musb_cppi41.c
> @@ -445,9 +445,16 @@ static struct dma_channel 
> *cppi41_dma_channel_allocate(struct dma_controller *c,
>  static void cppi41_dma_channel_release(struct dma_channel *channel)
>  {
>   struct cppi41_dma_channel *cppi41_channel