| Message ID | 20230716144612.32132-8-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mtd: rawnand: qcom: Misc fixes | expand |
On Sun, Jul 16, 2023 at 04:46:11PM +0200, Miquel Raynal wrote: > Instead of allocating a structure on the stack with random data and then > expect the callee to perform the initialization (which is, in general, > error prone), prefer zeroing the structure explicitly at allocation and > provide the already zeroed area, so no explicit memset operation is > needed. It is probably safer to do so, so we limit the timeframe when > dirty data could actually be accessed by mistake. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> Acked-by: Manivannan Sadhasivam <mani@kernel.org> - Mani > --- > drivers/mtd/nand/raw/qcom_nandc.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) > > diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c > index cb6ccaa19224..4fc8dafa8f03 100644 > --- a/drivers/mtd/nand/raw/qcom_nandc.c > +++ b/drivers/mtd/nand/raw/qcom_nandc.c > @@ -2600,8 +2600,6 @@ static void qcom_parse_instructions(struct nand_chip *chip, > unsigned int op_id; > int i; > > - memset(q_op, 0, sizeof(*q_op)); > - > for (op_id = 0; op_id < subop->ninstrs; op_id++) { > unsigned int offset, naddrs; > const u8 *addrs; > @@ -2681,7 +2679,7 @@ static int qcom_read_status_exec(struct nand_chip *chip, > struct qcom_nand_host *host = to_qcom_nand_host(chip); > struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip); > struct nand_ecc_ctrl *ecc = &chip->ecc; > - struct qcom_op q_op; > + struct qcom_op q_op = {}; > const struct nand_op_instr *instr = NULL; > unsigned int op_id = 0; > unsigned int len = 0; > @@ -2744,7 +2742,7 @@ static int qcom_read_id_type_exec(struct nand_chip *chip, const struct nand_subo > { > struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip); > struct qcom_nand_host *host = to_qcom_nand_host(chip); > - struct qcom_op q_op; > + struct qcom_op q_op = {}; > const struct nand_op_instr *instr = NULL; > unsigned int op_id = 0; > unsigned int len = 0; > @@ -2795,7 +2793,7 @@ static int qcom_misc_cmd_type_exec(struct nand_chip *chip, const struct nand_sub > { > struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip); > struct qcom_nand_host *host = to_qcom_nand_host(chip); > - struct qcom_op q_op; > + struct qcom_op q_op = {}; > int ret = 0; > > qcom_parse_instructions(chip, subop, &q_op); > @@ -2838,7 +2836,7 @@ static int qcom_param_page_type_exec(struct nand_chip *chip, const struct nand_ > { > struct qcom_nand_host *host = to_qcom_nand_host(chip); > struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip); > - struct qcom_op q_op; > + struct qcom_op q_op = {}; > const struct nand_op_instr *instr = NULL; > unsigned int op_id = 0; > unsigned int len = 0; > @@ -2935,7 +2933,7 @@ static int qcom_erase_cmd_type_exec(struct nand_chip *chip, const struct nand_su > { > struct qcom_nand_host *host = to_qcom_nand_host(chip); > struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip); > - struct qcom_op q_op; > + struct qcom_op q_op = {}; > int ret = 0; > > qcom_parse_instructions(chip, subop, &q_op); > -- > 2.34.1 >
On 7/16/23 15:46, Miquel Raynal wrote: > Instead of allocating a structure on the stack with random data and then > expect the callee to perform the initialization (which is, in general, > error prone), prefer zeroing the structure explicitly at allocation and > provide the already zeroed area, so no explicit memset operation is > needed. It is probably safer to do so, so we limit the timeframe when > dirty data could actually be accessed by mistake. Why is zeroed data considered safe or sane?
Hi Tudor, tudor.ambarus@linaro.org wrote on Thu, 27 Jul 2023 16:03:40 +0100: > On 7/16/23 15:46, Miquel Raynal wrote: > > Instead of allocating a structure on the stack with random data and then > > expect the callee to perform the initialization (which is, in general, > > error prone), prefer zeroing the structure explicitly at allocation and > > provide the already zeroed area, so no explicit memset operation is > > needed. It is probably safer to do so, so we limit the timeframe when > > dirty data could actually be accessed by mistake. > > Why is zeroed data considered safe or sane? I believe allocating structures like that on the stack will make their content inherit from previous values used there, which is generally a bad idea if we expect the structure to be zeroed, which is the case here. This structure is meant to be zeroed before being used, so instead of carrying stale data that will be wiped off later, I prefer to have it zeroed earlier. Reducing the time when one could access stale data or write something that will be reset does not sound totally useless to me, in particular given the number of changes this driver has been subject to recently. Thanks, Miquèl
On 7/16/23 15:46, Miquel Raynal wrote: > Instead of allocating a structure on the stack with random data and then > expect the callee to perform the initialization (which is, in general, > error prone), prefer zeroing the structure explicitly at allocation and > provide the already zeroed area, so no explicit memset operation is > needed. It is probably safer to do so, so we limit the timeframe when > dirty data could actually be accessed by mistake. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> Reviewed-by: Tudor Ambarus <tudor.ambarus@linaro.org> > --- > drivers/mtd/nand/raw/qcom_nandc.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) > > diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c > index cb6ccaa19224..4fc8dafa8f03 100644 > --- a/drivers/mtd/nand/raw/qcom_nandc.c > +++ b/drivers/mtd/nand/raw/qcom_nandc.c > @@ -2600,8 +2600,6 @@ static void qcom_parse_instructions(struct nand_chip *chip, > unsigned int op_id; > int i; > > - memset(q_op, 0, sizeof(*q_op)); > - > for (op_id = 0; op_id < subop->ninstrs; op_id++) { > unsigned int offset, naddrs; > const u8 *addrs; > @@ -2681,7 +2679,7 @@ static int qcom_read_status_exec(struct nand_chip *chip, > struct qcom_nand_host *host = to_qcom_nand_host(chip); > struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip); > struct nand_ecc_ctrl *ecc = &chip->ecc; > - struct qcom_op q_op; > + struct qcom_op q_op = {}; > const struct nand_op_instr *instr = NULL; > unsigned int op_id = 0; > unsigned int len = 0; > @@ -2744,7 +2742,7 @@ static int qcom_read_id_type_exec(struct nand_chip *chip, const struct nand_subo > { > struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip); > struct qcom_nand_host *host = to_qcom_nand_host(chip); > - struct qcom_op q_op; > + struct qcom_op q_op = {}; > const struct nand_op_instr *instr = NULL; > unsigned int op_id = 0; > unsigned int len = 0; > @@ -2795,7 +2793,7 @@ static int qcom_misc_cmd_type_exec(struct nand_chip *chip, const struct nand_sub > { > struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip); > struct qcom_nand_host *host = to_qcom_nand_host(chip); > - struct qcom_op q_op; > + struct qcom_op q_op = {}; > int ret = 0; > > qcom_parse_instructions(chip, subop, &q_op); > @@ -2838,7 +2836,7 @@ static int qcom_param_page_type_exec(struct nand_chip *chip, const struct nand_ > { > struct qcom_nand_host *host = to_qcom_nand_host(chip); > struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip); > - struct qcom_op q_op; > + struct qcom_op q_op = {}; > const struct nand_op_instr *instr = NULL; > unsigned int op_id = 0; > unsigned int len = 0; > @@ -2935,7 +2933,7 @@ static int qcom_erase_cmd_type_exec(struct nand_chip *chip, const struct nand_su > { > struct qcom_nand_host *host = to_qcom_nand_host(chip); > struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip); > - struct qcom_op q_op; > + struct qcom_op q_op = {}; > int ret = 0; > > qcom_parse_instructions(chip, subop, &q_op);
On Sun, 2023-07-16 at 14:46:11 UTC, Miquel Raynal wrote: > Instead of allocating a structure on the stack with random data and then > expect the callee to perform the initialization (which is, in general, > error prone), prefer zeroing the structure explicitly at allocation and > provide the already zeroed area, so no explicit memset operation is > needed. It is probably safer to do so, so we limit the timeframe when > dirty data could actually be accessed by mistake. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > Acked-by: Manivannan Sadhasivam <mani@kernel.org> > Reviewed-by: Tudor Ambarus <tudor.ambarus@linaro.org> Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next. Miquel
diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c index cb6ccaa19224..4fc8dafa8f03 100644 --- a/drivers/mtd/nand/raw/qcom_nandc.c +++ b/drivers/mtd/nand/raw/qcom_nandc.c @@ -2600,8 +2600,6 @@ static void qcom_parse_instructions(struct nand_chip *chip, unsigned int op_id; int i; - memset(q_op, 0, sizeof(*q_op)); - for (op_id = 0; op_id < subop->ninstrs; op_id++) { unsigned int offset, naddrs; const u8 *addrs; @@ -2681,7 +2679,7 @@ static int qcom_read_status_exec(struct nand_chip *chip, struct qcom_nand_host *host = to_qcom_nand_host(chip); struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip); struct nand_ecc_ctrl *ecc = &chip->ecc; - struct qcom_op q_op; + struct qcom_op q_op = {}; const struct nand_op_instr *instr = NULL; unsigned int op_id = 0; unsigned int len = 0; @@ -2744,7 +2742,7 @@ static int qcom_read_id_type_exec(struct nand_chip *chip, const struct nand_subo { struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip); struct qcom_nand_host *host = to_qcom_nand_host(chip); - struct qcom_op q_op; + struct qcom_op q_op = {}; const struct nand_op_instr *instr = NULL; unsigned int op_id = 0; unsigned int len = 0; @@ -2795,7 +2793,7 @@ static int qcom_misc_cmd_type_exec(struct nand_chip *chip, const struct nand_sub { struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip); struct qcom_nand_host *host = to_qcom_nand_host(chip); - struct qcom_op q_op; + struct qcom_op q_op = {}; int ret = 0; qcom_parse_instructions(chip, subop, &q_op); @@ -2838,7 +2836,7 @@ static int qcom_param_page_type_exec(struct nand_chip *chip, const struct nand_ { struct qcom_nand_host *host = to_qcom_nand_host(chip); struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip); - struct qcom_op q_op; + struct qcom_op q_op = {}; const struct nand_op_instr *instr = NULL; unsigned int op_id = 0; unsigned int len = 0; @@ -2935,7 +2933,7 @@ static int qcom_erase_cmd_type_exec(struct nand_chip *chip, const struct nand_su { struct qcom_nand_host *host = to_qcom_nand_host(chip); struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip); - struct qcom_op q_op; + struct qcom_op q_op = {}; int ret = 0; qcom_parse_instructions(chip, subop, &q_op);
Instead of allocating a structure on the stack with random data and then expect the callee to perform the initialization (which is, in general, error prone), prefer zeroing the structure explicitly at allocation and provide the already zeroed area, so no explicit memset operation is needed. It is probably safer to do so, so we limit the timeframe when dirty data could actually be accessed by mistake. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/mtd/nand/raw/qcom_nandc.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)