Re: First kernel patch (optimization)

[Greg KH]

On Thu, Sep 17, 2015 at 11:12:51PM -0400, Theodore Ts'o wrote:
> On Wed, Sep 16, 2015 at 01:26:51PM -0400, Josh Boyer wrote:
> > 
> > That isn't true.  It helps the submitter understand the workflow and
> > expectations.  What you meant to say is that it doesn't help you.
> 
> 
> The problem is that workflow isn't the hard part.  It's the part that
> can be taught most easily, sure.  But people seem to get really hung
> up on it, and I fear that we have people who never progress beyond
> sending trivial patches and spelling fixes and white space fixes and
> micro-optimizations.
> 
> If the "you too can be a kernel developer" classes and web sites and
> tutorials also taught people how to take performance measurements, and
> something about the scientific measurement, that would be something.
> Or if it taught people how to create tests and to run regression
> testing.  Or if it taught people how to try to do fuzz testing, and
> then once they find a sequence which causes crash, how to narrow down
> the failure to a specific part of the kernel, and how to fix and
> confirm that the kernel no longer crashes with the fix --- that would
> be useful.
> 
> If they can understand kernel code; if they can understand the
> scientific measurement; if they can understand how to do performance
> measurements --- being able to properly format patches is something
> which most kernel developers can very easily guide a new contributor
> to do correctly.  Or in the worst case, it doesn't take much time for
> me to fix a whitespace problem and just tell the contributor --- by
> the way, I fixed up this minor issue; could you please make sure you
> do this in the future?
> 
> But if a test hasn't been tested, or if the contributor things it's a
> micro-optimization, but it actually takes more CPU time and/or more
> stack space and/or bloats the kernel --- that's much more work for the
> kernel maintainer to have to deal with when reviewing a patch.
> 
> So I have a very strong disagreement with the belief that teaching
> people the workflow is the more important thing.  In my mind, that's
> like first focusing on the proper how to properly fill out a golf
> score card, and the ettiquette and traditions around handicaps, etc
> --- before making sure the prospective player is good at putting and
> driving.  Personally, I'm terrible at putting and driving, so spending
> a lot of time learning how to fill out a golf score card would be a
> waste of my time.
> 
> A good kernel programmer has to understand systems thinking; how to
> figure out abstractions and when it's a good thing to add a new layer
> of abstraction and when it's better to rework an exsting abstraction
> layer.
> 
> If we have someone who knows the workflow, but which doesn't
> understand systems thinking, or how to do testing, then what?  Great,
> we've just created another Nick Krause.  Do you think encouraging a
> Nick Krause helps anyone?
> 
> If people really are hung up on learning the workflow, I don't mind if
> they want to learn that part and send some silly micro-optimization or
> spelling fix or whitespace fix.  But it's really, really important
> that they move beyond that.  And if they aren't capable of moving
> beyond that, trying to inflate are recruitment numbers by encouraging
> someone who can only do trivial fixes means that we may be get what we
> can easily measure --- but it may not be what we really need as a
> community.

Ted, you are full of crap.

Where do you think that "new developers" come from?  Do they show up in
our inbox, with full knowledge of kernel internals and OS theory yet
they somehow just can't grasp how to submit a patch correctly?  Yes,
they sometimes rarely do.  But for the majority of people who got into
Linux, that is not the case at all.

People need to start with something simple, and easy, to get over the
hurdles of:
- generating a patch
- sending an email
- fixing the email client as it just corrupted the patch
- fix the subject line as it was incorrect
- fix the changelog as it was missing
- fix the email client again as it corrupted the patch in a
  different way
- giving up on using a web email client as it just will not work
- figuring out who to send the patch to
- fixing the email client as the mailing list bounced the email

Those are non-trivial tasks.  And by starting with "remove this space"
you take the worry away from the specific content of the patch, and let
them worry about the "hard" part first.

Then, after all of the above is finished, and working, then they can
start submitting real patches, that do real things, in patch series, as
they can focus on the content much more, as the problems of how to make
the patch into an acceptable format is not an issue anymore.

I see this every single day with patches in the staging tree, which is
why it is there, and is why I don't just go and remove all coding style
errors in o

Re: [PATCH] usb: renesas_usbhs: Add support for R-Car H3

[Kuninori Morimoto]

Hi Shimoda-san

> This patch adds a compatible string to support for R-Car H3.
> 
> Since the HS-USB controller of R-Car H3 is almost the same specification
> with R-Car Gen2 (these have 16 pipes and usb-dmac), this patch
> sets the "type" of renesas_usbhs_driver_param to USBHS_TYPE_RCAR_GEN2.
> 
> Signed-off-by: Yoshihiro Shimoda 
> ---
(snip)
> diff --git a/drivers/usb/renesas_usbhs/common.c 
> b/drivers/usb/renesas_usbhs/common.c
> index 7b98e1d..2becd6b 100644
> --- a/drivers/usb/renesas_usbhs/common.c
> +++ b/drivers/usb/renesas_usbhs/common.c
> @@ -476,6 +476,10 @@ static const struct of_device_id usbhs_of_match[] = {
>   .compatible = "renesas,usbhs-r8a7794",
>   .data = (void *)USBHS_TYPE_RCAR_GEN2,
>   },
> + {
> + .compatible = "renesas,usbhs-r8a7795",
> + .data = (void *)USBHS_TYPE_RCAR_GEN2,
> + },

It is helpful for user if it is indicating Gen2 compatible.
Because r8a7795 is Gen3

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

[GIT PULL] USB chipidea fixes for v4.3-rc2

[Peter Chen]

The following changes since commit 6ff33f3902c3b1c5d0db6b1e2c70b6d76fba357f:

  Linux 4.3-rc1 (2015-09-12 16:35:56 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git/ 
tags/usb-ci-v4.3-rc2

for you to fetch changes up to 8315b77d72c5f0b18ceb513303d845e73166133c:

  usb: chipidea: imx: fix a typo for imx6sx (2015-09-16 13:45:11 +0800)


USB Chipidea fixes for v4.3-rc2

- Fix the stall implementation
- Fix device mode transfer at zynq platform
- other small fixes


Li Jun (1):
  usb: chipidea: imx: fix a typo for imx6sx

Nathan Sullivan (2):
  usb: chipidea: add xilinx zynq platform data
  Documentation: bindings: add doc for zynq USB

Peter Chen (1):
  usb: chipidea: udc: using the correct stall implementation

 .../devicetree/bindings/usb/ci-hdrc-usb2.txt   |  1 +
 drivers/usb/chipidea/ci_hdrc_imx.c |  2 +-
 drivers/usb/chipidea/ci_hdrc_usb2.c| 25 +--
 drivers/usb/chipidea/udc.c | 84 +++---
 4 files changed, 65 insertions(+), 47 deletions(-)

-- 

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: USB bug

[Clemens Ladisch]

Andrew Gillis wrote:
> Sep 17 10:07:38 sonicorbiter kernel: xhci_hcd :06:00.0: ERROR: unexpected 
> command completion code 0x11.

This means that the USB controller does not like what the controller
driver has told it to do.  This could be a bug in some driver, but if
it happens only with that NEC controller, the bug is likely to be there.

> Sep 17 10:07:38 sonicorbiter kernel: usb 1-2: Warning! Unlikely big volume 
> range (=32767), cval->res is proba

This is a firmware bug, but probably unrelated.

> Bus 002 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub

This is not the sound device.

> Sep 17 10:07:39 sonicorbiter kernel: usb 1-2: USB disconnect, device number 3

Either the USB controller or the device got really confused.

Try to get the "lsusb -v" output on some other controller/PC.


Regards,
Clemens
--
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: First kernel patch (optimization)

[Raymond Jennings]

On 09/18/15 00:42, Greg KH wrote:

On Thu, Sep 17, 2015 at 11:12:51PM -0400, Theodore Ts'o wrote:

On Wed, Sep 16, 2015 at 01:26:51PM -0400, Josh Boyer wrote:

That isn't true.  It helps the submitter understand the workflow and
expectations.  What you meant to say is that it doesn't help you.

The problem is that workflow isn't the hard part.  It's the part that
can be taught most easily, sure.  But people seem to get really hung
up on it, and I fear that we have people who never progress beyond
sending trivial patches and spelling fixes and white space fixes and
micro-optimizations.

If the "you too can be a kernel developer" classes and web sites and
tutorials also taught people how to take performance measurements, and
something about the scientific measurement, that would be something.
Or if it taught people how to create tests and to run regression
testing.  Or if it taught people how to try to do fuzz testing, and
then once they find a sequence which causes crash, how to narrow down
the failure to a specific part of the kernel, and how to fix and
confirm that the kernel no longer crashes with the fix --- that would
be useful.

If they can understand kernel code; if they can understand the
scientific measurement; if they can understand how to do performance
measurements --- being able to properly format patches is something
which most kernel developers can very easily guide a new contributor
to do correctly.  Or in the worst case, it doesn't take much time for
me to fix a whitespace problem and just tell the contributor --- by
the way, I fixed up this minor issue; could you please make sure you
do this in the future?

But if a test hasn't been tested, or if the contributor things it's a
micro-optimization, but it actually takes more CPU time and/or more
stack space and/or bloats the kernel --- that's much more work for the
kernel maintainer to have to deal with when reviewing a patch.

So I have a very strong disagreement with the belief that teaching
people the workflow is the more important thing.  In my mind, that's
like first focusing on the proper how to properly fill out a golf
score card, and the ettiquette and traditions around handicaps, etc
--- before making sure the prospective player is good at putting and
driving.  Personally, I'm terrible at putting and driving, so spending
a lot of time learning how to fill out a golf score card would be a
waste of my time.

A good kernel programmer has to understand systems thinking; how to
figure out abstractions and when it's a good thing to add a new layer
of abstraction and when it's better to rework an exsting abstraction
layer.

If we have someone who knows the workflow, but which doesn't
understand systems thinking, or how to do testing, then what?  Great,
we've just created another Nick Krause.  Do you think encouraging a
Nick Krause helps anyone?

If people really are hung up on learning the workflow, I don't mind if
they want to learn that part and send some silly micro-optimization or
spelling fix or whitespace fix.  But it's really, really important
that they move beyond that.  And if they aren't capable of moving
beyond that, trying to inflate are recruitment numbers by encouraging
someone who can only do trivial fixes means that we may be get what we
can easily measure --- but it may not be what we really need as a
community.

Ted, you are full of crap.

Where do you think that "new developers" come from?  Do they show up in
our inbox, with full knowledge of kernel internals and OS theory yet
they somehow just can't grasp how to submit a patch correctly?  Yes,
they sometimes rarely do.  But for the majority of people who got into
Linux, that is not the case at all.

People need to start with something simple, and easy, to get over the
hurdles of:
- generating a patch
- sending an email
- fixing the email client as it just corrupted the patch
- fix the subject line as it was incorrect
- fix the changelog as it was missing
- fix the email client again as it corrupted the patch in a
  different way
- giving up on using a web email client as it just will not work
- figuring out who to send the patch to
- fixing the email client as the mailing list bounced the email

Those are non-trivial tasks.  And by starting with "remove this space"
you take the worry away from the specific content of the patch, and let
them worry about the "hard" part first.

+1 for this.

For example, I for one cannot tell you how many times gmail snuck html 
sections into my outgoing emails before I finally caught it red handed 
and switched to using a local native client.



Then, after all of the above is finished, and working, then they can
start submitting real patches, that do real things, in patch series, as
they can focus on the content much more, as the problems of how to make
the patch into an acceptable format is not an issue anymore.


Did anyone read linus torvald's post that I

Re: USB client crash on Vybrid with USB gadget RNDIS connection

[Felipe Tonello]

Hi Peter,

On Fri, Sep 18, 2015 at 6:39 AM, Peter Chen  wrote:
> On Wed, Sep 16, 2015 at 02:48:50PM +0530, maitysancha...@gmail.com wrote:
>> On 15-09-16 15:54:21, Peter Chen wrote:
>> > On Wed, Sep 16, 2015 at 02:18:49PM +0530, maitysancha...@gmail.com wrote:
>> > > Hello Peter,
>> > >
>> > > >
>> > > > Enable CONFIG_DEBUG_LIST, it has below position if you
>> > > > run make menuconfig
>> > > > Kernel hacking  --->
>> > > > [*] Debug linked list manipulation
>> > > >
>> > >
>> > > Sorry for the delay. When I enabled this config the first time my test
>> > > application ran for 24 hours or so and I did not get any stack traces.
>> > >
>> > > I restarted the test again and finally got the trace below. You were
>> > > spot on, its a list corruption issue. I modified the trace a bit after
>> > > copying to remove the sprinkled debug messages throughout the trace
>> > > from my test application.
>> > >
>> > > [  622.204134] WARNING: CPU: 0 PID: 0 at lib/list_debug.c:59 
>> > > __list_del_entry+0xc4/0xe8()
>> > > [  622.212870] list_del corruption. prev->next should be 8db63600, but 
>> > > was 36008db6
>> >
>> > You see the higher 16 bits were swapped with lower 16 bits, and the
>> > virtual memory address should begin from 0x8, right?
>>
>> Yes, I saw that but beats me how this happens.
>>
>> >
>> > Check with Vybrid errata to see if all ARM/memory system have applied.
>>
>> What do you mean by "all ARM/memory system have applied" ? I checked with 
>> the Vybrid errata
>> and I do not see anything related.
>>
>
> Just system level errata, like ARM Cortex A5, memory (L1/L2 Cache), etc.
>
> Would you please do more tests to see if the error pattern is always
> the same? And print the address to store prev-next.

This looks exactly the same error I am having with MIDI gadget. In my
case in the udc driver the add_td_to_list() is returning an error
after a dma_pool_alloc() failed. In this case I believe is something
related to f_midi not releasing properly the usb request.

Felipe
--
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: MIDI gadget allocating too much from coherent pool

[Felipe Tonello]

After some debugging, here is where I am:

The crash trace is like this:

(f_midi.c)
-> ALSA calls f_midi_in_trigger()
  ->tasklet_hi_schedule(&midi->tasklet)
-> f_midi_transmit(midi, NULL) the NULL here causes the f_midi to
request a usb_request allocation, also it sets req->complete (which is
not been called? I am investigating this)
  -> usb_ep_queue(ep, req, GFP_ATOMIC)
(chipidea/udc.c)
-> ep_queue(ep, req, gfp_flags)
  -> _ep_queue(ep, req, gfp_flags)
-> _hardware_enqueue(hwep, hwreq) here is where it is crashing
  -> add_td_to_list(hwep, hwreq, count) this guy returns
an error from dma_pool_alloc(), out of mem, which is not been checked
(I have a patch for this)
-> lastnode = list_entry(hwreq->tds.prev, struct
td_node, td) get last node (which is NULL)
  -> lastnode->ptr->next = cpu_to_le32(TD_TERMINATE)
CRASH!!! lastnode->ptr is NULL

My idea is that some how the f_midi is not calling free_ep_req(ep,
req) properly. I am still investigating this.

One thing to bear in mind is that I used ether gadget and mass_storage
gadget as well and they both work just fine. Also that the device I am
running generates a *lot* of MIDI output, much more than the normal
usage, which might be triggering this bug that no one previously had.

Any comments?

Felipe
--
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] Add spurious wakeup quirk for Lynxpoint controllers

[Mathias Nyman]

On 10.09.2015 20:27, Laura Abbott wrote:


We received several reports of systems rebooting and powering on
after an attempted shutdown. Testing showed that setting
XHCI_SPURIOUS_WAKEUP quirk in addition to the XHCI_SPURIOUS_REBOOT
quirk allowed the system to shutdown as expected for Lynxpoint
xHCI controllers. Set the qurik.

Signed-off-by: Laura Abbott 
---


We used to have the XHCI_SPURIOUS_WAKEUP flag set for lynxpoint controllers,
but it was removed in commit:

commit b45abacde3d551c6696c6738bef4a1805d0bf27a
xhci: no switching back on non-ULT Haswell

The switch back is limited to ULT even on HP. The contrary

finding arose by bad luck in BIOS versions for testing.
This fixes spontaneous resume from S3 on some HP laptops.

Adding the SPURIOUS_WAKEUP flag back looks reasonable to me,
but I don't want to break suspend.
I don't understand how it could have caused spontaneous resume in HP laptops
in the first place, it really shouldn't do anything before shutdown.

Better ask Oliver,
Do you still have access to the HP laptop?
Any chance you could see if the flag still causes spontaneous resume?

-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: USB client crash on Vybrid with USB gadget RNDIS connection

[maitysanchayan]

On 15-09-18 13:39:11, Peter Chen wrote:
> On Wed, Sep 16, 2015 at 02:48:50PM +0530, maitysancha...@gmail.com wrote:
> > On 15-09-16 15:54:21, Peter Chen wrote:
> > > On Wed, Sep 16, 2015 at 02:18:49PM +0530, maitysancha...@gmail.com wrote:
> > > > Hello Peter,
> > > > 
> > > > > 
> > > > > Enable CONFIG_DEBUG_LIST, it has below position if you
> > > > > run make menuconfig
> > > > > Kernel hacking  --->
> > > > > [*] Debug linked list manipulation  
> > > > > 
> > > > 
> > > > Sorry for the delay. When I enabled this config the first time my test
> > > > application ran for 24 hours or so and I did not get any stack traces.
> > > > 
> > > > I restarted the test again and finally got the trace below. You were
> > > > spot on, its a list corruption issue. I modified the trace a bit after
> > > > copying to remove the sprinkled debug messages throughout the trace
> > > > from my test application.
> > > > 
> > > > [  622.204134] WARNING: CPU: 0 PID: 0 at lib/list_debug.c:59 
> > > > __list_del_entry+0xc4/0xe8()
> > > > [  622.212870] list_del corruption. prev->next should be 8db63600, but 
> > > > was 36008db6
> > > 
> > > You see the higher 16 bits were swapped with lower 16 bits, and the
> > > virtual memory address should begin from 0x8, right?
> > 
> > Yes, I saw that but beats me how this happens.
> > 
> > > 
> > > Check with Vybrid errata to see if all ARM/memory system have applied.
> > 
> > What do you mean by "all ARM/memory system have applied" ? I checked with 
> > the Vybrid errata
> > and I do not see anything related.
> > 
> 
> Just system level errata, like ARM Cortex A5, memory (L1/L2 Cache), etc.
> 
> Would you please do more tests to see if the error pattern is always
> the same?

I got more or less the same logs as below the last five times I tried today
and this time I got the crashes quickly enough somehow. Did not have to wait
for more than half an hour.

> And print the address to store prev-next.

Isn't that what's given by list_del corruption info?

Interesting that atleast one more person Felipe Tonello sees the same issue.

Felipe mentions a DMA issue, I saw a DMA error message from ci_hdrc once in the
last five times I tried but mistakenly I did not take that one down. The message
was something along the lines "ci_hdrc: ci_hdrc bad dma alloc" or similar.

There seems to be corruption on both addition and deletion and only deletion
corruption has it's lower and upper 16 bits swapped! And it is the always the
first reported one that is swapped.

[  476.872014] WARNING: CPU: 0 PID: 0 at lib/list_debug.c:59 
__list_del_entry+0xc4/0xe8()
[  476.880749] list_del corruption. prev->next should be 8daf74c0, but was 
74c08daf
[  476.888147] Modules linked in:
[  476.891239] CPU: 0 PID: 0 Comm: swapper Not tainted 4.1.5-4-g326879d #327
[  476.898378] Hardware name: Freescale Vybrid VF5xx/VF6xx (Device Tree)
[  476.904825] Backtrace:
[  476.907324] [<80012b78>] (dump_backtrace) from [<80012d98>] 
(show_stack+0x18/0x1c)
[  476.914894]  r7:802a5ff4 r6:003b r5:0009 r4:
[  476.920637] [<80012d80>] (show_stack) from [<80590990>] 
(dump_stack+0x24/0x28)
[  476.927884] [<8059096c>] (dump_stack) from [<80023e24>] 
(warn_slowpath_common+0x88/0xb4)
[  476.935993] [<80023d9c>] (warn_slowpath_common) from [<80023e88>] 
(warn_slowpath_fmt+0x38/0x40)
[  476.944694]  r8:8dae9cbc r7:8e02f464 r6:8dae9c80 r5:007e r4:80704238
[  476.951484] [<80023e54>] (warn_slowpath_fmt) from [<802a5ff4>] 
(__list_del_entry+0xc4/0xe8)
[  476.959837]  r3:8daf74c0 r2:80704238
[  476.963439]  r4:8dae9cbc
[  476.966003] [<802a5f30>] (__list_del_entry) from [<803a0740>] 
(udc_irq+0x3d8/0xcdc)
[  476.973676] [<803a0368>] (udc_irq) from [<8039d6bc>] (ci_irq+0x58/0x11c)
[  476.980379]  r10:807ddefe r9:8e0bd480 r8:0027 r7: r6: 
r5:807bd6c4
[  476.988293]  r4:8e02f010
[  476.990860] [<8039d664>] (ci_irq) from [<8004d670>] 
(handle_irq_event_percpu+0x80/0x148)
[  476.998951]  r5:807bd6c4 r4:8e2d0880
[  477.002569] [<8004d5f0>] (handle_irq_event_percpu) from [<8004d768>] 
(handle_irq_event+0x30/0x40)
[  477.011439]  r10:807ac0d4 r9:807d2540 r8:8e006000 r7: r6: 
r5:807bd6c4
[  477.019353]  r4:8e0bd480
[  477.021916] [<8004d738>] (handle_irq_event) from [<8004fce0>] 
(handle_fasteoi_irq+0xa4/0x16c)
[  477.030444]  r5:807bd6c4 r4:8e0bd480
[  477.034063] [<8004fc3c>] (handle_fasteoi_irq) from [<8004cdd8>] 
(generic_handle_irq+0x34/0x44)
[  477.042681]  r5:0027 r4:0027
[  477.046298] [<8004cda4>] (generic_handle_irq) from [<8004d03c>] 
(__handle_domain_irq+0x5c/0xb0)
[  477.054995]  r5:0027 r4:807bd4fc
[  477.058612] [<8004cfe0>] (__handle_domain_irq) from [<80009364>] 
(gic_handle_irq+0x2c/0x5c)
[  477.066963]  r9:807d2540 r8:807ac0cc r7:90002100 r6:807abf20 r5:807ac364 
r4:9000210c
[  477.074803] [<80009338>] (gic_handle_irq) from [<80013800>] 
(__irq_svc+0x40/0x54)
[  477.082292] Exception stack(0x807abf20 to 0x807abf68)
[  477.087364] bf20: 0001  000

Re: [RFC][PATCH] Add spurious wakeup quirk for Lynxpoint controllers

[Oliver Neukum]

On Fri, 2015-09-18 at 13:18 +0300, Mathias Nyman wrote:
> Better ask Oliver,
> Do you still have access to the HP laptop?

No I am sorry, we no longer have those laptops.

Regards
Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: xhci_hcd: unstable communication with Opella-XD JTAG probe

[Mathias Nyman]

On 16.09.2015 00:06, Alexey Brodkin wrote:

Hi Mathias,

On Tue, 2015-09-15 at 16:18 +0300, Mathias Nyman wrote:

On 10.09.2015 14:54, Alexey Brodkin wrote:



I'm not sure what happens here, have you tried the 4.2 kernel?


I've just tried vanilla 4.3-rc1 and see exactly the same picture.


If I send additional xhci debugging patches can you apply and test them?


Sure!


An additional usbmon trace showing which URBs are actually sent could help.


Below are simple "usbmon" outputs. If there's a need for additional
options for usbmon let me know which ones I need to pass.




Thanks,

Looks like a we have at least one bug in how we handle short packet transfers 
for bulk transfer
we expect to be large.

The driver ask for 65664 bytes from the device, but the device only sends 73 
bytes (see log below)

 09c4b540 0.374261 C Bo:1:002:1 0 5 >

09c4b540 0.374312 S Bi:1:002:2 - 65664 <
09c4b540 0.374368 C Bi:1:002:2 0 73 =
  0001 08200716   00100400  
  . . . .  . . . .  .   . .  . . . .  . . . .  . . . .  . . . .  . . . .


If xhci gets less data than requested it will issue a short transfer event. But 
as the
difference is bigger than 64k we will get this event before the last TRB of 
that TD, and then
we will get an additional event for the actual last TRB.

This doesn't yet explain the 5 seconds timeout, but it should solve the
"WARN Event TRB for slot 8 ep 4 with no TDs queued?" warning.

I'll scribble down a patch for this shortly

-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: MIDI gadget allocating too much from coherent pool

[Felipe Balbi]

On Fri, Sep 18, 2015 at 11:03:26AM +0100, Felipe Tonello wrote:
> After some debugging, here is where I am:
> 
> The crash trace is like this:
> 
> (f_midi.c)
> -> ALSA calls f_midi_in_trigger()
>   ->tasklet_hi_schedule(&midi->tasklet)
> -> f_midi_transmit(midi, NULL) the NULL here causes the f_midi to
> request a usb_request allocation, also it sets req->complete (which is
> not been called? I am investigating this)

->complete() is called by the UDC when the request actually completes.

>   -> usb_ep_queue(ep, req, GFP_ATOMIC)
> (chipidea/udc.c)
> -> ep_queue(ep, req, gfp_flags)
>   -> _ep_queue(ep, req, gfp_flags)
> -> _hardware_enqueue(hwep, hwreq) here is where it is crashing
>   -> add_td_to_list(hwep, hwreq, count) this guy returns
> an error from dma_pool_alloc(), out of mem, which is not been checked
> (I have a patch for this)

oh, this probably part of the error. Care to send it ?

> -> lastnode = list_entry(hwreq->tds.prev, struct
> td_node, td) get last node (which is NULL)
>   -> lastnode->ptr->next = cpu_to_le32(TD_TERMINATE)
> CRASH!!! lastnode->ptr is NULL

if lastnode was initialized with list_entry(), it can't be
NULL. You're missing something

> My idea is that some how the f_midi is not calling free_ep_req(ep,
> req) properly. I am still investigating this.
> 
> One thing to bear in mind is that I used ether gadget and mass_storage
> gadget as well and they both work just fine. Also that the device I am
> running generates a *lot* of MIDI output, much more than the normal
> usage, which might be triggering this bug that no one previously had.
> 
> Any comments?

yeah, continue debugging, what other information can you gather ? Does the
problem go away if you add a ton of printks() around the code ? 

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH] usb: gadget: atmel: remove useless include

[Nicolas Ferre]

Le 10/08/2015 16:29, Alexandre Belloni a écrit :
> Definitions from linux/platform_data/atmel.h are not used, remove the
> include.
> 
> Signed-off-by: Alexandre Belloni 

Acked-by: Nicolas Ferre 

> ---
>  drivers/usb/gadget/udc/atmel_usba_udc.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c 
> b/drivers/usb/gadget/udc/atmel_usba_udc.c
> index 4095cce05e6a..548192f77dcb 100644
> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
> @@ -22,7 +22,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  
> 


-- 
Nicolas Ferre
--
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/9] xhci: give command abortion one more chance before killing xhci

[Mathias Nyman]

We want to give the command abortion an additonal try to stop
the command ring before we completely hose xhci.

Tested-by: Vincent Pelletier 
Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-ring.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index a47a1e8..1c61e5e 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -302,6 +302,15 @@ static int xhci_abort_cmd_ring(struct xhci_hcd *xhci)
ret = xhci_handshake(&xhci->op_regs->cmd_ring,
CMD_RING_RUNNING, 0, 5 * 1000 * 1000);
if (ret < 0) {
+   /* we are about to kill xhci, give it one more chance */
+   xhci_write_64(xhci, temp_64 | CMD_RING_ABORT,
+ &xhci->op_regs->cmd_ring);
+   udelay(1000);
+   ret = xhci_handshake(&xhci->op_regs->cmd_ring,
+CMD_RING_RUNNING, 0, 3 * 1000 * 1000);
+   if (ret == 0)
+   return 0;
+
xhci_err(xhci, "Stopped the command ring failed, "
"maybe the host is dead\n");
xhci->xhc_state |= XHCI_STATE_DYING;
-- 
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 6/9] usb: xhci: stop everything on the first call to xhci_stop

[Mathias Nyman]

From: Roger Quadros 

xhci_stop will be called twice, once for the shared hcd
and again for the primary hcd.

We stop the XHCI controller in any case so clean up
everything on the first call else we can timeout
waiting for pending requests to complete.

Signed-off-by: Roger Quadros 
Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci.c | 20 +---
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 5fe2419..f881d5a 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -655,15 +655,6 @@ int xhci_run(struct usb_hcd *hcd)
 }
 EXPORT_SYMBOL_GPL(xhci_run);
 
-static void xhci_only_stop_hcd(struct usb_hcd *hcd)
-{
-   struct xhci_hcd *xhci = hcd_to_xhci(hcd);
-
-   spin_lock_irq(&xhci->lock);
-   xhci_halt(xhci);
-   spin_unlock_irq(&xhci->lock);
-}
-
 /*
  * Stop xHCI driver.
  *
@@ -678,15 +669,14 @@ void xhci_stop(struct usb_hcd *hcd)
u32 temp;
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
 
-   mutex_lock(&xhci->mutex);
-
-   if (!usb_hcd_is_primary_hcd(hcd)) {
-   xhci_only_stop_hcd(xhci->shared_hcd);
-   mutex_unlock(&xhci->mutex);
+   if (xhci->xhc_state & XHCI_STATE_HALTED)
return;
-   }
 
+   mutex_lock(&xhci->mutex);
spin_lock_irq(&xhci->lock);
+   xhci->xhc_state |= XHCI_STATE_HALTED;
+   xhci->cmd_ring_state = CMD_RING_STATE_STOPPED;
+
/* Make sure the xHC is halted for a USB3 roothub
 * (xhci_stop() could be called as part of failed init).
 */
-- 
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 8/9] xhci: change xhci 1.0 only restictions to support xhci 1.1

[Mathias Nyman]

Some changes between xhci 0.96 and xhci 1.0 specifications forced us to
check the hci version in code, some of these checks were implemented as
hci_version == 1.0, which will not work with new xhci 1.1 controllers.

xhci 1.1 behaves similar to xhci 1.0 in these cases, so change these
checks to hci_version >= 1.0

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-mem.c  | 6 +++---
 drivers/usb/host/xhci-ring.c | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 9a8c936..8497fb8 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1498,10 +1498,10 @@ int xhci_endpoint_init(struct xhci_hcd *xhci,
 * use Event Data TRBs, and we don't chain in a link TRB on short
 * transfers, we're basically dividing by 1.
 *
-* xHCI 1.0 specification indicates that the Average TRB Length should
-* be set to 8 for control endpoints.
+* xHCI 1.0 and 1.1 specification indicates that the Average TRB Length
+* should be set to 8 for control endpoints.
 */
-   if (usb_endpoint_xfer_control(&ep->desc) && xhci->hci_version == 0x100)
+   if (usb_endpoint_xfer_control(&ep->desc) && xhci->hci_version >= 0x100)
ep_ctx->tx_info |= cpu_to_le32(AVG_TRB_LENGTH_FOR_EP(8));
else
ep_ctx->tx_info |=
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 1c61e5e..43291f9 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3470,8 +3470,8 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t 
mem_flags,
if (start_cycle == 0)
field |= 0x1;
 
-   /* xHCI 1.0 6.4.1.2.1: Transfer Type field */
-   if (xhci->hci_version == 0x100) {
+   /* xHCI 1.0/1.1 6.4.1.2.1: Transfer Type field */
+   if (xhci->hci_version >= 0x100) {
if (urb->transfer_buffer_length > 0) {
if (setup->bRequestType & USB_DIR_IN)
field |= TRB_TX_TYPE(TRB_DATA_IN);
-- 
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 3/9] xhci: Move xhci_pme_quirk() behind #ifdef CONFIG_PM

[Mathias Nyman]

From: Tomer Barletz 

xhci_pme_quirk() is only used when CONFIG_PM is defined.
compiling a kernel without PM complains about this function

[reworded commit message -Mathias]
Signed-off-by: Tomer Barletz 
Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-pci.c | 90 ++---
 1 file changed, 45 insertions(+), 45 deletions(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 5590eac..c79d336 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -180,51 +180,6 @@ static void xhci_pci_quirks(struct device *dev, struct 
xhci_hcd *xhci)
"QUIRK: Resetting on resume");
 }
 
-/*
- * In some Intel xHCI controllers, in order to get D3 working,
- * through a vendor specific SSIC CONFIG register at offset 0x883c,
- * SSIC PORT need to be marked as "unused" before putting xHCI
- * into D3. After D3 exit, the SSIC port need to be marked as "used".
- * Without this change, xHCI might not enter D3 state.
- * Make sure PME works on some Intel xHCI controllers by writing 1 to clear
- * the Internal PME flag bit in vendor specific PMCTRL register at offset 
0x80a4
- */
-static void xhci_pme_quirk(struct usb_hcd *hcd, bool suspend)
-{
-   struct xhci_hcd *xhci = hcd_to_xhci(hcd);
-   struct pci_dev  *pdev = to_pci_dev(hcd->self.controller);
-   u32 val;
-   void __iomem *reg;
-
-   if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
-pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI) {
-
-   reg = (void __iomem *) xhci->cap_regs + PORT2_SSIC_CONFIG_REG2;
-
-   /* Notify SSIC that SSIC profile programming is not done */
-   val = readl(reg) & ~PROG_DONE;
-   writel(val, reg);
-
-   /* Mark SSIC port as unused(suspend) or used(resume) */
-   val = readl(reg);
-   if (suspend)
-   val |= SSIC_PORT_UNUSED;
-   else
-   val &= ~SSIC_PORT_UNUSED;
-   writel(val, reg);
-
-   /* Notify SSIC that SSIC profile programming is done */
-   val = readl(reg) | PROG_DONE;
-   writel(val, reg);
-   readl(reg);
-   }
-
-   reg = (void __iomem *) xhci->cap_regs + 0x80a4;
-   val = readl(reg);
-   writel(val | BIT(28), reg);
-   readl(reg);
-}
-
 #ifdef CONFIG_ACPI
 static void xhci_pme_acpi_rtd3_enable(struct pci_dev *dev)
 {
@@ -345,6 +300,51 @@ static void xhci_pci_remove(struct pci_dev *dev)
 }
 
 #ifdef CONFIG_PM
+/*
+ * In some Intel xHCI controllers, in order to get D3 working,
+ * through a vendor specific SSIC CONFIG register at offset 0x883c,
+ * SSIC PORT need to be marked as "unused" before putting xHCI
+ * into D3. After D3 exit, the SSIC port need to be marked as "used".
+ * Without this change, xHCI might not enter D3 state.
+ * Make sure PME works on some Intel xHCI controllers by writing 1 to clear
+ * the Internal PME flag bit in vendor specific PMCTRL register at offset 
0x80a4
+ */
+static void xhci_pme_quirk(struct usb_hcd *hcd, bool suspend)
+{
+   struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+   struct pci_dev  *pdev = to_pci_dev(hcd->self.controller);
+   u32 val;
+   void __iomem *reg;
+
+   if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
+pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI) {
+
+   reg = (void __iomem *) xhci->cap_regs + PORT2_SSIC_CONFIG_REG2;
+
+   /* Notify SSIC that SSIC profile programming is not done */
+   val = readl(reg) & ~PROG_DONE;
+   writel(val, reg);
+
+   /* Mark SSIC port as unused(suspend) or used(resume) */
+   val = readl(reg);
+   if (suspend)
+   val |= SSIC_PORT_UNUSED;
+   else
+   val &= ~SSIC_PORT_UNUSED;
+   writel(val, reg);
+
+   /* Notify SSIC that SSIC profile programming is done */
+   val = readl(reg) | PROG_DONE;
+   writel(val, reg);
+   readl(reg);
+   }
+
+   reg = (void __iomem *) xhci->cap_regs + 0x80a4;
+   val = readl(reg);
+   writel(val | BIT(28), reg);
+   readl(reg);
+}
+
 static int xhci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
 {
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
-- 
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 7/9] usb: xhci: exit early in xhci_setup_device() if we're halted or dying

[Mathias Nyman]

From: Roger Quadros 

During quick plug/removal of OTG adapter during dual-role testing
it can happen that xhci_alloc_device() is called for the newly
detected device after the DRD library has called xhci_stop to
remove the HCD.

If that is the case, just fail early to prevent the following warning.

[  154.732649] hub 4-0:1.0: USB hub found
[  154.742204] hub 4-0:1.0: 1 port detected
[  154.824458] hub 3-0:1.0: state 7 ports 1 chg 0002 evt 
[  154.854609] hub 4-0:1.0: state 7 ports 1 chg  evt 
[  154.944430] usb 3-1: new high-speed USB device number 2 using xhci-hcd
[  154.951009] xhci-hcd xhci-hcd.0.auto: xhci_setup_device
[  155.038191] xhci-hcd xhci-hcd.0.auto: remove, state 4
[  155.043315] usb usb4: USB disconnect, device number 1
[  155.055270] xhci-hcd xhci-hcd.0.auto: xhci_stop
[  155.060094] xhci-hcd xhci-hcd.0.auto: USB bus 4 deregistered
[  155.066576] xhci-hcd xhci-hcd.0.auto: remove, state 1
[  155.071710] usb usb3: USB disconnect, device number 1
[  155.077124] xhci-hcd xhci-hcd.0.auto: xhci_setup_device
[  155.082389] [ cut here ]
[  155.087690] WARNING: CPU: 0 PID: 72 at drivers/usb/host/xhci.c:3800 
xhci_setup_device+0x410/0x484 [xhci_hcd]()
[  155.097861] Modules linked in: sd_mod usb_storage scsi_mod usb_f_ss_lb 
g_zero libcomposite ipv6 xhci_plat_hcd xhci_hcd usbcore dwc3 udc_core evdev 
ti_am335x_adc joydev kfifo_buf industrialio snd_soc_simple_cc
[  155.146734] CPU: 0 PID: 72 Comm: kworker/0:3 Tainted: GW   
4.1.4-00834-gcd9380b-dirty #50
[  155.156073] Hardware name: Generic AM43 (Flattened Device Tree)
[  155.162117] Workqueue: usb_hub_wq hub_event [usbcore]
[  155.167249] Backtrace:
[  155.169751] [] (dump_backtrace) from [] 
(show_stack+0x18/0x1c)
[  155.177390]  r6:c089d4a4 r5: r4: r3:ee46c000
[  155.183137] [] (show_stack) from [] 
(dump_stack+0x84/0xd0)
[  155.190446] [] (dump_stack) from [] 
(warn_slowpath_common+0x80/0xbc)
[  155.198605]  r7:0009 r6:0ed8 r5:bf27eb70 r4:
[  155.204348] [] (warn_slowpath_common) from [] 
(warn_slowpath_null+0x24/0x2c)
[  155.213202]  r8:ee49f000 r7:ee7c0004 r6: r5:ee7c0158 r4:ee7c
[  155.220051] [] (warn_slowpath_null) from [] 
(xhci_setup_device+0x410/0x484 [xhci_hcd])
[  155.229816] [] (xhci_setup_device [xhci_hcd]) from [] 
(xhci_address_device+0x14/0x18 [xhci_hcd])
[  155.240415]  r10:ee598200 r9:0001 r8:0002 r7:0001 r6:0003 
r5:0002
[  155.248363]  r4:ee49f000
[  155.250978] [] (xhci_address_device [xhci_hcd]) from [] 
(hub_port_init+0x1b8/0xa9c [usbcore])
[  155.261403] [] (hub_port_init [usbcore]) from [] 
(hub_event+0x738/0x1020 [usbcore])
[  155.270874]  r10:ee598200 r9:ee7c r8:ee7c0038 r7:ee518800 r6:ee49f000 
r5:0001
[  155.278822]  r4:
[  155.281426] [] (hub_event [usbcore]) from [] 
(process_one_work+0x128/0x340)
[  155.290196]  r10: r9:0003 r8: r7:fedfa000 r6:eeec5400 
r5:ee598314
[  155.298151]  r4:ee434380
[  155.300718] [] (process_one_work) from [] 
(worker_thread+0x158/0x49c)
[  155.308963]  r10:ee434380 r9:0003 r8:eeec5400 r7:0008 r6:ee434398 
r5:eeec5400
[  155.316913]  r4:eeec5414
[  155.319482] [] (worker_thread) from [] 
(kthread+0xdc/0xf8)
[  155.326765]  r10: r9: r8: r7:c00577a0 r6:ee434380 
r5:ee4441c0
[  155.334713]  r4: r3:
[  155.338341] [] (kthread) from [] 
(ret_from_fork+0x14/0x2c)
[  155.345626]  r7: r6: r5:c005cb64 r4:ee4441c0
[  155.356108] ---[ end trace a58d34c223b190e6 ]---
[  155.360783] xhci-hcd xhci-hcd.0.auto: Virt dev invalid for slot_id 0x1!
[  155.574404] xhci-hcd xhci-hcd.0.auto: xhci_setup_device
[  155.579667] [ cut here ]

Signed-off-by: Roger Quadros 
Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index f881d5a..9957bd9 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3788,6 +3788,9 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct 
usb_device *udev,
 
mutex_lock(&xhci->mutex);
 
+   if (xhci->xhc_state)/* dying or halted */
+   goto out;
+
if (!udev->slot_id) {
xhci_dbg_trace(xhci, trace_xhci_dbg_address,
"Bad Slot ID %d", udev->slot_id);
-- 
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 5/9] usb: xhci: Clear XHCI_STATE_DYING on start

[Mathias Nyman]

From: Roger Quadros 

For whatever reason if XHCI died in the previous instant
then it will never recover on the next xhci_start unless we
clear the DYING flag.

Signed-off-by: Roger Quadros 
Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index f560c41..5fe2419 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -146,7 +146,8 @@ static int xhci_start(struct xhci_hcd *xhci)
"waited %u microseconds.\n",
XHCI_MAX_HALT_USEC);
if (!ret)
-   xhci->xhc_state &= ~XHCI_STATE_HALTED;
+   xhci->xhc_state &= ~(XHCI_STATE_HALTED | XHCI_STATE_DYING);
+
return ret;
 }
 
-- 
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 0/9] xhci fixes for usb-linus, including a small usb core fix

[Mathias Nyman]

Hi Greg

This series contain one usb core fix that makes sure we don't
use bmAttribute bits that were reseved 0 in usb 3 specification.
These bits are taken into use in USB 3.1, and we want to make sure
usb 3.1 devices don't mess up calculations.

The xhci fixes are mostly minor ones, except for the series by Roger Quadros 
which fixes issues discovered while quickly unbinding and rebinding xhci
during dual role testing.

-Mathias


Mathias Nyman (4):
  usb: Use the USB_SS_MULT() macro to get the burst multiplier.
  xhci: give command abortion one more chance before killing xhci
  xhci: change xhci 1.0 only restictions to support xhci 1.1
  xhci: init command timeout timer earlier to avoid deleting it before
init

Roger Quadros (4):
  usb: xhci: lock mutex on xhci_stop
  usb: xhci: Clear XHCI_STATE_DYING on start
  usb: xhci: stop everything on the first call to xhci_stop
  usb: xhci: exit early in xhci_setup_device() if we're halted or dying

Tomer Barletz (1):
  xhci: Move xhci_pme_quirk() behind #ifdef CONFIG_PM

 drivers/usb/core/config.c|  5 ++-
 drivers/usb/host/xhci-mem.c  | 17 -
 drivers/usb/host/xhci-pci.c  | 90 ++--
 drivers/usb/host/xhci-ring.c | 13 ++-
 drivers/usb/host/xhci.c  | 24 ++--
 5 files changed, 78 insertions(+), 71 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

[PATCH 4/9] usb: xhci: lock mutex on xhci_stop

[Mathias Nyman]

From: Roger Quadros 

Else it races with xhci_setup_device

Signed-off-by: Roger Quadros 
Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 6b0f4a4..f560c41 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -677,8 +677,11 @@ void xhci_stop(struct usb_hcd *hcd)
u32 temp;
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
 
+   mutex_lock(&xhci->mutex);
+
if (!usb_hcd_is_primary_hcd(hcd)) {
xhci_only_stop_hcd(xhci->shared_hcd);
+   mutex_unlock(&xhci->mutex);
return;
}
 
@@ -717,6 +720,7 @@ void xhci_stop(struct usb_hcd *hcd)
xhci_dbg_trace(xhci, trace_xhci_dbg_init,
"xhci_stop completed - status = %x",
readl(&xhci->op_regs->status));
+   mutex_unlock(&xhci->mutex);
 }
 
 /*
-- 
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 1/9] usb: Use the USB_SS_MULT() macro to get the burst multiplier.

[Mathias Nyman]

Bits 1:0 of the bmAttributes are used for the burst multiplier.
The rest of the bits used to be reserverd (zero), but USB3.1 takes bit 7
into use.

Use the existing USB_SS_MULT() macro instead to make sure the mult value
and hence max packet calculations are correct for USB3.1 devices.

Note that burts multipier in bmAttributes is zero based and that
the USB_SS_MULT() macro adds one.

Signed-off-by: Mathias Nyman 
---
 drivers/usb/core/config.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
index b2a540b..b9ddf0c 100644
--- a/drivers/usb/core/config.c
+++ b/drivers/usb/core/config.c
@@ -112,7 +112,7 @@ static void usb_parse_ss_endpoint_companion(struct device 
*ddev, int cfgno,
cfgno, inum, asnum, ep->desc.bEndpointAddress);
ep->ss_ep_comp.bmAttributes = 16;
} else if (usb_endpoint_xfer_isoc(&ep->desc) &&
-   desc->bmAttributes > 2) {
+  USB_SS_MULT(desc->bmAttributes) > 3) {
dev_warn(ddev, "Isoc endpoint has Mult of %d in "
"config %d interface %d altsetting %d ep %d: "
"setting to 3\n", desc->bmAttributes + 1,
@@ -121,7 +121,8 @@ static void usb_parse_ss_endpoint_companion(struct device 
*ddev, int cfgno,
}
 
if (usb_endpoint_xfer_isoc(&ep->desc))
-   max_tx = (desc->bMaxBurst + 1) * (desc->bmAttributes + 1) *
+   max_tx = (desc->bMaxBurst + 1) *
+   (USB_SS_MULT(desc->bmAttributes)) *
usb_endpoint_maxp(&ep->desc);
else if (usb_endpoint_xfer_int(&ep->desc))
max_tx = usb_endpoint_maxp(&ep->desc) *
-- 
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 9/9] xhci: init command timeout timer earlier to avoid deleting it before init

[Mathias Nyman]

Don't  check if timer is running with a timer_pending() before
deleting it with del_timer_sync(), this defies the whole point of
the sync part and can cause a possible race.

Instead we just want to make sure the timer is initialized early enough
before we have a chance to delete it.

Reported-by: Oliver Neukum 
Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-mem.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 8497fb8..41f841f 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1792,8 +1792,7 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
int size;
int i, j, num_ports;
 
-   if (timer_pending(&xhci->cmd_timer))
-   del_timer_sync(&xhci->cmd_timer);
+   del_timer_sync(&xhci->cmd_timer);
 
/* Free the Event Ring Segment Table and the actual Event Ring */
size = sizeof(struct xhci_erst_entry)*(xhci->erst.num_entries);
@@ -2321,6 +2320,10 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
 
INIT_LIST_HEAD(&xhci->cmd_list);
 
+   /* init command timeout timer */
+   setup_timer(&xhci->cmd_timer, xhci_handle_command_timeout,
+   (unsigned long)xhci);
+
page_size = readl(&xhci->op_regs->page_size);
xhci_dbg_trace(xhci, trace_xhci_dbg_init,
"Supported page size register = 0x%x", page_size);
@@ -2505,10 +2508,6 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
"Wrote ERST address to ir_set 0.");
xhci_print_ir_set(xhci, 0);
 
-   /* init command timeout timer */
-   setup_timer(&xhci->cmd_timer, xhci_handle_command_timeout,
-   (unsigned long)xhci);
-
/*
 * XXX: Might need to set the Interrupter Moderation Register to
 * something other than the default (~1ms minimum between interrupts).
-- 
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: gadget: at91_udc: move at91_udc_data in at91_udc.h

[Nicolas Ferre]

Le 10/08/2015 16:46, Alexandre Belloni a écrit :
> struct at91_udc_data is now only used inside the driver, move it to its
> include.
> 
> Signed-off-by: Alexandre Belloni 

Acked-by: Nicolas Ferre 

Thanks.

> ---
>  drivers/usb/gadget/udc/at91_udc.h   | 8 
>  include/linux/platform_data/atmel.h | 9 -
>  2 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/at91_udc.h 
> b/drivers/usb/gadget/udc/at91_udc.h
> index 2679c8b217cc..0a433e6b346b 100644
> --- a/drivers/usb/gadget/udc/at91_udc.h
> +++ b/drivers/usb/gadget/udc/at91_udc.h
> @@ -112,6 +112,14 @@ struct at91_udc_caps {
>   void (*pullup)(struct at91_udc *udc, int is_on);
>  };
>  
> +struct at91_udc_data {
> + int vbus_pin;   /* high == host powering us */
> + u8  vbus_active_low;/* vbus polarity */
> + u8  vbus_polled;/* Use polling, not interrupt */
> + int pullup_pin; /* active == D+ pulled up */
> + u8  pullup_active_low;  /* true == pullup_pin is active low */
> +};
> +
>  /*
>   * driver is non-SMP, and just blocks IRQs whenever it needs
>   * access protection for chip registers or driver state
> diff --git a/include/linux/platform_data/atmel.h 
> b/include/linux/platform_data/atmel.h
> index 4b452c6a2f7b..2d67e466c51b 100644
> --- a/include/linux/platform_data/atmel.h
> +++ b/include/linux/platform_data/atmel.h
> @@ -25,15 +25,6 @@
>   */
>  #define ATMEL_MAX_UART   7
>  
> - /* USB Device */
> -struct at91_udc_data {
> - int vbus_pin;   /* high == host powering us */
> - u8  vbus_active_low;/* vbus polarity */
> - u8  vbus_polled;/* Use polling, not interrupt */
> - int pullup_pin; /* active == D+ pulled up */
> - u8  pullup_active_low;  /* true == pullup_pin is active low */
> -};
> -
>   /* Compact Flash */
>  struct at91_cf_data {
>   int irq_pin;/* I/O IRQ */
> 


-- 
Nicolas Ferre
--
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] USB: EHCI: fix dereference of ERR_PTR

[Alan Stern]

On Fri, 18 Sep 2015, Sudip Mukherjee wrote:

> On Wed, Sep 16, 2015 at 12:54:03PM -0400, Alan Stern wrote:
> > On Wed, 16 Sep 2015, Sudip Mukherjee wrote:
> > 
> > > On error find_tt() returns either a NULL pointer or the error value in
> > > ERR_PTR. But we were dereferencing it directly without even checking if
> > > find_tt() returned a valid pointer or not.
> > > 
> > > Signed-off-by: Sudip Mukherjee 
> > > ---
> 
> > > @@ -1373,6 +1375,8 @@ static void reserve_release_iso_bandwidth(struct 
> > > ehci_hcd *ehci,
> > >   }
> > >  
> > >   tt = find_tt(stream->ps.udev);
> > > + if (IS_ERR_OR_NULL(tt))
> > > + return;
> > >   if (sign > 0)
> > >   list_add_tail(&stream->ps.ps_list, &tt->ps_list);
> > >   else
> > 
> > This patch isn't needed.  In both reserve_release_intr_bandwidth() and 
> > reserve_release_iso_bandwidth() it is known that find_tt() will return 
> > a valid pointer.
> > 
> > This is because each of those functions is called from only one place.  
> > For example, reserve_release_intr_bandwidth() is called only at the end
> > of qh_schedule().  But near the start of qh_schedule() there is earlier
> > call to tt_find(), and there we do test for error pointers.  If the
> > first call doesn't return an error then the second call won't either.
> > 
> > The same sort of thing happens in reserve_release_iso_bandwidth().
> Yes, I should have looked more before sending. Sorry for the noise.
> But in those checkes for find_tt() only IS_ERR is checked, shouldn't we
> check for IS_ERR_OR_NULL as find_tt() can return NULL also?

I knew someone would ask about that!  :-)

The NULL case is similar to the ERR_PTR case, but more complicated.  
Basically, find_tt() returns NULL only when the device doesn't lie 
below a TT -- all the other invalid returns are ERR_PTRs.

In reserve_release_intr_bandwidth(), for instance, the call to 
find_tt() occurs only if tt_usecs = qh->ps.tt_usecs is nonzero.  This 
value is guaranteed to be 0 if the device doesn't run at low speed or 
full speed -- see qh_make() in ehci-q.c, where qh->ps.tt_usecs is 
initialized only when urb->dev->speed != USB_SPEED_HIGH.

To see that udev->tt is non-NULL whenever the speed isn't 
USB_SPEED_HIGH, you have to look through the hub driver code.  The 
relevant routine is hub_port_init() in core/hub.c, the section headed 
by the comment:

/* Set up TT records, if needed  */

Similar reasoning applies to reserve_release_iso_bandwidth(); here the 
condition is that stream->splits is nonzero, which is true only if the 
device is full speed (see iso_stream_init()).

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

[PATCH] usb: gadget: atmel_usba_udc: add ep capabilities support on device tree binding

[Nicolas Ferre]

From: Sylvain Rochet 

The recently added endpoint capabilities flags verification breaks Atmel
USBA because the endpoint configuration was only added when the driver
is bound using the legacy pdata interface.

Convert endpoint configuration to new capabilities model when driver is
bound to a device tree as well.

Signed-off-by: Sylvain Rochet 
Fixes: 47bef3865115 ("usb: gadget: atmel_usba_udc: add ep capabilities support")
Signed-off-by: Nicolas Ferre 
---
Felipe,

As you've just requested, here is the same patch sent to linux-usb ml. I had
added the "Fixes" and my SoB tags.

For the record:
It is considered as a fix for 4.3. Can you please queue it for the "4.3-rc"
phase?

Bye,


 drivers/usb/gadget/udc/atmel_usba_udc.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c 
b/drivers/usb/gadget/udc/atmel_usba_udc.c
index 3dfada8d6061..f0f2b066ac08 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.c
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
@@ -2002,6 +2002,17 @@ static struct usba_ep * atmel_udc_of_init(struct 
platform_device *pdev,
ep->udc = udc;
INIT_LIST_HEAD(&ep->queue);
 
+   if (ep->index == 0) {
+   ep->ep.caps.type_control = true;
+   } else {
+   ep->ep.caps.type_iso = ep->can_isoc;
+   ep->ep.caps.type_bulk = true;
+   ep->ep.caps.type_int = true;
+   }
+
+   ep->ep.caps.dir_in = true;
+   ep->ep.caps.dir_out = true;
+
if (i)
list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list);
 
-- 
2.1.3

--
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: atmel_usba_udc: add ep capabilities support on device tree binding

[Felipe Balbi]

On Fri, Sep 18, 2015 at 04:58:28PM +0200, Nicolas Ferre wrote:
> From: Sylvain Rochet 
> 
> The recently added endpoint capabilities flags verification breaks Atmel
> USBA because the endpoint configuration was only added when the driver
> is bound using the legacy pdata interface.
> 
> Convert endpoint configuration to new capabilities model when driver is
> bound to a device tree as well.
> 
> Signed-off-by: Sylvain Rochet 
> Fixes: 47bef3865115 ("usb: gadget: atmel_usba_udc: add ep capabilities 
> support")
> Signed-off-by: Nicolas Ferre 
> ---
> Felipe,
> 
> As you've just requested, here is the same patch sent to linux-usb ml. I had
> added the "Fixes" and my SoB tags.
> 
> For the record:
> It is considered as a fix for 4.3. Can you please queue it for the "4.3-rc"
> phase?

thanks, I'll take it for -rc3 (already sent my -rc2 pull request).

-- 
balbi


signature.asc
Description: Digital signature

[RFT PATCH] xhci: don't finish a TD if we get a short transfer event mid TD

[Mathias Nyman]

If the difference is big enough between the bytes asked and received
in a bulk tranfer we can get a short transfer event pointing to a TRB in
the middle of the TD. We don't want to handle the TD yet as we will anyway
receive a new event for the last TRB in the TD.

Hold off removing TD from list and finishing it before we reveive a event
for the last TRB in the TD

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-ring.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index a47a1e8..4d3cb61 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2182,6 +2182,10 @@ static int process_bulk_intr_td(struct xhci_hcd *xhci, 
struct xhci_td *td,
}
/* Fast path - was this the last TRB in the TD for this URB? */
} else if (event_trb == td->last_trb) {
+   if (td->urb_length_set && trb_comp_code == COMP_SHORT_TX)
+   return finish_td(xhci, td, event_trb, event, ep,
+status, false);
+
if (EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)) != 0) {
td->urb->actual_length =
td->urb->transfer_buffer_length -
@@ -2233,6 +2237,12 @@ static int process_bulk_intr_td(struct xhci_hcd *xhci, 
struct xhci_td *td,
td->urb->actual_length +=
TRB_LEN(le32_to_cpu(cur_trb->generic.field[2])) 
-
EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
+
+   if (trb_comp_code == COMP_SHORT_TX) {
+   xhci_dbg(xhci, "mid bulk/intr SP, wait for last TRB 
event\n");
+   td->urb_length_set = true;
+   return 0;
+   }
}
 
return finish_td(xhci, td, event_trb, event, ep, status, false);
-- 
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: MIDI gadget allocating too much from coherent pool

[Felipe Tonello]

Hi Balbi,

On Fri, Sep 18, 2015 at 3:25 PM, Felipe Balbi  wrote:
> On Fri, Sep 18, 2015 at 11:03:26AM +0100, Felipe Tonello wrote:
>> After some debugging, here is where I am:
>>
>> The crash trace is like this:
>>
>> (f_midi.c)
>> -> ALSA calls f_midi_in_trigger()
>>   ->tasklet_hi_schedule(&midi->tasklet)
>> -> f_midi_transmit(midi, NULL) the NULL here causes the f_midi to
>> request a usb_request allocation, also it sets req->complete (which is
>> not been called? I am investigating this)
>
> ->complete() is called by the UDC when the request actually completes.

The problem is that the udc driver is never calling it. I just realised that.

(chipidea/udc.c) irqreturn_t udc_irq(struct ci_hdrc *ci) is not been
called after a f_midi_transmit(). Think here is the main problem. Any
ideas?

>
>>   -> usb_ep_queue(ep, req, GFP_ATOMIC)
>> (chipidea/udc.c)
>> -> ep_queue(ep, req, gfp_flags)
>>   -> _ep_queue(ep, req, gfp_flags)
>> -> _hardware_enqueue(hwep, hwreq) here is where it is crashing
>>   -> add_td_to_list(hwep, hwreq, count) this guy returns
>> an error from dma_pool_alloc(), out of mem, which is not been checked
>> (I have a patch for this)
>
> oh, this probably part of the error. Care to send it ?

Yes. It doesn't fix anything, but it prevents the kernel to panic.

>
>> -> lastnode = list_entry(hwreq->tds.prev, struct
>> td_node, td) get last node (which is NULL)
>>   -> lastnode->ptr->next = cpu_to_le32(TD_TERMINATE)
>> CRASH!!! lastnode->ptr is NULL
>
> if lastnode was initialized with list_entry(), it can't be
> NULL. You're missing something

lastnode is not NULL, lastnode->ptr is NULL. (because of the
dma_pool_alloc, check add_td_to_list function).

>
>> My idea is that some how the f_midi is not calling free_ep_req(ep,
>> req) properly. I am still investigating this.
>>
>> One thing to bear in mind is that I used ether gadget and mass_storage
>> gadget as well and they both work just fine. Also that the device I am
>> running generates a *lot* of MIDI output, much more than the normal
>> usage, which might be triggering this bug that no one previously had.
>>
>> Any comments?
>
> yeah, continue debugging, what other information can you gather ? Does the
> problem go away if you add a ton of printks() around the code ?

It doesn't change anything.
--
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: MIDI gadget allocating too much from coherent pool

[Felipe Balbi]

Hi,

On Fri, Sep 18, 2015 at 04:53:09PM +0100, Felipe Tonello wrote:
> Hi Balbi,
> 
> On Fri, Sep 18, 2015 at 3:25 PM, Felipe Balbi  wrote:
> > On Fri, Sep 18, 2015 at 11:03:26AM +0100, Felipe Tonello wrote:
> >> After some debugging, here is where I am:
> >>
> >> The crash trace is like this:
> >>
> >> (f_midi.c)
> >> -> ALSA calls f_midi_in_trigger()
> >>   ->tasklet_hi_schedule(&midi->tasklet)
> >> -> f_midi_transmit(midi, NULL) the NULL here causes the f_midi to
> >> request a usb_request allocation, also it sets req->complete (which is
> >> not been called? I am investigating this)
> >
> > ->complete() is called by the UDC when the request actually completes.
> 
> The problem is that the udc driver is never calling it. I just realised that.
> 
> (chipidea/udc.c) irqreturn_t udc_irq(struct ci_hdrc *ci) is not been
> called after a f_midi_transmit(). Think here is the main problem. Any
> ideas?

have you looked at ci_irq() ? Look at how many return points it has. It's
not inconceivable that it might be returning before calling ->udc_irq().

Now, if that's valid or not, it's for you decide. I don't even have this
HW and can't help much more than this.

> >>   -> usb_ep_queue(ep, req, GFP_ATOMIC)
> >> (chipidea/udc.c)
> >> -> ep_queue(ep, req, gfp_flags)
> >>   -> _ep_queue(ep, req, gfp_flags)
> >> -> _hardware_enqueue(hwep, hwreq) here is where it is crashing
> >>   -> add_td_to_list(hwep, hwreq, count) this guy returns
> >> an error from dma_pool_alloc(), out of mem, which is not been checked
> >> (I have a patch for this)
> >
> > oh, this probably part of the error. Care to send it ?
> 
> Yes. It doesn't fix anything, but it prevents the kernel to panic.

fair enough, and where's the patch ?

> >> -> lastnode = list_entry(hwreq->tds.prev, struct
> >> td_node, td) get last node (which is NULL)
> >>   -> lastnode->ptr->next = cpu_to_le32(TD_TERMINATE)
> >> CRASH!!! lastnode->ptr is NULL
> >
> > if lastnode was initialized with list_entry(), it can't be
> > NULL. You're missing something
> 
> lastnode is not NULL, lastnode->ptr is NULL. (because of the
> dma_pool_alloc, check add_td_to_list function).

right, so you're running out of memory. Why are you running out of memory ?
probably because ->udc_irq() is never called and, thus, you never free from
coherent. Find reason for that and your problems will be gone, probably.

> >> Any comments?
> >
> > yeah, continue debugging, what other information can you gather ? Does the
> > problem go away if you add a ton of printks() around the code ?
> 
> It doesn't change anything.

ok, then it should be easy to reproduce. Wonder if this UDC has ever been
tested with isochronous transfers at all.

-- 
balbi


signature.asc
Description: Digital signature

[PATCH] usb: chipidea: udc: improve error handling on ep_queue and f_midi

[eu]

From: "Felipe F. Tonello" 

_ep_queue() didn't check for errors when using add_td_to_list()
which can fail if dma_pool_alloc fails, thus causing a kernel
panic when lastnode->ptr is NULL.

Also f_midi is not checking weather the is an error on usb_ep_queue
request, ignoring potential problems, such as memory leaks.

Signed-off-by: Felipe F. Tonello 
---
 drivers/usb/chipidea/udc.c   | 26 +++---
 drivers/usb/gadget/function/f_midi.c | 11 +--
 2 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index 764f668..7169113e 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -404,9 +404,9 @@ static inline u8 _usb_addr(struct ci_hw_ep *ep)
 }
 
 /**
- * _hardware_queue: configures a request at hardware level
- * @gadget: gadget
+ * _hardware_enqueue: configures a request at hardware level
  * @hwep:   endpoint
+ * @hwreq:  request
  *
  * This function returns an error code
  */
@@ -435,19 +435,27 @@ static int _hardware_enqueue(struct ci_hw_ep *hwep, 
struct ci_hw_req *hwreq)
if (hwreq->req.dma % PAGE_SIZE)
pages--;
 
-   if (rest == 0)
-   add_td_to_list(hwep, hwreq, 0);
+   if (rest == 0) {
+   ret = add_td_to_list(hwep, hwreq, 0);
+   if (ret < 0)
+   goto done;
+   }
 
while (rest > 0) {
unsigned count = min(hwreq->req.length - hwreq->req.actual,
(unsigned)(pages * CI_HDRC_PAGE_SIZE));
-   add_td_to_list(hwep, hwreq, count);
+   ret = add_td_to_list(hwep, hwreq, count);
+   if (ret < 0)
+   goto done;
rest -= count;
}
 
if (hwreq->req.zero && hwreq->req.length
-   && (hwreq->req.length % hwep->ep.maxpacket == 0))
-   add_td_to_list(hwep, hwreq, 0);
+   && (hwreq->req.length % hwep->ep.maxpacket == 0)) {
+   ret = add_td_to_list(hwep, hwreq, 0);
+   if (ret < 0)
+   goto done;
+   }
 
firstnode = list_first_entry(&hwreq->tds, struct td_node, td);
 
@@ -750,8 +758,12 @@ static void isr_get_status_complete(struct usb_ep *ep, 
struct usb_request *req)
 
 /**
  * _ep_queue: queues (submits) an I/O request to an endpoint
+ * @ep:endpoint
+ * @req:   request
+ * @gfp_flags: GFP flags (not used)
  *
  * Caller must hold lock
+ * This function returns an error code
  */
 static int _ep_queue(struct usb_ep *ep, struct usb_request *req,
gfp_t __maybe_unused gfp_flags)
diff --git a/drivers/usb/gadget/function/f_midi.c 
b/drivers/usb/gadget/function/f_midi.c
index ad50a67..bb580e8 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -543,8 +543,15 @@ static void f_midi_transmit(struct f_midi *midi, struct 
usb_request *req)
}
}
 
-   if (req->length > 0)
-   usb_ep_queue(ep, req, GFP_ATOMIC);
+   if (req->length > 0) {
+   int err;
+
+   err = usb_ep_queue(ep, req, GFP_ATOMIC);
+   if (err < 0) {
+   ERROR(midi, "%s queue req: %d\n",
+ midi->out_ep->name, err);
+   }
+   }
else
free_ep_req(ep, req);
 }
-- 
2.1.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: MIDI gadget allocating too much from coherent pool

[Felipe Tonello]

Balbi,

On Fri, Sep 18, 2015 at 5:02 PM, Felipe Balbi  wrote:
> Hi,
>
> On Fri, Sep 18, 2015 at 04:53:09PM +0100, Felipe Tonello wrote:
>> Hi Balbi,
>>
>> On Fri, Sep 18, 2015 at 3:25 PM, Felipe Balbi  wrote:
>> > On Fri, Sep 18, 2015 at 11:03:26AM +0100, Felipe Tonello wrote:
>> >> After some debugging, here is where I am:
>> >>
>> >> The crash trace is like this:
>> >>
>> >> (f_midi.c)
>> >> -> ALSA calls f_midi_in_trigger()
>> >>   ->tasklet_hi_schedule(&midi->tasklet)
>> >> -> f_midi_transmit(midi, NULL) the NULL here causes the f_midi to
>> >> request a usb_request allocation, also it sets req->complete (which is
>> >> not been called? I am investigating this)
>> >
>> > ->complete() is called by the UDC when the request actually completes.
>>
>> The problem is that the udc driver is never calling it. I just realised that.
>>
>> (chipidea/udc.c) irqreturn_t udc_irq(struct ci_hdrc *ci) is not been
>> called after a f_midi_transmit(). Think here is the main problem. Any
>> ideas?
>
> have you looked at ci_irq() ? Look at how many return points it has. It's
> not inconceivable that it might be returning before calling ->udc_irq().

Looking into that. Thanks for the hint.

>
> Now, if that's valid or not, it's for you decide. I don't even have this
> HW and can't help much more than this.
>
>> >>   -> usb_ep_queue(ep, req, GFP_ATOMIC)
>> >> (chipidea/udc.c)
>> >> -> ep_queue(ep, req, gfp_flags)
>> >>   -> _ep_queue(ep, req, gfp_flags)
>> >> -> _hardware_enqueue(hwep, hwreq) here is where it is crashing
>> >>   -> add_td_to_list(hwep, hwreq, count) this guy returns
>> >> an error from dma_pool_alloc(), out of mem, which is not been checked
>> >> (I have a patch for this)
>> >
>> > oh, this probably part of the error. Care to send it ?
>>
>> Yes. It doesn't fix anything, but it prevents the kernel to panic.
>
> fair enough, and where's the patch ?

Sent.

>
>> >> -> lastnode = list_entry(hwreq->tds.prev, struct
>> >> td_node, td) get last node (which is NULL)
>> >>   -> lastnode->ptr->next = cpu_to_le32(TD_TERMINATE)
>> >> CRASH!!! lastnode->ptr is NULL
>> >
>> > if lastnode was initialized with list_entry(), it can't be
>> > NULL. You're missing something
>>
>> lastnode is not NULL, lastnode->ptr is NULL. (because of the
>> dma_pool_alloc, check add_td_to_list function).
>
> right, so you're running out of memory. Why are you running out of memory ?
> probably because ->udc_irq() is never called and, thus, you never free from
> coherent. Find reason for that and your problems will be gone, probably.

Bingo! That is what it looks like.

The weird thing, like I said, is that I use different gadgets and they
don't have this leak problem, apparently. I didn't test but it never
panic'ed.

>
>> >> Any comments?
>> >
>> > yeah, continue debugging, what other information can you gather ? Does the
>> > problem go away if you add a ton of printks() around the code ?
>>
>> It doesn't change anything.
>
> ok, then it should be easy to reproduce. Wonder if this UDC has ever been
> tested with isochronous transfers at all.

Perhaps Peter could say better?

Thanks Balbi.

Felipe Tonello
--
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/9] usb: Use the USB_SS_MULT() macro to get the burst multiplier.

[Greg KH]

On Fri, Sep 18, 2015 at 05:42:05PM +0300, Mathias Nyman wrote:
> Bits 1:0 of the bmAttributes are used for the burst multiplier.
> The rest of the bits used to be reserverd (zero), but USB3.1 takes bit 7
> into use.
> 
> Use the existing USB_SS_MULT() macro instead to make sure the mult value
> and hence max packet calculations are correct for USB3.1 devices.
> 
> Note that burts multipier in bmAttributes is zero based and that
> the USB_SS_MULT() macro adds one.
> 
> Signed-off-by: Mathias Nyman 
> ---
>  drivers/usb/core/config.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
> index b2a540b..b9ddf0c 100644
> --- a/drivers/usb/core/config.c
> +++ b/drivers/usb/core/config.c
> @@ -112,7 +112,7 @@ static void usb_parse_ss_endpoint_companion(struct device 
> *ddev, int cfgno,
>   cfgno, inum, asnum, ep->desc.bEndpointAddress);
>   ep->ss_ep_comp.bmAttributes = 16;
>   } else if (usb_endpoint_xfer_isoc(&ep->desc) &&
> - desc->bmAttributes > 2) {
> +USB_SS_MULT(desc->bmAttributes) > 3) {
>   dev_warn(ddev, "Isoc endpoint has Mult of %d in "
>   "config %d interface %d altsetting %d ep %d: "
>   "setting to 3\n", desc->bmAttributes + 1,
> @@ -121,7 +121,8 @@ static void usb_parse_ss_endpoint_companion(struct device 
> *ddev, int cfgno,
>   }
>  
>   if (usb_endpoint_xfer_isoc(&ep->desc))
> - max_tx = (desc->bMaxBurst + 1) * (desc->bmAttributes + 1) *
> + max_tx = (desc->bMaxBurst + 1) *
> + (USB_SS_MULT(desc->bmAttributes)) *
>   usb_endpoint_maxp(&ep->desc);
>   else if (usb_endpoint_xfer_int(&ep->desc))
>   max_tx = usb_endpoint_maxp(&ep->desc) *

Can this wait for 4.4?  This isn't a bugfix for 4.3, so I don't see why
it's really needed right now.

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 8/9] xhci: change xhci 1.0 only restictions to support xhci 1.1

[Greg KH]

On Fri, Sep 18, 2015 at 05:42:12PM +0300, Mathias Nyman wrote:
> Some changes between xhci 0.96 and xhci 1.0 specifications forced us to
> check the hci version in code, some of these checks were implemented as
> hci_version == 1.0, which will not work with new xhci 1.1 controllers.
> 
> xhci 1.1 behaves similar to xhci 1.0 in these cases, so change these
> checks to hci_version >= 1.0
> 
> Signed-off-by: Mathias Nyman 
> ---
>  drivers/usb/host/xhci-mem.c  | 6 +++---
>  drivers/usb/host/xhci-ring.c | 4 ++--
>  2 files changed, 5 insertions(+), 5 deletions(-)

Should this also go to stable kernels so that they will work with 1.1
controllers?

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 0/9] xhci fixes for usb-linus, including a small usb core fix

[Greg KH]

On Fri, Sep 18, 2015 at 05:42:04PM +0300, Mathias Nyman wrote:
> Hi Greg
> 
> This series contain one usb core fix that makes sure we don't
> use bmAttribute bits that were reseved 0 in usb 3 specification.
> These bits are taken into use in USB 3.1, and we want to make sure
> usb 3.1 devices don't mess up calculations.
> 
> The xhci fixes are mostly minor ones, except for the series by Roger Quadros 
> which fixes issues discovered while quickly unbinding and rebinding xhci
> during dual role testing.

And none of these are for stable kernels?  why not?

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: [GIT PULL] USB chipidea fixes for v4.3-rc2

[Greg KH]

On Fri, Sep 18, 2015 at 03:54:31PM +0800, Peter Chen wrote:
> The following changes since commit 6ff33f3902c3b1c5d0db6b1e2c70b6d76fba357f:
> 
>   Linux 4.3-rc1 (2015-09-12 16:35:56 -0700)
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git/ 
> tags/usb-ci-v4.3-rc2

Pulled and pushed out, 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: [RFC][PATCH] Add spurious wakeup quirk for Lynxpoint controllers

[Laura Abbott]

On 09/18/2015 03:18 AM, Mathias Nyman wrote:

On 10.09.2015 20:27, Laura Abbott wrote:


We received several reports of systems rebooting and powering on
after an attempted shutdown. Testing showed that setting
XHCI_SPURIOUS_WAKEUP quirk in addition to the XHCI_SPURIOUS_REBOOT
quirk allowed the system to shutdown as expected for Lynxpoint
xHCI controllers. Set the qurik.

Signed-off-by: Laura Abbott 
---


We used to have the XHCI_SPURIOUS_WAKEUP flag set for lynxpoint controllers,
but it was removed in commit:

commit b45abacde3d551c6696c6738bef4a1805d0bf27a
 xhci: no switching back on non-ULT Haswell
 The switch back is limited to ULT even on HP. The contrary
 finding arose by bad luck in BIOS versions for testing.
 This fixes spontaneous resume from S3 on some HP laptops.

Adding the SPURIOUS_WAKEUP flag back looks reasonable to me,
but I don't want to break suspend.
I don't understand how it could have caused spontaneous resume in HP laptops
in the first place, it really shouldn't do anything before shutdown.

Better ask Oliver,
Do you still have access to the HP laptop?
Any chance you could see if the flag still causes spontaneous resume?

-Mathias


Would you rather see a revert of the patch you gave rather than a new
one re-introducing the flag?

Thanks,
Laura
--
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: chipidea: udc: improve error handling on ep_queue and f_midi

[Felipe Balbi]

Hi,

On Fri, Sep 18, 2015 at 05:48:45PM +0100, e...@felipetonello.com wrote:
> From: "Felipe F. Tonello" 
> 
> _ep_queue() didn't check for errors when using add_td_to_list()
> which can fail if dma_pool_alloc fails, thus causing a kernel
> panic when lastnode->ptr is NULL.
> 
> Also f_midi is not checking weather the is an error on usb_ep_queue
> request, ignoring potential problems, such as memory leaks.
> 
> Signed-off-by: Felipe F. Tonello 
> ---
>  drivers/usb/chipidea/udc.c   | 26 +++---
>  drivers/usb/gadget/function/f_midi.c | 11 +--

why are you patching f_midi.c in a chipidea patch ? Have a read at
Documentation/SubmittingPatches, Documentation/SubmitChecklist

-- 
balbi


signature.asc
Description: Digital signature

[PATCH 1/2] usb: chipidea: udc: improve error handling on ep_queue

[eu]

From: "Felipe F. Tonello" 

_ep_queue() didn't check for errors when using add_td_to_list()
which can fail if dma_pool_alloc fails, thus causing a kernel
panic when lastnode->ptr is NULL.

Signed-off-by: Felipe F. Tonello 
---
 drivers/usb/chipidea/udc.c | 26 +++---
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index 764f668..7169113e 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -404,9 +404,9 @@ static inline u8 _usb_addr(struct ci_hw_ep *ep)
 }
 
 /**
- * _hardware_queue: configures a request at hardware level
- * @gadget: gadget
+ * _hardware_enqueue: configures a request at hardware level
  * @hwep:   endpoint
+ * @hwreq:  request
  *
  * This function returns an error code
  */
@@ -435,19 +435,27 @@ static int _hardware_enqueue(struct ci_hw_ep *hwep, 
struct ci_hw_req *hwreq)
if (hwreq->req.dma % PAGE_SIZE)
pages--;
 
-   if (rest == 0)
-   add_td_to_list(hwep, hwreq, 0);
+   if (rest == 0) {
+   ret = add_td_to_list(hwep, hwreq, 0);
+   if (ret < 0)
+   goto done;
+   }
 
while (rest > 0) {
unsigned count = min(hwreq->req.length - hwreq->req.actual,
(unsigned)(pages * CI_HDRC_PAGE_SIZE));
-   add_td_to_list(hwep, hwreq, count);
+   ret = add_td_to_list(hwep, hwreq, count);
+   if (ret < 0)
+   goto done;
rest -= count;
}
 
if (hwreq->req.zero && hwreq->req.length
-   && (hwreq->req.length % hwep->ep.maxpacket == 0))
-   add_td_to_list(hwep, hwreq, 0);
+   && (hwreq->req.length % hwep->ep.maxpacket == 0)) {
+   ret = add_td_to_list(hwep, hwreq, 0);
+   if (ret < 0)
+   goto done;
+   }
 
firstnode = list_first_entry(&hwreq->tds, struct td_node, td);
 
@@ -750,8 +758,12 @@ static void isr_get_status_complete(struct usb_ep *ep, 
struct usb_request *req)
 
 /**
  * _ep_queue: queues (submits) an I/O request to an endpoint
+ * @ep:endpoint
+ * @req:   request
+ * @gfp_flags: GFP flags (not used)
  *
  * Caller must hold lock
+ * This function returns an error code
  */
 static int _ep_queue(struct usb_ep *ep, struct usb_request *req,
gfp_t __maybe_unused gfp_flags)
-- 
2.1.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

[PATCH 2/2] usb: gadget: f_midi: check for error on usb_ep_queue

[eu]

From: "Felipe F. Tonello" 

f_midi is not checking weather the is an error on usb_ep_queue
request, ignoring potential problems, such as memory leaks.

Signed-off-by: Felipe F. Tonello 
---
 drivers/usb/gadget/function/f_midi.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_midi.c 
b/drivers/usb/gadget/function/f_midi.c
index ad50a67..a5e446d 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -543,8 +543,14 @@ static void f_midi_transmit(struct f_midi *midi, struct 
usb_request *req)
}
}
 
-   if (req->length > 0)
-   usb_ep_queue(ep, req, GFP_ATOMIC);
+   if (req->length > 0) {
+   int err;
+
+   err = usb_ep_queue(ep, req, GFP_ATOMIC);
+   if (err < 0)
+   ERROR(midi, "%s queue req: %d\n",
+ midi->out_ep->name, err);
+   }
else
free_ep_req(ep, req);
 }
-- 
2.1.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 1/2] usb: chipidea: udc: improve error handling on ep_queue

[Felipe Balbi]

Hi,

On Fri, Sep 18, 2015 at 06:12:40PM +0100, e...@felipetonello.com wrote:
> From: "Felipe F. Tonello" 
> 
> _ep_queue() didn't check for errors when using add_td_to_list()
> which can fail if dma_pool_alloc fails, thus causing a kernel
> panic when lastnode->ptr is NULL.
> 
> Signed-off-by: Felipe F. Tonello 

this can still be split down further.

> ---
>  drivers/usb/chipidea/udc.c | 26 +++---
>  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> index 764f668..7169113e 100644
> --- a/drivers/usb/chipidea/udc.c
> +++ b/drivers/usb/chipidea/udc.c
> @@ -404,9 +404,9 @@ static inline u8 _usb_addr(struct ci_hw_ep *ep)
>  }
>  
>  /**
> - * _hardware_queue: configures a request at hardware level
> - * @gadget: gadget
> + * _hardware_enqueue: configures a request at hardware level
>   * @hwep:   endpoint
> + * @hwreq:  request

this is a cleanup and you shouldn't have a fix depending on a
cleanup. Fixes are merged during the -rc cycle, while cleanups will be
deferred to the following merge window.

>   *
>   * This function returns an error code
>   */
> @@ -435,19 +435,27 @@ static int _hardware_enqueue(struct ci_hw_ep *hwep, 
> struct ci_hw_req *hwreq)
>   if (hwreq->req.dma % PAGE_SIZE)
>   pages--;
>  
> - if (rest == 0)
> - add_td_to_list(hwep, hwreq, 0);
> + if (rest == 0) {
> + ret = add_td_to_list(hwep, hwreq, 0);
> + if (ret < 0)
> + goto done;
> + }

this is your fix.

>  
>   while (rest > 0) {
>   unsigned count = min(hwreq->req.length - hwreq->req.actual,
>   (unsigned)(pages * CI_HDRC_PAGE_SIZE));
> - add_td_to_list(hwep, hwreq, count);
> + ret = add_td_to_list(hwep, hwreq, count);
> + if (ret < 0)
> + goto done;

and this

>   rest -= count;
>   }
>  
>   if (hwreq->req.zero && hwreq->req.length
> - && (hwreq->req.length % hwep->ep.maxpacket == 0))
> - add_td_to_list(hwep, hwreq, 0);
> + && (hwreq->req.length % hwep->ep.maxpacket == 0)) {
> + ret = add_td_to_list(hwep, hwreq, 0);
> + if (ret < 0)
> + goto done;
> + }
>


and this.

>   firstnode = list_first_entry(&hwreq->tds, struct td_node, td);
>  
> @@ -750,8 +758,12 @@ static void isr_get_status_complete(struct usb_ep *ep, 
> struct usb_request *req)
>  
>  /**
>   * _ep_queue: queues (submits) an I/O request to an endpoint
> + * @ep:endpoint
> + * @req:   request
> + * @gfp_flags: GFP flags (not used)

cleanup

>   *
>   * Caller must hold lock
> + * This function returns an error code

somewhat pointless, but could come with the cleanup, no strong
feelings.

>   */
>  static int _ep_queue(struct usb_ep *ep, struct usb_request *req,
>   gfp_t __maybe_unused gfp_flags)
> -- 
> 2.1.4
> 

-- 
balbi


signature.asc
Description: Digital signature

[PATCH 0/2] improve error handling on chipidea/udc.c and f_midi.c

[eu]

From: "Felipe F. Tonello" 

Felipe F. Tonello (2):
  usb: chipidea: udc: improve error handling on ep_queue
  usb: gadget: f_midi: check for error on usb_ep_queue

 drivers/usb/chipidea/udc.c   | 26 +++---
 drivers/usb/gadget/function/f_midi.c | 10 --
 2 files changed, 27 insertions(+), 9 deletions(-)

-- 
2.1.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 2/2] usb: gadget: f_midi: check for error on usb_ep_queue

[Felipe Balbi]

On Fri, Sep 18, 2015 at 06:12:41PM +0100, e...@felipetonello.com wrote:
> From: "Felipe F. Tonello" 
> 
> f_midi is not checking weather the is an error on usb_ep_queue
> request, ignoring potential problems, such as memory leaks.
> 
> Signed-off-by: Felipe F. Tonello 
> ---
>  drivers/usb/gadget/function/f_midi.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_midi.c 
> b/drivers/usb/gadget/function/f_midi.c
> index ad50a67..a5e446d 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -543,8 +543,14 @@ static void f_midi_transmit(struct f_midi *midi, struct 
> usb_request *req)
>   }
>   }
>  
> - if (req->length > 0)
> - usb_ep_queue(ep, req, GFP_ATOMIC);
> + if (req->length > 0) {
> + int err;
> +
> + err = usb_ep_queue(ep, req, GFP_ATOMIC);
> + if (err < 0)
> + ERROR(midi, "%s queue req: %d\n",
> +   midi->out_ep->name, err);
> + }
>   else

yeah, cool, but you need to fix the style here. This else needs
to be after the curly brace and you need to curly brace to the
else branch too.

>   free_ep_req(ep, req);
>  }
> -- 
> 2.1.4
> 

-- 
balbi


signature.asc
Description: Digital signature

[PATCH 2/3] usb: chipidea: udc: improve error handling on ep_queue

[eu]

From: "Felipe F. Tonello" 

_ep_queue() didn't check for errors when using add_td_to_list()
which can fail if dma_pool_alloc fails, thus causing a kernel
panic when lastnode->ptr is NULL.

Signed-off-by: Felipe F. Tonello 
---

Changes for v2:
  - Use separate patch for cleanups.

 drivers/usb/chipidea/udc.c | 18 +-
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index c936c72..7169113e 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -435,19 +435,27 @@ static int _hardware_enqueue(struct ci_hw_ep *hwep, 
struct ci_hw_req *hwreq)
if (hwreq->req.dma % PAGE_SIZE)
pages--;
 
-   if (rest == 0)
-   add_td_to_list(hwep, hwreq, 0);
+   if (rest == 0) {
+   ret = add_td_to_list(hwep, hwreq, 0);
+   if (ret < 0)
+   goto done;
+   }
 
while (rest > 0) {
unsigned count = min(hwreq->req.length - hwreq->req.actual,
(unsigned)(pages * CI_HDRC_PAGE_SIZE));
-   add_td_to_list(hwep, hwreq, count);
+   ret = add_td_to_list(hwep, hwreq, count);
+   if (ret < 0)
+   goto done;
rest -= count;
}
 
if (hwreq->req.zero && hwreq->req.length
-   && (hwreq->req.length % hwep->ep.maxpacket == 0))
-   add_td_to_list(hwep, hwreq, 0);
+   && (hwreq->req.length % hwep->ep.maxpacket == 0)) {
+   ret = add_td_to_list(hwep, hwreq, 0);
+   if (ret < 0)
+   goto done;
+   }
 
firstnode = list_first_entry(&hwreq->tds, struct td_node, td);
 
-- 
2.1.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

[PATCH 1/3] usb: chipidea: udc: _ep_queue and _hw_queue cleanup

[eu]

From: "Felipe F. Tonello" 

Update comments to reflect current state of functions.

Signed-off-by: Felipe F. Tonello 
---

Changes for v2:
  - Introduced this patch.

 drivers/usb/chipidea/udc.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index 764f668..c936c72 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -404,9 +404,9 @@ static inline u8 _usb_addr(struct ci_hw_ep *ep)
 }
 
 /**
- * _hardware_queue: configures a request at hardware level
- * @gadget: gadget
+ * _hardware_enqueue: configures a request at hardware level
  * @hwep:   endpoint
+ * @hwreq:  request
  *
  * This function returns an error code
  */
@@ -750,8 +750,12 @@ static void isr_get_status_complete(struct usb_ep *ep, 
struct usb_request *req)
 
 /**
  * _ep_queue: queues (submits) an I/O request to an endpoint
+ * @ep:endpoint
+ * @req:   request
+ * @gfp_flags: GFP flags (not used)
  *
  * Caller must hold lock
+ * This function returns an error code
  */
 static int _ep_queue(struct usb_ep *ep, struct usb_request *req,
gfp_t __maybe_unused gfp_flags)
-- 
2.1.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 1/2] usb: chipidea: udc: improve error handling on ep_queue

[Felipe Tonello]

Hi Felipe,

On Fri, Sep 18, 2015 at 6:17 PM, Felipe Balbi  wrote:
> Hi,
>
> On Fri, Sep 18, 2015 at 06:12:40PM +0100, e...@felipetonello.com wrote:
>> From: "Felipe F. Tonello" 
>>
>> _ep_queue() didn't check for errors when using add_td_to_list()
>> which can fail if dma_pool_alloc fails, thus causing a kernel
>> panic when lastnode->ptr is NULL.
>>
>> Signed-off-by: Felipe F. Tonello 
>
> this can still be split down further.
>
>> ---
>>  drivers/usb/chipidea/udc.c | 26 +++---
>>  1 file changed, 19 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
>> index 764f668..7169113e 100644
>> --- a/drivers/usb/chipidea/udc.c
>> +++ b/drivers/usb/chipidea/udc.c
>> @@ -404,9 +404,9 @@ static inline u8 _usb_addr(struct ci_hw_ep *ep)
>>  }
>>
>>  /**
>> - * _hardware_queue: configures a request at hardware level
>> - * @gadget: gadget
>> + * _hardware_enqueue: configures a request at hardware level
>>   * @hwep:   endpoint
>> + * @hwreq:  request
>
> this is a cleanup and you shouldn't have a fix depending on a
> cleanup. Fixes are merged during the -rc cycle, while cleanups will be
> deferred to the following merge window.

Got it.

>
>>   *
>>   * This function returns an error code
>>   */
>> @@ -435,19 +435,27 @@ static int _hardware_enqueue(struct ci_hw_ep *hwep, 
>> struct ci_hw_req *hwreq)
>>   if (hwreq->req.dma % PAGE_SIZE)
>>   pages--;
>>
>> - if (rest == 0)
>> - add_td_to_list(hwep, hwreq, 0);
>> + if (rest == 0) {
>> + ret = add_td_to_list(hwep, hwreq, 0);
>> + if (ret < 0)
>> + goto done;
>> + }
>
> this is your fix.
>
>>
>>   while (rest > 0) {
>>   unsigned count = min(hwreq->req.length - hwreq->req.actual,
>>   (unsigned)(pages * CI_HDRC_PAGE_SIZE));
>> - add_td_to_list(hwep, hwreq, count);
>> + ret = add_td_to_list(hwep, hwreq, count);
>> + if (ret < 0)
>> + goto done;
>
> and this
>
>>   rest -= count;
>>   }
>>
>>   if (hwreq->req.zero && hwreq->req.length
>> - && (hwreq->req.length % hwep->ep.maxpacket == 0))
>> - add_td_to_list(hwep, hwreq, 0);
>> + && (hwreq->req.length % hwep->ep.maxpacket == 0)) {
>> + ret = add_td_to_list(hwep, hwreq, 0);
>> + if (ret < 0)
>> + goto done;
>> + }
>>
>
>
> and this.
>
>>   firstnode = list_first_entry(&hwreq->tds, struct td_node, td);
>>
>> @@ -750,8 +758,12 @@ static void isr_get_status_complete(struct usb_ep *ep, 
>> struct usb_request *req)
>>
>>  /**
>>   * _ep_queue: queues (submits) an I/O request to an endpoint
>> + * @ep:endpoint
>> + * @req:   request
>> + * @gfp_flags: GFP flags (not used)
>
> cleanup
>
>>   *
>>   * Caller must hold lock
>> + * This function returns an error code
>
> somewhat pointless, but could come with the cleanup, no strong
> feelings.
>
>>   */
>>  static int _ep_queue(struct usb_ep *ep, struct usb_request *req,
>>   gfp_t __maybe_unused gfp_flags)
>> --
>> 2.1.4
>>
>
> --
> 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

[PATCH 3/3] usb: gadget: f_midi: check for error on usb_ep_queue

[eu]

From: "Felipe F. Tonello" 

f_midi is not checking weather the is an error on usb_ep_queue
request, ignoring potential problems, such as memory leaks.

Signed-off-by: Felipe F. Tonello 
---

Changes for v2:
  - Update code style.

 drivers/usb/gadget/function/f_midi.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/function/f_midi.c 
b/drivers/usb/gadget/function/f_midi.c
index ad50a67..c04133422 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -543,10 +543,16 @@ static void f_midi_transmit(struct f_midi *midi, struct 
usb_request *req)
}
}
 
-   if (req->length > 0)
-   usb_ep_queue(ep, req, GFP_ATOMIC);
-   else
+   if (req->length > 0) {
+   int err;
+
+   err = usb_ep_queue(ep, req, GFP_ATOMIC);
+   if (err < 0)
+   ERROR(midi, "%s queue req: %d\n",
+ midi->out_ep->name, err);
+   } else {
free_ep_req(ep, req);
+   }
 }
 
 static void f_midi_in_tasklet(unsigned long data)
-- 
2.1.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 3/3] usb: gadget: f_midi: check for error on usb_ep_queue

[Felipe Tonello]

On Fri, Sep 18, 2015 at 6:30 PM,   wrote:
> From: "Felipe F. Tonello" 
>
> f_midi is not checking weather the is an error on usb_ep_queue
> request, ignoring potential problems, such as memory leaks.
>
> Signed-off-by: Felipe F. Tonello 
> ---
>
> Changes for v2:
>   - Update code style.
>
>  drivers/usb/gadget/function/f_midi.c | 12 +---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_midi.c 
> b/drivers/usb/gadget/function/f_midi.c
> index ad50a67..c04133422 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -543,10 +543,16 @@ static void f_midi_transmit(struct f_midi *midi, struct 
> usb_request *req)
> }
> }
>
> -   if (req->length > 0)
> -   usb_ep_queue(ep, req, GFP_ATOMIC);
> -   else
> +   if (req->length > 0) {
> +   int err;
> +
> +   err = usb_ep_queue(ep, req, GFP_ATOMIC);
> +   if (err < 0)
> +   ERROR(midi, "%s queue req: %d\n",
> + midi->out_ep->name, err);

I just realised there is a problem here. It is in_ep in this case. I
will fix it in v3.

> +   } else {
> free_ep_req(ep, req);
> +   }
>  }
>
>  static void f_midi_in_tasklet(unsigned long data)
> --
> 2.1.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

[PATCH 3/3] usb: gadget: f_midi: check for error on usb_ep_queue

[eu]

From: "Felipe F. Tonello" 

f_midi is not checking whether there is an error on usb_ep_queue
request, ignoring potential problems, such as memory leaks.

Signed-off-by: Felipe F. Tonello 
---

Changes for v2:
  - Update code style.

Changes for v3:
  - Use ip_ep instead of out_ep. Fixed typo in commit message.

 drivers/usb/gadget/function/f_midi.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/function/f_midi.c 
b/drivers/usb/gadget/function/f_midi.c
index ad50a67..edb84ca 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -543,10 +543,16 @@ static void f_midi_transmit(struct f_midi *midi, struct 
usb_request *req)
}
}
 
-   if (req->length > 0)
-   usb_ep_queue(ep, req, GFP_ATOMIC);
-   else
+   if (req->length > 0) {
+   int err;
+
+   err = usb_ep_queue(ep, req, GFP_ATOMIC);
+   if (err < 0)
+   ERROR(midi, "%s queue req: %d\n",
+ midi->in_ep->name, err);
+   } else {
free_ep_req(ep, req);
+   }
 }
 
 static void f_midi_in_tasklet(unsigned long data)
-- 
2.1.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

[PATCH 0/4] usb: host: Fix module autoload for OF platform drivers

[Luis de Bethencourt]

Hi,

These patches add the missing MODULE_DEVICE_TABLE() for OF to export
the information so modules have the correct aliases built-in and
autoloading works correctly.

A longer explanation by Javier Canillas can be found here:
https://lkml.org/lkml/2015/7/30/519

Thanks,
Luis

Luis de Bethencourt (4):
  usb: host: ehci-spear: Fix module autoload for OF platform driver
  usb: host: fsl-mph-dr-of: Fix module autoload for OF platform driver
  usb: host: ohci-spear: Fix module autoload for OF platform driver
  usb: host: uhci-platform: Fix module autoload for OF platform driver

 drivers/usb/host/ehci-spear.c| 1 +
 drivers/usb/host/fsl-mph-dr-of.c | 1 +
 drivers/usb/host/ohci-spear.c| 1 +
 drivers/usb/host/uhci-platform.c | 1 +
 4 files changed, 4 insertions(+)

-- 
2.4.6

--
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 1/4] usb: host: ehci-spear: Fix module autoload for OF platform driver

[Luis de Bethencourt]

This platform driver has a OF device ID table but the OF module
alias information is not created so module autoloading won't work.

Signed-off-by: Luis de Bethencourt 
---
 drivers/usb/host/ehci-spear.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/host/ehci-spear.c b/drivers/usb/host/ehci-spear.c
index 34e1474..be7bcf2 100644
--- a/drivers/usb/host/ehci-spear.c
+++ b/drivers/usb/host/ehci-spear.c
@@ -149,6 +149,7 @@ static const struct of_device_id spear_ehci_id_table[] = {
{ .compatible = "st,spear600-ehci", },
{ },
 };
+MODULE_DEVICE_TABLE(spear_ehci_id_table);
 
 static struct platform_driver spear_ehci_hcd_driver = {
.probe  = spear_ehci_hcd_drv_probe,
-- 
2.4.6

--
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: host: fsl-mph-dr-of: Fix module autoload for OF platform driver

[Luis de Bethencourt]

This platform driver has a OF device ID table but the OF module
alias information is not created so module autoloading won't work.

Signed-off-by: Luis de Bethencourt 
---
 drivers/usb/host/fsl-mph-dr-of.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/host/fsl-mph-dr-of.c b/drivers/usb/host/fsl-mph-dr-of.c
index 534c4c5..0c38265 100644
--- a/drivers/usb/host/fsl-mph-dr-of.c
+++ b/drivers/usb/host/fsl-mph-dr-of.c
@@ -351,6 +351,7 @@ static const struct of_device_id fsl_usb2_mph_dr_of_match[] 
= {
 #endif
{},
 };
+MODULE_DEVICE_TABLE(of, fsl_usb2_mph_dr_of_match);
 
 static struct platform_driver fsl_usb2_mph_dr_driver = {
.driver = {
-- 
2.4.6

--
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 3/4] usb: host: ohci-spear: Fix module autoload for OF platform driver

[Luis de Bethencourt]

Signed-off-by: Luis de Bethencourt 
---
 drivers/usb/host/ohci-spear.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/host/ohci-spear.c b/drivers/usb/host/ohci-spear.c
index 707437c..56478ed 100644
--- a/drivers/usb/host/ohci-spear.c
+++ b/drivers/usb/host/ohci-spear.c
@@ -161,6 +161,7 @@ static const struct of_device_id spear_ohci_id_table[] = {
{ .compatible = "st,spear600-ohci", },
{ },
 };
+MODULE_DEVICE_TABLE(of, spear_ohci_id_table);
 
 /* Driver definition to register with the platform bus */
 static struct platform_driver spear_ohci_hcd_driver = {
-- 
2.4.6

--
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: host: uhci-platform: Fix module autoload for OF platform driver

[Luis de Bethencourt]

This platform driver has a OF device ID table but the OF module
alias information is not created so module autoloading won't work.

Signed-off-by: Luis de Bethencourt 
---
 drivers/usb/host/uhci-platform.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/host/uhci-platform.c b/drivers/usb/host/uhci-platform.c
index 3a3e3ee..32a6f3d 100644
--- a/drivers/usb/host/uhci-platform.c
+++ b/drivers/usb/host/uhci-platform.c
@@ -140,6 +140,7 @@ static const struct of_device_id platform_uhci_ids[] = {
{ .compatible = "platform-uhci", },
{}
 };
+MODULE_DEVICE_TABLE(of, platform_uhci_ids);
 
 static struct platform_driver uhci_platform_driver = {
.probe  = uhci_hcd_platform_probe,
-- 
2.4.6

--
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 00/16] usb: gadget: amd5536udc: fix memory leaks

[Felipe Balbi]

On Mon, Sep 14, 2015 at 08:42:47PM +0530, Sudip Mukherjee wrote:
> This amd5536udc was a complete mess. The major problems that i could
> find are:
> 
> 1) if udc_pci_probe() fails in any stage then it just calls the
> udc_pci_remove() to handle error. And udc_pci_remove() works with
> struct udc *dev which we get from pci_get_drvdata(pdev). But we do the
> pci_set_drvdata(pdev, dev) almost at the end of probe. So basically
> incase of error we are handling the error by dereferencing a NULL
> pointer.
> 
> 2) udc_pci_remove() does a BUG_ON(dev->driver != NULL) and dev->driver
> will be set only if probe is success. So that means if probe fails then
> probe will call udc_pci_remove() for error handling and udc_pci_remove()
> will inturn halts the kernel by calling BUG().
> 
> And apart from these numerous memory leaks and not releasing of
> resources. Here comes a rewrite of few of the functions in an
> attempt to fix these.

run checkpatch.pl and try again

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v2 1/3] usb: of: add an api to get dr_mode by the phy node

[Felipe Balbi]

On Tue, Sep 15, 2015 at 01:58:16PM -0500, Bin Liu wrote:
> Some USB phy drivers have different handling for the controller in each
> dr_mode. But the phy driver does not have visibility to the dr_mode of
> the controller.
> 
> This adds an api to return the dr_mode of the controller which
> associates the given phy node.
> 
> Signed-off-by: Bin Liu 
> ---
> v2: move drivers/usb/phy/phy-am335x.c changes into patch 3/3.
> 
>  drivers/usb/common/common.c | 27 +++
>  include/linux/usb/of.h  |  6 ++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
> index b530fd4..3c7dee8 100644
> --- a/drivers/usb/common/common.c
> +++ b/drivers/usb/common/common.c
> @@ -114,6 +114,33 @@ enum usb_dr_mode of_usb_get_dr_mode(struct device_node 
> *np)
>  EXPORT_SYMBOL_GPL(of_usb_get_dr_mode);
>  
>  /**
> + * of_usb_get_dr_mode_by_phy - Get dual role mode for the controller device
> + * which is associated with the given phy device_node
> + * @np:  Pointer to the given phy device_node
> + *
> + * In dts a usb controller associates with a phy device.  The function gets
> + * the string from property 'dr_mode' of the controller associated with the
> + * given phy device node, and returns the correspondig enum usb_dr_mode.
> + */
> +enum usb_dr_mode of_usb_get_dr_mode_by_phy(struct device_node *phy_np)
> +{
> + struct device_node *controller;
> + struct device_node *phy;
> +
> + controller = of_get_parent(phy_np);
> + do {
> + controller = of_find_node_with_property(controller, "phys");
> + if (!controller)
> + return USB_DR_MODE_UNKNOWN;
> +
> + phy = of_parse_phandle(controller, "phys", 0);
> + } while (phy != phy_np);
> +
> + return of_usb_get_dr_mode(controller);
> +}
> +EXPORT_SYMBOL_GPL(of_usb_get_dr_mode_by_phy);

this assumes that the USB controller is described as a parent of
the PHY, but there's no requirement for this; this is merely a
function of how TI integrated the USB subsystem in HW.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v2 1/3] usb: of: add an api to get dr_mode by the phy node

[Bin Liu]

Hi,

On 09/18/2015 01:41 PM, Felipe Balbi wrote:

On Tue, Sep 15, 2015 at 01:58:16PM -0500, Bin Liu wrote:

Some USB phy drivers have different handling for the controller in each
dr_mode. But the phy driver does not have visibility to the dr_mode of
the controller.

This adds an api to return the dr_mode of the controller which
associates the given phy node.

Signed-off-by: Bin Liu 
---
v2: move drivers/usb/phy/phy-am335x.c changes into patch 3/3.

  drivers/usb/common/common.c | 27 +++
  include/linux/usb/of.h  |  6 ++
  2 files changed, 33 insertions(+)

diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
index b530fd4..3c7dee8 100644
--- a/drivers/usb/common/common.c
+++ b/drivers/usb/common/common.c
@@ -114,6 +114,33 @@ enum usb_dr_mode of_usb_get_dr_mode(struct device_node *np)
  EXPORT_SYMBOL_GPL(of_usb_get_dr_mode);

  /**
+ * of_usb_get_dr_mode_by_phy - Get dual role mode for the controller device
+ * which is associated with the given phy device_node
+ * @np:Pointer to the given phy device_node
+ *
+ * In dts a usb controller associates with a phy device.  The function gets
+ * the string from property 'dr_mode' of the controller associated with the
+ * given phy device node, and returns the correspondig enum usb_dr_mode.
+ */
+enum usb_dr_mode of_usb_get_dr_mode_by_phy(struct device_node *phy_np)
+{
+   struct device_node *controller;
+   struct device_node *phy;
+
+   controller = of_get_parent(phy_np);
+   do {
+   controller = of_find_node_with_property(controller, "phys");
+   if (!controller)
+   return USB_DR_MODE_UNKNOWN;
+
+   phy = of_parse_phandle(controller, "phys", 0);
+   } while (phy != phy_np);
+
+   return of_usb_get_dr_mode(controller);
+}
+EXPORT_SYMBOL_GPL(of_usb_get_dr_mode_by_phy);


this assumes that the USB controller is described as a parent of
the PHY, but there's no requirement for this; this is merely a
function of how TI integrated the USB subsystem in HW.

Okay, then how about looking up the controller node from root? My first 
draft of the patch was:


+enum usb_dr_mode of_usb_get_dr_mode_by_phy(struct device_node *phy_np)
+{
+   struct device_node *controller = NULL;
+   struct device_node *phy;
+
+   do {
+   controller = of_find_node_with_property(controller, "phys");
..

Regards,
-Bin.
--
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 v3] usb: dwc2: Use platform endianness when accessing registers

[Felipe Balbi]

On Thu, Aug 27, 2015 at 12:10:24AM +, John Youn wrote:
> On 8/20/2015 11:43 AM, Antti Seppälä wrote:
> > This patch switches calls to readl/writel to their
> > dwc2_readl/dwc2_writel equivalents which preserve platform endianness.
> > 
> > This patch is necessary to access dwc2 registers correctly on big-endian
> > systems such as the mips based SoCs made by Lantiq. Then dwc2 can be
> > used to replace ifx-hcd driver for Lantiq platforms found e.g. in
> > OpenWrt.
> > 
> > The patch was autogenerated with the following commands:
> > $EDITOR core.h
> > sed -i "s/\/dwc2_readl/g" *.c hcd.h hw.h
> > sed -i "s/\/dwc2_writel/g" *.c hcd.h hw.h
> > 
> > Some files were then hand-edited to fix checkpatch.pl warnings about
> > too long lines.
> > 
> > Signed-off-by: Antti Seppälä 
> > Signed-off-by: Vincent Pelletier 
> > ---
> > 
> > Notes:
> > Changes in v2:
> >  - Fixed wrong comment style
> > 
> > Changes in v3
> >  - Rebased against latest linux-next
> >  - Tweaked indentation in some files
> > 
> >  drivers/usb/dwc2/core.c  | 469 
> > ++-
> >  drivers/usb/dwc2/core.h  |  28 ++-
> >  drivers/usb/dwc2/core_intr.c |  73 +++
> >  drivers/usb/dwc2/debugfs.c   |  52 ++---
> >  drivers/usb/dwc2/gadget.c| 314 ++---
> >  drivers/usb/dwc2/hcd.c   | 140 ++---
> >  drivers/usb/dwc2/hcd.h   |  15 +-
> >  drivers/usb/dwc2/hcd_ddma.c  |  10 +-
> >  drivers/usb/dwc2/hcd_intr.c  |  82 
> >  drivers/usb/dwc2/hcd_queue.c |  10 +-
> >  10 files changed, 606 insertions(+), 587 deletions(-)
> > 
> 
> Acked-by: John Youn 
> 
> 
> Hi Felipe,
> 
> Can you queue this for 4.4? I can remind you after the merge
> window.

okay, this doesn't apply anymore. We need a refresh on top of
my testing/next.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v3] usb: dwc2: Use platform endianness when accessing registers

[John Youn]

On 9/18/2015 11:50 AM, Felipe Balbi wrote:
> On Thu, Aug 27, 2015 at 12:10:24AM +, John Youn wrote:
>> On 8/20/2015 11:43 AM, Antti Seppälä wrote:
>>> This patch switches calls to readl/writel to their
>>> dwc2_readl/dwc2_writel equivalents which preserve platform endianness.
>>>
>>> This patch is necessary to access dwc2 registers correctly on big-endian
>>> systems such as the mips based SoCs made by Lantiq. Then dwc2 can be
>>> used to replace ifx-hcd driver for Lantiq platforms found e.g. in
>>> OpenWrt.
>>>
>>> The patch was autogenerated with the following commands:
>>> $EDITOR core.h
>>> sed -i "s/\/dwc2_readl/g" *.c hcd.h hw.h
>>> sed -i "s/\/dwc2_writel/g" *.c hcd.h hw.h
>>>
>>> Some files were then hand-edited to fix checkpatch.pl warnings about
>>> too long lines.
>>>
>>> Signed-off-by: Antti Seppälä 
>>> Signed-off-by: Vincent Pelletier 
>>> ---
>>>
>>> Notes:
>>> Changes in v2:
>>>  - Fixed wrong comment style
>>> 
>>> Changes in v3
>>>  - Rebased against latest linux-next
>>>  - Tweaked indentation in some files
>>>
>>>  drivers/usb/dwc2/core.c  | 469 
>>> ++-
>>>  drivers/usb/dwc2/core.h  |  28 ++-
>>>  drivers/usb/dwc2/core_intr.c |  73 +++
>>>  drivers/usb/dwc2/debugfs.c   |  52 ++---
>>>  drivers/usb/dwc2/gadget.c| 314 ++---
>>>  drivers/usb/dwc2/hcd.c   | 140 ++---
>>>  drivers/usb/dwc2/hcd.h   |  15 +-
>>>  drivers/usb/dwc2/hcd_ddma.c  |  10 +-
>>>  drivers/usb/dwc2/hcd_intr.c  |  82 
>>>  drivers/usb/dwc2/hcd_queue.c |  10 +-
>>>  10 files changed, 606 insertions(+), 587 deletions(-)
>>>
>>
>> Acked-by: John Youn 
>>
>>
>> Hi Felipe,
>>
>> Can you queue this for 4.4? I can remind you after the merge
>> window.
> 
> okay, this doesn't apply anymore. We need a refresh on top of
> my testing/next.
> 

I have just fixed this up locally on your testing/next
intending to resend. It is also based on your s3c->dwc2
patch. I will send shortly.

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] usb: host: ehci-spear: Fix module autoload for OF platform driver

[Alan Stern]

On Fri, 18 Sep 2015, Luis de Bethencourt wrote:

> This platform driver has a OF device ID table but the OF module
> alias information is not created so module autoloading won't work.
> 
> Signed-off-by: Luis de Bethencourt 
> ---
>  drivers/usb/host/ehci-spear.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/host/ehci-spear.c b/drivers/usb/host/ehci-spear.c
> index 34e1474..be7bcf2 100644
> --- a/drivers/usb/host/ehci-spear.c
> +++ b/drivers/usb/host/ehci-spear.c
> @@ -149,6 +149,7 @@ static const struct of_device_id spear_ehci_id_table[] = {
>   { .compatible = "st,spear600-ehci", },
>   { },
>  };
> +MODULE_DEVICE_TABLE(spear_ehci_id_table);

Shouldn't this be MODULE_DEVICE_TABLE(of, spear_ehci_id_table)?

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 3/4] usb: host: ohci-spear: Fix module autoload for OF platform driver

[Alan Stern]

On Fri, 18 Sep 2015, Luis de Bethencourt wrote:

> Signed-off-by: Luis de Bethencourt 
> ---

-ENODESCRIPTION

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 v3] usb: dwc2: Use platform endianness when accessing registers

[Felipe Balbi]

On Fri, Sep 18, 2015 at 06:54:56PM +, John Youn wrote:
> On 9/18/2015 11:50 AM, Felipe Balbi wrote:
> > On Thu, Aug 27, 2015 at 12:10:24AM +, John Youn wrote:
> >> On 8/20/2015 11:43 AM, Antti Seppälä wrote:
> >>> This patch switches calls to readl/writel to their
> >>> dwc2_readl/dwc2_writel equivalents which preserve platform endianness.
> >>>
> >>> This patch is necessary to access dwc2 registers correctly on big-endian
> >>> systems such as the mips based SoCs made by Lantiq. Then dwc2 can be
> >>> used to replace ifx-hcd driver for Lantiq platforms found e.g. in
> >>> OpenWrt.
> >>>
> >>> The patch was autogenerated with the following commands:
> >>> $EDITOR core.h
> >>> sed -i "s/\/dwc2_readl/g" *.c hcd.h hw.h
> >>> sed -i "s/\/dwc2_writel/g" *.c hcd.h hw.h
> >>>
> >>> Some files were then hand-edited to fix checkpatch.pl warnings about
> >>> too long lines.
> >>>
> >>> Signed-off-by: Antti Seppälä 
> >>> Signed-off-by: Vincent Pelletier 
> >>> ---
> >>>
> >>> Notes:
> >>> Changes in v2:
> >>>  - Fixed wrong comment style
> >>> 
> >>> Changes in v3
> >>>  - Rebased against latest linux-next
> >>>  - Tweaked indentation in some files
> >>>
> >>>  drivers/usb/dwc2/core.c  | 469 
> >>> ++-
> >>>  drivers/usb/dwc2/core.h  |  28 ++-
> >>>  drivers/usb/dwc2/core_intr.c |  73 +++
> >>>  drivers/usb/dwc2/debugfs.c   |  52 ++---
> >>>  drivers/usb/dwc2/gadget.c| 314 ++---
> >>>  drivers/usb/dwc2/hcd.c   | 140 ++---
> >>>  drivers/usb/dwc2/hcd.h   |  15 +-
> >>>  drivers/usb/dwc2/hcd_ddma.c  |  10 +-
> >>>  drivers/usb/dwc2/hcd_intr.c  |  82 
> >>>  drivers/usb/dwc2/hcd_queue.c |  10 +-
> >>>  10 files changed, 606 insertions(+), 587 deletions(-)
> >>>
> >>
> >> Acked-by: John Youn 
> >>
> >>
> >> Hi Felipe,
> >>
> >> Can you queue this for 4.4? I can remind you after the merge
> >> window.
> > 
> > okay, this doesn't apply anymore. We need a refresh on top of
> > my testing/next.
> > 
> 
> I have just fixed this up locally on your testing/next
> intending to resend. It is also based on your s3c->dwc2
> patch. I will send shortly.

thanks :-)

-- 
balbi


signature.asc
Description: Digital signature

Re: First kernel patch (optimization)

[Austin S Hemmelgarn]

On 2015-09-18 05:31, Raymond Jennings wrote:

On 09/18/15 00:42, Greg KH wrote:

On Thu, Sep 17, 2015 at 11:12:51PM -0400, Theodore Ts'o wrote:

On Wed, Sep 16, 2015 at 01:26:51PM -0400, Josh Boyer wrote:

That isn't true.  It helps the submitter understand the workflow and
expectations.  What you meant to say is that it doesn't help you.

The problem is that workflow isn't the hard part.  It's the part that
can be taught most easily, sure.  But people seem to get really hung
up on it, and I fear that we have people who never progress beyond
sending trivial patches and spelling fixes and white space fixes and
micro-optimizations.

If the "you too can be a kernel developer" classes and web sites and
tutorials also taught people how to take performance measurements, and
something about the scientific measurement, that would be something.
Or if it taught people how to create tests and to run regression
testing.  Or if it taught people how to try to do fuzz testing, and
then once they find a sequence which causes crash, how to narrow down
the failure to a specific part of the kernel, and how to fix and
confirm that the kernel no longer crashes with the fix --- that would
be useful.

If they can understand kernel code; if they can understand the
scientific measurement; if they can understand how to do performance
measurements --- being able to properly format patches is something
which most kernel developers can very easily guide a new contributor
to do correctly.  Or in the worst case, it doesn't take much time for
me to fix a whitespace problem and just tell the contributor --- by
the way, I fixed up this minor issue; could you please make sure you
do this in the future?

But if a test hasn't been tested, or if the contributor things it's a
micro-optimization, but it actually takes more CPU time and/or more
stack space and/or bloats the kernel --- that's much more work for the
kernel maintainer to have to deal with when reviewing a patch.

So I have a very strong disagreement with the belief that teaching
people the workflow is the more important thing.  In my mind, that's
like first focusing on the proper how to properly fill out a golf
score card, and the ettiquette and traditions around handicaps, etc
--- before making sure the prospective player is good at putting and
driving.  Personally, I'm terrible at putting and driving, so spending
a lot of time learning how to fill out a golf score card would be a
waste of my time.

A good kernel programmer has to understand systems thinking; how to
figure out abstractions and when it's a good thing to add a new layer
of abstraction and when it's better to rework an exsting abstraction
layer.

If we have someone who knows the workflow, but which doesn't
understand systems thinking, or how to do testing, then what?  Great,
we've just created another Nick Krause.  Do you think encouraging a
Nick Krause helps anyone?

If people really are hung up on learning the workflow, I don't mind if
they want to learn that part and send some silly micro-optimization or
spelling fix or whitespace fix.  But it's really, really important
that they move beyond that.  And if they aren't capable of moving
beyond that, trying to inflate are recruitment numbers by encouraging
someone who can only do trivial fixes means that we may be get what we
can easily measure --- but it may not be what we really need as a
community.

Ted, you are full of crap.

Where do you think that "new developers" come from?  Do they show up in
our inbox, with full knowledge of kernel internals and OS theory yet
they somehow just can't grasp how to submit a patch correctly?  Yes,
they sometimes rarely do.  But for the majority of people who got into
Linux, that is not the case at all.

People need to start with something simple, and easy, to get over the
hurdles of:
- generating a patch
- sending an email
- fixing the email client as it just corrupted the patch
- fix the subject line as it was incorrect
- fix the changelog as it was missing
- fix the email client again as it corrupted the patch in a
  different way
- giving up on using a web email client as it just will not work
- figuring out who to send the patch to
- fixing the email client as the mailing list bounced the email

Those are non-trivial tasks.  And by starting with "remove this space"
you take the worry away from the specific content of the patch, and let
them worry about the "hard" part first.

+1 for this.

For example, I for one cannot tell you how many times gmail snuck html
sections into my outgoing emails before I finally caught it red handed
and switched to using a local native client.

It's also worth pointing out that if we don't have people submitting 
code cleanups and typo fixes and stuff like that, then either we end up 
over time with unmaintainable gibberish, or the people who do know 
systems programming spending most of their time trying to deal with 
cleaning up the code (and this is com

Re: [PATCH 1/4] usb: host: ehci-spear: Fix module autoload for OF platform driver

[Luis de Bethencourt]

On Fri, Sep 18, 2015 at 02:57:10PM -0400, Alan Stern wrote:
> On Fri, 18 Sep 2015, Luis de Bethencourt wrote:
> 
> > This platform driver has a OF device ID table but the OF module
> > alias information is not created so module autoloading won't work.
> > 
> > Signed-off-by: Luis de Bethencourt 
> > ---
> >  drivers/usb/host/ehci-spear.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/usb/host/ehci-spear.c b/drivers/usb/host/ehci-spear.c
> > index 34e1474..be7bcf2 100644
> > --- a/drivers/usb/host/ehci-spear.c
> > +++ b/drivers/usb/host/ehci-spear.c
> > @@ -149,6 +149,7 @@ static const struct of_device_id spear_ehci_id_table[] 
> > = {
> > { .compatible = "st,spear600-ehci", },
> > { },
> >  };
> > +MODULE_DEVICE_TABLE(spear_ehci_id_table);
> 
> Shouldn't this be MODULE_DEVICE_TABLE(of, spear_ehci_id_table)?
> 
> Alan Stern
> 

Hi Alan,

Sorry about this. It looks like I accidentally sent a temporary version of
patch 1/4 and 3/4.

Sending the final and correct version now.

Thanks for the review,
Luis
--
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 1/3] usb: of: add an api to get dr_mode by the phy node

[Felipe Balbi]

On Fri, Sep 18, 2015 at 01:45:56PM -0500, Bin Liu wrote:
> Hi,
> 
> On 09/18/2015 01:41 PM, Felipe Balbi wrote:
> >On Tue, Sep 15, 2015 at 01:58:16PM -0500, Bin Liu wrote:
> >>Some USB phy drivers have different handling for the controller in each
> >>dr_mode. But the phy driver does not have visibility to the dr_mode of
> >>the controller.
> >>
> >>This adds an api to return the dr_mode of the controller which
> >>associates the given phy node.
> >>
> >>Signed-off-by: Bin Liu 
> >>---
> >>v2: move drivers/usb/phy/phy-am335x.c changes into patch 3/3.
> >>
> >>  drivers/usb/common/common.c | 27 +++
> >>  include/linux/usb/of.h  |  6 ++
> >>  2 files changed, 33 insertions(+)
> >>
> >>diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
> >>index b530fd4..3c7dee8 100644
> >>--- a/drivers/usb/common/common.c
> >>+++ b/drivers/usb/common/common.c
> >>@@ -114,6 +114,33 @@ enum usb_dr_mode of_usb_get_dr_mode(struct device_node 
> >>*np)
> >>  EXPORT_SYMBOL_GPL(of_usb_get_dr_mode);
> >>
> >>  /**
> >>+ * of_usb_get_dr_mode_by_phy - Get dual role mode for the controller device
> >>+ * which is associated with the given phy device_node
> >>+ * @np:Pointer to the given phy device_node
> >>+ *
> >>+ * In dts a usb controller associates with a phy device.  The function gets
> >>+ * the string from property 'dr_mode' of the controller associated with the
> >>+ * given phy device node, and returns the correspondig enum usb_dr_mode.
> >>+ */
> >>+enum usb_dr_mode of_usb_get_dr_mode_by_phy(struct device_node *phy_np)
> >>+{
> >>+   struct device_node *controller;
> >>+   struct device_node *phy;
> >>+
> >>+   controller = of_get_parent(phy_np);
> >>+   do {
> >>+   controller = of_find_node_with_property(controller, "phys");
> >>+   if (!controller)
> >>+   return USB_DR_MODE_UNKNOWN;
> >>+
> >>+   phy = of_parse_phandle(controller, "phys", 0);
> >>+   } while (phy != phy_np);
> >>+
> >>+   return of_usb_get_dr_mode(controller);
> >>+}
> >>+EXPORT_SYMBOL_GPL(of_usb_get_dr_mode_by_phy);
> >
> >this assumes that the USB controller is described as a parent of
> >the PHY, but there's no requirement for this; this is merely a
> >function of how TI integrated the USB subsystem in HW.
> >
> Okay, then how about looking up the controller node from root? My first
> draft of the patch was:
> 
> +enum usb_dr_mode of_usb_get_dr_mode_by_phy(struct device_node *phy_np)
> +{
> + struct device_node *controller = NULL;
> + struct device_node *phy;
> +
> + do {
> + controller = of_find_node_with_property(controller, "phys");
>   ..

if it works. No idea if there's a better way for doing this.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v2 1/3] usb: of: add an api to get dr_mode by the phy node

[Bin Liu]

Hi,

On 09/18/2015 02:09 PM, Felipe Balbi wrote:

On Fri, Sep 18, 2015 at 01:45:56PM -0500, Bin Liu wrote:

Hi,

On 09/18/2015 01:41 PM, Felipe Balbi wrote:

On Tue, Sep 15, 2015 at 01:58:16PM -0500, Bin Liu wrote:

Some USB phy drivers have different handling for the controller in each
dr_mode. But the phy driver does not have visibility to the dr_mode of
the controller.

This adds an api to return the dr_mode of the controller which
associates the given phy node.

Signed-off-by: Bin Liu 
---
v2: move drivers/usb/phy/phy-am335x.c changes into patch 3/3.

  drivers/usb/common/common.c | 27 +++
  include/linux/usb/of.h  |  6 ++
  2 files changed, 33 insertions(+)

diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
index b530fd4..3c7dee8 100644
--- a/drivers/usb/common/common.c
+++ b/drivers/usb/common/common.c
@@ -114,6 +114,33 @@ enum usb_dr_mode of_usb_get_dr_mode(struct device_node *np)
  EXPORT_SYMBOL_GPL(of_usb_get_dr_mode);

  /**
+ * of_usb_get_dr_mode_by_phy - Get dual role mode for the controller device
+ * which is associated with the given phy device_node
+ * @np:Pointer to the given phy device_node
+ *
+ * In dts a usb controller associates with a phy device.  The function gets
+ * the string from property 'dr_mode' of the controller associated with the
+ * given phy device node, and returns the correspondig enum usb_dr_mode.
+ */
+enum usb_dr_mode of_usb_get_dr_mode_by_phy(struct device_node *phy_np)
+{
+   struct device_node *controller;
+   struct device_node *phy;
+
+   controller = of_get_parent(phy_np);
+   do {
+   controller = of_find_node_with_property(controller, "phys");
+   if (!controller)
+   return USB_DR_MODE_UNKNOWN;
+
+   phy = of_parse_phandle(controller, "phys", 0);
+   } while (phy != phy_np);
+
+   return of_usb_get_dr_mode(controller);
+}
+EXPORT_SYMBOL_GPL(of_usb_get_dr_mode_by_phy);


this assumes that the USB controller is described as a parent of
the PHY, but there's no requirement for this; this is merely a
function of how TI integrated the USB subsystem in HW.


Okay, then how about looking up the controller node from root? My first
draft of the patch was:

+enum usb_dr_mode of_usb_get_dr_mode_by_phy(struct device_node *phy_np)
+{
+   struct device_node *controller = NULL;
+   struct device_node *phy;
+
+   do {
+   controller = of_find_node_with_property(controller, "phys");
..


if it works. No idea if there's a better way for doing this.



Yes, I tested it and it works. Then I thought only looking up from PHY's 
parent would make the search quicker but forgot that the PHY node could 
be at anywhere in dts.


Will send v3 soon.

Thanks,
-Bin.
--
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 v2 1/4] usb: host: ehci-spear: Fix module autoload for OF platform driver

[Luis de Bethencourt]

This platform driver has a OF device ID table but the OF module
alias information is not created so module autoloading won't work.

Signed-off-by: Luis de Bethencourt 
---
 drivers/usb/host/ehci-spear.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/host/ehci-spear.c b/drivers/usb/host/ehci-spear.c
index 34e1474..3c4e525 100644
--- a/drivers/usb/host/ehci-spear.c
+++ b/drivers/usb/host/ehci-spear.c
@@ -149,6 +149,7 @@ static const struct of_device_id spear_ehci_id_table[] = {
{ .compatible = "st,spear600-ehci", },
{ },
 };
+MODULE_DEVICE_TABLE(of, spear_ehci_id_table);
 
 static struct platform_driver spear_ehci_hcd_driver = {
.probe  = spear_ehci_hcd_drv_probe,
-- 
2.4.6

--
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] usb: dwc2: Use platform endianness when accessing registers

[John Youn]

From: Antti Seppälä 

This patch switches calls to readl/writel to their
dwc2_readl/dwc2_writel equivalents which preserve platform endianness.

This patch is necessary to access dwc2 registers correctly on big-endian
systems such as the mips based SoCs made by Lantiq. Then dwc2 can be
used to replace ifx-hcd driver for Lantiq platforms found e.g. in
OpenWrt.

The patch was autogenerated with the following commands:
$EDITOR core.h
sed -i "s/\/dwc2_readl/g" *.c hcd.h hw.h
sed -i "s/\/dwc2_writel/g" *.c hcd.h hw.h

Some files were then hand-edited to fix checkpatch.pl warnings about
too long lines.

Signed-off-by: Antti Seppälä 
Signed-off-by: Vincent Pelletier 
Signed-off-by: John Youn 
---

Resend of previous patch from Antti Seppälä.
Fixed up by John Youn to apply to Felipe's
latest tree.


 drivers/usb/dwc2/core.c  | 469 ++-
 drivers/usb/dwc2/core.h  |  28 ++-
 drivers/usb/dwc2/core_intr.c |  73 +++
 drivers/usb/dwc2/debugfs.c   |  52 ++---
 drivers/usb/dwc2/gadget.c| 284 +-
 drivers/usb/dwc2/hcd.c   | 140 ++---
 drivers/usb/dwc2/hcd.h   |  15 +-
 drivers/usb/dwc2/hcd_ddma.c  |  10 +-
 drivers/usb/dwc2/hcd_intr.c  |  82 
 drivers/usb/dwc2/hcd_queue.c |  10 +-
 10 files changed, 591 insertions(+), 572 deletions(-)

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index b00fe95..fc0521a 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -73,13 +73,13 @@ static int dwc2_backup_host_registers(struct dwc2_hsotg 
*hsotg)
 
/* Backup Host regs */
hr = &hsotg->hr_backup;
-   hr->hcfg = readl(hsotg->regs + HCFG);
-   hr->haintmsk = readl(hsotg->regs + HAINTMSK);
+   hr->hcfg = dwc2_readl(hsotg->regs + HCFG);
+   hr->haintmsk = dwc2_readl(hsotg->regs + HAINTMSK);
for (i = 0; i < hsotg->core_params->host_channels; ++i)
-   hr->hcintmsk[i] = readl(hsotg->regs + HCINTMSK(i));
+   hr->hcintmsk[i] = dwc2_readl(hsotg->regs + HCINTMSK(i));
 
-   hr->hprt0 = readl(hsotg->regs + HPRT0);
-   hr->hfir = readl(hsotg->regs + HFIR);
+   hr->hprt0 = dwc2_readl(hsotg->regs + HPRT0);
+   hr->hfir = dwc2_readl(hsotg->regs + HFIR);
hr->valid = true;
 
return 0;
@@ -108,14 +108,14 @@ static int dwc2_restore_host_registers(struct dwc2_hsotg 
*hsotg)
}
hr->valid = false;
 
-   writel(hr->hcfg, hsotg->regs + HCFG);
-   writel(hr->haintmsk, hsotg->regs + HAINTMSK);
+   dwc2_writel(hr->hcfg, hsotg->regs + HCFG);
+   dwc2_writel(hr->haintmsk, hsotg->regs + HAINTMSK);
 
for (i = 0; i < hsotg->core_params->host_channels; ++i)
-   writel(hr->hcintmsk[i], hsotg->regs + HCINTMSK(i));
+   dwc2_writel(hr->hcintmsk[i], hsotg->regs + HCINTMSK(i));
 
-   writel(hr->hprt0, hsotg->regs + HPRT0);
-   writel(hr->hfir, hsotg->regs + HFIR);
+   dwc2_writel(hr->hprt0, hsotg->regs + HPRT0);
+   dwc2_writel(hr->hfir, hsotg->regs + HFIR);
 
return 0;
 }
@@ -146,15 +146,15 @@ static int dwc2_backup_device_registers(struct dwc2_hsotg 
*hsotg)
/* Backup dev regs */
dr = &hsotg->dr_backup;
 
-   dr->dcfg = readl(hsotg->regs + DCFG);
-   dr->dctl = readl(hsotg->regs + DCTL);
-   dr->daintmsk = readl(hsotg->regs + DAINTMSK);
-   dr->diepmsk = readl(hsotg->regs + DIEPMSK);
-   dr->doepmsk = readl(hsotg->regs + DOEPMSK);
+   dr->dcfg = dwc2_readl(hsotg->regs + DCFG);
+   dr->dctl = dwc2_readl(hsotg->regs + DCTL);
+   dr->daintmsk = dwc2_readl(hsotg->regs + DAINTMSK);
+   dr->diepmsk = dwc2_readl(hsotg->regs + DIEPMSK);
+   dr->doepmsk = dwc2_readl(hsotg->regs + DOEPMSK);
 
for (i = 0; i < hsotg->num_of_eps; i++) {
/* Backup IN EPs */
-   dr->diepctl[i] = readl(hsotg->regs + DIEPCTL(i));
+   dr->diepctl[i] = dwc2_readl(hsotg->regs + DIEPCTL(i));
 
/* Ensure DATA PID is correctly configured */
if (dr->diepctl[i] & DXEPCTL_DPID)
@@ -162,11 +162,11 @@ static int dwc2_backup_device_registers(struct dwc2_hsotg 
*hsotg)
else
dr->diepctl[i] |= DXEPCTL_SETD0PID;
 
-   dr->dieptsiz[i] = readl(hsotg->regs + DIEPTSIZ(i));
-   dr->diepdma[i] = readl(hsotg->regs + DIEPDMA(i));
+   dr->dieptsiz[i] = dwc2_readl(hsotg->regs + DIEPTSIZ(i));
+   dr->diepdma[i] = dwc2_readl(hsotg->regs + DIEPDMA(i));
 
/* Backup OUT EPs */
-   dr->doepctl[i] = readl(hsotg->regs + DOEPCTL(i));
+   dr->doepctl[i] = dwc2_readl(hsotg->regs + DOEPCTL(i));
 
/* Ensure DATA PID is correctly configured */
if (dr->doepctl[i] & DXEPCTL_DPID)
@@ -174,8 +174,8 @@ static int dwc2_backup_device_registers(struct dwc2_hsotg 
*hsotg)
else
dr->doepctl[i] |= DXEPCTL_SETD0PID;
 

Re: [PATCH v2 0/7] usb: usbtest misc changes

[Felipe Balbi]

Hi Greg,

On Tue, Sep 01, 2015 at 09:47:57AM +0800, Peter Chen wrote:
> Alan Stern (1):
>   usb: misc: usbtest: format the data pattern according to max packet
> size
> 
> Peter Chen (6):
>   usb: misc: usbtest: allocate size of urb array according to user
> parameter
>   usb: misc: usbtest: delete useless memset for urbs array
>   usb: misc: usbtest: using the same data format among
> write/compare/output
>   usb: gadget: f_sourcesink: format data pattern according to max packet
> size
>   tools: usb: testusb: change the help text
>   tools: usb: testusb: change the default value for length from 512 to
> 1024

I'm adding this series to my queue, if you want to take any patches
yourself, just let me know and I'll drop them at my end.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v2 1/4] usb: host: ehci-spear: Fix module autoload for OF platform driver

[Alan Stern]

On Fri, 18 Sep 2015, Luis de Bethencourt wrote:

> This platform driver has a OF device ID table but the OF module
> alias information is not created so module autoloading won't work.
> 
> Signed-off-by: Luis de Bethencourt 
> ---
>  drivers/usb/host/ehci-spear.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/host/ehci-spear.c b/drivers/usb/host/ehci-spear.c
> index 34e1474..3c4e525 100644
> --- a/drivers/usb/host/ehci-spear.c
> +++ b/drivers/usb/host/ehci-spear.c
> @@ -149,6 +149,7 @@ static const struct of_device_id spear_ehci_id_table[] = {
>   { .compatible = "st,spear600-ehci", },
>   { },
>  };
> +MODULE_DEVICE_TABLE(of, spear_ehci_id_table);
>  
>  static struct platform_driver spear_ehci_hcd_driver = {
>   .probe  = spear_ehci_hcd_drv_probe,
> 

Acked-by: Alan Stern 

Also for V2 of patches 3/4 and 4/4.

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 1/5] usb: common: of_usb_get_maximum_speed to usb_get_maximum_speed

[Felipe Balbi]

On Tue, Aug 25, 2015 at 02:04:31PM +0300, Heikki Krogerus wrote:
> By using the unified device property interface, the function
> can be made available for all platforms and not just the
> ones using DT.
> 
> Signed-off-by: Heikki Krogerus 

this breaks compilation of my current testing/next.

-- 
balbi


signature.asc
Description: Digital signature

[PATCH v3] usb: of: add an api to get dr_mode by the phy node

[Bin Liu]

Some USB phy drivers have different handling for the controller in each
dr_mode. But the phy driver does not have visibility to the dr_mode of
the controller.

This adds an api to return the dr_mode of the controller which
associates the given phy node.

Signed-off-by: Bin Liu 
---
v3: search controller node from dt root, as the phy and controller nodes
might not have the same parent.

 drivers/usb/common/common.c | 26 ++
 include/linux/usb/of.h  |  6 ++
 2 files changed, 32 insertions(+)

diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
index b530fd4..fc32055 100644
--- a/drivers/usb/common/common.c
+++ b/drivers/usb/common/common.c
@@ -114,6 +114,32 @@ enum usb_dr_mode of_usb_get_dr_mode(struct device_node *np)
 EXPORT_SYMBOL_GPL(of_usb_get_dr_mode);
 
 /**
+ * of_usb_get_dr_mode_by_phy - Get dual role mode for the controller device
+ * which is associated with the given phy device_node
+ * @np:Pointer to the given phy device_node
+ *
+ * In dts a usb controller associates with a phy device.  The function gets
+ * the string from property 'dr_mode' of the controller associated with the
+ * given phy device node, and returns the correspondig enum usb_dr_mode.
+ */
+enum usb_dr_mode of_usb_get_dr_mode_by_phy(struct device_node *phy_np)
+{
+   struct device_node *controller = NULL;
+   struct device_node *phy;
+
+   do {
+   controller = of_find_node_with_property(controller, "phys");
+   if (!controller)
+   return USB_DR_MODE_UNKNOWN;
+
+   phy = of_parse_phandle(controller, "phys", 0);
+   } while (phy != phy_np);
+
+   return of_usb_get_dr_mode(controller);
+}
+EXPORT_SYMBOL_GPL(of_usb_get_dr_mode_by_phy);
+
+/**
  * of_usb_get_maximum_speed - Get maximum requested speed for a given USB
  * controller.
  * @np: Pointer to the given device_node
diff --git a/include/linux/usb/of.h b/include/linux/usb/of.h
index cfe0528..14ebd5a 100644
--- a/include/linux/usb/of.h
+++ b/include/linux/usb/of.h
@@ -13,6 +13,7 @@
 
 #if IS_ENABLED(CONFIG_OF)
 enum usb_dr_mode of_usb_get_dr_mode(struct device_node *np);
+enum usb_dr_mode of_usb_get_dr_mode_by_phy(struct device_node *phy_np);
 enum usb_device_speed of_usb_get_maximum_speed(struct device_node *np);
 bool of_usb_host_tpl_support(struct device_node *np);
 #else
@@ -21,6 +22,11 @@ static inline enum usb_dr_mode of_usb_get_dr_mode(struct 
device_node *np)
return USB_DR_MODE_UNKNOWN;
 }
 
+enum usb_dr_mode of_usb_get_dr_mode_by_phy(struct device_node *phy_np)
+{
+   return USB_DR_MODE_UNKNOWN;
+}
+
 static inline enum usb_device_speed
 of_usb_get_maximum_speed(struct device_node *np)
 {
-- 
1.8.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: [RFC][PATCH] Add spurious wakeup quirk for Lynxpoint controllers

[Oliver Neukum]

On Fri, 2015-09-18 at 09:56 -0700, Laura Abbott wrote:
> Would you rather see a revert of the patch you gave rather than a new
> one re-introducing the flag?

We need a big fat comment here saying that different tests should
different results and the quirk is needed for LynxPoint. That
suggests a new patch.

Regards
Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] improve error handling on chipidea/udc.c and f_midi.c

[Albino B Neto]

2015-09-18 14:12 GMT-03:00  :
> From: "Felipe F. Tonello" 

Signed-off-by ?

  Albino
--
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 0/2] improve error handling on chipidea/udc.c and f_midi.c

[Felipe Balbi]

On Fri, Sep 18, 2015 at 05:56:48PM -0300, Albino B Neto wrote:
> 2015-09-18 14:12 GMT-03:00  :
> > From: "Felipe F. Tonello" 
> 
> Signed-off-by ?

this is not a patch

-- 
balbi


signature.asc
Description: Digital signature

Re: First kernel patch (optimization)

[Theodore Ts'o]

On Fri, Sep 18, 2015 at 12:42:48AM -0700, Greg KH wrote:
> So don't take cleanup patches for your code, that's fine, and I totally
> understand why _you_ don't want to do that.  But to blow off the effort
> as being somehow trivial and not worthy of us, that's totally missing
> the point, and making things harder on yourself in the end.

It's not that I think cleanup patches are "trivial and not worthy of
us", although I'll note that cleanup patches can end up causing real
bug fixes (including possibly fixes that address security problems) to
not apply to stable kernels, which means someone needs to deal with
failed patches --- or what is much more likely, the failed patch gets
bounced back to the overworked maintainer who then drops the patch
because he or she doesn't have the bandwidth to manually backport the
patch to N different stable trees, and then we all suffer.  So cleanup
patches *do* have a cost.

Rather, what concerns me is that we aren't pushing people to go
*beyond* cleanup patches.  We have lots of tutorials about how to
create perfectly formed patches; but we don't seem to have patches
about how to do proper benchmarking.  Or how to check system call and
ioctl interfaces to make sure the code is doing appropriate input
checking.  So I don't want to pre-reject these people; but to rather
push them and to help them to try their hand at more substantive work.

What surprises me is the apparent assumption that people need a huge
amount of hand-holding on how to properly form a patch, but that
people will be able to figure out how to do proper benchmarking, or
design proper abstractions, etc., as skills that will magically appear
full-formed into the new kernel programmer's mind without any help or
work on our part.

> So don't be a jerk, and spout off as to how we only need "skilled"
> people contributing.  Of course we need that, but the way we get those
> people is to help them become skilled, not requiring them to show up
> with those skills automatically.

I think where we disagree is in how to make them become skilled.  I
don't think just focusing on contents of SubmittingPatches is
sufficient, and there seems to be a "Step 2: And then a miracle
occurs" between the SubmittingPatches step and the "skilled
contributor" end goal.

Cheers,

- Ted
--
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 v3] usb: of: add an api to get dr_mode by the phy node

[Felipe Balbi]

Hi,

On Fri, Sep 18, 2015 at 03:27:58PM -0500, Bin Liu wrote:
> Some USB phy drivers have different handling for the controller in each
> dr_mode. But the phy driver does not have visibility to the dr_mode of
> the controller.
> 
> This adds an api to return the dr_mode of the controller which
> associates the given phy node.
> 
> Signed-off-by: Bin Liu 
> ---
> v3: search controller node from dt root, as the phy and controller nodes
> might not have the same parent.
> 
>  drivers/usb/common/common.c | 26 ++
>  include/linux/usb/of.h  |  6 ++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
> index b530fd4..fc32055 100644
> --- a/drivers/usb/common/common.c
> +++ b/drivers/usb/common/common.c
> @@ -114,6 +114,32 @@ enum usb_dr_mode of_usb_get_dr_mode(struct device_node 
> *np)
>  EXPORT_SYMBOL_GPL(of_usb_get_dr_mode);
>  
>  /**
> + * of_usb_get_dr_mode_by_phy - Get dual role mode for the controller device
> + * which is associated with the given phy device_node
> + * @np:  Pointer to the given phy device_node
> + *
> + * In dts a usb controller associates with a phy device.  The function gets
> + * the string from property 'dr_mode' of the controller associated with the
> + * given phy device node, and returns the correspondig enum usb_dr_mode.
> + */
> +enum usb_dr_mode of_usb_get_dr_mode_by_phy(struct device_node *phy_np)
> +{
> + struct device_node *controller = NULL;
> + struct device_node *phy;
> +
> + do {
> + controller = of_find_node_with_property(controller, "phys");
> + if (!controller)
> + return USB_DR_MODE_UNKNOWN;
> +
> + phy = of_parse_phandle(controller, "phys", 0);

another question: How do you know this is the USB PHY and not a SATA or
MII/GMII/RGMII, or anything else ?

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 00/16] usb: gadget: amd5536udc: fix memory leaks

[Sudip Mukherjee]

On Fri, Sep 18, 2015 at 01:39:54PM -0500, Felipe Balbi wrote:
> On Mon, Sep 14, 2015 at 08:42:47PM +0530, Sudip Mukherjee wrote:
> > This amd5536udc was a complete mess. The major problems that i could
> > find are:
> > 
> > 1) if udc_pci_probe() fails in any stage then it just calls the
> > udc_pci_remove() to handle error. And udc_pci_remove() works with
> > struct udc *dev which we get from pci_get_drvdata(pdev). But we do the
> > pci_set_drvdata(pdev, dev) almost at the end of probe. So basically
> > incase of error we are handling the error by dereferencing a NULL
> > pointer.
> > 
> > 2) udc_pci_remove() does a BUG_ON(dev->driver != NULL) and dev->driver
> > will be set only if probe is success. So that means if probe fails then
> > probe will call udc_pci_remove() for error handling and udc_pci_remove()
> > will inturn halts the kernel by calling BUG().
> > 
> > And apart from these numerous memory leaks and not releasing of
> > resources. Here comes a rewrite of few of the functions in an
> > attempt to fix these.
> 
> run checkpatch.pl and try again
I know checkpatch gives warning on some of my patches but as the warning
was not related to the part I have modified so I have not done any thing
with them as they will become unrelated changes than what is mentioned
in the commit log.
Anyways, I will fix up all the warnings and send v2. But do you want me
to also fix the checkpatch warnings in those patch where functions are
rearranged? Because in those patches functions were just moved and there
was no change in the body of the function.

regards
sudip
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: First kernel patch (optimization)

[Sudip Mukherjee]

On Fri, Sep 18, 2015 at 10:26:24PM -0400, Theodore Ts'o wrote:
> On Fri, Sep 18, 2015 at 12:42:48AM -0700, Greg KH wrote:
>
> Rather, what concerns me is that we aren't pushing people to go
> *beyond* cleanup patches.  We have lots of tutorials about how to
> create perfectly formed patches; but we don't seem to have patches
> about how to do proper benchmarking.  Or how to check system call and
> ioctl interfaces to make sure the code is doing appropriate input
> checking.  So I don't want to pre-reject these people; but to rather
> push them and to help them to try their hand at more substantive work.
> 
> What surprises me is the apparent assumption that people need a huge
> amount of hand-holding on how to properly form a patch, but that
> people will be able to figure out how to do proper benchmarking, or
> design proper abstractions, etc., as skills that will magically appear
> full-formed into the new kernel programmer's mind without any help or
> work on our part.
Reading Ted's earlier mail I was thinking since I don't have some of the
skills mentioned, maybe I am in wrong place. But what Ted said now is
almost the same what I said in the Ksummit discussion about recruitment.
I will copy-paste here for reference:
"In my opinion the main problem is lack of direction or guidance. As a 
newbie I send my first patch, it gets accepted, I have a party to
celebrate and do more style correction and few more patches are accepted.
But by that time I am getting bored with just style correction and want
to do something more.
Now the problem starts. No one is there to guide me and I as a newbie
will not be that much capable enough to find things to do on my own. And
I start loosing the interest. Newbies who are coming from Eudyptula or
starting on their own will face this. But on the otherhand participants
of Outreachy will get a Mentor to guide them and gets a stipend to keep
them motivated. Stipend may not matter to the right candidate who has
interest but having a mentor is the big difference."

This is from my own experience as I have gone through that time phrase.
But now after one year I know there are numerous things to do. But still
I don't have some of the skills Ted mentioned.

I know I will get the skills which I don't have now but since I am on my
own that will be a time consuming process. Even more time consuming as
this is not part of my dayjob. But if I had a mentor/guide who could
have given some hint the process might have been much much faster.

regards
sudip

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/7] usb: usbtest misc changes

[Greg KH]

On Fri, Sep 18, 2015 at 02:30:09PM -0500, Felipe Balbi wrote:
> Hi Greg,
> 
> On Tue, Sep 01, 2015 at 09:47:57AM +0800, Peter Chen wrote:
> > Alan Stern (1):
> >   usb: misc: usbtest: format the data pattern according to max packet
> > size
> > 
> > Peter Chen (6):
> >   usb: misc: usbtest: allocate size of urb array according to user
> > parameter
> >   usb: misc: usbtest: delete useless memset for urbs array
> >   usb: misc: usbtest: using the same data format among
> > write/compare/output
> >   usb: gadget: f_sourcesink: format data pattern according to max packet
> > size
> >   tools: usb: testusb: change the help text
> >   tools: usb: testusb: change the default value for length from 512 to
> > 1024
> 
> I'm adding this series to my queue, if you want to take any patches
> yourself, just let me know and I'll drop them at my end.

That's fine with me, you can take them.

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 v2 1/4] usb: host: ehci-spear: Fix module autoload for OF platform driver

[Greg Kroah-Hartman]

On Fri, Sep 18, 2015 at 09:14:52PM +0200, Luis de Bethencourt wrote:
> This platform driver has a OF device ID table but the OF module
> alias information is not created so module autoloading won't work.
> 
> Signed-off-by: Luis de Bethencourt 
> ---
>  drivers/usb/host/ehci-spear.c | 1 +
>  1 file changed, 1 insertion(+)

Ugh, what a mess, please resend the whole series when you fix up a patch
in it as this is hard to figure out what to do.

And can you "thread" the patch series properly?  That makes it much
easier for me to apply them as I can group them correctly.

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: First kernel patch (optimization)

[Greg KH]

On Fri, Sep 18, 2015 at 10:26:24PM -0400, Theodore Ts'o wrote:
> On Fri, Sep 18, 2015 at 12:42:48AM -0700, Greg KH wrote:
> > So don't take cleanup patches for your code, that's fine, and I totally
> > understand why _you_ don't want to do that.  But to blow off the effort
> > as being somehow trivial and not worthy of us, that's totally missing
> > the point, and making things harder on yourself in the end.
> 
> It's not that I think cleanup patches are "trivial and not worthy of
> us", although I'll note that cleanup patches can end up causing real
> bug fixes (including possibly fixes that address security problems) to
> not apply to stable kernels, which means someone needs to deal with
> failed patches --- or what is much more likely, the failed patch gets
> bounced back to the overworked maintainer who then drops the patch
> because he or she doesn't have the bandwidth to manually backport the
> patch to N different stable trees, and then we all suffer.  So cleanup
> patches *do* have a cost.

All patches have a "cost", and that cost is something that you have to
deal with as a maintainer.  Nothing new here at all, and if you don't
like cleanup patches, great, don't take them.  But never go around
saying that people should NOT send cleanup patches to subsystems you
don't maintain like you did.  That's rude and unhelpful to everyone
involved.

The best way to ensure that you never get whitespace patches, is to
clean your subsystem up.  And frankly, it needs a lot of work, have you
run checkpatch on it in a while?  You could resolve this issue for
yourself with a simple afternoon's worth of work.  Why you don't do so
is a mystery to me.

> Rather, what concerns me is that we aren't pushing people to go
> *beyond* cleanup patches.

On the contrary, I do this ALL THE TIME!  Maybe you don't see it as you
aren't on the staging mailing lists or other places where I do this.
Maybe if you started accepting cleanup patches to your subsystem, you
could start doing this yourself as well.

> We have lots of tutorials about how to
> create perfectly formed patches; but we don't seem to have patches
> about how to do proper benchmarking.  Or how to check system call and
> ioctl interfaces to make sure the code is doing appropriate input
> checking.  So I don't want to pre-reject these people; but to rather
> push them and to help them to try their hand at more substantive work.

You first have to crawl before you can run.

And write those tutorials please.  The act of writing the "write your
first kernel patch" tutorial showed that it is NOT a trivial thing to
do, and that there are tons of steps involved now that people use web
email clients and email servers that corrupt text emails (i.e.
Exchange), things that when we started out doing kernel development were
not an issue at all.

> What surprises me is the apparent assumption that people need a huge
> amount of hand-holding on how to properly form a patch, but that
> people will be able to figure out how to do proper benchmarking, or
> design proper abstractions, etc., as skills that will magically appear
> full-formed into the new kernel programmer's mind without any help or
> work on our part.

I have never said that at all, and have actively worked for the past 2
years to help provide guidance and work to get people from the "cleanup
patch" to "writing real code."  And it works, look at the Outreachy
program for loads of examples of this (i.e. almost every person who goes
through it gets a job offer.)

I have been saying for years that we have a lack of real projects /
tasks / ideas for people who are skilled, yet have no idea what to do.
I know of well over a hundred people I have email addresses of that have
asked me for these types of things, and have patches in the kernel that
are non-trivial to prove that they have the skill to do real things.

It's a real problem, and one that I don't have an answer for.  We need
these types of tasks, and I don't have them, and every maintainer I ask
about it also doesn't have them.  What we are asking for is people to
somehow come up with tasks on their own, as if they know what needs to
be done.

Remember, when we started, the number of things that needed to be done
was hugely obvious, everywhere we turned needed lots of work.  For
people new to the kernel, this isn't the case anymore.  Linux has taken
over the world because it is so capable and feature rich.  Picking up a
week-end task is almost impossible because of this.  I know, I have no
week-end-length tasks on my TODO list.

So, have any tasks that people can do on a week-end that you know of?

> > So don't be a jerk, and spout off as to how we only need "skilled"
> > people contributing.  Of course we need that, but the way we get those
> > people is to help them become skilled, not requiring them to show up
> > with those skills automatically.
> 
> I think where we disagree is in how to make them become skilled.  I
> don't think just focusing on contents of