Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 12 Jun 2020 09:38:15 +0200
From:   Krzysztof Kozlowski <krzk@kernel.org>
To:     Oleksij Rempel <o.rempel@pengutronix.de>
Cc:     Oleksij Rempel <linux@rempel-privat.de>,
        Pengutronix Kernel Team <kernel@pengutronix.de>,
        Shawn Guo <shawnguo@kernel.org>,
        Sascha Hauer <s.hauer@pengutronix.de>,
        Fabio Estevam <festevam@gmail.com>,
        NXP Linux Team <linux-imx@nxp.com>,
        Gao Pan <b54642@freescale.com>,
        Fugang Duan <B38611@freescale.com>,
        Wolfram Sang <wsa@kernel.org>, linux-i2c@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        stable@vger.kernel.org
Subject: Re: [PATCH] i2c: imx: Fix external abort on early interrupt
Message-ID: <20200612073815.GA25803@pi3>
References: <1591796802-23504-1-git-send-email-krzk@kernel.org>
 <20200612055114.alhm2uakoze6epvf@pengutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20200612055114.alhm2uakoze6epvf@pengutronix.de>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@vger.kernel.org>
> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> 
> 
> 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.

Best regards,
Krzysztof