linux-kernel - Re: [PATCH v6 00/18] mtd: nand: denali: Denali NAND IP patch bomb

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: <CAK7LNAQr+pxE_orE5m1+9v3aFjMJmWMy+rWCecNk_RsVdhezoA@mail.gmail.com>
Date:   Tue, 13 Jun 2017 17:04:11 +0900
From:   Masahiro Yamada <yamada.masahiro@...ionext.com>
To:     Boris Brezillon <boris.brezillon@...e-electrons.com>
Cc:     Cyrille Pitchen <cyrille.pitchen@...ev4u.fr>,
        Richard Weinberger <richard@....at>,
        Marek Vasut <marek.vasut@...il.com>,
        David Woodhouse <dwmw2@...radead.org>,
        Chuanxiao Dong <chuanxiao.dong@...el.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Dinh Nguyen <dinguyen@...nel.org>,
        linux-mtd@...ts.infradead.org,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Artem Bityutskiy <artem.bityutskiy@...ux.intel.com>,
        Jassi Brar <jaswinder.singh@...aro.org>,
        Brian Norris <computersforpeace@...il.com>,
        Enrico Jorns <ejo@...gutronix.de>
Subject: Re: [PATCH v6 00/18] mtd: nand: denali: Denali NAND IP patch bomb

Hi Boris,


2017-06-13 16:02 GMT+09:00 Boris Brezillon <boris.brezillon@...e-electrons.com>:
> Le Tue, 13 Jun 2017 14:03:52 +0900,
> Masahiro Yamada <yamada.masahiro@...ionext.com> a écrit :
>
>> This patch series intends to solve various problems.
>>
>> [1] The driver just retrieves the OOB area as-is
>>     whereas the controller uses syndrome page layout.
>> [2] ONFi devices are not working
>> [3] It can not read Bad Block Marker
>>
>> Outstanding changes are:
>>  - Fix raw/oob callbacks for syndrome page layout
>>  - Implement setup_data_interface() callback
>>  - Fix/implement more commands for ONFi devices
>>  - Allow to skip the driver internal bounce buffer
>>  - Support PIO in case DMA is not supported
>>  - Switch from ->cmdfunc over to ->cmd_ctrl
>>
>> 18 patches were merged by v2.
>> 11 patches were merged by v3.
>> 2 patches were merged by v4.
>> 5 patches were merged by v5.
>> Here is the rest of the series.
>>
>> v1: https://lkml.org/lkml/2016/11/26/144
>> v2: https://lkml.org/lkml/2017/3/22/804
>> v3: https://lkml.org/lkml/2017/3/30/90
>> v4: https://lkml.org/lkml/2017/6/5/1005
>>
>> Masahiro Yamada (18):
>>   mtd: nand: denali: set NAND_ECC_CUSTOM_PAGE_ACCESS
>>   mtd: nand: denali: remove unneeded find_valid_banks()
>>   mtd: nand: denali: handle timing parameters by setup_data_interface()
>>   mtd: nand: denali: rework interrupt handling
>>   mtd: nand: denali: fix NAND_CMD_STATUS handling
>>   mtd: nand: denali: fix NAND_CMD_PARAM handling
>
> AFAICT, patch 5 and 6 are unneeded...
>
>>   mtd: nand: denali: switch over to cmd_ctrl instead of cmdfunc
>
> ... because you're anyway switching to ->cmd_ctrl() in patch 7, which
> fixes the problem you were addressing in patch 5 and 6.
>
> Please squash those 3 patches into a single one and adjust your commit
> message accordingly (explaining that it fixes STATUS and PARAM handling).
> See below if you need an example.

Squashing 3 patches is OK, but I did not get your additional implementation.


> BTW, I also implemented ->read/write_buf_word() since the core may one
> day call these functions, and the default implementations used by the
> core when these hooks are NULL are not appropriate in your case.

I implemented

denali_read_buf()
denali_write_buf()
denali_read_buf16()
denali_write_buf16()

in 13/18 in a bit different way:
http://patchwork.ozlabs.org/patch/774961/


If you like, I can modify 13/18 so that
denali_read/write_byte() is implemented based on denali_read/write_buf().





> --->8---
> From 136727ba7b453ca1567c711037230aa6ec0f124a Mon Sep 17 00:00:00 2001
> From: Masahiro Yamada <yamada.masahiro@...ionext.com>
> Date: Tue, 13 Jun 2017 14:03:57 +0900
> Subject: [PATCH] mtd: nand: denali: switch over to cmd_ctrl instead of cmdfunc
>
> The NAND_CMD_SET_FEATURES support is missing from denali_cmdfunc().
>
> Besides, we see /* TODO: Read OOB data */ comment line.
>
> It would be possible to add more commands along with the current
> implementation, but having ->cmd_ctrl() seems a better approach from
> the discussion with Boris [1].
>
> Rely on the default ->cmdfunc() from the framework and implement the
> driver's own ->cmd_ctrl(). We are also implementing
> ->read/write_buf/byte/word().
>
> Also add ->write_byte(), which is needed for write direction commands.
>
> Then, we can drop nand_onfi_get_set_features_notsupp from this driver.
>
> This transition also fixes NAND_CMD_STATUS and NAND_CMD_PARAM handling.
> NAND_CMD_STATUS was just faked by the implementation, and the only valid
> bit returned in this case was the WP bit.
> NAND_CMD_PARAM was completely broken: not only the command sent on the
> bus was NAND_CMD_STATUS instead of NAND_CMD_PARAM, but the driver was
> only reading 8 bytes of data, while the parameter page is contains
> several hundreds of bytes.

Probably "... page contains" instead of "... page is contains".





-- 
Best Regards
Masahiro Yamada