linux-kernel - Re: [PATCH 2/2] spi: spi-geni-qcom: Do not do DMA map/unmap inside driver, use framework instead

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]

Date:   Wed, 17 May 2023 17:46:51 +0530
From:   Vijaya Krishna Nivarthi <quic_vnivarth@...cinc.com>
To:     Doug Anderson <dianders@...omium.org>
CC:     <agross@...nel.org>, <andersson@...nel.org>,
        <konrad.dybcio@...aro.org>, <broonie@...nel.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-spi@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <quic_msavaliy@...cinc.com>,
        <mka@...omium.org>, <swboyd@...omium.org>,
        <quic_vtanuku@...cinc.com>, <quic_ptalari@...cinc.com>
Subject: Re: [PATCH 2/2] spi: spi-geni-qcom: Do not do DMA map/unmap inside
 driver, use framework instead

Hi,

Thank you very much for the review...


On 5/15/2023 9:43 PM, Doug Anderson wrote:
> Hi,
>
> On Fri, May 12, 2023 at 10:07 AM Vijaya Krishna Nivarthi
> <quic_vnivarth@...cinc.com> wrote:
>> @@ -836,35 +858,24 @@ static int setup_se_xfer(struct spi_transfer *xfer,
>>          geni_se_setup_m_cmd(se, m_cmd, FRAGMENTATION);
>>
>>          if (mas->cur_xfer_mode == GENI_SE_DMA) {
>> +               dma_addr_t dma_ptr_sg;
>> +               unsigned int dma_len_sg;
>> +
>>                  if (m_cmd & SPI_RX_ONLY) {
>> -                       ret =  geni_se_rx_dma_prep(se, xfer->rx_buf,
>> -                               xfer->len, &mas->rx_se_dma);
>> -                       if (ret) {
>> -                               dev_err(mas->dev, "Failed to setup Rx dma %d\n", ret);
>> -                               mas->rx_se_dma = 0;
>> -                               goto unlock_and_return;
>> -                       }
>> +                       dma_ptr_sg = sg_dma_address(xfer->rx_sg.sgl);
>> +                       dma_len_sg = sg_dma_len(xfer->rx_sg.sgl);
>> +                       geni_se_rx_init_dma(se, &dma_ptr_sg, dma_len_sg);
> nit: probably don't need local variables if you change patch set #1
> like I suggested and don't pass in a pointer for the iova.
>
Done.

> One last question: should you call:
>
> dma_set_max_seg_size(dev, INT_MAX)
>
> ...in your probe function? I don't think you have any limitations of
> maximum segment size, right? Right now if you don't set anything it
> looks as if it considers your max to be 64K. That would cause the SPI
> framework to break things up into multiple chunks which would make you
> fall back to FIFO mode, right?


Actually we would need to call:

dma_set_max_seg_size(dev->parent, INT_MAX)

Please note that in probe()

spi->dma_map_dev = dev->parent;

and in __spi_map_msg()

tx_dev = ctlr->dma_map_dev;

ret = spi_map_buf(ctlr, tx_dev, &xfer->tx_sg,...


Since the dev->parent is QUP containing other SEs and its max_seg_size 
seems to be getting set from elsewhere than code (perhaps kernel 
scripts) it seemed safer not to modify that.

So I made below change and uploaded v2...

spi->max_dma_len = 0xffff0; /* 24 bits for tx/rx dma length */

which actually doesnt help much because spi_map_buf() picks the lower of 
these 2 but added it anyway

desc_len = min_t(size_t, max_seg_size, ctlr->max_dma_len);


Any case as next step we will look into scatter list support to DMA; and 
practically we may not have transfers over 64KB, so its ok for now?

Thank you,

Vijay/


>
> Other than that this looks good to me.
>
> -Doug