| 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
| ||
|
Message-ID: <09a03767-1bd5-c398-7894-5fd1392d1fb8@quicinc.com>
Date: Wed, 27 Nov 2024 14:35:11 +0530
From: Md Sadre Alam <quic_mdalam@...cinc.com>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
CC: <miquel.raynal@...tlin.com>, <richard@....at>, <vigneshr@...com>,
<bbrezillon@...nel.org>, <linux-mtd@...ts.infradead.org>,
<linux-arm-msm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<quic_srichara@...cinc.com>, <quic_varada@...cinc.com>,
<quic_nainmeht@...cinc.com>, <quic_laksd@...cinc.com>
Subject: Re: [PATCH v2 1/3] mtd: rawnand: qcom: Pass 18 bit offset from QPIC
base address to BAM
On 11/26/2024 11:00 AM, Manivannan Sadhasivam wrote:
> On Fri, Nov 22, 2024 at 02:29:31PM +0530, Md Sadre Alam wrote:
>> Currently we are configuring lower 24 bits of address in descriptor
>> whereas QPIC design expects 18 bit register offset from QPIC base
>> address to be configured in cmd descriptors. This is leading to a
>> different address actually being used in HW, leading to wrong value
>> read.
>>
>> the actual issue is that the NANDc base address is different from the
>> QPIC base address. But the driver doesn't take it into account and just
>> used the QPIC base as the NANDc base. This used to work as the NANDc IP
>> only considers the lower 18 bits of the address passed by the driver to
>> derive the register offset. Since the base address of QPIC used to contain
>> all 0 for lower 18 bits (like 0x07980000), the driver ended up passing the
>
> SDX55's NANDc base is 0x01b30000 and it has bits 17 and 18 set corresponding to
> 0x30000. So it is correct that the IP only considers lower 18 bits and it used
> to work as the driver ended up passing 0x3000 + register offset.
This address 0x30000 is the address from QPIC_BASE to QPIC_EBI2NAND
e.g for SDX55 and SDX65 the QPIC_BASE is 0x01B00000. So here lower 18-bits
are zero only.
>
> Your wording is not correct.
Ok, will fix in next revision.
>
>> actual register offset in it and NANDc worked properly. But on newer SoCs
>> like SDX75, the QPIC base address doesn't contain all 0 for lower 18 bits
>> (like 0x01C98000). So NANDc sees wrong offset as per the current logic
>>
>
> 'all 0 for lower 18 bits' is not true.
The lower 18 bits zero we have to see in QPIC_BASE not in EBI2NAND_BASE.
e.g SDX55 & SDX65 the QPIC_BASE = 0x01B00000, all lower 18 bits are zero
but in SDX75 the QPIC_BASE = 0x01C98000, all lower 18 bits are non-zero.
>
>> Older targets also used same configuration (lower 24 bits) like SDX55,
>> SDX65, IPQ8074, IPQ6018 etc. but issue is masked in older targets due
>> to lower 18 bits of QPIC base address being zero leading to expected
>> address generation.
>>
>
> This paragraph is redundant now.
Ok, will remove in next revision.
>
>> The address should be passed to BAM 0x30000 + offset. In older targets
>> the lower 18-bits are zero so that correct address being paased. But
>> in newer targets the lower 18-bits are non-zero in QPIC base so that
>> 0x300000 + offset giving the wrong value.
>>
>> SDX75 : QPIC_QPIC | 0x01C98000 (Lower 18 bits are non zero)
>> SDX55 : QPIC_QPIC | 0x07980000 (Lower 18 bits are zero) Same for
>
> This address is wrong as I mentioned above.
Ok, will fix in next revision.
>
>> older targets.
>>
>> Cc: stable@...r.kernel.org
>> Fixes: 8d6b6d7e135e ("mtd: nand: qcom: support for command descriptor formation")
>> Signed-off-by: Md Sadre Alam <quic_mdalam@...cinc.com>
>> ---
>>
>> Change in [v2]
>>
>> * Updated commit message
>>
>> * Added Fixes tag
>>
>> * Added stable kernel tag
>>
>> * Renamed the variable from offset_from_qpic to nandc_offset
>>
>> Change in [v1]
>>
>> * Preliminary correction for the register address forwarded to BAM
>>
>> drivers/mtd/nand/raw/qcom_nandc.c | 9 +++++++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
>> index b8cff9240b28..cc59461df72e 100644
>> --- a/drivers/mtd/nand/raw/qcom_nandc.c
>> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
>> @@ -207,7 +207,7 @@ nandc_set_reg(chip, reg, \
>> #define dev_cmd_reg_addr(nandc, reg) ((nandc)->props->dev_cmd_reg_start + (reg))
>>
>> /* Returns the NAND register physical address */
>> -#define nandc_reg_phys(chip, offset) ((chip)->base_phys + (offset))
>> +#define nandc_reg_phys(chip, offset) ((nandc)->props->nandc_offset + (offset))
>>
>> /* Returns the dma address for reg read buffer */
>> #define reg_buf_dma_addr(chip, vaddr) \
>> @@ -561,6 +561,7 @@ struct qcom_nandc_props {
>> bool is_qpic;
>> bool qpic_v2;
>> bool use_codeword_fixup;
>> + u32 nandc_offset;
>> };
>>
>> /* Frees the BAM transaction memory */
>> @@ -3477,6 +3478,7 @@ static const struct qcom_nandc_props ipq806x_nandc_props = {
>> .is_bam = false,
>> .use_codeword_fixup = true,
>> .dev_cmd_reg_start = 0x0,
>> + .nandc_offset = 0x30000,
>> };
>>
>> static const struct qcom_nandc_props ipq4019_nandc_props = {
>> @@ -3484,6 +3486,7 @@ static const struct qcom_nandc_props ipq4019_nandc_props = {
>> .is_bam = true,
>> .is_qpic = true,
>> .dev_cmd_reg_start = 0x0,
>> + .nandc_offset = 0x30000,
>> };
>>
>> static const struct qcom_nandc_props ipq8074_nandc_props = {
>> @@ -3491,6 +3494,7 @@ static const struct qcom_nandc_props ipq8074_nandc_props = {
>> .is_bam = true,
>> .is_qpic = true,
>> .dev_cmd_reg_start = 0x7000,
>> + .nandc_offset = 0x30000,
>> };
>>
>> static const struct qcom_nandc_props sdx55_nandc_props = {
>> @@ -3498,7 +3502,8 @@ static const struct qcom_nandc_props sdx55_nandc_props = {
>> .is_bam = true,
>> .is_qpic = true,
>> .qpic_v2 = true,
>> - .dev_cmd_reg_start = 0x7000,
>> + .dev_cmd_reg_start = 0x0,
>
> What is this change?
Sorry, by mistake this get modified. Will fix in next revision.
>
> - Mani
>
Powered by blists - more mailing lists