Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <03b86107-e03f-b41e-09e8-c8509c307005@sberdevices.ru>
Date:   Fri, 9 Jun 2023 13:55:07 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH v2] mtd: rawnand: meson: check buffer length
Content-Language: en-US
To:     Liang Yang <liang.yang@amlogic.com>,
        Miquel Raynal <miquel.raynal@bootlin.com>
CC:     Richard Weinberger <richard@nod.at>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Neil Armstrong <neil.armstrong@linaro.org>,
        Kevin Hilman <khilman@baylibre.com>,
        Jerome Brunet <jbrunet@baylibre.com>,
        Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
        <oxffffaa@gmail.com>, <kernel@sberdevices.ru>,
        <linux-mtd@lists.infradead.org>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-amlogic@lists.infradead.org>, <linux-kernel@vger.kernel.org>
References: <20230606101644.3297859-1-AVKrasnov@sberdevices.ru>
 <9adef6da-5930-dcaa-f148-e4a398d39f2d@sberdevices.ru>
 <3a9470ed-d7ad-6cae-0d58-732399590272@sberdevices.ru>
 <7903580d-685c-14e6-5572-81a4cb31cced@amlogic.com>
 <20230608085458.449a24c0@xps-13>
 <4e6cd8a8-f618-4bc3-5fa9-eab2b501dd89@amlogic.com>
 <a6a20b50-02b0-53c0-1e65-75035bc59c87@amlogic.com>
 <20230608152114.3a1d82ac@xps-13>
 <3cc9765c-f32a-fe8a-f7af-fb884ee63b48@amlogic.com>
From:   Arseniy Krasnov <avkrasnov@sberdevices.ru>
In-Reply-To: <3cc9765c-f32a-fe8a-f7af-fb884ee63b48@amlogic.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 09.06.2023 10:59, Liang Yang wrote:
> Hi Miquel,
> 
> On 2023/6/8 21:21, Miquel Raynal wrote:
>> [ EXTERNAL EMAIL ]
>>
>> Hi Liang,
>>
>> liang.yang@amlogic.com wrote on Thu, 8 Jun 2023 20:37:14 +0800:
>>
>>> Hi Arseniy and Miquel,
>>>
>>> On 2023/6/8 15:12, Liang Yang wrote:
>>>> Hi Miquel,
>>>>
>>>> On 2023/6/8 14:54, Miquel Raynal wrote:
>>>>> [ EXTERNAL EMAIL ]
>>>>>
>>>>> Hi Liang,
>>>>>
>>>>> liang.yang@amlogic.com wrote on Thu, 8 Jun 2023 14:42:53 +0800:
>>>>>
>>>>>> Hi Arseniy,
>>>>>>
>>>>>> On 2023/6/8 5:17, Arseniy Krasnov wrote:
>>>>>>> [ EXTERNAL EMAIL ]
>>>>>>>
>>>>>>> Hi again Miquel, Liang!
>>>>>>>
>>>>>>> What do You think about this patch?
>>>>>>>
>>>>>>> Thanks, Arseniy
>>>>>>>
>>>>>>> On 06.06.2023 19:29, Arseniy Krasnov wrote:
>>>>>>>> Sorry, here is changelog:
>>>>>>>> v1 -> v2:
>>>>>>>> * Move checks from 'switch/case' which executes commands in >>>>> 'meson_nfc_exec_op()' to a special
>>>>>>>>      separated function 'meson_nfc_check_op()' which is called >>>>> before commands processing.
>>>>>>>>
>>>>>>>> On 06.06.2023 13:16, Arseniy Krasnov wrote:
>>>>>>>>> Meson NAND controller has limited buffer length, so check it before
>>>>>>>>> command execution to avoid length trim. Also check MTD write size on
>>>>>>>>> chip attach.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>>>>>>>>> ---
>>>>>>>>>     drivers/mtd/nand/raw/meson_nand.c | 47 >>>>>> +++++++++++++++++++++++++++----
>>>>>>>>>     1 file changed, 42 insertions(+), 5 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/mtd/nand/raw/meson_nand.c >>>>>> b/drivers/mtd/nand/raw/meson_nand.c
>>>>>>>>> index 23a73268421b..db6b18753071 100644
>>>>>>>>> --- a/drivers/mtd/nand/raw/meson_nand.c
>>>>>>>>> +++ b/drivers/mtd/nand/raw/meson_nand.c
>>>>>>>>> @@ -111,6 +111,8 @@
>>>>>>>>>
>>>>>>>>>     #define PER_INFO_BYTE               8
>>>>>>>>>
>>>>>>>>> +#define NFC_CMD_RAW_LEN     GENMASK(13, 0)
>>>>>>>>> +
>>>>>>>>>     struct meson_nfc_nand_chip {
>>>>>>>>>         struct list_head node;
>>>>>>>>>         struct nand_chip nand;
>>>>>>>>> @@ -284,7 +286,7 @@ static void meson_nfc_cmd_access(struct >>>>>> nand_chip *nand, int raw, bool dir,
>>>>>>>>>
>>>>>>>>>         if (raw) {
>>>>>>>>>                 len = mtd->writesize + mtd->oobsize;
>>>>>>>>> -            cmd = (len & GENMASK(13, 0)) | scrambler | DMA_DIR(dir);
>>>>>>>>> +            cmd = len | scrambler | DMA_DIR(dir);
>>>>>>>>>                 writel(cmd, nfc->reg_base + NFC_REG_CMD);
>>>>>>
>>>>>> I think we could keep "& GENMASK(13, 0)". it is better here to >>> indicate how many bits of length in this command and don't destory >>> the command once we don't check the "len" outside of this function. >>> or the code "if (len >  NFC_CMD_RAW_LEN)" is better to put inside >>> this function nearly. Thanks.
>>>>>
>>>>> It depends who calls this helper. If only used inside >> exec_op,write_page_raw() and read_page_raw() also call >> meson_nfc_cmd_access(), > but i don't find where constrain the "len".
>>>
>>> Is the helper "meson_nfc_check_op()" needed by exec_op() after the constraint in attach_chip()? the length passed in exec_op() seems smaller than "mtd->writesize + mtd->oobsize" now.
>>
>> exec_op() is primarily dedicated to performing side commands than page
>> accesses, typically the parameter page is X bytes, you might want to
>> read it 3 times, depending on the capabilities of the controller, you
>> might need to split the operation or not, so the core checks what are
>> the controller capabilities before doing the operation. So yes, the
>> check_op() thing is necessary.
> 
> ok, i get it. thanks
> 
> @Arseniy I have no other questions about this patch.

Got it!

Thanks, Arseniy

>>
>>>
>>> @Arseniy if it does need, I think the same constraint is needed by
>>> "meson_nfc_cmd_access()"
>>>
>>>>
>>>>> it's not useful. If used outside, then if you want to clarify
>>>>> things you may want to use:
>>>>>
>>>>> #define NFC_CMD_OP_LEN(cmd) FIELD_PREP(GENMASK(13, 0), (cmd))
>>>>>
>>>>> This is by far my favorite construction. Could apply to many other
>>>>> fields, like DMA_DIR, scrambler, etc.
>>>
>>> @Miquel, FIELD_PREP() is better, but i have no idea whether we should add FIELD_PREP() in this patch, or keep the previous code.
>>>
>>>>>
>>>>>>>>>                 return;
>>>>>>>>>         }
>>>>>>>>> @@ -573,7 +575,7 @@ static int meson_nfc_read_buf(struct nand_chip >>>>>> *nand, u8 *buf, int len)
>>>>>>>>>         if (ret)
>>>>>>>>>                 goto out;
>>>>>>>>>
>>>>>>>>> -    cmd = NFC_CMD_N2M | (len & GENMASK(13, 0));
>>>>>>>>> +    cmd = NFC_CMD_N2M | len;
>>>>>>>>>         writel(cmd, nfc->reg_base + NFC_REG_CMD);
>>>>>>>>>
>>>>>>>>>         meson_nfc_drain_cmd(nfc);
>>>>>>>>> @@ -597,7 +599,7 @@ static int meson_nfc_write_buf(struct >>>>>> nand_chip *nand, u8 *buf, int len)
>>>>>>>>>         if (ret)
>>>>>>>>>                 return ret;
>>>>>>>>>
>>>>>>>>> -    cmd = NFC_CMD_M2N | (len & GENMASK(13, 0));
>>>>>>>>> +    cmd = NFC_CMD_M2N | len;
>>>>>>>>>         writel(cmd, nfc->reg_base + NFC_REG_CMD);
>>>>>>>>>
>>>>>>>>>         meson_nfc_drain_cmd(nfc);
>>>>>>>>> @@ -1007,6 +1009,31 @@ meson_nand_op_put_dma_safe_output_buf(const >>>>>> struct nand_op_instr *instr,
>>>>>>>>>                 kfree(buf);
>>>>>>>>>     }
>>>>>>>>>
>>>>>>>>> +static int meson_nfc_check_op(struct nand_chip *chip,
>>>>>>>>> +                          const struct nand_operation *op)
>>>>>>>>> +{
>>>>>>>>> +    int op_id;
>>>>>>>>> +
>>>>>>>>> +    for (op_id = 0; op_id < op->ninstrs; op_id++) {
>>>>>>>>> +            const struct nand_op_instr *instr;
>>>>>>>>> +
>>>>>>>>> +            instr = &op->instrs[op_id];
>>>>>>>>> +
>>>>>>>>> +            switch (instr->type) {
>>>>>>>>> +            case NAND_OP_DATA_IN_INSTR:
>>>>>>>>> +            case NAND_OP_DATA_OUT_INSTR:
>>>>>>>>> +                    if (instr->ctx.data.len > NFC_CMD_RAW_LEN)
>>>>>>>>> +                            return -ENOTSUPP;
>>>>>>>>> +
>>>>>>>>> +                    break;
>>>>>>>>> +            default:
>>>>>>>>> +                    break;
>>>>>>>>> +            }
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>> +    return 0;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>>     static int meson_nfc_exec_op(struct nand_chip *nand,
>>>>>>>>>                              const struct nand_operation *op, bool >>>>>> check_only)
>>>>>>>>>     {
>>>>>>>>> @@ -1015,10 +1042,12 @@ static int meson_nfc_exec_op(struct >>>>>> nand_chip *nand,
>>>>>>>>>         const struct nand_op_instr *instr = NULL;
>>>>>>>>>         void *buf;
>>>>>>>>>         u32 op_id, delay_idle, cmd;
>>>>>>>>> +    int err;
>>>>>>>>>         int i;
>>>>>>>>>
>>>>>>>>> -    if (check_only)
>>>>>>>>> -            return 0;
>>>>>>>>> +    err = meson_nfc_check_op(nand, op);
>>>>>>>>> +    if (err || check_only)
>>>>>>>>> +            return err;
>>>>>>>>>
>>>>>>>>>         meson_nfc_select_chip(nand, op->cs);
>>>>>>>>>         for (op_id = 0; op_id < op->ninstrs; op_id++) {
>>>>>>>>> @@ -1293,6 +1322,7 @@ static int meson_nand_attach_chip(struct >>>>>> nand_chip *nand)
>>>>>>>>>         struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
>>>>>>>>>         struct mtd_info *mtd = nand_to_mtd(nand);
>>>>>>>>>         int nsectors = mtd->writesize / 1024;
>>>>>>>>> +    int raw_writesize;
>>>>>>>>>         int ret;
>>>>>>>>>
>>>>>>>>>         if (!mtd->name) {
>>>>>>>>> @@ -1304,6 +1334,13 @@ static int meson_nand_attach_chip(struct >>>>>> nand_chip *nand)
>>>>>>>>>                         return -ENOMEM;
>>>>>>>>>         }
>>>>>>>>>
>>>>>>>>> +    raw_writesize = mtd->writesize + mtd->oobsize;
>>>>>>>>> +    if (raw_writesize > NFC_CMD_RAW_LEN) {
>>>>>>>>> +            dev_err(nfc->dev, "too big write size in raw mode: %d >>>>>> > %ld\n",
>>>>>>>>> +                    raw_writesize, NFC_CMD_RAW_LEN);
>>>>>>>>> +            return -EINVAL;
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>>         if (nand->bbt_options & NAND_BBT_USE_FLASH)
>>>>>>>>>                 nand->bbt_options |= NAND_BBT_NO_OOB;
>>>>>>>>>
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Miquèl
>>
>>
>> Thanks,
>> Miquèl