On Fri, Jun 12, 2020 at 10:02:40AM +0200, Oleksij Rempel wrote:
> On Fri, Jun 12, 2020 at 09:38:15AM +0200, Krzysztof Kozlowski wrote:
> > On Fri, Jun 12, 2020 at 07:51:14AM +0200, Oleksij Rempel wrote:
> > > Hi Krzysztof,
> > >
> > > thank you for your patch.
> > >
> > > On Wed, Jun 10, 2020 at 03:46:42PM +0200, Krzysztof Kozlowski wrote:
> > > > If interrupt comes early (could be triggered with CONFIG_DEBUG_SHIRQ),
> > > > the i2c_imx_isr() will access registers before the I2C hardware is
> > > > initialized. This leads to external abort on non-linefetch on Toradex
> > > > Colibri VF50 module (with Vybrid VF5xx):
> > > >
> > > > Unhandled fault: external abort on non-linefetch (0x1008) at 0x8882d003
> > > > Internal error: : 1008 [#1] ARM
> > > > Modules linked in:
> > > > CPU: 0 PID: 1 Comm: swapper Not tainted 5.7.0 #607
> > > > Hardware name: Freescale Vybrid VF5xx/VF6xx (Device Tree)
> > > > (i2c_imx_isr) from [<8017009c>] (free_irq+0x25c/0x3b0)
> > > > (free_irq) from [<805844ec>] (release_nodes+0x178/0x284)
> > > > (release_nodes) from [<80580030>] (really_probe+0x10c/0x348)
> > > > (really_probe) from [<80580380>] (driver_probe_device+0x60/0x170)
> > > > (driver_probe_device) from [<80580630>] (device_driver_attach+0x58/0x60)
> > > > (device_driver_attach) from [<805806bc>] (__driver_attach+0x84/0xc0)
> > > > (__driver_attach) from [<8057e228>] (bus_for_each_dev+0x68/0xb4)
> > > > (bus_for_each_dev) from [<8057f3ec>] (bus_add_driver+0x144/0x1ec)
> > > > (bus_add_driver) from [<80581320>] (driver_register+0x78/0x110)
> > > > (driver_register) from [<8010213c>] (do_one_initcall+0xa8/0x2f4)
> > > > (do_one_initcall) from [<80c0100c>] (kernel_init_freeable+0x178/0x1dc)
> > > > (kernel_init_freeable) from [<80807048>] (kernel_init+0x8/0x110)
> > > > (kernel_init) from [<80100114>] (ret_from_fork+0x14/0x20)
> > > >
> > > > Additionally, the i2c_imx_isr() could wake up the wait queue
> > > > (imx_i2c_struct->queue) before its initialization happens.
> > > >
> > > > Fixes: 1c4b6c3bcf30 ("i2c: imx: implement bus recovery")
> > > > Cc: <stable@xxxxxxxxxxxxxxx>
> > > > Signed-off-by: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
> > >
> > >
> > > I assume register access is aborted, because the IP core clock is not
> > > enabled. In this case we have bigger problem then just probe.
> >
> > If by IP core clock you mean the clock which driver is getting, then
> > answer is no. This clock is enabled.
> >
> > > Since this driver support runtime power management, the clock will be
> > > disabled as soon as transfer is done. It means, on shared interrupt, we
> > > will get in trouble even if there is no active transfer.
> >
> > The driver's runtime PM plays only with this one clock, so it seems
> > you meant i2c_imx->clk. It is not this problem.
> >
> > >
> > > So, probably the only way to fix it, is to check in i2c_imx_isr() if the
> > > HW is expected to be active and register access should be save.
> >
> > Checking in every interrupt whether the interrupt should be serviced
> > based on some SW flag because HW might be disabled? That looks unusual,
> > like a hack.
> >
> > No, the interrupt should be registered when the driver and some other
> > pieces of HW are ready to service it.
>
> OK.
> please make sure, irq is probed before calling
> i2c_add_numbered_adapter(). This will trigger deferred probing of
> slave devices. Since the irq handler will be added later, tx completion
> of some requests will be lost or fail.
Right. I'll move the devm_request_irq().
Best regards,
Krzysztof
