Re: [RESEND] i2c: imx: defer probing on dma channel request

[Laurentiu Tudor]

Hi Leo,

On 27.03.2019 20:47, Li Yang wrote:
> On Wed, Mar 27, 2019 at 8:46 AM Laurentiu Tudor <laurentiu.tudor@xxxxxxx> wrote:
>>
>> Hello,
>>
>> Just FYI, I'm still seeing issues with the dma driver compiled _out_,
>> trying to test i2c without dma support. I get the crash below in generic
>> driver code later in the boot process, debug is in progress.
> 
> That probably means the dma API shouldn't return -EPROBE_DEFER when
> the driver is not compiled in?

Right, problem is that the dmaengine doesn't do this check, or in other 
words is presumes that if you have a "dmas" property in the device tree 
pointing to a certain dma controller then it's expected that the driver 
for that dma controller is compiled in. I'll could try to look for a 
possibility to add such a check but I'm not very optimistic.
On a side, I found the cause of the crash: the i2c adapter created in 
the i2c probe function is leaked on the error path and the device 
created behind it messes the deferal mechanism in the generic device 
code. I'll submit a patch to fix the leak.

---
Best Regards, Laurentiu

>>
>> P.S. This is seen on an NXP LS1043A chip.
>>
>> [    5.152697] Unable to handle kernel NULL pointer dereference at
>> virtual address 0000000000000010
>> [    5.161483] Mem abort info:
>> [    5.164311]   ESR = 0x96000004
>> [    5.167407]   Exception class = DABT (current EL), IL = 32 bits
>> [    5.173391] device_initialize: dev = ffff800027756808
>> [    5.178446] usb 4-1: new SuperSpeed Gen 1 USB device number 2 using
>> xhci-hcd
>> [    5.185489]   SET = 0, FnV = 0
>> [    5.188532]   EA = 0, S1PTW = 0
>> [    5.191676] Data abort info:
>> [    5.194599]   ISV = 0, ISS = 0x00000004
>> [    5.198476]   CM = 0, WnR = 0
>> [    5.201485] [0000000000000010] user address but active_mm is swapper
>> [    5.207894] Internal error: Oops: 96000004 [#1] PREEMPT SMP
>> [    5.213455] Modules linked in:
>> [    5.216502] CPU: 0 PID: 18 Comm: kworker/0:1 Not tainted
>> 5.1.0-rc2-next-20190327-00021-g7b1a4c075381-dirty #15
>> [    5.226489] Hardware name: LS1043A RDB Board (DT)
>> [    5.231189] Workqueue: events deferred_probe_work_func
>> [    5.236317] pstate: a0000005 (NzCv daif -PAN -UAO)
>> [    5.241098] pc : device_reorder_to_tail+0x13c/0x1b8
>> [    5.245965] lr : device_reorder_to_tail+0xd8/0x1b8
>> [    5.250743] sp : ffff000011823c70
>> [    5.254046] x29: ffff000011823c70 x28: 0000000000000000
>> [    5.259347] x27: ffff0000117abcd8 x26: ffff000011293a20
>> [    5.264649] x25: ffff00001102a000 x24: ffff00001102a708
>> [    5.269950] x23: ffff00001102a700 x22: ffff000010d28000
>> [    5.275251] x21: ffff80007393b9a0 x20: fffffffffffffff8
>> [    5.280552] x19: ffff80007393b8f0 x18: ffffffffffffffff
>> [    5.285853] x17: 0000000000000002 x16: 0000000000000000
>> [    5.291154] x15: ffff00001127d6c8 x14: 000000000000017e
>> [    5.296454] x13: 0000000000000001 x12: 0000000000000000
>> [    5.301755] x11: 0000000000000000 x10: 0000000000000980
>> [    5.307055] x9 : ffff0000118238c0 x8 : ffff800073872560
>> [    5.312356] x7 : ffff800073871d00 x6 : 0000000000000058
>> [    5.317656] x5 : 000000000000000f x4 : 0000000000000100
>> [    5.322957] x3 : 000080006a351000 x2 : 0991ec05b7534200
>> [    5.328257] x1 : fffffffffffffff8 x0 : 0000000000000000
>> [    5.333560] Process kworker/0:1 (pid: 18, stack limit =
>> 0x(____ptrval____))
>> [    5.340509] Call trace:
>> [    5.342945]  device_reorder_to_tail+0x13c/0x1b8
>> [    5.347466]  device_for_each_child+0x50/0xa8
>> [    5.351725]  device_reorder_to_tail+0xc4/0x1b8
>> [    5.356157]  device_pm_move_to_tail+0x34/0x50
>> [    5.360502]  deferred_probe_work_func+0x64/0xa0
>> [    5.365023]  process_one_work+0x1c8/0x320
>> [    5.369021]  worker_thread+0x234/0x428
>> [    5.372761]  kthread+0xf4/0x120
>> [    5.375892]  ret_from_fork+0x10/0x18
>> [    5.379458] Code: 911c2318 911c02f7 d0004e19 b4000274 (f9400e84)
>> [    5.385541] ---[ end trace ab4b151d346a8d41 ]---
>>
>> ---
>> Best Regards, Laurentiu
>>
>> On 25.03.2019 19:12, Steven Price wrote:
>>> On 25/03/2019 15:30, laurentiu.tudor@xxxxxxx wrote:
>>>> From: Laurentiu Tudor <laurentiu.tudor@xxxxxxx>
>>>>
>>>> If the dma controller is not yet probed, defer i2c probe.
>>>> The error path in probe was slightly modified (no functional change)
>>>
>>> There is a functional change for cases like:
>>>
>>>>       ret = pm_runtime_get_sync(&pdev->dev);
>>>>       if (ret < 0)
>>>>               goto rpm_disable;
>>>
>>> ...as rpm_disable will no longer fall through to the call of
>>> clk_disable_unprepare().
>>>
>>>> to avoid triggering this WARN_ON():
>>>> "cg-pll0-div1 already disabled
>>>> WARNING: CPU: 1 PID: 1 at drivers/clk/clk.c:828 clk_core_disable+0xa8/0xb0"
>>>
>>> I'm also not sure how this WARN_ON was hit. i2c_imx_probe() calls
>>> clk_prepare_enable() which should increment the reference count. So it
>>> should always be possible to decrememt the enable_count. What am I missing?
>>>
>>>> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@xxxxxxx>
>>>> ---
>>>>    drivers/i2c/busses/i2c-imx.c | 21 +++++++++++++--------
>>>>    1 file changed, 13 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
>>>> index 42fed40198a0..4e34b1572756 100644
>>>> --- a/drivers/i2c/busses/i2c-imx.c
>>>> +++ b/drivers/i2c/busses/i2c-imx.c
>>>> @@ -1111,7 +1111,8 @@ static int i2c_imx_probe(struct platform_device *pdev)
>>>>                               pdev->name, i2c_imx);
>>>>       if (ret) {
>>>>               dev_err(&pdev->dev, "can't claim irq %d\n", irq);
>>>> -            goto clk_disable;
>>>> +            clk_disable_unprepare(i2c_imx->clk);
>>>> +            return ret;
>>>>       }
>>>>
>>>>       /* Init queue */
>>>> @@ -1161,19 +1162,25 @@ static int i2c_imx_probe(struct platform_device *pdev)
>>>>       pm_runtime_mark_last_busy(&pdev->dev);
>>>>       pm_runtime_put_autosuspend(&pdev->dev);
>>>>
>>>> +    /* Init DMA config if supported */
>>>> +    ret = i2c_imx_dma_request(i2c_imx, phy_addr);
>>>> +    if (ret) {
>>>> +            if (ret != -EPROBE_DEFER)
>>>> +                    dev_info(&pdev->dev, "can't use DMA, using PIO instead.\n");
>>>> +            else
>>>> +                    goto del_adapter;
>>>> +    }
>>>> +
>>>
>>> This can be simplified by reversing the condition:
>>>
>>>        if (ret) {
>>>                if (ret == -EPROBE_DEFER)
>>>                        goto del_adapter;
>>>                dev_info();
>>>        }
>>>
>>> or even:
>>>
>>>        if (ret == -EPROBE_DEFER)
>>>                goto del_adapter;
>>>        else if (ret)
>>>                dev_info();
>>>
>>>>       dev_dbg(&i2c_imx->adapter.dev, "claimed irq %d\n", irq);
>>>>       dev_dbg(&i2c_imx->adapter.dev, "device resources: %pR\n", res);
>>>>       dev_dbg(&i2c_imx->adapter.dev, "adapter name: \"%s\"\n",
>>>>               i2c_imx->adapter.name);
>>>>
>>>> -    /* Init DMA config if supported */
>>>> -    ret = i2c_imx_dma_request(i2c_imx, phy_addr);
>>>> -    if (ret < 0)
>>>> -            goto clk_notifier_unregister;
>>>> -
>>>>       dev_info(&i2c_imx->adapter.dev, "IMX I2C adapter registered\n");
>>>>       return 0;   /* Return OK */
>>>>
>>>> +del_adapter:
>>>> +    i2c_del_adapter(&i2c_imx->adapter);
>>>
>>> This looks like a separate fix (previously the call to
>>> i2c_add_numbered_adapter() was not undone in case of later errors). It
>>> worth spelling this out in the commit message.
>>>
>>> Thanks,
>>>
>>> Steve
>>>
>>>>    clk_notifier_unregister:
>>>>       clk_notifier_unregister(i2c_imx->clk, &i2c_imx->clk_change_nb);
>>>>    rpm_disable:
>>>> @@ -1182,8 +1189,6 @@ static int i2c_imx_probe(struct platform_device *pdev)
>>>>       pm_runtime_set_suspended(&pdev->dev);
>>>>       pm_runtime_dont_use_autosuspend(&pdev->dev);
>>>>
>>>> -clk_disable:
>>>> -    clk_disable_unprepare(i2c_imx->clk);
>>>>       return ret;
>>>>    }
>>>>
>>>>
>>>