Re: [linux-usb-devel] [PATCH 1/2] usb_gigaset: suspend support

Re: [linux-usb-devel] [PATCH 1/2] usb_gigaset: suspend support

[Alan Stern]

On Mon, 29 Oct 2007, Tilman Schmidt wrote:

> From: Tilman Schmidt <tilman@imap.cc>
> 
> Add basic suspend/resume support to the usb_gigaset driver.

> @@ -117,6 +122,11 @@ static struct usb_driver gigaset_usb_dri
>  	.probe =	gigaset_probe,
>  	.disconnect =	gigaset_disconnect,
>  	.id_table =	gigaset_table,
> +	.suspend =	gigaset_suspend,
> +	.resume =	gigaset_resume,
> +	.reset_resume =	gigaset_resume,
> +	.pre_reset =	gigaset_suspend,
> +	.post_reset =	gigaset_resume,
>  };

Does this really compile?  The .pre_reset and .suspend members are 
pointers to functions with different prototypes; I don't see how you 
can assign the same function to both pointers.

Alan Stern

Re: [linux-usb-devel] [PATCH 1/2] usb_gigaset: suspend support

[Tilman Schmidt]

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

Am 29.10.2007 20:10 schrieb Alan Stern:
> On Mon, 29 Oct 2007, Tilman Schmidt wrote:
> 
>> From: Tilman Schmidt <tilman@imap.cc>
>>
>> Add basic suspend/resume support to the usb_gigaset driver.
> 
>> @@ -117,6 +122,11 @@ static struct usb_driver gigaset_usb_dri
>>  	.probe =	gigaset_probe,
>>  	.disconnect =	gigaset_disconnect,
>>  	.id_table =	gigaset_table,
>> +	.suspend =	gigaset_suspend,
>> +	.resume =	gigaset_resume,
>> +	.reset_resume =	gigaset_resume,
>> +	.pre_reset =	gigaset_suspend,
>> +	.post_reset =	gigaset_resume,
>>  };
> 
> Does this really compile?  The .pre_reset and .suspend members are 
> pointers to functions with different prototypes; I don't see how you 
> can assign the same function to both pointers.

Oops, you're right. Yes, it does compile, but with a warning
"initialization from incompatible pointer type" which I
foolishly ignored.

I'll produce a corrected version promptly.

Thanks,
Tilman

-- 
Tilman Schmidt                          E-Mail: tilman@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 253 bytes --]

Re: [linux-usb-devel] [PATCH 1/2] usb_gigaset: suspend support

[Oliver Neukum]

Am Montag 29 Oktober 2007 schrieb Tilman Schmidt:
> From: Tilman Schmidt <tilman@imap.cc>
> 
> Add basic suspend/resume support to the usb_gigaset driver.
> 
> Signed-off-by: Tilman Schmidt <tilman@imap.cc>
> ---
> 
>  drivers/isdn/gigaset/usb-gigaset.c |   69 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 68 insertions(+), 1 deletion(-)
> 
> --- a/drivers/isdn/gigaset/usb-gigaset.c	2007-10-23 01:48:12.000000000 +0200
> +++ b/drivers/isdn/gigaset/usb-gigaset.c	2007-10-24 00:47:32.000000000 +0200
> @@ -104,10 +104,15 @@ MODULE_DEVICE_TABLE(usb, gigaset_table);
>   * flags per packet.
>   */
>  
> +/* functions called if a device of this driver is connected/disconnected */
>  static int gigaset_probe(struct usb_interface *interface,
>  			 const struct usb_device_id *id);
>  static void gigaset_disconnect(struct usb_interface *interface);
>  
> +/* functions called before/after suspend */
> +static int gigaset_suspend(struct usb_interface *intf, pm_message_t message);
> +static int gigaset_resume(struct usb_interface *intf);
> +
>  static struct gigaset_driver *driver = NULL;
>  static struct cardstate *cardstate = NULL;
>  
> @@ -117,6 +122,11 @@ static struct usb_driver gigaset_usb_dri
>  	.probe =	gigaset_probe,
>  	.disconnect =	gigaset_disconnect,
>  	.id_table =	gigaset_table,
> +	.suspend =	gigaset_suspend,
> +	.resume =	gigaset_resume,
> +	.reset_resume =	gigaset_resume,
> +	.pre_reset =	gigaset_suspend,
> +	.post_reset =	gigaset_resume,
>  };
>  
>  struct usb_cardstate {
> @@ -831,7 +843,7 @@ static void gigaset_disconnect(struct us
>  	usb_set_intfdata(interface, NULL);
>  	tasklet_kill(&cs->write_tasklet);
>  
> -	usb_kill_urb(ucs->bulk_out_urb);	/* FIXME: only if active? */
> +	usb_kill_urb(ucs->bulk_out_urb);
>  
>  	kfree(ucs->bulk_out_buffer);
>  	usb_free_urb(ucs->bulk_out_urb);
> @@ -847,6 +859,63 @@ static void gigaset_disconnect(struct us
>  	gigaset_unassign(cs);
>  }
>  
> +/* gigaset_suspend
> + * This function is called before the USB connection is suspended or reset.
> + */
> +static int gigaset_suspend(struct usb_interface *intf, pm_message_t message)
> +{
> +	struct cardstate *cs;
> +	struct usb_cardstate *ucs;
> +	int rc;
> +
> +	if ((cs = usb_get_intfdata(intf)) == NULL ||
> +	    (ucs = cs->hw.usb) == NULL) {
> +		err("%s: no cardstate", __func__);
> +		return -EFAULT;
> +	}
> +
> +	//FIXME stop common module activities? ISDN_STAT_STOP? block open()?
> +
> +	/* stop submitting bulk URBs */
> +	tasklet_disable(&cs->write_tasklet);
> +
> +	/* kill pending read URB */
> +	usb_kill_urb(ucs->read_urb);

no pending write URB?

	Regards
		Oliver

Re: [linux-usb-devel] [PATCH 1/2] usb_gigaset: suspend support

[Tilman Schmidt]

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

Am 30.10.2007 09:52 schrieb Oliver Neukum:
> Am Montag 29 Oktober 2007 schrieb Tilman Schmidt:

>> --- a/drivers/isdn/gigaset/usb-gigaset.c	2007-10-23 01:48:12.000000000 +0200
>> +++ b/drivers/isdn/gigaset/usb-gigaset.c	2007-10-24 00:47:32.000000000 +0200
>> @@ -847,6 +859,63 @@ static void gigaset_disconnect(struct us
>>  	gigaset_unassign(cs);
>>  }
>>  
>> +/* gigaset_suspend
>> + * This function is called before the USB connection is suspended or reset.
>> + */
>> +static int gigaset_suspend(struct usb_interface *intf, pm_message_t message)
>> +{
>> +	struct cardstate *cs;
>> +	struct usb_cardstate *ucs;
>> +	int rc;
>> +
>> +	if ((cs = usb_get_intfdata(intf)) == NULL ||
>> +	    (ucs = cs->hw.usb) == NULL) {
>> +		err("%s: no cardstate", __func__);
>> +		return -EFAULT;
>> +	}
>> +
>> +	//FIXME stop common module activities? ISDN_STAT_STOP? block open()?
>> +
>> +	/* stop submitting bulk URBs */
>> +	tasklet_disable(&cs->write_tasklet);
>> +
>> +	/* kill pending read URB */
>> +	usb_kill_urb(ucs->read_urb);
> 
> no pending write URB?

There shouldn't be. A read URB is always posted in case the device
sends anything, even while the device is idle, but a write URB is
only submitted if there's actual activity. But you're right, it's
safer to add a "usb_kill_urb(ucs->bulk_out_urb);" here just in case.

Thanks,
Tilman

-- 
Tilman Schmidt                          E-Mail: tilman@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 253 bytes --]

Re: [PATCH 1/2] usb_gigaset: suspend support [v2]

[Andrew Morton]

On Mon, 29 Oct 2007 22:41:30 +0100 (CET)
Tilman Schmidt <tilman@imap.cc> wrote:

> From: Tilman Schmidt <tilman@imap.cc>
> 
> Add basic suspend/resume support to the usb_gigaset driver.
> (Corrected version.)
> 

You're not a big fan of checkpatch, I see.

> +static int gigaset_suspend(struct usb_interface *intf, pm_message_t message)
> +{
> +	struct cardstate *cs;
> +	struct usb_cardstate *ucs;
> +
> +	if ((cs = usb_get_intfdata(intf)) == NULL ||
> +	    (ucs = cs->hw.usb) == NULL) {
> +		err("%s: no cardstate", __func__);
> +		return -EFAULT;
> +	}

Is the above reeeeeely needed?  I bet it never happens.

> +static int gigaset_resume(struct usb_interface *intf)
> +{
> +	struct cardstate *cs;
> +	struct usb_cardstate *ucs;
> +	int rc;
> +
> +	if ((cs = usb_get_intfdata(intf)) == NULL ||
> +	    (ucs = cs->hw.usb) == NULL) {
> +		err("%s: no cardstate", __func__);
> +		return -EFAULT;
> +	}

ditto.

Re: [PATCH 1/2] usb_gigaset: suspend support [v2]

[Tilman Schmidt]

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

Am 30.10.2007 22:01 schrieb Andrew Morton:
> On Mon, 29 Oct 2007 22:41:30 +0100 (CET)
> Tilman Schmidt <tilman@imap.cc> wrote:
> 
>> From: Tilman Schmidt <tilman@imap.cc>
>>
>> Add basic suspend/resume support to the usb_gigaset driver.
>> (Corrected version.)
>>
> 
> You're not a big fan of checkpatch, I see.

Sorry. I promise to mend my ways.

>> +static int gigaset_suspend(struct usb_interface *intf, pm_message_t message)
>> +{
>> +	struct cardstate *cs;
>> +	struct usb_cardstate *ucs;
>> +
>> +	if ((cs = usb_get_intfdata(intf)) == NULL ||
>> +	    (ucs = cs->hw.usb) == NULL) {
>> +		err("%s: no cardstate", __func__);
>> +		return -EFAULT;
>> +	}
> 
> Is the above reeeeeely needed?  I bet it never happens.

I'm a great believer in defensive programming. :-)

Anyway, to be sure these checks aren't needed, I would need the
assurance that the suspend and resume methods are serialized with
the probe and disconnect methods. Are they?

Thanks,
Tilman

-- 
Tilman Schmidt                          E-Mail: tilman@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 253 bytes --]

Re: [PATCH 1/2] usb_gigaset: suspend support [v2]

[Andrew Morton]

On Thu, 01 Nov 2007 19:20:07 +0100
Tilman Schmidt <tilman@imap.cc> wrote:

> >> +static int gigaset_suspend(struct usb_interface *intf, pm_message_t message)
> >> +{
> >> +	struct cardstate *cs;
> >> +	struct usb_cardstate *ucs;
> >> +
> >> +	if ((cs = usb_get_intfdata(intf)) == NULL ||
> >> +	    (ucs = cs->hw.usb) == NULL) {
> >> +		err("%s: no cardstate", __func__);
> >> +		return -EFAULT;
> >> +	}
> > 
> > Is the above reeeeeely needed?  I bet it never happens.
> 
> I'm a great believer in defensive programming. :-)
> 
> Anyway, to be sure these checks aren't needed, I would need the
> assurance that the suspend and resume methods are serialized with
> the probe and disconnect methods. Are they?

dunno it beats me.  Not to my knowledge.  Perhaps Greg and Rafael would know?

Re: [linux-usb-devel] [PATCH 1/2] usb_gigaset: suspend support [v2]

[Alan Stern]

On Thu, 1 Nov 2007, Tilman Schmidt wrote:

> Anyway, to be sure these checks aren't needed, I would need the
> assurance that the suspend and resume methods are serialized with
> the probe and disconnect methods. Are they?

Yes.  See Documentation/usb/power-management.txt.

Alan Stern