Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 17 Jun 2022 12:45:18 +0100
From:   Mark Brown <broonie@kernel.org>
To:     Jon Lin <jon.lin@rock-chips.com>
Cc:     heiko@sntech.de, linux-spi@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] spi: rockchip: Disable local irq when pio write out of
 interrupt service
Message-ID: <YqxpTqvaQb4cohTh@sirena.org.uk>
References: <20220613092744.9726-1-jon.lin@rock-chips.com>
 <YqcveggUU7yaXuk1@sirena.org.uk>
 <b97ee70c-70ee-6b15-65d6-a176125dcfd8@rock-chips.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="CLGvc6v7On6xFTSG"
Content-Disposition: inline
In-Reply-To: <b97ee70c-70ee-6b15-65d6-a176125dcfd8@rock-chips.com>
Precedence: bulk


--CLGvc6v7On6xFTSG
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Fri, Jun 17, 2022 at 02:24:10PM +0800, Jon Lin wrote:
> On 2022/6/13 20:37, Mark Brown wrote:
> > On Mon, Jun 13, 2022 at 05:27:44PM +0800, Jon Lin wrote:

> > > +	spin_unlock_irqrestore(&rs->lock, flags);

> > So this is effectively just disabling interrupts during PIO, there's no
> > other users of the lock which is rather heavyweight.  What's the actual
> > issue here?  We should also have something saying what's going on in the
> > code since right now the lock just looks redundant.

> For lock: In order to avoid special situations, such as when the CPU
> frequency drops to close to the IO rate, the water line interrupt is
> triggered during FIFO filling (triggered by other CPUs), resulting in
> critical resources still not being protected in place. For local IRQ

So essentially we're so slow in filling the FIFO when starting a
transfer that the interrupt triggers in the middle of the initial FIFO
fill?  Something that tricky *really* needs a comment adding.

Ideally we'd just leave the interrupt masked until the FIFO is filled
though, looking at the driver I see that there is an interrupt mask
register which seems to have some level of masking available and I do
note that in rockchip_spi_prepare_irq() we unmask interrupts before we
start filling the FIFO rather than afterwards.  Would reversing the
unmask order there address the issue more cleanly?

> disable: Turning off the local interrupt is mainly to prevent the CPU
> schedule from being interrupted when filling FIFO.

If it were just this then there's preempt_disable(), but what's the
problem with being preempted during the FIFO fill?

--CLGvc6v7On6xFTSG
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmKsaU0ACgkQJNaLcl1U
h9B+Lgf/aLgqdBeHId+v7ymWQ/Ek1vdJ2rjI0CAdGbF8OAmLPqjmOXm8neX8rAJc
OSrQaiXEUmHSsWanCg2AuyX2kQ++TDePT55bA1wCTMwR8ICHSdK6GbdQaxUg7hk3
H2uyaPCZWr9bEsH0xM2//kgl26MqQ1PZhO/rirE+E3OHi0AOjeX/wSipAOn/X6+4
YNz/UW3cvbYWO+Oa8Lz5m2AB56Med0qOswwl65jp/jyF9f16UuwUfEyCbdY469/m
yUpz2PdongU1+0DVlaB8a4nUsUlOCENQzYmy+aPVIRp888Tbz4OYBq9ISjbPY2zU
WIleNV4RTnqq6TcM6o3thBpkJGuW/w==
=fIJJ
-----END PGP SIGNATURE-----

--CLGvc6v7On6xFTSG--