Re^2: [PATCH] usbnet: add reset_resume quirk to prevent resume failure
Hello David, Class driver .reset_resume() function will be called only when USB host driver has set below flag, otherwise it will follow normal resume (without reset). if (udev->quirks & USB_QUIRK_RESET_RESUME) <-- usb_resume_device() udev->reset_resume = 1; I shall remove cdc_eem changes, then it would be applicable for asix 2.0 device only. I shall make one patch for asix 2.0 (asix_devices.c) and asix 3.0 (ax88179_178a.c). We have verified on almost all type of available asix dongle. I hope it would be okay. Regards, Vivek --- Original Message --- Sender : David Miller Date : Jul 02, 2016 01:28 (GMT+05:30) Title : Re: [PATCH] usbnet: add reset_resume quirk to prevent resume failure From: Vivek Kumar Bhagat Date: Thu, 30 Jun 2016 10:41:59 + (GMT) > > Ideally, usbnet_resume is sufficient for device resume operation. > since usbcore function usb_resume_device() sets udev->reset_resume > flag as a quirk solution, reset_resume() quirk we can keep in > class driver also. We checked on dongle DA-Queen UFE20C, > without reset function resume operation fails. Reason could be > some power glitch during suspend time due to which device lose > its internal state and it needs a device phy reset again during > resume to recover. > > Signed-off-by: Vivek Kumar Bhagat > Signed-off-by: Vikas Bansal > Signed-off-by: Sangmin Bae You've seen this necessary for one device, yet you are doing this new reset for all usbnet devices. That doesn't sound correct or safe at all. I'm not applying this, sorry.
[PATCH] usbnet: add reset_resume quirk to prevent resume failure
Ideally, usbnet_resume is sufficient for device resume operation. since usbcore function usb_resume_device() sets udev->reset_resume flag as a quirk solution, reset_resume() quirk we can keep in class driver also. We checked on dongle DA-Queen UFE20C, without reset function resume operation fails. Reason could be some power glitch during suspend time due to which device lose its internal state and it needs a device phy reset again during resume to recover. Signed-off-by: Vivek Kumar Bhagat Signed-off-by: Vikas Bansal Signed-off-by: Sangmin Bae --- drivers/net/usb/asix_devices.c |1 + drivers/net/usb/cdc_eem.c |1 + drivers/net/usb/usbnet.c | 17 + include/linux/usb/usbnet.h |1 + 4 files changed, 20 insertions(+) diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c index 5cabefc..68dda61 100644 --- a/drivers/net/usb/asix_devices.c +++ b/drivers/net/usb/asix_devices.c @@ -1097,6 +1097,7 @@ static struct usb_driver asix_driver = { .probe =usbnet_probe, .suspend = usbnet_suspend, .resume = usbnet_resume, + .reset_resume = usbnet_reset_resume, .disconnect = usbnet_disconnect, .supports_autosuspend = 1, .disable_hub_initiated_lpm = 1, diff --git a/drivers/net/usb/cdc_eem.c b/drivers/net/usb/cdc_eem.c index f7180f8..83b6857 100644 --- a/drivers/net/usb/cdc_eem.c +++ b/drivers/net/usb/cdc_eem.c @@ -371,6 +371,7 @@ static struct usb_driver eem_driver = { .disconnect = usbnet_disconnect, .suspend = usbnet_suspend, .resume = usbnet_resume, + .reset_resume = usbnet_reset_resume, .disable_hub_initiated_lpm = 1, }; diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index 61ba464..f56c176 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -1876,6 +1876,23 @@ int usbnet_resume (struct usb_interface *intf) } EXPORT_SYMBOL_GPL(usbnet_resume); +int usbnet_reset_resume(struct usb_interface *intf) +{ + struct usbnet *dev = usb_get_intfdata(intf); + int ret; + + if (dev->driver_info->reset) { + ret = dev->driver_info->reset(dev); + if (ret < 0) + return ret; + + return usbnet_resume(intf); + } + + return -EINVAL; +} +EXPORT_SYMBOL_GPL(usbnet_reset_resume); + /* * Either a subdriver implements manage_power, then it is assumed to always * be ready to be suspended or it reports the readiness to be suspended diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h index 6e0ce8c..0b261d5 100644 --- a/include/linux/usb/usbnet.h +++ b/include/linux/usb/usbnet.h @@ -177,6 +177,7 @@ struct driver_info { extern int usbnet_probe(struct usb_interface *, const struct usb_device_id *); extern int usbnet_suspend(struct usb_interface *, pm_message_t); extern int usbnet_resume(struct usb_interface *); +extern int usbnet_reset_resume(struct usb_interface *); extern void usbnet_disconnect(struct usb_interface *); extern void usbnet_device_suggests_idle(struct usbnet *dev); -- 1.7.9.5
Re: Re: [PATCH] usbnet: dereference after null check in usbnet_start_xmit() and __usbnet_read_cmd()
Dear Bjorn, >>This is wrong. There are usbnet minidrivers depending on info->tx_fixup >> being called with a NULL skb. Also, if dev_hard_start_xmit() ensures that skb can not be NULL in usbnet_start_xmit() then we should remove below check. if (skb) <--- This check is confusing which says skb can be NULL. skb_tx_timestamp(skb); Best Regards, Vivek --- Original Message --- Sender : Bjørn Mork Date : Aug 19, 2015 17:21 (GMT+05:30) Title : Re: [PATCH] usbnet: dereference after null check in usbnet_start_xmit() and __usbnet_read_cmd() Vivek Kumar Bhagat writes: > usbnet_start_xmit() - If info->tx_fixup is not defined by class driver, > NULL check does not happen for skb pointer and leads to NULL dereference. > __usbnet_read_cmd() - if data pointer is passed as NULL, memcpy will > dereference NULL pointer. That's two completely different issues. Mixing them in a single patch is only confusing things. > Signed-off-by: Vivek Kumar Bhagat > --- > drivers/net/usb/usbnet.c |5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c > index 3c86b10..ec4d224 100644 > --- a/drivers/net/usb/usbnet.c > +++ b/drivers/net/usb/usbnet.c > @@ -1294,6 +1294,8 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb, > > if (skb) > skb_tx_timestamp(skb); > + else > + goto drop; > > // some devices want funky USB-level framing, for > // win32 driver (usually) and/or hardware quirks This is wrong. There are usbnet minidrivers depending on info->tx_fixup being called with a NULL skb. > @@ -1906,7 +1908,8 @@ static int __usbnet_read_cmd(struct usbnet *dev, u8 > cmd, u8 reqtype, > buf = kmalloc(size, GFP_KERNEL); > if (!buf) > goto out; > - } > + } else > + goto out; > > err = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0), > cmd, reqtype, value, index, buf, size, This is also wrong. It makes __usbnet_read_cmd() return -ENOMEM if called with a NULL data pointer. I don't know if it is used, but it's perfectly valid to call __usbnet_read_cmd() with data == NULL if size == 0. No memcpy will happen in this case because usb_control_msg can only return 0 or an error Please don't submit any more such patches without proper justification. You cannot trust that someone will actually take the time to sanity check your changes. Patches claiming to fix a NULL dereference should at least provide an oops. Bjørn
Re^2:: [PATCH] usbnet: dereference after null check in usbnet_start_xmit() and __usbnet_read_cmd()
Dear Bjorn, >> This is wrong. There are usbnet minidrivers depending on info->tx_fixup >> being called with a NULL skb. I am using ax88179_178a driver and not familiar with usbnet minidrivers. When ax88179_tx_fixup() is called with NULL skb from usbnet_start_xmit(), I get a kernel crash. Backtrace: 09:14:569>[0-553.5598] [] (ax88179_tx_fixup+0x0/0x16c [ax88179_178a]) from [] (usbnet_start_xmit+0xa 09:14:569>0/0x420 [usbnet]) 09:14:570>[0-553.5711] r6:cb5ec280 r5: r4:cb5ebd40 09:14:572>[0-553.5755] [] (usbnet_start_xmit+0x0/0x420 [usbnet]) from [] (dev_hard_start_xmit+0x2ec/ 09:14:572>0x558) 09:14:572>[0-553.5858] [] (dev_hard_start_xmit+0x0/0x558) In case skb NULL check can not be added in usbnet_start_xmit() then NULL check is required in ax88179_tx_fixup() or asix_tx_fixup(). >> This is also wrong. It makes __usbnet_read_cmd() return -ENOMEM if >> called with a NULL data pointer. >>No memcpy will happen in this case because usb_control_msg >>can only return 0 or an error Yes, some of the USB control message is sent from host to gadget without expecting any data in return. This is my mistake. I apologise for this. Best Regards, Vivek --- Original Message --- Sender : Bjørn Mork Date : Aug 19, 2015 17:21 (GMT+05:30) Title : Re: [PATCH] usbnet: dereference after null check in usbnet_start_xmit() and __usbnet_read_cmd() Vivek Kumar Bhagat writes: > usbnet_start_xmit() - If info->tx_fixup is not defined by class driver, > NULL check does not happen for skb pointer and leads to NULL dereference. > __usbnet_read_cmd() - if data pointer is passed as NULL, memcpy will > dereference NULL pointer. That's two completely different issues. Mixing them in a single patch is only confusing things. > Signed-off-by: Vivek Kumar Bhagat > --- > drivers/net/usb/usbnet.c |5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c > index 3c86b10..ec4d224 100644 > --- a/drivers/net/usb/usbnet.c > +++ b/drivers/net/usb/usbnet.c > @@ -1294,6 +1294,8 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb, > > if (skb) > skb_tx_timestamp(skb); > + else > + goto drop; > > // some devices want funky USB-level framing, for > // win32 driver (usually) and/or hardware quirks This is wrong. There are usbnet minidrivers depending on info->tx_fixup being called with a NULL skb. > @@ -1906,7 +1908,8 @@ static int __usbnet_read_cmd(struct usbnet *dev, u8 > cmd, u8 reqtype, > buf = kmalloc(size, GFP_KERNEL); > if (!buf) > goto out; > - } > + } else > + goto out; > > err = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0), > cmd, reqtype, value, index, buf, size, This is also wrong. It makes __usbnet_read_cmd() return -ENOMEM if called with a NULL data pointer. I don't know if it is used, but it's perfectly valid to call __usbnet_read_cmd() with data == NULL if size == 0. No memcpy will happen in this case because usb_control_msg can only return 0 or an error Please don't submit any more such patches without proper justification. You cannot trust that someone will actually take the time to sanity check your changes. Patches claiming to fix a NULL dereference should at least provide an oops. BjørnN�r��yb�X��ǧv�^�){.n�+���z�^�)w*jg����ݢj/���z�ޖ��2�ޙ&�)ߡ�a�����G���h��j:+v���w��٥
[PATCH] usbnet: dereference after null check in usbnet_start_xmit() and __usbnet_read_cmd()
usbnet_start_xmit() - If info->tx_fixup is not defined by class driver, NULL check does not happen for skb pointer and leads to NULL dereference. __usbnet_read_cmd() - if data pointer is passed as NULL, memcpy will dereference NULL pointer. Signed-off-by: Vivek Kumar Bhagat --- drivers/net/usb/usbnet.c |5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index 3c86b10..ec4d224 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -1294,6 +1294,8 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb, if (skb) skb_tx_timestamp(skb); + else + goto drop; // some devices want funky USB-level framing, for // win32 driver (usually) and/or hardware quirks @@ -1906,7 +1908,8 @@ static int __usbnet_read_cmd(struct usbnet *dev, u8 cmd, u8 reqtype, buf = kmalloc(size, GFP_KERNEL); if (!buf) goto out; - } + } else + goto out; err = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0), cmd, reqtype, value, index, buf, size, -- 1.7.9.5
Re: [PATCH] ax88179_178a: add reset functionality in reset_resume
Hello David, I configured my email client and below patch does not have indentation issue of converting tabs into spaces. I hope it should be accepted. Thanks, Vivek --- Original Message --- Sender : Vivek Kumar Bhagat Chief Engineer/SRI-Delhi-System S/W 1 Team/Samsung Electronics Date : Jul 01, 2015 09:44 (GMT+05:30) Title : [PATCH] ax88179_178a: add reset functionality in reset_resume Without reset functionality in reset_resume, iperf connection does not establish after suspend/resume however ping works at the same time. iperf connection fails by giving checksum error in tcpdump. reset function inside reset_resume solves above bug. We have verified it on ASIX based ST Lab, Cadyce dongle. Signed-off-by: Vivek Kumar Bhagat Signed-off-by: Praveen Kumar --- drivers/net/usb/ax88179_178a.c | 14 +- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c index e6338c1..00928c0 100644 --- a/drivers/net/usb/ax88179_178a.c +++ b/drivers/net/usb/ax88179_178a.c @@ -1630,6 +1630,18 @@ static int ax88179_stop(struct usbnet *dev) return 0; } +static int ax88179_reset_resume(struct usb_interface *intf) +{ + struct usbnet *dev = usb_get_intfdata(intf); + int ret; + + ret = ax88179_reset(dev); + if (ret < 0) + return ret; + + return ax88179_resume(intf); +} + static const struct driver_info ax88179_info = { .description = "ASIX AX88179 USB 3.0 Gigabit Ethernet", .bind = ax88179_bind, @@ -1744,7 +1756,7 @@ static struct usb_driver ax88179_178a_driver = { .probe = usbnet_probe, .suspend = ax88179_suspend, .resume = ax88179_resume, - .reset_resume = ax88179_resume, + .reset_resume = ax88179_reset_resume, .disconnect = usbnet_disconnect, .supports_autosuspend = 1, .disable_hub_initiated_lpm = 1, -- 1.7.9.5N�r��yb�X��ǧv�^�){.n�+���z�^�)w*jg����ݢj/���z�ޖ��2�ޙ&�)ߡ�a�����G���h��j:+v���w��٥
[PATCH] ax88179_178a: add reset functionality in reset_resume
Without reset functionality in reset_resume, iperf connection does not establish after suspend/resume however ping works at the same time. iperf connection fails by giving checksum error in tcpdump. reset function inside reset_resume solves above bug. We have verified it on ASIX based ST Lab, Cadyce dongle. Signed-off-by: Vivek Kumar Bhagat Signed-off-by: Praveen Kumar --- drivers/net/usb/ax88179_178a.c | 14 +- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c index e6338c1..00928c0 100644 --- a/drivers/net/usb/ax88179_178a.c +++ b/drivers/net/usb/ax88179_178a.c @@ -1630,6 +1630,18 @@ static int ax88179_stop(struct usbnet *dev) return 0; } +static int ax88179_reset_resume(struct usb_interface *intf) +{ + struct usbnet *dev = usb_get_intfdata(intf); + int ret; + + ret = ax88179_reset(dev); + if (ret < 0) + return ret; + + return ax88179_resume(intf); +} + static const struct driver_info ax88179_info = { .description = "ASIX AX88179 USB 3.0 Gigabit Ethernet", .bind = ax88179_bind, @@ -1744,7 +1756,7 @@ static struct usb_driver ax88179_178a_driver = { .probe =usbnet_probe, .suspend = ax88179_suspend, .resume = ax88179_resume, - .reset_resume = ax88179_resume, + .reset_resume = ax88179_reset_resume, .disconnect = usbnet_disconnect, .supports_autosuspend = 1, .disable_hub_initiated_lpm = 1, -- 1.7.9.5N�r��yb�X��ǧv�^�){.n�+���z�^�)w*jg����ݢj/���z�ޖ��2�ޙ&�)ߡ�a�����G���h��j:+v���w��٥
[EDT] [v2][PATCH] ax88179_178a: add reset function in reset_resume
EP-EC562D6B53594479BCA6FC73F17DEE54 Without reset functionality in reset_resume, iperf connection does not establish after suspend/resume however ping works at the same time. iperf connection fails by giving checksum error in tcpdump. reset function inside reset_resume solves above bug. We have verified it on ASIX based ST Lab, Cadyce dongle. Signed-off-by: Vivek Kumar Bhagat Signed-off-by: Praveen Kumar --- drivers/net/usb/ax88179_178a.c | 14 +- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c index e6338c1..00928c0 100644 --- a/drivers/net/usb/ax88179_178a.c +++ b/drivers/net/usb/ax88179_178a.c @@ -1630,6 +1630,18 @@ static int ax88179_stop(struct usbnet *dev) return 0; } +static int ax88179_reset_resume(struct usb_interface *intf) +{ + struct usbnet *dev = usb_get_intfdata(intf); + int ret; + + ret = ax88179_reset(dev); + if (ret < 0) + return ret; + + return ax88179_resume(intf); +} + static const struct driver_info ax88179_info = { .description = "ASIX AX88179 USB 3.0 Gigabit Ethernet", .bind = ax88179_bind, @@ -1744,7 +1756,7 @@ static struct usb_driver ax88179_178a_driver = { .probe =usbnet_probe, .suspend = ax88179_suspend, .resume = ax88179_resume, - .reset_resume = ax88179_resume, + .reset_resume = ax88179_reset_resume, .disconnect = usbnet_disconnect, .supports_autosuspend = 1, .disable_hub_initiated_lpm = 1, -- 1.7.9.5N�r��yb�X��ǧv�^�){.n�+���z�^�)w*jg����ݢj/���z�ޖ��2�ޙ&�)ߡ�a�����G���h��j:+v���w��٥
[EDT][PATCH] ax88179_178a: add reset function in reset_resume
EP-EC562D6B53594479BCA6FC73F17DEE54 Without reset functionality in reset_resume, iperf connection does not establish after suspend/resume however ping works at the same time. reset function inside reset_resume solves above bug. We have verified it on ASIX based ST Lab, Cadyce dongle. Signed-off-by: Vivek Kumar Bhagat Signed-off-by: Praveen Kumar --- drivers/net/usb/ax88179_178a.c | 14 +- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c index e6338c1..00928c0 100644 --- a/drivers/net/usb/ax88179_178a.c +++ b/drivers/net/usb/ax88179_178a.c @@ -1630,6 +1630,18 @@ static int ax88179_stop(struct usbnet *dev) return 0; } +static int ax88179_reset_resume(struct usb_interface *intf) +{ + struct usbnet *dev = usb_get_intfdata(intf); + int ret; + + ret = ax88179_reset(dev); + if (ret < 0) + return ret; + + return ax88179_resume(intf); +} + static const struct driver_info ax88179_info = { .description = "ASIX AX88179 USB 3.0 Gigabit Ethernet", .bind = ax88179_bind, @@ -1744,7 +1756,7 @@ static struct usb_driver ax88179_178a_driver = { .probe =usbnet_probe, .suspend = ax88179_suspend, .resume = ax88179_resume, - .reset_resume = ax88179_resume, + .reset_resume = ax88179_reset_resume, .disconnect = usbnet_disconnect, .supports_autosuspend = 1, .disable_hub_initiated_lpm = 1, -- 1.7.9.5N�r��yb�X��ǧv�^�){.n�+���z�^�)w*jg����ݢj/���z�ޖ��2�ޙ&�)ߡ�a�����G���h��j:+v���w��٥
[EDT] [PATCH] ax88179_178a: add reset function in reset_resume
EP-EC562D6B53594479BCA6FC73F17DEE54 Hello David, without reset functionality in reset_resume, iperf connection does not establish after suspend/resume however ping works at the same time. reset function inside reset_resume solves above bug. We have verified it on ASIX based ST Lab, Cadyce dongle. As my email client is giving problem of converting tabs into spaces, Please find patch attached herewith. Thanks, Vivek 0001-ax88179_178a-add-reset-function-in-reset_resume.patch Description: Binary data