dmaengine Archive on lore.kernel.org
 help / color / Atom feed
From: Alexander Gordeev <a.gordeev.box@gmail.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org,
	Michael Chen <micchen@altera.com>,
	dmaengine@vger.kernel.org
Subject: Re: [PATCH v2 1/2] dmaengine: avalon: Intel Avalon-MM DMA Interface for PCIe
Date: Wed, 9 Oct 2019 16:58:12 +0200
Message-ID: <20191009145811.GA3823@AlexGordeev-DPT-VI0092> (raw)
In-Reply-To: <20191009121441.GM25098@kadam>

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
> 

  reply index

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-09 10:12 [PATCH v2 0/2] dmaengine: avalon: Support " Alexander Gordeev
2019-10-09 10:12 ` [PATCH v2 1/2] dmaengine: avalon: Intel " Alexander Gordeev
2019-10-09 12:14   ` Dan Carpenter
2019-10-09 14:58     ` Alexander Gordeev [this message]
2019-10-09 18:53       ` Dan Carpenter
2019-10-10  8:51         ` Alexander Gordeev
2019-10-10 11:30           ` Dan Carpenter
2019-10-15 11:24             ` Alexander Gordeev
2019-10-15 11:41               ` Dan Carpenter
2019-10-15 12:27                 ` Alexander Gordeev
2019-10-15 13:19               ` Dan Carpenter
2019-10-15 14:31                 ` Alexander Gordeev
2019-10-15 14:47                   ` Dan Carpenter
2019-10-09 13:07   ` Greg KH
2019-10-15 10:33   ` Vinod Koul
2019-10-15 13:11     ` Alexander Gordeev
2019-10-09 10:12 ` [PATCH RFC v2 2/2] dmaengine: avalon: Intel Avalon-MM DMA Interface for PCIe test Alexander Gordeev
2019-10-09 13:08   ` Greg KH
2019-10-09 13:46   ` Dan Carpenter

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191009145811.GA3823@AlexGordeev-DPT-VI0092 \
    --to=a.gordeev.box@gmail.com \
    --cc=dan.carpenter@oracle.com \
    --cc=devel@driverdev.osuosl.org \
    --cc=dmaengine@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=micchen@altera.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

dmaengine Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dmaengine/0 dmaengine/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dmaengine dmaengine/ https://lore.kernel.org/dmaengine \
		dmaengine@vger.kernel.org dmaengine@archiver.kernel.org
	public-inbox-index dmaengine

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.dmaengine


AGPL code for this site: git clone https://public-inbox.org/ public-inbox