[PATCH] drivers: usb-misc: sisusbvga: remove dead code
The condition modex % 16 cannot be true when modex value is equal to 640 The condition du & 0xff cannot be true when du value is equal to 0x1400 Addresses-Coverity-Id: 101163 Addresses-Coverity-Id: 744373 Signed-off-by: Gustavo A. R. Silva --- drivers/usb/misc/sisusbvga/sisusb.c | 6 -- 1 file changed, 6 deletions(-) diff --git a/drivers/usb/misc/sisusbvga/sisusb.c b/drivers/usb/misc/sisusbvga/sisusb.c index 05bd39d..440d7fe 100644 --- a/drivers/usb/misc/sisusbvga/sisusb.c +++ b/drivers/usb/misc/sisusbvga/sisusb.c @@ -1831,16 +1831,10 @@ static int sisusb_set_default_mode(struct sisusb_usb_data *sisusb, SETIREGANDOR(SISCR, 0x09, 0x5f, ((crtcdata[16] & 0x01) << 5)); SETIREG(SISCR, 0x14, 0x4f); du = (modex / 16) * (bpp * 2); /* offset/pitch */ - if (modex % 16) - du += bpp; - SETIREGANDOR(SISSR, 0x0e, 0xf0, ((du >> 8) & 0x0f)); SETIREG(SISCR, 0x13, (du & 0xff)); du <<= 5; tmp8 = du >> 8; - if (du & 0xff) - tmp8++; - SETIREG(SISSR, 0x10, tmp8); SETIREG(SISSR, 0x31, 0x00); /* VCLK */ SETIREG(SISSR, 0x2b, 0x1b); -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] drivers: usb: early: remove unused code
Remove this line of code because devnum is overwritten before it can be used.
This could happen if line of code 609 (goto try_again;) is executed. Otherwise,
devnum is never used again.
Addresses-Coverity-ID: 1226870
Signed-off-by: Gustavo A. R. Silva
---
drivers/usb/early/ehci-dbgp.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/usb/early/ehci-dbgp.c b/drivers/usb/early/ehci-dbgp.c
index ea73afb..e265444 100644
--- a/drivers/usb/early/ehci-dbgp.c
+++ b/drivers/usb/early/ehci-dbgp.c
@@ -580,7 +580,6 @@ static int _dbgp_external_startup(void)
USB_DEBUG_DEVNUM);
goto err;
}
- devnum = USB_DEBUG_DEVNUM;
dbgp_printk("debug device renamed to 127\n");
}
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] drivers: usb: gadget: udc: add missing break in switch
Add missing break in switch. Addresses-Coverity-ID: 201385 Signed-off-by: Gustavo A. R. Silva --- drivers/usb/gadget/udc/mv_udc_core.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/gadget/udc/mv_udc_core.c b/drivers/usb/gadget/udc/mv_udc_core.c index 27ebb0d..56b3574 100644 --- a/drivers/usb/gadget/udc/mv_udc_core.c +++ b/drivers/usb/gadget/udc/mv_udc_core.c @@ -489,6 +489,7 @@ static int mv_ep_enable(struct usb_ep *_ep, break; case USB_ENDPOINT_XFER_CONTROL: ios = 1; + break; case USB_ENDPOINT_XFER_INT: mult = 0; break; -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] drivers: usb: gadget: udc: remove logically dead code
Remove unnecesary code because zlt never evaluates to zero. Addresses-Coverity-ID: 1226747 Signed-off-by: Gustavo A. R. Silva --- drivers/usb/gadget/udc/mv_udc_core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/gadget/udc/mv_udc_core.c b/drivers/usb/gadget/udc/mv_udc_core.c index 56b3574..9ca6d92 100644 --- a/drivers/usb/gadget/udc/mv_udc_core.c +++ b/drivers/usb/gadget/udc/mv_udc_core.c @@ -509,7 +509,7 @@ static int mv_ep_enable(struct usb_ep *_ep, dqh = ep->dqh; dqh->max_packet_length = (max << EP_QUEUE_HEAD_MAX_PKT_LEN_POS) | (mult << EP_QUEUE_HEAD_MULT_POS) - | (zlt ? EP_QUEUE_HEAD_ZLT_SEL : 0) + | EP_QUEUE_HEAD_ZLT_SEL | (ios ? EP_QUEUE_HEAD_IOS : 0); dqh->next_dtd_ptr = 1; dqh->size_ioc_int_sts = 0; -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] drivers: usb: gadget: udc: add missing break in switch
Hello Felipe,
Quoting Felipe Balbi :
Hi,
"Gustavo A. R. Silva" writes:
Add missing break in switch.
Addresses-Coverity-ID: 201385
Signed-off-by: Gustavo A. R. Silva
---
drivers/usb/gadget/udc/mv_udc_core.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/gadget/udc/mv_udc_core.c
b/drivers/usb/gadget/udc/mv_udc_core.c
index 27ebb0d..56b3574 100644
--- a/drivers/usb/gadget/udc/mv_udc_core.c
+++ b/drivers/usb/gadget/udc/mv_udc_core.c
@@ -489,6 +489,7 @@ static int mv_ep_enable(struct usb_ep *_ep,
break;
case USB_ENDPOINT_XFER_CONTROL:
ios = 1;
+ break;
are you SURE this is supposed to have this break statement? What if we
want to initialize mult to 0 *also* for control endpoints? How did you
test this? Do you have access to Marvel's documentation for this
controller?
Certainly I wasn't sure, but I also think this is kind of obscure
code. If that is the case that we also want to initialize mult to 0,
wouldn't it be clearer (for maintenance purposes) to add mult = 0 and
the break statement after ios = 1?
What do you think if I modify that piece of code as follows:
case USB_ENDPOINT_XFER_CONTROL:
ios = 1;
mult = 0;
break;
Thank you
--
Gustavo A. R. Silva
--
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] drivers: usb: gadget: udc: remove pointer dereference after free
Remove pointer dereference and write after free. Addresses-Coverity-ID: 1091173 Signed-off-by: Gustavo A. R. Silva --- drivers/usb/gadget/udc/pch_udc.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/usb/gadget/udc/pch_udc.c b/drivers/usb/gadget/udc/pch_udc.c index a97da64..8a365aa 100644 --- a/drivers/usb/gadget/udc/pch_udc.c +++ b/drivers/usb/gadget/udc/pch_udc.c @@ -1523,7 +1523,6 @@ static void pch_udc_free_dma_chain(struct pch_udc_dev *dev, td = phys_to_virt(addr); addr2 = (dma_addr_t)td->next; pci_pool_free(dev->data_requests, td, addr); - td->next = 0x00; addr = addr2; } req->chain_len = 1; -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] drivers: usb: gadget: udc: remove pointer dereference after free
Hi Andy, Quoting Andy Shevchenko : On Wed, 2017-02-08 at 13:15 -0600, Gustavo A. R. Silva wrote: Remove pointer dereference and write after free. It's wrong description. There is no write after free. The memory is still in pool and one may access it. Though the access is *formally* illegal. Code itself looks interesting. Addresses-Coverity-ID: 1091173 Signed-off-by: Gustavo A. R. Silva --- drivers/usb/gadget/udc/pch_udc.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/usb/gadget/udc/pch_udc.c b/drivers/usb/gadget/udc/pch_udc.c index a97da64..8a365aa 100644 --- a/drivers/usb/gadget/udc/pch_udc.c +++ b/drivers/usb/gadget/udc/pch_udc.c @@ -1523,7 +1523,6 @@ static void pch_udc_free_dma_chain(struct pch_udc_dev *dev, td = phys_to_virt(addr); addr2 = (dma_addr_t)td->next; pci_pool_free(dev->data_requests, td, addr); - td->next = 0x00; I think the better fix is to move this line before pci_pool_free() call. I dunno those td->next = 0x00; make any sense there. Yeah, I think it is better to do as you say. It seems to me that the intention of that line of code is to get rid of the address 'next' points to, after it has been backed up into 'addr2'. I will also change the description. Is it done under some lock / serialization? addr = addr2; } req->chain_len = 1; -- Andy Shevchenko Intel Finland Oy Thanks -- Gustavo A. R. Silva -- 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
drivers: usb: dwc3: A question...
Hello everybody,
I ran into the following piece of code at
drivers/usb/dwc3/dwc3-omap.c:218 (linux-next)
218static void dwc3_omap_set_mailbox(struct dwc3_omap *omap,
219enum omap_dwc3_vbus_id_status status)
220{
221int ret;
222u32 val;
223
224switch (status) {
225case OMAP_DWC3_ID_GROUND:
226if (omap->vbus_reg) {
227ret = regulator_enable(omap->vbus_reg);
228if (ret) {
229dev_err(omap->dev, "regulator
enable failed\n");
230return;
231}
232}
233
234val = dwc3_omap_read_utmi_ctrl(omap);
235val &= ~USBOTGSS_UTMI_OTG_CTRL_IDDIG;
236dwc3_omap_write_utmi_ctrl(omap, val);
237break;
238
239case OMAP_DWC3_VBUS_VALID:
240val = dwc3_omap_read_utmi_ctrl(omap);
241val &= ~USBOTGSS_UTMI_OTG_CTRL_SESSEND;
242val |= USBOTGSS_UTMI_OTG_CTRL_VBUSVALID
243| USBOTGSS_UTMI_OTG_CTRL_SESSVALID;
244dwc3_omap_write_utmi_ctrl(omap, val);
245break;
246
247case OMAP_DWC3_ID_FLOAT:
248if (omap->vbus_reg)
249regulator_disable(omap->vbus_reg);
250val = dwc3_omap_read_utmi_ctrl(omap);
251val |= USBOTGSS_UTMI_OTG_CTRL_IDDIG;
252dwc3_omap_write_utmi_ctrl(omap, val);
253
254case OMAP_DWC3_VBUS_OFF:
255val = dwc3_omap_read_utmi_ctrl(omap);
256val &= ~(USBOTGSS_UTMI_OTG_CTRL_SESSVALID
257| USBOTGSS_UTMI_OTG_CTRL_VBUSVALID);
258val |= USBOTGSS_UTMI_OTG_CTRL_SESSEND;
259dwc3_omap_write_utmi_ctrl(omap, val);
260break;
261
262default:
263dev_WARN(omap->dev, "invalid state\n");
264}
265}
The thing is that the case for OMAP_DWC3_ID_FLOAT is not terminated by
a break statement, and it falls through to the next case
OMAP_DWC3_VBUS_OFF.
My question here is if for any reason this code is intentional?
In case it is not, I will write a patch to fix this, but first it
would be great to hear any comment about it.
Thank you
--
Gustavo A. R. Silva
--
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] drivers: usb: usbip: Add missing break statement to switch
Add missing break statement to prevent the code for case
USB_PORT_FEAT_C_RESET falling through to the default case.
Addresses-Coverity-ID: 143155
Signed-off-by: Gustavo A. R. Silva
---
drivers/usb/usbip/vhci_hcd.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index c4724fb..e4cb9f0 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -313,6 +313,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16
typeReq, u16 wValue,
default:
break;
}
+ break;
default:
usbip_dbg_vhci_rh(" ClearPortFeature: default %x\n",
wValue);
--
2.5.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
drivers: usb: musb: question about missing break in switch
Hello everybody,
I ran into the following piece of code at
drivers/usb/musb/musb_core.c:1854 (linux-next)
1854/*
1855 * Check the musb devctl session bit to determine if we want to
1856 * allow PM runtime for the device. In general, we want to keep things
1857 * active when the session bit is set except after host disconnect.
1858 *
1859 * Only called from musb_irq_work. If this ever needs to get called
1860 * elsewhere, proper locking must be implemented for musb->session.
1861 */
1862static void musb_pm_runtime_check_session(struct musb *musb)
1863{
1864u8 devctl, s;
1865int error;
1866
1867devctl = musb_readb(musb->mregs, MUSB_DEVCTL);
1868
1869/* Handle session status quirks first */
1870s = MUSB_DEVCTL_FSDEV | MUSB_DEVCTL_LSDEV |
1871MUSB_DEVCTL_HR;
1872switch (devctl & ~s) {
1873case MUSB_QUIRK_B_INVALID_VBUS_91:
1874if (musb->quirk_retries--) {
1875musb_dbg(musb,
1876 "Poll devctl on invalid vbus,
assume no session");
1877schedule_delayed_work(&musb->irq_work,
1878 msecs_to_jiffies(1000));
1879
1880return;
1881}
1882case MUSB_QUIRK_A_DISCONNECT_19:
1883if (musb->quirk_retries--) {
1884musb_dbg(musb,
1885 "Poll devctl on possible host
mode disconnect");
1886schedule_delayed_work(&musb->irq_work,
1887 msecs_to_jiffies(1000));
1888
1889return;
1890}
1891if (!musb->session)
1892break;
1893musb_dbg(musb, "Allow PM on possible host mode
disconnect");
1894pm_runtime_mark_last_busy(musb->controller);
1895pm_runtime_put_autosuspend(musb->controller);
1896musb->session = false;
1897return;
1898default:
1899break;
1900}
1901
1902/* No need to do anything if session has not changed */
1903s = devctl & MUSB_DEVCTL_SESSION;
1904if (s == musb->session)
1905return;
1906
1907/* Block PM or allow PM? */
1908if (s) {
1909musb_dbg(musb, "Block PM on active session: %02x",
devctl);
1910error = pm_runtime_get_sync(musb->controller);
1911if (error < 0)
1912dev_err(musb->controller, "Could not
enable: %i\n",
1913error);
1914musb->quirk_retries = 3;
1915} else {
1916musb_dbg(musb, "Allow PM with no session: %02x", devctl);
1917pm_runtime_mark_last_busy(musb->controller);
1918pm_runtime_put_autosuspend(musb->controller);
1919}
1920
1921musb->session = s;
1922}
The thing is that the case for MUSB_QUIRK_B_INVALID_VBUS_91 is not
terminated by a break statement, and it falls through to the next case
MUSB_QUIRK_A_DISCONNECT_19, in case "if (musb->quirk_retries--)" turns
to be false.
My question here is if this code is intentional?
In case it is not, I will write a patch to fix this, but first it
would be great to hear any comment about it.
Thank you
--
Gustavo A. R. Silva
--
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: drivers: usb: musb: question about missing break in switch
Hello Bin,
Quoting Bin Liu :
On Thu, Feb 09, 2017 at 02:37:34AM -0600, Gustavo A. R. Silva wrote:
Hello everybody,
I ran into the following piece of code at
drivers/usb/musb/musb_core.c:1854 (linux-next)
1854/*
1855 * Check the musb devctl session bit to determine if we want to
1856 * allow PM runtime for the device. In general, we want to keep things
1857 * active when the session bit is set except after host disconnect.
1858 *
1859 * Only called from musb_irq_work. If this ever needs to get called
1860 * elsewhere, proper locking must be implemented for musb->session.
1861 */
1862static void musb_pm_runtime_check_session(struct musb *musb)
1863{
1864u8 devctl, s;
1865int error;
1866
1867devctl = musb_readb(musb->mregs, MUSB_DEVCTL);
1868
1869/* Handle session status quirks first */
1870s = MUSB_DEVCTL_FSDEV | MUSB_DEVCTL_LSDEV |
1871MUSB_DEVCTL_HR;
1872switch (devctl & ~s) {
1873case MUSB_QUIRK_B_INVALID_VBUS_91:
1874if (musb->quirk_retries--) {
1875musb_dbg(musb,
1876 "Poll devctl on invalid vbus,
assume no session");
1877schedule_delayed_work(&musb->irq_work,
1878 msecs_to_jiffies(1000));
1879
1880return;
1881}
1882case MUSB_QUIRK_A_DISCONNECT_19:
1883if (musb->quirk_retries--) {
1884musb_dbg(musb,
1885 "Poll devctl on possible host
mode disconnect");
1886schedule_delayed_work(&musb->irq_work,
1887 msecs_to_jiffies(1000));
1888
1889return;
1890}
1891if (!musb->session)
1892break;
1893musb_dbg(musb, "Allow PM on possible host mode
disconnect");
1894pm_runtime_mark_last_busy(musb->controller);
1895pm_runtime_put_autosuspend(musb->controller);
1896musb->session = false;
1897return;
1898default:
1899break;
1900}
1901
1902/* No need to do anything if session has not changed */
1903s = devctl & MUSB_DEVCTL_SESSION;
1904if (s == musb->session)
1905return;
1906
1907/* Block PM or allow PM? */
1908if (s) {
1909musb_dbg(musb, "Block PM on active session:
%02x", devctl);
1910error = pm_runtime_get_sync(musb->controller);
1911if (error < 0)
1912dev_err(musb->controller, "Could not
enable: %i\n",
1913error);
1914musb->quirk_retries = 3;
1915} else {
1916musb_dbg(musb, "Allow PM with no session:
%02x", devctl);
1917pm_runtime_mark_last_busy(musb->controller);
1918pm_runtime_put_autosuspend(musb->controller);
1919}
1920
1921musb->session = s;
1922}
The thing is that the case for MUSB_QUIRK_B_INVALID_VBUS_91 is not
terminated by a break statement, and it falls through to the next
case MUSB_QUIRK_A_DISCONNECT_19, in case "if
(musb->quirk_retries--)" turns to be false.
My question here is if this code is intentional?
Yes, it is. For both MUSB_QUIRK_B_INVALID_VBUS_91 and
MUSB_QUIRK_A_DISCONNECT_19 cases, we first do retries, then if
musb->session is set, we allow PM.
Thanks for clarifying :)
--
Gustavo A. R. Silva
--
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] drivers: usb: musb: add code comment for clarification
Add code comment to make it clear that the fall-through is intentional.
Read the link for more details: https://lkml.org/lkml/2017/2/9/292
Signed-off-by: Gustavo A. R. Silva
---
drivers/usb/musb/musb_core.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 892088f..1aec986 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -1869,6 +1869,7 @@ static void musb_pm_runtime_check_session(struct musb
*musb)
return;
}
+ /* FALLTHROUGH */
case MUSB_QUIRK_A_DISCONNECT_19:
if (musb->quirk_retries--) {
musb_dbg(musb,
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: musb: add code comment for clarification
Add code comment to make it clear that the fall-through is intentional.
Read the link for more details: https://lkml.org/lkml/2017/2/9/292
Signed-off-by: Gustavo A. R. Silva
---
drivers/usb/musb/musb_core.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 892088f..1aec986 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -1869,6 +1869,7 @@ static void musb_pm_runtime_check_session(struct
musb *musb)
return;
}
+ /* fall through */
case MUSB_QUIRK_A_DISCONNECT_19:
if (musb->quirk_retries--) {
musb_dbg(musb,
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] usb: gadget: udc: remove pointer dereference after free
Remove pointer dereference after free and set pointer to NULL after free. Addresses-Coverity-ID: 1091173 Signed-off-by: Gustavo A. R. Silva --- Changes in v2: Move pointer dereference before pci_pool_free() Set pointer to NULL after free drivers/usb/gadget/udc/pch_udc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/usb/gadget/udc/pch_udc.c b/drivers/usb/gadget/udc/pch_udc.c index a97da64..73bb58f 100644 --- a/drivers/usb/gadget/udc/pch_udc.c +++ b/drivers/usb/gadget/udc/pch_udc.c @@ -1522,8 +1522,9 @@ static void pch_udc_free_dma_chain(struct pch_udc_dev *dev, /* do not free first desc., will be done by free for request */ td = phys_to_virt(addr); addr2 = (dma_addr_t)td->next; - pci_pool_free(dev->data_requests, td, addr); td->next = 0x00; + pci_pool_free(dev->data_requests, td, addr); + td = NULL; addr = addr2; } req->chain_len = 1; -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] avoid use of freed pointer
Hello, This patch series addesses Coverity ID 1091172, which reports the use of a freed pointer. udc_free_dma_chain() function was rewritten in order to fix this issue. Unnecessary 'ret_val' variable was removed and the function prototype was modified. Thanks Gustavo A. R. Silva (2): usb: gadget: udc: avoid use of freed pointer usb: gadget: udc: remove unnecessary variable and update function prototype drivers/usb/gadget/udc/amd5536udc.c | 25 - 1 file changed, 12 insertions(+), 13 deletions(-) -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] usb: gadget: udc: remove unnecessary variable and update function prototype
Remove unnecessary variable and update function prototype.
Reviewed-by: Greg Kroah-Hartman
Signed-off-by: Gustavo A. R. Silva
---
drivers/usb/gadget/udc/amd5536udc.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/usb/gadget/udc/amd5536udc.c
b/drivers/usb/gadget/udc/amd5536udc.c
index ded97a3..3f64a06 100644
--- a/drivers/usb/gadget/udc/amd5536udc.c
+++ b/drivers/usb/gadget/udc/amd5536udc.c
@@ -608,9 +608,8 @@ udc_alloc_request(struct usb_ep *usbep, gfp_t gfp)
}
/* frees pci pool descriptors of a DMA chain */
-static int udc_free_dma_chain(struct udc *dev, struct udc_request *req)
+static void udc_free_dma_chain(struct udc *dev, struct udc_request *req)
{
- int ret_val = 0;
struct udc_data_dma *td = req->td_data;
unsigned int i;
@@ -629,8 +628,6 @@ static int udc_free_dma_chain(struct udc *dev, struct
udc_request *req)
td = NULL;
addr = addr_aux;
}
-
- return ret_val;
}
/* Frees request packet, called by gadget driver */
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] usb: gadget: udc: avoid use of freed pointer
Rewrite udc_free_dma_chain() function to avoid use of pointer after free.
Addresses-Coverity-ID: 1091172
Reviewed-by: Greg Kroah-Hartman
Signed-off-by: Gustavo A. R. Silva
---
drivers/usb/gadget/udc/amd5536udc.c | 20 +++-
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/gadget/udc/amd5536udc.c
b/drivers/usb/gadget/udc/amd5536udc.c
index ea03ca7..ded97a3 100644
--- a/drivers/usb/gadget/udc/amd5536udc.c
+++ b/drivers/usb/gadget/udc/amd5536udc.c
@@ -611,21 +611,23 @@ udc_alloc_request(struct usb_ep *usbep, gfp_t gfp)
static int udc_free_dma_chain(struct udc *dev, struct udc_request *req)
{
int ret_val = 0;
- struct udc_data_dma *td;
- struct udc_data_dma *td_last = NULL;
+ struct udc_data_dma *td = req->td_data;
unsigned int i;
+ dma_addr_t addr_aux = 0x00;
+ dma_addr_t addr = (dma_addr_t)td->next;
+ td->next = 0x00;
+
DBG(dev, "free chain req = %p\n", req);
/* do not free first desc., will be done by free for request */
- td_last = req->td_data;
- td = phys_to_virt(td_last->next);
-
for (i = 1; i < req->chain_len; i++) {
- pci_pool_free(dev->data_requests, td,
- (dma_addr_t)td_last->next);
- td_last = td;
- td = phys_to_virt(td_last->next);
+ td = phys_to_virt(addr);
+ addr_aux = (dma_addr_t)td->next;
+ td->next = 0x00;
+ pci_pool_free(dev->data_requests, td, addr);
+ td = NULL;
+ addr = addr_aux;
}
return ret_val;
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] usb: gadget: udc: remove pointer dereference after free
Hi Michal, Quoting Michal Nazarewicz : On Sat, Feb 11 2017, Gustavo A. R. Silva wrote: Remove pointer dereference after free and set pointer to NULL after free. Addresses-Coverity-ID: 1091173 Signed-off-by: Gustavo A. R. Silva Acked-by: Michal Nazarewicz --- Changes in v2: Move pointer dereference before pci_pool_free() Set pointer to NULL after free drivers/usb/gadget/udc/pch_udc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/usb/gadget/udc/pch_udc.c b/drivers/usb/gadget/udc/pch_udc.c index a97da64..73bb58f 100644 --- a/drivers/usb/gadget/udc/pch_udc.c +++ b/drivers/usb/gadget/udc/pch_udc.c @@ -1522,8 +1522,9 @@ static void pch_udc_free_dma_chain(struct pch_udc_dev *dev, /* do not free first desc., will be done by free for request */ td = phys_to_virt(addr); addr2 = (dma_addr_t)td->next; - pci_pool_free(dev->data_requests, td, addr); td->next = 0x00; Or just drop this. pci_pool_free doesn’t care about contents of td. It’s just a void* for it. + pci_pool_free(dev->data_requests, td, addr); + td = NULL; This isn’t necessary either. td will get overwritten on next iteration and once we’re done it’s not used again. addr = addr2; } req->chain_len = 1; Thanks for your comments, I will send version 3 shortly. -- Gustavo A. R. Silva -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3] usb: gadget: udc: remove pointer dereference after free
Remove pointer dereference after free. Addresses-Coverity-ID: 1091173 Acked-by: Michal Nazarewicz Signed-off-by: Gustavo A. R. Silva --- Changes in v2: Move pointer dereference before pci_pool_free() Set pointer to NULL after free Changes in v3: Remove 'td->next = 0x00' inside for loop. Remove unnecessary pointer nullification after free. drivers/usb/gadget/udc/pch_udc.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/usb/gadget/udc/pch_udc.c b/drivers/usb/gadget/udc/pch_udc.c index a97da64..8a365aa 100644 --- a/drivers/usb/gadget/udc/pch_udc.c +++ b/drivers/usb/gadget/udc/pch_udc.c @@ -1523,7 +1523,6 @@ static void pch_udc_free_dma_chain(struct pch_udc_dev *dev, td = phys_to_virt(addr); addr2 = (dma_addr_t)td->next; pci_pool_free(dev->data_requests, td, addr); - td->next = 0x00; addr = addr2; } req->chain_len = 1; -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: musb: add code comment for clarification
Quoting Greg KH :
On Fri, Feb 10, 2017 at 06:57:41PM -0600, Gustavo A. R. Silva wrote:
Add code comment to make it clear that the fall-through is intentional.
Read the link for more details: https://lkml.org/lkml/2017/2/9/292
Signed-off-by: Gustavo A. R. Silva
---
drivers/usb/musb/musb_core.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 892088f..1aec986 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -1869,6 +1869,7 @@ static void musb_pm_runtime_check_session(struct musb
*musb)
return;
}
+ /* fall through */
case MUSB_QUIRK_A_DISCONNECT_19:
if (musb->quirk_retries--) {
musb_dbg(musb,
The tabs are all gone from this patch, and it's line-wrapped, making it
impossible to be applied :(
Can you please fix this and resend?
OK. I'll send it shortly.
Thanks
--
Gustavo A. R. Silva
--
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: musb: add code comment for clarification
Add code comment to make it clear that the fall-through is intentional.
Read the link for more details: https://lkml.org/lkml/2017/2/9/292
Addresses-Coverity-ID: 1397608
Signed-off-by: Gustavo A. R. Silva
---
Changes in v2:
Fix tabs and line-wrapping in previous patch.
drivers/usb/musb/musb_core.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 892088f..d8bae6c 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -1869,6 +1869,7 @@ static void musb_pm_runtime_check_session(struct musb
*musb)
return;
}
+ /* fall through */
case MUSB_QUIRK_A_DISCONNECT_19:
if (musb->quirk_retries--) {
musb_dbg(musb,
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] usb: gadget: udc: avoid use of freed pointer
Hi Michal,
Quoting Michal Nazarewicz :
On Mon, Feb 13 2017, Gustavo A. R. Silva wrote:
Rewrite udc_free_dma_chain() function to avoid use of pointer after free.
Addresses-Coverity-ID: 1091172
Reviewed-by: Greg Kroah-Hartman
Signed-off-by: Gustavo A. R. Silva
Acked-by: Michal Nazarewicz
---
drivers/usb/gadget/udc/amd5536udc.c | 20 +++-
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/gadget/udc/amd5536udc.c
b/drivers/usb/gadget/udc/amd5536udc.c
index ea03ca7..ded97a3 100644
--- a/drivers/usb/gadget/udc/amd5536udc.c
+++ b/drivers/usb/gadget/udc/amd5536udc.c
@@ -611,21 +611,23 @@ udc_alloc_request(struct usb_ep *usbep, gfp_t gfp)
static int udc_free_dma_chain(struct udc *dev, struct udc_request *req)
{
int ret_val = 0;
- struct udc_data_dma *td;
- struct udc_data_dma *td_last = NULL;
+ struct udc_data_dma *td = req->td_data;
unsigned int i;
+ dma_addr_t addr_aux = 0x00;
Perhaps call it ‘addr_next’ or ‘next’?
+ dma_addr_t addr = (dma_addr_t)td->next;
+ td->next = 0x00;
+
DBG(dev, "free chain req = %p\n", req);
/* do not free first desc., will be done by free for request */
- td_last = req->td_data;
- td = phys_to_virt(td_last->next);
-
for (i = 1; i < req->chain_len; i++) {
- pci_pool_free(dev->data_requests, td,
- (dma_addr_t)td_last->next);
- td_last = td;
- td = phys_to_virt(td_last->next);
+ td = phys_to_virt(addr);
+ addr_aux = (dma_addr_t)td->next;
+ td->next = 0x00;
This is unnecessary.
+ pci_pool_free(dev->data_requests, td, addr);
+ td = NULL;
Ditto.
+ addr = addr_aux;
}
return ret_val;
--
2.5.0
Thanks for your comments, I will send version 2 shortly.
--
Gustavo A. R. Silva
--
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: gadget: udc: avoid use of freed pointer
Rewrite udc_free_dma_chain() function to avoid use of pointer after free.
Addresses-Coverity-ID: 1091172
Acked-by: Michal Nazarewicz
Reviewed-by: Greg Kroah-Hartman
Signed-off-by: Gustavo A. R. Silva
---
Changes in v2:
Remove 'td->next = 0x00' inside for loop.
Remove unnecessary pointer nullification after free.
Rename variable addr_aux to addr_next.
drivers/usb/gadget/udc/amd5536udc.c | 17 -
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/gadget/udc/amd5536udc.c
b/drivers/usb/gadget/udc/amd5536udc.c
index ea03ca7..821d088 100644
--- a/drivers/usb/gadget/udc/amd5536udc.c
+++ b/drivers/usb/gadget/udc/amd5536udc.c
@@ -611,21 +611,20 @@ udc_alloc_request(struct usb_ep *usbep, gfp_t gfp)
static int udc_free_dma_chain(struct udc *dev, struct udc_request *req)
{
int ret_val = 0;
- struct udc_data_dma *td;
- struct udc_data_dma *td_last = NULL;
+ struct udc_data_dma *td = req->td_data;
unsigned int i;
+ dma_addr_t addr_next = 0x00;
+ dma_addr_t addr = (dma_addr_t)td->next;
+
DBG(dev, "free chain req = %p\n", req);
/* do not free first desc., will be done by free for request */
- td_last = req->td_data;
- td = phys_to_virt(td_last->next);
-
for (i = 1; i < req->chain_len; i++) {
- pci_pool_free(dev->data_requests, td,
- (dma_addr_t)td_last->next);
- td_last = td;
- td = phys_to_virt(td_last->next);
+ td = phys_to_virt(addr);
+ addr_next = (dma_addr_t)td->next;
+ pci_pool_free(dev->data_requests, td, addr);
+ addr = addr_next;
}
return ret_val;
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/2] usb: gadget: udc: remove unnecessary variable and update function prototype
Remove unnecessary variable and update function prototype.
Acked-by: Michal Nazarewicz
Reviewed-by: Greg Kroah-Hartman
Signed-off-by: Gustavo A. R. Silva
---
Changes in v2:
None.
drivers/usb/gadget/udc/amd5536udc.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/usb/gadget/udc/amd5536udc.c
b/drivers/usb/gadget/udc/amd5536udc.c
index 821d088..67dd209 100644
--- a/drivers/usb/gadget/udc/amd5536udc.c
+++ b/drivers/usb/gadget/udc/amd5536udc.c
@@ -608,9 +608,8 @@ udc_alloc_request(struct usb_ep *usbep, gfp_t gfp)
}
/* frees pci pool descriptors of a DMA chain */
-static int udc_free_dma_chain(struct udc *dev, struct udc_request *req)
+static void udc_free_dma_chain(struct udc *dev, struct udc_request *req)
{
- int ret_val = 0;
struct udc_data_dma *td = req->td_data;
unsigned int i;
@@ -626,8 +625,6 @@ static int udc_free_dma_chain(struct udc *dev, struct
udc_request *req)
pci_pool_free(dev->data_requests, td, addr);
addr = addr_next;
}
-
- return ret_val;
}
/* Frees request packet, called by gadget driver */
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: class: remove logically dead code
Remove logically dead code.
'cntr' is always equal to zero when the following line of code is executed:
rv = cntr ? cntr : -EAGAIN;
Addresses-Coverity-ID: 113227
Signed-off-by: Gustavo A. R. Silva
---
drivers/usb/class/cdc-wdm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 0a63695..8fda45a 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -531,7 +531,7 @@ static ssize_t wdm_read
i++;
if (file->f_flags & O_NONBLOCK) {
if (!test_bit(WDM_READ, &desc->flags)) {
- rv = cntr ? cntr : -EAGAIN;
+ rv = -EAGAIN;
goto err;
}
rv = 0;
--
2.5.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
usb: storage: suspicious code
Hello,
I ran into the following piece of code at drivers/usb/storage/jumpshot.c:305
(linux-next), and it seems a little bit suspicious:
// read the result. apparently the bulk write can complete
// before the jumpshot drive is finished writing. so we loop
// here until we get a good return code
waitcount = 0;
do {
result = jumpshot_get_status(us);
if (result != USB_STOR_TRANSPORT_GOOD) {
// I have not experimented to find the smallest value.
//
msleep(50);
}
} while ((result != USB_STOR_TRANSPORT_GOOD) && (waitcount < 10));
if (result != USB_STOR_TRANSPORT_GOOD)
usb_stor_dbg(us, "Gah! Waitcount = 10. Bad write!?\n");
Variable 'waitcount' is never updated inside the do-while loop. So, either it
isn't needed at all or line 316 should be modified (++waitcount < 10)
In case 'waitcount' isn't needed, lines 318 and 319 should be removed.
Can someone help me to clarify this so I can write a patch to fix this code?
Thank you
--
Gustavo A. R. Silva
--
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-storage] usb: storage: suspicious code
Hi Oliver,
Quoting Oliver Neukum :
Am Dienstag, den 14.02.2017, 23:06 -0600 schrieb Gustavo A. R. Silva:
Hi,
waitcount = 0;
do {
result = jumpshot_get_status(us);
if (result != USB_STOR_TRANSPORT_GOOD) {
// I have not experimented to find the smallest
value.
//
msleep(50);
}
} while ((result != USB_STOR_TRANSPORT_GOOD) && (waitcount <
10));
if (result != USB_STOR_TRANSPORT_GOOD)
usb_stor_dbg(us, "Gah! Waitcount = 10. Bad
write!?\n");
Variable 'waitcount' is never updated inside the do-while loop. So,
either it isn't needed at all or line 316 should be modified
(++waitcount < 10)
you are correct. Waitcount needs to be incremented.
Thanks for clarifying, I'll send a patch shortly.
--
Gustavo A. R. Silva
--
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: storage: add missing pre-increment to variable
Add missing pre-increment to 'waitcount' variable used in do-while loop. Addresses-Coverity-ID: 1011631 Signed-off-by: Gustavo A. R. Silva --- drivers/usb/storage/jumpshot.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/storage/jumpshot.c b/drivers/usb/storage/jumpshot.c index 011e527..a26c4bb 100644 --- a/drivers/usb/storage/jumpshot.c +++ b/drivers/usb/storage/jumpshot.c @@ -313,7 +313,7 @@ static int jumpshot_write_data(struct us_data *us, // msleep(50); } - } while ((result != USB_STOR_TRANSPORT_GOOD) && (waitcount < 10)); + } while ((result != USB_STOR_TRANSPORT_GOOD) && (++waitcount < 10)); if (result != USB_STOR_TRANSPORT_GOOD) usb_stor_dbg(us, "Gah! Waitcount = 10. Bad write!?\n"); -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: misc: remove unnecessary code
'val' is an unsigned variable, and less-than-zero comparison of an unsigned variable is never true. Addresses-Coverity-ID: 1230256 Signed-off-by: Gustavo A. R. Silva --- drivers/usb/misc/lvstest.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/misc/lvstest.c b/drivers/usb/misc/lvstest.c index 7717651..c7c2104 100644 --- a/drivers/usb/misc/lvstest.c +++ b/drivers/usb/misc/lvstest.c @@ -222,7 +222,7 @@ static ssize_t u1_timeout_store(struct device *dev, return ret; } - if (val < 0 || val > 127) + if (val > 127) return -EINVAL; ret = lvs_rh_set_port_feature(hdev, lvs->portnum | (val << 8), -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: atm: remove unnecessary code
'index' is an unsigned variable, and less-than-zero comparison of an unsigned variable is never true. Addresses-Coverity-ID: 115396 Signed-off-by: Gustavo A. R. Silva --- drivers/usb/atm/cxacru.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/atm/cxacru.c b/drivers/usb/atm/cxacru.c index f9fe86b6..d65a64c 100644 --- a/drivers/usb/atm/cxacru.c +++ b/drivers/usb/atm/cxacru.c @@ -474,7 +474,7 @@ static ssize_t cxacru_sysfs_store_adsl_config(struct device *dev, ret = sscanf(buf + pos, "%x=%x%n", &index, &value, &tmp); if (ret < 2) return -EINVAL; - if (index < 0 || index > 0x7f) + if (index > 0x7f) return -EINVAL; if (tmp < 0 || tmp > len - pos) return -EINVAL; -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: host: add cast to avoid potential integer overflow
The type of variable 'sel' is unsigned int. Such variable is being used
multiple times in a context that expects an expression of type unsigned
long long. So, to avoid any potential integer overflow, a cast to type
unsigned long long is added.
Addresses-Coverity-ID: 703408
Signed-off-by: Gustavo A. R. Silva
---
drivers/usb/host/xhci.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 50aee8b..8094d9a 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4298,23 +4298,23 @@ static unsigned long long
xhci_calculate_intel_u1_timeout(
ep_type = usb_endpoint_type(desc);
switch (ep_type) {
case USB_ENDPOINT_XFER_CONTROL:
- timeout_ns = udev->u1_params.sel * 3;
+ timeout_ns = (unsigned long long)udev->u1_params.sel * 3;
break;
case USB_ENDPOINT_XFER_BULK:
- timeout_ns = udev->u1_params.sel * 5;
+ timeout_ns = (unsigned long long)udev->u1_params.sel * 5;
break;
case USB_ENDPOINT_XFER_INT:
intr_type = usb_endpoint_interrupt_type(desc);
if (intr_type == USB_ENDPOINT_INTR_NOTIFICATION) {
- timeout_ns = udev->u1_params.sel * 3;
+ timeout_ns = (unsigned long long)udev->u1_params.sel *
3;
break;
}
/* Otherwise the calculation is the same as isoc eps */
case USB_ENDPOINT_XFER_ISOC:
timeout_ns = xhci_service_interval_to_ns(desc);
timeout_ns = DIV_ROUND_UP_ULL(timeout_ns * 105, 100);
- if (timeout_ns < udev->u1_params.sel * 2)
- timeout_ns = udev->u1_params.sel * 2;
+ if (timeout_ns < (unsigned long long)udev->u1_params.sel * 2)
+ timeout_ns = (unsigned long long)udev->u1_params.sel *
2;
break;
default:
return 0;
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: misc: remove unnecessary code
'val' is an unsigned variable, and less-than-zero comparison of an unsigned variable is never true. Addresses-Coverity-ID: 1230257 Signed-off-by: Gustavo A. R. Silva --- drivers/usb/misc/lvstest.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/misc/lvstest.c b/drivers/usb/misc/lvstest.c index c7c2104..6f37610 100644 --- a/drivers/usb/misc/lvstest.c +++ b/drivers/usb/misc/lvstest.c @@ -193,7 +193,7 @@ static ssize_t u2_timeout_store(struct device *dev, return ret; } - if (val < 0 || val > 127) + if (val > 127) return -EINVAL; ret = lvs_rh_set_port_feature(hdev, lvs->portnum | (val << 8), -- 2.5.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
[usb-misc] question about missing break in switch
Hello everybody,
I ran into the following piece of code at
drivers/usb/misc/usbtest.c:149 (linux-next)
149/* take the first altsetting with in-bulk + out-bulk;
150 * ignore other endpoints and altsettings.
151 */
152for (ep = 0; ep < alt->desc.bNumEndpoints; ep++) {
153struct usb_host_endpoint*e;
154
155e = alt->endpoint + ep;
156switch (usb_endpoint_type(&e->desc)) {
157case USB_ENDPOINT_XFER_BULK:
158break;
159case USB_ENDPOINT_XFER_INT:
160if (dev->info->intr)
161goto try_intr;
162case USB_ENDPOINT_XFER_ISOC:
163if (dev->info->iso)
164goto try_iso;
165/* FALLTHROUGH */
166default:
167continue;
168}
The thing is that the case for USB_ENDPOINT_XFER_INT is not terminated
by a break statement, and it falls through to the next case
USB_ENDPOINT_XFER_ISOC, in case "if (dev->info->intr)" turns to be
false.
My question here is if this code is intentional?
Similar to the case for USB_ENDPOINT_XFER_ISOC, which falls through to
the default case. But in that case there is a code comment that
confirms such behavior.
In case it is not intentional, I will write a patch to fix this.
In case it is indeed intentional I think it would be good to add a
code comment (/* fall through */) before "case USB_ENDPOINT_XFER_ISOC:"
It would be great to hear any comment about this.
Thank you
--
Gustavo A. R. Silva
--
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-misc] question about missing break in switch
Hi Alan,
Quoting Alan Stern :
On Mon, 20 Feb 2017, Gustavo A. R. Silva wrote:
Hello everybody,
I ran into the following piece of code at
drivers/usb/misc/usbtest.c:149 (linux-next)
149/* take the first altsetting with in-bulk + out-bulk;
150 * ignore other endpoints and altsettings.
151 */
152for (ep = 0; ep < alt->desc.bNumEndpoints; ep++) {
153struct usb_host_endpoint*e;
154
155e = alt->endpoint + ep;
156switch (usb_endpoint_type(&e->desc)) {
157case USB_ENDPOINT_XFER_BULK:
158break;
159case USB_ENDPOINT_XFER_INT:
160if (dev->info->intr)
161goto try_intr;
162case USB_ENDPOINT_XFER_ISOC:
163if (dev->info->iso)
164goto try_iso;
165/* FALLTHROUGH */
166default:
167continue;
168}
The thing is that the case for USB_ENDPOINT_XFER_INT is not terminated
by a break statement, and it falls through to the next case
USB_ENDPOINT_XFER_ISOC, in case "if (dev->info->intr)" turns to be
false.
My question here is if this code is intentional?
As far as I can tell, it is not.
Similar to the case for USB_ENDPOINT_XFER_ISOC, which falls through to
the default case. But in that case there is a code comment that
confirms such behavior.
In case it is not intentional, I will write a patch to fix this.
In case it is indeed intentional I think it would be good to add a
code comment (/* fall through */) before "case USB_ENDPOINT_XFER_ISOC:"
It would be great to hear any comment about this.
The USB_ENDPOINT_XFER_INT case should end with "continue;".
Although, in fact that whole loop could be structured better. The
"goto" parts really should be all in-line. Then the flow would be a
lot easier to follow.
I'll code the "goto" parts into in-line functions and send a patch.
Thanks for clarifying.
--
Gustavo A. R. Silva
--
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: add a missing continue and refactor code
Code refactoring to make the flow easier to follow and add missing
'continue' for case USB_ENDPOINT_XFER_INT.
Addresses-Coverity-ID: 1248733
Cc: Alan Stern
Signed-off-by: Gustavo A. R. Silva
---
drivers/usb/misc/usbtest.c | 50 +++---
1 file changed, 30 insertions(+), 20 deletions(-)
diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
index 3525626..8723e33 100644
--- a/drivers/usb/misc/usbtest.c
+++ b/drivers/usb/misc/usbtest.c
@@ -124,6 +124,32 @@ static struct usb_device *testdev_to_usbdev(struct
usbtest_dev *test)
/*-*/
+static inline void try_intr(struct usb_host_endpoint *e,
+ struct usb_host_endpoint *int_in,
+ struct usb_host_endpoint *int_out)
+{
+ if (usb_endpoint_dir_in(&e->desc)) {
+ if (!int_in)
+ int_in = e;
+ } else {
+ if (!int_out)
+ int_out = e;
+ }
+}
+
+static inline void try_iso(struct usb_host_endpoint *e,
+ struct usb_host_endpoint *iso_in,
+ struct usb_host_endpoint *iso_out)
+{
+ if (usb_endpoint_dir_in(&e->desc)) {
+ if (!iso_in)
+ iso_in = e;
+ } else {
+ if (!iso_out)
+ iso_out = e;
+ }
+}
+
static int
get_endpoints(struct usbtest_dev *dev, struct usb_interface *intf)
{
@@ -158,11 +184,12 @@ get_endpoints(struct usbtest_dev *dev, struct
usb_interface *intf)
break;
case USB_ENDPOINT_XFER_INT:
if (dev->info->intr)
- goto try_intr;
+ try_intr(e, int_in, int_out);
+ continue;
case USB_ENDPOINT_XFER_ISOC:
if (dev->info->iso)
- goto try_iso;
- /* FALLTHROUGH */
+ try_iso(e, iso_in, iso_out);
+ /* fall through */
default:
continue;
}
@@ -174,23 +201,6 @@ get_endpoints(struct usbtest_dev *dev, struct
usb_interface *intf)
out = e;
}
continue;
-try_intr:
- if (usb_endpoint_dir_in(&e->desc)) {
- if (!int_in)
- int_in = e;
- } else {
- if (!int_out)
- int_out = e;
- }
- continue;
-try_iso:
- if (usb_endpoint_dir_in(&e->desc)) {
- if (!iso_in)
- iso_in = e;
- } else {
- if (!iso_out)
- iso_out = e;
- }
}
if ((in && out) || iso_in || iso_out || int_in || int_out)
goto found;
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: misc: add a missing continue and refactor code
Hi Alan,
Quoting Alan Stern :
On Tue, 21 Feb 2017, Gustavo A. R. Silva wrote:
Code refactoring to make the flow easier to follow and add missing
'continue' for case USB_ENDPOINT_XFER_INT.
Addresses-Coverity-ID: 1248733
Cc: Alan Stern
Signed-off-by: Gustavo A. R. Silva
---
drivers/usb/misc/usbtest.c | 50
+++---
1 file changed, 30 insertions(+), 20 deletions(-)
diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
index 3525626..8723e33 100644
--- a/drivers/usb/misc/usbtest.c
+++ b/drivers/usb/misc/usbtest.c
@@ -124,6 +124,32 @@ static struct usb_device
*testdev_to_usbdev(struct usbtest_dev *test)
/*-*/
+static inline void try_intr(struct usb_host_endpoint *e,
+ struct usb_host_endpoint *int_in,
+ struct usb_host_endpoint *int_out)
+{
+ if (usb_endpoint_dir_in(&e->desc)) {
+ if (!int_in)
+ int_in = e;
+ } else {
+ if (!int_out)
+ int_out = e;
+ }
+}
+
+static inline void try_iso(struct usb_host_endpoint *e,
+ struct usb_host_endpoint *iso_in,
+ struct usb_host_endpoint *iso_out)
+{
+ if (usb_endpoint_dir_in(&e->desc)) {
+ if (!iso_in)
+ iso_in = e;
+ } else {
+ if (!iso_out)
+ iso_out = e;
+ }
+}
+
This is not at all what I had in mind. First, it's incorrect (can you
see why?). Second, by "inline" I meant moving the code to be actually
in-line next to the conditional, not some place else in a separate
subroutine (even if the subroutine is declared inline).
Interesting... let me double check.
I thought it would've been better to have separate inline subroutines
for those "goto".
Also, the code for the USB_ENDPOINT_XFER_BULK case should look like the
other two.
Do you mean a 'continue' instead of the 'break'?
Thanks for you comments.
--
Gustavo A. R. Silva
--
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: add a missing continue and refactor code
Quoting "Gustavo A. R. Silva" :
Hi Alan,
Quoting Alan Stern :
On Tue, 21 Feb 2017, Gustavo A. R. Silva wrote:
Code refactoring to make the flow easier to follow and add missing
'continue' for case USB_ENDPOINT_XFER_INT.
Addresses-Coverity-ID: 1248733
Cc: Alan Stern
Signed-off-by: Gustavo A. R. Silva
---
drivers/usb/misc/usbtest.c | 50
+++---
1 file changed, 30 insertions(+), 20 deletions(-)
diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
index 3525626..8723e33 100644
--- a/drivers/usb/misc/usbtest.c
+++ b/drivers/usb/misc/usbtest.c
@@ -124,6 +124,32 @@ static struct usb_device
*testdev_to_usbdev(struct usbtest_dev *test)
/*-*/
+static inline void try_intr(struct usb_host_endpoint *e,
+ struct usb_host_endpoint *int_in,
+ struct usb_host_endpoint *int_out)
+{
+ if (usb_endpoint_dir_in(&e->desc)) {
+ if (!int_in)
+ int_in = e;
+ } else {
+ if (!int_out)
+ int_out = e;
+ }
+}
+
+static inline void try_iso(struct usb_host_endpoint *e,
+ struct usb_host_endpoint *iso_in,
+ struct usb_host_endpoint *iso_out)
+{
+ if (usb_endpoint_dir_in(&e->desc)) {
+ if (!iso_in)
+ iso_in = e;
+ } else {
+ if (!iso_out)
+ iso_out = e;
+ }
+}
+
This is not at all what I had in mind. First, it's incorrect (can you
see why?). Second, by "inline" I meant moving the code to be actually
in-line next to the conditional, not some place else in a separate
subroutine (even if the subroutine is declared inline).
Interesting... let me double check.
I thought it would've been better to have separate inline
subroutines for those "goto".
Also, the code for the USB_ENDPOINT_XFER_BULK case should look like the
other two.
Do you mean a 'continue' instead of the 'break'?
Oh I see, the following piece of code should be part of the
USB_ENDPOINT_XFER_BULK case:
if (usb_endpoint_dir_in(&e->desc)) {
if (!in)
in = e;
} else {
if (!out)
out = e;
}
continue;
--
Gustavo A. R. Silva
--
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: remove unnecessary code
Hi Peter, Quoting Peter Senna Tschudin : On Mon, Feb 20, 2017 at 05:28:46PM -0600, Gustavo A. R. Silva wrote: 'val' is an unsigned variable, and less-than-zero comparison of an unsigned variable is never true. I would add that val is set by kstrtoul() that converts a string to an unsigned long. Addresses-Coverity-ID: 1230257 Reviewed-by: Peter Senna Tschudin Signed-off-by: Gustavo A. R. Silva --- drivers/usb/misc/lvstest.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/misc/lvstest.c b/drivers/usb/misc/lvstest.c index c7c2104..6f37610 100644 --- a/drivers/usb/misc/lvstest.c +++ b/drivers/usb/misc/lvstest.c @@ -193,7 +193,7 @@ static ssize_t u2_timeout_store(struct device *dev, return ret; } - if (val < 0 || val > 127) + if (val > 127) return -EINVAL; ret = lvs_rh_set_port_feature(hdev, lvs->portnum | (val << 8), -- 2.5.0 Thanks for your comments. -- Gustavo A. R. Silva -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] usb: gadget: udc: remove pointer dereference after free
Hello, Quoting Felipe Balbi : "Gustavo A. R. Silva" writes: Remove pointer dereference after free. Addresses-Coverity-ID: 1091173 Acked-by: Michal Nazarewicz Signed-off-by: Gustavo A. R. Silva --- Changes in v2: Move pointer dereference before pci_pool_free() Set pointer to NULL after free Changes in v3: Remove 'td->next = 0x00' inside for loop. Remove unnecessary pointer nullification after free. drivers/usb/gadget/udc/pch_udc.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/usb/gadget/udc/pch_udc.c b/drivers/usb/gadget/udc/pch_udc.c index a97da64..8a365aa 100644 --- a/drivers/usb/gadget/udc/pch_udc.c +++ b/drivers/usb/gadget/udc/pch_udc.c @@ -1523,7 +1523,6 @@ static void pch_udc_free_dma_chain(struct pch_udc_dev *dev, line wrapped. Can't apply. I'll fix it right away. Thanks -- Gustavo A. R. Silva -- 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 v4] usb: gadget: udc: remove pointer dereference after free
Remove pointer dereference after free. Addresses-Coverity-ID: 1091173 Acked-by: Michal Nazarewicz Signed-off-by: Gustavo A. R. Silva --- Changes in v2: Move pointer dereference before pci_pool_free() Set pointer to NULL after free Changes in v3: Remove 'td->next = 0x00' inside for loop. Remove unnecessary pointer nullification after free. Changes in v4: Fix line-wrapping in previous patch. drivers/usb/gadget/udc/pch_udc.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/usb/gadget/udc/pch_udc.c b/drivers/usb/gadget/udc/pch_udc.c index a97da64..8a365aa 100644 --- a/drivers/usb/gadget/udc/pch_udc.c +++ b/drivers/usb/gadget/udc/pch_udc.c @@ -1523,7 +1523,6 @@ static void pch_udc_free_dma_chain(struct pch_udc_dev *dev, td = phys_to_virt(addr); addr2 = (dma_addr_t)td->next; pci_pool_free(dev->data_requests, td, addr); - td->next = 0x00; addr = addr2; } req->chain_len = 1; -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: misc: add missing continue and refactor code
-Code refactoring to make the flow easier to follow.
-Add missing 'continue' for case USB_ENDPOINT_XFER_INT.
Addresses-Coverity-ID: 1248733
Cc: Alan Stern
Signed-off-by: Gustavo A. R. Silva
---
drivers/usb/misc/usbtest.c | 68 +-
1 file changed, 31 insertions(+), 37 deletions(-)
diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
index 3525626..382491e 100644
--- a/drivers/usb/misc/usbtest.c
+++ b/drivers/usb/misc/usbtest.c
@@ -124,18 +124,32 @@ static struct usb_device *testdev_to_usbdev(struct
usbtest_dev *test)
/*-*/
+static inline void endpoint_update(int edi,
+ struct usb_host_endpoint **in,
+ struct usb_host_endpoint **out,
+ struct usb_host_endpoint *e)
+{
+ if (edi) {
+ if (!*in)
+ *in = e;
+ } else {
+ if (!*out)
+ *out = e;
+ }
+}
+
static int
get_endpoints(struct usbtest_dev *dev, struct usb_interface *intf)
{
- int tmp;
- struct usb_host_interface *alt;
- struct usb_host_endpoint*in, *out;
- struct usb_host_endpoint*iso_in, *iso_out;
- struct usb_host_endpoint*int_in, *int_out;
- struct usb_device *udev;
+ int tmp;
+ struct usb_host_interface *alt;
+ struct usb_host_endpoint*in, *out;
+ struct usb_host_endpoint*iso_in, *iso_out;
+ struct usb_host_endpoint*int_in, *int_out;
+ struct usb_device *udev;
for (tmp = 0; tmp < intf->num_altsetting; tmp++) {
- unsignedep;
+ unsignedep;
in = out = NULL;
iso_in = iso_out = NULL;
@@ -150,47 +164,27 @@ get_endpoints(struct usbtest_dev *dev, struct
usb_interface *intf)
* ignore other endpoints and altsettings.
*/
for (ep = 0; ep < alt->desc.bNumEndpoints; ep++) {
- struct usb_host_endpoint*e;
+ struct usb_host_endpoint*e;
+ int edi;
e = alt->endpoint + ep;
+ edi = usb_endpoint_dir_in(&e->desc);
+
switch (usb_endpoint_type(&e->desc)) {
case USB_ENDPOINT_XFER_BULK:
- break;
+ endpoint_update(edi, &in, &out, e);
+ continue;
case USB_ENDPOINT_XFER_INT:
if (dev->info->intr)
- goto try_intr;
+ endpoint_update(edi, &int_in, &int_out,
e);
+ continue;
case USB_ENDPOINT_XFER_ISOC:
if (dev->info->iso)
- goto try_iso;
- /* FALLTHROUGH */
+ endpoint_update(edi, &iso_in, &iso_out,
e);
+ /* fall through */
default:
continue;
}
- if (usb_endpoint_dir_in(&e->desc)) {
- if (!in)
- in = e;
- } else {
- if (!out)
- out = e;
- }
- continue;
-try_intr:
- if (usb_endpoint_dir_in(&e->desc)) {
- if (!int_in)
- int_in = e;
- } else {
- if (!int_out)
- int_out = e;
- }
- continue;
-try_iso:
- if (usb_endpoint_dir_in(&e->desc)) {
- if (!iso_in)
- iso_in = e;
- } else {
- if (!iso_out)
- iso_out = e;
- }
}
if ((in && out) || iso_in || iso_out || int_in || int_out)
goto found;
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] usb: misc: add missing continue in switch
Add missing continue in switch. Addresses-Coverity-ID: 1248733 Cc: Alan Stern Cc: Greg Kroah-Hartman Signed-off-by: Gustavo A. R. Silva --- drivers/usb/misc/usbtest.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c index 3525626..7bfb6b78 100644 --- a/drivers/usb/misc/usbtest.c +++ b/drivers/usb/misc/usbtest.c @@ -159,6 +159,7 @@ get_endpoints(struct usbtest_dev *dev, struct usb_interface *intf) case USB_ENDPOINT_XFER_INT: if (dev->info->intr) goto try_intr; + continue; case USB_ENDPOINT_XFER_ISOC: if (dev->info->iso) goto try_iso; -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: misc: add missing continue and refactor code
Quoting Alan Stern : On Mon, 3 Apr 2017, Greg Kroah-Hartman wrote: On Mon, Apr 03, 2017 at 09:39:53AM -0500, Gustavo A. R. Silva wrote: > -Code refactoring to make the flow easier to follow. > -Add missing 'continue' for case USB_ENDPOINT_XFER_INT. Don't do multiple things in the same patch, please make these multiple patches. And do the "add missing continue" first, so it can be backported to other kernels easier please. OK, I will send a patchset shortly. Also, make sure your patch does not contain gratuitous whitespace changes. Does it have any? I ran it through checkpatch.pl before sending it and didn't see any. Thanks -- Gustavo A. R. Silva -- 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: misc: refactor code
Code refactoring to make the flow easier to follow.
Cc: Alan Stern
Cc: Greg Kroah-Hartman
Signed-off-by: Gustavo A. R. Silva
---
drivers/usb/misc/usbtest.c | 67 +-
1 file changed, 30 insertions(+), 37 deletions(-)
diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
index 7bfb6b78..382491e 100644
--- a/drivers/usb/misc/usbtest.c
+++ b/drivers/usb/misc/usbtest.c
@@ -124,18 +124,32 @@ static struct usb_device *testdev_to_usbdev(struct
usbtest_dev *test)
/*-*/
+static inline void endpoint_update(int edi,
+ struct usb_host_endpoint **in,
+ struct usb_host_endpoint **out,
+ struct usb_host_endpoint *e)
+{
+ if (edi) {
+ if (!*in)
+ *in = e;
+ } else {
+ if (!*out)
+ *out = e;
+ }
+}
+
static int
get_endpoints(struct usbtest_dev *dev, struct usb_interface *intf)
{
- int tmp;
- struct usb_host_interface *alt;
- struct usb_host_endpoint*in, *out;
- struct usb_host_endpoint*iso_in, *iso_out;
- struct usb_host_endpoint*int_in, *int_out;
- struct usb_device *udev;
+ int tmp;
+ struct usb_host_interface *alt;
+ struct usb_host_endpoint*in, *out;
+ struct usb_host_endpoint*iso_in, *iso_out;
+ struct usb_host_endpoint*int_in, *int_out;
+ struct usb_device *udev;
for (tmp = 0; tmp < intf->num_altsetting; tmp++) {
- unsignedep;
+ unsignedep;
in = out = NULL;
iso_in = iso_out = NULL;
@@ -150,48 +164,27 @@ get_endpoints(struct usbtest_dev *dev, struct
usb_interface *intf)
* ignore other endpoints and altsettings.
*/
for (ep = 0; ep < alt->desc.bNumEndpoints; ep++) {
- struct usb_host_endpoint*e;
+ struct usb_host_endpoint*e;
+ int edi;
e = alt->endpoint + ep;
+ edi = usb_endpoint_dir_in(&e->desc);
+
switch (usb_endpoint_type(&e->desc)) {
case USB_ENDPOINT_XFER_BULK:
- break;
+ endpoint_update(edi, &in, &out, e);
+ continue;
case USB_ENDPOINT_XFER_INT:
if (dev->info->intr)
- goto try_intr;
+ endpoint_update(edi, &int_in, &int_out,
e);
continue;
case USB_ENDPOINT_XFER_ISOC:
if (dev->info->iso)
- goto try_iso;
- /* FALLTHROUGH */
+ endpoint_update(edi, &iso_in, &iso_out,
e);
+ /* fall through */
default:
continue;
}
- if (usb_endpoint_dir_in(&e->desc)) {
- if (!in)
- in = e;
- } else {
- if (!out)
- out = e;
- }
- continue;
-try_intr:
- if (usb_endpoint_dir_in(&e->desc)) {
- if (!int_in)
- int_in = e;
- } else {
- if (!int_out)
- int_out = e;
- }
- continue;
-try_iso:
- if (usb_endpoint_dir_in(&e->desc)) {
- if (!iso_in)
- iso_in = e;
- } else {
- if (!iso_out)
- iso_out = e;
- }
}
if ((in && out) || iso_in || iso_out || int_in || int_out)
goto found;
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] usb: misc: refactor code
Hi Alan,
Quoting Alan Stern :
On Mon, 3 Apr 2017, Gustavo A. R. Silva wrote:
Code refactoring to make the flow easier to follow.
Cc: Alan Stern
Cc: Greg Kroah-Hartman
Signed-off-by: Gustavo A. R. Silva
---
drivers/usb/misc/usbtest.c | 67
+-
1 file changed, 30 insertions(+), 37 deletions(-)
diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
index 7bfb6b78..382491e 100644
--- a/drivers/usb/misc/usbtest.c
+++ b/drivers/usb/misc/usbtest.c
@@ -124,18 +124,32 @@ static struct usb_device
*testdev_to_usbdev(struct usbtest_dev *test)
/*-*/
+static inline void endpoint_update(int edi,
+ struct usb_host_endpoint **in,
+ struct usb_host_endpoint **out,
+ struct usb_host_endpoint *e)
+{
+ if (edi) {
+ if (!*in)
+ *in = e;
+ } else {
+ if (!*out)
+ *out = e;
+ }
+}
+
static int
get_endpoints(struct usbtest_dev *dev, struct usb_interface *intf)
{
- int tmp;
- struct usb_host_interface *alt;
- struct usb_host_endpoint*in, *out;
- struct usb_host_endpoint*iso_in, *iso_out;
- struct usb_host_endpoint*int_in, *int_out;
- struct usb_device *udev;
+ int tmp;
+ struct usb_host_interface *alt;
+ struct usb_host_endpoint*in, *out;
+ struct usb_host_endpoint*iso_in, *iso_out;
+ struct usb_host_endpoint*int_in, *int_out;
+ struct usb_device *udev;
What's the difference between these 6 lines you added and the 6 lines
that were there originally?
Yeah, well, certainly none at all. This is what happened: I created a
local copy of my changes(this piece of code included) and fixed some
issues in a previous patch, then I did a git revert and moved my
changes back to the original file. During this process the tabs were
replaced with spaces in the original file, then I had to add the tabs
again and this was the resulting patch.
for (tmp = 0; tmp < intf->num_altsetting; tmp++) {
- unsignedep;
+ unsignedep;
And here?
Same as above.
in = out = NULL;
iso_in = iso_out = NULL;
@@ -150,48 +164,27 @@ get_endpoints(struct usbtest_dev *dev, struct
usb_interface *intf)
* ignore other endpoints and altsettings.
*/
for (ep = 0; ep < alt->desc.bNumEndpoints; ep++) {
- struct usb_host_endpoint*e;
+ struct usb_host_endpoint*e;
And here?
Same as above.
+ int edi;
e = alt->endpoint + ep;
+ edi = usb_endpoint_dir_in(&e->desc);
+
switch (usb_endpoint_type(&e->desc)) {
case USB_ENDPOINT_XFER_BULK:
- break;
+ endpoint_update(edi, &in, &out, e);
+ continue;
case USB_ENDPOINT_XFER_INT:
if (dev->info->intr)
- goto try_intr;
+ endpoint_update(edi, &int_in, &int_out,
e);
continue;
case USB_ENDPOINT_XFER_ISOC:
if (dev->info->iso)
- goto try_iso;
- /* FALLTHROUGH */
+ endpoint_update(edi, &iso_in, &iso_out,
e);
+ /* fall through */
Why change the comment?
Oh, I based this change in the following comment to another patch I
sent some weeks ago:
https://lkml.org/lkml/2017/2/10/293
It is due to Coding Style.
Alan Stern
default:
continue;
}
- if (usb_endpoint_dir_in(&e->desc)) {
- if (!in)
- in = e;
- } else {
- if (!out)
- out = e;
- }
- continue;
-try_intr:
- if (usb_endpoint_dir_in(&e->desc)) {
- if (!int_in)
- int_in = e;
- } else {
- if (!int_out)
-
[PATCH v2 2/2] usb: misc: refactor code
Code refactoring to make the flow easier to follow.
Cc: Alan Stern
Cc: Greg Kroah-Hartman
Signed-off-by: Gustavo A. R. Silva
---
Changes in v2:
Remove unfruitful changes in previous patch.
Revert change to comment.
drivers/usb/misc/usbtest.c | 49 --
1 file changed, 21 insertions(+), 28 deletions(-)
diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
index 7bfb6b78..88f627e 100644
--- a/drivers/usb/misc/usbtest.c
+++ b/drivers/usb/misc/usbtest.c
@@ -124,6 +124,20 @@ static struct usb_device *testdev_to_usbdev(struct
usbtest_dev *test)
/*-*/
+static inline void endpoint_update(int edi,
+ struct usb_host_endpoint **in,
+ struct usb_host_endpoint **out,
+ struct usb_host_endpoint *e)
+{
+ if (edi) {
+ if (!*in)
+ *in = e;
+ } else {
+ if (!*out)
+ *out = e;
+ }
+}
+
static int
get_endpoints(struct usbtest_dev *dev, struct usb_interface *intf)
{
@@ -151,47 +165,26 @@ get_endpoints(struct usbtest_dev *dev, struct
usb_interface *intf)
*/
for (ep = 0; ep < alt->desc.bNumEndpoints; ep++) {
struct usb_host_endpoint*e;
+ int edi;
e = alt->endpoint + ep;
+ edi = usb_endpoint_dir_in(&e->desc);
+
switch (usb_endpoint_type(&e->desc)) {
case USB_ENDPOINT_XFER_BULK:
- break;
+ endpoint_update(edi, &in, &out, e);
+ continue;
case USB_ENDPOINT_XFER_INT:
if (dev->info->intr)
- goto try_intr;
+ endpoint_update(edi, &int_in, &int_out,
e);
continue;
case USB_ENDPOINT_XFER_ISOC:
if (dev->info->iso)
- goto try_iso;
+ endpoint_update(edi, &iso_in, &iso_out,
e);
/* FALLTHROUGH */
default:
continue;
}
- if (usb_endpoint_dir_in(&e->desc)) {
- if (!in)
- in = e;
- } else {
- if (!out)
- out = e;
- }
- continue;
-try_intr:
- if (usb_endpoint_dir_in(&e->desc)) {
- if (!int_in)
- int_in = e;
- } else {
- if (!int_out)
- int_out = e;
- }
- continue;
-try_iso:
- if (usb_endpoint_dir_in(&e->desc)) {
- if (!iso_in)
- iso_in = e;
- } else {
- if (!iso_out)
- iso_out = e;
- }
}
if ((in && out) || iso_in || iso_out || int_in || int_out)
goto found;
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/2] usb: misc: add missing continue in switch
Add missing continue in switch. Addresses-Coverity-ID: 1248733 Cc: Alan Stern Cc: Greg Kroah-Hartman Signed-off-by: Gustavo A. R. Silva --- Changes in v2: None. drivers/usb/misc/usbtest.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c index 3525626..7bfb6b78 100644 --- a/drivers/usb/misc/usbtest.c +++ b/drivers/usb/misc/usbtest.c @@ -159,6 +159,7 @@ get_endpoints(struct usbtest_dev *dev, struct usb_interface *intf) case USB_ENDPOINT_XFER_INT: if (dev->info->intr) goto try_intr; + continue; case USB_ENDPOINT_XFER_ISOC: if (dev->info->iso) goto try_iso; -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/2] usb: gadget: udc: avoid use of freed pointer
Rewrite udc_free_dma_chain() function to avoid use of pointer after free.
Addresses-Coverity-ID: 1091172
Acked-by: Michal Nazarewicz
Reviewed-by: Greg Kroah-Hartman
Signed-off-by: Gustavo A. R. Silva
---
Changes in v2:
Remove 'td->next = 0x00' inside for loop.
Remove unnecessary pointer nullification after free.
Rename variable addr_aux to addr_next.
drivers/usb/gadget/udc/amd5536udc.c | 17 -
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/gadget/udc/amd5536udc.c
b/drivers/usb/gadget/udc/amd5536udc.c
index ea03ca7..821d088 100644
--- a/drivers/usb/gadget/udc/amd5536udc.c
+++ b/drivers/usb/gadget/udc/amd5536udc.c
@@ -611,21 +611,20 @@ udc_alloc_request(struct usb_ep *usbep, gfp_t gfp)
static int udc_free_dma_chain(struct udc *dev, struct udc_request *req)
{
int ret_val = 0;
- struct udc_data_dma *td;
- struct udc_data_dma *td_last = NULL;
+ struct udc_data_dma *td = req->td_data;
unsigned int i;
+ dma_addr_t addr_next = 0x00;
+ dma_addr_t addr = (dma_addr_t)td->next;
+
DBG(dev, "free chain req = %p\n", req);
/* do not free first desc., will be done by free for request */
- td_last = req->td_data;
- td = phys_to_virt(td_last->next);
-
for (i = 1; i < req->chain_len; i++) {
- pci_pool_free(dev->data_requests, td,
- (dma_addr_t)td_last->next);
- td_last = td;
- td = phys_to_virt(td_last->next);
+ td = phys_to_virt(addr);
+ addr_next = (dma_addr_t)td->next;
+ pci_pool_free(dev->data_requests, td, addr);
+ addr = addr_next;
}
return ret_val;
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/2] usb: gadget: udc: remove unnecessary variable and update function prototype
Remove unnecessary variable and update function prototype.
Acked-by: Michal Nazarewicz
Reviewed-by: Greg Kroah-Hartman
Signed-off-by: Gustavo A. R. Silva
---
Changes in v2:
None.
drivers/usb/gadget/udc/amd5536udc.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/usb/gadget/udc/amd5536udc.c
b/drivers/usb/gadget/udc/amd5536udc.c
index 821d088..67dd209 100644
--- a/drivers/usb/gadget/udc/amd5536udc.c
+++ b/drivers/usb/gadget/udc/amd5536udc.c
@@ -608,9 +608,8 @@ udc_alloc_request(struct usb_ep *usbep, gfp_t gfp)
}
/* frees pci pool descriptors of a DMA chain */
-static int udc_free_dma_chain(struct udc *dev, struct udc_request *req)
+static void udc_free_dma_chain(struct udc *dev, struct udc_request *req)
{
- int ret_val = 0;
struct udc_data_dma *td = req->td_data;
unsigned int i;
@@ -626,8 +625,6 @@ static int udc_free_dma_chain(struct udc *dev, struct
udc_request *req)
pci_pool_free(dev->data_requests, td, addr);
addr = addr_next;
}
-
- return ret_val;
}
/* Frees request packet, called by gadget driver */
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] usb: misc: refactor code
Hello,
Quoting Felipe Balbi :
Hi,
"Gustavo A. R. Silva" writes:
Code refactoring to make the flow easier to follow.
Cc: Alan Stern
Cc: Greg Kroah-Hartman
Signed-off-by: Gustavo A. R. Silva
---
drivers/usb/misc/usbtest.c | 67
+-
1 file changed, 30 insertions(+), 37 deletions(-)
diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
index 7bfb6b78..382491e 100644
--- a/drivers/usb/misc/usbtest.c
+++ b/drivers/usb/misc/usbtest.c
@@ -124,18 +124,32 @@ static struct usb_device
*testdev_to_usbdev(struct usbtest_dev *test)
/*-*/
+static inline void endpoint_update(int edi,
+ struct usb_host_endpoint **in,
+ struct usb_host_endpoint **out,
+ struct usb_host_endpoint *e)
+{
+ if (edi) {
+ if (!*in)
+ *in = e;
+ } else {
+ if (!*out)
+ *out = e;
+ }
+}
+
static int
get_endpoints(struct usbtest_dev *dev, struct usb_interface *intf)
{
- int tmp;
- struct usb_host_interface *alt;
- struct usb_host_endpoint*in, *out;
- struct usb_host_endpoint*iso_in, *iso_out;
- struct usb_host_endpoint*int_in, *int_out;
- struct usb_device *udev;
+ int tmp;
+ struct usb_host_interface *alt;
+ struct usb_host_endpoint*in, *out;
+ struct usb_host_endpoint*iso_in, *iso_out;
+ struct usb_host_endpoint*int_in, *int_out;
+ struct usb_device *udev;
unnecessary change
for (tmp = 0; tmp < intf->num_altsetting; tmp++) {
- unsignedep;
+ unsignedep;
unnecessary change
in = out = NULL;
iso_in = iso_out = NULL;
@@ -150,48 +164,27 @@ get_endpoints(struct usbtest_dev *dev, struct
usb_interface *intf)
* ignore other endpoints and altsettings.
*/
for (ep = 0; ep < alt->desc.bNumEndpoints; ep++) {
- struct usb_host_endpoint*e;
+ struct usb_host_endpoint*e;
unnecessary change
I already sent the version 2 of this patch: https://lkml.org/lkml/2017/4/3/856
Thanks
--
Gustavo A. R. Silva
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: serial: io_edgeport: fix up switch fall-through comments
On 5/2/19 12:35 PM, Greg Kroah-Hartman wrote:
> Gustavo has been working to fix up all of the switch statements that
> "fall through" such that we can eventually turn on
> -Wimplicit-fallthrough. As part of that, the io_edgeport.c driver is a
> bit "messy" with the parsing logic of a data packet. Clean that logic
> up a bit by unindenting one level of the logic, and properly label
> /* Fall through */ to make gcc happy.
>
> Reported-by: Gustavo A. R. Silva
> Signed-off-by: Greg Kroah-Hartman
>
Acked-by: Gustavo A. R. Silva
Thanks, Greg.
--
Gustavo
> diff --git a/drivers/usb/serial/io_edgeport.c
> b/drivers/usb/serial/io_edgeport.c
> index 4ca31c0e4174..48a439298a68 100644
> --- a/drivers/usb/serial/io_edgeport.c
> +++ b/drivers/usb/serial/io_edgeport.c
> @@ -1751,7 +1751,7 @@ static void process_rcvd_data(struct edgeport_serial
> *edge_serial,
> edge_serial->rxState = EXPECT_HDR2;
> break;
> }
> - /* otherwise, drop on through */
> + /* Fall through */
> case EXPECT_HDR2:
> edge_serial->rxHeader2 = *buffer;
> ++buffer;
> @@ -1790,29 +1790,20 @@ static void process_rcvd_data(struct edgeport_serial
> *edge_serial,
> edge_serial->rxHeader2, 0);
> edge_serial->rxState = EXPECT_HDR1;
> break;
> - } else {
> - edge_serial->rxPort =
> - IOSP_GET_HDR_PORT(edge_serial->rxHeader1);
> - edge_serial->rxBytesRemaining =
> - IOSP_GET_HDR_DATA_LEN(
> - edge_serial->rxHeader1,
> - edge_serial->rxHeader2);
> - dev_dbg(dev, "%s - Data for Port %u Len %u\n",
> - __func__,
> - edge_serial->rxPort,
> - edge_serial->rxBytesRemaining);
> -
> - /* ASSERT(DevExt->RxPort < DevExt->NumPorts);
> - * ASSERT(DevExt->RxBytesRemaining <
> - * IOSP_MAX_DATA_LENGTH);
> - */
> -
> - if (bufferLength == 0) {
> - edge_serial->rxState = EXPECT_DATA;
> - break;
> - }
> - /* Else, drop through */
> }
> +
> + edge_serial->rxPort =
> IOSP_GET_HDR_PORT(edge_serial->rxHeader1);
> + edge_serial->rxBytesRemaining =
> IOSP_GET_HDR_DATA_LEN(edge_serial->rxHeader1,
> +
> edge_serial->rxHeader2);
> + dev_dbg(dev, "%s - Data for Port %u Len %u\n", __func__,
> + edge_serial->rxPort,
> + edge_serial->rxBytesRemaining);
> +
> + if (bufferLength == 0) {
> + edge_serial->rxState = EXPECT_DATA;
> + break;
> + }
> + /* Fall through */
> case EXPECT_DATA: /* Expect data */
> if (bufferLength < edge_serial->rxBytesRemaining) {
> rxLen = bufferLength;
>
Re: [PATCH] USB: serial: io_edgeport: fix up switch fall-through comments
On 5/3/19 1:09 AM, Johan Hovold wrote: > On Thu, May 02, 2019 at 07:35:15PM +0200, Greg Kroah-Hartman wrote: >> Gustavo has been working to fix up all of the switch statements that >> "fall through" such that we can eventually turn on >> -Wimplicit-fallthrough. As part of that, the io_edgeport.c driver is a >> bit "messy" with the parsing logic of a data packet. Clean that logic >> up a bit by unindenting one level of the logic, and properly label >> /* Fall through */ to make gcc happy. >> >> Reported-by: Gustavo A. R. Silva >> Signed-off-by: Greg Kroah-Hartman > > Now applied, thanks. > > Gustavo, how many of these warnings are there left in the kernel now > that the last one in USB is gone? :) > Today, we woke up with 37 of these warnings left in linux-next. :) There were more than 2000 when I first started auditing them. Thanks -- Gustavo
[PATCH] usb: phy: ab8500-usb: Mark expected switch fall-throughs
Mark switch cases where we are expecting to fall through.
This patch fixes the following warnings:
drivers/usb/phy/phy-ab8500-usb.c: In function 'ab8500_usb_link_status_update':
drivers/usb/phy/phy-ab8500-usb.c:424:9: warning: this statement may fall
through [-Wimplicit-fallthrough=]
event = UX500_MUSB_RIDB;
~~^
drivers/usb/phy/phy-ab8500-usb.c:425:2: note: here
case USB_LINK_NOT_CONFIGURED_8500:
^~~~
drivers/usb/phy/phy-ab8500-usb.c:440:9: warning: this statement may fall
through [-Wimplicit-fallthrough=]
event = UX500_MUSB_RIDC;
~~^
drivers/usb/phy/phy-ab8500-usb.c:441:2: note: here
case USB_LINK_STD_HOST_NC_8500:
^~~~
drivers/usb/phy/phy-ab8500-usb.c:459:9: warning: this statement may fall
through [-Wimplicit-fallthrough=]
event = UX500_MUSB_RIDA;
~~^
drivers/usb/phy/phy-ab8500-usb.c:460:2: note: here
case USB_LINK_HM_IDGND_8500:
^~~~
drivers/usb/phy/phy-ab8500-usb.c: In function 'ab8505_usb_link_status_update':
drivers/usb/phy/phy-ab8500-usb.c:332:9: warning: this statement may fall
through [-Wimplicit-fallthrough=]
event = UX500_MUSB_RIDB;
~~^
drivers/usb/phy/phy-ab8500-usb.c:333:2: note: here
case USB_LINK_NOT_CONFIGURED_8505:
^~~~
drivers/usb/phy/phy-ab8500-usb.c:352:9: warning: this statement may fall
through [-Wimplicit-fallthrough=]
event = UX500_MUSB_RIDC;
~~^
drivers/usb/phy/phy-ab8500-usb.c:353:2: note: here
case USB_LINK_STD_HOST_NC_8505:
^~~~
drivers/usb/phy/phy-ab8500-usb.c:370:9: warning: this statement may fall
through [-Wimplicit-fallthrough=]
event = UX500_MUSB_RIDA;
~~^
drivers/usb/phy/phy-ab8500-usb.c:371:2: note: here
case USB_LINK_HM_IDGND_8505:
^~~~
Reported-by: Stephen Rothwell
Signed-off-by: Gustavo A. R. Silva
---
drivers/usb/phy/phy-ab8500-usb.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/usb/phy/phy-ab8500-usb.c b/drivers/usb/phy/phy-ab8500-usb.c
index aaf363f19714..14b432982fd3 100644
--- a/drivers/usb/phy/phy-ab8500-usb.c
+++ b/drivers/usb/phy/phy-ab8500-usb.c
@@ -330,6 +330,7 @@ static int ab8505_usb_link_status_update(struct ab8500_usb
*ab,
switch (lsts) {
case USB_LINK_ACA_RID_B_8505:
event = UX500_MUSB_RIDB;
+ /* Fall through */
case USB_LINK_NOT_CONFIGURED_8505:
case USB_LINK_RESERVED0_8505:
case USB_LINK_RESERVED1_8505:
@@ -350,6 +351,7 @@ static int ab8505_usb_link_status_update(struct ab8500_usb
*ab,
case USB_LINK_ACA_RID_C_NM_8505:
event = UX500_MUSB_RIDC;
+ /* Fall through */
case USB_LINK_STD_HOST_NC_8505:
case USB_LINK_STD_HOST_C_NS_8505:
case USB_LINK_STD_HOST_C_S_8505:
@@ -368,6 +370,7 @@ static int ab8505_usb_link_status_update(struct ab8500_usb
*ab,
case USB_LINK_ACA_RID_A_8505:
case USB_LINK_ACA_DOCK_CHGR_8505:
event = UX500_MUSB_RIDA;
+ /* Fall through */
case USB_LINK_HM_IDGND_8505:
if (ab->mode == USB_IDLE) {
ab->mode = USB_HOST;
@@ -422,6 +425,7 @@ static int ab8500_usb_link_status_update(struct ab8500_usb
*ab,
switch (lsts) {
case USB_LINK_ACA_RID_B_8500:
event = UX500_MUSB_RIDB;
+ /* Fall through */
case USB_LINK_NOT_CONFIGURED_8500:
case USB_LINK_NOT_VALID_LINK_8500:
ab->mode = USB_IDLE;
@@ -438,6 +442,7 @@ static int ab8500_usb_link_status_update(struct ab8500_usb
*ab,
case USB_LINK_ACA_RID_C_HS_8500:
case USB_LINK_ACA_RID_C_HS_CHIRP_8500:
event = UX500_MUSB_RIDC;
+ /* Fall through */
case USB_LINK_STD_HOST_NC_8500:
case USB_LINK_STD_HOST_C_NS_8500:
case USB_LINK_STD_HOST_C_S_8500:
@@ -457,6 +462,7 @@ static int ab8500_usb_link_status_update(struct ab8500_usb
*ab,
case USB_LINK_ACA_RID_A_8500:
event = UX500_MUSB_RIDA;
+ /* Fall through */
case USB_LINK_HM_IDGND_8500:
if (ab->mode == USB_IDLE) {
ab->mode = USB_HOST;
--
2.22.0
[PATCH] usb: host: ohci-tmio: Mark expected switch fall-throughs
Mark switch cases where we are expecting to fall through.
This patch fixes the following warning (Building: arm):
drivers/usb/host/ohci-tmio.c: In function ‘tmio_stop_hc’:
./include/linux/device.h:1499:2: warning: this statement may fall through
[-Wimplicit-fallthrough=]
_dev_err(dev, dev_fmt(fmt), ##__VA_ARGS__)
^~
drivers/usb/host/ohci-tmio.c:99:4: note: in expansion of macro ‘dev_err’
dev_err(&dev->dev, "Unsupported amount of ports: %d\n", ohci->num_ports);
^~~
In file included from drivers/usb/host/ohci-hcd.c:1257:0:
drivers/usb/host/ohci-tmio.c:100:3: note: here
case 3:
^~~~
drivers/usb/host/ohci-tmio.c:101:7: warning: this statement may fall through
[-Wimplicit-fallthrough=]
pm |= CCR_PM_USBPW3;
^
drivers/usb/host/ohci-tmio.c:102:3: note: here
case 2:
^~~~
drivers/usb/host/ohci-tmio.c:103:7: warning: this statement may fall through
[-Wimplicit-fallthrough=]
pm |= CCR_PM_USBPW2;
^
drivers/usb/host/ohci-tmio.c:104:3: note: here
case 1:
^~~~
Signed-off-by: Gustavo A. R. Silva
---
drivers/usb/host/ohci-tmio.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/usb/host/ohci-tmio.c b/drivers/usb/host/ohci-tmio.c
index d5a293a707b6..fb6f5e9ae5c6 100644
--- a/drivers/usb/host/ohci-tmio.c
+++ b/drivers/usb/host/ohci-tmio.c
@@ -97,10 +97,13 @@ static void tmio_stop_hc(struct platform_device *dev)
switch (ohci->num_ports) {
default:
dev_err(&dev->dev, "Unsupported amount of ports: %d\n",
ohci->num_ports);
+ /* fall through */
case 3:
pm |= CCR_PM_USBPW3;
+ /* fall through */
case 2:
pm |= CCR_PM_USBPW2;
+ /* fall through */
case 1:
pm |= CCR_PM_USBPW1;
}
--
2.22.0
[PATCH] usb: gadget: atmel_usba_udc: Mark expected switch fall-through
Mark switch cases where we are expecting to fall through.
This patch fixes the following warning (Building: at91_dt_defconfig arm):
drivers/usb/gadget/udc/atmel_usba_udc.c:329:13: warning: this statement may
fall through [-Wimplicit-fallthrough=]
Signed-off-by: Gustavo A. R. Silva
---
drivers/usb/gadget/udc/atmel_usba_udc.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c
b/drivers/usb/gadget/udc/atmel_usba_udc.c
index 503d275bc4c4..86ffc8307864 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.c
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
@@ -327,6 +327,7 @@ static int usba_config_fifo_table(struct usba_udc *udc)
switch (fifo_mode) {
default:
fifo_mode = 0;
+ /* fall through */
case 0:
udc->fifo_cfg = NULL;
n = 0;
--
2.22.0
[PATCH] USB: gadget: udc: s3c2410_udc: Mark expected switch fall-throughs
Mark switch cases where we are expecting to fall through.
This patch fixes the following warning (Building: tct_hammer_defconfig arm):
drivers/usb/gadget/udc/s3c2410_udc.c:314:7: warning: this statement may fall
through [-Wimplicit-fallthrough=]
drivers/usb/gadget/udc/s3c2410_udc.c:418:7: warning: this statement may fall
through [-Wimplicit-fallthrough=]
Signed-off-by: Gustavo A. R. Silva
---
drivers/usb/gadget/udc/s3c2410_udc.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/usb/gadget/udc/s3c2410_udc.c
b/drivers/usb/gadget/udc/s3c2410_udc.c
index af3e63316ace..f82208fbc249 100644
--- a/drivers/usb/gadget/udc/s3c2410_udc.c
+++ b/drivers/usb/gadget/udc/s3c2410_udc.c
@@ -312,6 +312,7 @@ static int s3c2410_udc_write_fifo(struct s3c2410_ep *ep,
switch (idx) {
default:
idx = 0;
+ /* fall through */
case 0:
fifo_reg = S3C2410_UDC_EP0_FIFO_REG;
break;
@@ -416,6 +417,7 @@ static int s3c2410_udc_read_fifo(struct s3c2410_ep *ep,
switch (idx) {
default:
idx = 0;
+ /* fall through */
case 0:
fifo_reg = S3C2410_UDC_EP0_FIFO_REG;
break;
--
2.22.0
[PATCH] USB: wusbcore: crypto: Remove VLA usage
In preparation to enabling -Wvla, remove VLA and replace it with dynamic memory allocation instead. The use of stack Variable Length Arrays needs to be avoided, as they can be a vector for stack exhaustion, which can be both a runtime bug or a security flaw. Also, in general, as code evolves it is easy to lose track of how big a VLA can get. Thus, we can end up having runtime failures that are hard to debug. Also, fixed as part of the directive to remove all VLAs from the kernel: https://lkml.org/lkml/2018/3/7/621 Notice that in this particular case, an alternative to kzalloc is kcalloc, in which case the code would look as follows instead: iv = kcalloc(crypto_skcipher_ivsize(tfm_cbc), sizeof(*iv), GFP_KERNEL); but if the data type of _iv_ never changes, or the type size is always one byte, kzalloc is good enough. Signed-off-by: Gustavo A. R. Silva --- drivers/usb/wusbcore/crypto.c | 11 +++ 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/drivers/usb/wusbcore/crypto.c b/drivers/usb/wusbcore/crypto.c index 4c00be2d..3511473 100644 --- a/drivers/usb/wusbcore/crypto.c +++ b/drivers/usb/wusbcore/crypto.c @@ -202,7 +202,7 @@ static int wusb_ccm_mac(struct crypto_skcipher *tfm_cbc, struct scatterlist sg[4], sg_dst; void *dst_buf; size_t dst_size; - u8 iv[crypto_skcipher_ivsize(tfm_cbc)]; + u8 *iv; size_t zero_padding; /* @@ -222,9 +222,11 @@ static int wusb_ccm_mac(struct crypto_skcipher *tfm_cbc, zero_padding; dst_buf = kzalloc(dst_size, GFP_KERNEL); if (!dst_buf) - goto error_dst_buf; + goto error_alloc; - memset(iv, 0, sizeof(iv)); + iv = kzalloc(crypto_skcipher_ivsize(tfm_cbc), GFP_KERNEL); + if (!iv) + goto error_alloc; /* Setup B0 */ scratch->b0.flags = 0x59; /* Format B0 */ @@ -276,8 +278,9 @@ static int wusb_ccm_mac(struct crypto_skcipher *tfm_cbc, bytewise_xor(mic, &scratch->ax, iv, 8); result = 8; error_cbc_crypt: + kfree(iv); kfree(dst_buf); -error_dst_buf: +error_alloc: return result; } -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: wusbcore: crypto: Remove VLA usage
I just discovered an issue with this patch. Please, drop it. I'll send v2 shortly. Thanks -- Gustavo On 03/16/2018 08:01 AM, Gustavo A. R. Silva wrote: In preparation to enabling -Wvla, remove VLA and replace it with dynamic memory allocation instead. The use of stack Variable Length Arrays needs to be avoided, as they can be a vector for stack exhaustion, which can be both a runtime bug or a security flaw. Also, in general, as code evolves it is easy to lose track of how big a VLA can get. Thus, we can end up having runtime failures that are hard to debug. Also, fixed as part of the directive to remove all VLAs from the kernel: https://lkml.org/lkml/2018/3/7/621 Notice that in this particular case, an alternative to kzalloc is kcalloc, in which case the code would look as follows instead: iv = kcalloc(crypto_skcipher_ivsize(tfm_cbc), sizeof(*iv), GFP_KERNEL); but if the data type of _iv_ never changes, or the type size is always one byte, kzalloc is good enough. Signed-off-by: Gustavo A. R. Silva --- drivers/usb/wusbcore/crypto.c | 11 +++ 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/drivers/usb/wusbcore/crypto.c b/drivers/usb/wusbcore/crypto.c index 4c00be2d..3511473 100644 --- a/drivers/usb/wusbcore/crypto.c +++ b/drivers/usb/wusbcore/crypto.c @@ -202,7 +202,7 @@ static int wusb_ccm_mac(struct crypto_skcipher *tfm_cbc, struct scatterlist sg[4], sg_dst; void *dst_buf; size_t dst_size; - u8 iv[crypto_skcipher_ivsize(tfm_cbc)]; + u8 *iv; size_t zero_padding; /* @@ -222,9 +222,11 @@ static int wusb_ccm_mac(struct crypto_skcipher *tfm_cbc, zero_padding; dst_buf = kzalloc(dst_size, GFP_KERNEL); if (!dst_buf) - goto error_dst_buf; + goto error_alloc; - memset(iv, 0, sizeof(iv)); + iv = kzalloc(crypto_skcipher_ivsize(tfm_cbc), GFP_KERNEL); + if (!iv) + goto error_alloc; /* Setup B0 */ scratch->b0.flags = 0x59;/* Format B0 */ @@ -276,8 +278,9 @@ static int wusb_ccm_mac(struct crypto_skcipher *tfm_cbc, bytewise_xor(mic, &scratch->ax, iv, 8); result = 8; error_cbc_crypt: + kfree(iv); kfree(dst_buf); -error_dst_buf: +error_alloc: return result; } -- 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: wusbcore: crypto: Remove VLA usage
In preparation to enabling -Wvla, remove VLA and replace it with dynamic memory allocation instead. The use of stack Variable Length Arrays needs to be avoided, as they can be a vector for stack exhaustion, which can be both a runtime bug or a security flaw. Also, in general, as code evolves it is easy to lose track of how big a VLA can get. Thus, we can end up having runtime failures that are hard to debug. Also, fixed as part of the directive to remove all VLAs from the kernel: https://lkml.org/lkml/2018/3/7/621 Notice that in this particular case, an alternative to kzalloc is kcalloc, in which case the code would look as follows instead: iv = kcalloc(crypto_skcipher_ivsize(tfm_cbc), sizeof(*iv), GFP_KERNEL); but if the data type of _iv_ never changes, or the type size is always one byte, kzalloc is good enough. Signed-off-by: Gustavo A. R. Silva --- Changes in v2: - Fix a memory leak in previous patch. drivers/usb/wusbcore/crypto.c | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/usb/wusbcore/crypto.c b/drivers/usb/wusbcore/crypto.c index 4c00be2d..aff50eb 100644 --- a/drivers/usb/wusbcore/crypto.c +++ b/drivers/usb/wusbcore/crypto.c @@ -202,7 +202,7 @@ static int wusb_ccm_mac(struct crypto_skcipher *tfm_cbc, struct scatterlist sg[4], sg_dst; void *dst_buf; size_t dst_size; - u8 iv[crypto_skcipher_ivsize(tfm_cbc)]; + u8 *iv; size_t zero_padding; /* @@ -224,7 +224,9 @@ static int wusb_ccm_mac(struct crypto_skcipher *tfm_cbc, if (!dst_buf) goto error_dst_buf; - memset(iv, 0, sizeof(iv)); + iv = kzalloc(crypto_skcipher_ivsize(tfm_cbc), GFP_KERNEL); + if (!iv) + goto error_iv; /* Setup B0 */ scratch->b0.flags = 0x59; /* Format B0 */ @@ -276,6 +278,8 @@ static int wusb_ccm_mac(struct crypto_skcipher *tfm_cbc, bytewise_xor(mic, &scratch->ax, iv, 8); result = 8; error_cbc_crypt: + kfree(iv); +error_iv: kfree(dst_buf); error_dst_buf: return result; -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: core: hcd: mark expected switch fall-through
In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.
Addresses-Coverity-ID: 1468266 ("Missing break in switch")
Signed-off-by: Gustavo A. R. Silva
---
drivers/usb/core/hcd.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 66cd3f9..1c21955 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2819,6 +2819,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
case HCD_USB32:
rhdev->rx_lanes = 2;
rhdev->tx_lanes = 2;
+ /* fall through */
case HCD_USB31:
rhdev->speed = USB_SPEED_SUPER_PLUS;
break;
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] typec: tcpm: Fix incorrect 'and' operator
Currently, logical and is being used instead of *bitwise* and.
Fix this by using a proper bitwise and operator.
Addresses-Coverity-ID: 1468455 ("Logical vs. bitwise operator")
Fixes: 64f7c494a3c0 ("typec: tcpm: Add support for sink PPS related
messages")
Signed-off-by: Gustavo A. R. Silva
---
drivers/usb/typec/tcpm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
index 1ee259b..7ee417a 100644
--- a/drivers/usb/typec/tcpm.c
+++ b/drivers/usb/typec/tcpm.c
@@ -1772,7 +1772,7 @@ static void tcpm_pd_ext_msg_request(struct tcpm_port
*port,
enum pd_ext_msg_type type = pd_header_type_le(msg->header);
unsigned int data_size =
pd_ext_header_data_size_le(msg->ext_msg.header);
- if (!(msg->ext_msg.header && PD_EXT_HDR_CHUNKED)) {
+ if (!(msg->ext_msg.header & PD_EXT_HDR_CHUNKED)) {
tcpm_log(port, "Unchunked extended messages unsupported");
return;
}
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usbip: vhci_sysfs: fix potential Spectre v1
pdev_nr and rhport can be controlled by user-space, hence leading to
a potential exploitation of the Spectre variant 1 vulnerability.
This issue was detected with the help of Smatch:
drivers/usb/usbip/vhci_sysfs.c:238 detach_store() warn: potential
spectre issue 'vhcis'
drivers/usb/usbip/vhci_sysfs.c:328 attach_store() warn: potential
spectre issue 'vhcis'
drivers/usb/usbip/vhci_sysfs.c:338 attach_store() warn: potential
spectre issue 'vhci->vhci_hcd_ss->vdev'
drivers/usb/usbip/vhci_sysfs.c:340 attach_store() warn: potential
spectre issue 'vhci->vhci_hcd_hs->vdev'
Fix this by sanitizing pdev_nr and rhport before using them to index
vhcis and vhci->vhci_hcd_ss->vdev respectively.
Notice that given that speculation windows are large, the policy is
to kill the speculation on the first load and not worry if it can be
completed with a dependent load/store [1].
[1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2
Cc: sta...@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva
---
drivers/usb/usbip/vhci_sysfs.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
index 4880838..9045888 100644
--- a/drivers/usb/usbip/vhci_sysfs.c
+++ b/drivers/usb/usbip/vhci_sysfs.c
@@ -10,6 +10,8 @@
#include
#include
+#include
+
#include "usbip_common.h"
#include "vhci.h"
@@ -235,6 +237,8 @@ static ssize_t detach_store(struct device *dev, struct
device_attribute *attr,
if (!valid_port(pdev_nr, rhport))
return -EINVAL;
+ pdev_nr = array_index_nospec(pdev_nr, vhci_num_controllers);
+ rhport = array_index_nospec(rhport, VHCI_HC_PORTS);
hcd = platform_get_drvdata(vhcis[pdev_nr].pdev);
if (hcd == NULL) {
dev_err(dev, "port is not ready %u\n", port);
@@ -325,6 +329,8 @@ static ssize_t attach_store(struct device *dev, struct
device_attribute *attr,
if (!valid_args(pdev_nr, rhport, speed))
return -EINVAL;
+ pdev_nr = array_index_nospec(pdev_nr, vhci_num_controllers);
+ rhport = array_index_nospec(rhport, VHCI_HC_PORTS);
hcd = platform_get_drvdata(vhcis[pdev_nr].pdev);
if (hcd == NULL) {
dev_err(dev, "port %d is not ready\n", port);
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usbip: vhci_sysfs: fix potential Spectre v1
Hi Greg, On 05/17/2018 01:51 AM, Greg Kroah-Hartman wrote: On Wed, May 16, 2018 at 05:22:00PM -0500, Gustavo A. R. Silva wrote: pdev_nr and rhport can be controlled by user-space, hence leading to a potential exploitation of the Spectre variant 1 vulnerability. This issue was detected with the help of Smatch: drivers/usb/usbip/vhci_sysfs.c:238 detach_store() warn: potential spectre issue 'vhcis' drivers/usb/usbip/vhci_sysfs.c:328 attach_store() warn: potential spectre issue 'vhcis' drivers/usb/usbip/vhci_sysfs.c:338 attach_store() warn: potential spectre issue 'vhci->vhci_hcd_ss->vdev' drivers/usb/usbip/vhci_sysfs.c:340 attach_store() warn: potential spectre issue 'vhci->vhci_hcd_hs->vdev' Nit, no need to line-wrap long error messages from tools :) Got it. Fix this by sanitizing pdev_nr and rhport before using them to index vhcis and vhci->vhci_hcd_ss->vdev respectively. Notice that given that speculation windows are large, the policy is to kill the speculation on the first load and not worry if it can be completed with a dependent load/store [1]. [1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2 Cc: sta...@vger.kernel.org Signed-off-by: Gustavo A. R. Silva --- drivers/usb/usbip/vhci_sysfs.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c index 4880838..9045888 100644 --- a/drivers/usb/usbip/vhci_sysfs.c +++ b/drivers/usb/usbip/vhci_sysfs.c @@ -10,6 +10,8 @@ #include #include +#include + #include "usbip_common.h" #include "vhci.h" @@ -235,6 +237,8 @@ static ssize_t detach_store(struct device *dev, struct device_attribute *attr, if (!valid_port(pdev_nr, rhport)) return -EINVAL; + pdev_nr = array_index_nospec(pdev_nr, vhci_num_controllers); + rhport = array_index_nospec(rhport, VHCI_HC_PORTS); Shouldn't we just do this in one place, in the valid_port() function? That way it keeps the range checking logic in one place (now it is in 3 places in the function), which should make maintenance much simpler. Yep, I thought about that, the thing is: what happens if the hardware is "trained" to predict that valid_port always evaluates to false, and then malicious values are stored in pdev_nr and nhport? It seems to me that under this scenario we need to serialize instructions in this place. What do you think? Thanks -- Gustavo -- 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] usbip: vhci_sysfs: fix potential Spectre v1
On 05/17/2018 02:15 PM, Greg Kroah-Hartman wrote: Shouldn't we just do this in one place, in the valid_port() function? That way it keeps the range checking logic in one place (now it is in 3 places in the function), which should make maintenance much simpler. Yep, I thought about that, the thing is: what happens if the hardware is "trained" to predict that valid_port always evaluates to false, and then malicious values are stored in pdev_nr and nhport? It seems to me that under this scenario we need to serialize instructions in this place. What do you think? I don't understand, it should not matter where you put the barrier. Be it a function call back or right after it, it does the same thing, it stops speculation from crossing that barrier. Yeah. It makes sense. So it _should_ work either way, if I understand the issue correctly. If not, what am I missing? No. It seems I'm the one who was missing something. I'll place the barrier into valid_port and send v2 shortly. Thanks! -- Gustavo -- 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] usbip: vhci_sysfs: fix potential Spectre v1
pdev_nr and rhport can be controlled by user-space, hence leading to
a potential exploitation of the Spectre variant 1 vulnerability.
This issue was detected with the help of Smatch:
drivers/usb/usbip/vhci_sysfs.c:238 detach_store() warn: potential spectre issue
'vhcis'
drivers/usb/usbip/vhci_sysfs.c:328 attach_store() warn: potential spectre issue
'vhcis'
drivers/usb/usbip/vhci_sysfs.c:338 attach_store() warn: potential spectre issue
'vhci->vhci_hcd_ss->vdev'
drivers/usb/usbip/vhci_sysfs.c:340 attach_store() warn: potential spectre issue
'vhci->vhci_hcd_hs->vdev'
Fix this by sanitizing pdev_nr and rhport before using them to index
vhcis and vhci->vhci_hcd_ss->vdev respectively.
Notice that given that speculation windows are large, the policy is
to kill the speculation on the first load and not worry if it can be
completed with a dependent load/store [1].
[1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2
Cc: sta...@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva
---
Changes in v2:
- Place the barriers into valid_port.
drivers/usb/usbip/vhci_sysfs.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
index 4880838..69db0c9 100644
--- a/drivers/usb/usbip/vhci_sysfs.c
+++ b/drivers/usb/usbip/vhci_sysfs.c
@@ -10,6 +10,8 @@
#include
#include
+#include
+
#include "usbip_common.h"
#include "vhci.h"
@@ -211,10 +213,14 @@ static int valid_port(__u32 pdev_nr, __u32 rhport)
pr_err("pdev %u\n", pdev_nr);
return 0;
}
+ pdev_nr = array_index_nospec(pdev_nr, vhci_num_controllers);
+
if (rhport >= VHCI_HC_PORTS) {
pr_err("rhport %u\n", rhport);
return 0;
}
+ rhport = array_index_nospec(rhport, VHCI_HC_PORTS);
+
return 1;
}
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] usbip: vhci_sysfs: fix potential Spectre v1
On 05/18/2018 11:06 AM, Shuah Khan wrote: On 05/18/2018 07:47 AM, Greg Kroah-Hartman wrote: On Thu, May 17, 2018 at 03:16:28PM -0500, Gustavo A. R. Silva wrote: pdev_nr and rhport can be controlled by user-space, hence leading to a potential exploitation of the Spectre variant 1 vulnerability. This issue was detected with the help of Smatch: drivers/usb/usbip/vhci_sysfs.c:238 detach_store() warn: potential spectre issue 'vhcis' drivers/usb/usbip/vhci_sysfs.c:328 attach_store() warn: potential spectre issue 'vhcis' drivers/usb/usbip/vhci_sysfs.c:338 attach_store() warn: potential spectre issue 'vhci->vhci_hcd_ss->vdev' drivers/usb/usbip/vhci_sysfs.c:340 attach_store() warn: potential spectre issue 'vhci->vhci_hcd_hs->vdev' Fix this by sanitizing pdev_nr and rhport before using them to index vhcis and vhci->vhci_hcd_ss->vdev respectively. Notice that given that speculation windows are large, the policy is to kill the speculation on the first load and not worry if it can be completed with a dependent load/store [1]. [1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2 Cc: sta...@vger.kernel.org Signed-off-by: Gustavo A. R. Silva --- Changes in v2: - Place the barriers into valid_port. attach_store() doesn't call valid_port() - can you make the change to have attach_store() call valid_port() to protect that code path. Thanks for the change. I'll wait for Shuah's ack/review before queueing this up just as she knows that codebase much better than anyone else. Greg, I've been talking with Dan Williams (intel) about this kind of issues [1] and it seems my original assumptions are correct. Hence, this patch is not useful and, in order to actually prevent speculation here we would need to pass the address of pdev_nr and rhport into valid_port, otherwise there may be speculation at drivers/usb/usbip/vhci_sysfs.c:235: if (!valid_port(pdev_nr, rhport)) return -EINVAL; hcd = platform_get_drvdata(vhcis[pdev_nr].pdev); [1] https://marc.info/?l=linux-kernel&m=152668152509103&w=2 Thanks -- Gustavo -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3] usbip: vhci_sysfs: fix potential Spectre v1
pdev_nr and rhport can be controlled by user-space, hence leading to
a potential exploitation of the Spectre variant 1 vulnerability.
This issue was detected with the help of Smatch:
drivers/usb/usbip/vhci_sysfs.c:238 detach_store() warn: potential spectre issue
'vhcis'
drivers/usb/usbip/vhci_sysfs.c:328 attach_store() warn: potential spectre issue
'vhcis'
drivers/usb/usbip/vhci_sysfs.c:338 attach_store() warn: potential spectre issue
'vhci->vhci_hcd_ss->vdev'
drivers/usb/usbip/vhci_sysfs.c:340 attach_store() warn: potential spectre issue
'vhci->vhci_hcd_hs->vdev'
Fix this by sanitizing pdev_nr and rhport before using them to index
vhcis and vhci->vhci_hcd_ss->vdev respectively.
Notice that given that speculation windows are large, the policy is
to kill the speculation on the first load and not worry if it can be
completed with a dependent load/store [1].
[1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2
Cc: sta...@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva
---
Changes in v3:
- Pass the addresses of pdev_nr and rhport into valid_port and
valid_args.
Changes in v2:
- Place the barriers into valid_port.
drivers/usb/usbip/vhci_sysfs.c | 24
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
index 4880838..be37aec 100644
--- a/drivers/usb/usbip/vhci_sysfs.c
+++ b/drivers/usb/usbip/vhci_sysfs.c
@@ -10,6 +10,9 @@
#include
#include
+/* Hardening for Spectre-v1 */
+#include
+
#include "usbip_common.h"
#include "vhci.h"
@@ -205,16 +208,20 @@ static int vhci_port_disconnect(struct vhci_hcd
*vhci_hcd, __u32 rhport)
return 0;
}
-static int valid_port(__u32 pdev_nr, __u32 rhport)
+static int valid_port(__u32 *pdev_nr, __u32 *rhport)
{
- if (pdev_nr >= vhci_num_controllers) {
- pr_err("pdev %u\n", pdev_nr);
+ if (*pdev_nr >= vhci_num_controllers) {
+ pr_err("pdev %u\n", *pdev_nr);
return 0;
}
- if (rhport >= VHCI_HC_PORTS) {
- pr_err("rhport %u\n", rhport);
+ *pdev_nr = array_index_nospec(*pdev_nr, vhci_num_controllers);
+
+ if (*rhport >= VHCI_HC_PORTS) {
+ pr_err("rhport %u\n", *rhport);
return 0;
}
+ *rhport = array_index_nospec(*rhport, VHCI_HC_PORTS);
+
return 1;
}
@@ -232,7 +239,7 @@ static ssize_t detach_store(struct device *dev, struct
device_attribute *attr,
pdev_nr = port_to_pdev_nr(port);
rhport = port_to_rhport(port);
- if (!valid_port(pdev_nr, rhport))
+ if (!valid_port(&pdev_nr, &rhport))
return -EINVAL;
hcd = platform_get_drvdata(vhcis[pdev_nr].pdev);
@@ -258,7 +265,8 @@ static ssize_t detach_store(struct device *dev, struct
device_attribute *attr,
}
static DEVICE_ATTR_WO(detach);
-static int valid_args(__u32 pdev_nr, __u32 rhport, enum usb_device_speed speed)
+static int valid_args(__u32 *pdev_nr, __u32 *rhport,
+ enum usb_device_speed speed)
{
if (!valid_port(pdev_nr, rhport)) {
return 0;
@@ -322,7 +330,7 @@ static ssize_t attach_store(struct device *dev, struct
device_attribute *attr,
sockfd, devid, speed);
/* check received parameters */
- if (!valid_args(pdev_nr, rhport, speed))
+ if (!valid_args(&pdev_nr, &rhport, speed))
return -EINVAL;
hcd = platform_get_drvdata(vhcis[pdev_nr].pdev);
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] usbip: vhci_sysfs: fix potential Spectre v1
On 05/19/2018 02:04 AM, Greg Kroah-Hartman wrote: Greg, I've been talking with Dan Williams (intel) about this kind of issues [1] and it seems my original assumptions are correct. Hence, this patch is not useful and, in order to actually prevent speculation here we would need to pass the address of pdev_nr and rhport into valid_port, otherwise there may be speculation at drivers/usb/usbip/vhci_sysfs.c:235: if (!valid_port(pdev_nr, rhport)) return -EINVAL; hcd = platform_get_drvdata(vhcis[pdev_nr].pdev); Ah, yes, sorry, you do need to pass the address through, my mistake completely. But the location for the checking is still the right place to do it, so I was half-right :) Yep. And that totally make sense. I already sent v3: https://marc.info/?l=linux-kernel&m=152669243313887&w=2 Thanks! -- Gustavo -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] usbip: vhci_sysfs: fix potential Spectre v1
On 05/22/2018 10:56 AM, Shuah Khan wrote:
On 05/18/2018 07:13 PM, Gustavo A. R. Silva wrote:
pdev_nr and rhport can be controlled by user-space, hence leading to
a potential exploitation of the Spectre variant 1 vulnerability.
This issue was detected with the help of Smatch:
drivers/usb/usbip/vhci_sysfs.c:238 detach_store() warn: potential spectre issue
'vhcis'
drivers/usb/usbip/vhci_sysfs.c:328 attach_store() warn: potential spectre issue
'vhcis'
drivers/usb/usbip/vhci_sysfs.c:338 attach_store() warn: potential spectre issue
'vhci->vhci_hcd_ss->vdev'
drivers/usb/usbip/vhci_sysfs.c:340 attach_store() warn: potential spectre issue
'vhci->vhci_hcd_hs->vdev'
Fix this by sanitizing pdev_nr and rhport before using them to index
vhcis and vhci->vhci_hcd_ss->vdev respectively.
Notice that given that speculation windows are large, the policy is
to kill the speculation on the first load and not worry if it can be
completed with a dependent load/store [1].
[1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2
Cc: sta...@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva
---
Changes in v3:
- Pass the addresses of pdev_nr and rhport into valid_port and
valid_args.
Changes in v2:
- Place the barriers into valid_port.
drivers/usb/usbip/vhci_sysfs.c | 24
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
index 4880838..be37aec 100644
--- a/drivers/usb/usbip/vhci_sysfs.c
+++ b/drivers/usb/usbip/vhci_sysfs.c
@@ -10,6 +10,9 @@
#include
#include
+/* Hardening for Spectre-v1 */
+#include
+
#include "usbip_common.h"
#include "vhci.h"
@@ -205,16 +208,20 @@ static int vhci_port_disconnect(struct vhci_hcd *vhci_hcd, __u32 rhport)
return 0;
}
-static int valid_port(__u32 pdev_nr, __u32 rhport)
+static int valid_port(__u32 *pdev_nr, __u32 *rhport)
{
- if (pdev_nr >= vhci_num_controllers) {
- pr_err("pdev %u\n", pdev_nr);
+ if (*pdev_nr >= vhci_num_controllers) {
+ pr_err("pdev %u\n", *pdev_nr);
return 0;
}
- if (rhport >= VHCI_HC_PORTS) {
- pr_err("rhport %u\n", rhport);
+ *pdev_nr = array_index_nospec(*pdev_nr, vhci_num_controllers);
+
+ if (*rhport >= VHCI_HC_PORTS) {
+ pr_err("rhport %u\n", *rhport);
return 0;
}
+ *rhport = array_index_nospec(*rhport, VHCI_HC_PORTS);
+
return 1;
}
@@ -232,7 +239,7 @@ static ssize_t detach_store(struct device *dev, struct device_attribute *attr,
pdev_nr = port_to_pdev_nr(port);
rhport = port_to_rhport(port);
- if (!valid_port(pdev_nr, rhport))
+ if (!valid_port(&pdev_nr, &rhport))
return -EINVAL;
hcd = platform_get_drvdata(vhcis[pdev_nr].pdev);
@@ -258,7 +265,8 @@ static ssize_t detach_store(struct device *dev, struct
device_attribute *attr,
}
static DEVICE_ATTR_WO(detach);
-static int valid_args(__u32 pdev_nr, __u32 rhport, enum usb_device_speed speed)
+static int valid_args(__u32 *pdev_nr, __u32 *rhport,
+ enum usb_device_speed speed)
{
if (!valid_port(pdev_nr, rhport)) {
return 0;
@@ -322,7 +330,7 @@ static ssize_t attach_store(struct device *dev, struct
device_attribute *attr,
sockfd, devid, speed);
/* check received parameters */
- if (!valid_args(pdev_nr, rhport, speed))
+ if (!valid_args(&pdev_nr, &rhport, speed))
return -EINVAL;
hcd = platform_get_drvdata(vhcis[pdev_nr].pdev);
Looks good to me. Thanks for taking care of this.
Glad to help. :)
Acked-by: Shuah Khan (Samsung OSG)
Thanks
--
Gustavo
--
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: phy: phy-msm-usb: mark expected switch fall-through
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. Addresses-Coverity-ID: 1222118 Signed-off-by: Gustavo A. R. Silva --- drivers/usb/phy/phy-msm-usb.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/phy/phy-msm-usb.c b/drivers/usb/phy/phy-msm-usb.c index 3d0dd2f..8bc3403 100644 --- a/drivers/usb/phy/phy-msm-usb.c +++ b/drivers/usb/phy/phy-msm-usb.c @@ -1261,6 +1261,7 @@ static void msm_chg_detect_work(struct work_struct *w) /* fall through */ case USB_CHG_STATE_SECONDARY_DONE: motg->chg_state = USB_CHG_STATE_DETECTED; + /* fall through */ case USB_CHG_STATE_DETECTED: msm_chg_block_off(motg); dev_dbg(phy->dev, "charger = %d\n", motg->chg_type); -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: image: mdc800: mark expected switch fall-through
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. Signed-off-by: Gustavo A. R. Silva --- drivers/usb/image/mdc800.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/image/mdc800.c b/drivers/usb/image/mdc800.c index e92540a..185c4e2 100644 --- a/drivers/usb/image/mdc800.c +++ b/drivers/usb/image/mdc800.c @@ -893,6 +893,7 @@ static ssize_t mdc800_device_write (struct file *file, const char __user *buf, s return -EIO; } mdc800->pic_len=-1; + /* fall through */ case 0x09: /* Download Thumbnail */ mdc800->download_left=answersize+64; -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: core: urb: mark expected switch fall-through
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. Addresses-Coverity-ID: 1162594 Signed-off-by: Gustavo A. R. Silva --- drivers/usb/core/urb.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c index 8b800e3..06e0151 100644 --- a/drivers/usb/core/urb.c +++ b/drivers/usb/core/urb.c @@ -514,6 +514,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags) if ((urb->interval < 6) && (xfertype == USB_ENDPOINT_XFER_INT)) return -EINVAL; + /* fall through */ default: if (urb->interval <= 0) return -EINVAL; -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: gadget: f_tcm: mark expected switch fall-through
In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.
Addresses-Coverity-ID: 703128
Signed-off-by: Gustavo A. R. Silva
---
drivers/usb/gadget/function/f_tcm.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/gadget/function/f_tcm.c
b/drivers/usb/gadget/function/f_tcm.c
index a82e2bd..c9d741d 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -1145,6 +1145,7 @@ static int usbg_submit_command(struct f_uas *fu,
default:
pr_debug_once("Unsupported prio_attr: %02x.\n",
cmd_iu->prio_attr);
+ /* fall through */
case UAS_SIMPLE_TAG:
cmd->prio_attr = TCM_SIMPLE_TAG;
break;
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: musb_core: mark expected switch fall-through
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. Addresses-Coverity-ID: 1397608 Signed-off-by: Gustavo A. R. Silva --- drivers/usb/musb/musb_core.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c index ff5a1a8..889ca9b 100644 --- a/drivers/usb/musb/musb_core.c +++ b/drivers/usb/musb/musb_core.c @@ -767,6 +767,7 @@ static irqreturn_t musb_stage0_irq(struct musb *musb, u8 int_usb, case OTG_STATE_B_IDLE: if (!musb->is_active) break; + /* fall through */ case OTG_STATE_B_PERIPHERAL: musb_g_suspend(musb); musb->is_active = musb->g.b_hnp_enable; -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: gadget: serial: mark expected switch fall-through
In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.
Addresses-Coverity-ID: 1350962
Signed-off-by: Gustavo A. R. Silva
---
drivers/usb/gadget/function/u_serial.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/gadget/function/u_serial.c
b/drivers/usb/gadget/function/u_serial.c
index 4176216..961457e 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -1078,6 +1078,7 @@ static void gs_complete_out(struct usb_ep *ep, struct
usb_request *req)
default:
pr_warn("%s: unexpected %s status %d\n",
__func__, ep->name, req->status);
+ /* fall through */
case 0:
/* normal completion */
spin_lock(&info->con_lock);
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: gadget: goku_udc: mark expected switch fall-throughs
In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.
Addresses-Coverity-ID: 145713
Signed-off-by: Gustavo A. R. Silva
---
drivers/usb/gadget/udc/goku_udc.c | 14 +-
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/gadget/udc/goku_udc.c
b/drivers/usb/gadget/udc/goku_udc.c
index 8433c22..a85407e 100644
--- a/drivers/usb/gadget/udc/goku_udc.c
+++ b/drivers/usb/gadget/udc/goku_udc.c
@@ -127,11 +127,15 @@ goku_ep_enable(struct usb_ep *_ep, const struct
usb_endpoint_descriptor *desc)
mode = 0;
max = get_unaligned_le16(&desc->wMaxPacketSize);
switch (max) {
- case 64:mode++;
- case 32:mode++;
- case 16:mode++;
- case 8: mode <<= 3;
- break;
+ case 64:
+ mode++; /* fall through */
+ case 32:
+ mode++; /* fall through */
+ case 16:
+ mode++; /* fall through */
+ case 8:
+ mode <<= 3;
+ break;
default:
return -EINVAL;
}
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: storage: uas: mark expected switch fall-through
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. Addresses-Coverity-ID: 115016 Signed-off-by: Gustavo A. R. Silva --- drivers/usb/storage/uas.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c index 63cf981..bd4671d 100644 --- a/drivers/usb/storage/uas.c +++ b/drivers/usb/storage/uas.c @@ -668,6 +668,7 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd, break; case DMA_BIDIRECTIONAL: cmdinfo->state |= ALLOC_DATA_IN_URB | SUBMIT_DATA_IN_URB; + /* fall through */ case DMA_TO_DEVICE: cmdinfo->state |= ALLOC_DATA_OUT_URB | SUBMIT_DATA_OUT_URB; case DMA_NONE: -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: serial: kobil_sct: mark expected switch fall-through
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. Addresses-Coverity-ID: 115014 Signed-off-by: Gustavo A. R. Silva --- drivers/usb/serial/kobil_sct.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/serial/kobil_sct.c b/drivers/usb/serial/kobil_sct.c index 3024b9b..2f8fa35 100644 --- a/drivers/usb/serial/kobil_sct.c +++ b/drivers/usb/serial/kobil_sct.c @@ -491,6 +491,7 @@ static void kobil_set_termios(struct tty_struct *tty, break; default: speed = 9600; + /* fall through */ case 9600: urb_val = SUSBCR_SBR_9600; break; -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: gadget: f_phonet: mark expected switch fall-throughs
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. Addresses-Coverity-ID: 115004 Addresses-Coverity-ID: 115005 Signed-off-by: Gustavo A. R. Silva --- drivers/usb/gadget/function/f_phonet.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/usb/gadget/function/f_phonet.c b/drivers/usb/gadget/function/f_phonet.c index 9c4c58e..710b688 100644 --- a/drivers/usb/gadget/function/f_phonet.c +++ b/drivers/usb/gadget/function/f_phonet.c @@ -215,6 +215,7 @@ static void pn_tx_complete(struct usb_ep *ep, struct usb_request *req) case -ESHUTDOWN: /* disconnected */ case -ECONNRESET: /* disabled */ dev->stats.tx_aborted_errors++; + /* fall through */ default: dev->stats.tx_errors++; } @@ -362,6 +363,7 @@ static void pn_rx_complete(struct usb_ep *ep, struct usb_request *req) /* Do resubmit in these cases: */ case -EOVERFLOW: /* request buffer overflow */ dev->stats.rx_over_errors++; + /* fall through */ default: dev->stats.rx_errors++; break; -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: host: isp116x-hcd: mark expected switch fall-throughs
In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.
Addresses-Coverity-ID: 115006
Addresses-Coverity-ID: 115007
Signed-off-by: Gustavo A. R. Silva
---
drivers/usb/host/isp116x-hcd.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/usb/host/isp116x-hcd.c b/drivers/usb/host/isp116x-hcd.c
index 73fec38..ebf2ff2 100644
--- a/drivers/usb/host/isp116x-hcd.c
+++ b/drivers/usb/host/isp116x-hcd.c
@@ -1018,6 +1018,7 @@ static int isp116x_hub_control(struct usb_hcd *hcd,
spin_lock_irqsave(&isp116x->lock, flags);
isp116x_write_reg32(isp116x, HCRHSTATUS, RH_HS_OCIC);
spin_unlock_irqrestore(&isp116x->lock, flags);
+ /* fall through */
case C_HUB_LOCAL_POWER:
DBG("C_HUB_LOCAL_POWER\n");
break;
@@ -1433,8 +1434,10 @@ static int isp116x_bus_suspend(struct usb_hcd *hcd)
isp116x_write_reg32(isp116x, HCCONTROL,
(val & ~HCCONTROL_HCFS) |
HCCONTROL_USB_RESET);
+ /* fall through */
case HCCONTROL_USB_RESET:
ret = -EBUSY;
+ /* fall through */
default:/* HCCONTROL_USB_SUSPEND */
spin_unlock_irqrestore(&isp116x->lock, flags);
break;
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: atm: cxacru: mark expected switch fall-throughs
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. Signed-off-by: Gustavo A. R. Silva --- drivers/usb/atm/cxacru.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/usb/atm/cxacru.c b/drivers/usb/atm/cxacru.c index 600a670..e4f177d 100644 --- a/drivers/usb/atm/cxacru.c +++ b/drivers/usb/atm/cxacru.c @@ -424,6 +424,7 @@ static ssize_t cxacru_sysfs_store_adsl_state(struct device *dev, case CXPOLL_STOPPING: /* abort stop request */ instance->poll_state = CXPOLL_POLLING; + /* fall through */ case CXPOLL_POLLING: case CXPOLL_SHUTDOWN: /* don't start polling */ @@ -795,6 +796,7 @@ static int cxacru_atm_start(struct usbatm_data *usbatm_instance, case CXPOLL_STOPPING: /* abort stop request */ instance->poll_state = CXPOLL_POLLING; + /* fall through */ case CXPOLL_POLLING: case CXPOLL_SHUTDOWN: /* don't start polling */ -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: class: usbtmc: mark expected switch fall-through
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. Signed-off-by: Gustavo A. R. Silva --- drivers/usb/class/usbtmc.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 6ebfabf..6adceac 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -1343,6 +1343,7 @@ static void usbtmc_interrupt(struct urb *urb) case -EOVERFLOW: dev_err(dev, "overflow with length %d, actual length is %d\n", data->iin_wMaxPacketSize, urb->actual_length); + /* fall through */ case -ECONNRESET: case -ENOENT: case -ESHUTDOWN: -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: gadget: composite: mark expected switch fall-throughs
In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.
Signed-off-by: Gustavo A. R. Silva
---
drivers/usb/gadget/composite.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 5d061b3..1f1ab6f 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -170,20 +170,20 @@ int config_ep_by_speed(struct usb_gadget *g,
want_comp_desc = 1;
break;
}
- /* else: Fall trough */
+ /* fall through */
case USB_SPEED_SUPER:
if (gadget_is_superspeed(g)) {
speed_desc = f->ss_descriptors;
want_comp_desc = 1;
break;
}
- /* else: Fall trough */
+ /* fall through */
case USB_SPEED_HIGH:
if (gadget_is_dualspeed(g)) {
speed_desc = f->hs_descriptors;
break;
}
- /* else: fall through */
+ /* fall through */
default:
speed_desc = f->fs_descriptors;
}
@@ -224,6 +224,7 @@ int config_ep_by_speed(struct usb_gadget *g,
case USB_ENDPOINT_XFER_ISOC:
/* mult: bits 1:0 of bmAttributes */
_ep->mult = (comp_desc->bmAttributes & 0x3) + 1;
+ /* fall through */
case USB_ENDPOINT_XFER_BULK:
case USB_ENDPOINT_XFER_INT:
_ep->maxburst = comp_desc->bMaxBurst + 1;
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: gadget: udc: dummy_hcd: mark expected switch fall-throughs
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. Signed-off-by: Gustavo A. R. Silva --- drivers/usb/gadget/udc/dummy_hcd.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c index 664b64e..9dd26ff 100644 --- a/drivers/usb/gadget/udc/dummy_hcd.c +++ b/drivers/usb/gadget/udc/dummy_hcd.c @@ -569,10 +569,12 @@ static int dummy_enable(struct usb_ep *_ep, if (max <= 1024) break; /* save a return statement */ + /* fall through */ case USB_SPEED_FULL: if (max <= 64) break; /* save a return statement */ + /* fall through */ default: if (max <= 8) break; @@ -590,6 +592,7 @@ static int dummy_enable(struct usb_ep *_ep, if (max <= 1024) break; /* save a return statement */ + /* fall through */ case USB_SPEED_FULL: if (max <= 1023) break; -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/9] usb: host: xhci-mem: mark expected switch fall-through
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. Signed-off-by: Gustavo A. R. Silva --- drivers/usb/host/xhci-mem.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c index 795219a..57be885 100644 --- a/drivers/usb/host/xhci-mem.c +++ b/drivers/usb/host/xhci-mem.c @@ -1311,6 +1311,7 @@ static unsigned int xhci_get_endpoint_interval(struct usb_device *udev, * since it uses the same rules as low speed interrupt * endpoints. */ + /* fall through */ case USB_SPEED_LOW: if (usb_endpoint_xfer_int(&ep->desc) || -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 7/9] usb: host: oxu210hp-hcd: mark expected switch fall-through
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. Signed-off-by: Gustavo A. R. Silva --- drivers/usb/host/oxu210hp-hcd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/host/oxu210hp-hcd.c b/drivers/usb/host/oxu210hp-hcd.c index ed20fb3..9f8c61e 100644 --- a/drivers/usb/host/oxu210hp-hcd.c +++ b/drivers/usb/host/oxu210hp-hcd.c @@ -3040,7 +3040,7 @@ static void oxu_endpoint_disable(struct usb_hcd *hcd, qh_put(qh); break; } - /* else FALL THROUGH */ + /* fall through */ default: nogood: /* caller was supposed to have unlinked any requests; -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 6/9] usb: host: isp1362-hcd: mark expected switch fall-through
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. Signed-off-by: Gustavo A. R. Silva --- drivers/usb/host/isp1362-hcd.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/host/isp1362-hcd.c b/drivers/usb/host/isp1362-hcd.c index 9b7e307..753d576 100644 --- a/drivers/usb/host/isp1362-hcd.c +++ b/drivers/usb/host/isp1362-hcd.c @@ -1578,6 +1578,7 @@ static int isp1362_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, spin_lock_irqsave(&isp1362_hcd->lock, flags); isp1362_write_reg32(isp1362_hcd, HCRHSTATUS, RH_HS_OCIC); spin_unlock_irqrestore(&isp1362_hcd->lock, flags); + /* fall through */ case C_HUB_LOCAL_POWER: DBG(0, "C_HUB_LOCAL_POWER\n"); break; -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 9/9] usb: host: pci-quirks: mark expected switch fall-through
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. Signed-off-by: Gustavo A. R. Silva --- drivers/usb/host/pci-quirks.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c index 6dda362..6731f8d8d 100644 --- a/drivers/usb/host/pci-quirks.c +++ b/drivers/usb/host/pci-quirks.c @@ -841,7 +841,7 @@ static void quirk_usb_disable_ehci(struct pci_dev *pdev) ehci_bios_handoff(pdev, op_reg_base, cap, offset); break; case 0: /* Illegal reserved cap, set cap=0 so we exit */ - cap = 0; /* then fallthrough... */ + cap = 0; /* fall through */ default: dev_warn(&pdev->dev, "EHCI: unrecognized capability %02x\n", -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 8/9] usb: host: xhci-hub: mark expected switch fall-through
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. Signed-off-by: Gustavo A. R. Silva --- drivers/usb/host/xhci-hub.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c index 9762333..3693b1f 100644 --- a/drivers/usb/host/xhci-hub.c +++ b/drivers/usb/host/xhci-hub.c @@ -1377,6 +1377,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, break; case USB_PORT_FEAT_C_SUSPEND: bus_state->port_c_suspend &= ~(1 << wIndex); + /* fall through */ case USB_PORT_FEAT_C_RESET: case USB_PORT_FEAT_C_BH_PORT_RESET: case USB_PORT_FEAT_C_CONNECTION: -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/9] usb: host: isp1362-hcd: mark expected switch fall-through
Greg,
Quoting "Gustavo A. R. Silva" :
In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.
Signed-off-by: Gustavo A. R. Silva
---
drivers/usb/host/isp1362-hcd.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/host/isp1362-hcd.c b/drivers/usb/host/isp1362-hcd.c
index 9b7e307..753d576 100644
--- a/drivers/usb/host/isp1362-hcd.c
+++ b/drivers/usb/host/isp1362-hcd.c
@@ -1578,6 +1578,7 @@ static int isp1362_hub_control(struct usb_hcd
*hcd, u16 typeReq, u16 wValue,
spin_lock_irqsave(&isp1362_hcd->lock, flags);
isp1362_write_reg32(isp1362_hcd, HCRHSTATUS,
RH_HS_OCIC);
spin_unlock_irqrestore(&isp1362_hcd->lock, flags);
+ /* fall through */
I'm suspicious this should be a 'break' instead.
What do you think?
case C_HUB_LOCAL_POWER:
DBG(0, "C_HUB_LOCAL_POWER\n");
break;
--
2.7.4
Thanks
--
Gustavo A. R. Silva
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/9] usb: host: xhci: mark expected switch fall-through
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. Signed-off-by: Gustavo A. R. Silva --- drivers/usb/host/xhci.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index ee077a2..05db6e97 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -4294,6 +4294,7 @@ static unsigned long long xhci_calculate_intel_u1_timeout( break; } /* Otherwise the calculation is the same as isoc eps */ + /* fall through */ case USB_ENDPOINT_XFER_ISOC: timeout_ns = xhci_service_interval_to_ns(desc); timeout_ns = DIV_ROUND_UP_ULL(timeout_ns * 105, 100); -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/9] usb: host: ohci-hcd: mark expected switch fall-through
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. Signed-off-by: Gustavo A. R. Silva --- drivers/usb/host/ohci-hcd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c index 4492482..15ec8f9 100644 --- a/drivers/usb/host/ohci-hcd.c +++ b/drivers/usb/host/ohci-hcd.c @@ -382,7 +382,7 @@ ohci_endpoint_disable (struct usb_hcd *hcd, struct usb_host_endpoint *ep) ed_free (ohci, ed); break; } - /* else FALL THROUGH */ + /* fall through */ default: /* caller was supposed to have unlinked any requests; * that's not our job. can't recover; must leak ed. -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/9] mark expected switch fall-throughs
In preparation to enabling -Wimplicit-fallthrough, this patchset aims to mark switch cases where we are expecting to fall through. In Kees Cook words: "This is an unfortunate omission in the C language, and thankfully both gcc and clang have stepped up to solve this the same way static analyzers have solved it. It does both document the intention for humans and provide a way for analyzers to report issues. Having the compiler help us not make mistakes is quite handy." In some cases there were "else FALL THROUGH" or "then fallthrough..." comments already in place. So I replaced them with proper "fall through" comments, which is what GCC is expecting to find. Thanks! Gustavo A. R. Silva (9): usb: host: fotg210-hcd: mark expected switch fall-through usb: host: xhci: mark expected switch fall-through usb: host: xhci-mem: mark expected switch fall-through usb: host: ohci-hcd: mark expected switch fall-through usb: host: ehci-hcd: mark expected switch fall-through usb: host: isp1362-hcd: mark expected switch fall-through usb: host: oxu210hp-hcd: mark expected switch fall-through usb: host: xhci-hub: mark expected switch fall-through usb: host: pci-quirks: mark expected switch fall-through drivers/usb/host/ehci-hcd.c | 2 +- drivers/usb/host/fotg210-hcd.c | 2 +- drivers/usb/host/isp1362-hcd.c | 1 + drivers/usb/host/ohci-hcd.c | 2 +- drivers/usb/host/oxu210hp-hcd.c | 2 +- drivers/usb/host/pci-quirks.c | 2 +- drivers/usb/host/xhci-hub.c | 1 + drivers/usb/host/xhci-mem.c | 1 + drivers/usb/host/xhci.c | 1 + 9 files changed, 9 insertions(+), 5 deletions(-) -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/9] usb: host: fotg210-hcd: mark expected switch fall-through
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. Signed-off-by: Gustavo A. R. Silva --- drivers/usb/host/fotg210-hcd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/host/fotg210-hcd.c b/drivers/usb/host/fotg210-hcd.c index 457cc65..33a4f7e 100644 --- a/drivers/usb/host/fotg210-hcd.c +++ b/drivers/usb/host/fotg210-hcd.c @@ -5449,7 +5449,7 @@ static void fotg210_endpoint_disable(struct usb_hcd *hcd, qh_destroy(fotg210, qh); break; } - /* else FALL THROUGH */ + /* fall through */ default: /* caller was supposed to have unlinked any requests; * that's not our job. just leak this memory. -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/9] usb: host: ehci-hcd: mark expected switch fall-through
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. Signed-off-by: Gustavo A. R. Silva --- drivers/usb/host/ehci-hcd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index 6e834b83..c560a01 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -1012,7 +1012,7 @@ ehci_endpoint_disable (struct usb_hcd *hcd, struct usb_host_endpoint *ep) qh_destroy(ehci, qh); break; } - /* else FALL THROUGH */ + /* fall through */ default: /* caller was supposed to have unlinked any requests; * that's not our job. just leak this memory. -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: wusbcore: wa-xfer: mark expected switch fall-through
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. Signed-off-by: Gustavo A. R. Silva --- drivers/usb/wusbcore/wa-xfer.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/wusbcore/wa-xfer.c b/drivers/usb/wusbcore/wa-xfer.c index e70322b..bee2404 100644 --- a/drivers/usb/wusbcore/wa-xfer.c +++ b/drivers/usb/wusbcore/wa-xfer.c @@ -2156,6 +2156,7 @@ static void wa_complete_remaining_xfer_segs(struct wa_xfer *xfer, * do not increment RPIPE avail for the WA_SEG_DELAYED case * since it has not been submitted to the RPIPE. */ + /* fall through */ case WA_SEG_DELAYED: xfer->segs_done++; current_seg->status = status; -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: composite: mark expected switch fall-throughs
Hi Felipe, Quoting Felipe Balbi : "Gustavo A. R. Silva" writes: In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. Signed-off-by: Gustavo A. R. Silva Acked-by: Felipe Balbi Thank you for the ACKs. -- Gustavo A. R. Silva -- 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: storage: sddr55: mark expected switch fall-through
In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.
Signed-off-by: Gustavo A. R. Silva
---
drivers/usb/storage/sddr55.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/storage/sddr55.c b/drivers/usb/storage/sddr55.c
index 147c50b..b973da4 100644
--- a/drivers/usb/storage/sddr55.c
+++ b/drivers/usb/storage/sddr55.c
@@ -604,6 +604,7 @@ static unsigned long sddr55_get_capacity(struct us_data
*us) {
case 0x64:
info->pageshift = 8;
info->smallpageshift = 1;
+ /* fall through */
case 0x5d: // 5d is a ROM card with pagesize 512.
return 0x0020;
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: typec: tps6598x: mark expected switch fall-through
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. Signed-off-by: Gustavo A. R. Silva --- drivers/usb/typec/tps6598x.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c index 8893f79..b728d9e 100644 --- a/drivers/usb/typec/tps6598x.c +++ b/drivers/usb/typec/tps6598x.c @@ -403,6 +403,7 @@ static int tps6598x_probe(struct i2c_client *client) case TPS_PORTINFO_DRP_UFP_DRD: case TPS_PORTINFO_DRP_DFP_DRD: tps->typec_cap.dr_set = tps6598x_dr_set; + /* fall through */ case TPS_PORTINFO_DRP_UFP: case TPS_PORTINFO_DRP_DFP: tps->typec_cap.pr_set = tps6598x_pr_set; -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: serial: io_edgeport: mark expected switch fall-throughs
Quoting Bjørn Mork :
"Gustavo A. R. Silva" writes:
In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.
Notice that in this particular case I replaced "...drop on through"
comments with a proper "fall through" comment on its own line, which
is what GCC is expecting to find.
Sounds to me like GCC is the wrong tool for this. But I would of course
not mind if it was *just* the text. However, as your patch cleary
shows, the simplified logic leads to real problems:
@@ -1819,8 +1819,8 @@ static void process_rcvd_data(struct
edgeport_serial *edge_serial,
edge_serial->rxState = EXPECT_DATA;
break;
}
- /* Else, drop through */
}
+ /* fall through */
case EXPECT_DATA: /* Expect data */
if (bufferLength < edge_serial->rxBytesRemaining) {
rxLen = bufferLength;
The original comment clearly marked a *conditional* fall through at the
correct place. Your patch makes it appear as if there is an
unconditional fall through here. There is not. The fallthrough only
applies to one of a number of nested if blocks. There are no less than
3 break statements in the same case block.
I see.
You are right.
Not a big deal maybe, just as the lack of any "fall through" comment
isn't a big deal in the first place. But this change is clearly making
this code harder to read, and the change is therefore harmful IMHO.
If you can't make -Wimplicit-fallthrough work without removing such
precise fallthrough markings, then my proposal is to drop it and use
some other tool.
I will talk with the hardening guys to see what we can do about this.
I appreciate for your comments.
Thanks
--
Gustavo A. R. Silva
--
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
