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

[Jay Fang <f.fangjian@xxxxxxxxxx>]

Dear Mark,

On 2021/3/4 20:34, Mark Brown wrote:
> 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.

> +
> +	/* Disable is needed to deal with transfer timeout */
> +	hisi_spi_disable(hs);
> +
> +	hisi_spi_flush_fifo(hs);
> +	hisi_spi_update_cr(hs, spi, transfer);
Especially given this, if there may be some left over operations going
on elsewhere might there be races due to that?  I'm also wondering if
it's faster to just reset the controller as is done in some error
handling paths. (Mark)

As the previous review said, would it be better to reset controller
and waiting for interrupt to complete in error handling paths?
As shown in the code below.

	static void hisi_spi_handle_err(struct spi_controller *master,
		struct spi_message *msg)
	{
		struct hisi_spi *hs = spi_controller_get_devdata(master);

		hisi_spi_reset(hs);
		hisi_spi_flush_fifo(hs);

		/*
		 * Wait for interrupt handler that is
		 * already in timeout to complete.
		 */
		synchronize_irq(hs->irq);
	}

This ensures that the new transfer will not start until the previous
transfer is completed. And we don't need to disable the controller
every time before data transfer.


Thanks
Jay

> 
>>>> +	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.
Will fix.
> 
>>>> +	/* 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.
Will fix.
>