[PATCH 1/1] usb: dwc3: keystone: check return value

[Pan Bian]

Function devm_clk_get() returns an ERR_PTR when it fails. However, in
function kdwc3_probe(), its return value is not checked, which may
result in a bad memory access bug. This patch fixes the bug.

Signed-off-by: Pan Bian 
---
 drivers/usb/dwc3/dwc3-keystone.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/dwc3/dwc3-keystone.c b/drivers/usb/dwc3/dwc3-keystone.c
index 7266470..12ee23f 100644
--- a/drivers/usb/dwc3/dwc3-keystone.c
+++ b/drivers/usb/dwc3/dwc3-keystone.c
@@ -107,6 +107,10 @@ static int kdwc3_probe(struct platform_device *pdev)
return PTR_ERR(kdwc->usbss);
 
kdwc->clk = devm_clk_get(kdwc->dev, "usb");
+   if (IS_ERR(kdwc->clk)) {
+   dev_err(kdwc->dev, "unable to get usb clock\n");
+   return PTR_ERR(kdwc->clk);
+   }
 
error = clk_prepare_enable(kdwc->clk);
if (error < 0) {
-- 
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: core: Warn if an URB's transfer_buffer is on stack

[Alan Stern]

On Sat, 22 Apr 2017, Florian Fainelli wrote:

> We see a large number of fixes to several drivers to remove the usage of
> on-stack buffers feeding into USB transfer functions. Make it easier to spot
> the offenders by adding a warning in usb_start_wait_urb() for
> urb->transfer_buffer to be located on the stack.
> 
> Signed-off-by: Florian Fainelli 
> ---
>  drivers/usb/core/message.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> index 2184ef40a82a..abefddbe9243 100644
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
> @@ -8,6 +8,7 @@
>  #include/* for scatterlist macros */
>  #include 
>  #include 
> +#include  /* for object_is_on_stack */
>  #include 
>  #include 
>  #include 
> @@ -50,6 +51,8 @@ static int usb_start_wait_urb(struct urb *urb, int timeout, 
> int *actual_length)
>   unsigned long expire;
>   int retval;
>  
> + WARN_ON(object_is_on_stack(urb->transfer_buffer));
> +
>   init_completion();
>   urb->context = 
>   urb->actual_length = 0;

Does this really help?  I mean, don't we already get warnings when 
the host controller drivers try to map on-stack buffers for DMA?

The only difference is that one wouldn't have to read as far back into
the stack trace.  But that hardly seems like an important
consideration.

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: usb: dwc2: NMI watchdog: BUG: soft lockup - CPU#0 stuck for 146s

[Stefan Wahren]

Hi,

> Eric Anholt  hat am 20. April 2017 um 20:54 geschrieben:
> 
> 
> Stefan Wahren  writes:
> 
> > Hi,
> >
> >> Doug Anderson  hat am 18. April 2017 um 22:41 
> >> geschrieben:
> >> 
> >> 
> >> It's hard to know for sure that all of this time is really in
> >> urb_enqueue().  Possible we could have task switched out and been
> >> blocked elsewhere.  Using ftrace to get more fine-grained timings
> >> would be useful.  ktime_get(), ktime_sub(), and ktime_to_us() are your
> >> friends here if you want to use trace_printk.
> >
> > i'm a newbie to ftrace, so i hope this would be helpful.
> >
> > # connect PL2303 to the onboard hub
> > # echo 0 > options/sleep-time
> > # echo 0 > function_profile_enabled
> > # echo 1 > function_profile_enabled
> > # ./usb_test
> > # Waiting for at least 20 seconds and then disconnect PL2303
> > # echo 0 > function_profile_enabled
> > # cat trace_stat/function0
> >
> >   Function   HitTimeAvg 
> > s^2
> >      ------ 
> > ---
> >   bcm2835_handle_irq  361347219567633 us 607.636 us 
> >  1485199 us  
> >   __handle_domain_irq1082482212639551 us 196.437 us 
> >  3642030 us  
> >   generic_handle_irq 1082482100592051 us 92.927 us  
> >  50511334 us 
> >   irq_exit   108248298197771 us 90.715 us   
> > 29649040 us 
> >   handle_level_irq   108248295812379 us 88.511 us   
> > 51910093 us 
> 
> If I'm reading this output right, we're spending half of our interrupt
> processing time in irq_exit(), so even if dwc2's interrupt was free (the
> generic_handle_irq() chain), we'd be eating about half the CPU getting
> back out of the interrupt handler, right?
> 
> I don't really know anything about DWC2 or USB, but is there any way we
> could mitigate the interrupt frequency with this hardware?  If nothing
> else, could we loop reading gintsts until it reads back 0?

first of all i updated my kernel to 4.11rc7 and the issue still occures. Today 
i had some time to investigate this issue further and i made some interesting 
observations:

1. The lockup doesn't occure always after starting usb_test. In rare cases i 
was able to run the program without lockup.
2. In case the lockup occured we are always able to "rescue" the BCM2835 from 
this state by sending some serial data to the PL2303.
3. I looked again at the logic analyzer traces of the PL2303 and FTDI. After 
usb_test has been started the time between 2 _dwc2_hcd_irq calls is mostly 
smaller than 6 us (this can't be correct for a single device doesn't send any 
data). With other words it seems to me one or more interrupt(s) are never acked.
4. I placed a trace_printk into dwc2_handle_hcd_intr() in order to dump 
GINTSTS. In normal state we usually reach the value 0x0429, but not in 
lockup state.

Regards
Stefan
--
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: core: Warn if an URB's transfer_buffer is on stack

[Florian Fainelli]

We see a large number of fixes to several drivers to remove the usage of
on-stack buffers feeding into USB transfer functions. Make it easier to spot
the offenders by adding a warning in usb_start_wait_urb() for
urb->transfer_buffer to be located on the stack.

Signed-off-by: Florian Fainelli 
---
 drivers/usb/core/message.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 2184ef40a82a..abefddbe9243 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -8,6 +8,7 @@
 #include  /* for scatterlist macros */
 #include 
 #include 
+#include  /* for object_is_on_stack */
 #include 
 #include 
 #include 
@@ -50,6 +51,8 @@ static int usb_start_wait_urb(struct urb *urb, int timeout, 
int *actual_length)
unsigned long expire;
int retval;
 
+   WARN_ON(object_is_on_stack(urb->transfer_buffer));
+
init_completion();
urb->context = 
urb->actual_length = 0;
-- 
2.11.0

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

[PATCH v2] usb: misc: legousbtower: Fix buffers on stack

[Maksim Salau]

Allocate buffers on HEAP instead of STACK for local structures
that are to be received using usb_control_msg().

Signed-off-by: Maksim Salau 
Tested-by: Alfredo Rafael Vicente Boix 
Cc: sta...@vger.kernel.org
---

 Changes in v2:
 * made checkpatch happy with the format string passed to dev_info
   in tower_probe() (merged two parts into a single string literal);
 * changed commit message to better reflect location of the module;
   was: USB: legousbtower: Fix buffers on stack;
 * added Tested-by: Alfredo Rafael Vicente Boix 
   and Cc: sta...@vger.kernel.org

 drivers/usb/misc/legousbtower.c | 37 +++--
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index b10e26c..ed1999e 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -317,9 +317,16 @@ static int tower_open (struct inode *inode, struct file 
*file)
int subminor;
int retval = 0;
struct usb_interface *interface;
-   struct tower_reset_reply reset_reply;
+   struct tower_reset_reply *reset_reply = NULL;
int result;
 
+   reset_reply = kmalloc(sizeof(*reset_reply), GFP_KERNEL);
+
+   if (!reset_reply) {
+   retval = -ENOMEM;
+   goto exit;
+   }
+
nonseekable_open(inode, file);
subminor = iminor(inode);
 
@@ -364,8 +371,8 @@ static int tower_open (struct inode *inode, struct file 
*file)
  USB_TYPE_VENDOR | USB_DIR_IN | 
USB_RECIP_DEVICE,
  0,
  0,
- _reply,
- sizeof(reset_reply),
+ reset_reply,
+ sizeof(*reset_reply),
  1000);
if (result < 0) {
dev_err(>udev->dev,
@@ -406,6 +413,7 @@ static int tower_open (struct inode *inode, struct file 
*file)
mutex_unlock(>lock);
 
 exit:
+   kfree(reset_reply);
return retval;
 }
 
@@ -808,7 +816,7 @@ static int tower_probe (struct usb_interface *interface, 
const struct usb_device
struct lego_usb_tower *dev = NULL;
struct usb_host_interface *iface_desc;
struct usb_endpoint_descriptor* endpoint;
-   struct tower_get_version_reply get_version_reply;
+   struct tower_get_version_reply *get_version_reply = NULL;
int i;
int retval = -ENOMEM;
int result;
@@ -886,6 +894,13 @@ static int tower_probe (struct usb_interface *interface, 
const struct usb_device
dev->interrupt_in_interval = interrupt_in_interval ? 
interrupt_in_interval : dev->interrupt_in_endpoint->bInterval;
dev->interrupt_out_interval = interrupt_out_interval ? 
interrupt_out_interval : dev->interrupt_out_endpoint->bInterval;
 
+   get_version_reply = kmalloc(sizeof(*get_version_reply), GFP_KERNEL);
+
+   if (!get_version_reply) {
+   retval = -ENOMEM;
+   goto error;
+   }
+
/* get the firmware version and log it */
result = usb_control_msg (udev,
  usb_rcvctrlpipe(udev, 0),
@@ -893,18 +908,19 @@ static int tower_probe (struct usb_interface *interface, 
const struct usb_device
  USB_TYPE_VENDOR | USB_DIR_IN | 
USB_RECIP_DEVICE,
  0,
  0,
- _version_reply,
- sizeof(get_version_reply),
+ get_version_reply,
+ sizeof(*get_version_reply),
  1000);
if (result < 0) {
dev_err(idev, "LEGO USB Tower get version control request 
failed\n");
retval = result;
goto error;
}
-   dev_info(>dev, "LEGO USB Tower firmware version is %d.%d "
-"build %d\n", get_version_reply.major,
-get_version_reply.minor,
-le16_to_cpu(get_version_reply.build_no));
+   dev_info(>dev,
+"LEGO USB Tower firmware version is %d.%d build %d\n",
+get_version_reply->major,
+get_version_reply->minor,
+le16_to_cpu(get_version_reply->build_no));
 
/* we can register the device now, as it is ready */
usb_set_intfdata (interface, dev);
@@ -928,6 +944,7 @@ static int tower_probe (struct usb_interface *interface, 
const struct usb_device
return retval;
 
 error:
+   kfree(get_version_reply);
tower_delete(dev);
return retval;
 }
-- 
2.9.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  

Re: Regression - Linux 4.9: ums_eneub6250 broken: transfer buffer not dma capable - Trace

[Alan Stern]

On Fri, 21 Apr 2017, Andreas Hartmann wrote:

> Here it goes:
> 
> [  337.698916] usb 1-1.1: new high-speed USB device number 3 using ehci-pci
> [  337.808695] usb 1-1.1: New USB device found, idVendor=0cf2, idProduct=6250
> [  337.808702] usb 1-1.1: New USB device strings: Mfr=1, Product=2, 
> SerialNumber=4
> [  337.808706] usb 1-1.1: Product: UB6250
> [  337.808709] usb 1-1.1: Manufacturer: ENE Flash
> [  337.808713] usb 1-1.1: SerialNumber: 606569746801
> [  337.809148] ums_eneub6250 1-1.1:1.0: USB Mass Storage device detected
> [  337.810232] scsi host6: usb-storage 1-1.1:1.0
> [  338.838194] usb 1-1.1: direct-loading ene-ub6250/sd_init1.bin
> [  339.021724] usb 1-1.1: direct-loading ene-ub6250/sd_init2.bin
> [  339.032399] scsi 6:0:0:0: Direct-Access USB2.0   CardReader   0100 
> PQ: 0 ANSI: 2
> [  339.032654] sd 6:0:0:0: [sdb] 3985408 512-byte logical blocks: (2.04 
> GB/1.90 GiB)
> [  339.032697] sd 6:0:0:0: [sdb] Write Protect is off
> [  339.032698] sd 6:0:0:0: Attached scsi generic sg2 type 0
> [  339.032704] sd 6:0:0:0: [sdb] Mode Sense: 0b 00 00 08
> [  339.032736] sd 6:0:0:0: [sdb] No Caching mode page found
> [  339.032744] sd 6:0:0:0: [sdb] Assuming drive cache: write through
> [  339.118833] usb 1-1.1: reset high-speed USB device number 3 using ehci-pci
> [  339.322803] usb 1-1.1: reset high-speed USB device number 3 using ehci-pci
> [  339.526902] usb 1-1.1: reset high-speed USB device number 3 using ehci-pci
> [  339.730932] usb 1-1.1: reset high-speed USB device number 3 using ehci-pci
> [  339.934901] usb 1-1.1: reset high-speed USB device number 3 using ehci-pci
> [  340.138893] usb 1-1.1: reset high-speed USB device number 3 using ehci-pci
> [  340.252054] usb 1-1.1: direct-loading ene-ub6250/sd_rdwr.bin

> Hope this helps!

Oops -- I forgot to tell you to enable CONFIG_USB_STORAGE_DEBUG before 
rebuilding the driver!  Well, you can do it this time around...

In the meanwhile, I see another problem.  The SCSI residue value is
getting overwritten when new firmware is sent to the device.  Like I said
before, it's amazing this driver has ever worked.

Please apply this patch on top of the previous one and let's see what 
happens.

Alan Stern



Index: usb-4.x/drivers/usb/storage/ene_ub6250.c
===
--- usb-4.x.orig/drivers/usb/storage/ene_ub6250.c
+++ usb-4.x/drivers/usb/storage/ene_ub6250.c
@@ -1942,6 +1942,8 @@ static int ene_load_bincode(struct us_da
bcb->CDB[0] = 0xEF;
 
result = ene_send_scsi_cmd(us, FDIR_WRITE, buf, 0);
+   if (us->srb != NULL)
+   scsi_set_resid(us->srb, 0);
info->BIN_FLAG = flag;
kfree(buf);
 

--
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 v17 2/3] usb: USB Type-C connector class

[Rajaram R]

On Fri, Apr 21, 2017 at 10:13 PM, Guenter Roeck  wrote:
> On Fri, Apr 21, 2017 at 07:57:52PM +0530, Rajaram R wrote:
>> On Fri, Apr 21, 2017 at 1:16 AM, Badhri Jagan Sridharan
>>  wrote:
>> > Thanks for the responses :)
>> >
>> > So seems like we have a plan.
>> >
>> > In Type-C connector class the checks for TYPEC_PWR_MODE_PD
>> > and pd_revision for both the port and the partner will be removed in
>> > power_role_store and the data_role_store and will be delegated
>> > to the low level drivers.
>>
>> It is important to remember what USB Type-C provide is mechanisms for
>> "TRYing" to become a particular role and not guaranteeing.
>>
>> With what device combination do you fore see we could get the desired
>> role with this change ?
>>
>
> If the partner is not PD capable, if a preferred role is specified,
> if the current cole does not match the preferred role, and if the request
> is to set the role to match the preferred role, I think it is reasonable
> to expect that re-establishing the connection would accomplish that if the
> partner supports it.
>
In this context I believe we have two different inputs as follows:

/sys/class/typec//supported_power_roles
/sys/class/typec//preferred_role

The need of preferred role is required when DRP is set in
supported_power_roles option.
Ideally a battery powered device will TRY to be SNK and a a/c plugged
device will TRY to be SRC

We need to understand which non-PD device will set to DRP? In the
current ecosystem all legacy devices
will sit behind adapters which either present an Rp or Rd.

If it is a power adapter in 5V range can either present Rp or DRP with
TRY.SRC and there is no role swap requirement.

If it is a laptop port or similar with non-PD (??) DRP  there is no
guaranteed role swap in a non-PD mode.
So we need to understand what non PD device will fit into this scenario ?

> Of course, that won't change anything if the partner does not support the
> desired role, but it is better than doing nothing. This is also comparable
> to requesting a role change from the partner if it does support PD.

All I am highlighting is that we can only TRY and there is no
guaranteed role swap with Type-C

> Do you have a better idea ?
>
If need a guaranteed role in a non-PD mode we need to set the required
role in supported_power_roles.
An understanding of scenario will help take better approach.

> Thanks,
> Guenter
>
>>
>> >
>> > TCPM code will issue hard reset in tcpm_dr_set and tcpm_pr_set if
>> > current_role is not same as the preferred_role.
>> >
>
> ... if the partner is not PD capable.
>
>> > I am going to make changes in my local kernel code base to start
>> > making the corresponding changes in userspace.
>> > Should I post-back the local kernel changes or Heikki and Geunter
>> > you are planning to upload them ?
>> >
>> > Thanks for the support !!
>> > Badhri.
>> >
>> > On Thu, Apr 20, 2017 at 5:24 AM, Heikki Krogerus
>> >  wrote:
>> >> On Wed, Apr 19, 2017 at 10:22:47AM -0700, Badhri Jagan Sridharan wrote:
>> >>> On Wed, Apr 19, 2017 at 8:14 AM, Guenter Roeck  
>> >>> wrote:
>> >>> > On Wed, Apr 19, 2017 at 07:45:00AM -0700, Badhri Jagan Sridharan wrote:
>> >>> >> On Wed, Apr 19, 2017 at 4:23 AM, Heikki Krogerus
>> >>> >>  wrote:
>> >>> >> > Hi,
>> >>> >> >
>> >>> >> > On Tue, Apr 18, 2017 at 11:52:33AM -0700, Badhri Jagan Sridharan 
>> >>> >> > wrote:
>> >>> >> >> Hi Heikki,
>> >>> >> >>
>> >>> >> >> I have a question regarding the preferred_role node.
>> >>> >> >>
>> >>> >> >> +What:  /sys/class/typec//preferred_role
>> >>> >> >> +Date:  March 2017
>> >>> >> >> +Contact:   Heikki Krogerus 
>> >>> >> >> +Description:
>> >>> >> >> +   The user space can notify the driver about the 
>> >>> >> >> preferred role.
>> >>> >> >> +   It should be handled as enabling of Try.SRC or 
>> >>> >> >> Try.SNK, as
>> >>> >> >> +   defined in USB Type-C specification, in the port 
>> >>> >> >> drivers. By
>> >>> >> >> +   default the preferred role should come from the 
>> >>> >> >> platform.
>> >>> >> >> +
>> >>> >> >> +   Valid values: source, sink, none (to remove 
>> >>> >> >> preference)
>> >>> >> >>
>> >>> >> >> What is the expected behavior when the userspace changes the
>> >>> >> >> preferred_role node when the port is in connected state ?
>> >>> >> >>
>> >>> >> >> 1.  the state machine re-resolves the port roles right away based 
>> >>> >> >> on
>> >>> >> >> the new state machine in place ? (or)
>> >>> >> >
>> >>> >> > No! There are separate attributes for sending role swap requests.
>> >>> >>
>> >>> >> Right. But, that might not be helpful in cases when PD is not 
>> >>> >> implemented.
>> >>> >> and Implementing PD is not mandatory according the spec :/
>> >>> >>
>> >>> >> FYI quoting from the Type-C specification release(page 

Re: [PATCH] USB: legousbtower: Fix buffers on stack

[Alfredo Rafael Vicente Boix]

Hello,

I have tested it and it works great!!! Thank you very much!! I use
nqc to test it and it has uploaded the firmware to the brick
perfectly.

[  164.835935] usb 4-5: new low-speed USB device number 3 using ohci-pci
[  165.050272] usb 4-5: New USB device found, idVendor=0694, idProduct=0001
[  165.050276] usb 4-5: New USB device strings: Mfr=4, Product=26,
SerialNumber=0
[  165.050278] usb 4-5: Product: LEGO USB Tower
[  165.050280] usb 4-5: Manufacturer: LEGO Group
[  165.055275] legousbtower 4-5:1.0: LEGO USB Tower firmware version
is 1.0 build 134
[  165.055481] legousbtower 4-5:1.0: LEGO USB Tower #-160 now attached
to major 180 minor 0

HOME Escritorio # nqc -Susb -firmware  ./firm0332.lgo
Downloading 
firmware:.
Current Version: 00030001/00030302

HOME Escritorio # uname -a
Linux HOME 4.10.12 #1 SMP Sat Apr 22 09:30:06 CEST 2017 x86_64 x86_64
x86_64 GNU/Linux

Thank you very much



2017-04-22 7:03 GMT+02:00 Greg Kroah-Hartman :
> On Fri, Apr 21, 2017 at 10:48:48PM +0300, Maksim Salau wrote:
>> Allocate buffers on HEAP instead of STACK for local structures
>> that are to be received using usb_control_msg().
>>
>> Signed-off-by: Maksim Salau 
>> ---
>>
>> I took the liberty to fix the module if Greg don't mind.
>> It is to be applied on vanilla v4.10.12 (without Greg's patch).
>
> Yeah!  Thank you for this, I got distracted with stable kernel work for
> the past few days.
>
>> Changes compared to Greg's version:
>> * fixed tower_reset() which is used in the open callback;
>> * better deallocation handling in case of failures.
>
> Alfredo, can you test this patch out?
>
> thanks again,
>
> 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