[PATCH] mfd: dln2: add suspend/resume functionality

[PATCH] mfd: dln2: add suspend/resume functionality

[Octavian Purdila]

Without suspend/resume functionality in the USB driver the USB core
will disconnect and reconnect the DLN2 port and because the GPIO
framework does not yet support removal of an in-use controller a
suspend/resume operation will result in a crash.

This patch provides suspend, resume and reset_resume functions for the
DLN2 driver so that the above scenario is avoided.

In the suspend routine we flush any pending trasactions and prevent
any new transactions from being started.

In the resume_reset routine we just reinitializing the RX URBS as
the hardware does not lose it's state when a USB reset is performed.

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 drivers/mfd/dln2.c | 41 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
index b9019e4..0f1697c 100644
--- a/drivers/mfd/dln2.c
+++ b/drivers/mfd/dln2.c
@@ -651,9 +651,8 @@ static const struct mfd_cell dln2_devs[] = {
 	},
 };
 
-static void dln2_disconnect(struct usb_interface *interface)
+static void dln2_stop(struct dln2_dev *dln2)
 {
-	struct dln2_dev *dln2 = usb_get_intfdata(interface);
 	int i, j;
 
 	/* don't allow starting new transfers */
@@ -681,6 +680,13 @@ static void dln2_disconnect(struct usb_interface *interface)
 
 	/* wait for transfers to end */
 	wait_event(dln2->disconnect_wq, !dln2->active_transfers);
+}
+
+static void dln2_disconnect(struct usb_interface *interface)
+{
+	struct dln2_dev *dln2 = usb_get_intfdata(interface);
+
+	dln2_stop(dln2);
 
 	mfd_remove_devices(&interface->dev);
 
@@ -753,11 +759,42 @@ static const struct usb_device_id dln2_table[] = {
 
 MODULE_DEVICE_TABLE(usb, dln2_table);
 
+static int dln2_suspend(struct usb_interface *iface, pm_message_t message)
+{
+	struct dln2_dev *dln2 = usb_get_intfdata(iface);
+
+	dln2_stop(dln2);
+	return 0;
+}
+
+static int dln2_resume(struct usb_interface *iface)
+{
+	struct dln2_dev *dln2 = usb_get_intfdata(iface);
+
+	dln2->disconnect = false;
+	return 0;
+}
+
+static int dln2_reset_resume(struct usb_interface *iface)
+{
+	struct dln2_dev *dln2 = usb_get_intfdata(iface);
+	int ret;
+
+	dln2_free_rx_urbs(dln2);
+	ret = dln2_setup_rx_urbs(dln2, iface->cur_altsetting);
+	dln2->disconnect = false;
+
+	return ret;
+}
+
 static struct usb_driver dln2_driver = {
 	.name = "dln2",
 	.probe = dln2_probe,
 	.disconnect = dln2_disconnect,
 	.id_table = dln2_table,
+	.suspend = dln2_suspend,
+	.resume = dln2_resume,
+	.reset_resume = dln2_reset_resume,
 };
 
 module_usb_driver(dln2_driver);
-- 
1.9.1

Re: [PATCH] mfd: dln2: add suspend/resume functionality

[Johan Hovold]

On Thu, Nov 27, 2014 at 01:57:14PM +0200, Octavian Purdila wrote:

> @@ -753,11 +759,42 @@ static const struct usb_device_id dln2_table[] = {
>  
>  MODULE_DEVICE_TABLE(usb, dln2_table);

Place the new callbacks above the device id table.

> +static int dln2_suspend(struct usb_interface *iface, pm_message_t message)
> +{
> +	struct dln2_dev *dln2 = usb_get_intfdata(iface);
> +
> +	dln2_stop(dln2);

You should also stop the reads urbs here.

> +	return 0;
> +}
> +
> +static int dln2_resume(struct usb_interface *iface)
> +{
> +	struct dln2_dev *dln2 = usb_get_intfdata(iface);
> +
> +	dln2->disconnect = false;

And surely you need to resubmit the read urbs in resume, or you will
never receive any more data.

How did you test this patch?

> +	return 0;
> +}
> +
> +static int dln2_reset_resume(struct usb_interface *iface)
> +{
> +	struct dln2_dev *dln2 = usb_get_intfdata(iface);
> +	int ret;
> +
> +	dln2_free_rx_urbs(dln2);
> +	ret = dln2_setup_rx_urbs(dln2, iface->cur_altsetting);

This doesn't make much sense. Why would you ever want to reallocate the
urbs and their buffers here?  

If the device does not lose its state as you claim, then all you need to
do is to resubmit the read urbs (as in resume).

Johan

Re: [PATCH] mfd: dln2: add suspend/resume functionality

[Octavian Purdila]

On Fri, Dec 5, 2014 at 12:17 PM, Johan Hovold <johan@kernel.org> wrote:
> On Thu, Nov 27, 2014 at 01:57:14PM +0200, Octavian Purdila wrote:
>
>> @@ -753,11 +759,42 @@ static const struct usb_device_id dln2_table[] = {
>>
>>  MODULE_DEVICE_TABLE(usb, dln2_table);
>
> Place the new callbacks above the device id table.
>
>> +static int dln2_suspend(struct usb_interface *iface, pm_message_t message)
>> +{
>> +     struct dln2_dev *dln2 = usb_get_intfdata(iface);
>> +
>> +     dln2_stop(dln2);
>
> You should also stop the reads urbs here.

Do you mean usb_kill_urb()? I thought that was not necessary unless
the device is reset. However, going throught
Documentation/usb/power-management.txt again looks like it must be
done in suspend.

>
>> +     return 0;
>> +}
>> +
>> +static int dln2_resume(struct usb_interface *iface)
>> +{
>> +     struct dln2_dev *dln2 = usb_get_intfdata(iface);
>> +
>> +     dln2->disconnect = false;
>
> And surely you need to resubmit the read urbs in resume, or you will
> never receive any more data.
>
> How did you test this patch?
>

The resume cb is not called in my setup (kvm), only reset_resume. But
I assume since the port is not reset when resume is called the URBs
are still queued.

>> +     return 0;
>> +}
>> +
>> +static int dln2_reset_resume(struct usb_interface *iface)
>> +{
>> +     struct dln2_dev *dln2 = usb_get_intfdata(iface);
>> +     int ret;
>> +
>> +     dln2_free_rx_urbs(dln2);
>> +     ret = dln2_setup_rx_urbs(dln2, iface->cur_altsetting);
>
> This doesn't make much sense. Why would you ever want to reallocate the
> urbs and their buffers here?
>
> If the device does not lose its state as you claim, then all you need to
> do is to resubmit the read urbs (as in resume).
>

The device itself does not lose state as it does not lose power and
does not react to USB port reset AFAICS (e.g. GPIO settings are
preserved). However the USB port is reset and I assumed I must
reallocate the URBs.

I just found out that usb-serial uses usb_poisoin_urb and
usb_unpoison_urb for suspend/resume and this two looks like just what
I need. Should I use that instead?

Re: [PATCH] mfd: dln2: add suspend/resume functionality

[Johan Hovold]

On Fri, Dec 05, 2014 at 01:51:17PM +0200, Octavian Purdila wrote:
> On Fri, Dec 5, 2014 at 12:17 PM, Johan Hovold <johan@kernel.org> wrote:
> > On Thu, Nov 27, 2014 at 01:57:14PM +0200, Octavian Purdila wrote:
> >
> >> @@ -753,11 +759,42 @@ static const struct usb_device_id dln2_table[] = {
> >>
> >>  MODULE_DEVICE_TABLE(usb, dln2_table);
> >
> > Place the new callbacks above the device id table.
> >
> >> +static int dln2_suspend(struct usb_interface *iface, pm_message_t message)
> >> +{
> >> +     struct dln2_dev *dln2 = usb_get_intfdata(iface);
> >> +
> >> +     dln2_stop(dln2);
> >
> > You should also stop the reads urbs here.
> 
> Do you mean usb_kill_urb()? I thought that was not necessary unless
> the device is reset. However, going throught
> Documentation/usb/power-management.txt again looks like it must be
> done in suspend.

Yes, you should kill them explicitly. Any outstanding urbs will be
killed by usb core if you fail to do this.

> >> +     return 0;
> >> +}
> >> +
> >> +static int dln2_resume(struct usb_interface *iface)
> >> +{
> >> +     struct dln2_dev *dln2 = usb_get_intfdata(iface);
> >> +
> >> +     dln2->disconnect = false;
> >
> > And surely you need to resubmit the read urbs in resume, or you will
> > never receive any more data.
> >
> > How did you test this patch?
> >
> 
> The resume cb is not called in my setup (kvm), only reset_resume. 

Please make sure to test your patches on proper hardware.

> But I assume since the port is not reset when resume is called the
> URBs are still queued.

No, they will have been killed by usb core even if you forget to do it,
so this would prevent any further reads.

> >> +     return 0;
> >> +}
> >> +
> >> +static int dln2_reset_resume(struct usb_interface *iface)
> >> +{
> >> +     struct dln2_dev *dln2 = usb_get_intfdata(iface);
> >> +     int ret;
> >> +
> >> +     dln2_free_rx_urbs(dln2);
> >> +     ret = dln2_setup_rx_urbs(dln2, iface->cur_altsetting);
> >
> > This doesn't make much sense. Why would you ever want to reallocate the
> > urbs and their buffers here?
> >
> > If the device does not lose its state as you claim, then all you need to
> > do is to resubmit the read urbs (as in resume).
> >
> The device itself does not lose state as it does not lose power and
> does not react to USB port reset AFAICS (e.g. GPIO settings are
> preserved). However the USB port is reset and I assumed I must
> reallocate the URBs.

You don't and should not.

> I just found out that usb-serial uses usb_poisoin_urb and
> usb_unpoison_urb for suspend/resume and this two looks like just what
> I need.

Why do you think so?

> Should I use that instead?

No.

Johan

Re: [PATCH] mfd: dln2: add suspend/resume functionality

[Octavian Purdila]

On Fri, Dec 5, 2014 at 2:06 PM, Johan Hovold <johan@kernel.org> wrote:
> On Fri, Dec 05, 2014 at 01:51:17PM +0200, Octavian Purdila wrote:
>> On Fri, Dec 5, 2014 at 12:17 PM, Johan Hovold <johan@kernel.org> wrote:
>> > On Thu, Nov 27, 2014 at 01:57:14PM +0200, Octavian Purdila wrote:
>> >
>> >> @@ -753,11 +759,42 @@ static const struct usb_device_id dln2_table[] = {
>> >>
>> >>  MODULE_DEVICE_TABLE(usb, dln2_table);
>> >
>> > Place the new callbacks above the device id table.
>> >
>> >> +static int dln2_suspend(struct usb_interface *iface, pm_message_t message)
>> >> +{
>> >> +     struct dln2_dev *dln2 = usb_get_intfdata(iface);
>> >> +
>> >> +     dln2_stop(dln2);
>> >
>> > You should also stop the reads urbs here.
>>
>> Do you mean usb_kill_urb()? I thought that was not necessary unless
>> the device is reset. However, going throught
>> Documentation/usb/power-management.txt again looks like it must be
>> done in suspend.
>
> Yes, you should kill them explicitly. Any outstanding urbs will be
> killed by usb core if you fail to do this.
>
>> >> +     return 0;
>> >> +}
>> >> +
>> >> +static int dln2_resume(struct usb_interface *iface)
>> >> +{
>> >> +     struct dln2_dev *dln2 = usb_get_intfdata(iface);
>> >> +
>> >> +     dln2->disconnect = false;
>> >
>> > And surely you need to resubmit the read urbs in resume, or you will
>> > never receive any more data.
>> >
>> > How did you test this patch?
>> >
>>
>> The resume cb is not called in my setup (kvm), only reset_resume.
>
> Please make sure to test your patches on proper hardware.

It was tested with the proper hardware, I was just testing it in the
setup we use, which is kvm with USB pass through.

>
>> But I assume since the port is not reset when resume is called the
>> URBs are still queued.
>
> No, they will have been killed by usb core even if you forget to do it,
> so this would prevent any further reads.
>
>> >> +     return 0;
>> >> +}
>> >> +
>> >> +static int dln2_reset_resume(struct usb_interface *iface)
>> >> +{
>> >> +     struct dln2_dev *dln2 = usb_get_intfdata(iface);
>> >> +     int ret;
>> >> +
>> >> +     dln2_free_rx_urbs(dln2);
>> >> +     ret = dln2_setup_rx_urbs(dln2, iface->cur_altsetting);
>> >
>> > This doesn't make much sense. Why would you ever want to reallocate the
>> > urbs and their buffers here?
>> >
>> > If the device does not lose its state as you claim, then all you need to
>> > do is to resubmit the read urbs (as in resume).
>> >
>> The device itself does not lose state as it does not lose power and
>> does not react to USB port reset AFAICS (e.g. GPIO settings are
>> preserved). However the USB port is reset and I assumed I must
>> reallocate the URBs.
>
> You don't and should not.
>
>> I just found out that usb-serial uses usb_poisoin_urb and
>> usb_unpoison_urb for suspend/resume and this two looks like just what
>> I need.
>
> Why do you think so?
>

Hmm, never mind, now I understand that usb_poisoin_urb is just used
defensive in usb-serial to prevent the drivers to submit the URB until
resume time.

Thanks for the review Johan, I will send v2 with usb_kill_urb in
suspend() and usb_submit_urb() in resume and resume_reset.

Re: [PATCH] mfd: dln2: add suspend/resume functionality

[Johan Hovold]

On Fri, Dec 05, 2014 at 03:05:45PM +0200, Octavian Purdila wrote:
> On Fri, Dec 5, 2014 at 2:06 PM, Johan Hovold <johan@kernel.org> wrote:
> > On Fri, Dec 05, 2014 at 01:51:17PM +0200, Octavian Purdila wrote:
> >> On Fri, Dec 5, 2014 at 12:17 PM, Johan Hovold <johan@kernel.org> wrote:

> >> > And surely you need to resubmit the read urbs in resume, or you will
> >> > never receive any more data.
> >> >
> >> > How did you test this patch?
> >> >
> >>
> >> The resume cb is not called in my setup (kvm), only reset_resume.
> >
> > Please make sure to test your patches on proper hardware.
> 
> It was tested with the proper hardware, I was just testing it in the
> setup we use, which is kvm with USB pass through.

I was referring to your kvm setup.

Johan

Re: [PATCH] mfd: dln2: add suspend/resume functionality

[Octavian Purdila]

On Mon, Dec 8, 2014 at 12:57 PM, Johan Hovold <johan@kernel.org> wrote:
> On Fri, Dec 05, 2014 at 03:05:45PM +0200, Octavian Purdila wrote:
>> On Fri, Dec 5, 2014 at 2:06 PM, Johan Hovold <johan@kernel.org> wrote:
>> > On Fri, Dec 05, 2014 at 01:51:17PM +0200, Octavian Purdila wrote:
>> >> On Fri, Dec 5, 2014 at 12:17 PM, Johan Hovold <johan@kernel.org> wrote:
>
>> >> > And surely you need to resubmit the read urbs in resume, or you will
>> >> > never receive any more data.
>> >> >
>> >> > How did you test this patch?
>> >> >
>> >>
>> >> The resume cb is not called in my setup (kvm), only reset_resume.
>> >
>> > Please make sure to test your patches on proper hardware.
>>
>> It was tested with the proper hardware, I was just testing it in the
>> setup we use, which is kvm with USB pass through.
>
> I was referring to your kvm setup.
>

I see the same behavior (reset_resume called) when testing on bare
bone hardware. BTW, KVM allows me to test more thoroughly, as I can
use custom SSDT tables required by i2c devices to find their IRQs.