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: [email protected] # v4.9+
Signed-off-by: Paul Cercueil <[email protected]>
Reviewed-by: Tony Lindgren <[email protected]>
Tested-by: Tony Lindgren <[email protected]>
---
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.29.2
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 <[email protected]>
Suggested-by: Maarten ter Huurne <[email protected]>
Acked-by: Tony Lindgren <[email protected]>
---
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.29.2
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 <[email protected]>
---
drivers/usb/musb/jz4740.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/musb/jz4740.c b/drivers/usb/musb/jz4740.c
index c4fe1f4cd17a..5b7d576bf6ee 100644
--- a/drivers/usb/musb/jz4740.c
+++ b/drivers/usb/musb/jz4740.c
@@ -116,13 +116,13 @@ static int jz4740_musb_init(struct musb *musb)
if (IS_ERR(musb->xceiv)) {
err = PTR_ERR(musb->xceiv);
if (err != -EPROBE_DEFER)
- dev_err(dev, "No transceiver configured: %d", err);
+ dev_err(dev, "No transceiver configured: %d\n", err);
return err;
}
glue->role_sw = usb_role_switch_register(dev, &role_sw_desc);
if (IS_ERR(glue->role_sw)) {
- dev_err(dev, "Failed to register USB role switch");
+ dev_err(dev, "Failed to register USB role switch\n");
return PTR_ERR(glue->role_sw);
}
@@ -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.29.2
Remove unused-but-set devctl variable.
Signed-off-by: Paul Cercueil <[email protected]>
---
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.29.2
On 1/23/21 5:24 PM, Paul Cercueil wrote:
> 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).
Awakened. :-)
> 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: [email protected] # v4.9+
> Signed-off-by: Paul Cercueil <[email protected]>
> Reviewed-by: Tony Lindgren <[email protected]>
> Tested-by: Tony Lindgren <[email protected]>
[...]
MBR, Sergei
Hi Sergei,
Le sam. 23 janv. 2021 ? 19:41, Sergei Shtylyov
<[email protected]> a ?crit :
> On 1/23/21 5:24 PM, Paul Cercueil wrote:
>
>> 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).
>
> Awakened. :-)
Oops. Hopefully Bin or Greg can fix it when merging (if I don't need to
v2, that is to say - feedback welcome).
Cheers,
-Paul
>> 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: [email protected] # v4.9+
>> Signed-off-by: Paul Cercueil <[email protected]>
>> Reviewed-by: Tony Lindgren <[email protected]>
>> Tested-by: Tony Lindgren <[email protected]>
> [...]
>
>
> MBR, Sergei