linux-kernel - Re: [PATCH v2 1/2] dmaengine: qcom: gpi: Add GPI immediate DMA support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fbpdzrwmlmqhyblchgaq6etmnc5wjd3ierwmtrer5hnwjf7qb3@axgwdegmbs6z>
Date: Fri, 29 Nov 2024 22:05:07 -0600
From: Bjorn Andersson <andersson@...nel.org>
To: Jyothi Kumar Seerapu <quic_jseerapu@...cinc.com>
Cc: Vinod Koul <vkoul@...nel.org>, Mark Brown <broonie@...nel.org>, 
	linux-arm-msm@...r.kernel.org, dmaengine@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-spi@...r.kernel.org, quic_msavaliy@...cinc.com, quic_vtanuku@...cinc.com
Subject: Re: [PATCH v2 1/2] dmaengine: qcom: gpi: Add GPI immediate DMA
 support

On Fri, Nov 29, 2024 at 05:02:22PM +0530, Jyothi Kumar Seerapu wrote:
> On 11/28/2024 8:53 PM, Bjorn Andersson wrote:
> > On Thu, Nov 28, 2024 at 07:03:50PM +0530, Jyothi Kumar Seerapu wrote:
> > > diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
[..]
> > 
> > >   	/* first create config tre if applicable */
> > >   	if (direction == DMA_MEM_TO_DEV && spi->set_config) {
> > > @@ -1763,14 +1767,32 @@ static int gpi_create_spi_tre(struct gchan *chan, struct gpi_desc *desc,
> > >   	tre_idx++;
> > >   	address = sg_dma_address(sgl);
> > > -	tre->dword[0] = lower_32_bits(address);
> > > -	tre->dword[1] = upper_32_bits(address);
> > > +	len = sg_dma_len(sgl);
> > > -	tre->dword[2] = u32_encode_bits(sg_dma_len(sgl), TRE_DMA_LEN);
> > > +	/* Support Immediate dma for write transfers for data length up to 8 bytes */
> > 
> > And what happens if the developer writing the SPI driver forgets to read
> > this comment and sets QCOM_GPI_IMMEDIATE_DMA for a 9 byte transfer?
> In V2 patch, QCOM_GPI_IMMEDIATE_DMA is set based on
> QCOM_GPI_IMMEDIATE_DMA_LEN only.
> 

I assume you mean "patch 2/2". So, what happens if someone refactors the
SPI driver in the future, will they read this comment?

> As per Hardware programming guide, immediate dma support is for up to 8
> bytes only.
> Need to check what is the behavior if we want to handle 9 bytes using
> immediate dma feature support.
> 

I'm saying that you have a comment here which says that the caller must
not pass len > 8. Write that comment in code to avoid mistakes - either
now or in the future.

> > 
> > > +	if ((spi->flags & QCOM_GPI_IMMEDIATE_DMA) && direction == DMA_MEM_TO_DEV) {
> > 
> > Why is this flag introduced?
> > 
> > If I understand the next patch, all DMA_MEM_TO_DEV transfers of <=
> > QCOM_GPI_IMMEDIATE_DMA_LEN can use the immediate mode, so why not move
> > the condition here?
> > 
> > Also ordering[1].
> > 
> > 	if (direction == DMA_MEM_TO_DEV && len <= 2 * sizeof(tre->dword[0]))
> > 
> > 
> Sure, thanks for the suggestion.
> so, instead using "QCOM_GPI_IMMEDIATE_DMA_LEN" need to use " 2 *
> sizeof(tre->dword[0])" for 8 bytes length check.
> 

Either one works, but I'm guessing that while 8 is the right number the
reason for 8 is that the data is passed in 2 * dword.


The important thing is that you're encoding the length check here, so
that the client can't by mistake trigger immediate mode with > 8 bytes.
As a side effect, you no longer need the QCOM_GPI_IMMEDIATE_DMA flag and
should be able to drop patch 2.

> > [1] Compare "all transfers of length 8 or less, which are mem to device"
> > vs "all transfers which are mem to device, with a length of 8 or less".
> > The bigger "selection criteria" is the direction, then that's fine tuned
> > by the length query.
> > 
> > > +		buf = sg_virt(sgl);
> > 
> > It's a question of style, but I think you could just put the sg_virt()
> > directly in the memcpy() call and avoid the extra variable.
> 
> Okay, i will directly put sg_virt() in memcpy().

Try it out, pick the option that look the best.

Regards,
Bjorn