Re^2: [PATCH] usbnet: add reset_resume quirk to prevent resume failure

2016-07-03 Thread Vivek Kumar Bhagat 
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<da...@davemloft.net>
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

2016-06-30 Thread Vivek Kumar Bhagat 

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 <vivek.bha...@samsung.com>
Signed-off-by: Vikas Bansal <vikas.ban...@samsung.com>
Signed-off-by: Sangmin Bae <sm79@samsung.com>
---
 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

[PATCH] usbnet: dereference after null check in usbnet_start_xmit() and __usbnet_read_cmd()

2015-08-19 Thread Vivek Kumar Bhagat 
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 vivek.bha...@samsung.com
---
 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^2:: [PATCH] usbnet: dereference after null check in usbnet_start_xmit() and __usbnet_read_cmd()

2015-08-19 Thread Vivek Kumar Bhagat 
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] [befaa194] (ax88179_tx_fixup+0x0/0x16c 
[ax88179_178a]) from [befef7a8] (usbnet_start_xmit+0xa
09:14:5690/0x420 [usbnet])
09:14:570[0-553.5711]  r6:cb5ec280 r5: r4:cb5ebd40
09:14:572[0-553.5755] [befef708] (usbnet_start_xmit+0x0/0x420 [usbnet]) 
from [c0400040] (dev_hard_start_xmit+0x2ec/
09:14:5720x558)
09:14:572[0-553.5858] [c03ffd54] (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 Morkbj...@mork.no
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��٥

Re: Re: [PATCH] usbnet: dereference after null check in usbnet_start_xmit() and __usbnet_read_cmd()

2015-08-19 Thread Vivek Kumar Bhagat 
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 Morkbj...@mork.no
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: [PATCH] ax88179_178a: add reset functionality in reset_resume

2015-07-01 Thread Vivek Kumar Bhagat 
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 Bhagatvivek.bha...@samsung.com 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��٥

[EDT] [v2][PATCH] ax88179_178a: add reset function in reset_resume

2015-06-30 Thread Vivek Kumar Bhagat 
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 vivek.bha...@samsung.com
Signed-off-by: Praveen Kumar praveen@samsung.com

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

2015-06-30 Thread Vivek Kumar Bhagat 
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 vivek.bha...@samsung.com
Signed-off-by: Praveen Kumar praveen@samsung.com

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

2015-06-25 Thread Vivek Kumar Bhagat 
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 vivek.bha...@samsung.com
Signed-off-by: Praveen Kumar praveen@samsung.com
---
 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

2015-06-25 Thread Vivek Kumar Bhagat 
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