Re: [PATCH] staging: usbip: Fix build on Debian ppc

[navin patidar]

On September 19, 2013 at 3:40 PM Dominik Paulus domi...@d-paulus.de wrote:
 On Thu, Sep 19, 2013 at 12:12:44PM +0300, Dan Carpenter wrote:
  On Thu, Sep 19, 2013 at 10:55:18AM +0200, Tobias Polzer wrote:
   When testing usbip under powerpc, it (unexpectedly) worked, but only after
   removing the following lines from vhch_hcd.c:
  
   1004 /* will be removed */
   1005 if (pdev-dev.dma_mask) {
   1006 dev_info(pdev-dev, vhci_hcd DMA not supported\n);
   1007 return -EINVAL;
   1008 }
  
   We encountered no problems without those lines. Is it safe to remove this
   check?
 
  Hehe. No.
 
  Also which vhch_hcd.c are you talking about? find -name vhch_hcd.c
  doesn't show anything.

 Sorry for the typo, we meant vhci_hcd.c in
 drivers/staging/usbip/vhci_hcd.c.

  What the error message?

 When modprobing vhci_hcd.ko, vhci_hcd_probe() fails and returns -EINVAL,
 resulting in this error message:

 [ 592.623292] vhci_hcd vhci_hcd: vhci_hcd DMA not supported
 [ 592.624031] vhci_hcd: probe of vhci_hcd failed with error -22

 This leads to the virtual host controller device (vhci) not being
 created, so usbip cannot work.
 It works after removing the codeblock mentioned above.

 We did some research and discovered that dma_mask is only set on PowerPC
 at the moment (in arch/powerpc/kernel/setup-common.c,
 arch_setup_pdev_archdata()).

 All of our testing was done in QEMU emulating Debian on POWER7.


I have already submitted a patch to remove mentioned code block.
patch link : https://lkml.org/lkml/2013/9/10/26


regards,
--navin-patidar
--
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/3] staging: usbip: vhci_hcd: remove check for dma

[navin patidar]

vhci_hcd is a virtual usb host controller, so no need to
check for dma.

Signed-off-by: navin patidar nav...@cdac.in
---
 drivers/staging/usbip/vhci_hcd.c |6 --
 1 file changed, 6 deletions(-)

diff --git a/drivers/staging/usbip/vhci_hcd.c b/drivers/staging/usbip/vhci_hcd.c
index b3c9217..e810ad5 100644
--- a/drivers/staging/usbip/vhci_hcd.c
+++ b/drivers/staging/usbip/vhci_hcd.c
@@ -999,12 +999,6 @@ static int vhci_hcd_probe(struct platform_device *pdev)
 
usbip_dbg_vhci_hc(name %s id %d\n, pdev-name, pdev-id);
 
-   /* will be removed */
-   if (pdev-dev.dma_mask) {
-   dev_info(pdev-dev, vhci_hcd DMA not supported\n);
-   return -EINVAL;
-   }
-
/*
 * Allocate and initialize hcd.
 * Our private data is also allocated automatically.
-- 
1.7.10.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 2/3] staging: usbip: vhci_hcd: correctly handle return value

[navin patidar]

ret == 0 means success, anything else is failure.

Signed-off-by: navin patidar nav...@cdac.in
---
 drivers/staging/usbip/vhci_hcd.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/usbip/vhci_hcd.c b/drivers/staging/usbip/vhci_hcd.c
index d7974cb..b3c9217 100644
--- a/drivers/staging/usbip/vhci_hcd.c
+++ b/drivers/staging/usbip/vhci_hcd.c
@@ -1146,11 +1146,11 @@ static int __init vhci_hcd_init(void)
return -ENODEV;
 
ret = platform_driver_register(vhci_driver);
-   if (ret  0)
+   if (ret)
goto err_driver_register;
 
ret = platform_device_register(the_pdev);
-   if (ret  0)
+   if (ret)
goto err_platform_device_register;
 
pr_info(DRIVER_DESC  v USBIP_VERSION \n);
-- 
1.7.10.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/3] staging: usbip: stub_main: correctly handle return value

[navin patidar]

ret == 0 means success, anything else is failure.

Signed-off-by: navin patidar nav...@cdac.in
---
 drivers/staging/usbip/stub_main.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/usbip/stub_main.c 
b/drivers/staging/usbip/stub_main.c
index 33027cc..baf857f 100644
--- a/drivers/staging/usbip/stub_main.c
+++ b/drivers/staging/usbip/stub_main.c
@@ -255,14 +255,14 @@ static int __init usbip_host_init(void)
}
 
ret = usb_register(stub_driver);
-   if (ret  0) {
+   if (ret) {
pr_err(usb_register failed %d\n, ret);
goto err_usb_register;
}
 
ret = driver_create_file(stub_driver.drvwrap.driver,
 driver_attr_match_busid);
-   if (ret  0) {
+   if (ret) {
pr_err(driver_create_file failed\n);
goto err_create_file;
}
-- 
1.7.10.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 v4] staging: usbip: replace pr_warning() with dev_warn().

[navin patidar]

On Thu, Jul 25, 2013, Greg KH gre...@linuxfoundation.org said:

 On Thu, Jul 25, 2013 at 10:19:31AM +0530, navin patidar wrote:
  -pr_warning(Unable to start control thread\n);
  +struct device *dev;
  +
  +if (ud-side == USBIP_STUB)
  +dev = container_of(ud, struct stub_device, ud)-udev-dev;
  +else
  +dev = container_of(ud, struct vhci_device, ud)-udev-dev;
 
  Putting '' in front of container_of seems odd, are you sure it's needed
  here?  If ud is a pointer, everything else should just work properly
  without that.

 Removing ''   caused following error.
 drivers/staging/usbip/usbip_event.c: In function ‘usbip_start_eh’:
 drivers/staging/usbip/usbip_event.c:93:8: error: incompatible types
 when assigning to type ‘struct device *’ from type ‘struct device’
 drivers/staging/usbip/usbip_event.c:95:8: error: incompatible types
 when assigning to type ‘struct device *’ from type ‘struct device’

 dev needs to be initialized with address of dev (struct device ) which
 is member of struct usb_device pointed by the udev.

 To make it more clear i can change it to

 dev = (container_of(ud, struct vhci_device, ud)-udev-dev);

  or

 struct vhci_device *vdev = container_of(ud, struct vhci_device, ud);
 dev = (vdev-udev-dev);

 Or perhaps:
 dev = container_of(ud, struct stub_device, ud).udev-dev;

 container_of() returns stub_device pointer so container_of(ud,
struct stub_device, ud).udev-dev  won't  work.

 or -udev.dev; I don't remember which, but that should work, right?
udev is also a pointer to usb_device structure inside  stub_device structure.
-udev-dev   only will work.

v4 patch gets compiled without any error or warning.
and actually Joe Perches reviewed v3 of the patch and suggested changes.
https://lkml.org/lkml/2013/6/27/15

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

Re: [PATCH v4] staging: usbip: replace pr_warning() with dev_warn().

[navin patidar]

On Wed, Jul 24, 2013, Greg KH gre...@linuxfoundation.org said:

 On Thu, Jun 27, 2013 at 03:34:52PM +0530, navin patidar wrote:
 dev_warn() is preferred over pr_warning().

 container_of() is used to get usb_driver pointer from usbip_device container
 (stub_device or vhci_device), to get device structure required for 
 dev_warn().

 Signed-off-by: navin patidar nav...@cdac.in
 ---
  drivers/staging/usbip/usbip_event.c |   11 ++-
  1 file changed, 10 insertions(+), 1 deletion(-)

 diff --git a/drivers/staging/usbip/usbip_event.c 
 b/drivers/staging/usbip/usbip_event.c
 index 82123be..1f3a571 100644
 --- a/drivers/staging/usbip/usbip_event.c
 +++ b/drivers/staging/usbip/usbip_event.c
 @@ -21,6 +21,8 @@
  #include linux/export.h

  #include usbip_common.h
 +#include stub.h
 +#include vhci.h

  static int event_handler(struct usbip_device *ud)
  {
 @@ -85,7 +87,14 @@ int usbip_start_eh(struct usbip_device *ud)

  ud-eh = kthread_run(event_handler_loop, ud, usbip_eh);
  if (IS_ERR(ud-eh)) {
 -pr_warning(Unable to start control thread\n);
 +struct device *dev;
 +
 +if (ud-side == USBIP_STUB)
 +dev = container_of(ud, struct stub_device, ud)-udev-dev;
 +else
 +dev = container_of(ud, struct vhci_device, ud)-udev-dev;

 Putting '' in front of container_of seems odd, are you sure it's needed
 here?  If ud is a pointer, everything else should just work properly
 without that.

Removing ''   caused following error.
drivers/staging/usbip/usbip_event.c: In function ‘usbip_start_eh’:
drivers/staging/usbip/usbip_event.c:93:8: error: incompatible types
when assigning to type ‘struct device *’ from type ‘struct device’
drivers/staging/usbip/usbip_event.c:95:8: error: incompatible types
when assigning to type ‘struct device *’ from type ‘struct device’

dev needs to be initialized with address of dev (struct device ) which
is member of struct usb_device pointed by the udev.

To make it more clear i can change it to

dev = (container_of(ud, struct vhci_device, ud)-udev-dev);

 or

struct vhci_device *vdev = container_of(ud, struct vhci_device, ud);
dev = (vdev-udev-dev);

regards,
--navin-patidar
--
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] staging: usbip: replace pr_warning() with dev_warn().

[navin patidar]

dev_warn() is preferred over pr_warning().

container_of() is used to get usb_driver pointer from usbip_device container
(stub_device or vhci_device), to get device structure required for dev_warn().

Signed-off-by: navin patidar nav...@cdac.in
---
 drivers/staging/usbip/usbip_event.c |   11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/usbip/usbip_event.c 
b/drivers/staging/usbip/usbip_event.c
index 82123be..1f3a571 100644
--- a/drivers/staging/usbip/usbip_event.c
+++ b/drivers/staging/usbip/usbip_event.c
@@ -21,6 +21,8 @@
 #include linux/export.h

 #include usbip_common.h
+#include stub.h
+#include vhci.h

 static int event_handler(struct usbip_device *ud)
 {
@@ -85,7 +87,14 @@ int usbip_start_eh(struct usbip_device *ud)

ud-eh = kthread_run(event_handler_loop, ud, usbip_eh);
if (IS_ERR(ud-eh)) {
-   pr_warning(Unable to start control thread\n);
+   struct device *dev;
+
+   if (ud-side == USBIP_STUB)
+   dev = container_of(ud, struct stub_device, 
ud)-udev-dev;
+   else
+   dev = container_of(ud, struct vhci_device, 
ud)-udev-dev;
+
+   dev_warn(dev, Unable to start control thread\n);
return PTR_ERR(ud-eh);
}

--
1.7.10.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] staging: usbip: replace pr_warning() with pr_warn()

[navin patidar]

On Tue, Jun 25, 2013, Greg KH gre...@linuxfoundation.org said:

 On Tue, Jun 25, 2013 at 04:27:18PM +0530, navin patidar wrote:
 On Tue, Jun 25, 2013, Greg KH gre...@linuxfoundation.org said:

  On Fri, Jun 21, 2013 at 03:01:04PM +0530, navin patidar wrote:
  pr_warn() is preferred over pr_warning().
 
  And dev_warn() is preferred over both of them, can you convert the code
  to use that instead?
 

 struct device is not available in usbip_start_eh()  which is required
 for dev_warn().

 usbip_device's containers struct stub_device and struct  vhci_device
 both have member structure usb_device but inside usbip_start_eh(), it
 is difficult to determine  to which container struct usbip_device
 belongs, thus container_of() can not be used here to get struct
 usb_device.

 Then the code should be reworked in order to be able to properly
 determine that.


struct usbip_device has enum type usbip_side variable which can be
used to determine container of usbip_device.
I should have looked usbip_device struct more carefully.
sorry for previous wrong info.

regards,
--navin-patidar
--
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] staging: usbip: replace pr_warning() with dev_warn().

[navin patidar]

dev_warn() is preferred over pr_warning().

container_of() is used to get usb_driver pointer from usbip_device container
(stub_device or vhci_device), to get device structure required for dev_warn().

Signed-off-by: navin patidar nav...@cdac.in
---
 drivers/staging/usbip/usbip_event.c |   14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/usbip/usbip_event.c 
b/drivers/staging/usbip/usbip_event.c
index 82123be..3bd1b94 100644
--- a/drivers/staging/usbip/usbip_event.c
+++ b/drivers/staging/usbip/usbip_event.c
@@ -21,6 +21,8 @@
 #include linux/export.h

 #include usbip_common.h
+#include stub.h
+#include vhci.h

 static int event_handler(struct usbip_device *ud)
 {
@@ -85,7 +87,17 @@ int usbip_start_eh(struct usbip_device *ud)

ud-eh = kthread_run(event_handler_loop, ud, usbip_eh);
if (IS_ERR(ud-eh)) {
-   pr_warning(Unable to start control thread\n);
+   struct device dev;
+   if (ud-side == USBIP_STUB) {
+   struct stub_device *sdev;
+   sdev = container_of(ud, struct stub_device, ud);
+   dev = sdev-udev-dev;
+   } else {
+   struct vhci_device *vdev;
+   vdev = container_of(ud, struct vhci_device, ud);
+   dev = vdev-udev-dev;
+   }
+   dev_warn(dev, Unable to start control thread\n);
return PTR_ERR(ud-eh);
}

--
1.7.10.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 v3] staging: usbip: replace pr_warning() with dev_warn().

[navin patidar]

dev_warn() is preferred over pr_warning().

container_of() is used to get usb_driver pointer from usbip_device container
(stub_device or vhci_device), to get device structure required for dev_warn().

Signed-off-by: navin patidar nav...@cdac.in
---
 drivers/staging/usbip/usbip_event.c |   17 -
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/usbip/usbip_event.c 
b/drivers/staging/usbip/usbip_event.c
index 82123be..eca14b7 100644
--- a/drivers/staging/usbip/usbip_event.c
+++ b/drivers/staging/usbip/usbip_event.c
@@ -21,6 +21,8 @@
 #include linux/export.h

 #include usbip_common.h
+#include stub.h
+#include vhci.h

 static int event_handler(struct usbip_device *ud)
 {
@@ -85,7 +87,20 @@ int usbip_start_eh(struct usbip_device *ud)

ud-eh = kthread_run(event_handler_loop, ud, usbip_eh);
if (IS_ERR(ud-eh)) {
-   pr_warning(Unable to start control thread\n);
+   struct device dev;
+
+   if (ud-side == USBIP_STUB) {
+   struct stub_device *sdev;
+
+   sdev = container_of(ud, struct stub_device, ud);
+   dev = sdev-udev-dev;
+   } else {
+   struct vhci_device *vdev;
+
+   vdev = container_of(ud, struct vhci_device, ud);
+   dev = vdev-udev-dev;
+   }
+   dev_warn(dev, Unable to start control thread\n);
return PTR_ERR(ud-eh);
}

--
1.7.10.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] staging: usbip: replace pr_warning() with pr_warn()

[navin patidar]

On Tue, Jun 25, 2013, Greg KH gre...@linuxfoundation.org said:

 On Fri, Jun 21, 2013 at 03:01:04PM +0530, navin patidar wrote:
 pr_warn() is preferred over pr_warning().

 And dev_warn() is preferred over both of them, can you convert the code
 to use that instead?


struct device is not available in usbip_start_eh()  which is required
for dev_warn().

usbip_device's containers struct stub_device and struct  vhci_device
both have member structure usb_device but inside usbip_start_eh(), it
is difficult to determine  to which container struct usbip_device
belongs, thus container_of() can not be used here to get struct
usb_device.


regards,
--navin-patidar
--
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

[no subject]

[navin patidar]

On Tue, Jun 25, 2013, Greg KH gre...@linuxfoundation.org said:

 On Tue, Jun 25, 2013 at 04:27:18PM +0530, navin patidar wrote:
 On Tue, Jun 25, 2013, Greg KH gre...@linuxfoundation.org said:

  On Fri, Jun 21, 2013 at 03:01:04PM +0530, navin patidar wrote:
  pr_warn() is preferred over pr_warning().
 
  And dev_warn() is preferred over both of them, can you convert the code
  to use that instead?
 

 struct device is not available in usbip_start_eh()  which is required
 for dev_warn().

 usbip_device's containers struct stub_device and struct  vhci_device
 both have member structure usb_device but inside usbip_start_eh(), it
 is difficult to determine  to which container struct usbip_device
 belongs, thus container_of() can not be used here to get struct
 usb_device.

 Then the code should be reworked in order to be able to properly
 determine that.

How about adding a usb_device pointer in  usbip_device structure ?

regards,
--navin-patidar
--
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] staging: usbip: replace pr_warning() with pr_warn()

[navin patidar]

pr_warn() is preferred over pr_warning().

Signed-off-by: navin patidar nav...@cdac.in
---
 drivers/staging/usbip/usbip_event.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/usbip/usbip_event.c 
b/drivers/staging/usbip/usbip_event.c
index 82123be..64933b9 100644
--- a/drivers/staging/usbip/usbip_event.c
+++ b/drivers/staging/usbip/usbip_event.c
@@ -85,7 +85,7 @@ int usbip_start_eh(struct usbip_device *ud)
 
ud-eh = kthread_run(event_handler_loop, ud, usbip_eh);
if (IS_ERR(ud-eh)) {
-   pr_warning(Unable to start control thread\n);
+   pr_warn(Unable to start control thread\n);
return PTR_ERR(ud-eh);
}
 
-- 
1.7.10.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 v2] staging: usbip: vhci_hcd: Fixed oops during removal of vhci_hcd

[navin patidar]

From: navin nav...@cdac.in

In response to usbip detach -p [port_number] user 
command,vhci_shutdown_connection() gets
executed which kills tcp_tx,tcp_rx kernel threads but doesn't set thread 
pointers to NULL.
so, at the time of vhci_hcd removal vhci_shutdown_connection() again tries to 
kill kernel
threads which are already killed.

[  312.220259] BUG: unable to handle kernel NULL pointer dereference at 
  (null)
[  312.220313] IP: [8108186f] exit_creds+0x1f/0x70
[  312.220349] PGD 5b7be067 PUD 5b7dc067 PMD 0
[  312.220388] Oops:  [#1] SMP
[  312.220415] Modules linked in: vhci_hcd(O-) usbip_host(O) usbip_core(O) uas 
usb_storage joydev parport_pc bnep rfcomm ppdev binfmt_misc
snd_hda_codec_hdmi snd_hda_codec_conexant snd_hda_intel snd_hda_codec snd_hwdep 
snd_pcm snd_seq_midi arc4 snd_rawmidi snd_seq_midi_event snd_seq
ath9k mac80211 ath9k_common ath9k_hw snd_timer snd_seq_device snd ath i915 
drm_kms_helper drm psmouse cfg80211 coretemp soundcore lpc_ich i2c_algo_bit
snd_page_alloc video intel_ips wmi btusb mac_hid bluetooth ideapad_laptop lp 
sparse_keymap serio_raw mei microcode parport r8169
[  312.220862] CPU 0
[  312.220882] Pid: 2095, comm: usbip_eh Tainted: G   O 3.6.0-rc3+ #2 
LENOVO 20042   /Base Board Product Name
[  312.220938] RIP: 0010:[8108186f]  [8108186f] 
exit_creds+0x1f/0x70
[  312.220979] RSP: 0018:88004d6ebdb0  EFLAGS: 00010282
[  312.221008] RAX:  RBX: 880041c4db40 RCX: 88005ad52230
[  312.221041] RDX: 0034 RSI: 0286 RDI: 
[  312.221074] RBP: 88004d6ebdc0 R08: 88004d6ea000 R09: 
[  312.221107] R10: 0001 R11:  R12: 
[  312.221144] R13:  R14: 88005ad521d8 R15: 88004d6ebea0
[  312.221177] FS:  () GS:88007740() 
knlGS:
[  312.221214] CS:  0010 DS:  ES:  CR0: 8005003b
[  312.221241] CR2:  CR3: 5b7b9000 CR4: 07f0
[  312.221277] DR0:  DR1:  DR2: 
[  312.221309] DR3:  DR6: 0ff0 DR7: 0400
[  312.221342] Process usbip_eh (pid: 2095, threadinfo 88004d6ea000, task 
88004d44db40)
[  312.221384] Stack:
[  312.221396]  88004d6ebdc0 880041c4db40 88004d6ebde0 
81052aaa
[  312.221442]  880041c4db40  88004d6ebe10 
8107a148
[  312.221487]  88005ad521f0 88005ad52228 88004d44db40 
88005ad521d8
[  312.221536] Call Trace:
[  312.221556]  [81052aaa] __put_task_struct+0x4a/0x140
[  312.221587]  [8107a148] kthread_stop+0x108/0x110
[  312.221616]  [a0412569] vhci_shutdown_connection+0x49/0x2b4 
[vhci_hcd]
[  312.221657]  [a0139920] event_handler_loop+0x70/0x140 [usbip_core]
[  312.221692]  [8107ad30] ? add_wait_queue+0x60/0x60
[  312.221721]  [a01398b0] ? usbip_stop_eh+0x30/0x30 [usbip_core]
[  312.221754]  [8107a453] kthread+0x93/0xa0
[  312.221784]  [81670e84] kernel_thread_helper+0x4/0x10
[  312.221814]  [8107a3c0] ? flush_kthread_worker+0xb0/0xb0
[  312.223589]  [81670e80] ? gs_change+0x13/0x13
[  312.225360] Code: 0b 0f 0b 66 0f 1f 84 00 00 00 00 00 55 48 89 e5 53 48 83 
ec 08 66 66 66 66 90 48 8b 87 58 04 00 00 48 89 fb 48 8b bf 50 04 00 00
8b 00 48 c7 83 50 04 00 00 00 00 00 00 f0 ff 0f 0f 94 c0 84 c0
[  312.229663] RIP  [8108186f] exit_creds+0x1f/0x70
[  312.231696]  RSP 88004d6ebdb0
[  312.233648] CR2: 
[  312.256464] ---[ end trace 1971ce612a167279 ]---

Signed-off-by: navin patidar nav...@cdac.in
---
 drivers/staging/usbip/vhci_hcd.c |9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/usbip/vhci_hcd.c b/drivers/staging/usbip/vhci_hcd.c
index 12a9a5f..67358d4 100644
--- a/drivers/staging/usbip/vhci_hcd.c
+++ b/drivers/staging/usbip/vhci_hcd.c
@@ -804,11 +804,14 @@ static void vhci_shutdown_connection(struct usbip_device 
*ud)
}
 
/* kill threads related to this sdev, if v.c. exists */
-   if (vdev-ud.tcp_rx)
+   if (vdev-ud.tcp_rx) {
kthread_stop_put(vdev-ud.tcp_rx);
-   if (vdev-ud.tcp_tx)
+   vdev-ud.tcp_rx = NULL;
+   }
+   if (vdev-ud.tcp_tx) {
kthread_stop_put(vdev-ud.tcp_tx);
-
+   vdev-ud.tcp_tx = NULL;
+   }
pr_info(stop threads\n);
 
/* active connection is closed */
-- 
1.7.9.5

--
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] staging: usbip: stub_dev: Fixed oops during removal of usbip_host

[navin patidar]

From: navin nav...@cdac.in

stub_shutdown_connection() should set kernel thread pointers to NULL after 
killing them.
so that at the time of usbip_host removal stub_shutdown_connection() doesn't 
try to kill kernel threads
which are already killed.

[ 1504.312158] BUG: unable to handle kernel NULL pointer dereference at 
  (null)
[ 1504.315833] IP: [8108186f] exit_creds+0x1f/0x70
[ 1504.317688] PGD 41f1c067 PUD 41d0f067 PMD 0
[ 1504.319611] Oops:  [#2] SMP
[ 1504.321480] Modules linked in: vhci_hcd(O) usbip_host(O-) usbip_core(O) uas 
usb_storage joydev parport_pc bnep rfcomm ppdev binfmt_misc
snd_hda_codec_hdmi snd_hda_codec_conexant snd_hda_intel snd_hda_codec snd_hwdep 
snd_pcm snd_seq_midi arc4 snd_rawmidi snd_seq_midi_event
snd_seq ath9k mac80211 ath9k_common ath9k_hw snd_timer snd_seq_device snd ath 
i915 drm_kms_helper drm psmouse cfg80211 coretemp soundcore
lpc_ich i2c_algo_bit snd_page_alloc video intel_ips wmi btusb mac_hid bluetooth 
ideapad_laptop lp sparse_keymap serio_raw mei microcode parport r8169
[ 1504.329666] CPU 1
[ 1504.329687] Pid: 2434, comm: usbip_eh Tainted: G  DO 3.6.0-rc31+ #2 
LENOVO 20042   /Base Board Product Name
[ 1504.333502] RIP: 0010:[8108186f]  [8108186f] 
exit_creds+0x1f/0x70
[ 1504.335371] RSP: 0018:880041c7fdd0  EFLAGS: 00010282
[ 1504.337226] RAX:  RBX: 880041c2db40 RCX: 880041e4ae50
[ 1504.339101] RDX: 0044 RSI: 0286 RDI: 
[ 1504.341027] RBP: 880041c7fde0 R08: 880041c7e000 R09: 
[ 1504.342934] R10: 0001 R11:  R12: 
[ 1504.344840] R13:  R14: 88004d448000 R15: 880041c7fea0
[ 1504.346743] FS:  () GS:88007744() 
knlGS:
[ 1504.348671] CS:  0010 DS:  ES:  CR0: 8005003b
[ 1504.350612] CR2:  CR3: 41d0d000 CR4: 07e0
[ 1504.352723] DR0:  DR1:  DR2: 
[ 1504.354734] DR3:  DR6: 0ff0 DR7: 0400
[ 1504.356737] Process usbip_eh (pid: 2434, threadinfo 880041c7e000, task 
88004d448000)
[ 1504.358711] Stack:
[ 1504.360635]  880041c7fde0 880041c2db40 880041c7fe00 
81052aaa
[ 1504.362589]  880041c2db40  880041c7fe30 
8107a148
[ 1504.364539]  880041e4ae10 880041e4ae00 88004d448000 
88004d448000
[ 1504.366470] Call Trace:
[ 1504.368368]  [81052aaa] __put_task_struct+0x4a/0x140
[ 1504.370307]  [8107a148] kthread_stop+0x108/0x110
[ 1504.370312]  [a040da4e] stub_shutdown_connection+0x3e/0x1b0 
[usbip_host]
[ 1504.370315]  [a03fd920] event_handler_loop+0x70/0x140 [usbip_core]
[ 1504.370318]  [8107ad30] ? add_wait_queue+0x60/0x60
[ 1504.370320]  [a03fd8b0] ? usbip_stop_eh+0x30/0x30 [usbip_core]
[ 1504.370322]  [8107a453] kthread+0x93/0xa0
[ 1504.370327]  [81670e84] kernel_thread_helper+0x4/0x10
[ 1504.370330]  [8107a3c0] ? flush_kthread_worker+0xb0/0xb0
[ 1504.370332]  [81670e80] ? gs_change+0x13/0x13
[ 1504.370351] Code: 0b 0f 0b 66 0f 1f 84 00 00 00 00 00 55 48 89 e5 53 48 83 
ec 08 66 66 66 66 90 48 8b 87 58 04 00 00 48 89 fb 48 8b bf 50 04 00 00
8b 00 48 c7 83 50 04 00 00 00 00 00 00 f0 ff 0f 0f 94 c0 84 c0
[ 1504.370353] RIP  [8108186f] exit_creds+0x1f/0x70
[ 1504.370353]  RSP 880041c7fdd0
[ 1504.370354] CR2: 
[ 1504.401376] ---[ end trace 1971ce612a16727a ]---

Signed-off-by: navin patidar nav...@cdac.in
---
 drivers/staging/usbip/stub_dev.c |8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/usbip/stub_dev.c b/drivers/staging/usbip/stub_dev.c
index 92ced35..c8d79a7 100644
--- a/drivers/staging/usbip/stub_dev.c
+++ b/drivers/staging/usbip/stub_dev.c
@@ -187,10 +187,14 @@ static void stub_shutdown_connection(struct usbip_device 
*ud)
}
 
/* 1. stop threads */
-   if (ud-tcp_rx)
+   if (ud-tcp_rx) {
kthread_stop_put(ud-tcp_rx);
-   if (ud-tcp_tx)
+   ud-tcp_rx = NULL;
+   }
+   if (ud-tcp_tx) {
kthread_stop_put(ud-tcp_tx);
+   ud-tcp_tx = NULL;
+   }
 
/*
 * 2. close the socket
-- 
1.7.9.5

--
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] staging: usbip: stub_dev: Fixed oops during removal of usbip_host

[navin patidar]

On Tue, Sep 18, 2012 at 1:10 PM, Dan Carpenter dan.carpen...@oracle.com wrote:
 On Tue, Sep 18, 2012 at 09:30:06AM +0530, navin patidar wrote:
 stub_device_reset should set kernel thread pointers to NULL.
 so that at the time of usbip_host removal stub_shoutdown_connection
 doesn't try to kill kernel threads which are already killed.


 If you have the Oops output, that's always nice to put in the commit
 message.

i'll surely keep this in mind  before submitting further patches.

 Why don't you set the pointers to NULL in stub_shutdown_connection()
 since that's where you actually kill the threads.  Setting them to
 NULL in stub_device_reset() will (sometimes) solve the problem but
 it gives you a new problem of a resource leak.

 stub_device_reset() always gets executed after
stub_shutdown_connection() , never before.



 regards,
 dan carpenter


--navin-patidar
--
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] staging: usbip: stub_dev: Fixed oops during removal of usbip_host

[navin patidar]

for usbip_host  event_handler()   handles following events.  defined
in usbip_common.h

 1. SDEV_EVENT_REMOVED   (USBIP_EH_SHUTDOWN | USBIP_EH_RESET | USBIP_EH_BYE)
 2. SDEV_EVENT_DOWN (USBIP_EH_SHUTDOWN | USBIP_EH_RESET)
 3. SDEV_EVENT_ERROR_TCP(USBIP_EH_SHUTDOWN | USBIP_EH_RESET)
 4. SDEV_EVENT_ERROR_SUBMIT (USBIP_EH_SHUTDOWN | USBIP_EH_RESET)
 5. VDEV_EVENT_ERROR_MALLOC (USBIP_EH_SHUTDOWN | USBIP_EH_UNUSABLE)

In case of events(1,2,3,4),  stub_shoutdown_connection() gets executed
first and than stub_device_reset() .

In case of event 5, stub_shoutdown_connection()  kills kernel threads
and stub_device_unusable()   changes devices status to
SDEV_ST_ERROR(fatal error).

 thus stub_device_reset() can't  be called without
stub_shutdown_connection(), so there is no problem of resource leak .
you are also right, i could have set pointers to  NULL in
stub_shutdown_connection() but i used  stub_device_reset() which is
intended to reset usbip_device stuct member variables.

i'll resend patches, if maintainer ask for that.
thanks

--navin-patidar


On Tue, Sep 18, 2012 at 3:06 PM, Dan Carpenter dan.carpen...@oracle.com wrote:
 On Tue, Sep 18, 2012 at 03:02:15PM +0530, navin patidar wrote:
 On Tue, Sep 18, 2012 at 1:10 PM, Dan Carpenter dan.carpen...@oracle.com 
 wrote:
  On Tue, Sep 18, 2012 at 09:30:06AM +0530, navin patidar wrote:
  stub_device_reset should set kernel thread pointers to NULL.
  so that at the time of usbip_host removal stub_shoutdown_connection
  doesn't try to kill kernel threads which are already killed.
 
 
  If you have the Oops output, that's always nice to put in the commit
  message.

 i'll surely keep this in mind  before submitting further patches.

  Why don't you set the pointers to NULL in stub_shutdown_connection()
  since that's where you actually kill the threads.  Setting them to
  NULL in stub_device_reset() will (sometimes) solve the problem but
  it gives you a new problem of a resource leak.

  stub_device_reset() always gets executed after
 stub_shutdown_connection() , never before.


 No it isn't.  Read event_handler() more carefully.  They can be
 executed independently.

 In other words, stub_shutdown_connection() can be called without
 calling stub_device_reset() and stub_device_reset() can be called
 without stub_shutdown_connection().  If either of those happen then
 it causes a problem with the patch you have just sent.

 regards,
 dan carpenter

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

[PATCH] staging: usbip: stub_dev: Fixed oops during removal of usbip_host

[navin patidar]

stub_device_reset should set kernel thread pointers to NULL.
so that at the time of usbip_host removal stub_shoutdown_connection
doesn't try to kill kernel threads which are already killed.

Signed-off-by: navin patidar nav...@cdac.in
---
 drivers/staging/usbip/stub_dev.c |7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/usbip/stub_dev.c b/drivers/staging/usbip/stub_dev.c
index 92ced35..447a98c 100644
--- a/drivers/staging/usbip/stub_dev.c
+++ b/drivers/staging/usbip/stub_dev.c
@@ -198,10 +198,8 @@ static void stub_shutdown_connection(struct usbip_device 
*ud)
 * tcp_socket is freed after threads are killed so that usbip_xmit does
 * not touch NULL socket.
 */
-   if (ud-tcp_socket) {
+   if (ud-tcp_socket)
sock_release(ud-tcp_socket);
-   ud-tcp_socket = NULL;
-   }
 
/* 3. free used data */
stub_device_cleanup_urbs(sdev);
@@ -233,6 +231,9 @@ static void stub_device_reset(struct usbip_device *ud)
 
dev_dbg(udev-dev, device reset);
 
+   ud-tcp_socket = NULL;
+   ud-tcp_rx = NULL;
+   ud-tcp_tx = NULL;
ret = usb_lock_device_for_reset(udev, sdev-interface);
if (ret  0) {
dev_err(udev-dev, lock for reset\n);
-- 
1.7.9.5

--
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] staging: usbip: vhci_hcd: Fixed oops during removal of vhci_hcd

[navin patidar]

In response to usbip detach -p [port_number] user 
command,vhci_shoutdown_connection gets
executed which kills tcp_tx,tcp_rx kernel threads and then vhci_device_reset 
resets
all usb_device struct variables except kernel thread pointers.

so, at the time of vhci_hcd removal vhci_shoutdown_connection tries to kill 
kernel threads
which are already killed.

Signed-off-by: navin patidar nav...@cdac.in
---
 drivers/staging/usbip/vhci_hcd.c |2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/usbip/vhci_hcd.c b/drivers/staging/usbip/vhci_hcd.c
index 12a9a5f..eea8298 100644
--- a/drivers/staging/usbip/vhci_hcd.c
+++ b/drivers/staging/usbip/vhci_hcd.c
@@ -859,6 +859,8 @@ static void vhci_device_reset(struct usbip_device *ud)
usb_put_dev(vdev-udev);
vdev-udev = NULL;
 
+   ud-tcp_rx = NULL;
+   ud-tcp_tx = NULL;
ud-tcp_socket = NULL;
ud-status = VDEV_ST_NULL;
 
-- 
1.7.9.5

--
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] staging: usbip: vhci_hcd: Fixed oops during removal of vhci_hcd

[navin patidar]

In response to usbip detach -p [port_number] user 
command,vhci_shoutdown_connection gets
executed which kills tcp_tx,tcp_rx kernel threads and then vhci_device_reset 
resets
all usb_device struct variables except kernel thread pointers.

so, at the time of vhci_hcd removal vhci_shoutdown_connection tries to kill 
kernel threads
which are already killed.

Signed-off-by: navin patidar nav...@cdac.in
---
 drivers/staging/usbip/vhci_hcd.c |3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/usbip/vhci_hcd.c b/drivers/staging/usbip/vhci_hcd.c
index 6076f42..b1f427c 100644
--- a/drivers/staging/usbip/vhci_hcd.c
+++ b/drivers/staging/usbip/vhci_hcd.c
@@ -858,6 +858,9 @@ static void vhci_device_reset(struct usbip_device *ud)
if (vdev-udev)
usb_put_dev(vdev-udev);
vdev-udev = NULL;
+   /* Reset kernel thread pointers */
+   ud-tcp_rx = NULL;
+   ud-tcp_tx = NULL;
 
ud-tcp_socket = NULL;
ud-status = VDEV_ST_NULL;
-- 
1.7.9.5


---

This e-mail is for the sole use of the intended recipient(s) and may
contain confidential and privileged information. If you are not the
intended recipient, please contact the sender by reply e-mail and destroy
all copies and the original message. Any unauthorized review, use,
disclosure, dissemination, forwarding, printing or copying of this email
is strictly prohibited and appropriate legal action will be taken.
---

--
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] staging: usbip: stub_dev: Fixed oops during removal of usbip_host

[navin patidar]

stub_device_reset should set kernel thread pointers to NULL.
so that at the time of usbip_host removal stub_shoutdown_connection
doesn't try to kill kernel threads which are already killed.

Signed-off-by: navin patidar nav...@cdac.in
---
 drivers/staging/usbip/stub_dev.c |   14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/usbip/stub_dev.c b/drivers/staging/usbip/stub_dev.c
index 92ced35..f584af8 100644
--- a/drivers/staging/usbip/stub_dev.c
+++ b/drivers/staging/usbip/stub_dev.c
@@ -192,16 +192,13 @@ static void stub_shutdown_connection(struct usbip_device 
*ud)
if (ud-tcp_tx)
kthread_stop_put(ud-tcp_tx);
 
-   /*
-* 2. close the socket
+   /* 2. close the socket
 *
 * tcp_socket is freed after threads are killed so that usbip_xmit does
 * not touch NULL socket.
 */
-   if (ud-tcp_socket) {
+   if (ud-tcp_socket)
sock_release(ud-tcp_socket);
-   ud-tcp_socket = NULL;
-   }
 
/* 3. free used data */
stub_device_cleanup_urbs(sdev);
@@ -233,6 +230,13 @@ static void stub_device_reset(struct usbip_device *ud)
 
dev_dbg(udev-dev, device reset);
 
+   /*reset tcp socket*/
+   ud-tcp_socket = NULL;
+
+   /*reset kernel thread pointers */
+   ud-tcp_rx = NULL;
+   ud-tcp_tx = NULL;
+
ret = usb_lock_device_for_reset(udev, sdev-interface);
if (ret  0) {
dev_err(udev-dev, lock for reset\n);
-- 
1.7.9.5


---

This e-mail is for the sole use of the intended recipient(s) and may
contain confidential and privileged information. If you are not the
intended recipient, please contact the sender by reply e-mail and destroy
all copies and the original message. Any unauthorized review, use,
disclosure, dissemination, forwarding, printing or copying of this email
is strictly prohibited and appropriate legal action will be taken.
---

--
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] staging: usbip: stub_dev: Fixed oops during removal of usbip_host

[navin patidar]

stub_device_reset should set kernel thread pointers to NULL.
so that at the time of usbip_host removal stub_shoutdown_connection
doesn't try to kill kernel threads which are already killed.

Signed-off-by: navin patidar nav...@cdac.in
---
 drivers/staging/usbip/stub_dev.c |7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/usbip/stub_dev.c b/drivers/staging/usbip/stub_dev.c
index 92ced35..447a98c 100644
--- a/drivers/staging/usbip/stub_dev.c
+++ b/drivers/staging/usbip/stub_dev.c
@@ -198,10 +198,8 @@ static void stub_shutdown_connection(struct usbip_device 
*ud)
 * tcp_socket is freed after threads are killed so that usbip_xmit does
 * not touch NULL socket.
 */
-   if (ud-tcp_socket) {
+   if (ud-tcp_socket)
sock_release(ud-tcp_socket);
-   ud-tcp_socket = NULL;
-   }
 
/* 3. free used data */
stub_device_cleanup_urbs(sdev);
@@ -233,6 +231,9 @@ static void stub_device_reset(struct usbip_device *ud)
 
dev_dbg(udev-dev, device reset);
 
+   ud-tcp_socket = NULL;
+   ud-tcp_rx = NULL;
+   ud-tcp_tx = NULL;
ret = usb_lock_device_for_reset(udev, sdev-interface);
if (ret  0) {
dev_err(udev-dev, lock for reset\n);
-- 
1.7.9.5


---

This e-mail is for the sole use of the intended recipient(s) and may
contain confidential and privileged information. If you are not the
intended recipient, please contact the sender by reply e-mail and destroy
all copies and the original message. Any unauthorized review, use,
disclosure, dissemination, forwarding, printing or copying of this email
is strictly prohibited and appropriate legal action will be taken.
---

--
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] staging: usbip: stub_dev: Fixed oops during removal of usbip_host

[navin patidar]

hi,

I have sent this patch again with corrections.
thank for reviewing the patch.

--navin-patidar

On 9/14/12, Sebastian Andrzej Siewior sebast...@breakpoint.cc wrote:
 On Fri, Sep 14, 2012 at 03:42:42PM +0400, Sergei Shtylyov wrote:
 diff --git a/drivers/staging/usbip/stub_dev.c
  b/drivers/staging/usbip/stub_dev.c
 index 92ced35..f584af8 100644
 --- a/drivers/staging/usbip/stub_dev.c
 +++ b/drivers/staging/usbip/stub_dev.c
 @@ -233,6 +230,13 @@ static void stub_device_reset(struct usbip_device
  *ud)
 
 dev_dbg(udev-dev, device reset);
 
 +   /*reset tcp socket*/

Add spaces after /* and before */, please.

 +   ud-tcp_socket = NULL;
 +
 +   /*reset kernel thread pointers */

Here too.

 I'm not sure it is required to comment the obvious things.

 WBR, Sergei

 Sebastian

--
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] staging: usbip: vhci_hcd: fixed suspend-resume loop

[navin patidar]

USB autosuspend suspends vhci_hcd. In this process hcd_bus_suspend gets 
executed which puts vhci_hcd in suspend state and calls vhci_hub_status.
vhci_hub_status function checks hub state and if it is in suspend state, 
usb_hcd_resume_root_hub gets executed which resumes hub
and if hub is idle, again autosuspend puts it in suspend state and this goes on.

vhci_hub_status should resume hub only when hub port is in suspend state and 
hub port status has changed.

Signed-off-by: navin patidar nav...@cdac.in
---
 drivers/staging/usbip/vhci_hcd.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/usbip/vhci_hcd.c b/drivers/staging/usbip/vhci_hcd.c
index 12a9a5f..6076f42 100644
--- a/drivers/staging/usbip/vhci_hcd.c
+++ b/drivers/staging/usbip/vhci_hcd.c
@@ -220,7 +220,7 @@ static int vhci_hub_status(struct usb_hcd *hcd, char *buf)
 
pr_info(changed %d\n, changed);
 
-   if (hcd-state == HC_STATE_SUSPENDED)
+   if ((hcd-state == HC_STATE_SUSPENDED)  (changed == 1))
usb_hcd_resume_root_hub(hcd);
 
 done:
-- 
1.7.9.5


---

This e-mail is for the sole use of the intended recipient(s) and may
contain confidential and privileged information. If you are not the
intended recipient, please contact the sender by reply e-mail and destroy
all copies and the original message. Any unauthorized review, use,
disclosure, dissemination, forwarding, printing or copying of this email
is strictly prohibited and appropriate legal action will be taken.
---

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