| Message ID | 20230805050146.1858880-1-ruanjinjie@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | [-next,v2] mtd: rawnand: Propagate error values for brcmstb_nand_wait_for_completion() | expand |
Hi Jinjie, On 08/04/2023 10:01 PM, 'Ruan Jinjie' via BCM-KERNEL-FEEDBACK-LIST,PDL wrote: > As bcmnand_ctrl_poll_status() return negative errno, and the < 0 > case does not exist for wait_for_completion_timeout(), so return > -ETIMEDOUT if sts = 0 and zero otherwise. It is not sensible for > brcmstb_nand_wait_for_completion() to return bool value, change it > to int to propagate the error. > > Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com> > --- > v2: > - Update the commit title and message. > - Propagate the error instead of upon error. > --- > drivers/mtd/nand/raw/brcmnand/brcmnand.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c > index 03764b589ec5..25e5b15fbd6a 100644 > --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c > +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c > @@ -1653,12 +1653,12 @@ static void brcmnand_cmd_ctrl(struct nand_chip *chip, int dat, > /* intentionally left blank */ > } > > -static bool brcmstb_nand_wait_for_completion(struct nand_chip *chip) > +static int brcmstb_nand_wait_for_completion(struct nand_chip *chip) > { > struct brcmnand_host *host = nand_get_controller_data(chip); > struct brcmnand_controller *ctrl = host->ctrl; > struct mtd_info *mtd = nand_to_mtd(chip); > - bool err = false; > + int err = 0; > int sts; > > if (mtd->oops_panic_write || ctrl->irq < 0) { > @@ -1666,13 +1666,14 @@ static bool brcmstb_nand_wait_for_completion(struct nand_chip *chip) > disable_ctrl_irqs(ctrl); > sts = bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY, > NAND_CTRL_RDY, 0); > - err = (sts < 0) ? true : false; > + err = sts; > } else { > unsigned long timeo = msecs_to_jiffies( > NAND_POLL_STATUS_TIMEOUT_MS); > /* wait for completion interrupt */ > sts = wait_for_completion_timeout(&ctrl->done, timeo); > - err = (sts <= 0) ? true : false; > + if (!sts) > + err = -ETIMEDOUT; > } > > return err; > Sorry that I missed your first patch but the original goal of this function is to return if time out happens. So you can simplify the ternary operator but propagate -ETIMEDOUT from its caller brcmnand_waitfunc instead.
On 2023/8/6 14:09, William Zhang wrote: > Hi Jinjie, > > On 08/04/2023 10:01 PM, 'Ruan Jinjie' via BCM-KERNEL-FEEDBACK-LIST,PDL > wrote: >> As bcmnand_ctrl_poll_status() return negative errno, and the < 0 >> case does not exist for wait_for_completion_timeout(), so return >> -ETIMEDOUT if sts = 0 and zero otherwise. It is not sensible for >> brcmstb_nand_wait_for_completion() to return bool value, change it >> to int to propagate the error. >> >> Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com> >> --- >> v2: >> - Update the commit title and message. >> - Propagate the error instead of upon error. >> --- >> drivers/mtd/nand/raw/brcmnand/brcmnand.c | 9 +++++---- >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c >> b/drivers/mtd/nand/raw/brcmnand/brcmnand.c >> index 03764b589ec5..25e5b15fbd6a 100644 >> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c >> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c >> @@ -1653,12 +1653,12 @@ static void brcmnand_cmd_ctrl(struct nand_chip >> *chip, int dat, >> /* intentionally left blank */ >> } >> -static bool brcmstb_nand_wait_for_completion(struct nand_chip *chip) >> +static int brcmstb_nand_wait_for_completion(struct nand_chip *chip) >> { >> struct brcmnand_host *host = nand_get_controller_data(chip); >> struct brcmnand_controller *ctrl = host->ctrl; >> struct mtd_info *mtd = nand_to_mtd(chip); >> - bool err = false; >> + int err = 0; >> int sts; >> if (mtd->oops_panic_write || ctrl->irq < 0) { >> @@ -1666,13 +1666,14 @@ static bool >> brcmstb_nand_wait_for_completion(struct nand_chip *chip) >> disable_ctrl_irqs(ctrl); >> sts = bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY, >> NAND_CTRL_RDY, 0); >> - err = (sts < 0) ? true : false; >> + err = sts; >> } else { >> unsigned long timeo = msecs_to_jiffies( >> NAND_POLL_STATUS_TIMEOUT_MS); >> /* wait for completion interrupt */ >> sts = wait_for_completion_timeout(&ctrl->done, timeo); >> - err = (sts <= 0) ? true : false; >> + if (!sts) >> + err = -ETIMEDOUT; >> } >> return err; >> > Sorry that I missed your first patch but the original goal of this > function is to return if time out happens. So you can simplify the > ternary operator but propagate -ETIMEDOUT from its caller > brcmnand_waitfunc instead. So not change the return from bool to int? Just simplify ternary operator. How to propagate -ETIMEDOUT to brcmstb_nand_wait_for_completion?
On 08/07/2023 02:42 AM, Ruan Jinjie wrote: > > > On 2023/8/6 14:09, William Zhang wrote: >> Hi Jinjie, >> >> On 08/04/2023 10:01 PM, 'Ruan Jinjie' via BCM-KERNEL-FEEDBACK-LIST,PDL >> wrote: >>> As bcmnand_ctrl_poll_status() return negative errno, and the < 0 >>> case does not exist for wait_for_completion_timeout(), so return >>> -ETIMEDOUT if sts = 0 and zero otherwise. It is not sensible for >>> brcmstb_nand_wait_for_completion() to return bool value, change it >>> to int to propagate the error. >>> >>> Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com> >>> --- >>> v2: >>> - Update the commit title and message. >>> - Propagate the error instead of upon error. >>> --- >>> drivers/mtd/nand/raw/brcmnand/brcmnand.c | 9 +++++---- >>> 1 file changed, 5 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c >>> b/drivers/mtd/nand/raw/brcmnand/brcmnand.c >>> index 03764b589ec5..25e5b15fbd6a 100644 >>> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c >>> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c >>> @@ -1653,12 +1653,12 @@ static void brcmnand_cmd_ctrl(struct nand_chip >>> *chip, int dat, >>> /* intentionally left blank */ >>> } >>> -static bool brcmstb_nand_wait_for_completion(struct nand_chip *chip) >>> +static int brcmstb_nand_wait_for_completion(struct nand_chip *chip) >>> { >>> struct brcmnand_host *host = nand_get_controller_data(chip); >>> struct brcmnand_controller *ctrl = host->ctrl; >>> struct mtd_info *mtd = nand_to_mtd(chip); >>> - bool err = false; >>> + int err = 0; >>> int sts; >>> if (mtd->oops_panic_write || ctrl->irq < 0) { >>> @@ -1666,13 +1666,14 @@ static bool >>> brcmstb_nand_wait_for_completion(struct nand_chip *chip) >>> disable_ctrl_irqs(ctrl); >>> sts = bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY, >>> NAND_CTRL_RDY, 0); >>> - err = (sts < 0) ? true : false; >>> + err = sts; >>> } else { >>> unsigned long timeo = msecs_to_jiffies( >>> NAND_POLL_STATUS_TIMEOUT_MS); >>> /* wait for completion interrupt */ >>> sts = wait_for_completion_timeout(&ctrl->done, timeo); >>> - err = (sts <= 0) ? true : false; >>> + if (!sts) >>> + err = -ETIMEDOUT; >>> } >>> return err; >>> >> Sorry that I missed your first patch but the original goal of this >> function is to return if time out happens. So you can simplify the >> ternary operator but propagate -ETIMEDOUT from its caller >> brcmnand_waitfunc instead. > > So not change the return from bool to int? Just simplify ternary > operator. Yes > How to propagate -ETIMEDOUT to brcmstb_nand_wait_for_completion? In the brcmnand_waitfunc, if err is true, return -ETIMEDOUT. Otherwise, keep the original execution path. I can submit a separate patch late for this if it is more than what you want to fix. Totally up to you.
diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c index 03764b589ec5..25e5b15fbd6a 100644 --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c @@ -1653,12 +1653,12 @@ static void brcmnand_cmd_ctrl(struct nand_chip *chip, int dat, /* intentionally left blank */ } -static bool brcmstb_nand_wait_for_completion(struct nand_chip *chip) +static int brcmstb_nand_wait_for_completion(struct nand_chip *chip) { struct brcmnand_host *host = nand_get_controller_data(chip); struct brcmnand_controller *ctrl = host->ctrl; struct mtd_info *mtd = nand_to_mtd(chip); - bool err = false; + int err = 0; int sts; if (mtd->oops_panic_write || ctrl->irq < 0) { @@ -1666,13 +1666,14 @@ static bool brcmstb_nand_wait_for_completion(struct nand_chip *chip) disable_ctrl_irqs(ctrl); sts = bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY, NAND_CTRL_RDY, 0); - err = (sts < 0) ? true : false; + err = sts; } else { unsigned long timeo = msecs_to_jiffies( NAND_POLL_STATUS_TIMEOUT_MS); /* wait for completion interrupt */ sts = wait_for_completion_timeout(&ctrl->done, timeo); - err = (sts <= 0) ? true : false; + if (!sts) + err = -ETIMEDOUT; } return err;
As bcmnand_ctrl_poll_status() return negative errno, and the < 0 case does not exist for wait_for_completion_timeout(), so return -ETIMEDOUT if sts = 0 and zero otherwise. It is not sensible for brcmstb_nand_wait_for_completion() to return bool value, change it to int to propagate the error. Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com> --- v2: - Update the commit title and message. - Propagate the error instead of upon error. --- drivers/mtd/nand/raw/brcmnand/brcmnand.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)