| 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
| ||
|
Date: Tue, 15 Feb 2022 21:03:52 +0530
From: Apurva Nandan <a-nandan@...com>
To: Boris Brezillon <boris.brezillon@...labora.com>
CC: Miquel Raynal <miquel.raynal@...tlin.com>,
Richard Weinberger <richard@....at>,
Vignesh Raghavendra <vigneshr@...com>,
Mark Brown <broonie@...nel.org>,
Patrice Chotard <patrice.chotard@...s.st.com>,
Christophe Kerello <christophe.kerello@...s.st.com>,
Daniel Palmer <daniel@...f.com>,
Alexander Lobakin <alobakin@...me>,
<linux-mtd@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
<linux-spi@...r.kernel.org>, <p.yadav@...com>
Subject: Re: [PATCH v3 05/17] mtd: spinand: Define ctrl_ops for non-page
read/write op templates
Hi Boris,
On 03/01/22 15:31, Boris Brezillon wrote:
> On Sat, 1 Jan 2022 13:12:38 +0530
> Apurva Nandan <a-nandan@...com> wrote:
>
>> 'ctrl_ops' are op templates for non-page read/write operations,
>> which are: reset, get_feature, set_feature, write_enable, block_erase,
>> page_read and program_execute ops. The 'ctrl_ops' struct contains in it
>> op templates for each of this op, as well as enum spinand_protocol
>> denoting protocol of all these ops.
>>
>> We require these new op templates because of deviation in standard
>> SPINAND ops by manufacturers and also due to changes when there is a
>> change in SPI protocol/mode. This prevents the core from live-patching
>> and vendor-specific adjustments in ops.
>>
>> Define 'ctrl_ops', add macros to initialize it and add it in
>> spinand_device.
>>
>> Signed-off-by: Apurva Nandan <a-nandan@...com>
>> ---
>> include/linux/mtd/spinand.h | 33 +++++++++++++++++++++++++++++++++
>> 1 file changed, 33 insertions(+)
>>
>> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
>> index 439d8ce40e1d..e5df6220ec1e 100644
>> --- a/include/linux/mtd/spinand.h
>> +++ b/include/linux/mtd/spinand.h
>> @@ -356,6 +356,35 @@ struct spinand_op_variants {
>> sizeof(struct spi_mem_op), \
>> }
>>
>> +struct spinand_ctrl_ops {
>> + const struct {
>> + struct spi_mem_op reset;
>> + struct spi_mem_op get_feature;
>> + struct spi_mem_op set_feature;
>> + struct spi_mem_op write_enable;
>> + struct spi_mem_op block_erase;
>> + struct spi_mem_op page_read;
>> + struct spi_mem_op program_execute;
>> + } ops;
>> + const enum spinand_protocol protocol;
> Do you really need that protocol field?
>
>> +};
>> +
>> +#define SPINAND_CTRL_OPS(__protocol, __reset, __get_feature, __set_feature, \
>> + __write_enable, __block_erase, __page_read, \
>> + __program_execute) \
>> + { \
>> + .ops = { \
>> + .reset = __reset, \
>> + .get_feature = __get_feature, \
>> + .set_feature = __set_feature, \
>> + .write_enable = __write_enable, \
>> + .block_erase = __block_erase, \
>> + .page_read = __page_read, \
>> + .program_execute = __program_execute, \
>> + }, \
>> + .protocol = __protocol, \
>> + }
>> +
>> /**
>> * spinand_ecc_info - description of the on-die ECC implemented by a SPI NAND
>> * chip
>> @@ -468,6 +497,8 @@ struct spinand_dirmap {
>> * @data_ops.read_cache: read cache op template
>> * @data_ops.write_cache: write cache op template
>> * @data_ops.update_cache: update cache op template
>> + * @ctrl_ops: various SPI mem op templates for handling the flash device, i.e.
>> + * non page-read/write ops.
>> * @select_target: select a specific target/die. Usually called before sending
>> * a command addressing a page or an eraseblock embedded in
>> * this die. Only required if your chip exposes several dies
>> @@ -498,6 +529,8 @@ struct spinand_device {
>> const struct spi_mem_op *update_cache;
>> } data_ops;
>>
>> + const struct spinand_ctrl_ops *ctrl_ops;
>> +
> Okay, I had something slightly different in mind. First, I'd put all
> templates in a struct:
>
> struct spinand_op_templates {
> const struct spi_mem_op *read_cache;
> const struct spi_mem_op *write_cache;
> const struct spi_mem_op *update_cache;
> const struct spi_mem_op *reset;
> const struct spi_mem_op *get_feature;
> const struct spi_mem_op *set_feature;
> const struct spi_mem_op *write_enable;
> const struct spi_mem_op *block_erase;
> const struct spi_mem_op *page_load;
> const struct spi_mem_op *program_execute;
> };
>
> Then, at the spinand level, I'd define an array of templates:
>
> enum spinand_protocol {
> SPINAND_1S_1S_1S,
> SPINAND_2S_2S_2S,
> SPINAND_4S_4S_4S,
> SPINAND_8S_8S_8S,
> SPINAND_8D_8D_8D,
> SPINAND_NUM_PROTOCOLS,
> };
>
> struct spinand_device {
> ...
> enum spinand_protocol protocol;
> const struct spinand_op_templates *op_templates[SPINAND_NUM_PROTOCOLS];
> ...
> };
>
> This way, you can easily pick the right set of operations based
> on the protocol/mode you're in:
>
> #define spinand_get_op_template(spinand, opname) \
> ((spinand)->op_templates[(spinand)->protocol]->opname)
>
> static int spinand_read_reg_op(struct spinand_device *spinand, u8 reg, u8 *val)
> {
> struct spi_mem_op op = *spinand_get_op_template(spinand, get_feature);
> int ret;
>
> ...
> }
I find a couple of issues with this method,
1. read_cache, write_cache, update_cache op templates don't fit well
with the other non-data ops, as
these data ops are used to create a dirmap, and that can be done only
once at probe time. Hence, there
is a different mechanism of selecting of data ops and non-data ops.
Hence, this division in the op templates
struct as data_ops and ctrl_ops is required. Currently, the core only
supports using a single protocol for
data ops, chosen at the time of probing.
2. If we use this single op_templates struct, I can't think of any good
way to initialize these in the
manufacturers driver (winbond.c), refer to 17th patch in this series.
Could you please suggest a macro
implementation also for winbond.c with the suggested op_templates struct.
Thanks,
Apurva Nandan
Powered by blists - more mailing lists