[PATCH v3 0/2] DLN2 fixes related to suspend/resume

[PATCH v3 0/2] DLN2 fixes related to suspend/resume

[Octavian Purdila]

Hi Lee,

Here is the 3rd version of the patch-set against for-mfd-next branch
with the Reviewed-by tags from Johan. Note that I dropped the two GPIO
patches as they were already merged by Linus W in the GPIO tree.

Thanks,
Tavi

Changes since v2:

 * Dropped the GPIO fixes from this series as they were merged in the
   GPIO tree

 * Removed a stray newline

 * Updated the resume / suspend patch commit with more information
   about which cases it fixes and which it does not

Changes since v1:

 * Fix an issue introduced by v1 where we can get a use after free in
   the error path of probe

 * Add start/stop RX URBs helpers

 * move the suspend/resume routines above the module device table

 * used GFP_NOIO in resume

Octavian Purdila (2):
  mfd: dln2: add start/stop RX URBs helpers
  mfd: dln2: add suspend/resume functionality

 drivers/mfd/dln2.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 61 insertions(+), 10 deletions(-)

-- 
1.9.1

[PATCH v3 1/2] mfd: dln2: add start/stop RX URBs helpers

[Octavian Purdila]

This is in preparation for adding suspend / resume support.

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
Reviewed-by: Johan Hovold <johan@kernel.org>
---
 drivers/mfd/dln2.c | 51 +++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 41 insertions(+), 10 deletions(-)

diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
index 6d49685..8311820 100644
--- a/drivers/mfd/dln2.c
+++ b/drivers/mfd/dln2.c
@@ -587,12 +587,19 @@ static void dln2_free_rx_urbs(struct dln2_dev *dln2)
 	int i;
 
 	for (i = 0; i < DLN2_MAX_URBS; i++) {
-		usb_kill_urb(dln2->rx_urb[i]);
 		usb_free_urb(dln2->rx_urb[i]);
 		kfree(dln2->rx_buf[i]);
 	}
 }
 
+static void dln2_stop_rx_urbs(struct dln2_dev *dln2)
+{
+	int i;
+
+	for (i = 0; i < DLN2_MAX_URBS; i++)
+		usb_kill_urb(dln2->rx_urb[i]);
+}
+
 static void dln2_free(struct dln2_dev *dln2)
 {
 	dln2_free_rx_urbs(dln2);
@@ -604,9 +611,7 @@ static int dln2_setup_rx_urbs(struct dln2_dev *dln2,
 			      struct usb_host_interface *hostif)
 {
 	int i;
-	int ret;
 	const int rx_max_size = DLN2_RX_BUF_SIZE;
-	struct device *dev = &dln2->interface->dev;
 
 	for (i = 0; i < DLN2_MAX_URBS; i++) {
 		dln2->rx_buf[i] = kmalloc(rx_max_size, GFP_KERNEL);
@@ -620,8 +625,19 @@ static int dln2_setup_rx_urbs(struct dln2_dev *dln2,
 		usb_fill_bulk_urb(dln2->rx_urb[i], dln2->usb_dev,
 				  usb_rcvbulkpipe(dln2->usb_dev, dln2->ep_in),
 				  dln2->rx_buf[i], rx_max_size, dln2_rx, dln2);
+	}
+
+	return 0;
+}
 
-		ret = usb_submit_urb(dln2->rx_urb[i], GFP_KERNEL);
+static int dln2_start_rx_urbs(struct dln2_dev *dln2, gfp_t gfp)
+{
+	struct device *dev = &dln2->interface->dev;
+	int ret;
+	int i;
+
+	for (i = 0; i < DLN2_MAX_URBS; i++) {
+		ret = usb_submit_urb(dln2->rx_urb[i], gfp);
 		if (ret < 0) {
 			dev_err(dev, "failed to submit RX URB: %d\n", ret);
 			return ret;
@@ -665,9 +681,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 */
@@ -696,6 +711,15 @@ static void dln2_disconnect(struct usb_interface *interface)
 	/* wait for transfers to end */
 	wait_event(dln2->disconnect_wq, !dln2->active_transfers);
 
+	dln2_stop_rx_urbs(dln2);
+}
+
+static void dln2_disconnect(struct usb_interface *interface)
+{
+	struct dln2_dev *dln2 = usb_get_intfdata(interface);
+
+	dln2_stop(dln2);
+
 	mfd_remove_devices(&interface->dev);
 
 	dln2_free(dln2);
@@ -738,23 +762,30 @@ static int dln2_probe(struct usb_interface *interface,
 
 	ret = dln2_setup_rx_urbs(dln2, hostif);
 	if (ret)
-		goto out_cleanup;
+		goto out_free;
+
+	ret = dln2_start_rx_urbs(dln2, GFP_KERNEL);
+	if (ret)
+		goto out_stop_rx;
 
 	ret = dln2_hw_init(dln2);
 	if (ret < 0) {
 		dev_err(dev, "failed to initialize hardware\n");
-		goto out_cleanup;
+		goto out_stop_rx;
 	}
 
 	ret = mfd_add_hotplug_devices(dev, dln2_devs, ARRAY_SIZE(dln2_devs));
 	if (ret != 0) {
 		dev_err(dev, "failed to add mfd devices to core\n");
-		goto out_cleanup;
+		goto out_stop_rx;
 	}
 
 	return 0;
 
-out_cleanup:
+out_stop_rx:
+	dln2_stop_rx_urbs(dln2);
+
+out_free:
 	dln2_free(dln2);
 
 	return ret;
-- 
1.9.1

Re: [PATCH v3 1/2] mfd: dln2: add start/stop RX URBs helpers

[Lee Jones]

On Mon, 19 Jan 2015, Octavian Purdila wrote:

> This is in preparation for adding suspend / resume support.
> 
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> Reviewed-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/mfd/dln2.c | 51 +++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 41 insertions(+), 10 deletions(-)

With Johan's Ack, I'm fairly sure I can just apply this without an in
depth review from me.

After a quick glance; applied, thanks.

> diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
> index 6d49685..8311820 100644
> --- a/drivers/mfd/dln2.c
> +++ b/drivers/mfd/dln2.c
> @@ -587,12 +587,19 @@ static void dln2_free_rx_urbs(struct dln2_dev *dln2)
>  	int i;
>  
>  	for (i = 0; i < DLN2_MAX_URBS; i++) {
> -		usb_kill_urb(dln2->rx_urb[i]);
>  		usb_free_urb(dln2->rx_urb[i]);
>  		kfree(dln2->rx_buf[i]);
>  	}
>  }
>  
> +static void dln2_stop_rx_urbs(struct dln2_dev *dln2)
> +{
> +	int i;
> +
> +	for (i = 0; i < DLN2_MAX_URBS; i++)
> +		usb_kill_urb(dln2->rx_urb[i]);
> +}
> +
>  static void dln2_free(struct dln2_dev *dln2)
>  {
>  	dln2_free_rx_urbs(dln2);
> @@ -604,9 +611,7 @@ static int dln2_setup_rx_urbs(struct dln2_dev *dln2,
>  			      struct usb_host_interface *hostif)
>  {
>  	int i;
> -	int ret;
>  	const int rx_max_size = DLN2_RX_BUF_SIZE;
> -	struct device *dev = &dln2->interface->dev;
>  
>  	for (i = 0; i < DLN2_MAX_URBS; i++) {
>  		dln2->rx_buf[i] = kmalloc(rx_max_size, GFP_KERNEL);
> @@ -620,8 +625,19 @@ static int dln2_setup_rx_urbs(struct dln2_dev *dln2,
>  		usb_fill_bulk_urb(dln2->rx_urb[i], dln2->usb_dev,
>  				  usb_rcvbulkpipe(dln2->usb_dev, dln2->ep_in),
>  				  dln2->rx_buf[i], rx_max_size, dln2_rx, dln2);
> +	}
> +
> +	return 0;
> +}
>  
> -		ret = usb_submit_urb(dln2->rx_urb[i], GFP_KERNEL);
> +static int dln2_start_rx_urbs(struct dln2_dev *dln2, gfp_t gfp)
> +{
> +	struct device *dev = &dln2->interface->dev;
> +	int ret;
> +	int i;
> +
> +	for (i = 0; i < DLN2_MAX_URBS; i++) {
> +		ret = usb_submit_urb(dln2->rx_urb[i], gfp);
>  		if (ret < 0) {
>  			dev_err(dev, "failed to submit RX URB: %d\n", ret);
>  			return ret;
> @@ -665,9 +681,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 */
> @@ -696,6 +711,15 @@ static void dln2_disconnect(struct usb_interface *interface)
>  	/* wait for transfers to end */
>  	wait_event(dln2->disconnect_wq, !dln2->active_transfers);
>  
> +	dln2_stop_rx_urbs(dln2);
> +}
> +
> +static void dln2_disconnect(struct usb_interface *interface)
> +{
> +	struct dln2_dev *dln2 = usb_get_intfdata(interface);
> +
> +	dln2_stop(dln2);
> +
>  	mfd_remove_devices(&interface->dev);
>  
>  	dln2_free(dln2);
> @@ -738,23 +762,30 @@ static int dln2_probe(struct usb_interface *interface,
>  
>  	ret = dln2_setup_rx_urbs(dln2, hostif);
>  	if (ret)
> -		goto out_cleanup;
> +		goto out_free;
> +
> +	ret = dln2_start_rx_urbs(dln2, GFP_KERNEL);
> +	if (ret)
> +		goto out_stop_rx;
>  
>  	ret = dln2_hw_init(dln2);
>  	if (ret < 0) {
>  		dev_err(dev, "failed to initialize hardware\n");
> -		goto out_cleanup;
> +		goto out_stop_rx;
>  	}
>  
>  	ret = mfd_add_hotplug_devices(dev, dln2_devs, ARRAY_SIZE(dln2_devs));
>  	if (ret != 0) {
>  		dev_err(dev, "failed to add mfd devices to core\n");
> -		goto out_cleanup;
> +		goto out_stop_rx;
>  	}
>  
>  	return 0;
>  
> -out_cleanup:
> +out_stop_rx:
> +	dln2_stop_rx_urbs(dln2);
> +
> +out_free:
>  	dln2_free(dln2);
>  
>  	return ret;

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v3 2/2] 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 and resume functions for the DLN2 driver
so that the above scenario is avoided, if the host controller does not
drop VBUS during suspend, since in this case the device state is
preserved.

We chose not implemented reset_resume so that if the host controller
does drop VBUS the resume path will go through above the
disconnect/reconnect process since it is probably better to fix the
GPIO framework disconnect issue then to save and restore the device
state for every driver.

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
Reviewed-by: Johan Hovold <johan@kernel.org>
---
 drivers/mfd/dln2.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
index 8311820..1be9bd1 100644
--- a/drivers/mfd/dln2.c
+++ b/drivers/mfd/dln2.c
@@ -791,6 +791,24 @@ out_free:
 	return ret;
 }
 
+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 dln2_start_rx_urbs(dln2, GFP_NOIO);
+}
+
 static const struct usb_device_id dln2_table[] = {
 	{ USB_DEVICE(0xa257, 0x2013) },
 	{ }
@@ -803,6 +821,8 @@ static struct usb_driver dln2_driver = {
 	.probe = dln2_probe,
 	.disconnect = dln2_disconnect,
 	.id_table = dln2_table,
+	.suspend = dln2_suspend,
+	.resume = dln2_resume,
 };
 
 module_usb_driver(dln2_driver);
-- 
1.9.1

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

[Lee Jones]

On Mon, 19 Jan 2015, Octavian Purdila wrote:

> 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 and resume functions for the DLN2 driver
> so that the above scenario is avoided, if the host controller does not
> drop VBUS during suspend, since in this case the device state is
> preserved.
> 
> We chose not implemented reset_resume so that if the host controller
> does drop VBUS the resume path will go through above the
> disconnect/reconnect process since it is probably better to fix the
> GPIO framework disconnect issue then to save and restore the device
> state for every driver.
> 
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> Reviewed-by: Johan Hovold <johan@kernel.org>

With Johan's Ack, I'm fairly sure I can just apply this without an in
depth review from me.

After a quick glance; applied, thanks.

> ---
>  drivers/mfd/dln2.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
> index 8311820..1be9bd1 100644
> --- a/drivers/mfd/dln2.c
> +++ b/drivers/mfd/dln2.c
> @@ -791,6 +791,24 @@ out_free:
>  	return ret;
>  }
>  
> +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 dln2_start_rx_urbs(dln2, GFP_NOIO);
> +}
> +
>  static const struct usb_device_id dln2_table[] = {
>  	{ USB_DEVICE(0xa257, 0x2013) },
>  	{ }
> @@ -803,6 +821,8 @@ static struct usb_driver dln2_driver = {
>  	.probe = dln2_probe,
>  	.disconnect = dln2_disconnect,
>  	.id_table = dln2_table,
> +	.suspend = dln2_suspend,
> +	.resume = dln2_resume,
>  };
>  
>  module_usb_driver(dln2_driver);

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 0/2] DLN2 fixes related to suspend/resume

[Johan Hovold]

On Mon, Jan 19, 2015 at 01:51:34PM +0200, Octavian Purdila wrote:
> Hi Lee,
> 
> Here is the 3rd version of the patch-set against for-mfd-next branch
> with the Reviewed-by tags from Johan. Note that I dropped the two GPIO
> patches as they were already merged by Linus W in the GPIO tree.
> 
> Thanks,
> Tavi
> 
> Changes since v2:
> 
>  * Dropped the GPIO fixes from this series as they were merged in the
>    GPIO tree
> 
>  * Removed a stray newline
> 
>  * Updated the resume / suspend patch commit with more information
>    about which cases it fixes and which it does not

Update looks good too.

Thanks,
Johan