linux-kernel - Re: [PATCH] spi: spi-qpic-snand: use correct CW_PER

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: <ee95cbc8-c1e6-4b23-9e1d-4a74ef441adc@oss.qualcomm.com>
Date: Mon, 4 Aug 2025 14:48:27 +0200
From: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
To: Gabor Juhos <j4g8y7@...il.com>, Mark Brown <broonie@...nel.org>,
        Md Sadre Alam <quic_mdalam@...cinc.com>,
        Varadarajan Narayanan <quic_varada@...cinc.com>,
        Sricharan Ramabadhran <quic_srichara@...cinc.com>
Cc: linux-spi@...r.kernel.org, linux-mtd@...ts.infradead.org,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] spi: spi-qpic-snand: use correct CW_PER_PAGE value for
 OOB write

On 8/4/25 9:40 AM, Gabor Juhos wrote:
> Hi Konrad,
> 
> 2025. 08. 01. 13:08 keltezéssel, Konrad Dybcio írta:
> 
> ...
> 
>>> diff --git a/drivers/spi/spi-qpic-snand.c b/drivers/spi/spi-qpic-snand.c
>>> index 0cfa0d960fd3c245c2bbf4f5e02d0fc0b13e7696..5216d60e01aab26f927baaea24296571a77527cb 100644
>>> --- a/drivers/spi/spi-qpic-snand.c
>>> +++ b/drivers/spi/spi-qpic-snand.c
>>> @@ -1196,7 +1196,7 @@ static int qcom_spi_program_oob(struct qcom_nand_controller *snandc,
>>>  	u32 cfg0, cfg1, ecc_bch_cfg, ecc_buf_cfg;
>>>  
>>>  	cfg0 = (ecc_cfg->cfg0 & ~CW_PER_PAGE_MASK) |
>>> -	       FIELD_PREP(CW_PER_PAGE_MASK, num_cw - 1);
>>> +	       FIELD_PREP(CW_PER_PAGE_MASK, 0);
>>
>> FWIW I'm just a fly-by reviewer for this driver, but the docs say:
>>
>> The value is the number of codewords per page minus one
> 
> Well, the driver uses that differently even without the patch. See below.
> 
>> "NOTE: This field must be cleared for block erase operation"
> 
>     $ git grep -hp 'FIELD_PREP(CW_PER_PAGE_MASK,.*;' drivers/spi/spi-qpic-snand.c
>     static int qcom_spi_block_erase(struct qcom_nand_controller *snandc)
>                                              FIELD_PREP(CW_PER_PAGE_MASK, 0));
>   
> This function implements the block erase operation and it corresponds to the
> documentation. So far so good.
> 
>     static int qcom_spi_read_last_cw(struct qcom_nand_controller *snandc,
>                    FIELD_PREP(CW_PER_PAGE_MASK, 0);
>     static int qcom_spi_read_cw_raw(struct qcom_nand_controller *snandc, u8 *data_buf,
>                    FIELD_PREP(CW_PER_PAGE_MASK, 0);
>   
> 
> These two functions are using a single codeword (with zero CW_PER_PAGE value).
> So, it seems that in reality the CW_PER_PAGE value means the number of codewords
> (minus one) used within a single operation executed. Of course it is possible
> that the existing code is wrong here.
> 
>     static int qcom_spi_read_page_ecc(struct qcom_nand_controller *snandc,
>                    FIELD_PREP(CW_PER_PAGE_MASK, num_cw - 1);
>     static int qcom_spi_read_page_oob(struct qcom_nand_controller *snandc,
>                    FIELD_PREP(CW_PER_PAGE_MASK, num_cw - 1);
>     static int qcom_spi_program_raw(struct qcom_nand_controller *snandc,
>                    FIELD_PREP(CW_PER_PAGE_MASK, num_cw - 1);
>     static int qcom_spi_program_ecc(struct qcom_nand_controller *snandc,
>                    FIELD_PREP(CW_PER_PAGE_MASK, num_cw - 1);
> 
> 
> The previous functions are operating on whole pages, so those are using all codewords
> within a page thus 'num_cw - 1' is getting set in the register field. This also matches
> with the documentation.
> 
>     static int qcom_spi_program_oob(struct qcom_nand_controller *snandc,
>                    FIELD_PREP(CW_PER_PAGE_MASK, num_cw - 1);
> 
> This is the function fixed by the patch. As it is indicated in the commit description
> this also uses a single codeword similarly to the qcom_spi_read_(last_cw,cw_raw) functions
> described above so the CW_PER_PAGE value should be set to zero.

I didn't mean to dispute what you said :)
Simply included some context for other reviewers

But thanks for the insight, this seems to indeed make sense
the way you presented it

Konrad