[PATCH] fix hso soft-lockup

[PATCH] fix hso soft-lockup

[Antti Kaijanmäki]

[-- Attachment #1: Type: text/plain, Size: 2234 bytes --]

Fix soft-lockup in hso.c which is triggered on SMP machine when
modem is removed while file descriptor(s) under /dev are still open:

  old version called kref_put() too early which resulted in destroying
  hso_serial and hso_device objects which were still used later on.

Also fix driver debug routines (not compiled in by default).
---
 drivers/net/usb/hso.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index fa4e581..539642a 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -2,6 +2,7 @@
  *
  * Driver for Option High Speed Mobile Devices.
  *
+ *  Copyright (C) 2009 Antti Kaijanmäki <antti.kaijanmaki@nomovok.com>
  *  Copyright (C) 2008 Option International
  *                     Filip Aben <f.aben@option.com>
  *                     Denis Joseph Barrow <d.barow@option.com>
@@ -378,7 +379,7 @@ static void dbg_dump(int line_count, const char *func_name, unsigned char *buf,
 }
 
 #define DUMP(buf_, len_)	\
-	dbg_dump(__LINE__, __func__, buf_, len_)
+	dbg_dump(__LINE__, __func__, (unsigned char*)buf_, len_)
 
 #define DUMP1(buf_, len_)			\
 	do {					\
@@ -1363,7 +1364,7 @@ static void hso_serial_close(struct tty_struct *tty, struct file *filp)
 	/* reset the rts and dtr */
 	/* do the actual close */
 	serial->open_count--;
-	kref_put(&serial->parent->ref, hso_serial_ref_free);
+
 	if (serial->open_count <= 0) {
 		serial->open_count = 0;
 		spin_lock_irq(&serial->serial_lock);
@@ -1383,6 +1384,8 @@ static void hso_serial_close(struct tty_struct *tty, struct file *filp)
 		usb_autopm_put_interface(serial->parent->interface);
 
 	mutex_unlock(&serial->parent->mutex);
+
+	kref_put(&serial->parent->ref, hso_serial_ref_free);
 }
 
 /* close the requested serial port */
@@ -1527,7 +1530,7 @@ static void tiocmget_intr_callback(struct urb *urb)
 		dev_warn(&usb->dev,
 			 "hso received invalid serial state notification\n");
 		DUMP(serial_state_notification,
-		     sizeof(hso_serial_state_notifation))
+		     sizeof(struct hso_serial_state_notification));
 	} else {
 
 		UART_state_bitmap = le16_to_cpu(serial_state_notification->
-- 
1.6.3.3


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] fix hso soft-lockup

[Antti Kaijanmäki]

[-- Attachment #1: Type: text/plain, Size: 559 bytes --]

On Thu, 2009-10-22 at 11:36 +0300, Antti Kaijanmäki wrote:
> Fix soft-lockup in hso.c which is triggered on SMP machine when
> modem is removed while file descriptor(s) under /dev are still open:
> 
>   old version called kref_put() too early which resulted in destroying
>   hso_serial and hso_device objects which were still used later on.
> 
> Also fix driver debug routines (not compiled in by default).

I created a new bug report out of this with some additional info:

    http://bugzilla.kernel.org/show_bug.cgi?id=14469


 -- Antti


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] fix hso soft-lockup

[Greg KH]

On Thu, Oct 22, 2009 at 11:36:18AM +0300, Antti Kaijanmäki wrote:
> Fix soft-lockup in hso.c which is triggered on SMP machine when
> modem is removed while file descriptor(s) under /dev are still open:
> 
>   old version called kref_put() too early which resulted in destroying
>   hso_serial and hso_device objects which were still used later on.
> 
> Also fix driver debug routines (not compiled in by default).

Patches need to have a "signed-off-by:" line in order to be applied.

Also, please do not gpg sign your patches, that only causes problems
with our tools.

> ---
>  drivers/net/usb/hso.c |    9 ++++++---
>  1 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
> index fa4e581..539642a 100644
> --- a/drivers/net/usb/hso.c
> +++ b/drivers/net/usb/hso.c
> @@ -2,6 +2,7 @@
>   *
>   * Driver for Option High Speed Mobile Devices.
>   *
> + *  Copyright (C) 2009 Antti Kaijanmäki <antti.kaijanmaki@nomovok.com>

Adding a copyright for a few lines changed is not really correct.

>   *  Copyright (C) 2008 Option International
>   *                     Filip Aben <f.aben@option.com>
>   *                     Denis Joseph Barrow <d.barow@option.com>
> @@ -378,7 +379,7 @@ static void dbg_dump(int line_count, const char *func_name, unsigned char *buf,
>  }
>  
>  #define DUMP(buf_, len_)	\
> -	dbg_dump(__LINE__, __func__, buf_, len_)
> +	dbg_dump(__LINE__, __func__, (unsigned char*)buf_, len_)

Why did you mane this change?

>  
>  #define DUMP1(buf_, len_)			\
>  	do {					\
> @@ -1363,7 +1364,7 @@ static void hso_serial_close(struct tty_struct *tty, struct file *filp)
>  	/* reset the rts and dtr */
>  	/* do the actual close */
>  	serial->open_count--;
> -	kref_put(&serial->parent->ref, hso_serial_ref_free);
> +
>  	if (serial->open_count <= 0) {
>  		serial->open_count = 0;
>  		spin_lock_irq(&serial->serial_lock);
> @@ -1383,6 +1384,8 @@ static void hso_serial_close(struct tty_struct *tty, struct file *filp)
>  		usb_autopm_put_interface(serial->parent->interface);
>  
>  	mutex_unlock(&serial->parent->mutex);
> +
> +	kref_put(&serial->parent->ref, hso_serial_ref_free);
>  }
>  
>  /* close the requested serial port */
> @@ -1527,7 +1530,7 @@ static void tiocmget_intr_callback(struct urb *urb)
>  		dev_warn(&usb->dev,
>  			 "hso received invalid serial state notification\n");
>  		DUMP(serial_state_notification,
> -		     sizeof(hso_serial_state_notifation))
> +		     sizeof(struct hso_serial_state_notification));

Is this a build fix not related to the bug above?

Also, network patches need to be sent to the network maintainers, they
usually miss them if you only copy the linux-kernel list.

Try using the scripts/get_maintainer.pl script on your patch to figure
out who to send the patch to, and also the scripts/checkpatch.pl script
to verify that you got everything correct before you send it off.

Also, for a bugfix like this, it would be good to send it to the
stable@kernel.org developers when it goes into Linus's tree as others
are reporting problems with this driver in the 2.6.31 kernel.

thanks,

greg k-h

Re: [PATCH] fix hso soft-lockup

[Antti Kaijanmäki]

On Mon, 2009-10-26 at 10:15 -0700, Greg KH wrote:
> On Thu, Oct 22, 2009 at 11:36:18AM +0300, Antti Kaijanmäki wrote:
> > Fix soft-lockup in hso.c which is triggered on SMP machine when
> > modem is removed while file descriptor(s) under /dev are still open:
> > 
> >   old version called kref_put() too early which resulted in destroying
> >   hso_serial and hso_device objects which were still used later on.
> > 
> > Also fix driver debug routines (not compiled in by default).
> 
> Patches need to have a "signed-off-by:" line in order to be applied.
> 
> Also, please do not gpg sign your patches, that only causes problems
> with our tools.

OK. I'll sent a new patch tomorrow.


> > ---
> >  drivers/net/usb/hso.c |    9 ++++++---
> >  1 files changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
> > index fa4e581..539642a 100644
> > --- a/drivers/net/usb/hso.c
> > +++ b/drivers/net/usb/hso.c
> > @@ -2,6 +2,7 @@
> >   *
> >   * Driver for Option High Speed Mobile Devices.
> >   *
> > + *  Copyright (C) 2009 Antti Kaijanmäki <antti.kaijanmaki@nomovok.com>
> 
> Adding a copyright for a few lines changed is not really correct.

Well, it depends on policy. Some might argue that if you make a change
of any sort you must include your copyright notice. I also used a fair
amount of time to track down the problem so this is not just a
whitespace fix or something. But I understand if this is not considered
substantial enough to justify copyright notice and will leave this out
from the revised patch.


> >   *  Copyright (C) 2008 Option International
> >   *                     Filip Aben <f.aben@option.com>
> >   *                     Denis Joseph Barrow <d.barow@option.com>
> > @@ -378,7 +379,7 @@ static void dbg_dump(int line_count, const char *func_name, unsigned char *buf,
> >  }
> >  
> >  #define DUMP(buf_, len_)	\
> > -	dbg_dump(__LINE__, __func__, buf_, len_)
> > +	dbg_dump(__LINE__, __func__, (unsigned char*)buf_, len_)
> 
> Why did you mane this change?

This fixes compile time warning about type mismatch if DEBUG is defined.
Please, see below.


> >  
> >  #define DUMP1(buf_, len_)			\
> >  	do {					\
> > @@ -1363,7 +1364,7 @@ static void hso_serial_close(struct tty_struct *tty, struct file *filp)
> >  	/* reset the rts and dtr */
> >  	/* do the actual close */
> >  	serial->open_count--;
> > -	kref_put(&serial->parent->ref, hso_serial_ref_free);
> > +
> >  	if (serial->open_count <= 0) {
> >  		serial->open_count = 0;
> >  		spin_lock_irq(&serial->serial_lock);
> > @@ -1383,6 +1384,8 @@ static void hso_serial_close(struct tty_struct *tty, struct file *filp)
> >  		usb_autopm_put_interface(serial->parent->interface);
> >  
> >  	mutex_unlock(&serial->parent->mutex);
> > +
> > +	kref_put(&serial->parent->ref, hso_serial_ref_free);
> >  }
> >  
> >  /* close the requested serial port */
> > @@ -1527,7 +1530,7 @@ static void tiocmget_intr_callback(struct urb *urb)
> >  		dev_warn(&usb->dev,
> >  			 "hso received invalid serial state notification\n");
> >  		DUMP(serial_state_notification,
> > -		     sizeof(hso_serial_state_notifation))
> > +		     sizeof(struct hso_serial_state_notification));
> 
> Is this a build fix not related to the bug above?

No, as I commented on commit log message the patch also fixes the debug
routines. They have to be enabled by hand by uncommenting a DEBUG define
on the top of the file so this is not anything fatal, just minor
inconvenience.

I thought the debug routines were not important enough to have their own
patch and could be included in this patch as the patch is very small and
the debug routines do not affect any default functionality. Naturally I
can remove the debug routine fix if you want. It's trivial for anyone to
fix them when needed :)


> Also, network patches need to be sent to the network maintainers, they
> usually miss them if you only copy the linux-kernel list.

> Try using the scripts/get_maintainer.pl script on your patch to figure
> out who to send the patch to, and also the scripts/checkpatch.pl script
> to verify that you got everything correct before you send it off.

Thanks! Didn't know about those.


> Also, for a bugfix like this, it would be good to send it to the
> stable@kernel.org developers when it goes into Linus's tree as others
> are reporting problems with this driver in the 2.6.31 kernel.

I'll CC the revised patch there, too.

Thanks for your comments!

  -- Antti

Re: [PATCH] fix hso soft-lockup

[Greg KH]

On Mon, Oct 26, 2009 at 09:37:35PM +0200, Antti Kaijanmäki wrote:
> > > ---
> > >  drivers/net/usb/hso.c |    9 ++++++---
> > >  1 files changed, 6 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
> > > index fa4e581..539642a 100644
> > > --- a/drivers/net/usb/hso.c
> > > +++ b/drivers/net/usb/hso.c
> > > @@ -2,6 +2,7 @@
> > >   *
> > >   * Driver for Option High Speed Mobile Devices.
> > >   *
> > > + *  Copyright (C) 2009 Antti Kaijanmäki <antti.kaijanmaki@nomovok.com>
> > 
> > Adding a copyright for a few lines changed is not really correct.
> 
> Well, it depends on policy. Some might argue that if you make a change
> of any sort you must include your copyright notice. I also used a fair
> amount of time to track down the problem so this is not just a
> whitespace fix or something. But I understand if this is not considered
> substantial enough to justify copyright notice and will leave this out
> from the revised patch.

According to some lawyers whom I have discussed this with, you need to
have written at least 1/3 of the lines in the file to be able to claim
this in the file itself.  That might not be true, but is a good
guideline that I've used over the years.

You always own the copyright to your individual change though, and git
preserves that information for all time as well.

> > > @@ -1527,7 +1530,7 @@ static void tiocmget_intr_callback(struct urb *urb)
> > >  		dev_warn(&usb->dev,
> > >  			 "hso received invalid serial state notification\n");
> > >  		DUMP(serial_state_notification,
> > > -		     sizeof(hso_serial_state_notifation))
> > > +		     sizeof(struct hso_serial_state_notification));
> > 
> > Is this a build fix not related to the bug above?
> 
> No, as I commented on commit log message the patch also fixes the debug
> routines. They have to be enabled by hand by uncommenting a DEBUG define
> on the top of the file so this is not anything fatal, just minor
> inconvenience.
> 
> I thought the debug routines were not important enough to have their own
> patch and could be included in this patch as the patch is very small and
> the debug routines do not affect any default functionality. Naturally I
> can remove the debug routine fix if you want. It's trivial for anyone to
> fix them when needed :)

Yes, that should be a new patch, especially as it would not be needed
to fix older kernels for the original bug.

So, care to send 2 patches?  The debug one isn't needed to be sent to
the stable@kernel.org address.

thanks,

greg k-h

[PATCH] hso: fix debug routines

[Antti Kaijanmäki]

On Mon, 2009-10-26 at 12:40 -0700, Greg KH wrote:
> Yes, that should be a new patch, especially as it would not be needed
> to fix older kernels for the original bug.
> 
> So, care to send 2 patches?  The debug one isn't needed to be sent to
> the stable@kernel.org address.


Signed-off-by: Antti Kaijanmäki <antti.kaijanmaki@nomovok.com>
---
 drivers/net/usb/hso.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index fa4e581..746839b 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -378,7 +378,7 @@ static void dbg_dump(int line_count, const char *func_name, unsigned char *buf,
 }
 
 #define DUMP(buf_, len_)	\
-	dbg_dump(__LINE__, __func__, buf_, len_)
+	dbg_dump(__LINE__, __func__, (unsigned char *)buf_, len_)
 
 #define DUMP1(buf_, len_)			\
 	do {					\
@@ -1527,7 +1527,7 @@ static void tiocmget_intr_callback(struct urb *urb)
 		dev_warn(&usb->dev,
 			 "hso received invalid serial state notification\n");
 		DUMP(serial_state_notification,
-		     sizeof(hso_serial_state_notifation))
+		     sizeof(struct hso_serial_state_notification));
 	} else {
 
 		UART_state_bitmap = le16_to_cpu(serial_state_notification->
-- 
1.6.3.3

Re: [PATCH] hso: fix debug routines

[Andrew Morton]

On Tue, 27 Oct 2009 16:26:55 +0200
Antti Kaijanmäki <antti.kaijanmaki@nomovok.com> wrote:

> On Mon, 2009-10-26 at 12:40 -0700, Greg KH wrote:
> > Yes, that should be a new patch, especially as it would not be needed
> > to fix older kernels for the original bug.
> > 
> > So, care to send 2 patches?  The debug one isn't needed to be sent to
> > the stable@kernel.org address.
> 
> 
> Signed-off-by: Antti Kaijanmäki <antti.kaijanmaki@nomovok.com>
> ---
>  drivers/net/usb/hso.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
> index fa4e581..746839b 100644
> --- a/drivers/net/usb/hso.c
> +++ b/drivers/net/usb/hso.c
> @@ -378,7 +378,7 @@ static void dbg_dump(int line_count, const char *func_name, unsigned char *buf,
>  }
>  
>  #define DUMP(buf_, len_)	\
> -	dbg_dump(__LINE__, __func__, buf_, len_)
> +	dbg_dump(__LINE__, __func__, (unsigned char *)buf_, len_)
>  
>  #define DUMP1(buf_, len_)			\
>  	do {					\
> @@ -1527,7 +1527,7 @@ static void tiocmget_intr_callback(struct urb *urb)
>  		dev_warn(&usb->dev,
>  			 "hso received invalid serial state notification\n");
>  		DUMP(serial_state_notification,
> -		     sizeof(hso_serial_state_notifation))
> +		     sizeof(struct hso_serial_state_notification));
>  	} else {
>  
>  		UART_state_bitmap = le16_to_cpu(serial_state_notification->

This patch has no changelog, and I'm not seeing any description of what
it fixes and how it fixes it up-thread.

Re: [PATCH] hso: fix debug routines

[Antti Kaijanmäki]

[-- Attachment #1: Type: text/plain, Size: 457 bytes --]

On Wed, 2009-10-28 at 16:16 -0700, Andrew Morton wrote:
> This patch has no changelog, and I'm not seeing any description of what
> it fixes and how it fixes it up-thread.

The patch fixes debug routines of the hso driver. Debug has to be
enabled manually with a #define so this patch has 0 impact by default.

Broken down from original patch:
http://bugzilla.kernel.org/show_bug.cgi?id=14469
http://lkml.org/lkml/2009/10/22/37


 -- Antti



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH] hso: fix soft-lockup

[Antti Kaijanmäki]

On Mon, 2009-10-26 at 12:40 -0700, Greg KH wrote:
> Yes, that should be a new patch, especially as it would not be needed
> to fix older kernels for the original bug.
> 
> So, care to send 2 patches?  The debug one isn't needed to be sent to
> the stable@kernel.org address.


Fix soft-lockup in hso.c which is triggered on SMP machine when
modem is removed while file descriptor(s) under /dev are still open:

  old version called kref_put() too early which resulted in destroying
  hso_serial and hso_device objects which were still used later on.

Signed-off-by: Antti Kaijanmäki <antti.kaijanmaki@nomovok.com>
---
 drivers/net/usb/hso.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 746839b..43bc3fc 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -1363,7 +1363,7 @@ static void hso_serial_close(struct tty_struct *tty, struct file *filp)
 	/* reset the rts and dtr */
 	/* do the actual close */
 	serial->open_count--;
-	kref_put(&serial->parent->ref, hso_serial_ref_free);
+
 	if (serial->open_count <= 0) {
 		serial->open_count = 0;
 		spin_lock_irq(&serial->serial_lock);
@@ -1383,6 +1383,8 @@ static void hso_serial_close(struct tty_struct *tty, struct file *filp)
 		usb_autopm_put_interface(serial->parent->interface);
 
 	mutex_unlock(&serial->parent->mutex);
+
+	kref_put(&serial->parent->ref, hso_serial_ref_free);
 }
 
 /* close the requested serial port */
-- 
1.6.3.3