* [RESEND PATCH 1/4] usb: musb: Fix runtime PM race in musb_queue_resume_work
2020-10-27 16:41 [RESEND PATCH 0/4] MUSB and jz4740-musb fixes Paul Cercueil
@ 2020-10-27 16:41 ` Paul Cercueil
2020-10-27 16:41 ` [RESEND PATCH 2/4] usb: musb: Fix NULL check on struct musb_request field Paul Cercueil
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Paul Cercueil @ 2020-10-27 16:41 UTC (permalink / raw)
To: Bin Liu
Cc: Greg Kroah-Hartman, Tony Lindgren, od, linux-usb, linux-kernel,
Paul Cercueil, stable
musb_queue_resume_work() would call the provided callback if the runtime
PM status was 'active'. Otherwise, it would enqueue the request if the
hardware was still suspended (musb->is_runtime_suspended is true).
This causes a race with the runtime PM handlers, as it is possible to be
in the case where the runtime PM status is not yet 'active', but the
hardware has been awaken (PM resume function has been called).
When hitting the race, the resume work was not enqueued, which probably
triggered other bugs further down the stack. For instance, a telnet
connection on Ingenic SoCs would result in a 50/50 chance of a
segmentation fault somewhere in the musb code.
Rework the code so that either we call the callback directly if
(musb->is_runtime_suspended == 0), or enqueue the query otherwise.
Fixes: ea2f35c01d5e ("usb: musb: Fix sleeping function called from invalid context for hdrc glue")
Cc: stable@vger.kernel.org # v4.9
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Reviewed-by: Tony Lindgren <tony@atomide.com>
Tested-by: Tony Lindgren <tony@atomide.com>
---
drivers/usb/musb/musb_core.c | 31 +++++++++++++++++--------------
1 file changed, 17 insertions(+), 14 deletions(-)
diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 849e0b770130..1cd87729ba60 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -2240,32 +2240,35 @@ int musb_queue_resume_work(struct musb *musb,
{
struct musb_pending_work *w;
unsigned long flags;
+ bool is_suspended;
int error;
if (WARN_ON(!callback))
return -EINVAL;
- if (pm_runtime_active(musb->controller))
- return callback(musb, data);
+ spin_lock_irqsave(&musb->list_lock, flags);
+ is_suspended = musb->is_runtime_suspended;
+
+ if (is_suspended) {
+ w = devm_kzalloc(musb->controller, sizeof(*w), GFP_ATOMIC);
+ if (!w) {
+ error = -ENOMEM;
+ goto out_unlock;
+ }
- w = devm_kzalloc(musb->controller, sizeof(*w), GFP_ATOMIC);
- if (!w)
- return -ENOMEM;
+ w->callback = callback;
+ w->data = data;
- w->callback = callback;
- w->data = data;
- spin_lock_irqsave(&musb->list_lock, flags);
- if (musb->is_runtime_suspended) {
list_add_tail(&w->node, &musb->pending_list);
error = 0;
- } else {
- dev_err(musb->controller, "could not add resume work %p\n",
- callback);
- devm_kfree(musb->controller, w);
- error = -EINPROGRESS;
}
+
+out_unlock:
spin_unlock_irqrestore(&musb->list_lock, flags);
+ if (!is_suspended)
+ error = callback(musb, data);
+
return error;
}
EXPORT_SYMBOL_GPL(musb_queue_resume_work);
--
2.28.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [RESEND PATCH 2/4] usb: musb: Fix NULL check on struct musb_request field
2020-10-27 16:41 [RESEND PATCH 0/4] MUSB and jz4740-musb fixes Paul Cercueil
2020-10-27 16:41 ` [RESEND PATCH 1/4] usb: musb: Fix runtime PM race in musb_queue_resume_work Paul Cercueil
@ 2020-10-27 16:41 ` Paul Cercueil
2020-10-29 14:58 ` Tony Lindgren
2020-10-27 16:41 ` [RESEND PATCH 3/4] usb: musb: dma: Remove unused variable Paul Cercueil
2020-10-27 16:42 ` [RESEND PATCH 4/4] musb: jz4740: Add missing CR to error strings Paul Cercueil
3 siblings, 1 reply; 6+ messages in thread
From: Paul Cercueil @ 2020-10-27 16:41 UTC (permalink / raw)
To: Bin Liu
Cc: Greg Kroah-Hartman, Tony Lindgren, od, linux-usb, linux-kernel,
Paul Cercueil, Maarten ter Huurne
The 'request' variable is a pointer to the 'request' field of the
struct musb_request 'req' pointer. It only worked until now because
the 'request' field is the first one in the musb_request structure, but
as soon as that changes, the check will be invalid.
Fix it preventively by doing the NULL-check on the 'req' pointer
instead.
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Suggested-by: Maarten ter Huurne <maarten@treewalker.org>
---
drivers/usb/musb/musb_gadget.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index f62ffaede1ab..ef374d4dd94a 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -451,7 +451,7 @@ void musb_g_tx(struct musb *musb, u8 epnum)
return;
}
- if (request) {
+ if (req) {
trace_musb_req_tx(req);
--
2.28.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [RESEND PATCH 2/4] usb: musb: Fix NULL check on struct musb_request field
2020-10-27 16:41 ` [RESEND PATCH 2/4] usb: musb: Fix NULL check on struct musb_request field Paul Cercueil
@ 2020-10-29 14:58 ` Tony Lindgren
0 siblings, 0 replies; 6+ messages in thread
From: Tony Lindgren @ 2020-10-29 14:58 UTC (permalink / raw)
To: Paul Cercueil
Cc: Bin Liu, Greg Kroah-Hartman, od, linux-usb, linux-kernel,
Maarten ter Huurne
* Paul Cercueil <paul@crapouillou.net> [201027 16:42]:
> The 'request' variable is a pointer to the 'request' field of the
> struct musb_request 'req' pointer. It only worked until now because
> the 'request' field is the first one in the musb_request structure, but
> as soon as that changes, the check will be invalid.
>
> Fix it preventively by doing the NULL-check on the 'req' pointer
> instead.
Acked-by: Tony Lindgren <tony@atomide.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [RESEND PATCH 3/4] usb: musb: dma: Remove unused variable
2020-10-27 16:41 [RESEND PATCH 0/4] MUSB and jz4740-musb fixes Paul Cercueil
2020-10-27 16:41 ` [RESEND PATCH 1/4] usb: musb: Fix runtime PM race in musb_queue_resume_work Paul Cercueil
2020-10-27 16:41 ` [RESEND PATCH 2/4] usb: musb: Fix NULL check on struct musb_request field Paul Cercueil
@ 2020-10-27 16:41 ` Paul Cercueil
2020-10-27 16:42 ` [RESEND PATCH 4/4] musb: jz4740: Add missing CR to error strings Paul Cercueil
3 siblings, 0 replies; 6+ messages in thread
From: Paul Cercueil @ 2020-10-27 16:41 UTC (permalink / raw)
To: Bin Liu
Cc: Greg Kroah-Hartman, Tony Lindgren, od, linux-usb, linux-kernel,
Paul Cercueil
Remove unused-but-set devctl variable.
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
drivers/usb/musb/musbhsdma.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c
index 0aacfc8be5a1..7acd1635850d 100644
--- a/drivers/usb/musb/musbhsdma.c
+++ b/drivers/usb/musb/musbhsdma.c
@@ -321,8 +321,6 @@ irqreturn_t dma_controller_irq(int irq, void *private_data)
musb_channel->channel.status =
MUSB_DMA_STATUS_BUS_ABORT;
} else {
- u8 devctl;
-
addr = musb_read_hsdma_addr(mbase,
bchannel);
channel->actual_len = addr
@@ -336,8 +334,6 @@ irqreturn_t dma_controller_irq(int irq, void *private_data)
< musb_channel->len) ?
"=> reconfig 0" : "=> complete");
- devctl = musb_readb(mbase, MUSB_DEVCTL);
-
channel->status = MUSB_DMA_STATUS_FREE;
/* completed */
--
2.28.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [RESEND PATCH 4/4] musb: jz4740: Add missing CR to error strings
2020-10-27 16:41 [RESEND PATCH 0/4] MUSB and jz4740-musb fixes Paul Cercueil
` (2 preceding siblings ...)
2020-10-27 16:41 ` [RESEND PATCH 3/4] usb: musb: dma: Remove unused variable Paul Cercueil
@ 2020-10-27 16:42 ` Paul Cercueil
3 siblings, 0 replies; 6+ messages in thread
From: Paul Cercueil @ 2020-10-27 16:42 UTC (permalink / raw)
To: Bin Liu
Cc: Greg Kroah-Hartman, Tony Lindgren, od, linux-usb, linux-kernel,
Paul Cercueil
If you pass a string that is not terminated with a carriage return to
dev_err(), it will eventually be printed with a carriage return, but
not right away, since the kernel will wait for a pr_cont().
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
drivers/usb/musb/jz4740.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/musb/jz4740.c b/drivers/usb/musb/jz4740.c
index c4fe1f4cd17a..50afc3b94a37 100644
--- a/drivers/usb/musb/jz4740.c
+++ b/drivers/usb/musb/jz4740.c
@@ -205,26 +205,26 @@ static int jz4740_probe(struct platform_device *pdev)
pdata = of_device_get_match_data(dev);
if (!pdata) {
- dev_err(dev, "missing platform data");
+ dev_err(dev, "missing platform data\n");
return -EINVAL;
}
musb = platform_device_alloc("musb-hdrc", PLATFORM_DEVID_AUTO);
if (!musb) {
- dev_err(dev, "failed to allocate musb device");
+ dev_err(dev, "failed to allocate musb device\n");
return -ENOMEM;
}
clk = devm_clk_get(dev, "udc");
if (IS_ERR(clk)) {
- dev_err(dev, "failed to get clock");
+ dev_err(dev, "failed to get clock\n");
ret = PTR_ERR(clk);
goto err_platform_device_put;
}
ret = clk_prepare_enable(clk);
if (ret) {
- dev_err(dev, "failed to enable clock");
+ dev_err(dev, "failed to enable clock\n");
goto err_platform_device_put;
}
@@ -240,19 +240,19 @@ static int jz4740_probe(struct platform_device *pdev)
ret = platform_device_add_resources(musb, pdev->resource,
pdev->num_resources);
if (ret) {
- dev_err(dev, "failed to add resources");
+ dev_err(dev, "failed to add resources\n");
goto err_clk_disable;
}
ret = platform_device_add_data(musb, pdata, sizeof(*pdata));
if (ret) {
- dev_err(dev, "failed to add platform_data");
+ dev_err(dev, "failed to add platform_data\n");
goto err_clk_disable;
}
ret = platform_device_add(musb);
if (ret) {
- dev_err(dev, "failed to register musb device");
+ dev_err(dev, "failed to register musb device\n");
goto err_clk_disable;
}
--
2.28.0
^ permalink raw reply related [flat|nested] 6+ messages in thread