All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alexander Amelkin <alexander@amelkin.msk.ru>
To: linux-usb@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>
Subject: [PATCH 2/3] usb: max3421-hcd: Fix crash on the driver removal
Date: Fri, 26 May 2017 14:30:56 +0300	[thread overview]
Message-ID: <365366d9802b5b4ed1ec3a33d0ea0da3@amelkin.msk.ru> (raw)
In-Reply-To: <fe4f9b0f3a18d48dec6c0a8d79cae386@amelkin.msk.ru>

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

NOTE:
Please don't use the plain text here as a patch because it most probably 
is corrupted by my webmail client.
Attached is a copy of the following text guaranteed to have correct 
tabs/spaces.
-------------------------
 From 8c4c65d3892df3721474023836216a02e03fb23e Mon Sep 17 00:00:00 2001
 From: Alexander Amelkin <amelkin@fastwel.ru>
Date: Fri, 14 Apr 2017 17:58:07 +0300
Subject: [PATCH 2/3] usb: max3421-hcd: Fix crash on the driver removal

The driver was calling kthread_stop inside a spin-locked
region while the thread was calling schedule() on a regular
basis. That resulted in panic due to 'scheduling while atomic'.

There was no need to get the spin-lock before stopping the
thread as thread stopping is asynchronous and the thread
only stops when it detects the stop flag set by kthread_stop(),
at which point it is not accessing any resources anyway.

Hence, this patch removes the spin-locking around the
kthread_stop() call.

Additionally, the allocated resources were not free'd in
the correct order. Some were not free'd at all. This patch
switches all resource allocations to devm_* functions
and also reorders deallocation to properly revert the
allocations (although not actually needed for devm-allocated
resources).

Signed-off-by: Alexander Amelkin <alexander@amelkin.msk.ru>
---
  drivers/usb/host/max3421-hcd.c | 29 +++++++++++++++--------------
  1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/host/max3421-hcd.c 
b/drivers/usb/host/max3421-hcd.c
index f600052..bd59c16 100644
--- a/drivers/usb/host/max3421-hcd.c
+++ b/drivers/usb/host/max3421-hcd.c
@@ -1925,10 +1925,10 @@ max3421_probe(struct spi_device *spi)
  	max3421_hcd_list = max3421_hcd;
  	INIT_LIST_HEAD(&max3421_hcd->ep_list);

-	max3421_hcd->tx = kmalloc(sizeof(*max3421_hcd->tx), GFP_KERNEL);
+	max3421_hcd->tx = devm_kzalloc(&spi->dev, sizeof(*max3421_hcd->tx), 
GFP_KERNEL);
  	if (!max3421_hcd->tx)
  		goto error;
-	max3421_hcd->rx = kmalloc(sizeof(*max3421_hcd->rx), GFP_KERNEL);
+	max3421_hcd->rx = devm_kzalloc(&spi->dev, sizeof(*max3421_hcd->rx), 
GFP_KERNEL);
  	if (!max3421_hcd->rx)
  		goto error;

@@ -1946,8 +1946,8 @@ max3421_probe(struct spi_device *spi)
  		goto error;
  	}

-	retval = request_irq(spi->irq, max3421_irq_handler,
-			     IRQF_TRIGGER_LOW, "max3421", hcd);
+	retval = devm_request_irq(&spi->dev, spi->irq, max3421_irq_handler,
+	                          IRQF_TRIGGER_LOW, "max3421", hcd);
  	if (retval < 0) {
  		dev_err(&spi->dev, "failed to request irq %d\n", spi->irq);
  		goto error;
@@ -1976,7 +1976,6 @@ max3421_remove(struct spi_device *spi)
  {
  	struct max3421_hcd *max3421_hcd = NULL, **prev;
  	struct usb_hcd *hcd = NULL;
-	unsigned long flags;

  	for (prev = &max3421_hcd_list; *prev; prev = &(*prev)->next) {
  		max3421_hcd = *prev;
@@ -1990,12 +1989,20 @@ max3421_remove(struct spi_device *spi)
  			spi);
  		return -ENODEV;
  	}
-	spin_lock_irqsave(&max3421_hcd->lock, flags);
+
+	devm_free_irq(&spi->dev, spi->irq, hcd);
+
+	usb_remove_hcd(hcd);

  	kthread_stop(max3421_hcd->spi_thread);
-	*prev = max3421_hcd->next;

-	spin_unlock_irqrestore(&max3421_hcd->lock, flags);
+	devm_kfree(&spi->dev, max3421_hcd->rx);
+	max3421_hcd->rx = NULL;
+	devm_kfree(&spi->dev, max3421_hcd->tx);
+	max3421_hcd->tx = NULL;
+
+	*prev = max3421_hcd->next;
+	usb_put_hcd(hcd);

  #if defined(CONFIG_OF)
  	if (spi->dev.platform_data) {
@@ -2005,12 +2012,6 @@ max3421_remove(struct spi_device *spi)
  	}
  #endif

-	free_irq(spi->irq, hcd);
-
-	usb_remove_hcd(hcd);
-
-
-	usb_put_hcd(hcd);
  	return 0;
  }

-- 
2.7.4


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-usb-max3421-hcd-Fix-crash-on-the-driver-removal.patch --]
[-- Type: text/x-diff; name=0002-usb-max3421-hcd-Fix-crash-on-the-driver-removal.patch, Size: 3337 bytes --]

From 8c4c65d3892df3721474023836216a02e03fb23e Mon Sep 17 00:00:00 2001
From: Alexander Amelkin <amelkin@fastwel.ru>
Date: Fri, 14 Apr 2017 17:58:07 +0300
Subject: [PATCH 2/3] usb: max3421-hcd: Fix crash on the driver removal

The driver was calling kthread_stop inside a spin-locked
region while the thread was calling schedule() on a regular
basis. That resulted in panic due to 'scheduling while atomic'.

There was no need to get the spin-lock before stopping the
thread as thread stopping is asynchronous and the thread
only stops when it detects the stop flag set by kthread_stop(),
at which point it is not accessing any resources anyway.

Hence, this patch removes the spin-locking around the
kthread_stop() call.

Additionally, the allocated resources were not free'd in
the correct order. Some were not free'd at all. This patch
switches all resource allocations to devm_* functions
and also reorders deallocation to properly revert the
allocations (although not actually needed for devm-allocated
resources).

Signed-off-by: Alexander Amelkin <alexander@amelkin.msk.ru>
---
 drivers/usb/host/max3421-hcd.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/host/max3421-hcd.c b/drivers/usb/host/max3421-hcd.c
index f600052..bd59c16 100644
--- a/drivers/usb/host/max3421-hcd.c
+++ b/drivers/usb/host/max3421-hcd.c
@@ -1925,10 +1925,10 @@ max3421_probe(struct spi_device *spi)
 	max3421_hcd_list = max3421_hcd;
 	INIT_LIST_HEAD(&max3421_hcd->ep_list);
 
-	max3421_hcd->tx = kmalloc(sizeof(*max3421_hcd->tx), GFP_KERNEL);
+	max3421_hcd->tx = devm_kzalloc(&spi->dev, sizeof(*max3421_hcd->tx), GFP_KERNEL);
 	if (!max3421_hcd->tx)
 		goto error;
-	max3421_hcd->rx = kmalloc(sizeof(*max3421_hcd->rx), GFP_KERNEL);
+	max3421_hcd->rx = devm_kzalloc(&spi->dev, sizeof(*max3421_hcd->rx), GFP_KERNEL);
 	if (!max3421_hcd->rx)
 		goto error;
 
@@ -1946,8 +1946,8 @@ max3421_probe(struct spi_device *spi)
 		goto error;
 	}
 
-	retval = request_irq(spi->irq, max3421_irq_handler,
-			     IRQF_TRIGGER_LOW, "max3421", hcd);
+	retval = devm_request_irq(&spi->dev, spi->irq, max3421_irq_handler,
+	                          IRQF_TRIGGER_LOW, "max3421", hcd);
 	if (retval < 0) {
 		dev_err(&spi->dev, "failed to request irq %d\n", spi->irq);
 		goto error;
@@ -1976,7 +1976,6 @@ max3421_remove(struct spi_device *spi)
 {
 	struct max3421_hcd *max3421_hcd = NULL, **prev;
 	struct usb_hcd *hcd = NULL;
-	unsigned long flags;
 
 	for (prev = &max3421_hcd_list; *prev; prev = &(*prev)->next) {
 		max3421_hcd = *prev;
@@ -1990,12 +1989,20 @@ max3421_remove(struct spi_device *spi)
 			spi);
 		return -ENODEV;
 	}
-	spin_lock_irqsave(&max3421_hcd->lock, flags);
+
+	devm_free_irq(&spi->dev, spi->irq, hcd);
+
+	usb_remove_hcd(hcd);
 
 	kthread_stop(max3421_hcd->spi_thread);
-	*prev = max3421_hcd->next;
 
-	spin_unlock_irqrestore(&max3421_hcd->lock, flags);
+	devm_kfree(&spi->dev, max3421_hcd->rx);
+	max3421_hcd->rx = NULL;
+	devm_kfree(&spi->dev, max3421_hcd->tx);
+	max3421_hcd->tx = NULL;
+
+	*prev = max3421_hcd->next;
+	usb_put_hcd(hcd);
 
 #if defined(CONFIG_OF)
 	if (spi->dev.platform_data) {
@@ -2005,12 +2012,6 @@ max3421_remove(struct spi_device *spi)
 	}
 #endif
 
-	free_irq(spi->irq, hcd);
-
-	usb_remove_hcd(hcd);
-
-
-	usb_put_hcd(hcd);
 	return 0;
 }
 
-- 
2.7.4


  parent reply	other threads:[~2017-05-26 11:31 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-05-26 11:22 [GIT][PULL] Improvements to max3421-hcd driver Alexander Amelkin
2017-05-26 11:22 ` Alexander Amelkin
2017-05-26 11:28 ` [PATCH 1/3] usb: max3421-hcd: Add devicetree support to the driver Alexander Amelkin
2017-05-26 11:28   ` Alexander Amelkin
2017-05-31 19:03   ` Rob Herring
2017-05-31 19:03     ` Rob Herring
2017-05-26 11:30 ` Alexander Amelkin [this message]
2017-05-26 11:32 ` [PATCH 3/3] usb: max3421-hcd: Remove function name from dev_dbg calls Alexander Amelkin
2017-05-26 18:41   ` Greg Kroah-Hartman
2017-05-26 18:41     ` Greg Kroah-Hartman
2017-05-26 18:40 ` [GIT][PULL] Improvements to max3421-hcd driver Greg Kroah-Hartman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=365366d9802b5b4ed1ec3a33d0ea0da3@amelkin.msk.ru \
    --to=alexander@amelkin.msk.ru \
    --cc=devicetree@vger.kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=robh+dt@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.