Re: [PATCH] spi: Add HiSilicon SPI controller driver support

From: Mark Brown <broonie@kernel.org>
To: "Fangjian (Jay)" <f.fangjian@huawei.com>
Cc: linux-spi@vger.kernel.org, huangdaode@huawei.com, linuxarm@huawei.com
Subject: Re: [PATCH] spi: Add HiSilicon SPI controller driver support
Date: Thu, 4 Mar 2021 12:34:25 +0000	[thread overview]
Message-ID: <20210304123425.GA4731@sirena.org.uk> (raw)
In-Reply-To: <79a0bb79-654b-8afc-f34a-c3a08bae275c@huawei.com>

[-- Attachment #1: Type: text/plain, Size: 1997 bytes --]

On Thu, Mar 04, 2021 at 10:54:40AM +0800, Fangjian (Jay) wrote:
> On 2021/3/1 21:54, Mark Brown wrote:
> > On Mon, Mar 01, 2021 at 07:56:11PM +0800, Jay Fang wrote:

> > > +/* Disable IRQ bits */
> > > +static void hisi_spi_mask_intr(struct hisi_spi *hs, u32 mask)
> > > +{
> > > +	u32 new_mask;
> > > +
> > > +	new_mask = readl(hs->regs + HISI_SPI_IMR) | mask;
> > > +	writel(new_mask, hs->regs + HISI_SPI_IMR);
> > > +}

> > This is a read/modify/write cycle and appears to be called from at least
> > process and interrupt context but I'm not seeing anything that stops two
> > different callers of it or the matching unmask function from running at
> > the same time.

> Those mask/unmask will not be called at the same time from process and
> interrupt context. In process context, unmask will be called after SPI
> controller be Disable and Flush (interrupt handing has ended).

Given that this is disabling the interrupt that doesn't sound like it's
going to be entirely robust - if we need to disable the interrupt
presumably there's some chance it might fire.

> > > +	struct hisi_spi *hs = spi_controller_get_devdata(master);
> > > +
> > > +	hs->n_bytes = hisi_spi_n_bytes(transfer);
> > > +	hs->tx = (void *)transfer->tx_buf;
> > If there's a need to cast to void * something is very wrong here.

> Yes, fix compile warning.

This cast just masks whatever the problem is, if the compiler is
complaining about using a void pointer it's spotted an issue.

> > > +	/* Ensure the data above is visible for all CPUs */
> > > +	smp_mb();

> > This memory barrier seems worrying...  are you *sure* this is the best
> > way to sync, and that the sync is best done here if it is needed rather
> > than after everything else is set up?

> The commit 0b6bfad ("spi: spi-dw: Remove extraneous locking") explains
> why memory barrier is needed here. And put it here to make it easier to
> understand.

The reader of this code won't have any kind of pointer to that commit,
this needs to be clearer.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]