| Message ID | 20230716144612.32132-9-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:12PM +0200, Miquel Raynal wrote: > The naddrs variable is initialized but not used. Fixing this could have > been a matter of dropping the variable, but the right way to do it looks > a bit more complex: we can avoid useless writes to the q_op structure by > using it. In practice we could even have possible out-of-bound bugs with > the existing implementation. Let's fix all that by just performing the > right number of assignments in the addr{1,2}_reg fields. > > Fixes: 89550beb098e ("mtd: rawnand: qcom: Implement exec_op()") > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202307131959.PdPSC86K-lkp@intel.com/ > Closes: https://lore.kernel.org/oe-kbuild-all/202307131730.NOYbcjBr-lkp@intel.com/ > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> LGTM! But I'm relying on Sadre to test it. Acked-by: Manivannan Sadhasivam <mani@kernel.org> - Mani > --- > drivers/mtd/nand/raw/qcom_nandc.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c > index 4fc8dafa8f03..dc8ca60fc2e2 100644 > --- a/drivers/mtd/nand/raw/qcom_nandc.c > +++ b/drivers/mtd/nand/raw/qcom_nandc.c > @@ -2616,12 +2616,13 @@ static void qcom_parse_instructions(struct nand_chip *chip, > offset = nand_subop_get_addr_start_off(subop, op_id); > naddrs = nand_subop_get_num_addr_cyc(subop, op_id); > addrs = &instr->ctx.addr.addrs[offset]; > - for (i = 0; i < MAX_ADDRESS_CYCLE; i++) { > - if (i < 4) > - q_op->addr1_reg |= (u32)addrs[i] << i * 8; > - else > - q_op->addr2_reg |= addrs[i]; > - } > + > + for (i = 0; i < min_t(unsigned int, 4, naddrs); i++) > + q_op->addr1_reg |= addrs[i] << (i * 8); > + > + if (naddrs > 4) > + q_op->addr2_reg |= addrs[4]; > + > q_op->rdy_delay_ns = instr->delay_ns; > break; > > -- > 2.34.1 >
Hi Sadre, mani@kernel.org wrote on Mon, 17 Jul 2023 12:08:43 +0530: > On Sun, Jul 16, 2023 at 04:46:12PM +0200, Miquel Raynal wrote: > > The naddrs variable is initialized but not used. Fixing this could have > > been a matter of dropping the variable, but the right way to do it looks > > a bit more complex: we can avoid useless writes to the q_op structure by > > using it. In practice we could even have possible out-of-bound bugs with > > the existing implementation. Let's fix all that by just performing the > > right number of assignments in the addr{1,2}_reg fields. > > > > Fixes: 89550beb098e ("mtd: rawnand: qcom: Implement exec_op()") > > Reported-by: kernel test robot <lkp@intel.com> > > Closes: https://lore.kernel.org/oe-kbuild-all/202307131959.PdPSC86K-lkp@intel.com/ > > Closes: https://lore.kernel.org/oe-kbuild-all/202307131730.NOYbcjBr-lkp@intel.com/ > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > LGTM! But I'm relying on Sadre to test it. > > Acked-by: Manivannan Sadhasivam <mani@kernel.org> I don't think I've received feedback from Sadre, I would like to close these issues, can you please test and give us feedback? Thanks a lot, Miquèl > > - Mani > > > --- > > drivers/mtd/nand/raw/qcom_nandc.c | 13 +++++++------ > > 1 file changed, 7 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c > > index 4fc8dafa8f03..dc8ca60fc2e2 100644 > > --- a/drivers/mtd/nand/raw/qcom_nandc.c > > +++ b/drivers/mtd/nand/raw/qcom_nandc.c > > @@ -2616,12 +2616,13 @@ static void qcom_parse_instructions(struct nand_chip *chip, > > offset = nand_subop_get_addr_start_off(subop, op_id); > > naddrs = nand_subop_get_num_addr_cyc(subop, op_id); > > addrs = &instr->ctx.addr.addrs[offset]; > > - for (i = 0; i < MAX_ADDRESS_CYCLE; i++) { > > - if (i < 4) > > - q_op->addr1_reg |= (u32)addrs[i] << i * 8; > > - else > > - q_op->addr2_reg |= addrs[i]; > > - } > > + > > + for (i = 0; i < min_t(unsigned int, 4, naddrs); i++) > > + q_op->addr1_reg |= addrs[i] << (i * 8); > > + > > + if (naddrs > 4) > > + q_op->addr2_reg |= addrs[4]; > > + > > q_op->rdy_delay_ns = instr->delay_ns; > > break; > > > > -- > > 2.34.1 > > >
On 7/16/23 15:46, Miquel Raynal wrote: > The naddrs variable is initialized but not used. Fixing this could have > been a matter of dropping the variable, but the right way to do it looks > a bit more complex: we can avoid useless writes to the q_op structure by > using it. In practice we could even have possible out-of-bound bugs with > the existing implementation. Let's fix all that by just performing the > right number of assignments in the addr{1,2}_reg fields. > > Fixes: 89550beb098e ("mtd: rawnand: qcom: Implement exec_op()") > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202307131959.PdPSC86K-lkp@intel.com/ > Closes: https://lore.kernel.org/oe-kbuild-all/202307131730.NOYbcjBr-lkp@intel.com/ > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> Reviewed-by: Tudor Ambarus <tudor.ambarus@linaro.org>
Hi, Miquel, On 7/27/23 15:59, Miquel Raynal wrote: > Hi Sadre, > > mani@kernel.org wrote on Mon, 17 Jul 2023 12:08:43 +0530: > >> On Sun, Jul 16, 2023 at 04:46:12PM +0200, Miquel Raynal wrote: >>> The naddrs variable is initialized but not used. Fixing this could have >>> been a matter of dropping the variable, but the right way to do it looks >>> a bit more complex: we can avoid useless writes to the q_op structure by >>> using it. In practice we could even have possible out-of-bound bugs with >>> the existing implementation. Let's fix all that by just performing the >>> right number of assignments in the addr{1,2}_reg fields. >>> >>> Fixes: 89550beb098e ("mtd: rawnand: qcom: Implement exec_op()") >>> Reported-by: kernel test robot <lkp@intel.com> >>> Closes: https://lore.kernel.org/oe-kbuild-all/202307131959.PdPSC86K-lkp@intel.com/ >>> Closes: https://lore.kernel.org/oe-kbuild-all/202307131730.NOYbcjBr-lkp@intel.com/ >>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> >> LGTM! But I'm relying on Sadre to test it. >> >> Acked-by: Manivannan Sadhasivam <mani@kernel.org> > I don't think I've received feedback from Sadre, I would like to close > these issues, can you please test and give us feedback? All patches are pretty straight forward. Let the 0-day bot run with them and then apply all if no further feedback. Cheers, ta
Hi Miquel, On 7/27/2023 8:29 PM, Miquel Raynal wrote: > Hi Sadre, > > mani@kernel.org wrote on Mon, 17 Jul 2023 12:08:43 +0530: > >> On Sun, Jul 16, 2023 at 04:46:12PM +0200, Miquel Raynal wrote: >>> The naddrs variable is initialized but not used. Fixing this could have >>> been a matter of dropping the variable, but the right way to do it looks >>> a bit more complex: we can avoid useless writes to the q_op structure by >>> using it. In practice we could even have possible out-of-bound bugs with >>> the existing implementation. Let's fix all that by just performing the >>> right number of assignments in the addr{1,2}_reg fields. >>> >>> Fixes: 89550beb098e ("mtd: rawnand: qcom: Implement exec_op()") >>> Reported-by: kernel test robot <lkp@intel.com> >>> Closes: https://lore.kernel.org/oe-kbuild-all/202307131959.PdPSC86K-lkp@intel.com/ >>> Closes: https://lore.kernel.org/oe-kbuild-all/202307131730.NOYbcjBr-lkp@intel.com/ >>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> >> >> LGTM! But I'm relying on Sadre to test it. >> >> Acked-by: Manivannan Sadhasivam <mani@kernel.org> > > I don't think I've received feedback from Sadre, I would like to close > these issues, can you please test and give us feedback? > Sorry for the delay in response. Testing this, will respond back shortly today. Regards, Sricharan
Hi Miquel, On 7/28/2023 8:03 AM, Sricharan Ramabadhran wrote: > Hi Miquel, > > On 7/27/2023 8:29 PM, Miquel Raynal wrote: >> Hi Sadre, >> >> mani@kernel.org wrote on Mon, 17 Jul 2023 12:08:43 +0530: >> >>> On Sun, Jul 16, 2023 at 04:46:12PM +0200, Miquel Raynal wrote: >>>> The naddrs variable is initialized but not used. Fixing this could have >>>> been a matter of dropping the variable, but the right way to do it >>>> looks >>>> a bit more complex: we can avoid useless writes to the q_op >>>> structure by >>>> using it. In practice we could even have possible out-of-bound bugs >>>> with >>>> the existing implementation. Let's fix all that by just performing the >>>> right number of assignments in the addr{1,2}_reg fields. >>>> >>>> Fixes: 89550beb098e ("mtd: rawnand: qcom: Implement exec_op()") >>>> Reported-by: kernel test robot <lkp@intel.com> >>>> Closes: >>>> https://lore.kernel.org/oe-kbuild-all/202307131959.PdPSC86K-lkp@intel.com/ >>>> >>>> Closes: >>>> https://lore.kernel.org/oe-kbuild-all/202307131730.NOYbcjBr-lkp@intel.com/ >>>> >>>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> >>> >>> LGTM! But I'm relying on Sadre to test it. >>> >>> Acked-by: Manivannan Sadhasivam <mani@kernel.org> >> >> I don't think I've received feedback from Sadre, I would like to close >> these issues, can you please test and give us feedback? >> > > Sorry for the delay in response. Testing this, will respond back > shortly today. Tested this series on ipq8074-hk01 board. Tested bootcheck, both /dev/mtd and /dev/mtdblock for data integrity. All looks fine. So, Tested-by: Sricharan Ramabadhran <quic_srichara@quicinc.com> Regards, Sricharan
Hi Sricharan, quic_srichara@quicinc.com wrote on Fri, 28 Jul 2023 09:44:37 +0530: > Hi Miquel, > > On 7/28/2023 8:03 AM, Sricharan Ramabadhran wrote: > > Hi Miquel, > > > > On 7/27/2023 8:29 PM, Miquel Raynal wrote: > >> Hi Sadre, > >> > >> mani@kernel.org wrote on Mon, 17 Jul 2023 12:08:43 +0530: > >> > >>> On Sun, Jul 16, 2023 at 04:46:12PM +0200, Miquel Raynal wrote: > >>>> The naddrs variable is initialized but not used. Fixing this could have > >>>> been a matter of dropping the variable, but the right way to do it >>>> looks > >>>> a bit more complex: we can avoid useless writes to the q_op >>>> structure by > >>>> using it. In practice we could even have possible out-of-bound bugs >>>> with > >>>> the existing implementation. Let's fix all that by just performing the > >>>> right number of assignments in the addr{1,2}_reg fields. > >>>> > >>>> Fixes: 89550beb098e ("mtd: rawnand: qcom: Implement exec_op()") > >>>> Reported-by: kernel test robot <lkp@intel.com> > >>>> Closes: >>>> https://lore.kernel.org/oe-kbuild-all/202307131959.PdPSC86K-lkp@intel.com/ >>>> > >>>> Closes: >>>> https://lore.kernel.org/oe-kbuild-all/202307131730.NOYbcjBr-lkp@intel.com/ >>>> > >>>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > >>> > >>> LGTM! But I'm relying on Sadre to test it. > >>> > >>> Acked-by: Manivannan Sadhasivam <mani@kernel.org> > >> > >> I don't think I've received feedback from Sadre, I would like to close > >> these issues, can you please test and give us feedback? > >> > > > > Sorry for the delay in response. Testing this, will respond back > > shortly today. > > Tested this series on ipq8074-hk01 board. > Tested bootcheck, both /dev/mtd and /dev/mtdblock for data integrity. > All looks fine. So, > > Tested-by: Sricharan Ramabadhran <quic_srichara@quicinc.com> Thanks for the test! I'll take the series now then. Thanks, Miquèl
Hi Tudor, tudor.ambarus@linaro.org wrote on Fri, 28 Jul 2023 03:31:41 +0100: > Hi, Miquel, > > On 7/27/23 15:59, Miquel Raynal wrote: > > Hi Sadre, > > > > mani@kernel.org wrote on Mon, 17 Jul 2023 12:08:43 +0530: > > > >> On Sun, Jul 16, 2023 at 04:46:12PM +0200, Miquel Raynal wrote: > >>> The naddrs variable is initialized but not used. Fixing this could have > >>> been a matter of dropping the variable, but the right way to do it looks > >>> a bit more complex: we can avoid useless writes to the q_op structure by > >>> using it. In practice we could even have possible out-of-bound bugs with > >>> the existing implementation. Let's fix all that by just performing the > >>> right number of assignments in the addr{1,2}_reg fields. > >>> > >>> Fixes: 89550beb098e ("mtd: rawnand: qcom: Implement exec_op()") > >>> Reported-by: kernel test robot <lkp@intel.com> > >>> Closes: https://lore.kernel.org/oe-kbuild-all/202307131959.PdPSC86K-lkp@intel.com/ > >>> Closes: https://lore.kernel.org/oe-kbuild-all/202307131730.NOYbcjBr-lkp@intel.com/ > >>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > >> LGTM! But I'm relying on Sadre to test it. > >> > >> Acked-by: Manivannan Sadhasivam <mani@kernel.org> > > I don't think I've received feedback from Sadre, I would like to close > > these issues, can you please test and give us feedback? > > All patches are pretty straight forward. Let the 0-day bot run with them > and then apply all if no further feedback. Thanks a lot for all the review! Fortunately I just received the Tested-by, otherwise I agree most patches are simple enough (besides the last one maybe) and 0-day might have done a good first iteration. Thanks, Miquèl
On Sun, 2023-07-16 at 14:46:12 UTC, Miquel Raynal wrote: > The naddrs variable is initialized but not used. Fixing this could have > been a matter of dropping the variable, but the right way to do it looks > a bit more complex: we can avoid useless writes to the q_op structure by > using it. In practice we could even have possible out-of-bound bugs with > the existing implementation. Let's fix all that by just performing the > right number of assignments in the addr{1,2}_reg fields. > > Fixes: 89550beb098e ("mtd: rawnand: qcom: Implement exec_op()") > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202307131959.PdPSC86K-lkp@intel.com/ > Closes: https://lore.kernel.org/oe-kbuild-all/202307131730.NOYbcjBr-lkp@intel.com/ > 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 4fc8dafa8f03..dc8ca60fc2e2 100644 --- a/drivers/mtd/nand/raw/qcom_nandc.c +++ b/drivers/mtd/nand/raw/qcom_nandc.c @@ -2616,12 +2616,13 @@ static void qcom_parse_instructions(struct nand_chip *chip, offset = nand_subop_get_addr_start_off(subop, op_id); naddrs = nand_subop_get_num_addr_cyc(subop, op_id); addrs = &instr->ctx.addr.addrs[offset]; - for (i = 0; i < MAX_ADDRESS_CYCLE; i++) { - if (i < 4) - q_op->addr1_reg |= (u32)addrs[i] << i * 8; - else - q_op->addr2_reg |= addrs[i]; - } + + for (i = 0; i < min_t(unsigned int, 4, naddrs); i++) + q_op->addr1_reg |= addrs[i] << (i * 8); + + if (naddrs > 4) + q_op->addr2_reg |= addrs[4]; + q_op->rdy_delay_ns = instr->delay_ns; break;
The naddrs variable is initialized but not used. Fixing this could have been a matter of dropping the variable, but the right way to do it looks a bit more complex: we can avoid useless writes to the q_op structure by using it. In practice we could even have possible out-of-bound bugs with the existing implementation. Let's fix all that by just performing the right number of assignments in the addr{1,2}_reg fields. Fixes: 89550beb098e ("mtd: rawnand: qcom: Implement exec_op()") Reported-by: kernel test robot <lkp@intel.com> Closes: https://lore.kernel.org/oe-kbuild-all/202307131959.PdPSC86K-lkp@intel.com/ Closes: https://lore.kernel.org/oe-kbuild-all/202307131730.NOYbcjBr-lkp@intel.com/ Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/mtd/nand/raw/qcom_nandc.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)