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
