[PATCH v2] usb: typec: fix an IS_ERR() vs NULL bug in hd3ss3220_probe()
The device_get_named_child_node() function doesn't return error
pointers, it returns NULL on error.
Fixes: 1c48c759ef4b ("usb: typec: driver for TI HD3SS3220 USB Type-C DRP port
controller")
Signed-off-by: Dan Carpenter
---
v2: remove -ENODEV instead of -EIO
drivers/usb/typec/hd3ss3220.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
index 9715600aeb04..8afaf5768a17 100644
--- a/drivers/usb/typec/hd3ss3220.c
+++ b/drivers/usb/typec/hd3ss3220.c
@@ -172,8 +172,8 @@ static int hd3ss3220_probe(struct i2c_client *client,
hd3ss3220_set_source_pref(hd3ss3220,
HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_DEFAULT);
connector = device_get_named_child_node(hd3ss3220->dev, "connector");
- if (IS_ERR(connector))
- return PTR_ERR(connector);
+ if (!connector)
+ return -ENODEV;
hd3ss3220->role_sw = fwnode_usb_role_switch_get(connector);
fwnode_handle_put(connector);
--
2.20.1
[PATCH v2] USB: legousbtower: fix a signedness bug in tower_probe()
The problem is that sizeof() is unsigned long so negative error codes
are type promoted to high positive values and the condition becomes
false.
Fixes: 1d427be4a39d ("USB: legousbtower: fix slab info leak at probe")
Signed-off-by: Dan Carpenter
---
v2: style improvement suggested by Walter Harms.
drivers/usb/misc/legousbtower.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 9d4c52a7ebe0..9bd240df8f4c 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -881,7 +881,7 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
get_version_reply,
sizeof(*get_version_reply),
1000);
- if (result < sizeof(*get_version_reply)) {
+ if (result != sizeof(*get_version_reply)) {
if (result >= 0)
result = -EIO;
dev_err(idev, "get version request failed: %d\n", result);
--
2.20.1
Re: [PATCH] USB: legousbtower: fix a signedness bug in tower_probe()
On Fri, Oct 11, 2019 at 03:51:26PM +0200, walter harms wrote:
>
>
> Am 11.10.2019 15:35, schrieb Dan Carpenter:
> > The problem is that sizeof() is unsigned long so negative error codes
> > are type promoted to high positive values and the condition becomes
> > false.
> >
> > Fixes: 1d427be4a39d ("USB: legousbtower: fix slab info leak at probe")
> > Signed-off-by: Dan Carpenter
> > ---
> > drivers/usb/misc/legousbtower.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/misc/legousbtower.c
> > b/drivers/usb/misc/legousbtower.c
> > index 9d4c52a7ebe0..835908fe1e65 100644
> > --- a/drivers/usb/misc/legousbtower.c
> > +++ b/drivers/usb/misc/legousbtower.c
> > @@ -881,7 +881,7 @@ static int tower_probe (struct usb_interface
> > *interface, const struct usb_device
> > get_version_reply,
> > sizeof(*get_version_reply),
> > 1000);
> > - if (result < sizeof(*get_version_reply)) {
> > + if (result < 0 || result < sizeof(*get_version_reply)) {
> > if (result >= 0)
> > result = -EIO;
> > dev_err(idev, "get version request failed: %d\n", result);
>
> i am not an USB expert but it seems that this is a complicated way
> to check for result != sizeof(*get_version_reply).
Yeah. You're right. That would look nicer. I will resend.
regards,
dan carpenter
[PATCH] usb: typec: fix an IS_ERR() vs NULL bug in hd3ss3220_probe()
The device_get_named_child_node() function doesn't return error
pointers, it returns NULL on error.
Fixes: 1c48c759ef4b ("usb: typec: driver for TI HD3SS3220 USB Type-C DRP port
controller")
Signed-off-by: Dan Carpenter
---
drivers/usb/typec/hd3ss3220.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
index 9715600aeb04..37b08f57aac4 100644
--- a/drivers/usb/typec/hd3ss3220.c
+++ b/drivers/usb/typec/hd3ss3220.c
@@ -172,8 +172,8 @@ static int hd3ss3220_probe(struct i2c_client *client,
hd3ss3220_set_source_pref(hd3ss3220,
HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_DEFAULT);
connector = device_get_named_child_node(hd3ss3220->dev, "connector");
- if (IS_ERR(connector))
- return PTR_ERR(connector);
+ if (!connector)
+ return -EIO;
hd3ss3220->role_sw = fwnode_usb_role_switch_get(connector);
fwnode_handle_put(connector);
--
2.20.1
[PATCH] USB: legousbtower: fix a signedness bug in tower_probe()
The problem is that sizeof() is unsigned long so negative error codes
are type promoted to high positive values and the condition becomes
false.
Fixes: 1d427be4a39d ("USB: legousbtower: fix slab info leak at probe")
Signed-off-by: Dan Carpenter
---
drivers/usb/misc/legousbtower.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 9d4c52a7ebe0..835908fe1e65 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -881,7 +881,7 @@ static int tower_probe (struct usb_interface *interface,
const struct usb_device
get_version_reply,
sizeof(*get_version_reply),
1000);
- if (result < sizeof(*get_version_reply)) {
+ if (result < 0 || result < sizeof(*get_version_reply)) {
if (result >= 0)
result = -EIO;
dev_err(idev, "get version request failed: %d\n", result);
--
2.20.1
[bug report] usb: chipidea: imx: enable vbus and id wakeup only for OTG events
398 ret = PTR_ERR(data->phy);
399 /* Return -EINVAL if no usbphy is available */
400 if (ret == -ENODEV)
401 data->phy = NULL;
402 else
403 goto err_clk;
404 }
405
406 pdata.usb_phy = data->phy;
407
408 if ((of_device_is_compatible(np, "fsl,imx53-usb") ||
409 of_device_is_compatible(np, "fsl,imx51-usb")) &&
pdata.usb_phy &&
410 of_usb_get_phy_mode(np) == USBPHY_INTERFACE_MODE_ULPI) {
411 pdata.flags |= CI_HDRC_OVERRIDE_PHY_CONTROL;
412 data->override_phy_control = true;
413 usb_phy_init(pdata.usb_phy);
414 }
415
416 if (pdata.flags & CI_HDRC_SUPPORTS_RUNTIME_PM)
417 data->supports_runtime_pm = true;
418
419 ret = imx_usbmisc_init(data->usbmisc_data);
420 if (ret) {
421 dev_err(dev, "usbmisc init failed, ret=%d\n", ret);
422 goto err_clk;
423 }
424
425 data->ci_pdev = ci_hdrc_add_device(dev,
426 pdev->resource, pdev->num_resources,
427 &pdata);
428 if (IS_ERR(data->ci_pdev)) {
429 ret = PTR_ERR(data->ci_pdev);
430 if (ret != -EPROBE_DEFER)
431 dev_err(dev, "ci_hdrc_add_device failed,
err=%d\n",
432 ret);
433 goto err_clk;
434 }
435
436 if (!IS_ERR(pdata.id_extcon.edev) ||
437 of_property_read_bool(np, "usb-role-switch"))
438 data->usbmisc_data->ext_id = 1;
^^
439
440 if (!IS_ERR(pdata.vbus_extcon.edev) ||
441 of_property_read_bool(np, "usb-role-switch"))
442 data->usbmisc_data->ext_vbus = 1;
These should probably check for NULL? This driver seems to check pretty
consistently.
443
444 ret = imx_usbmisc_init_post(data->usbmisc_data);
regards,
dan carpenter
[PATCH v2] usb: typec: tcpm: usb: typec: tcpm: Fix a signedness bug in tcpm_fw_get_caps()
The "port->typec_caps.data" and "port->typec_caps.type" variables are
enums and in this context GCC will treat them as an unsigned int so they
can never be less than zero.
Fixes: ae8a2ca8a221 ("usb: typec: Group all TCPCI/TCPM code together")
Signed-off-by: Dan Carpenter
---
v2: preserve the error code
drivers/usb/typec/tcpm/tcpm.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 96562744101c..5f61d9977a15 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -4409,18 +4409,20 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
/* USB data support is optional */
ret = fwnode_property_read_string(fwnode, "data-role", &cap_str);
if (ret == 0) {
- port->typec_caps.data = typec_find_port_data_role(cap_str);
- if (port->typec_caps.data < 0)
- return -EINVAL;
+ ret = typec_find_port_data_role(cap_str);
+ if (ret < 0)
+ return ret;
+ port->typec_caps.data = ret;
}
ret = fwnode_property_read_string(fwnode, "power-role", &cap_str);
if (ret < 0)
return ret;
- port->typec_caps.type = typec_find_port_power_role(cap_str);
- if (port->typec_caps.type < 0)
- return -EINVAL;
+ ret = typec_find_port_power_role(cap_str);
+ if (ret < 0)
+ return ret;
+ port->typec_caps.type = ret;
port->port_type = port->typec_caps.type;
if (port->port_type == TYPEC_PORT_SNK)
--
2.20.1
[bug report] usb: cdns3: Add Cadence USB3 DRD Driver
Hello Pawel Laszczak,
The patch 7733f6c32e36: "usb: cdns3: Add Cadence USB3 DRD Driver"
from Aug 26, 2019, leads to the following static checker warning:
drivers/usb/cdns3/ep0.c:335 cdns3_ep0_feature_handle_device()
error: scheduling with locks held: 'spin_lock:lock'
drivers/usb/cdns3/ep0.c
309
310 priv_dev->u2_allowed = !!set;
311 break;
312 case USB_DEVICE_LTM_ENABLE:
313 ret = -EINVAL;
314 break;
315 case USB_DEVICE_TEST_MODE:
316 if (state != USB_STATE_CONFIGURED || speed >
USB_SPEED_HIGH)
317 return -EINVAL;
318
319 tmode = le16_to_cpu(ctrl->wIndex);
320
321 if (!set || (tmode & 0xff) != 0)
322 return -EINVAL;
323
324 switch (tmode >> 8) {
325 case TEST_J:
326 case TEST_K:
327 case TEST_SE0_NAK:
328 case TEST_PACKET:
329 cdns3_ep0_complete_setup(priv_dev, 0, 1);
330 /**
331 * Little delay to give the controller some
time
332 * for sending status stage.
333 * This time should be less then 3ms.
334 */
335 usleep_range(1000, 2000);
This is called with locks held and IRQs disabled in
cdns3_device_thread_irq_handler(). The call tree is:
cdns3_device_thread_irq_handler()
cdns3_check_ep0_interrupt_proceed()
cdns3_ep0_setup_phase()
cdns3_ep0_standard_request()
cdns3_req_ep0_handle_feature()
cdns3_ep0_feature_handle_device()
You would need to have certain debug config options enabled to see the
might_sleep() warnings and probably USB_DEVICE_TEST_MODE isn't used
very often.
336 cdns3_set_register_bit(&priv_dev->regs->usb_cmd,
337 USB_CMD_STMODE |
338 USB_STS_TMODE_SEL(tmode
- 1));
339 break;
340 default:
341 ret = -EINVAL;
342 }
343 break;
344 default:
345 ret = -EINVAL;
346 }
regards,
dan carpenter
[PATCH] usb: cdns3: Fix use after free in probe error handling
We can't use "wrap" after it has been freed.
Fixes: 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")
Signed-off-by: Dan Carpenter
---
drivers/usb/cdns3/cdns3-pci-wrap.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/cdns3/cdns3-pci-wrap.c
b/drivers/usb/cdns3/cdns3-pci-wrap.c
index c41ddb61b857..b0a29efe7d31 100644
--- a/drivers/usb/cdns3/cdns3-pci-wrap.c
+++ b/drivers/usb/cdns3/cdns3-pci-wrap.c
@@ -159,8 +159,9 @@ static int cdns3_pci_probe(struct pci_dev *pdev,
wrap->plat_dev = platform_device_register_full(&plat_info);
if (IS_ERR(wrap->plat_dev)) {
pci_disable_device(pdev);
+ err = PTR_ERR(wrap->plat_dev);
kfree(wrap);
- return PTR_ERR(wrap->plat_dev);
+ return err;
}
}
--
2.20.1
[PATCH] usb: host: ohci-pxa27x: Fix and & vs | typo
The code is supposed to clear the RH_A_NPS and RH_A_PSM bits, but it's a no-op because of the & vs | typo. This bug predates git and it was only discovered using static analysis so it must not affect too many people in real life. Signed-off-by: Dan Carpenter Acked-by: Alan Stern --- drivers/usb/host/ohci-pxa27x.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/host/ohci-pxa27x.c b/drivers/usb/host/ohci-pxa27x.c index 3e2474959735..7679fb583e41 100644 --- a/drivers/usb/host/ohci-pxa27x.c +++ b/drivers/usb/host/ohci-pxa27x.c @@ -148,7 +148,7 @@ static int pxa27x_ohci_select_pmm(struct pxa27x_ohci *pxa_ohci, int mode) uhcrhda |= RH_A_NPS; break; case PMM_GLOBAL_MODE: - uhcrhda &= ~(RH_A_NPS & RH_A_PSM); + uhcrhda &= ~(RH_A_NPS | RH_A_PSM); break; case PMM_PERPORT_MODE: uhcrhda &= ~(RH_A_NPS); -- 2.20.1
Re: [PATCH] usb: host: ohci-pxa27x: Fix and & vs | typo
I was looking at this code again today and I'm still convinced this patch is correct. Should I resend? regards, dan carpenter On Fri, Feb 23, 2018 at 03:33:00PM +0300, Dan Carpenter wrote: > The code is supposed to clear the RH_A_NPS and RH_A_PSM bits, but it's > a no-op because of the & vs | typo. This bug predates git and it was > only discovered using static analysis so it must not affect too many > people in real life. > > Signed-off-by: Dan Carpenter > --- > Not tested. > > diff --git a/drivers/usb/host/ohci-pxa27x.c b/drivers/usb/host/ohci-pxa27x.c > index 3e2474959735..7679fb583e41 100644 > --- a/drivers/usb/host/ohci-pxa27x.c > +++ b/drivers/usb/host/ohci-pxa27x.c > @@ -148,7 +148,7 @@ static int pxa27x_ohci_select_pmm(struct pxa27x_ohci > *pxa_ohci, int mode) > uhcrhda |= RH_A_NPS; > break; > case PMM_GLOBAL_MODE: > - uhcrhda &= ~(RH_A_NPS & RH_A_PSM); > + uhcrhda &= ~(RH_A_NPS | RH_A_PSM); > break; > case PMM_PERPORT_MODE: > uhcrhda &= ~(RH_A_NPS); > -- > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [bug report] usb: gadget: u_serial: process RX in workqueue instead of tasklet
On Mon, Jun 24, 2019 at 03:41:54PM +0200, mirq-li...@rere.qmqm.pl wrote: > On Mon, Jun 24, 2019 at 03:32:58PM +0300, Dan Carpenter wrote: > > Hello Michał Mirosław, > > > > This is a semi-automatic email about new static checker warnings. > > > > The patch 8b4c62aef6f6: "usb: gadget: u_serial: process RX in > > workqueue instead of tasklet" from Dec 16, 2018, leads to the > > following Smatch complaint: > [...] > >431 /* We want our data queue to become empty ASAP, keeping > > data > >432 * in the tty and ldisc (not here). If we couldn't > > push any > >433 * this time around, RX may be starved, so wait until > > next jiffy. > >434 * > >435 * We may leave non-empty queue only when there is a > > tty, and > >436 * either it is throttled or there is no more room in > > flip buffer. > >437 */ > >438 if (!list_empty(queue) && !tty_throttled(tty)) > > ^^^ > > in the original code there was check for NULL here but the patch removed > > it. > > > >439 schedule_delayed_work(&port->push, 1); > >440 > > Hi Dan, > > The code is correct and explained in the comment above it - while() loop > above can be exited before emptying the queue only when tty != NULL. > :( Sorry... Smatch isn't supposed to generate those warnings. I don't know exactly what happened. I thought it was because of some recent code changes I had made but now I think it it's because my build system had a disk failure... Anyway, I can't reproduce the warning any more. Thanks for looking at this. regards, dan carpenter
[bug report] usb: gadget: u_serial: process RX in workqueue instead of tasklet
Hello Michał Mirosław,
This is a semi-automatic email about new static checker warnings.
The patch 8b4c62aef6f6: "usb: gadget: u_serial: process RX in
workqueue instead of tasklet" from Dec 16, 2018, leads to the
following Smatch complaint:
drivers/usb/gadget/function/u_serial.c:438 gs_rx_push()
error: we previously assumed 'tty' could be null (see line 373)
drivers/usb/gadget/function/u_serial.c
372 /* leave data queued if tty was rx throttled */
373 if (tty && tty_throttled(tty))
Other checks for NULL
374 break;
375
376 switch (req->status) {
377 case -ESHUTDOWN:
378 disconnect = true;
379 pr_vdebug("ttyGS%d: shutdown\n",
port->port_num);
380 break;
381
382 default:
383 /* presumably a transient fault */
384 pr_warn("ttyGS%d: unexpected RX status %d\n",
385 port->port_num, req->status);
386 /* FALLTHROUGH */
387 case 0:
388 /* normal completion */
389 break;
390 }
391
392 /* push data to (open) tty */
393 if (req->actual && tty) {
394 char*packet = req->buf;
395 unsignedsize = req->actual;
396 unsignedn;
397 int count;
398
399 /* we may have pushed part of this packet
already... */
400 n = port->n_read;
401 if (n) {
402 packet += n;
403 size -= n;
404 }
405
406 count = tty_insert_flip_string(&port->port,
packet,
407 size);
408 if (count)
409 do_push = true;
410 if (count != size) {
411 /* stop pushing; TTY layer can't handle
more */
412 port->n_read += count;
413 pr_vdebug("ttyGS%d: rx block %d/%d\n",
414port->port_num, count,
req->actual);
415 break;
416 }
417 port->n_read = 0;
418 }
419
420 list_move(&req->list, &port->read_pool);
421 port->read_started--;
422 }
423
424 /* Push from tty to ldisc; this is handled by a workqueue,
425 * so we won't get callbacks and can hold port_lock
426 */
427 if (do_push)
428 tty_flip_buffer_push(&port->port);
429
430
431 /* We want our data queue to become empty ASAP, keeping data
432 * in the tty and ldisc (not here). If we couldn't push any
433 * this time around, RX may be starved, so wait until next
jiffy.
434 *
435 * We may leave non-empty queue only when there is a tty, and
436 * either it is throttled or there is no more room in flip
buffer.
437 */
438 if (!list_empty(queue) && !tty_throttled(tty))
^^^
in the original code there was check for NULL here but the patch removed
it.
439 schedule_delayed_work(&port->push, 1);
440
regards,
dan carpenter
[bug report] usb: gadget: f_fs: OS descriptors support
Hello Andrzej Pietrasiewicz,
The patch f0175ab51993: "usb: gadget: f_fs: OS descriptors support"
from Jul 9, 2014, leads to the following static checker warning:
drivers/usb/gadget/function/f_fs.c:2992 __ffs_func_bind_do_os_desc()
error: 'ext_prop->data_len' from user is not capped properly
drivers/usb/gadget/function/f_fs.c
2961 ARRAY_SIZE(desc->CompatibleID) +
2962 ARRAY_SIZE(desc->SubCompatibleID));
2963 length = sizeof(*desc);
2964 }
2965 break;
2966 case FFS_OS_DESC_EXT_PROP: {
2967 struct usb_ext_prop_desc *desc = data;
2968 struct usb_os_desc_table *t;
2969 struct usb_os_desc_ext_prop *ext_prop;
2970 char *ext_prop_name;
2971 char *ext_prop_data;
2972
2973 t = &func->function.os_desc_table[h->interface];
2974 t->if_id = func->interfaces_nums[h->interface];
2975
2976 ext_prop = func->ffs->ms_os_descs_ext_prop_avail;
2977 func->ffs->ms_os_descs_ext_prop_avail +=
sizeof(*ext_prop);
2978
2979 ext_prop->type = le32_to_cpu(desc->dwPropertyDataType);
2980 ext_prop->name_len =
le16_to_cpu(desc->wPropertyNameLength);
2981 ext_prop->data_len = le32_to_cpu(*(__le32 *)
2982 usb_ext_prop_data_len_ptr(data,
ext_prop->name_len));
Smatch is very suspicious of "ext_prop->data_len".
2983 length = ext_prop->name_len + ext_prop->data_len + 14;
2984
2985 ext_prop_name =
func->ffs->ms_os_descs_ext_prop_name_avail;
2986 func->ffs->ms_os_descs_ext_prop_name_avail +=
2987 ext_prop->name_len;
2988
2989 ext_prop_data =
func->ffs->ms_os_descs_ext_prop_data_avail;
2990 func->ffs->ms_os_descs_ext_prop_data_avail +=
2991 ext_prop->data_len;
2992 memcpy(ext_prop_data,
2993 usb_ext_prop_data_ptr(data, ext_prop->name_len),
2994 ext_prop->data_len);
^^
so it complians that this memcpy() can overflow. That seems like maybe
a real issue?
2995 /* unicode data reported to the host as "WCHAR"s */
2996 switch (ext_prop->type) {
2997 case USB_EXT_PROP_UNICODE:
2998 case USB_EXT_PROP_UNICODE_ENV:
2999 case USB_EXT_PROP_UNICODE_LINK:
3000 case USB_EXT_PROP_UNICODE_MULTI:
3001 ext_prop->data_len *= 2;
3002 break;
3003 }
3004 ext_prop->data = ext_prop_data;
regards,
dan carpenter
[PATCH] usb/hcd: Fix a NULL vs IS_ERR() bug in usb_hcd_setup_local_mem()
The devm_memremap() function doesn't return NULL, it returns error
pointers.
Fixes: b0310c2f09bb ("USB: use genalloc for USB HCs with local memory")
Signed-off-by: Dan Carpenter
---
drivers/usb/core/hcd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index fe631d18c1ed..df8f358685e6 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -3052,8 +3052,8 @@ int usb_hcd_setup_local_mem(struct usb_hcd *hcd,
phys_addr_t phys_addr,
local_mem = devm_memremap(hcd->self.sysdev, phys_addr,
size, MEMREMAP_WC);
- if (!local_mem)
- return -ENOMEM;
+ if (IS_ERR(local_mem))
+ return PTR_ERR(local_mem);
/*
* Here we pass a dma_addr_t but the arg type is a phys_addr_t.
--
2.20.1
[PATCH] usbip: stub_rx: tidy the indenting in is_clear_halt_cmd()
There is an extra space character before the return statement. Signed-off-by: Dan Carpenter diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c index 97b09a42a10c..f3230bed18af 100644 --- a/drivers/usb/usbip/stub_rx.c +++ b/drivers/usb/usbip/stub_rx.c @@ -17,9 +17,9 @@ static int is_clear_halt_cmd(struct urb *urb) req = (struct usb_ctrlrequest *) urb->setup_packet; -return (req->bRequest == USB_REQ_CLEAR_FEATURE) && -(req->bRequestType == USB_RECIP_ENDPOINT) && -(req->wValue == USB_ENDPOINT_HALT); + return (req->bRequest == USB_REQ_CLEAR_FEATURE) && + (req->bRequestType == USB_RECIP_ENDPOINT) && + (req->wValue == USB_ENDPOINT_HALT); } static int is_set_interface_cmd(struct urb *urb)
[bug report] usb: chipidea: imx: add HSIC support
Hello Peter Chen,
The patch 7c8e8909417e: "usb: chipidea: imx: add HSIC support" from
Oct 16, 2018, leads to the following static checker warning:
drivers/usb/chipidea/ci_hdrc_imx.c:321 ci_hdrc_imx_probe()
warn: 'data->usbmisc_data' can also be NULL
drivers/usb/chipidea/ci_hdrc_imx.c
309
310 data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
311 if (!data)
312 return -ENOMEM;
313
314 platform_set_drvdata(pdev, data);
315 data->usbmisc_data = usbmisc_get_init_data(dev);
^^^
This can be error pointers on error or NULL if there is no misc data.
316 if (IS_ERR(data->usbmisc_data))
317 return PTR_ERR(data->usbmisc_data);
318
319 if (of_usb_get_phy_mode(dev->of_node) ==
USBPHY_INTERFACE_MODE_HSIC) {
320 pdata.flags |= CI_HDRC_IMX_IS_HSIC;
321 data->usbmisc_data->hsic = 1;
Smatch thinks this can be NULL. Smatch doesn't understand enough of the
context here so possibly this is a false positive.
322 data->pinctrl = devm_pinctrl_get(dev);
323 if (IS_ERR(data->pinctrl)) {
324 dev_err(dev, "pinctrl get failed, err=%ld\n",
325 PTR_ERR(data->pinctrl));
326 return PTR_ERR(data->pinctrl);
327 }
regards,
dan carpenter
[PATCH] usb: gadget: Potential NULL dereference on allocation error
The source_sink_alloc_func() function is supposed to return error pointers on error. The function is called from usb_get_function() which doesn't check for NULL returns so it would result in an Oops. Of course, in the current kernel, small allocations always succeed so this doesn't affect runtime. Signed-off-by: Dan Carpenter --- drivers/usb/gadget/function/f_sourcesink.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/gadget/function/f_sourcesink.c b/drivers/usb/gadget/function/f_sourcesink.c index 9cdef108fb1b..ed68a4860b7d 100644 --- a/drivers/usb/gadget/function/f_sourcesink.c +++ b/drivers/usb/gadget/function/f_sourcesink.c @@ -838,7 +838,7 @@ static struct usb_function *source_sink_alloc_func( ss = kzalloc(sizeof(*ss), GFP_KERNEL); if (!ss) - return NULL; + return ERR_PTR(-ENOMEM); ss_opts = container_of(fi, struct f_ss_opts, func_inst); -- 2.17.1
[bug report] staging: typec: Fairchild FUSB302 Type-c chip driver
Hello Yueyao Zhu,
The patch c034a43e72dd: "staging: typec: Fairchild FUSB302 Type-c
chip driver" from Apr 27, 2017, leads to the following static checker
warning:
drivers/usb/typec/tcpm/fusb302.c:158 _fusb302_log()
error: testing array offset 'chip->logbuffer_head' after use.
drivers/usb/typec/tcpm/fusb302.c
141 if (!chip->logbuffer[chip->logbuffer_head]) {
We use ->logbuffer_head here.
142 chip->logbuffer[chip->logbuffer_head] =
143 kzalloc(LOG_BUFFER_ENTRY_SIZE,
GFP_KERNEL);
144 if (!chip->logbuffer[chip->logbuffer_head])
145 return;
146 }
147
148 vsnprintf(tmpbuffer, sizeof(tmpbuffer), fmt, args);
149
150 mutex_lock(&chip->logbuffer_lock);
151
152 if (fusb302_log_full(chip)) {
153 chip->logbuffer_head = max(chip->logbuffer_head - 1, 0);
^^^
set here
154 strlcpy(tmpbuffer, "overflow", sizeof(tmpbuffer));
155 }
156
157 if (chip->logbuffer_head < 0 ||
158 chip->logbuffer_head >= LOG_BUFFER_ENTRIES) {
^^
But we don't check the it's valid until here.
159 dev_warn(chip->dev,
160 "Bad log buffer index %d\n",
chip->logbuffer_head);
161 goto abort;
162 }
163
164 if (!chip->logbuffer[chip->logbuffer_head]) {
165 dev_warn(chip->dev,
166 "Log buffer index %d is NULL\n",
chip->logbuffer_head);
167 goto abort;
168 }
169
170 rem_nsec = do_div(ts_nsec, 10);
171 scnprintf(chip->logbuffer[chip->logbuffer_head],
172LOG_BUFFER_ENTRY_SIZE, "[%5lu.%06lu] %s",
173(unsigned long)ts_nsec, rem_nsec / 1000,
174tmpbuffer);
175 chip->logbuffer_head = (chip->logbuffer_head + 1) %
LOG_BUFFER_ENTRIES;
^^
Mostly set here. As you can see it's always set to a valid value, but
it looks like there should be some locking around ->logbuffer_head and
->logbuffer_tail overwise it could easily be corrupted because multiple
thread log stuff.
So I don't really want to remove the checks because they do narrow the
window where a race would have to occur before we would get memory
corruption. But ideally we would just fix the race.
regards,
dan carpenter
Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect"
On Fri, Dec 07, 2018 at 02:13:18PM +, Minas Harutyunyan wrote: > > My patch doesn't pass sparse checking: "warning: context imbalance in > 'dwc2_hsotg_core_init_disconnected' - unexpected unlock". Sparse persist! > So, I need to re-work patch. Can I use your idea with > dwc2_hsotg_ep_disable_lock() function to prepare new one? > Sparse lock checking is pretty crap, and I wouldn't go out of my way normally to make it happy... But of course you can use my idea. regards, dan carpenter
Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect"
On Wed, Dec 05, 2018 at 12:52:22PM +, Minas Harutyunyan wrote:
> Hi,
>
> On 12/4/2018 5:29 PM, Dan Carpenter wrote:
> > On Tue, Dec 04, 2018 at 12:34:08PM +, Minas Harutyunyan wrote:
> >> @@ -3185,12 +3183,13 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg
> >> *hsotg)
> >> hsotg->connected = 0;
> >> hsotg->test_mode = 0;
> >>
> >> - /* all endpoints should be shutdown */
> >> for (ep = 0; ep < hsotg->num_of_eps; ep++) {
> >> if (hsotg->eps_in[ep])
> >> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> >> + kill_all_requests(hsotg, hsotg->eps_in[ep],
> >> + -ESHUTDOWN);
> >> if (hsotg->eps_out[ep])
> >> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> >> + kill_all_requests(hsotg, hsotg->eps_out[ep],
> >> + -ESHUTDOWN);
> >
> >
> > Should this part be in a separate patch?
> >
> > I'm not trying to be rhetorical at all. I literally don't know the
> > code very well. Hopefully the full commit message will explain it.
> >
>
> Actually, this fragment of patch revert changes from V2 and keep
> untouched dwc2_hsotg_disconnect() function.
>
To me it feels like there are two issues. The first is this change, and
the second is fixing the lockdep warning.
> >> }
> >>
> >> call_gadget(hsotg, disconnect);
> >> @@ -3234,6 +3233,8 @@ static void dwc2_hsotg_irq_fifoempty(struct
> >> dwc2_hsotg *hsotg, bool periodic)
> >> GINTSTS_PTXFEMP | \
> >> GINTSTS_RXFLVL)
> >>
> >> +static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
> >> +
> >>/**
> >> * dwc2_hsotg_core_init - issue softreset to the core
> >> * @hsotg: The device state
> >> @@ -3258,12 +3259,14 @@ void dwc2_hsotg_core_init_disconnected(struct
> >> dwc2_hsotg *hsotg,
> >> return;
> >> } else {
> >> /* all endpoints should be shutdown */
> >> + spin_unlock(&hsotg->lock);
> >> for (ep = 1; ep < hsotg->num_of_eps; ep++) {
> >> if (hsotg->eps_in[ep])
> >>
> >> dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> >> if (hsotg->eps_out[ep])
> >>
> >> dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> >> }
> >> + spin_lock(&hsotg->lock);
> >> }
> >>
> >> /*
> >
> > The idea here is that this is the only caller which is holding the
> > lock and we drop it here and take it again inside dwc2_hsotg_ep_disable().
> > I don't know the code very well and can't totally swear that this
> > doesn't introduce a small race condition...
> >
> Above fragment of patch allow to keep untouched dwc2_hsotg_ep_disble()
> function also, without changing spin_lock/_unlock stuff inside function.
>
> My approach here minimally update code to add any races. Just in
> dwc2_hsotg_core_init_disconnected() function on USB reset interrupt
> perform disabling all EP's. Because on USB reset interrupt, called from
> interrupt
> handler with acquired lock and dwc2_hsotg_ep_disble() function (without
> changes) acquire lock, just need to unlock lock to avoid any troubles.
>
Yes. I understand that. I just don't like it.
Although your patch is more "minimal" in that it touches fewer lines of
code it's actually more complicated because we have to verify that it's
safe to drop the lock.
> > Another option would be to introduce a new function which takes the lock
> > and change all the other callers instead. To me that would be easier to
> > review... See below for how it might look:
> >
> > regards,
> > dan carpenter
> >
> > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> > index 94f3ba995580..b17a5dbefd5f 100644
> > --- a/drivers/usb/dwc2/gadget.c
> > +++ b/drivers/usb/dwc2/gadget.c
> > @@ -3166,6 +3166,7 @@ static void kill_all_requests(struct dwc2_hsotg
> > *hsotg,
> > }
> >
> > static int dwc2_hsotg_ep_disable(struct
Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect"
Hi Marek, I'm surprised you don't get deadlocks when you apply this patch. On Wed, Nov 21, 2018 at 04:45:04PM +0100, Marek Szyprowski wrote: > @@ -4020,9 +4008,7 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep) > > epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index); > > - locked = spin_is_locked(&hsotg->lock); > - if (!locked) > - spin_lock_irqsave(&hsotg->lock, flags); > + spin_lock_irqsave(&hsotg->lock, flags); > One of the callers is already holding the hsotg->log. The spin_is_locked() test would avoid the deadlock. regards, dan carpenter
Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect"
On Tue, Dec 04, 2018 at 12:34:08PM +, Minas Harutyunyan wrote:
> @@ -3185,12 +3183,13 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg)
> hsotg->connected = 0;
> hsotg->test_mode = 0;
>
> - /* all endpoints should be shutdown */
> for (ep = 0; ep < hsotg->num_of_eps; ep++) {
> if (hsotg->eps_in[ep])
> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> + kill_all_requests(hsotg, hsotg->eps_in[ep],
> + -ESHUTDOWN);
> if (hsotg->eps_out[ep])
> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> + kill_all_requests(hsotg, hsotg->eps_out[ep],
> + -ESHUTDOWN);
Should this part be in a separate patch?
I'm not trying to be rhetorical at all. I literally don't know the
code very well. Hopefully the full commit message will explain it.
> }
>
> call_gadget(hsotg, disconnect);
> @@ -3234,6 +3233,8 @@ static void dwc2_hsotg_irq_fifoempty(struct
> dwc2_hsotg *hsotg, bool periodic)
> GINTSTS_PTXFEMP | \
> GINTSTS_RXFLVL)
>
> +static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
> +
> /**
>* dwc2_hsotg_core_init - issue softreset to the core
>* @hsotg: The device state
> @@ -3258,12 +3259,14 @@ void dwc2_hsotg_core_init_disconnected(struct
> dwc2_hsotg *hsotg,
> return;
> } else {
> /* all endpoints should be shutdown */
> + spin_unlock(&hsotg->lock);
> for (ep = 1; ep < hsotg->num_of_eps; ep++) {
> if (hsotg->eps_in[ep])
>
> dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> if (hsotg->eps_out[ep])
>
> dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> }
> + spin_lock(&hsotg->lock);
> }
>
> /*
The idea here is that this is the only caller which is holding the
lock and we drop it here and take it again inside dwc2_hsotg_ep_disable().
I don't know the code very well and can't totally swear that this
doesn't introduce a small race condition...
Another option would be to introduce a new function which takes the lock
and change all the other callers instead. To me that would be easier to
review... See below for how it might look:
regards,
dan carpenter
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 94f3ba995580..b17a5dbefd5f 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -3166,6 +3166,7 @@ static void kill_all_requests(struct dwc2_hsotg *hsotg,
}
static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
+static int dwc2_hsotg_ep_disable_lock(struct usb_ep *ep);
/**
* dwc2_hsotg_disconnect - disconnect service
@@ -3188,9 +3189,9 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg)
/* all endpoints should be shutdown */
for (ep = 0; ep < hsotg->num_of_eps; ep++) {
if (hsotg->eps_in[ep])
- dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
+ dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
if (hsotg->eps_out[ep])
- dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
+ dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
}
call_gadget(hsotg, disconnect);
@@ -4069,10 +4070,8 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
struct dwc2_hsotg *hsotg = hs_ep->parent;
int dir_in = hs_ep->dir_in;
int index = hs_ep->index;
- unsigned long flags;
u32 epctrl_reg;
u32 ctrl;
- int locked;
dev_dbg(hsotg->dev, "%s(ep %p)\n", __func__, ep);
@@ -4088,10 +4087,6 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index);
- locked = spin_is_locked(&hsotg->lock);
- if (!locked)
- spin_lock_irqsave(&hsotg->lock, flags);
-
ctrl = dwc2_readl(hsotg, epctrl_reg);
if (ctrl & DXEPCTL_EPENA)
@@ -4114,12 +4109,23 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
hs_ep->fifo_index = 0;
hs_ep->fifo_size = 0;
- if (!locked)
- spin_unlock_irqrestore(&hsotg->lock, flags);
-
return 0;
}
+static int dwc2_hsotg_ep_disable_lock(struct usb_ep *ep)
+{
+ struct dwc2_hsotg_ep *hs_ep = our_ep(ep);
+ struct dwc2_hsotg *hsotg =
Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect"
Ugh... We also had a long thread about the v2 patch but it turns out the list was not CC'd. I should have checked for that. You have to pass a flag to say if the caller holds the lock or not... regards, dan carpenter
[PATCH 1/2] usb: phy: ab8500: silence some uninitialized variable warnings
Smatch complains that "reg" can be uninitialized if the
abx500_get_register_interruptible() call fails. It's an interruptable
function, so we should check if the user presses CTRL-C.
Signed-off-by: Dan Carpenter
---
drivers/usb/phy/phy-ab8500-usb.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/phy/phy-ab8500-usb.c b/drivers/usb/phy/phy-ab8500-usb.c
index 66143ab8c043..aaf363f19714 100644
--- a/drivers/usb/phy/phy-ab8500-usb.c
+++ b/drivers/usb/phy/phy-ab8500-usb.c
@@ -505,15 +505,19 @@ static int abx500_usb_link_status_update(struct
ab8500_usb *ab)
if (is_ab8500(ab->ab8500)) {
enum ab8500_usb_link_status lsts;
- abx500_get_register_interruptible(ab->dev,
+ ret = abx500_get_register_interruptible(ab->dev,
AB8500_USB, AB8500_USB_LINE_STAT_REG, ®);
+ if (ret < 0)
+ return ret;
lsts = (reg >> 3) & 0x0F;
ret = ab8500_usb_link_status_update(ab, lsts);
} else if (is_ab8505(ab->ab8500)) {
enum ab8505_usb_link_status lsts;
- abx500_get_register_interruptible(ab->dev,
+ ret = abx500_get_register_interruptible(ab->dev,
AB8500_USB, AB8505_USB_LINE_STAT_REG, ®);
+ if (ret < 0)
+ return ret;
lsts = (reg >> 3) & 0x1F;
ret = ab8505_usb_link_status_update(ab, lsts);
}
--
2.11.0
[PATCH] usb: dwc2: pci: Fix an error code in probe
We added some error handling to this function but forgot to set the
error code on this path.
Fixes: ecd29dabb2ba ("usb: dwc2: pci: Handle error cleanup in probe")
Signed-off-by: Dan Carpenter
---
drivers/usb/dwc2/pci.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/dwc2/pci.c b/drivers/usb/dwc2/pci.c
index d257c541e51b..7afc10872f1f 100644
--- a/drivers/usb/dwc2/pci.c
+++ b/drivers/usb/dwc2/pci.c
@@ -120,6 +120,7 @@ static int dwc2_pci_probe(struct pci_dev *pci,
dwc2 = platform_device_alloc("dwc2", PLATFORM_DEVID_AUTO);
if (!dwc2) {
dev_err(dev, "couldn't allocate dwc2 device\n");
+ ret = -ENOMEM;
goto err;
}
--
2.11.0
Re: [PATCH -next] USB: cypress_m8: remove set but not used variables 'actual_size, iflag'
On Sun, Sep 30, 2018 at 06:02:24PM +0200, Johan Hovold wrote:
> On Sat, Sep 29, 2018 at 09:54:03AM +, YueHaibing wrote:
> > Fixes gcc '-Wunused-but-set-variable' warning:
> >
> > drivers/usb/serial/cypress_m8.c: In function 'cypress_send':
> > drivers/usb/serial/cypress_m8.c:689:33: warning:
> > variable 'actual_size' set but not used [-Wunused-but-set-variable]
> >
> > drivers/usb/serial/cypress_m8.c: In function 'cypress_set_termios':
> > drivers/usb/serial/cypress_m8.c:866:18: warning:
> > variable 'iflag' set but not used [-Wunused-but-set-variable]
> >
> > Signed-off-by: YueHaibing
> > ---
> > drivers/usb/serial/cypress_m8.c | 11 ++-
> > 1 file changed, 2 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/usb/serial/cypress_m8.c
> > b/drivers/usb/serial/cypress_m8.c
> > index 31c6091..98dff12 100644
> > --- a/drivers/usb/serial/cypress_m8.c
> > +++ b/drivers/usb/serial/cypress_m8.c
> > @@ -686,7 +686,7 @@ static int cypress_write(struct tty_struct *tty, struct
> > usb_serial_port *port,
> >
> > static void cypress_send(struct usb_serial_port *port)
> > {
> > - int count = 0, result, offset, actual_size;
> > + int count = 0, result, offset;
> > struct cypress_private *priv = usb_get_serial_port_data(port);
> > struct device *dev = &port->dev;
> > unsigned long flags;
> > @@ -758,12 +758,6 @@ static void cypress_send(struct usb_serial_port *port)
> > priv->write_urb_in_use = 1;
> > spin_unlock_irqrestore(&priv->lock, flags);
> >
> > - if (priv->cmd_ctrl)
> > - actual_size = 1;
> > - else
> > - actual_size = count +
> > - (priv->pkt_fmt == packet_format_1 ? 2 : 1);
> > -
>
> This looks like we have a bug in the driver, and sure enough we do.
> Commit 9aa8dae7b1fa ("cypress_m8: use usb_fill_int_urb where
> appropriate") incorrectly started setting the transfer length to the
> size of the buffer.
>
This is like the third time recently where we've had found a real bug.
YueHaibing, could you mention in the commit message where the variable
became unused? It would help reviewing. It's not a Fixes tag but it
would look like this:
"After commit 9aa8dae7b1fa ("cypress_m8: use usb_fill_int_urb where
appropriate") then we don't use actual_size any more so that can
be removed."
regards,
dan carpenter
[PATCH] ALSA: usb-audio: ensure that cluster header size is enough
I think we need a check to make sure that "hc_header.wLength" is large
enough for a struct struct uac3_cluster_header_descriptor.
Fixes: 9a2fe9b801f5 ("ALSA: usb: initial USB Audio Device Class 3.0 support")
Signed-off-by: Dan Carpenter
diff --git a/sound/usb/stream.c b/sound/usb/stream.c
index 67cf849aa16b..f7179ce39a19 100644
--- a/sound/usb/stream.c
+++ b/sound/usb/stream.c
@@ -968,6 +968,12 @@ snd_usb_get_audioformat_uac3(struct snd_usb_audio *chip,
* and request Cluster Descriptor
*/
wLength = le16_to_cpu(hc_header.wLength);
+ if (wLength < sizeof(*cluster)) {
+ dev_err(&dev->dev,
+ "%u:%d : cluster header size too small %d\n",
+ iface_no, altno, wLength);
+ return ERR_PTR(-EIO);
+ }
cluster = kzalloc(wLength, GFP_KERNEL);
if (!cluster)
return ERR_PTR(-ENOMEM);
[usb:usb-testing 34/67] drivers/usb/class/usbtmc.c:339 usbtmc_ioctl_abort_bulk_in_tag() error: uninitialized symbol 'actual'.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
usb-testing
head: ae8a2ca8a2215c7e31e6d874f7303801bb15fbbc
commit: cbe743f1333b23040d1312afd58224dbd58fcc25 [34/67] usb: usbtmc: Fix ioctl
USBTMC_IOCTL_ABORT_BULK_IN
New smatch warnings:
drivers/usb/class/usbtmc.c:339 usbtmc_ioctl_abort_bulk_in_tag() error:
uninitialized symbol 'actual'.
Old smatch warnings:
drivers/usb/class/usbtmc.c:1975 usbtmc_ioctl_request() warn: possible memory
leak of 'buffer'
drivers/usb/class/usbtmc.c:1978 usbtmc_ioctl_request() warn: overwrite may leak
'buffer'
#
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/?id=cbe743f1333b23040d1312afd58224dbd58fcc25
git remote add usb
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
git remote update usb
git checkout cbe743f1333b23040d1312afd58224dbd58fcc25
vim +/actual +339 drivers/usb/class/usbtmc.c
5b775f67 Greg Kroah-Hartman 2008-08-26 272
cbe743f1 Guido Kiener 2018-09-12 273 static int
usbtmc_ioctl_abort_bulk_in_tag(struct usbtmc_device_data *data,
cbe743f1 Guido Kiener 2018-09-12 274
u8 tag)
5b775f67 Greg Kroah-Hartman 2008-08-26 275 {
b361a6e3 Chris Malley 2008-10-25 276 u8 *buffer;
5b775f67 Greg Kroah-Hartman 2008-08-26 277 struct device *dev;
5b775f67 Greg Kroah-Hartman 2008-08-26 278 int rv;
5b775f67 Greg Kroah-Hartman 2008-08-26 279 int n;
5b775f67 Greg Kroah-Hartman 2008-08-26 280 int actual;
5b775f67 Greg Kroah-Hartman 2008-08-26 281
5b775f67 Greg Kroah-Hartman 2008-08-26 282 dev = &data->intf->dev;
cbe743f1 Guido Kiener 2018-09-12 283 buffer =
kmalloc(USBTMC_BUFSIZE, GFP_KERNEL);
5b775f67 Greg Kroah-Hartman 2008-08-26 284 if (!buffer)
5b775f67 Greg Kroah-Hartman 2008-08-26 285 return -ENOMEM;
5b775f67 Greg Kroah-Hartman 2008-08-26 286
5b775f67 Greg Kroah-Hartman 2008-08-26 287 rv =
usb_control_msg(data->usb_dev,
5b775f67 Greg Kroah-Hartman 2008-08-26 288
usb_rcvctrlpipe(data->usb_dev, 0),
5b775f67 Greg Kroah-Hartman 2008-08-26 289
USBTMC_REQUEST_INITIATE_ABORT_BULK_IN,
5b775f67 Greg Kroah-Hartman 2008-08-26 290 USB_DIR_IN
| USB_TYPE_CLASS | USB_RECIP_ENDPOINT,
cbe743f1 Guido Kiener 2018-09-12 291 tag,
data->bulk_in,
cbe743f1 Guido Kiener 2018-09-12 292 buffer, 2,
USB_CTRL_GET_TIMEOUT);
5b775f67 Greg Kroah-Hartman 2008-08-26 293
5b775f67 Greg Kroah-Hartman 2008-08-26 294 if (rv < 0) {
5b775f67 Greg Kroah-Hartman 2008-08-26 295 dev_err(dev,
"usb_control_msg returned %d\n", rv);
5b775f67 Greg Kroah-Hartman 2008-08-26 296 goto exit;
5b775f67 Greg Kroah-Hartman 2008-08-26 297 }
5b775f67 Greg Kroah-Hartman 2008-08-26 298
cbe743f1 Guido Kiener 2018-09-12 299 dev_dbg(dev,
"INITIATE_ABORT_BULK_IN returned %x with tag %02x\n",
cbe743f1 Guido Kiener 2018-09-12 300 buffer[0], buffer[1]);
5b775f67 Greg Kroah-Hartman 2008-08-26 301
5b775f67 Greg Kroah-Hartman 2008-08-26 302 if (buffer[0] ==
USBTMC_STATUS_FAILED) {
cbe743f1 Guido Kiener 2018-09-12 303 /* No transfer in
progress and the Bulk-OUT FIFO is empty. */
5b775f67 Greg Kroah-Hartman 2008-08-26 304 rv = 0;
5b775f67 Greg Kroah-Hartman 2008-08-26 305 goto exit;
5b775f67 Greg Kroah-Hartman 2008-08-26 306 }
5b775f67 Greg Kroah-Hartman 2008-08-26 307
cbe743f1 Guido Kiener 2018-09-12 308 if (buffer[0] ==
USBTMC_STATUS_TRANSFER_NOT_IN_PROGRESS) {
cbe743f1 Guido Kiener 2018-09-12 309 /* The device returns
this status if either:
cbe743f1 Guido Kiener 2018-09-12 310 * - There is a
transfer in progress, but the specified bTag
cbe743f1 Guido Kiener 2018-09-12 311 * does not match.
cbe743f1 Guido Kiener 2018-09-12 312 * - There is no
transfer in progress, but the Bulk-OUT FIFO
cbe743f1 Guido Kiener 2018-09-12 313 * is not empty.
cbe743f1 Guido Kiener 2018-09-12 314 */
cbe743f1 Guido Kiener 2018-09-12 315 rv = -ENOMSG;
5b775f67 Greg Kroah-Hartman 2008-08-26 316 goto exit;
5b775f67 Greg Kroah-Hartman 2008-08-26 317 }
5b775f67 Greg Kroah-Hartman 2008-08-26 318
cbe743f1 Guido Kiener 2018-09-12 319 if (buffer[0] !=
USBTMC_STATUS_SUCCESS) {
cbe743f1 Guido Kiener 2018-09-12 320 dev_err(dev,
"INITIATE_ABORT_BULK_IN returned %x\n",
cbe743f1 Guido Kiener 2018-09-12 321 buffer[0]);
5b775f67 Greg Kroah-Hartman 2008-08-26 322 rv = -EPERM;
5b775f67 Greg Kroah-Hartman 2008-08-26 323 goto exit;
5b775f67 Greg Kroah-Hartman 2008-08-26 324 }
5b775f67 Greg Kroah-Hartman 2008-08-26 325
5b775f67 Greg Kroah-Hartman 2008-08-2
[usb:usb-testing 21/67] drivers/usb/class/usbtmc.c:1275 usbtmc_ioctl_request() warn: possible memory leak of 'buffer'
tree: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
usb-testing
head: ae8a2ca8a2215c7e31e6d874f7303801bb15fbbc
commit: 658f24f4523e41cda6a389c38b763f4c0cad6fbc [21/67] usb: usbtmc: Add ioctl
for generic requests on control
smatch warnings:
drivers/usb/class/usbtmc.c:1275 usbtmc_ioctl_request() warn: possible memory
leak of 'buffer'
drivers/usb/class/usbtmc.c:1278 usbtmc_ioctl_request() warn: overwrite may leak
'buffer'
#
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/?id=658f24f4523e41cda6a389c38b763f4c0cad6fbc
git remote add usb
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
git remote update usb
git checkout 658f24f4523e41cda6a389c38b763f4c0cad6fbc
vim +/buffer +1275 drivers/usb/class/usbtmc.c
5b775f67 Greg Kroah-Hartman 2008-08-26 1256
658f24f4 Guido Kiener 2018-09-12 1257 static int
usbtmc_ioctl_request(struct usbtmc_device_data *data,
658f24f4 Guido Kiener 2018-09-12 1258void
__user *arg)
658f24f4 Guido Kiener 2018-09-12 1259 {
658f24f4 Guido Kiener 2018-09-12 1260struct device *dev =
&data->intf->dev;
658f24f4 Guido Kiener 2018-09-12 1261struct usbtmc_ctrlrequest
request;
658f24f4 Guido Kiener 2018-09-12 1262u8 *buffer = NULL;
658f24f4 Guido Kiener 2018-09-12 1263int rv;
658f24f4 Guido Kiener 2018-09-12 1264unsigned long res;
658f24f4 Guido Kiener 2018-09-12 1265
658f24f4 Guido Kiener 2018-09-12 1266res = copy_from_user(&request,
arg, sizeof(struct usbtmc_ctrlrequest));
658f24f4 Guido Kiener 2018-09-12 1267if (res)
658f24f4 Guido Kiener 2018-09-12 1268return -EFAULT;
658f24f4 Guido Kiener 2018-09-12 1269
658f24f4 Guido Kiener 2018-09-12 1270buffer =
kmalloc(request.req.wLength, GFP_KERNEL);
^
658f24f4 Guido Kiener 2018-09-12 1271if (!buffer)
658f24f4 Guido Kiener 2018-09-12 1272return -ENOMEM;
658f24f4 Guido Kiener 2018-09-12 1273
658f24f4 Guido Kiener 2018-09-12 1274if (request.req.wLength >
USBTMC_BUFSIZE)
658f24f4 Guido Kiener 2018-09-12 @1275return -EMSGSIZE;
^
658f24f4 Guido Kiener 2018-09-12 1276
658f24f4 Guido Kiener 2018-09-12 1277if (request.req.wLength) {
658f24f4 Guido Kiener 2018-09-12 @1278buffer =
kmalloc(request.req.wLength, GFP_KERNEL);
^
658f24f4 Guido Kiener 2018-09-12 1279if (!buffer)
658f24f4 Guido Kiener 2018-09-12 1280return -ENOMEM;
658f24f4 Guido Kiener 2018-09-12 1281
658f24f4 Guido Kiener 2018-09-12 1282if
((request.req.bRequestType & USB_DIR_IN) == 0) {
658f24f4 Guido Kiener 2018-09-12 1283/* Send control
data to device */
658f24f4 Guido Kiener 2018-09-12 1284res =
copy_from_user(buffer, request.data,
658f24f4 Guido Kiener 2018-09-12 1285
request.req.wLength);
658f24f4 Guido Kiener 2018-09-12 1286if (res) {
658f24f4 Guido Kiener 2018-09-12 1287rv =
-EFAULT;
658f24f4 Guido Kiener 2018-09-12 1288goto
exit;
658f24f4 Guido Kiener 2018-09-12 1289}
658f24f4 Guido Kiener 2018-09-12 1290}
658f24f4 Guido Kiener 2018-09-12 1291}
658f24f4 Guido Kiener 2018-09-12 1292
658f24f4 Guido Kiener 2018-09-12 1293rv =
usb_control_msg(data->usb_dev,
658f24f4 Guido Kiener 2018-09-12 1294
usb_rcvctrlpipe(data->usb_dev, 0),
658f24f4 Guido Kiener 2018-09-12 1295
request.req.bRequest,
658f24f4 Guido Kiener 2018-09-12 1296
request.req.bRequestType,
658f24f4 Guido Kiener 2018-09-12 1297
request.req.wValue,
658f24f4 Guido Kiener 2018-09-12 1298
request.req.wIndex,
658f24f4 Guido Kiener 2018-09-12 1299buffer,
request.req.wLength, USB_CTRL_GET_TIMEOUT);
658f24f4 Guido Kiener 2018-09-12 1300
658f24f4 Guido Kiener 2018-09-12 1301if (rv < 0) {
658f24f4 Guido Kiener 2018-09-12 1302dev_err(dev, "%s failed
%d\n", __func__, rv);
658f24f4 Guido Kiener 2018-09-12 1303goto exit;
658f24f4 Guido Kiener 2018-09-12 1304}
658f24f4 Guido Kiener 2018-09-12 1305
658f24f4 Guido Kiener 2018-09-12 1306if (rv &&
(request.req.bRequestType & USB_DIR_IN)) {
658f24f4 Guido Kiener 2018-09-12 1307
Re: [PATCH] usb: dwc2: Disable all EP's on disconnect
On Thu, Sep 20, 2018 at 10:21:26AM +, Minas Harutyunyan wrote: > Hi Dan, > > On 9/20/2018 1:49 PM, Dan Carpenter wrote: > > is is obviously deliberate that we drop the other thread's lock and > > then take it ourselves. But I don't think that can be right at all. > > How do we know that it's safe for the other thread to drop the lock? > > > > There should at least be a long comment in front of the code explaining > > why it's safe to steal the lock like this. > > Please review patch version 2: "[PATCH v2] usb: dwc2: Disable all EP's > on disconnect" These are automated emails from the kbuild bot. I just review them and forward them. I'm not on the list. I'm going to trust that you fixed it in an appropriate way. :) regards, dan carpenter
Re: [PATCH] usb: dwc2: Disable all EP's on disconnect
Hi Minas,
url:
https://github.com/0day-ci/linux/commits/Minas-Harutyunyan/usb-dwc2-Disable-all-EP-s-on-disconnect/20180919-104259
base: https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git next
smatch warnings:
drivers/usb/dwc2/gadget.c:4024 dwc2_hsotg_ep_disable() error: double unlock
'spin_lock:&hsotg->lock'
drivers/usb/dwc2/gadget.c:4350 dwc2_hsotg_udc_stop() error: double unlock
'spin_lock:&hsotg->lock'
#
https://github.com/0day-ci/linux/commit/a4705c3fee2053e95791210eb435a04b665f6dc3
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout a4705c3fee2053e95791210eb435a04b665f6dc3
vim +4024 drivers/usb/dwc2/gadget.c
5b7d70c6d drivers/usb/gadget/s3c-hsotg.c Ben Dooks 2009-06-02 3993
8b9bc4608 drivers/usb/gadget/s3c-hsotg.c Lukasz Majewski2012-05-04 3994
/**
1f91b4cc0 drivers/usb/dwc2/gadget.c Felipe Balbi 2015-08-06 3995
* dwc2_hsotg_ep_disable - disable given endpoint
8b9bc4608 drivers/usb/gadget/s3c-hsotg.c Lukasz Majewski2012-05-04 3996
* @ep: The endpoint to disable.
8b9bc4608 drivers/usb/gadget/s3c-hsotg.c Lukasz Majewski2012-05-04 3997
*/
1f91b4cc0 drivers/usb/dwc2/gadget.c Felipe Balbi 2015-08-06 3998
static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
5b7d70c6d drivers/usb/gadget/s3c-hsotg.c Ben Dooks 2009-06-02 3999 {
1f91b4cc0 drivers/usb/dwc2/gadget.c Felipe Balbi 2015-08-06 4000
struct dwc2_hsotg_ep *hs_ep = our_ep(ep);
941fcce4f drivers/usb/dwc2/gadget.c Dinh Nguyen2014-11-11 4001
struct dwc2_hsotg *hsotg = hs_ep->parent;
5b7d70c6d drivers/usb/gadget/s3c-hsotg.c Ben Dooks 2009-06-02 4002
int dir_in = hs_ep->dir_in;
5b7d70c6d drivers/usb/gadget/s3c-hsotg.c Ben Dooks 2009-06-02 4003
int index = hs_ep->index;
5b7d70c6d drivers/usb/gadget/s3c-hsotg.c Ben Dooks 2009-06-02 4004
unsigned long flags;
5b7d70c6d drivers/usb/gadget/s3c-hsotg.c Ben Dooks 2009-06-02 4005
u32 epctrl_reg;
5b7d70c6d drivers/usb/gadget/s3c-hsotg.c Ben Dooks 2009-06-02 4006
u32 ctrl;
a4705c3fe drivers/usb/dwc2/gadget.c Minas Harutyunyan 2018-09-18 4007
int locked;
5b7d70c6d drivers/usb/gadget/s3c-hsotg.c Ben Dooks 2009-06-02 4008
1e0112937 drivers/usb/dwc2/gadget.c Marek Szyprowski 2014-09-09 4009
dev_dbg(hsotg->dev, "%s(ep %p)\n", __func__, ep);
5b7d70c6d drivers/usb/gadget/s3c-hsotg.c Ben Dooks 2009-06-02 4010
c6f5c050e drivers/usb/dwc2/gadget.c Mian Yousaf Kaukab 2015-01-09 4011
if (ep == &hsotg->eps_out[0]->ep) {
5b7d70c6d drivers/usb/gadget/s3c-hsotg.c Ben Dooks 2009-06-02 4012
dev_err(hsotg->dev, "%s: called for ep0\n", __func__);
5b7d70c6d drivers/usb/gadget/s3c-hsotg.c Ben Dooks 2009-06-02 4013
return -EINVAL;
9b481092c drivers/usb/dwc2/gadget.c John Stultz2017-10-23 4014
}
9b481092c drivers/usb/dwc2/gadget.c John Stultz2017-10-23 4015
9b481092c drivers/usb/dwc2/gadget.c John Stultz2017-10-23 4016
if (hsotg->op_state != OTG_STATE_B_PERIPHERAL) {
9b481092c drivers/usb/dwc2/gadget.c John Stultz2017-10-23 4017
dev_err(hsotg->dev, "%s: called in host mode?\n", __func__);
9b481092c drivers/usb/dwc2/gadget.c John Stultz2017-10-23 4018
return -EINVAL;
5b7d70c6d drivers/usb/gadget/s3c-hsotg.c Ben Dooks 2009-06-02 4019
}
5b7d70c6d drivers/usb/gadget/s3c-hsotg.c Ben Dooks 2009-06-02 4020
94cb8fd63 drivers/usb/gadget/s3c-hsotg.c Lukasz Majewski2012-05-04 4021
epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index);
5b7d70c6d drivers/usb/gadget/s3c-hsotg.c Ben Dooks 2009-06-02 4022
a4705c3fe drivers/usb/dwc2/gadget.c Minas Harutyunyan 2018-09-18 4023
locked = spin_trylock(&hsotg->lock);
a4705c3fe drivers/usb/dwc2/gadget.c Minas Harutyunyan 2018-09-18 @4024
spin_unlock(&hsotg->lock);
5ad1d3160 drivers/usb/gadget/s3c-hsotg.c Lukasz Majewski2012-06-14 4025
spin_lock_irqsave(&hsotg->lock, flags);
This is obviously deliberate that we drop the other thread's lock and
then take it ourselves. But I don't think that can be right at all.
How do we know that it's safe for the other thread to drop the lock?
There should at least be a long comment in front of the code explaining
why it's safe to steal the lock like this.
5b7d70c6d drivers/usb/gadget/s3c-hsotg.c Ben Dooks 2009-06-02 4026
f25c42b8d drivers/usb/dwc2/gadget.c Gevorg Sahakyan2018-07-26 4027
ctrl = dwc2_readl(hsotg, epctrl_reg);
a4f827714 drivers/usb/dwc2/gadget.c Vahram Aharonyan 2016-11-14 4028
a4f827714 drivers/usb/dwc2/gadget.c Vahram Aharonyan 2016-11-14 4029
if (ctrl & DXEPCTL_EPENA)
a4f827714 drivers/usb/dwc2/gadget.c Vahram Aharonyan 2016-11-14 4030
dwc2_hsotg_ep_stop_xfr(hsotg, hs
[bug report] usb: host: fotg2: add silicon clock handling
Hello Linus Walleij, This is a semi-automatic email about new static checker warnings. The patch ffa8a31b5b3b: "usb: host: fotg2: add silicon clock handling" from Sep 1, 2018, leads to the following Smatch complaint: drivers/usb/host/fotg210-hcd.c:5666 fotg210_hcd_remove() warn: variable dereferenced before check 'hcd' (see line 5661) drivers/usb/host/fotg210-hcd.c 5660 struct usb_hcd *hcd = dev_get_drvdata(dev); 5661 struct fotg210_hcd *fotg210 = hcd_to_fotg210(hcd); ^^^ Patch adds a dereference 5662 5663 if (!IS_ERR(fotg210->pclk)) 5664 clk_disable_unprepare(fotg210->pclk); 5665 5666 if (!hcd) ^^^ The old code assumes "hcd" can be NULL. 5667 return 0; 5668 regards, dan carpenter
[PATCH] USB: serial: io_ti: array underflow in edge_interrupt_callback()
A malicious USB device could set port_number to -3 and we would
underflow the edge_serial->serial->port[] array.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Dan Carpenter
diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c
index 6d1d6efa3055..fa2af18c6efe 100644
--- a/drivers/usb/serial/io_ti.c
+++ b/drivers/usb/serial/io_ti.c
@@ -1629,7 +1629,7 @@ static void edge_interrupt_callback(struct urb *urb)
struct device *dev;
unsigned char *data = urb->transfer_buffer;
int length = urb->actual_length;
- int port_number;
+ unsigned int port_number;
int function;
int retval;
__u8 lsr;
[PATCH] xhci: xhci-mem: off by one in xhci_stream_id_to_ring()
The > should be >= here so that we don't read one element beyond the end
of the ep->stream_info->stream_rings[] array.
Fixes: e9df17eb1408 ("USB: xhci: Correct assumptions about number of rings per
endpoint.")
Signed-off-by: Dan Carpenter
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 8a62eee9eee1..ef350c33dc4a 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -595,7 +595,7 @@ struct xhci_ring *xhci_stream_id_to_ring(
if (!ep->stream_info)
return NULL;
- if (stream_id > ep->stream_info->num_streams)
+ if (stream_id >= ep->stream_info->num_streams)
return NULL;
return ep->stream_info->stream_rings[stream_id];
}
--
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: serial: ch341: type promotion bug in ch341_control_in()
The "r" variable is an int and "bufsize" is an unsigned int so the
comparison is type promoted to unsigned. If usb_control_msg() returns a
negative that is treated as a high positive value and the error handling
doesn't work.
Fixes: 2d5a9c72d0c4 ("USB: serial: ch341: fix control-message error handling")
Signed-off-by: Dan Carpenter
diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index bdd7a5ad3bf1..3bb1fff02bed 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -128,7 +128,7 @@ static int ch341_control_in(struct usb_device *dev,
r = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0), request,
USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
value, index, buf, bufsize, DEFAULT_TIMEOUT);
- if (r < bufsize) {
+ if (r < (int)bufsize) {
if (r >= 0) {
dev_err(&dev->dev,
"short control message received (%d < %u)\n",
--
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: host: ehci-npcm7xx: Fix some error codes in probe
We accidentally return 1 instead of negative error codes.
Fixes: df44831ee2dd ("USB host: Add USB ehci support for nuvoton npcm7xx
platform")
Signed-off-by: Dan Carpenter
---
v2: fix typo in the commit message
diff --git a/drivers/usb/host/ehci-npcm7xx.c b/drivers/usb/host/ehci-npcm7xx.c
index c80a8792d3b0..adaf8fb4b459 100644
--- a/drivers/usb/host/ehci-npcm7xx.c
+++ b/drivers/usb/host/ehci-npcm7xx.c
@@ -74,14 +74,14 @@ static int npcm7xx_ehci_hcd_drv_probe(struct
platform_device *pdev)
if (IS_ERR(gcr_regmap)) {
dev_err(&pdev->dev, "%s: failed to find nuvoton,npcm750-gcr\n",
__func__);
- return IS_ERR(gcr_regmap);
+ return PTR_ERR(gcr_regmap);
}
rst_regmap = syscon_regmap_lookup_by_compatible("nuvoton,npcm750-rst");
if (IS_ERR(rst_regmap)) {
dev_err(&pdev->dev, "%s: failed to find nuvoton,npcm750-rst\n",
__func__);
- return IS_ERR(rst_regmap);
+ return PTR_ERR(rst_regmap);
}
/* phy init **/
--
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: host: ehci-npcm7xx: Fix some error codes in probe
We accidentally return 1 instead instead the negative error codes.
Fixes: df44831ee2dd ("USB host: Add USB ehci support for nuvoton npcm7xx
platform")
Signed-off-by: Dan Carpenter
---
The original patch used the "USB host:" prefix. This is driver code and
not generic USB host code so that's not the right thing. This is a very
common mistake in new drivers... It's a pain because then it's up to me
to guess what people want. Some subsystems have their own specific
rules and I can't remember them all so I sometimes get it wrong.
I probably guessed the obvious prefix this time so that's fine...
diff --git a/drivers/usb/host/ehci-npcm7xx.c b/drivers/usb/host/ehci-npcm7xx.c
index c80a8792d3b0..adaf8fb4b459 100644
--- a/drivers/usb/host/ehci-npcm7xx.c
+++ b/drivers/usb/host/ehci-npcm7xx.c
@@ -74,14 +74,14 @@ static int npcm7xx_ehci_hcd_drv_probe(struct
platform_device *pdev)
if (IS_ERR(gcr_regmap)) {
dev_err(&pdev->dev, "%s: failed to find nuvoton,npcm750-gcr\n",
__func__);
- return IS_ERR(gcr_regmap);
+ return PTR_ERR(gcr_regmap);
}
rst_regmap = syscon_regmap_lookup_by_compatible("nuvoton,npcm750-rst");
if (IS_ERR(rst_regmap)) {
dev_err(&pdev->dev, "%s: failed to find nuvoton,npcm750-rst\n",
__func__);
- return IS_ERR(rst_regmap);
+ return PTR_ERR(rst_regmap);
}
/* phy init **/
--
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] typec: tcpm: Fix a msecs vs jiffies bug
The tcpm_set_state() function take msecs not jiffies.
Fixes: f0690a25a140 ("staging: typec: USB Type-C Port Manager (tcpm)")
Signed-off-by: Dan Carpenter
diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
index 8a201dd53d36..0dfd755020f4 100644
--- a/drivers/usb/typec/tcpm.c
+++ b/drivers/usb/typec/tcpm.c
@@ -3043,7 +3043,8 @@ static void run_state_machine(struct tcpm_port *port)
tcpm_port_is_sink(port) &&
time_is_after_jiffies(port->delayed_runtime)) {
tcpm_set_state(port, SNK_DISCOVERY,
- port->delayed_runtime - jiffies);
+ jiffies_to_msecs(port->delayed_runtime -
+ jiffies));
break;
}
tcpm_set_state(port, unattached_state(port), 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
[bug report] usb: musb: break the huge isr musb_stage0_irq() into small functions
Hello Bin Liu, The patch bcb8fd3a2fba: "usb: musb: break the huge isr musb_stage0_irq() into small functions" from May 21, 2018, leads to the following static checker warning: drivers/usb/musb/musb_core.c:825 musb_handle_intr_disconnect() warn: variable dereferenced before check 'musb->hcd' (see line 824) drivers/usb/musb/musb_core.c 818 case OTG_STATE_B_HOST: 819 /* REVISIT this behaves for "real disconnect" 820 * cases; make sure the other transitions from 821 * from B_HOST act right too. The B_HOST code 822 * in hnp_stop() is currently not used... 823 */ 824 musb_root_disconnect(musb); disconnect will oops if musb->hcd is NULL 825 if (musb->hcd) ^ 826 musb->hcd->self.is_b_host = 0; 827 musb->xceiv->otg->state = OTG_STATE_B_PERIPHERAL; 828 MUSB_DEV_MODE(musb); 829 musb_g_disconnect(musb); 830 break; 831 case OTG_STATE_A_PERIPHERAL: 832 musb_hnp_stop(musb); regards, dan carpenter -- 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
[bug report] usb: musb: break the huge isr musb_stage0_irq() into small functions
Hello Bin Liu,
The patch bcb8fd3a2fba: "usb: musb: break the huge isr
musb_stage0_irq() into small functions" from May 21, 2018, leads to
the following static checker warning:
drivers/usb/musb/musb_core.c:797 musb_handle_intr_connect()
error: we previously assumed 'musb->hcd' could be null (see line 783)
drivers/usb/musb/musb_core.c
779 case OTG_STATE_B_WAIT_ACON:
780 musb_dbg(musb, "HNP: CONNECT, now b_host");
781 b_host:
782 musb->xceiv->otg->state = OTG_STATE_B_HOST;
783 if (musb->hcd)
^
Checked for NULL
784 musb->hcd->self.is_b_host = 1;
785 del_timer(&musb->otg_timer);
786 break;
787 default:
788 if ((devctl & MUSB_DEVCTL_VBUS)
789 == (3 << MUSB_DEVCTL_VBUS_SHIFT)) {
790 musb->xceiv->otg->state = OTG_STATE_A_HOST;
791 if (hcd)
792 hcd->self.is_b_host = 0;
793 }
794 break;
795 }
796
797 musb_host_poke_root_hub(musb);
Unchecked dereference inside function.
798
799 musb_dbg(musb, "CONNECT (%s) devctl %02x",
800 usb_otg_state_string(musb->xceiv->otg->state),
devctl);
801 }
regards,
dan carpenter
--
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 09/11] usb: musb: break the huge isr musb_stage0_irq() into small functions
On Wed, Apr 18, 2018 at 08:33:52AM -0500, Bin Liu wrote: > Hi Dan, > > I appreciate you scaning the patches and reporting the issues. > These are kbuild stuff so I basically just forward them. It's no effort. > On Wed, Apr 18, 2018 at 10:25:34AM +0300, Dan Carpenter wrote: > > Hi Bin, > > > > I love your patch! Perhaps something to improve: > > > > [auto build test WARNING on balbi-usb/next] > > [also build test WARNING on v4.17-rc1 next-20180417] > > [if your patch is applied to the wrong git tree, please drop us a note to > > help improve the system] > > > > url: > > https://github.com/0day-ci/linux/commits/Bin-Liu/usb-musb-cleanup/20180417-133633 > > base: https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git next > > > > smatch warnings: > > drivers/usb/musb/musb_core.c:797 musb_handle_intr_connect() error: we > > previously assumed 'musb->hcd' could be null (see line 783) > > It appears the condition check was introduced back in 2013 in an attempt > to separate host-only and gadget-only configurations for musb [1], but it > seems the work is not complete, because the musb->hcd allocation is > unconditional. So in the current code, musb->hcd won't be NULL. > > But I am now hesitated to remove this 'if(musb->hcd)' check to solve > this smatch warning, because I am still debating on either continue the > work to separate the two configurations or clean up the uncompleted code > left by the attempt to not separate them. > > So I want to leave the patch as is for now, is it reasonable? > Fine by me. Right now it's sort of hard for Smatch to parse the code well enough for it to know that ->hcd is always non-NULL but I'm hopeful that maybe in a couple years that will be possible... regards, dan carpenter -- 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 09/11] usb: musb: break the huge isr musb_stage0_irq() into small functions
Hi Bin,
I love your patch! Perhaps something to improve:
[auto build test WARNING on balbi-usb/next]
[also build test WARNING on v4.17-rc1 next-20180417]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Bin-Liu/usb-musb-cleanup/20180417-133633
base: https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git next
smatch warnings:
drivers/usb/musb/musb_core.c:797 musb_handle_intr_connect() error: we
previously assumed 'musb->hcd' could be null (see line 783)
#
https://github.com/0day-ci/linux/commit/92be75b4fb79173759af42e50a81d0020e945c1e
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 92be75b4fb79173759af42e50a81d0020e945c1e
vim +797 drivers/usb/musb/musb_core.c
1c25fda4a Arnaud Mandy 2009-12-28 745
92be75b4f Bin Liu 2018-04-16 746 static void
musb_handle_intr_connect(struct musb *musb, u8 devctl, u8 int_usb)
92be75b4f Bin Liu 2018-04-16 747 {
8b125df5b Daniel Mack 2013-04-10 748 struct usb_hcd *hcd =
musb->hcd;
550a7375f Felipe Balbi 2008-07-24 749
550a7375f Felipe Balbi 2008-07-24 750 musb->is_active = 1;
550a7375f Felipe Balbi 2008-07-24 751 musb->ep0_stage =
MUSB_EP0_START;
550a7375f Felipe Balbi 2008-07-24 752
b18d26f6a Sebastian Andrzej Siewior 2012-10-30 753 musb->intrtxe =
musb->epmask;
b18d26f6a Sebastian Andrzej Siewior 2012-10-30 754
musb_writew(musb->mregs, MUSB_INTRTXE, musb->intrtxe);
af5ec14d4 Sebastian Andrzej Siewior 2012-10-30 755 musb->intrrxe =
musb->epmask & 0xfffe;
af5ec14d4 Sebastian Andrzej Siewior 2012-10-30 756
musb_writew(musb->mregs, MUSB_INTRRXE, musb->intrrxe);
d709d22ee Ajay Kumar Gupta 2010-07-08 757
musb_writeb(musb->mregs, MUSB_INTRUSBE, 0xf7);
550a7375f Felipe Balbi 2008-07-24 758 musb->port1_status &=
~(USB_PORT_STAT_LOW_SPEED
550a7375f Felipe Balbi 2008-07-24 759
|USB_PORT_STAT_HIGH_SPEED
550a7375f Felipe Balbi 2008-07-24 760
|USB_PORT_STAT_ENABLE
550a7375f Felipe Balbi 2008-07-24 761
);
550a7375f Felipe Balbi 2008-07-24 762 musb->port1_status |=
USB_PORT_STAT_CONNECTION
550a7375f Felipe Balbi 2008-07-24 763
|(USB_PORT_STAT_C_CONNECTION << 16);
550a7375f Felipe Balbi 2008-07-24 764
550a7375f Felipe Balbi 2008-07-24 765 /* high vs full speed
is just a guess until after reset */
550a7375f Felipe Balbi 2008-07-24 766 if (devctl &
MUSB_DEVCTL_LSDEV)
550a7375f Felipe Balbi 2008-07-24 767
musb->port1_status |= USB_PORT_STAT_LOW_SPEED;
550a7375f Felipe Balbi 2008-07-24 768
550a7375f Felipe Balbi 2008-07-24 769 /* indicate new
connection to OTG machine */
e47d92545 Antoine Tenart2014-10-30 770 switch
(musb->xceiv->otg->state) {
550a7375f Felipe Balbi 2008-07-24 771 case
OTG_STATE_B_PERIPHERAL:
550a7375f Felipe Balbi 2008-07-24 772 if (int_usb &
MUSB_INTR_SUSPEND) {
b99d3659b Bin Liu 2016-06-30 773
musb_dbg(musb, "HNP: SUSPEND+CONNECT, now b_host");
550a7375f Felipe Balbi 2008-07-24 774 int_usb
&= ~MUSB_INTR_SUSPEND;
1de00dae8 David Brownell2009-04-02 775 goto
b_host;
550a7375f Felipe Balbi 2008-07-24 776 } else
b99d3659b Bin Liu 2016-06-30 777
musb_dbg(musb, "CONNECT as b_peripheral???");
550a7375f Felipe Balbi 2008-07-24 778 break;
550a7375f Felipe Balbi 2008-07-24 779 case
OTG_STATE_B_WAIT_ACON:
b99d3659b Bin Liu 2016-06-30 780 musb_dbg(musb,
"HNP: CONNECT, now b_host");
1de00dae8 David Brownell2009-04-02 781 b_host:
e47d92545 Antoine Tenart2014-10-30 782
musb->xceiv->otg->state = OTG_STATE_B_HOST;
74c2e9360 Daniel Mack 2013-04-10 @783 if (musb->hcd)
74c2e9360 Daniel Mack 2013-04-10 784
musb->hcd->self.is_b_host = 1;
1de00dae8 David Brownell2009-04-02 785
del_timer(&musb->otg_timer);
550a7375f Felipe Balbi 2008-07-24 786 break;
550a7375f Felipe Balbi 2008-07-24 787 default:
550a7375f Felipe Balbi 2008-07-24 788 if ((devctl &
MUSB_DEVCTL_VBUS)
550a7375f Felipe Balbi 2008-07-24 789
== (3 << MUSB_DEVCTL_VBUS_SHIFT)) {
e47d92545 Antoine Tenart2014-10-3
Re: [PATCH v4 07/13] staging: typec: tcpci: register port before request irq
On Sat, Mar 31, 2018 at 03:09:44AM +, Jun Li wrote: > This patch is to change the sequence of register port and request irq, > if error code checking of original code has the problem, I think that > should be another patch to fix it, I can do that later. Fair enough. That's reasonable. Thanks! regards, dan carpenter -- 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 v4 07/13] staging: typec: tcpci: register port before request irq
On Thu, Mar 29, 2018 at 12:06:12AM +0800, Li Jun wrote:
> With that we can clear any pending events and the port is registered
> so driver can be ready to handle typec events once we request irq.
>
> Signed-off-by: Peter Chen
> Signed-off-by: Li Jun
These sign offs aren't clear.
Sign offs mean that you handled the patch but didn't include any of
SCO's copyrighted UNIX code into it. Normally they're in the order of
who touched the code. So Peter touched the code first. Should he get
authorship credit? How did he touch the code first if he didn't write
the code? It doesn't make sense.
> ---
> drivers/staging/typec/tcpci.c | 15 ---
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
> index 4f7ad10..9e0014b 100644
> --- a/drivers/staging/typec/tcpci.c
> +++ b/drivers/staging/typec/tcpci.c
> @@ -537,25 +537,26 @@ static int tcpci_probe(struct i2c_client *client,
> if (IS_ERR(chip->data.regmap))
> return PTR_ERR(chip->data.regmap);
>
> + i2c_set_clientdata(client, chip);
> +
> /* Disable chip interrupts before requesting irq */
> err = regmap_raw_write(chip->data.regmap, TCPC_ALERT_MASK, &val,
> sizeof(u16));
> if (err < 0)
> return err;
>
> + chip->tcpci = tcpci_register_port(&client->dev, &chip->data);
> + if (PTR_ERR_OR_ZERO(chip->tcpci))
> + return PTR_ERR(chip->tcpci);
When a function returns both error pointers and NULL that means that
NULL is a secial case of success. Like for example:
p->my_feature = get_optional_feature();
If it returns NULL that means the optional feature isn't there, but it's
fine because it's optional. But if it returns an error pointer that
means the feature is there but the hardware is buggy or something so
we shouldn't continue.
If you return PTR_ERR(NULL) that means success.
I don't think this code makes sense just from looking at it and also
when I checked tcpci_register_port() doesn't return NULL.
> +
> err = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> _tcpci_irq,
> IRQF_ONESHOT | IRQF_TRIGGER_LOW,
> dev_name(&client->dev), chip);
> if (err < 0)
> - return err;
> + tcpci_unregister_port(chip->tcpci);
Can you put the "return err;" back, because that's better style. It's
better to keep the error path and success path separate if you can.
if (err < 0) {
tcpci_unregister_port(chip->tcpci);
return err;
}
return 0;
regards,
dan carpenter
--
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 v1 3/3] usb: dwc2: Add High Bandwidth ISOC OUT support
Hi Minas,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on next-20180309]
[cannot apply to v4.16-rc4 v4.16-rc3 v4.16-rc2 v4.16-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Minas-Harutyunyan/usb-dwc2-gadget-Update-ISOC-DDMA-flow/20180318-134649
smatch warnings:
drivers/usb/dwc2/gadget.c:2250 dwc2_gadget_complete_isoc_request_ddma() warn:
signed overflow undefined. 'index - (index / dpi) * dpi + 1 < dpi'
drivers/usb/dwc2/gadget.c:2270 dwc2_gadget_complete_isoc_request_ddma() warn:
should this be a bitwise op?
drivers/usb/dwc2/gadget.c:2270 dwc2_gadget_complete_isoc_request_ddma() warn:
should this be a bitwise op?
#
https://github.com/0day-ci/linux/commit/ffb8a411a9d55bf8c15292a6ded7c8663a8ac290
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout ffb8a411a9d55bf8c15292a6ded7c8663a8ac290
vim +2250 drivers/usb/dwc2/gadget.c
5b7d70c6 drivers/usb/gadget/s3c-hsotg.c Ben Dooks 2009-06-02 2066
ffb8a411 drivers/usb/dwc2/gadget.c Minas Harutyunyan 2018-03-17 2067
static void dwc2_hsotg_ep_stop_xfr(struct dwc2_hsotg *hsotg,
ffb8a411 drivers/usb/dwc2/gadget.c Minas Harutyunyan 2018-03-17 2068
struct dwc2_hsotg_ep *hs_ep);
ffb8a411 drivers/usb/dwc2/gadget.c Minas Harutyunyan 2018-03-17 2069
540ccba0 drivers/usb/dwc2/gadget.c Vahram Aharonyan 2016-11-14 2070 /*
540ccba0 drivers/usb/dwc2/gadget.c Vahram Aharonyan 2016-11-14 2071 *
dwc2_gadget_complete_isoc_request_ddma - complete an isoc request in DDMA
540ccba0 drivers/usb/dwc2/gadget.c Vahram Aharonyan 2016-11-14 2072 *
@hs_ep: The endpoint the request was on.
540ccba0 drivers/usb/dwc2/gadget.c Vahram Aharonyan 2016-11-14 2073 *
540ccba0 drivers/usb/dwc2/gadget.c Vahram Aharonyan 2016-11-14 2074 *
Get first request from the ep queue, determine descriptor on which complete
a19205da drivers/usb/dwc2/gadget.c Minas Harutyunyan 2018-03-17 2075 *
happened. SW discovers which descriptor currently in use by HW, adjusts
a19205da drivers/usb/dwc2/gadget.c Minas Harutyunyan 2018-03-17 2076 *
dma_address and calculates index of completed descriptor based on the value
a19205da drivers/usb/dwc2/gadget.c Minas Harutyunyan 2018-03-17 2077 *
of DEPDMA register. Update actual length of request, giveback to gadget.
540ccba0 drivers/usb/dwc2/gadget.c Vahram Aharonyan 2016-11-14 2078 */
540ccba0 drivers/usb/dwc2/gadget.c Vahram Aharonyan 2016-11-14 2079
static void dwc2_gadget_complete_isoc_request_ddma(struct dwc2_hsotg_ep *hs_ep)
540ccba0 drivers/usb/dwc2/gadget.c Vahram Aharonyan 2016-11-14 2080 {
540ccba0 drivers/usb/dwc2/gadget.c Vahram Aharonyan 2016-11-14 2081
struct dwc2_hsotg *hsotg = hs_ep->parent;
540ccba0 drivers/usb/dwc2/gadget.c Vahram Aharonyan 2016-11-14 2082
struct dwc2_hsotg_req *hs_req;
540ccba0 drivers/usb/dwc2/gadget.c Vahram Aharonyan 2016-11-14 2083
struct usb_request *ureq;
ffb8a411 drivers/usb/dwc2/gadget.c Minas Harutyunyan 2018-03-17 2084
int index, idx;
540ccba0 drivers/usb/dwc2/gadget.c Vahram Aharonyan 2016-11-14 2085
dma_addr_t dma_addr;
540ccba0 drivers/usb/dwc2/gadget.c Vahram Aharonyan 2016-11-14 2086
u32 dma_reg;
540ccba0 drivers/usb/dwc2/gadget.c Vahram Aharonyan 2016-11-14 2087
u32 depdma;
540ccba0 drivers/usb/dwc2/gadget.c Vahram Aharonyan 2016-11-14 2088
u32 desc_sts;
540ccba0 drivers/usb/dwc2/gadget.c Vahram Aharonyan 2016-11-14 2089
u32 mask;
ffb8a411 drivers/usb/dwc2/gadget.c Minas Harutyunyan 2018-03-17 2090
int dpi;
ffb8a411 drivers/usb/dwc2/gadget.c Minas Harutyunyan 2018-03-17 2091
int ret;
ffb8a411 drivers/usb/dwc2/gadget.c Minas Harutyunyan 2018-03-17 2092
int sumofpid;
ffb8a411 drivers/usb/dwc2/gadget.c Minas Harutyunyan 2018-03-17 2093
ffb8a411 drivers/usb/dwc2/gadget.c Minas Harutyunyan 2018-03-17 2094
dpi = 1;
ffb8a411 drivers/usb/dwc2/gadget.c Minas Harutyunyan 2018-03-17 2095
if (!hs_ep->dir_in)
ffb8a411 drivers/usb/dwc2/gadget.c Minas Harutyunyan 2018-03-17 2096
dpi = hs_ep->mc;
540ccba0 drivers/usb/dwc2/gadget.c Vahram Aharonyan 2016-11-14 2097
540ccba0 drivers/usb/dwc2/gadget.c Vahram Aharonyan 2016-11-14 2098
hs_req = get_ep_head(hs_ep);
540ccba0 drivers/usb/dwc2/gadget.c Vahram Aharonyan 2016-11-14 2099
if (!hs_req) {
ffb8a411 drivers/usb/dwc2/gadget.c Minas Harutyunyan 2018-03-17 2100
hs_ep->target_frame = TARGET_FRAME_INITIAL;
ffb8a411 drivers/usb/dwc2/gadget.c Minas Harutyunyan 2018-03-17 2101
dwc2_hsotg_ep_stop_xfr(hsotg, hs_ep);
540ccba0 drivers/usb/dwc2/gadget.c Vahram Aharonyan 2
Re: [bug report] usb: core: Add "quirks" parameter for usbcore
On Fri, Mar 23, 2018 at 02:16:23PM +0800, Kai-Heng Feng wrote: > Hi Dan, > > Dan Carpenter wrote: > > > Hello Kai-Heng Feng, > > > > This is a semi-automatic email about new static checker warnings. > > I ran Smatch but didn't see the error message: > $ make -j`nproc` CHECK="~/smatch/smatch -p=kernel" C=1 bzImage modules | tee > warns.txt`" > You need to have the cross function database built. It takes forever. smatch_scripts/build_kernel_data.sh > Also, "val" will never get passed as null in quirks_param_set() by the > .set() caller, so it's actually superfluous. Smatch tries not to warn about these if it's superfluous, but here it thinks that val can be NULL from parse_one(). It's non-NULL from the other two callers. kernel/params.c |parse_one | (struct kernel_param_ops)->set | PARAM_VALUE | 0 | val | 0,4096-2117778 kernel/params.c | param_attr_store | (struct kernel_param_ops)->set | PARAM_VALUE | 0 | val | 5-5 kernel/params.c | param_array | param_array param 7 | PARAM_VALUE | 0 | val | 4096-2117777 Anwyway, we should probably remove the NULL check just for consistency. regards, dan carpenter -- 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
[bug report] usb: core: Add "quirks" parameter for usbcore
Hello Kai-Heng Feng,
This is a semi-automatic email about new static checker warnings.
The patch 027bd6cafd9a: "usb: core: Add "quirks" parameter for
usbcore" from Mar 20, 2018, leads to the following Smatch complaint:
drivers/usb/core/quirks.c:136 quirks_param_set()
error: we previously assumed 'val' could be null (see line 37)
drivers/usb/core/quirks.c
36
37 if (!val || !*val) {
Patch adds check for NULL
38 quirk_count = 0;
39 kfree(quirk_list);
40 quirk_list = NULL;
41 goto unlock;
42 }
43
44 for (quirk_count = 1, i = 0; val[i]; i++)
45 if (val[i] == ',')
46 quirk_count++;
47
48 if (quirk_list) {
49 kfree(quirk_list);
50 quirk_list = NULL;
51 }
52
53 quirk_list = kcalloc(quirk_count, sizeof(struct quirk_entry),
54 GFP_KERNEL);
55 if (!quirk_list) {
56 mutex_unlock(&quirk_mutex);
57 return -ENOMEM;
58 }
59
60 for (i = 0, p = (char *)val; p && *p;) {
61 /* Each entry consists of VID:PID:flags */
62 field = strsep(&p, ":");
63 if (!field)
64 break;
65
66 if (kstrtou16(field, 16, &vid))
67 break;
68
69 field = strsep(&p, ":");
70 if (!field)
71 break;
72
73 if (kstrtou16(field, 16, &pid))
74 break;
75
76 field = strsep(&p, ",");
77 if (!field || !*field)
78 break;
79
80 /* Collect the flags */
81 for (flags = 0; *field; field++) {
82 switch (*field) {
83 case 'a':
84 flags |= USB_QUIRK_STRING_FETCH_255;
85 break;
86 case 'b':
87 flags |= USB_QUIRK_RESET_RESUME;
88 break;
89 case 'c':
90 flags |= USB_QUIRK_NO_SET_INTF;
91 break;
92 case 'd':
93 flags |= USB_QUIRK_CONFIG_INTF_STRINGS;
94 break;
95 case 'e':
96 flags |= USB_QUIRK_RESET;
97 break;
98 case 'f':
99 flags |= USB_QUIRK_HONOR_BNUMINTERFACES;
100 break;
101 case 'g':
102 flags |= USB_QUIRK_DELAY_INIT;
103 break;
104 case 'h':
105 flags |=
USB_QUIRK_LINEAR_UFRAME_INTR_BINTERVAL;
106 break;
107 case 'i':
108 flags |= USB_QUIRK_DEVICE_QUALIFIER;
109 break;
110 case 'j':
111 flags |= USB_QUIRK_IGNORE_REMOTE_WAKEUP;
112 break;
113 case 'k':
114 flags |= USB_QUIRK_NO_LPM;
115 break;
116 case 'l':
117 flags |=
USB_QUIRK_LINEAR_FRAME_INTR_BINTERVAL;
118 break;
119 case 'm':
120 flags |= USB_QUIRK_DISCONNECT_SUSPEND;
121 break;
122 /* Ignore unrecognized flag characters */
123 }
124 }
125
126 quirk_list[i++] = (struct quirk_entry)
127 { .vid = vid, .pid = pid, .flags = flags };
128 }
129
130 if (i < quirk_count)
131 quirk_count = i;
132
133 unlock:
134 mutex_unlock(&quirk_mutex)
[PATCH] usb: host: ohci-pxa27x: Fix and & vs | typo
The code is supposed to clear the RH_A_NPS and RH_A_PSM bits, but it's a no-op because of the & vs | typo. This bug predates git and it was only discovered using static analysis so it must not affect too many people in real life. Signed-off-by: Dan Carpenter --- Not tested. diff --git a/drivers/usb/host/ohci-pxa27x.c b/drivers/usb/host/ohci-pxa27x.c index 3e2474959735..7679fb583e41 100644 --- a/drivers/usb/host/ohci-pxa27x.c +++ b/drivers/usb/host/ohci-pxa27x.c @@ -148,7 +148,7 @@ static int pxa27x_ohci_select_pmm(struct pxa27x_ohci *pxa_ohci, int mode) uhcrhda |= RH_A_NPS; break; case PMM_GLOBAL_MODE: - uhcrhda &= ~(RH_A_NPS & RH_A_PSM); + uhcrhda &= ~(RH_A_NPS | RH_A_PSM); break; case PMM_PERPORT_MODE: uhcrhda &= ~(RH_A_NPS); -- 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 3/4] Staging: rtl8723bs: Change condition to assignment
On Wed, Jan 17, 2018 at 12:33:23AM +0530, Shreeya Patel wrote:
> Change the conditional operator to assignment as it is
> not a conditional statement.
>
> Signed-off-by: Shreeya Patel
> ---
>
> Changes in v2
> -Rebase and resend.
>
> drivers/staging/rtl8723bs/hal/sdio_ops.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8723bs/hal/sdio_ops.c
> b/drivers/staging/rtl8723bs/hal/sdio_ops.c
> index 00b20c0..314b31f 100644
> --- a/drivers/staging/rtl8723bs/hal/sdio_ops.c
> +++ b/drivers/staging/rtl8723bs/hal/sdio_ops.c
> @@ -460,7 +460,7 @@ static u32 sdio_read_port(
> if (mem == NULL) {
> DBG_8192C(KERN_WARNING "%s: allocate memory %d bytes
> fail!\n", __func__, cnt);
> mem = oldmem;
> - oldmem == NULL;
> + oldmem = NULL;
> }
> #else
This bug would cause a GCC warning if it were ever compiled. This
ifdefed code is dead. We should just delete it.
regards,
dan carpenter
--
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: xhci: Clean up error code in xhci_dbc_tty_register_device()
tty_port_register_device() returns error pointers on error, never NULL.
The IS_ERR_OR_NULL() function returns either 1 or 0 so it means we
return 1 on error instead of a proper error code. The caller only
checks for zero vs non-zero so this doesn't affect runtime.
Signed-off-by: Dan Carpenter
diff --git a/drivers/usb/host/xhci-dbgtty.c b/drivers/usb/host/xhci-dbgtty.c
index 8d47b6fbf973..6f534b6decef 100644
--- a/drivers/usb/host/xhci-dbgtty.c
+++ b/drivers/usb/host/xhci-dbgtty.c
@@ -443,9 +443,10 @@ int xhci_dbc_tty_register_device(struct xhci_hcd *xhci)
xhci_dbc_tty_init_port(xhci, port);
tty_dev = tty_port_register_device(&port->port,
dbc_tty_driver, 0, NULL);
- ret = IS_ERR_OR_NULL(tty_dev);
- if (ret)
+ if (IS_ERR(tty_dev)) {
+ ret = PTR_ERR(tty_dev);
goto register_fail;
+ }
ret = kfifo_alloc(&port->write_fifo, DBC_WRITE_BUF_SIZE, GFP_KERNEL);
if (ret)
--
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: [bug report] usbip: fix stub_rx: harden CMD_SUBMIT path to handle malicious input
On Thu, Dec 14, 2017 at 11:01:15AM -0700, Shuah Khan wrote:
> Hi Dan,
>
> On 12/14/2017 12:58 AM, Dan Carpenter wrote:
> > Hello Shuah Khan,
> >
> > The patch c6688ef9f297: "usbip: fix stub_rx: harden CMD_SUBMIT path
> > to handle malicious input" from Dec 7, 2017, leads to the following
> > static checker warning:
> >
> > drivers/usb/usbip/stub_rx.c:346 get_pipe()
> > warn: impossible condition '(pdu->u.cmd_submit.transfer_buffer_length >
> > ((~0 >> 1))) => (s32min-s32max > s32max)'
> > drivers/usb/usbip/stub_rx.c:486 stub_recv_cmd_submit()
> > warn: always true condition '(pdu->u.cmd_submit.transfer_buffer_length
> > <= ((~0 >> 1))) => (s32min-s32max <= s32max)'
> >
> > drivers/usb/usbip/stub_rx.c
> >343 epd = &ep->desc;
> >344
> >345 /* validate transfer_buffer_length */
> >346 if (pdu->u.cmd_submit.transfer_buffer_length > INT_MAX) {
> > ^^
> > This is an int.
>
> Yeah the check should have been against S32_MAX for the two checks
> in this patch.
TBH, I don't understand.
INT_MAX is always the same as S32_MAX on every arch (that wasn't always
true in ancient times but it's always true on linux arches).
regards,
dan carpenter
--
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
[bug report] usbip: fix stub_rx: harden CMD_SUBMIT path to handle malicious input
Hello Shuah Khan,
The patch c6688ef9f297: "usbip: fix stub_rx: harden CMD_SUBMIT path
to handle malicious input" from Dec 7, 2017, leads to the following
static checker warning:
drivers/usb/usbip/stub_rx.c:346 get_pipe()
warn: impossible condition '(pdu->u.cmd_submit.transfer_buffer_length >
((~0 >> 1))) => (s32min-s32max > s32max)'
drivers/usb/usbip/stub_rx.c:486 stub_recv_cmd_submit()
warn: always true condition '(pdu->u.cmd_submit.transfer_buffer_length <=
((~0 >> 1))) => (s32min-s32max <= s32max)'
drivers/usb/usbip/stub_rx.c
343 epd = &ep->desc;
344
345 /* validate transfer_buffer_length */
346 if (pdu->u.cmd_submit.transfer_buffer_length > INT_MAX) {
^^
This is an int.
347 dev_err(&sdev->udev->dev,
348 "CMD_SUBMIT: -EMSGSIZE transfer_buffer_length
%d\n",
349 pdu->u.cmd_submit.transfer_buffer_length);
350 return -1;
351 }
[ snip ]
479 if (!priv->urb) {
480 usbip_event_add(ud, SDEV_EVENT_ERROR_MALLOC);
481 return;
482 }
483
484 /* allocate urb transfer buffer, if needed */
485 if (pdu->u.cmd_submit.transfer_buffer_length > 0 &&
486 pdu->u.cmd_submit.transfer_buffer_length <= INT_MAX) {
^^
487 priv->urb->transfer_buffer =
488
kzalloc(pdu->u.cmd_submit.transfer_buffer_length,
489 GFP_KERNEL);
490 if (!priv->urb->transfer_buffer) {
491 usbip_event_add(ud, SDEV_EVENT_ERROR_MALLOC);
492 return;
493 }
494 }
regards,
dan carpenter
--
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: hub: Delete an error message for a failed memory allocation in usb_hub_clear_tt_buffer()
On Wed, Dec 06, 2017 at 09:20:05PM +0100, Geert Uytterhoeven wrote: > Hi Markus, > > On Wed, Dec 6, 2017 at 6:51 PM, SF Markus Elfring > wrote: > >> The system will come to a grinding halt anyway if it can't allocate 24 or > >> 40 bytes. > > > > Maybe. > > Since you've been sending zillions of patches for this, perhaps the time > is ripe to actually try to trigger this, and see what happens? > > >> Which is BTW more or less the amount of memory saved by killing > >> the useless (error) message. > > > > Would you dare to resend this update suggestion after such a view? > > Of course. That was implied. > No. Greg maintains USB and he's has blocked Markus, because he never listens to feedback but instead just repsonds that he has a different opinion. regards, dan carpenter -- 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: hub: Delete an error message for a failed memory allocation in usb_hub_clear_tt_buffer()
On Thu, Dec 07, 2017 at 10:12:27AM -0500, Alan Stern wrote: > The real problem is that the kernel development community doesn't have > a fixed policy on how to handle memory allocation errors. There are > several possibilities: > > Ignore them on the grounds that they will never happen. > (Really? And what is the size limit above which they > might happen?) > It's pretty rare to ignore allocation failures these days. It causes static checker warnings. Sometimes it's accepted for people to ignore errors during boot but I hate that because how am I supposed to filter out those static checker warnings? It's better to pretend that the kernel will still boot without essential hardware instead of wasting everyone's time who looks at checker output. > Ignore them on the grounds that the machine will hang or > crash in the near future. (Is this guaranteed?) On boot sometimes yes. > > Treat them like other errors: try to press forward (perhaps > in a degraded mode). > > Treat them like other errors: log an error message and try > to press forward. > The standard is to treat them like errors and try press forward in a degraded mode but don't print a message. Checkpatch.pl complains if you print a warning for allocation failures. A lot of low level functions handle their own messages pretty well but especially kmalloc() does. I also have a special static checker warning for when people do: foo = alloc(); BUG_ON(!foo); People do that occasionally but fortunately it's pretty rare. 10 years ago that's how btrfs did error handling, but now there are only 4 of these still remaining in btrfs. regards, dan carpenter -- 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: hub: Delete an error message for a failed memory allocation in usb_hub_clear_tt_buffer()
On Thu, Dec 07, 2017 at 09:45:38AM +0100, Geert Uytterhoeven wrote: > > > > Small allocations never fail in the current kernel. > > A few comments (this is in response to a patch from Markus, so there have > to be lots of questions and uncertainties ;-) > 1. In the current kernel. What about the future? Right. No one can predict. And the small allocations don't fail rule causes some problems. > 2. If a small allocation cannot fail, what happens if the small memory slab >is exhausted? A new page must be allocated, which will trigger an OOM, >and some other part of the system will be killed and fail. Right. > 3. This driver uses GFP_ATOMIC, is that guaranteed to succeed? I think not. > Right again. I was missing the first email in the thread because of my email filters so I didn't see this was atomic. regards, dan carpenter -- 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: hub: Delete an error message for a failed memory allocation in usb_hub_clear_tt_buffer()
On Thu, Dec 07, 2017 at 08:40:07AM +0100, Geert Uytterhoeven wrote: > Hi Alan, > > On Wed, Dec 6, 2017 at 11:02 PM, Alan Stern wrote: > > On Wed, 6 Dec 2017, SF Markus Elfring wrote: > >> >>> Does the existing memory allocation error message include the > >> >>> &udev->dev device name and driver name? If it doesn't, there will be > >> >>> no way for the user to tell that the error message is related to the > >> >>> device failure. > >> >> > >> >> No, but the effect is similar. > >> >> > >> >> OOM does a dump_stack() so this function's call tree is shown. > >> > > >> > A call stack doesn't tell you which device was being handled. > >> > >> Do you find a default Linux allocation failure report insufficient then? > >> > >> Would you like to to achieve that the requested information can be > >> determined > >> from a backtrace? > > > > It is not practical to do this. The memory allocation routines do not > > for what purpose the memory is being allocated; hence when a failure > > occurs they cannot tell what device (or other part of the system) will > > be affected. > > If even allocation of 24 bytes fails, lots of other devices and other parts of > the system will start failing really soon... > Small allocations never fail in the current kernel. regards, dan carpenter -- 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: emi26: Delete an error message for a failed memory allocation in emi26_writememory()
On Wed, Dec 06, 2017 at 01:12:32PM +0100, Julia Lawall wrote:
>
>
> On Wed, 6 Dec 2017, SF Markus Elfring wrote:
>
> > From: Markus Elfring
> > Date: Wed, 6 Dec 2017 13:03:21 +0100
> >
> > Omit an extra message for a memory allocation failure in this function.
> >
> > This issue was detected by using the Coccinelle software.
> >
> > Signed-off-by: Markus Elfring
> > ---
> > drivers/usb/misc/emi26.c | 5 ++---
> > 1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/usb/misc/emi26.c b/drivers/usb/misc/emi26.c
> > index 24d841850e05..654e93857d65 100644
> > --- a/drivers/usb/misc/emi26.c
> > +++ b/drivers/usb/misc/emi26.c
> > @@ -42,10 +42,9 @@ static int emi26_writememory (struct usb_device *dev,
> > int address,
> > int result;
> > unsigned char *buffer = kmemdup(data, length, GFP_KERNEL);
> >
> > - if (!buffer) {
> > - dev_err(&dev->dev, "kmalloc(%d) failed.\n", length);
>
> I guess the length information would not be so easy to find in the
> backtrace.
>
I think it prints the "order" but not the exact size.
regards,
dan carpeneter
--
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] mmc: vub300: Use common code in __download_offload_pseudocode()
On Mon, Oct 30, 2017 at 02:03:13PM +0100, Ulf Hansson wrote:
> On 30 October 2017 at 13:15, Dan Carpenter wrote:
> > On Mon, Oct 30, 2017 at 12:40:39PM +0100, Ulf Hansson wrote:
> >> On 27 October 2017 at 21:31, SF Markus Elfring
> >> wrote:
> >> > From: Markus Elfring
> >> > Date: Fri, 27 Oct 2017 21:21:40 +0200
> >> >
> >> > Add a jump target so that a specific string copy operation is stored
> >> > only once at the end of this function implementation.
> >> > Replace two calls of the function "strncpy" by goto statements.
> >> >
> >> > This issue was detected by using the Coccinelle software.
> >> >
> >> > Signed-off-by: Markus Elfring
> >>
> >> Thanks, applied for next!
> >>
> >
> > What's the advantage of this patch? The new code seems more complicated
> > to me and GCC automatically reuses duplicate constant strings so there
> > is no memory savings.
>
> It looked to me that the error path got a bit cleaner. However, I
> guess it's matter of taste.
>
> If you insist, I can drop it.
I'm on the kernel-janitors list so I am CC'd on all of Markus's patches.
It's not my code and I'm tired of being the anti-Markus guy so this
patch is fine. Markus has a tool that finds duplicate strings and he
uses gotos to avoid them. I don't think duplicate strings are a problem
or that it's a good idea to send over a hundred patches using this
method. But many people have explained that to Markus already and
that's not the bigger picture which is about error handling and labels.
What I like are labels that are necessary and self explanatory. Things
like "goto unlock" are a good example, because we know we need to unlock
and the goto tells us what the label does. Or here is another example:
foo = alloc_foo();
if (!foo)
return -ENOMEM;
bar = alloc_bar();
if (!foo) {
err = -ENOMEM;
goto free_foo;
}
>From reading the code, you know that you have to free foo and the label
name is clear so you literally don't need to scroll down to the bottom
of the function when you're reading this code. A bad label is like
this:
foo = alloc_foo();
if (!foo)
goto err;
You're reading along and you're like "what happens at the err" label?
So then you have to scroll down to the bottom and read the code, then
you have to think about how the variables line up with the variables
in the above code, then you have to scroll back and find your place
again and by that point you've forgotten what you were doing when you
started.
regards,
dan carpenter
--
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] mmc: vub300: Use common code in __download_offload_pseudocode()
On Mon, Oct 30, 2017 at 12:40:39PM +0100, Ulf Hansson wrote: > On 27 October 2017 at 21:31, SF Markus Elfring > wrote: > > From: Markus Elfring > > Date: Fri, 27 Oct 2017 21:21:40 +0200 > > > > Add a jump target so that a specific string copy operation is stored > > only once at the end of this function implementation. > > Replace two calls of the function "strncpy" by goto statements. > > > > This issue was detected by using the Coccinelle software. > > > > Signed-off-by: Markus Elfring > > Thanks, applied for next! > What's the advantage of this patch? The new code seems more complicated to me and GCC automatically reuses duplicate constant strings so there is no memory savings. regards, dan capenter -- 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: max3421: unlock on error in max3421_hub_control()
On Fri, Oct 27, 2017 at 02:08:38PM +0200, Jules Maselbas wrote: > > > On Fri, Oct 27, 2017 at 11:06:36AM +0200, Jules Maselbas wrote: > >> Hi, > >> > >> > We can't return directly in max3421_hub_control(), we have to unlock > >> > first. > >> Yes you are right. > >> I think testing if pdata is null is not necessary as driver_probe will > >> fail if > >> the device do not have a platform_data. > >> > > > > In that case, let's drop my patch. Could you send a patch for which > > removes the NULL check and add a Reported-by: for me? > It is already in the v2 (and v3) of my initial patch. > https://www.spinics.net/lists/devicetree/msg199901.html > > Should I make a brand new patch (not another version)? Yeah. Greg never rebases once the tree has been shared with the public so you'll need to send a new patch instead of a v3. regards, dan carpenter -- 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: max3421: unlock on error in max3421_hub_control()
On Fri, Oct 27, 2017 at 11:06:36AM +0200, Jules Maselbas wrote: > Hi, > > > We can't return directly in max3421_hub_control(), we have to unlock > > first. > Yes you are right. > I think testing if pdata is null is not necessary as driver_probe will fail if > the device do not have a platform_data. > In that case, let's drop my patch. Could you send a patch for which removes the NULL check and add a Reported-by: for me? regards, dan carpenter -- 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: max3421: unlock on error in max3421_hub_control()
We can't return directly in max3421_hub_control(), we have to unlock
first.
Fixes: 721fdc83b31b ("usb: max3421: Add devicetree support")
Signed-off-by: Dan Carpenter
diff --git a/drivers/usb/host/max3421-hcd.c b/drivers/usb/host/max3421-hcd.c
index 928a5aabee02..46a86a7f3360 100644
--- a/drivers/usb/host/max3421-hcd.c
+++ b/drivers/usb/host/max3421-hcd.c
@@ -1704,7 +1704,8 @@ max3421_hub_control(struct usb_hcd *hcd, u16 type_req,
u16 value, u16 index,
pdata = spi->dev.platform_data;
if (!pdata) {
dev_err(&spi->dev, "Device platform data is missing\n");
- return -EFAULT;
+ retval = -EFAULT;
+ goto unlock;
}
switch (type_req) {
@@ -1787,6 +1788,7 @@ max3421_hub_control(struct usb_hcd *hcd, u16 type_req,
u16 value, u16 index,
retval = -EPIPE;
}
+unlock:
spin_unlock_irqrestore(&max3421_hcd->lock, flags);
return retval;
}
--
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 4.14 REGRESSION fix] USB: devio: Revert "USB: devio: Don't corrupt user memory"
On Mon, Oct 16, 2017 at 03:12:07PM -0400, Alan Stern wrote: > On Mon, 16 Oct 2017, Hans de Goede wrote: > > > Taking the uurb->buffer_length userspace passes in as a maximum for the > > actual urbs transfer_buffer_length causes 2 serious issues: > > > > 1) It breaks isochronous support for all userspace apps using libusb, > >as existing libusb versions pass in 0 for uurb->buffer_length, > >relying on the kernel using the lenghts of the usbdevfs_iso_packet_desc > >descriptors passed in added together as buffer length. > > > >This for example causes redirection of USB audio and Webcam's into > >virtual machines using qemu-kvm to no longer work. This is a userspace > >ABI break and as such must be reverted. > > > >Note that the original commit does not protect other users / the > >kernels memory, it only stops the userspace process making the call > >from shooting itself in the foot. > > Okay, breaking userspace is reason enough all by itself to revert this > change. I didn't realize that libusb sets the buffer_length to 0 for > isochronous URBs. > Yeah... I should have seen how this worked just from reading the code. :/ regards, dan carpenter -- 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
[bug report] usb: gadget: R8A66597 peripheral controller support.
Hello Yoshihiro Shimoda, The patch c41442474a26: "usb: gadget: R8A66597 peripheral controller support." from Aug 19, 2009, leads to the following static checker warning: drivers/usb/gadget/udc/r8a66597-udc.c:1942 r8a66597_probe() warn: integer overflow: (-1) * 2 drivers/usb/gadget/udc/r8a66597-udc.c 1936 } 1937 usb_ep_set_maxpacket_limit(&r8a66597->ep[0].ep, 64); 1938 r8a66597->ep[0].pipenum = 0; 1939 r8a66597->ep[0].fifoaddr = CFIFO; 1940 r8a66597->ep[0].fifosel = CFIFOSEL; 1941 r8a66597->ep[0].fifoctr = CFIFOCTR; 1942 r8a66597->ep[0].pipectr = get_pipectr_addr(0); ^^^ #define get_pipectr_addr(pipenum) (M66592_PIPE1CTR + (pipenum - 1) * 2) It's not normally valid to pass 0 to get_pipectr_addr() because it ends up that we assign "r8a66597->ep[0].pipectr = M66592_PIPE1CTR - 2". 1943 r8a66597->pipenum2ep[0] = &r8a66597->ep[0]; 1944 r8a66597->epaddr2ep[0] = &r8a66597->ep[0]; 1945 Also: drivers/usb/gadget/udc/m66592-udc.c:1653 m66592_probe() warn: integer overflow: (-1) * 2 drivers/usb/gadget/udc/m66592-udc.c 1647 usb_ep_set_maxpacket_limit(&m66592->ep[0].ep, 64); 1648 m66592->ep[0].pipenum = 0; 1649 m66592->ep[0].fifoaddr = M66592_CFIFO; 1650 m66592->ep[0].fifosel = M66592_CFIFOSEL; 1651 m66592->ep[0].fifoctr = M66592_CFIFOCTR; 1652 m66592->ep[0].fifotrn = 0; 1653 m66592->ep[0].pipectr = get_pipectr_addr(0); 1654 m66592->pipenum2ep[0] = &m66592->ep[0]; 1655 m66592->epaddr2ep[0] = &m66592->ep[0]; regards, dan carpenter -- 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: misc: usbtest: Fix overflow in usbtest_do_ioctl()
On Sat, Sep 30, 2017 at 11:04:35AM -0700, Deepa Dinamani wrote:
> On Sat, Sep 30, 2017 at 1:15 AM, Dan Carpenter
> wrote:
> > There used to be a test against "if (param->sglen > MAX_SGLEN)" but it
> > was removed during a refactor. It leads to an integer overflow and a
> > stack overflow in test_queue() if we try to create a too large urbs[]
> > array on the stack.
> >
> > There is a second integer overflow in test_queue() as well if
> > "param->iterations" is too high. I don't immediately see that it's
> > harmful but I've added a check to prevent it and silence the static
> > checker warning.
> >
> > Fixes: 18fc4ebdc705 ("usb: misc: usbtest: Remove timeval usage")
> > Signed-off-by: Dan Carpenter
> >
> > diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
> > index eee82ca55b7b..113e38bfe0ef 100644
> > --- a/drivers/usb/misc/usbtest.c
> > +++ b/drivers/usb/misc/usbtest.c
> > @@ -1964,6 +1964,9 @@ test_queue(struct usbtest_dev *dev, struct
> > usbtest_param_32 *param,
> > int status = 0;
> > struct urb *urbs[param->sglen];
> >
> > + if (!param->sglen || param->iterations > UINT_MAX / param->sglen)
>
> Would adding a comment here that we are checking for overflow of
> context.count make sense?
> It takes some time to figure out that context.count is unsigned and
> hence the check is against UINT_MAX here.
To be honest, this is a very typical sort of integer overflow test and
it looks pretty self explanatory to me. We're avoiding a divide by zero
bug and we're making sure the result isn't more than UINT_MAX.
It has nothing to do with context.count. param->iterations and
param->sglen are both unsinged int, so if we do "param->iterations *
param->sglen" that will overflow.
regards,
dan carpenter
--
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: misc: usbtest: Fix overflow in usbtest_do_ioctl()
There used to be a test against "if (param->sglen > MAX_SGLEN)" but it
was removed during a refactor. It leads to an integer overflow and a
stack overflow in test_queue() if we try to create a too large urbs[]
array on the stack.
There is a second integer overflow in test_queue() as well if
"param->iterations" is too high. I don't immediately see that it's
harmful but I've added a check to prevent it and silence the static
checker warning.
Fixes: 18fc4ebdc705 ("usb: misc: usbtest: Remove timeval usage")
Signed-off-by: Dan Carpenter
diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
index eee82ca55b7b..113e38bfe0ef 100644
--- a/drivers/usb/misc/usbtest.c
+++ b/drivers/usb/misc/usbtest.c
@@ -1964,6 +1964,9 @@ test_queue(struct usbtest_dev *dev, struct
usbtest_param_32 *param,
int status = 0;
struct urb *urbs[param->sglen];
+ if (!param->sglen || param->iterations > UINT_MAX / param->sglen)
+ return -EINVAL;
+
memset(&context, 0, sizeof(context));
context.count = param->iterations * param->sglen;
context.dev = dev;
@@ -2087,6 +2090,8 @@ usbtest_do_ioctl(struct usb_interface *intf, struct
usbtest_param_32 *param)
if (param->iterations <= 0)
return -EINVAL;
+ if (param->sglen > MAX_SGLEN)
+ return -EINVAL;
/*
* Just a bunch of test cases that every HCD is expected to handle.
*
--
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/2] USB: devio: Prevent integer overflow in proc_do_submiturb()
There used to be an integer overflow check in proc_do_submiturb() but
we removed it. It turns out that it's still required. The
uurb->buffer_length variable is a signed integer and it's controlled by
the user. It can lead to an integer overflow when we do:
num_sgs = DIV_ROUND_UP(uurb->buffer_length, USB_SG_SIZE);
If we strip away the macro then that line looks like this:
num_sgs = (uurb->buffer_length + USB_SG_SIZE - 1) / USB_SG_SIZE;
^
It's the first addition which can overflow.
Fixes: 1129d270cbfb ("USB: Increase usbfs transfer limit")
Signed-off-by: Dan Carpenter
---
v2: Cast the ->buffer_length to unsigned int which is more readable than
relying on type promotion.
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 318bb3b96687..4664e543cf2f 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -140,6 +140,9 @@ module_param(usbfs_memory_mb, uint, 0644);
MODULE_PARM_DESC(usbfs_memory_mb,
"maximum MB allowed for usbfs buffers (0 = no limit)");
+/* Hard limit, necessary to avoid arithmetic overflow */
+#define USBFS_XFER_MAX (UINT_MAX / 2 - 100)
+
static atomic64_t usbfs_memory_usage; /* Total memory currently allocated */
/* Check whether it's okay to allocate more memory for a transfer */
@@ -1460,6 +1463,8 @@ static int proc_do_submiturb(struct usb_dev_state *ps,
struct usbdevfs_urb *uurb
USBDEVFS_URB_ZERO_PACKET |
USBDEVFS_URB_NO_INTERRUPT))
return -EINVAL;
+ if ((unsigned int)uurb->buffer_length >= USBFS_XFER_MAX)
+ return -EINVAL;
if (uurb->buffer_length > 0 && !uurb->buffer)
return -EINVAL;
if (!(uurb->type == USBDEVFS_URB_TYPE_CONTROL &&
--
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: devio: Don't corrupt user memory
The user buffer has "uurb->buffer_length" bytes. If the kernel has more information than that, we should truncate it instead of writing past the end of the user's buffer. I added a WARN_ONCE() to help the user debug the issue. Reported-by: Alan Stern Signed-off-by: Dan Carpenter diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c index 318bb3b96687..4664e543cf2f 100644 --- a/drivers/usb/core/devio.c +++ b/drivers/usb/core/devio.c @@ -1571,7 +1576,11 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb totlen += isopkt[u].length; } u *= sizeof(struct usb_iso_packet_descriptor); - uurb->buffer_length = totlen; + if (totlen <= uurb->buffer_length) + uurb->buffer_length = totlen; + else + WARN_ONCE(1, "uurb->buffer_length is too short %d vs %d", + totlen, uurb->buffer_length); break; default: -- 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: devio: Prevent integer overflow in proc_do_submiturb()
On Thu, Sep 21, 2017 at 10:17:03AM -0400, Alan Stern wrote:
> On Thu, 21 Sep 2017, Dan Carpenter wrote:
>
> > There used to be an integer overflow check in proc_do_submiturb() but
> > it accidentally got removed. We need to put it back. The
>
> The removal was deliberate, not accidental. :-)
>
> > uurb->buffer_length variable is a signed integer and it's controlled by
> > the user. It can lead to an integer overflow when we do:
> >
> > num_sgs = DIV_ROUND_UP(uurb->buffer_length, USB_SG_SIZE);
>
> Sorry, I don't understand. How can division by 16384 lead to an
> integer overflow?
>
> Now, I agree that uurb->buffer_length can cause problems. We don't
> check for meaningless negative values on all the execution paths (the
> field should have been unsigned from the beginning).
Checking against USBFS_XFER_MAX prevents negative values but I'll add
another check for readability.
> And in the
> USBDEVFS_URB_TYPE_ISO case, we overwrite uurb->buffer_length without
> checking that the new value is <= the old value, which could lead to a
> userspace buffer overflow.
Hm... Yeah. I'm not sure what's the right thing, should I print a
warning if we truncate the output or would users figure it out on their
own?
if (totlen <= uurb->buffer_length)
uurb->buffer_length = totlen;
else
WARN_ONCE(1, "uurb->buffer_length is too short %d vs %d",
totlen, uurb->buffer_length);
regards,
dan carpenter
--
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: devio: Prevent integer overflow in proc_do_submiturb()
On Thu, Sep 21, 2017 at 10:17:03AM -0400, Alan Stern wrote: > On Thu, 21 Sep 2017, Dan Carpenter wrote: > > > There used to be an integer overflow check in proc_do_submiturb() but > > it accidentally got removed. We need to put it back. The > > The removal was deliberate, not accidental. :-) > > > uurb->buffer_length variable is a signed integer and it's controlled by > > the user. It can lead to an integer overflow when we do: > > > > num_sgs = DIV_ROUND_UP(uurb->buffer_length, USB_SG_SIZE); > > Sorry, I don't understand. How can division by 16384 lead to an > integer overflow? It looks like this when you unwrap the macro: num_sgs = (uurb->buffer_length + USB_SG_SIZE - 1) / USB_SG_SIZE; ^^^^^ This first addition can overflow. regards, dan carpenter -- 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: devio: Prevent integer overflow in proc_do_submiturb()
There used to be an integer overflow check in proc_do_submiturb() but
it accidentally got removed. We need to put it back. The
uurb->buffer_length variable is a signed integer and it's controlled by
the user. It can lead to an integer overflow when we do:
num_sgs = DIV_ROUND_UP(uurb->buffer_length, USB_SG_SIZE);
Fixes: 1129d270cbfb ("USB: Increase usbfs transfer limit")
Signed-off-by: Dan Carpenter
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 318bb3b96687..f3c9cff2101c 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -140,6 +140,9 @@ module_param(usbfs_memory_mb, uint, 0644);
MODULE_PARM_DESC(usbfs_memory_mb,
"maximum MB allowed for usbfs buffers (0 = no limit)");
+/* Hard limit, necessary to avoid arithmetic overflow */
+#define USBFS_XFER_MAX (UINT_MAX / 2 - 100)
+
static atomic64_t usbfs_memory_usage; /* Total memory currently allocated */
/* Check whether it's okay to allocate more memory for a transfer */
@@ -1460,6 +1463,8 @@ static int proc_do_submiturb(struct usb_dev_state *ps,
struct usbdevfs_urb *uurb
USBDEVFS_URB_ZERO_PACKET |
USBDEVFS_URB_NO_INTERRUPT))
return -EINVAL;
+ if (uurb->buffer_length >= USBFS_XFER_MAX)
+ return -EINVAL;
if (uurb->buffer_length > 0 && !uurb->buffer)
return -EINVAL;
if (!(uurb->type == USBDEVFS_URB_TYPE_CONTROL &&
--
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 05/11] mux: Add Intel Cherrytrail USB mux driver
On Sat, Sep 02, 2017 at 01:19:05PM +0300, Andy Shevchenko wrote:
> > +static void intel_cht_usb_mux_vbus_work(struct work_struct *work)
> > +{
> > + struct intel_cht_usb_mux *mux =
> > + container_of(work, struct intel_cht_usb_mux, vbus_work);
> > + bool vbus_present = false;
> > + int i;
>
> unsigned int i; ?
>
int i is fine. Static checkers which complain about stuff like that
should be uninstalled.
> > +
> > + for (i = 0; i < ARRAY_SIZE(vbus_cable_ids); i++) {
regards,
dan carpenter
--
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
[bug report] usb: phy: Add USB charger support
Hello Baolin Wang,
This is a semi-automatic email about new static checker warnings.
The patch a9081a008f84: "usb: phy: Add USB charger support" from Aug
15, 2017, leads to the following Smatch complaint:
include/linux/usb/phy.h:327 usb_phy_set_power()
warn: variable dereferenced before check 'x' (see line 325)
include/linux/usb/phy.h
322 static inline int
323 usb_phy_set_power(struct usb_phy *x, unsigned mA)
324 {
325 usb_phy_set_charger_current(x, mA);
^
The patch adds an unchecked dereference.
326
327 if (x && x->set_power)
^
The existing code checks for NULL. But so far as I know non of the
existing caller pass a NULL usb_phy and it doesn't really make sense
to me to do that. Perhaps just remove the NULL check?
328 return x->set_power(x, mA);
329 return 0;
regards,
dan carpenter
--
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: snps_udc_plat: testing the wrong variable
"udc->regs" is zero here. We don't set it until a couple lines later.
The intent was to test "udc->virt_addr".
Fixes: 1b9f35adb0ff ("usb: gadget: udc: Add Synopsys UDC Platform driver")
Signed-off-by: Dan Carpenter
diff --git a/drivers/usb/gadget/udc/snps_udc_plat.c
b/drivers/usb/gadget/udc/snps_udc_plat.c
index 2e11f19e07ae..b8d1280faafc 100644
--- a/drivers/usb/gadget/udc/snps_udc_plat.c
+++ b/drivers/usb/gadget/udc/snps_udc_plat.c
@@ -122,8 +122,8 @@ static int udc_plat_probe(struct platform_device *pdev)
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
udc->virt_addr = devm_ioremap_resource(dev, res);
- if (IS_ERR(udc->regs))
- return PTR_ERR(udc->regs);
+ if (IS_ERR(udc->virt_addr))
+ return PTR_ERR(udc->virt_addr);
/* udc csr registers base */
udc->csr = udc->virt_addr + UDC_CSR_ADDR;
--
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
[bug report] USB: add Freescale high-speed USB SOC device controller driver
Hello Li Yang,
The patch b504882da539: "USB: add Freescale high-speed USB SOC device
controller driver" from Apr 23, 2007, leads to the following Smatch
warning:
drivers/usb/gadget/udc/fsl_udc_core.c:917 fsl_ep_queue()
warn: variable dereferenced before check 'req' (see line 880)
drivers/usb/gadget/udc/fsl_udc_core.c
899
900 ret = usb_gadget_map_request(&ep->udc->gadget, &req->req,
ep_is_in(ep));
901 if (ret)
902 return ret;
903
904 req->req.status = -EINPROGRESS;
905 req->req.actual = 0;
906 req->dtd_count = 0;
^
907
908 /* build dtds and push them to device queue */
909 if (!fsl_req_to_dtd(req, gfp_flags)) {
910 spin_lock_irqsave(&udc->lock, flags);
911 fsl_queue_td(ep, req);
912 } else {
913 return -ENOMEM;
914 }
915
916 /* irq handler advances the queue */
917 if (req != NULL)
^^^
"req" can't be NULL here, but I'm not sure if something else was
intended?
918 list_add_tail(&req->queue, &ep->queue);
919 spin_unlock_irqrestore(&udc->lock, flags);
920
921 return 0;
regards,
dan carpenter
--
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
[bug report] usbip: vhci-hcd: Set the vhci structure up to work
Hello Yuyang Du,
The patch 03cd00d538a6: "usbip: vhci-hcd: Set the vhci structure up
to work" from Jun 8, 2017, leads to the following static checker
warning:
drivers/usb/usbip/vhci_hcd.c:1355 vhci_hcd_probe()
error: potentially dereferencing uninitialized 'vhci'.
drivers/usb/usbip/vhci_hcd.c
1340 ret = usb_add_hcd(hcd_ss, 0, 0);
1341 if (ret) {
1342 pr_err("usb_add_hcd ss failed %d\n", ret);
1343 goto put_usb3_hcd;
1344 }
1345
1346 usbip_dbg_vhci_hc("bye\n");
1347 return 0;
1348
1349 put_usb3_hcd:
1350 usb_put_hcd(hcd_ss);
1351 remove_usb2_hcd:
1352 usb_remove_hcd(hcd_hs);
1353 put_usb2_hcd:
1354 usb_put_hcd(hcd_hs);
1355 vhci->vhci_hcd_hs = NULL;
^
1356 vhci->vhci_hcd_ss = NULL;
^
vhci is never initialized.
1357 return ret;
1358 }
regards,
dan carpenter
--
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: f_fs: use memdup_user
On Sat, May 13, 2017 at 11:15:59AM +0800, Geliang Tang wrote:
> Use memdup_user() helper instead of open-coding to simplify the code.
>
> Signed-off-by: Geliang Tang
> ---
> drivers/usb/gadget/function/f_fs.c | 11 +++
> 1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_fs.c
> b/drivers/usb/gadget/function/f_fs.c
> index 71dd27c..5754538 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -3692,14 +3692,9 @@ static char *ffs_prepare_buffer(const char __user
> *buf, size_t len)
> if (unlikely(!len))
> return NULL;
>
> - data = kmalloc(len, GFP_KERNEL);
> - if (unlikely(!data))
> - return ERR_PTR(-ENOMEM);
> -
> - if (unlikely(copy_from_user(data, buf, len))) {
> - kfree(data);
> - return ERR_PTR(-EFAULT);
> - }
> + data = memdup_user(buf, len);
> + if (unlikely(IS_ERR(data)))
Don't use likely/unlikely() here. It's not a fast path.
regards,
dan carpenter
--
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: udc-xilinx: clean up a variable name
"ep->udc->lock" and "udc->lock" are the same thing. It confuses Smatch
if we don't use the same name consistently.
Signed-off-by: Dan Carpenter
diff --git a/drivers/usb/gadget/udc/udc-xilinx.c
b/drivers/usb/gadget/udc/udc-xilinx.c
index 588e2531b8b8..de207a90571e 100644
--- a/drivers/usb/gadget/udc/udc-xilinx.c
+++ b/drivers/usb/gadget/udc/udc-xilinx.c
@@ -1151,7 +1151,7 @@ static int xudc_ep_dequeue(struct usb_ep *_ep, struct
usb_request *_req)
break;
}
if (&req->usb_req != _req) {
- spin_unlock_irqrestore(&ep->udc->lock, flags);
+ spin_unlock_irqrestore(&udc->lock, flags);
return -EINVAL;
}
xudc_done(ep, req, -ECONNRESET);
--
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
[bug report] usb: xhci: Add helper function xhci_set_power_on().
Hello Guoqing Zhang,
The patch a6ff6cbf1fab: "usb: xhci: Add helper function
xhci_set_power_on()." from Apr 7, 2017, leads to the following static
checker warning:
drivers/usb/host/xhci-hub.c:575 xhci_set_port_power()
error: calling 'spin_unlock_irqrestore()' with bogus flags
drivers/usb/host/xhci-hub.c
554 static void xhci_set_port_power(struct xhci_hcd *xhci, struct usb_hcd
*hcd,
555 u16 index, bool on)
556 {
557 __le32 __iomem *addr;
558 u32 temp;
559 unsigned long flags = 0;
560
561 addr = xhci_get_port_io_addr(hcd, index);
562 temp = readl(addr);
563 temp = xhci_port_state_to_neutral(temp);
564 if (on) {
565 /* Power on */
566 writel(temp | PORT_POWER, addr);
567 temp = readl(addr);
568 xhci_dbg(xhci, "set port power, actual port %d status
= 0x%x\n",
569 index, temp);
570 } else {
571 /* Power off */
572 writel(temp & ~PORT_POWER, addr);
573 }
574
575 spin_unlock_irqrestore(&xhci->lock, flags);
^
This won't work at all. This is zero, not the original flags. Please
CC me on the fix because I'm not subscribed to linux-usb.
576 temp = usb_acpi_power_manageable(hcd->self.root_hub,
577 index);
578 if (temp)
579 usb_acpi_set_power_state(hcd->self.root_hub,
580 index, on);
581 spin_lock_irqsave(&xhci->lock, flags);
582 }
regards,
dan carpenter
--
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
[bug report] usb: gadget: f_hid: fix: Move IN request allocation to set_alt()
Hello Krzysztof Opasiak,
This is a semi-automatic email about new static checker warnings.
The patch 749494b6bdbb: "usb: gadget: f_hid: fix: Move IN request
allocation to set_alt()" from Jan 24, 2017, leads to the following
Smatch complaint:
drivers/usb/gadget/function/f_hid.c:382 f_hidg_write()
warn: variable dereferenced before check 'hidg->req' (see line 370)
drivers/usb/gadget/function/f_hid.c
369 spin_unlock_irqrestore(&hidg->write_spinlock, flags);
370 status = copy_from_user(hidg->req->buf, buffer, count);
^^
Dereference.
371
372 if (status != 0) {
373 ERROR(hidg->func.config->cdev,
374 "copy_from_user error\n");
375 status = -EINVAL;
376 goto release_write_pending;
377 }
378
379 spin_lock_irqsave(&hidg->write_spinlock, flags);
380
381 /* we our function has been disabled by host */
382 if (!hidg->req) {
^
Check too late.
383 free_ep_req(hidg->in_ep, hidg->req);
^
This is also a dereference.
384 /*
regards,
dan carpenter
--
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
[bug report] USB: opticon: switch to generic read implementation
Hello Johan Hovold,
The patch 7a6ee2b02751: "USB: opticon: switch to generic read
implementation" from Nov 18, 2012, leads to the following static
checker warning:
drivers/usb/serial/opticon.c:146 opticon_open()
info: return a literal instead of 'res'
drivers/usb/serial/opticon.c
128 static int opticon_open(struct tty_struct *tty, struct usb_serial_port
*port)
129 {
130 struct opticon_private *priv = usb_get_serial_port_data(port);
131 unsigned long flags;
132 int res;
133
134 spin_lock_irqsave(&priv->lock, flags);
135 priv->rts = false;
136 spin_unlock_irqrestore(&priv->lock, flags);
137
138 /* Clear RTS line */
139 send_control_msg(port, CONTROL_RTS, 0);
140
141 /* clear the halt status of the endpoint */
142 usb_clear_halt(port->serial->dev, port->read_urb->pipe);
143
144 res = usb_serial_generic_open(tty, port);
145 if (!res)
146 return res;
I think this if (!ret) statement is reversed. In the original code we
used to send RESEND_CTS_STATE on both the failure and success paths.
147
148 /* Request CTS line state, sometimes during opening the current
149 * CTS state can be missed. */
150 send_control_msg(port, RESEND_CTS_STATE, 1);
151
152 return res;
153 }
regards,
dan carpenter
--
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
[bug report] [ARM] ohci-pxa27x: use ioremap() and offset for register access
Hi USB devs,
I have no idea who to contact for this, the bug predates git.
drivers/usb/host/ohci-pxa27x.c:150 pxa27x_ohci_select_pmm()
warn: odd binop '0x200 & 0x100'
drivers/usb/host/ohci-pxa27x.c
130 /*
131PMM_NPS_MODE -- PMM Non-power switching mode
132Ports are powered continuously.
133
134PMM_GLOBAL_MODE -- PMM global switching mode
135All ports are powered at the same time.
136
137PMM_PERPORT_MODE -- PMM per port switching mode
138Ports are powered individually.
139 */
140 static int pxa27x_ohci_select_pmm(struct pxa27x_ohci *pxa_ohci, int
mode)
141 {
142 uint32_t uhcrhda = __raw_readl(pxa_ohci->mmio_base + UHCRHDA);
143 uint32_t uhcrhdb = __raw_readl(pxa_ohci->mmio_base + UHCRHDB);
144
145 switch (mode) {
146 case PMM_NPS_MODE:
147 uhcrhda |= RH_A_NPS;
148 break;
149 case PMM_GLOBAL_MODE:
150 uhcrhda &= ~(RH_A_NPS & RH_A_PSM);
My guess is that | was intended instead of & here, but I am not sure.
151 break;
152 case PMM_PERPORT_MODE:
153 uhcrhda &= ~(RH_A_NPS);
154 uhcrhda |= RH_A_PSM;
155
156 /* Set port power control mask bits, only 3 ports. */
157 uhcrhdb |= (0x7<<17);
158 break;
159 default:
160 printk( KERN_ERR
161 "Invalid mode %d, set to non-power switch
mode.\n",
162 mode );
163
164 uhcrhda |= RH_A_NPS;
165 }
166
167 __raw_writel(uhcrhda, pxa_ohci->mmio_base + UHCRHDA);
168 __raw_writel(uhcrhdb, pxa_ohci->mmio_base + UHCRHDB);
169 return 0;
170 }
regards,
dan carpenter
--
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: xhci-mem: use passed in GFP flags instead of GFP_KERNEL
We normally use the passed in gfp flags for allocations, it's just these
two which were missed.
Fixes: 22d45f01a836 ("usb/xhci: replace pci_*_consistent() with
dma_*_coherent()")
Signed-off-by: Dan Carpenter
---
>From static analysis. Not tested.
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 6afe323..3c4cc29 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -2384,7 +2384,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
* "physically contiguous and 64-byte (cache line) aligned".
*/
xhci->dcbaa = dma_alloc_coherent(dev, sizeof(*xhci->dcbaa), &dma,
- GFP_KERNEL);
+ flags);
if (!xhci->dcbaa)
goto fail;
memset(xhci->dcbaa, 0, sizeof *(xhci->dcbaa));
@@ -2480,7 +2480,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
xhci->erst.entries = dma_alloc_coherent(dev,
sizeof(struct xhci_erst_entry) * ERST_NUM_SEGS, &dma,
- GFP_KERNEL);
+ flags);
if (!xhci->erst.entries)
goto fail;
xhci_dbg_trace(xhci, trace_xhci_dbg_init,
--
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: usbip: vhci extension: modifications to vhci driver
On Wed, Oct 12, 2016 at 05:24:31AM +, fx IWATA NOBUO wrote: > Hello, > > I will send a patch to clear this warning. > > The current behavior is as following: > vdev_to_vhci() is inline of container_of(). > A pointer (struct vhci_hcd *vhci) may be container_of() from NULL for a > while. > If it is container_of() from NULL, it will not be referenced because of > NULL check of source pointer of the container_of(). Are you looking at linux-next? vdev_to_vhci() derefernces "vdev" to get vdev->rhport so this is a bug and not a false positive. Smatch sometimes does have false positives because it thinks foo->array is a dereference when really we're taking the address of the array. I should fix that... But it understands that container_of(NULL) is ok. regards, dan carpenter -- 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
[bug report] usb: gadget: f_fs: handle control requests not directed to interface or endpoint
Hello Felix Hädicke,
The patch 54dfce6d07b0: "usb: gadget: f_fs: handle control requests
not directed to interface or endpoint" from Jun 22, 2016, leads to
the following static checker warning:
drivers/usb/gadget/function/f_fs.c:3152 ffs_func_req_match()
warn: always true condition '(((creq->wIndex)) >= 0) => (0-u16max >= 0)'
drivers/usb/gadget/function/f_fs.c
3140 static bool ffs_func_req_match(struct usb_function *f,
3141 const struct usb_ctrlrequest *creq,
3142 bool config0)
3143 {
3144 struct ffs_function *func = ffs_func_from_usb(f);
3145
3146 if (config0 && !(func->ffs->user_flags &
FUNCTIONFS_CONFIG0_SETUP))
3147 return false;
3148
3149 switch (creq->bRequestType & USB_RECIP_MASK) {
3150 case USB_RECIP_INTERFACE:
3151 return ffs_func_revmap_intf(func,
3152 le16_to_cpu(creq->wIndex)
>= 0);
^
3153 case USB_RECIP_ENDPOINT:
3154 return ffs_func_revmap_ep(func,
3155le16_to_cpu(creq->wIndex) >=
0);
^^
This doesn't work, but it's not even clear to me what we are trying to
do here.
3156 default:
3157 return (bool) (func->ffs->user_flags &
3158 FUNCTIONFS_ALL_CTRL_RECIP);
3159 }
3160 }
regards,
dan carpenter
--
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: usbip: vhci extension: modifications to vhci driver
Hello Nobuo Iwata,
This is a semi-automatic email about new static checker warnings.
The patch 0775a9cbc694: "usbip: vhci extension: modifications to vhci
driver" from Jun 13, 2016, leads to the following Smatch complaint:
drivers/usb/usbip/vhci_hcd.c:466 vhci_tx_urb()
warn: variable dereferenced before check 'vdev' (see line 463)
drivers/usb/usbip/vhci_hcd.c
462 struct vhci_priv *priv;
463 struct vhci_hcd *vhci = vdev_to_vhci(vdev);
^^
Patch adds a new dereference inside the vdev_to_vhci() function.
464 unsigned long flags;
465
466 if (!vdev) {
^
Old code assumed "vdev" could be NULL.
467 pr_err("could not get virtual device");
468 return;
regards,
dan carpenter
--
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: fsl_qe_udc: signedness bug in qe_get_frame()
We can't assign -EINVAL to a u16.
Fixes: 3948f0e0c999 ('usb: add Freescale QE/CPM USB peripheral controller
driver')
Signed-off-by: Dan Carpenter
diff --git a/drivers/usb/gadget/udc/fsl_qe_udc.c
b/drivers/usb/gadget/udc/fsl_qe_udc.c
index 93d28cb..901366f 100644
--- a/drivers/usb/gadget/udc/fsl_qe_udc.c
+++ b/drivers/usb/gadget/udc/fsl_qe_udc.c
@@ -1878,11 +1878,8 @@ static int qe_get_frame(struct usb_gadget *gadget)
tmp = in_be16(&udc->usb_param->frame_n);
if (tmp & 0x8000)
- tmp = tmp & 0x07ff;
- else
- tmp = -EINVAL;
-
- return (int)tmp;
+ return tmp & 0x07ff;
+ return -EINVAL;
}
static int fsl_qe_start(struct usb_gadget *gadget,
--
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: fsl_qe_udc: off by one in setup_received_handle()
The udc->eps[] array has USB_MAX_ENDPOINTS elements so > should be >=.
Fixes: 3948f0e0c999 ('usb: add Freescale QE/CPM USB peripheral controller
driver')
Signed-off-by: Dan Carpenter
diff --git a/drivers/usb/gadget/udc/fsl_qe_udc.c
b/drivers/usb/gadget/udc/fsl_qe_udc.c
index 93d28cb..cf8819a 100644
--- a/drivers/usb/gadget/udc/fsl_qe_udc.c
+++ b/drivers/usb/gadget/udc/fsl_qe_udc.c
@@ -2053,7 +2053,7 @@ static void setup_received_handle(struct qe_udc *udc,
struct qe_ep *ep;
if (wValue != 0 || wLength != 0
- || pipe > USB_MAX_ENDPOINTS)
+ || pipe >= USB_MAX_ENDPOINTS)
break;
ep = &udc->eps[pipe];
--
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: host: Faraday fotg210-hcd driver
Hello Feng-Hsin Chiang,
The patch 7d50195f6c50: "usb: host: Faraday fotg210-hcd driver" from
Jul 29, 2013, leads to the following static checker warning:
drivers/usb/host/fotg210-hcd.c:3967 iso_stream_init()
warn: mask and shift to zero
drivers/usb/host/fotg210-hcd.c
3944 static void iso_stream_init(struct fotg210_hcd *fotg210,
3945 struct fotg210_iso_stream *stream, struct usb_device
*dev,
3946 int pipe, unsigned interval)
3947 {
3948 u32 buf1;
3949 unsigned epnum, maxp;
3950 int is_input;
3951 long bandwidth;
3952 unsigned multi;
3953
3954 /*
3955 * this might be a "high bandwidth" highspeed endpoint,
3956 * as encoded in the ep descriptor's wMaxPacket field
3957 */
3958 epnum = usb_pipeendpoint(pipe);
3959 is_input = usb_pipein(pipe) ? USB_DIR_IN : 0;
3960 maxp = usb_maxpacket(dev, pipe, !is_input);
3961 if (is_input)
3962 buf1 = (1 << 11);
3963 else
3964 buf1 = 0;
3965
3966 maxp = max_packet(maxp);
3967 multi = hb_mult(maxp);
"multi" is always one. max_packet() takes the lower 11 bits and
hb_mult() shifts it to zero and adds one. Maybe introduce another
variable or calculate hb_mult() before max_packet().
3968 buf1 |= maxp;
3969 maxp *= multi;
3970
3971 stream->buf0 = cpu_to_hc32(fotg210, (epnum << 8) | dev->devnum);
3972 stream->buf1 = cpu_to_hc32(fotg210, buf1);
3973 stream->buf2 = cpu_to_hc32(fotg210, multi);
regards,
dan carpenter
--
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: f_fs: check for allocation failure
Return -ENOMEM if kmalloc() fails.
Fixes: 9353afbbfa7b ('usb: gadget: f_fs: buffer data from ‘oversized’ OUT
requests')
Signed-off-by: Dan Carpenter
diff --git a/drivers/usb/gadget/function/f_fs.c
b/drivers/usb/gadget/function/f_fs.c
index a91fcb0..5c8429f 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -775,6 +775,8 @@ static ssize_t __ffs_epfile_read_data(struct ffs_epfile
*epfile,
data_len -= ret;
buf = kmalloc(sizeof(*buf) + data_len, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
buf->length = data_len;
buf->data = buf->storage;
memcpy(buf->storage, data + ret, data_len);
--
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: f_fs: off by one bug in _ffs_func_bind()
On Sat, May 28, 2016 at 12:15:24PM +0200, Michal Nazarewicz wrote: > On Sat, May 28 2016, Dan Carpenter wrote: > > Also in the kernel we have to declare variables at the start of the > > block. > > /me shrugs > > I looked at this out of curiosity and there are precedents: > > $ git grep 'for (\(int\|unsigned\|signed\|long\|char\)[[:space:]]' |wc -l > 19 > > (albeit mostly in tools and scripts), CodingStyle is silent on that > front and checkpatch.pl doesn’t complain about it. There is one in Documentation, the rest are basically all in tools or ifdef 0. Otherwise it causes a GCC warning. regards, dan carpenter -- 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: f_fs: off by one bug in _ffs_func_bind()
On Sat, May 28, 2016 at 12:15:24PM +0200, Michal Nazarewicz wrote: > On Sat, May 28 2016, Dan Carpenter wrote: > > Also in the kernel we have to declare variables at the start of the > > block. > > /me shrugs > > I looked at this out of curiosity and there are precedents: > > $ git grep 'for (\(int\|unsigned\|signed\|long\|char\)[[:space:]]' |wc -l > 19 > > (albeit mostly in tools and scripts), CodingStyle is silent on that > front and checkpatch.pl doesn’t complain about it. Try compiling the code you suggested. regards, dan carpenter -- 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: f_fs: off by one bug in _ffs_func_bind()
This loop is supposed to set all the .num[] values to -1 but it's off by
one so it skips the first element and sets one element past the end of
the array.
I've cleaned up the loop a little as well.
Fixes: ddf8abd25994 ('USB: f_fs: the FunctionFS driver')
Signed-off-by: Dan Carpenter
---
v2: move the eps_ptr assignment outside the loop.
diff --git a/drivers/usb/gadget/function/f_fs.c
b/drivers/usb/gadget/function/f_fs.c
index 73515d5..d26eb64 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -2729,6 +2729,7 @@ static int _ffs_func_bind(struct usb_configuration *c,
func->ffs->ss_descs_count;
int fs_len, hs_len, ss_len, ret, i;
+ struct ffs_ep *eps_ptr;
/* Make it a single chunk, less management later on */
vla_group(d);
@@ -2777,12 +2778,9 @@ static int _ffs_func_bind(struct usb_configuration *c,
ffs->raw_descs_length);
memset(vla_ptr(vlabuf, d, inums), 0xff, d_inums__sz);
- for (ret = ffs->eps_count; ret; --ret) {
- struct ffs_ep *ptr;
-
- ptr = vla_ptr(vlabuf, d, eps);
- ptr[ret].num = -1;
- }
+ eps_ptr = vla_ptr(vlabuf, d, eps);
+ for (i = 0; i < ffs->eps_count; i++)
+ eps_ptr[i].num = -1;
/* Save pointers
* d_eps == vlabuf, func->eps used to kfree vlabuf later
--
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: f_fs: off by one bug in _ffs_func_bind()
On Fri, May 27, 2016 at 07:25:30PM +0200, walter harms wrote:
>
>
> Am 27.05.2016 14:23, schrieb Michal Nazarewicz:
> > On Fri, May 27 2016, Dan Carpenter wrote:
> >> diff --git a/drivers/usb/gadget/function/f_fs.c
> >> b/drivers/usb/gadget/function/f_fs.c
> >> index 73515d5..7fff81a 100644
> >> --- a/drivers/usb/gadget/function/f_fs.c
> >> +++ b/drivers/usb/gadget/function/f_fs.c
> >> @@ -2777,11 +2777,11 @@ static int _ffs_func_bind(struct usb_configuration
> >> *c,
> >> ffs->raw_descs_length);
> >>
> >>memset(vla_ptr(vlabuf, d, inums), 0xff, d_inums__sz);
> >> - for (ret = ffs->eps_count; ret; --ret) {
> >> + for (i = 0; i < ffs->eps_count; i++) {
> >>struct ffs_ep *ptr;
> >>
> >>ptr = vla_ptr(vlabuf, d, eps);
> >
> > As pointed by Walter, this could be moved outside. Maybe
> >
> > i = ffs->eps_count;
> > for (struct ffs_ep *ptr = vla_ptr(vlabuf, d, eps); i; ++ptr, --i)
> > ptr->num = -1;
> >
>
> I think staying with an array here improves readability.
>
I'm surprised you didn't comment on the --i. I was thinking about you
when I changed it to a ++ loop. Also in the kernel we have to declare
variables at the start of the block.
Anyway, let me send a v2.
regards,
dan carpenter
--
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: f_fs: off by one bug in _ffs_func_bind()
This loop is supposed to set all the .num values to -1 but it's doesn't
set the first element and it sets one element beyond the end of the
array. Really there is no reason for it to be done backwards. And
"ret" is the wrong variable to use for an iterator.
Fixes: ddf8abd25994 ('USB: f_fs: the FunctionFS driver')
Signed-off-by: Dan Carpenter
---
I just spotted this reviewing the code, I have not tested it. Please
review carefully, the vla_ptr() macro is difficult to understand.
diff --git a/drivers/usb/gadget/function/f_fs.c
b/drivers/usb/gadget/function/f_fs.c
index 73515d5..7fff81a 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -2777,11 +2777,11 @@ static int _ffs_func_bind(struct usb_configuration *c,
ffs->raw_descs_length);
memset(vla_ptr(vlabuf, d, inums), 0xff, d_inums__sz);
- for (ret = ffs->eps_count; ret; --ret) {
+ for (i = 0; i < ffs->eps_count; i++) {
struct ffs_ep *ptr;
ptr = vla_ptr(vlabuf, d, eps);
- ptr[ret].num = -1;
+ ptr[i].num = -1;
}
/* Save pointers
--
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] usbnet: smsc95xx: silence an uninitialized variable warning
If the call to fn() fails then "buf" is uninitialized. Just return the
error code in that case.
Signed-off-by: Dan Carpenter
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 2edc2bc..d9d2806 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -92,9 +92,11 @@ static int __must_check __smsc95xx_read_reg(struct usbnet
*dev, u32 index,
ret = fn(dev, USB_VENDOR_REQUEST_READ_REGISTER, USB_DIR_IN
| USB_TYPE_VENDOR | USB_RECIP_DEVICE,
0, index, &buf, 4);
- if (unlikely(ret < 0))
+ if (unlikely(ret < 0)) {
netdev_warn(dev->net, "Failed to read reg index 0x%08x: %d\n",
index, ret);
+ return ret;
+ }
le32_to_cpus(&buf);
*data = 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
[patch] usbnet/smsc75xx: silence uninitialized variable warning
If the fn() calls fail then "buf" is uninitialized. Just return early
in that situation.
Signed-off-by: Dan Carpenter
diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c
index c369db9..9af9799 100644
--- a/drivers/net/usb/smsc75xx.c
+++ b/drivers/net/usb/smsc75xx.c
@@ -99,9 +99,11 @@ static int __must_check __smsc75xx_read_reg(struct usbnet
*dev, u32 index,
ret = fn(dev, USB_VENDOR_REQUEST_READ_REGISTER, USB_DIR_IN
| USB_TYPE_VENDOR | USB_RECIP_DEVICE,
0, index, &buf, 4);
- if (unlikely(ret < 0))
+ if (unlikely(ret < 0)) {
netdev_warn(dev->net, "Failed to read reg index 0x%08x: %d\n",
index, ret);
+ return ret;
+ }
le32_to_cpus(&buf);
*data = 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
[patch] usb: dwc3: gadget: fix mask and shift order in DWC3_DCFG_NUMP()
In the original DWC3_DCFG_NUMP() was always zero. It looks like the
intent was to shift first and then do the mask.
Fixes: 2a58f9c12bb3 ('usb: dwc3: gadget: disable automatic calculation of ACK
TP NUMP')
Signed-off-by: Dan Carpenter
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 186a886..7ddf944 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -277,7 +277,7 @@
#define DWC3_DCFG_FULLSPEED1 (3 << 0)
#define DWC3_DCFG_NUMP_SHIFT 17
-#define DWC3_DCFG_NUMP(n) (((n) & 0x1f) >> DWC3_DCFG_NUMP_SHIFT)
+#define DWC3_DCFG_NUMP(n) (((n) >> DWC3_DCFG_NUMP_SHIFT) & 0x1f)
#define DWC3_DCFG_NUMP_MASK(0x1f << DWC3_DCFG_NUMP_SHIFT)
#define DWC3_DCFG_LPM_CAP (1 << 22)
--
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/8] staging: rtl8192u: check return value eprom_read
On Thu, Apr 28, 2016 at 06:53:14PM +0100, Salah Triki wrote:
> The call of eprom_read may fail, therefore its return value must be
> checked.
>
> Signed-off-by: Salah Triki
> ---
> drivers/staging/rtl8192u/r8192U_core.c | 147
> +++--
> 1 file changed, 104 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/staging/rtl8192u/r8192U_core.c
> b/drivers/staging/rtl8192u/r8192U_core.c
> index 3a93218..1c09c61 100644
> --- a/drivers/staging/rtl8192u/r8192U_core.c
> +++ b/drivers/staging/rtl8192u/r8192U_core.c
> @@ -2432,6 +2432,7 @@ static inline u16 endian_swap(u16 *data)
> *data = (tmp >> 8) | (tmp << 8);
> return *data;
> }
> +
Unrelated change.
> static void rtl8192_read_eeprom_info(struct net_device *dev)
> {
> u16 wEPROM_ID = 0;
> @@ -2440,9 +2441,13 @@ static void rtl8192_read_eeprom_info(struct net_device
> *dev)
> struct r8192_priv *priv = ieee80211_priv(dev);
> u16 tmpValue = 0;
> int i;
> + int ret;
>
> RT_TRACE(COMP_EPROM, "===>%s()\n", __func__);
> - wEPROM_ID = eprom_read(dev, 0); /* first read EEPROM ID out; */
> + ret = eprom_read(dev, 0); /* first read EEPROM ID out; */
> + if (ret)
> + return;
> + wEPROM_ID = (u16)ret;
This is wrong and nonsense.
I'm not reviewing the rest of the patch series.
regards,
dan carpenter
--
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
