"usb_wwan: error case of resume" (16871dcac) is buggy

"usb_wwan: error case of resume" (16871dcac) is buggy

[Jiri Kosina]

Hi,

the commit in subject make the kernel with CONFIG_PM_RUNTIME unset fail 
during compilation, as struct dev_pm_info doesn't have whole bunch of 
members in such case.

The commit in question adds this code:

	/* we have to throw away the rest */
	do {
		unbusy_queued_urb(urb, portdata);
		//extremely dirty
		atomic_dec(&port->serial->interface->dev.power.usage_count);
	} while ((urb = usb_get_from_anchor(&portdata->delayed)));

The 'extermely dirty' comment makes me a bit nervous whether the patch 
below is correct or some more thinking would be necessary.



From: Jiri Kosina <jkosina@suse.cz>
Subject: [PATCH] USB: usb_wvan: fix compilation for !CONFIG_PM_RUNTIME case

With CONFIG_PM_RUNTIME unset, struct dev_pm_info doesn't have
usage count which is used only for run-time power management
purposes.

Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 drivers/usb/serial/usb_wwan.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/serial/usb_wwan.c b/drivers/usb/serial/usb_wwan.c
index a65ddd5..8b456dc 100644
--- a/drivers/usb/serial/usb_wwan.c
+++ b/drivers/usb/serial/usb_wwan.c
@@ -698,8 +698,10 @@ static void play_delayed(struct usb_serial_port *port)
 			/* we have to throw away the rest */
 			do {
 				unbusy_queued_urb(urb, portdata);
-				//extremely dirty
+#ifdef CONFIG_PM_RUNTIME
+				/* extremely dirty */
 				atomic_dec(&port->serial->interface->dev.power.usage_count);
+#endif
 			} while ((urb = usb_get_from_anchor(&portdata->delayed)));
 			break;
 		}
-- 
1.7.3.1

Re: "usb_wwan: error case of resume" (16871dcac) is buggy

[Jiri Kosina]

[ fixing Oliver's address in CC ]

On Mon, 21 Mar 2011, Jiri Kosina wrote:

> Hi,
> 
> the commit in subject make the kernel with CONFIG_PM_RUNTIME unset fail 
> during compilation, as struct dev_pm_info doesn't have whole bunch of 
> members in such case.
> 
> The commit in question adds this code:
> 
> 	/* we have to throw away the rest */
> 	do {
> 		unbusy_queued_urb(urb, portdata);
> 		//extremely dirty
> 		atomic_dec(&port->serial->interface->dev.power.usage_count);
> 	} while ((urb = usb_get_from_anchor(&portdata->delayed)));
> 
> The 'extermely dirty' comment makes me a bit nervous whether the patch 
> below is correct or some more thinking would be necessary.
> 
> 
> 
> From: Jiri Kosina <jkosina@suse.cz>
> Subject: [PATCH] USB: usb_wvan: fix compilation for !CONFIG_PM_RUNTIME case
> 
> With CONFIG_PM_RUNTIME unset, struct dev_pm_info doesn't have
> usage count which is used only for run-time power management
> purposes.
> 
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> ---
>  drivers/usb/serial/usb_wwan.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/usb/serial/usb_wwan.c b/drivers/usb/serial/usb_wwan.c
> index a65ddd5..8b456dc 100644
> --- a/drivers/usb/serial/usb_wwan.c
> +++ b/drivers/usb/serial/usb_wwan.c
> @@ -698,8 +698,10 @@ static void play_delayed(struct usb_serial_port *port)
>  			/* we have to throw away the rest */
>  			do {
>  				unbusy_queued_urb(urb, portdata);
> -				//extremely dirty
> +#ifdef CONFIG_PM_RUNTIME
> +				/* extremely dirty */
>  				atomic_dec(&port->serial->interface->dev.power.usage_count);
> +#endif
>  			} while ((urb = usb_get_from_anchor(&portdata->delayed)));
>  			break;
>  		}
> -- 
> 1.7.3.1
> 
> 

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: "usb_wwan: error case of resume" (16871dcac) is buggy

[Randy Dunlap]

On Mon, 21 Mar 2011 01:41:14 +0100 (CET) Jiri Kosina wrote:

> 
> [ fixing Oliver's address in CC ]
> 
> On Mon, 21 Mar 2011, Jiri Kosina wrote:
> 
> > Hi,
> > 
> > the commit in subject make the kernel with CONFIG_PM_RUNTIME unset fail 
> > during compilation, as struct dev_pm_info doesn't have whole bunch of 
> > members in such case.
> > 
> > The commit in question adds this code:
> > 
> > 	/* we have to throw away the rest */
> > 	do {
> > 		unbusy_queued_urb(urb, portdata);
> > 		//extremely dirty
> > 		atomic_dec(&port->serial->interface->dev.power.usage_count);
> > 	} while ((urb = usb_get_from_anchor(&portdata->delayed)));
> > 
> > The 'extermely dirty' comment makes me a bit nervous whether the patch 
> > below is correct or some more thinking would be necessary.
> > 
> > 
> > 
> > From: Jiri Kosina <jkosina@suse.cz>
> > Subject: [PATCH] USB: usb_wvan: fix compilation for !CONFIG_PM_RUNTIME case
> > 
> > With CONFIG_PM_RUNTIME unset, struct dev_pm_info doesn't have
> > usage count which is used only for run-time power management
> > purposes.
> > 
> > Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> > ---
> >  drivers/usb/serial/usb_wwan.c |    4 +++-
> >  1 files changed, 3 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/usb/serial/usb_wwan.c b/drivers/usb/serial/usb_wwan.c
> > index a65ddd5..8b456dc 100644
> > --- a/drivers/usb/serial/usb_wwan.c
> > +++ b/drivers/usb/serial/usb_wwan.c
> > @@ -698,8 +698,10 @@ static void play_delayed(struct usb_serial_port *port)
> >  			/* we have to throw away the rest */
> >  			do {
> >  				unbusy_queued_urb(urb, portdata);
> > -				//extremely dirty
> > +#ifdef CONFIG_PM_RUNTIME
> > +				/* extremely dirty */
> >  				atomic_dec(&port->serial->interface->dev.power.usage_count);
> > +#endif
> >  			} while ((urb = usb_get_from_anchor(&portdata->delayed)));
> >  			break;
> >  		}
> > -- 
> > 1.7.3.1
> > 
> > 

Please see Oliver's patch for this at
http://marc.info/?l=linux-usb&m=130044862323463&w=2

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: "usb_wwan: error case of resume" (16871dcac) is buggy

[Oliver Neukum]

Am Montag, 21. März 2011, 01:36:45 schrieb Jiri Kosina:
> Hi,
> 
> the commit in subject make the kernel with CONFIG_PM_RUNTIME unset fail 
> during compilation, as struct dev_pm_info doesn't have whole bunch of 
> members in such case.
> 
> The commit in question adds this code:
> 
> 	/* we have to throw away the rest */
> 	do {
> 		unbusy_queued_urb(urb, portdata);
> 		//extremely dirty
> 		atomic_dec(&port->serial->interface->dev.power.usage_count);
> 	} while ((urb = usb_get_from_anchor(&portdata->delayed)));
> 
> The 'extermely dirty' comment makes me a bit nervous whether the patch 
> below is correct or some more thinking would be necessary.

I've since posted a clean patch, although your fix looks correct to me.
The problem was that I was unsure about the locking during resume().
I rather wrote a layering violation than risk a deadlock in an error path
hard to test. But I did not feel well doing it.

	Regards
		Oliver

Re: "usb_wwan: error case of resume" (16871dcac) is buggy

[Alan Stern]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 1216 bytes --]

On Mon, 21 Mar 2011, Oliver Neukum wrote:

> Am Montag, 21. März 2011, 01:36:45 schrieb Jiri Kosina:
> > Hi,
> > 
> > the commit in subject make the kernel with CONFIG_PM_RUNTIME unset fail 
> > during compilation, as struct dev_pm_info doesn't have whole bunch of 
> > members in such case.
> > 
> > The commit in question adds this code:
> > 
> > 	/* we have to throw away the rest */
> > 	do {
> > 		unbusy_queued_urb(urb, portdata);
> > 		//extremely dirty
> > 		atomic_dec(&port->serial->interface->dev.power.usage_count);
> > 	} while ((urb = usb_get_from_anchor(&portdata->delayed)));
> > 
> > The 'extermely dirty' comment makes me a bit nervous whether the patch 
> > below is correct or some more thinking would be necessary.
> 
> I've since posted a clean patch, although your fix looks correct to me.

In fact the original code and Jiri's fix are both wrong.  The layering 
violation here is not only inelegant, it is downright buggy.

The USB core contains its own usage_count for interfaces, in parallel 
with the usage_count in interface->dev.power.  Bypassing the USB layer 
in this way causes the two counts to get out of sync, which will lead 
to errors.

Oliver's new fix is correct.

Alan Stern