Re: [PATCH] staging: usbip: stub_dev: Fixed oops during removal of usbip_host
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. 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. regards, dan carpenter -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] staging: usbip: stub_dev: Fixed oops during removal of usbip_host
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
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
Re: [PATCH] staging: usbip: stub_dev: Fixed oops during removal of usbip_host
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
Re: [PATCH] staging: usbip: stub_dev: Fixed oops during removal of usbip_host
On Tue, Sep 18, 2012 at 05:14:41PM +0530, navin patidar wrote: 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). It's case #5 which I would be worried about. Where did the original Oops happen? I feel like it really would be helpful to see it. I don't see which check for -status != SDEV_ST_AVAILABLE you're talking about here which prevents the pointers from being reused... thus stub_device_reset() can't be called without stub_shutdown_connection(), so there is no problem of resource leak . Except in the case of #5 obviously. 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 Generally, that's normal. If you want to ensure that a pointer isn't used again then you clear it immediately. I'm honestly just trying to figure this out. When I saw that the patch, I immediately thought *resource leak*. I'm sorry that to take your time up, but it shouldn't be that complicated that I have to go tracking through the whole driver to understand this. regards, dan carpenter -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] staging: usbip: stub_dev: Fixed oops during removal of usbip_host
On Fri, Sep 14, 2012 at 03:23:23PM +0530, navin patidar wrote: --- 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. --- As others have pointed out, I can not accept patches sent with this type of wording in them, sorry. Please fix your email system and try again. greg k-h -- 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
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: stub_dev: Fixed oops during removal of usbip_host
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
Re: [PATCH] staging: usbip: stub_dev: Fixed oops during removal of usbip_host
Hello. On 14-09-2012 13:53, 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. 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 It's the preferred comment style -- why modify it? * * 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*/ Add spaces after /* and before */, please. + ud-tcp_socket = NULL; + + /*reset kernel thread pointers */ Here too. WBR, Sergei -- 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
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: stub_dev: Fixed oops during removal of usbip_host
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
Hello. On 09/14/2012 04:36 PM, navin patidar wrote: hi Sergei, checkpatch.pl http://checkpatch.pl didn't complain any thing about patch coding style. checkpatch.pl only complains about // comments, IIRC. But see Documentation/CodingStyle chapter 8 about multi-line comments. As for adding spaces, it's more for aesthetic reasons... Anyway thanks for reviewing the patch. i will send this patch again with corrections. Thanks you. --navin-patidar WBR, Sergei -- 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
On Fri, Sep 14, 2012 at 06:51:33PM +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 wanted to put a sample Oops stack trace in here, that would always be welcome. It's not something to resend over. Just an idea. --- 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. --- I don't think this text is allowed on patches. This is a thing where you probably will need to resend the patch. regards, dan carpenter -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] staging: usbip: stub_dev: Fixed oops during removal of usbip_host
On Fri, Sep 14, 2012 at 05:16:38PM +0300, Dan Carpenter wrote: I don't think this text is allowed on patches. Oh Oh Oh Dan, I think you crossed a line here. You were not one of the recipient and you reviewed and forwarded the email but you should have deleted this email instead. Now legal action will be taken… regards, dan carpenter 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
Re: [PATCH] staging: usbip: stub_dev: Fixed oops during removal of usbip_host
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
Re: [PATCH] staging: usbip: stub_dev: Fixed oops during removal of usbip_host
On Fri, Sep 14, 2012 at 04:28:33PM +0200, Sebastian Andrzej Siewior wrote: On Fri, Sep 14, 2012 at 05:16:38PM +0300, Dan Carpenter wrote: I don't think this text is allowed on patches. Oh Oh Oh Dan, I think you crossed a line here. You were not one of the recipient and you reviewed and forwarded the email but you should have deleted this email instead. Now legal action will be taken… Oh crap! Well, it's a good thing I am a fully AWCP certified survivalist. I'm off to live in the mountains until the statute of limitations runs out. If you need me, then print your email out and leave it under a rock. 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