linux-kernel - Re: [PATCH v4 5/5] spi: spi-qcom-qspi: Add DMA mode 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: <7a6fe89b-5898-08d3-6c44-2cfc9d8fae7a@quicinc.com>
Date:   Fri, 21 Apr 2023 22:27:49 +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>,
        <robh+dt@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>,
        <cros-qcom-dts-watchers@...omium.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-spi@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <quic_msavaliy@...cinc.com>, <mka@...omium.org>,
        <swboyd@...omium.org>, <quic_vtanuku@...cinc.com>
Subject: Re: [PATCH v4 5/5] spi: spi-qcom-qspi: Add DMA mode support

Hi,

Thanks a lot for the review and inputs...


On 4/20/2023 10:49 PM, Doug Anderson wrote:
> Hi,
>
> On Thu, Apr 20, 2023 at 6:13 AM Vijaya Krishna Nivarthi
> <quic_vnivarth@...cinc.com> wrote:
>> @@ -137,11 +155,29 @@ enum qspi_clocks {
>>          QSPI_NUM_CLKS
>>   };
>>
>> +enum qspi_xfer_mode {
>> +       QSPI_FIFO,
>> +       QSPI_DMA
>> +};
>> +
>> +/*
>> + * Number of entries in sgt returned from spi framework that-
>> + * will be supported. Can be modified as required.
>> + * In practice, given max_dma_len is 64KB, the number of
>> + * entries is not expected to exceed 1.
>> + */
>> +#define QSPI_MAX_SG 5
> I actually wonder if this would be more nicely done just using a
> linked list, which naturally mirrors how SGs work anyway. You'd add
> "struct list_head" to the end of "struct qspi_cmd_desc" and just store
> a pointer to the head in "struct qcom_qspi".
>
> For freeing, you can always get back the "virtual" address because
> it's just the address of each node. You can always get back the
> physical address because it's stored in "data_address".
>
Please note that in "struct qspi_cmd_desc"

data_address - dma_address of data buffer returned by spi framework

next_descriptor - dma_address of the next descriptor in chain


If we were to have a linked list of descriptors that we can parse and 
free, it would require 2 more fields

this_descriptor_dma - dma address of the current descriptor

next_descriptor_virt - virtual address of the next descriptor in chain


I tried adding same and it seemed like it was getting a little confusing.

Given the SGL is also an array in SGT, it seemed ok to retain an array, 
though it would have good to have a chain of cmd_descriptors in SW like 
in HW.

Agreed the fixed size array comes at a cost of artificial limitation of 
5 entries, which anyway seems like something we are not going to 
encounter in practice.


So for now, I retained the array.

All the other comments are addressed as recommended, except 1 change below

Please let know what you think.


Thank you,

Vijay/


>
>> +static int qcom_qspi_alloc_desc(struct qcom_qspi *ctrl, dma_addr_t dma_ptr,
>> +                       uint32_t n_bytes)
>> +{
>> +       struct qspi_cmd_desc *virt_cmd_desc, *prev;
>> +       dma_addr_t dma_cmd_desc;
>> +
>> +       /* allocate for dma cmd descriptor */
>> +       virt_cmd_desc = (struct qspi_cmd_desc *)dma_pool_alloc(ctrl->dma_cmd_pool,
>> +               GFP_KERNEL, &dma_cmd_desc);
> Remove unnecessary cast; "void *" assigns fine w/out a cast.
>
> Add "| GFP_ZERO" and then get rid of the need to clear the "reserved"
> and "next_descriptor" stuff below.
>
I needed __GFP_ZERO to prevent a compile error, used same.