| Message ID | 20240207202257.271784-12-william.zhang@broadcom.com |
|---|---|
| State | New |
| Headers | show |
| Series | mtd: rawnand: brcmnand: driver and doc updates | expand |
Hi, william.zhang@broadcom.com wrote on Wed, 7 Feb 2024 12:22:56 -0800: > From: David Regan <dregan@broadcom.com> > > fix return type for exec_op reset and status detect helper functions Please make a correct sentence (Fic, detect?, '.'). > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > Closes: http://lists.infradead.org/pipermail/linux-mtd/2023-December/102423.html > Fixes: 3c8260ce7663 ("mtd: rawnand: brcmnand: exec_op implementation") > Signed-off-by: David Regan <dregan@broadcom.com> > Signed-off-by: William Zhang <william.zhang@broadcom.com> > Reviewed-by: William Zhang <william.zhang@broadcom.com> > > --- > > Changes in v5: None > Changes in v4: > - Fix the commit id in the fixes tag > > Changes in v3: None > Changes in v2: > - Added to patch series > > drivers/mtd/nand/raw/brcmnand/brcmnand.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c > index 8c1489ff7bd6..7ce2b267676f 100644 > --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c > +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c > @@ -625,7 +625,7 @@ enum { > /* Only for v7.2 */ > #define ACC_CONTROL_ECC_EXT_SHIFT 13 > > -static u8 brcmnand_status(struct brcmnand_host *host); > +static int brcmnand_status(struct brcmnand_host *host); > > static inline bool brcmnand_non_mmio_ops(struct brcmnand_controller *ctrl) > { > @@ -1749,7 +1749,7 @@ static int brcmnand_waitfunc(struct nand_chip *chip) > INTFC_FLASH_STATUS; > } > > -static u8 brcmnand_status(struct brcmnand_host *host) > +static int brcmnand_status(struct brcmnand_host *host) > { > struct nand_chip *chip = &host->chip; > struct mtd_info *mtd = nand_to_mtd(chip); > @@ -1760,7 +1760,7 @@ static u8 brcmnand_status(struct brcmnand_host *host) > return brcmnand_waitfunc(chip); > } > > -static u8 brcmnand_reset(struct brcmnand_host *host) > +static int brcmnand_reset(struct brcmnand_host *host) > { > struct nand_chip *chip = &host->chip; > > @@ -2492,11 +2492,14 @@ static int brcmnand_exec_op(struct nand_chip *chip, > > if (brcmnand_op_is_status(op)) { > status = op->instrs[1].ctx.data.buf.in; > - *status = brcmnand_status(host); > + ret = brcmnand_status(host); > + if (ret < 0) > + return ret; > + > + *status = ret & 0xFF; > > return 0; > - } > - else if (brcmnand_op_is_reset(op)) { > + } else if (brcmnand_op_is_reset(op)) { This is another change, please make it in another patch. Also while you are at it, there are probably other checkpatch warnings that can be fixed, if you want. > ret = brcmnand_reset(host); > if (ret < 0) > return ret; Thanks, Miquèl
william.zhang@broadcom.com wrote on Wed, 7 Feb 2024 12:22:56 -0800: > From: David Regan <dregan@broadcom.com> > > fix return type for exec_op reset and status detect helper functions > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > Closes: http://lists.infradead.org/pipermail/linux-mtd/2023-December/102423.html > Fixes: 3c8260ce7663 ("mtd: rawnand: brcmnand: exec_op implementation") > Signed-off-by: David Regan <dregan@broadcom.com> > Signed-off-by: William Zhang <william.zhang@broadcom.com> > Reviewed-by: William Zhang <william.zhang@broadcom.com> By the way, a Cc: stable tag might be useful as otherwise you may leak an error code in a status value (which is a bug). And move this patch first in your series so we're sure it does not conflict with any of the other patches and can be backported more easily. Thanks, Miquèl
Hi Miquel, On 2/20/24 02:02, Miquel Raynal wrote: > > william.zhang@broadcom.com wrote on Wed, 7 Feb 2024 12:22:56 -0800: > >> From: David Regan <dregan@broadcom.com> >> >> fix return type for exec_op reset and status detect helper functions >> >> Reported-by: Dan Carpenter <dan.carpenter@linaro.org> >> Closes: http://lists.infradead.org/pipermail/linux-mtd/2023-December/102423.html >> Fixes: 3c8260ce7663 ("mtd: rawnand: brcmnand: exec_op implementation") >> Signed-off-by: David Regan <dregan@broadcom.com> >> Signed-off-by: William Zhang <william.zhang@broadcom.com> >> Reviewed-by: William Zhang <william.zhang@broadcom.com> > > By the way, a Cc: stable tag might be useful as otherwise you may leak Sorry I am not very familiar with these term and release procedure. But I don't see exec_op patch is in the current stable release 6.7.5 and LTS release. It is only in the mainline and linux-next. Do you mean the stable tag will make it to the mainline? > an error code in a status value (which is a bug). And move this patch > first in your series so we're sure it does not conflict with any of the > other patches and can be backported more easily. > > Thanks, > Miquèl
On Tue, Feb 20, 2024 at 11:02:40AM +0100, Miquel Raynal wrote: > > william.zhang@broadcom.com wrote on Wed, 7 Feb 2024 12:22:56 -0800: > > > From: David Regan <dregan@broadcom.com> > > > > fix return type for exec_op reset and status detect helper functions > > > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > > Closes: http://lists.infradead.org/pipermail/linux-mtd/2023-December/102423.html > > Fixes: 3c8260ce7663 ("mtd: rawnand: brcmnand: exec_op implementation") > > Signed-off-by: David Regan <dregan@broadcom.com> > > Signed-off-by: William Zhang <william.zhang@broadcom.com> > > Reviewed-by: William Zhang <william.zhang@broadcom.com> > > By the way, a Cc: stable tag might be useful as otherwise you may leak > an error code in a status value (which is a bug). And move this patch > first in your series so we're sure it does not conflict with any of the > other patches and can be backported more easily. The original commit 3c8260ce7663 ("mtd: rawnand: brcmnand: exec_op implementation") is not in stable so we should be okay on this. regards, dan carpenter
Hi, dan.carpenter@linaro.org wrote on Wed, 21 Feb 2024 09:16:31 +0300: > On Tue, Feb 20, 2024 at 11:02:40AM +0100, Miquel Raynal wrote: > > > > william.zhang@broadcom.com wrote on Wed, 7 Feb 2024 12:22:56 -0800: > > > > > From: David Regan <dregan@broadcom.com> > > > > > > fix return type for exec_op reset and status detect helper functions > > > > > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > > > Closes: http://lists.infradead.org/pipermail/linux-mtd/2023-December/102423.html > > > Fixes: 3c8260ce7663 ("mtd: rawnand: brcmnand: exec_op implementation") > > > Signed-off-by: David Regan <dregan@broadcom.com> > > > Signed-off-by: William Zhang <william.zhang@broadcom.com> > > > Reviewed-by: William Zhang <william.zhang@broadcom.com> > > > > By the way, a Cc: stable tag might be useful as otherwise you may leak > > an error code in a status value (which is a bug). And move this patch > > first in your series so we're sure it does not conflict with any of the > > other patches and can be backported more easily. > > The original commit 3c8260ce7663 ("mtd: rawnand: brcmnand: exec_op > implementation") is not in stable so we should be okay on this. Right. However please send again this patch quickly so I can queue it up before the rest of the series. Thanks, Miquèl
On 2/21/2024 12:32 AM, 'Miquel Raynal' via BCM-KERNEL-FEEDBACK-LIST,PDL wrote: > Hi, > > dan.carpenter@linaro.org wrote on Wed, 21 Feb 2024 09:16:31 +0300: > >> On Tue, Feb 20, 2024 at 11:02:40AM +0100, Miquel Raynal wrote: >>> >>> william.zhang@broadcom.com wrote on Wed, 7 Feb 2024 12:22:56 -0800: >>> >>>> From: David Regan <dregan@broadcom.com> >>>> >>>> fix return type for exec_op reset and status detect helper functions >>>> >>>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org> >>>> Closes: http://lists.infradead.org/pipermail/linux-mtd/2023-December/102423.html >>>> Fixes: 3c8260ce7663 ("mtd: rawnand: brcmnand: exec_op implementation") >>>> Signed-off-by: David Regan <dregan@broadcom.com> >>>> Signed-off-by: William Zhang <william.zhang@broadcom.com> >>>> Reviewed-by: William Zhang <william.zhang@broadcom.com> >>> >>> By the way, a Cc: stable tag might be useful as otherwise you may leak >>> an error code in a status value (which is a bug). And move this patch >>> first in your series so we're sure it does not conflict with any of the >>> other patches and can be backported more easily. >> >> The original commit 3c8260ce7663 ("mtd: rawnand: brcmnand: exec_op >> implementation") is not in stable so we should be okay on this. > > Right. > > However please send again this patch quickly so I can queue it up > before the rest of the series. Also just to be clear, please do not omit the Fixes: tag, even if the commit being fixed is not yet in any released kernel, it is valuable to have that information, for say, people who might consider backporting features into specific kernel versions. The stable team's tooling should be able to figure out that it is not required to pick up that fix since the offending commit is not in a released kernel yet anyway. Thanks!
diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c index 8c1489ff7bd6..7ce2b267676f 100644 --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c @@ -625,7 +625,7 @@ enum { /* Only for v7.2 */ #define ACC_CONTROL_ECC_EXT_SHIFT 13 -static u8 brcmnand_status(struct brcmnand_host *host); +static int brcmnand_status(struct brcmnand_host *host); static inline bool brcmnand_non_mmio_ops(struct brcmnand_controller *ctrl) { @@ -1749,7 +1749,7 @@ static int brcmnand_waitfunc(struct nand_chip *chip) INTFC_FLASH_STATUS; } -static u8 brcmnand_status(struct brcmnand_host *host) +static int brcmnand_status(struct brcmnand_host *host) { struct nand_chip *chip = &host->chip; struct mtd_info *mtd = nand_to_mtd(chip); @@ -1760,7 +1760,7 @@ static u8 brcmnand_status(struct brcmnand_host *host) return brcmnand_waitfunc(chip); } -static u8 brcmnand_reset(struct brcmnand_host *host) +static int brcmnand_reset(struct brcmnand_host *host) { struct nand_chip *chip = &host->chip; @@ -2492,11 +2492,14 @@ static int brcmnand_exec_op(struct nand_chip *chip, if (brcmnand_op_is_status(op)) { status = op->instrs[1].ctx.data.buf.in; - *status = brcmnand_status(host); + ret = brcmnand_status(host); + if (ret < 0) + return ret; + + *status = ret & 0xFF; return 0; - } - else if (brcmnand_op_is_reset(op)) { + } else if (brcmnand_op_is_reset(op)) { ret = brcmnand_reset(host); if (ret < 0) return ret;