Re: [PATCH v2 1/2] dmaengine: avalon: Intel Avalon-MM DMA Interface for PCIe

[Alexander Gordeev]

On Wed, Oct 09, 2019 at 03:14:41PM +0300, Dan Carpenter wrote:
> > +config AVALON_DMA_PCI_VENDOR_ID
> > +	hex "PCI vendor ID"
> > +	default "0x1172"
> > +
> > +config AVALON_DMA_PCI_DEVICE_ID
> > +	hex "PCI device ID"
> > +	default "0xe003"
> 
> This feels wrong.  Why isn't it known in advance.

Because device designers would likely use they own IDs. The ones I
put are just defaults inherited from the (Altera) reference design.

> > +	u32 *rd_flags = hw->dma_desc_table_rd.cpu_addr->flags;
> > +	u32 *wr_flags = hw->dma_desc_table_wr.cpu_addr->flags;
> > +	struct avalon_dma_desc *desc;
> > +	struct virt_dma_desc *vdesc;
> > +	bool rd_done;
> > +	bool wr_done;
> > +
> > +	spin_lock(lock);
> > +
> > +	rd_done = (hw->h2d_last_id < 0);
> > +	wr_done = (hw->d2h_last_id < 0);
> > +
> > +	if (rd_done && wr_done) {
> > +		spin_unlock(lock);
> > +		return IRQ_NONE;
> > +	}
> > +
> > +	do {
> > +		if (!rd_done && rd_flags[hw->h2d_last_id])
> > +			rd_done = true;
> > +
> > +		if (!wr_done && wr_flags[hw->d2h_last_id])
> > +			wr_done = true;
> > +	} while (!rd_done || !wr_done);
> 
> This loop is very strange.  It feels like the last_id indexes needs
> to atomic or protected from racing somehow so we don't do an out of
> bounds read.

My bad. I should have put a comment on this. This polling comes from my
reading of the Intel documentation:

"The MSI interrupt notifies the host when a DMA operation has completed.
After the host receives this interrupt, it can poll the DMA read or write
status table to determine which entry or entries have the done bit set."

"The Descriptor Controller writes a 1 to the done bit of the status DWORD
to indicate successful completion. The Descriptor Controller also sends
an MSI interrupt for the final descriptor. After receiving this MSI,
host software can poll the done bit to determine status."

I sense an ambiguity above. It sounds possible an MSI interrupt could be
delivered before corresponding done bit is set. May be imperfect wording..
Anyway, the loop does look weird and in reality I doubt I observed the
done bit unset even once. So I put this polling just in case.

> > +	struct avalon_dma_chan *chan = to_avalon_dma_chan(dma_chan);
> > +	struct avalon_dma_desc *desc;
> > +	gfp_t gfp_flags = in_interrupt() ? GFP_NOWAIT : GFP_KERNEL;
> > +	dma_addr_t dev_addr;
> > +
> > +	if (direction == DMA_MEM_TO_DEV)
> > +		dev_addr = chan->dst_addr;
> > +	else if (direction == DMA_DEV_TO_MEM)
> > +		dev_addr = chan->src_addr;
> > +	else
> > +		return NULL;
> > +
> > +	desc = kzalloc(sizeof(*desc), gfp_flags);
> 
> Everyone else does GFP_WAIT or GFP_ATOMIC.  Is GFP_KERNEL really okay?

I am not sure why not to use GFP_KERNEL from non-atomic context.
Documentation/driver-api/dmaengine/provider.rst claims always to
use GFP_NOWAIT though:

  - Any allocation you might do should be using the GFP_NOWAIT
    flag, in order not to potentially sleep, but without depleting
    the emergency pool either.

So probably I just should use GFP_NOWAIT.

Thanks, Dan!

> regards,
> dan carpenter
>