All of lore.kernel.org
 help / color / mirror / Atom feed
From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Wentao Liang <vulab@iscas.ac.cn>
Cc: richard@nod.at,  vigneshr@ti.com,  linux-mtd@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] mtd: bcm47xxnflash: Add error handling for bcm47xxnflash_ops_bcm4706_ctl_cmd()
Date: Mon, 07 Apr 2025 08:51:00 +0200	[thread overview]
Message-ID: <875xjgihwr.fsf@bootlin.com> (raw)
In-Reply-To: <20250405143348.2477-1-vulab@iscas.ac.cn> (Wentao Liang's message of "Sat, 5 Apr 2025 22:33:47 +0800")

Hello Liang,

On 05/04/2025 at 22:33:47 +08, Wentao Liang <vulab@iscas.ac.cn> wrote:

> The bcm47xxnflash_ops_bcm4706_cmd_ctrl() calls
> bcm47xxnflash_ops_bcm4706_ctl_cmd() without printing error message. A
> proper implementation can be found in
> bcm47xxnflash_ops_bcm4706_write().

This is a problem with cmd_ctrl, but this interface has been deprecated
~5y ago and people encouraged to transition to ->exec_op(). Did you
personally experience issues with that or is this just code analysis? If
you are actually using this driver, I'd highly suggest to update the
driver.

> Add error log to the bcm47xxnflash_ops_bcm4706_ctl_cmd() to prevent
> silent failure.
>
> Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
> ---
>  drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c b/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
> index 6487dfc64258..c89129588bb6 100644
> --- a/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
> +++ b/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
> @@ -182,7 +182,8 @@ static void bcm47xxnflash_ops_bcm4706_cmd_ctrl(struct nand_chip *nand_chip,
>  	if (cmd != NAND_CMD_RESET)
>  		code |= NCTL_CSA;
>  
> -	bcm47xxnflash_ops_bcm4706_ctl_cmd(b47n->cc, code);
> +	if (bcm47xxnflash_ops_bcm4706_ctl_cmd(b47n->cc, code))
> +		pr_err("%s ctl_cmd didn't work!\n", __func__);

dev_err() is probably better
__func__ is not needed
If we want to make it a bit usable, I'd say that the return code shall
be there as well.

>  }
>  
>  /* Default nand_select_chip calls cmd_ctrl, which is not used in BCM4706 */

Thanks,
Miquèl


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

WARNING: multiple messages have this Message-ID (diff)
From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Wentao Liang <vulab@iscas.ac.cn>
Cc: richard@nod.at,  vigneshr@ti.com,  linux-mtd@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] mtd: bcm47xxnflash: Add error handling for bcm47xxnflash_ops_bcm4706_ctl_cmd()
Date: Mon, 07 Apr 2025 08:51:00 +0200	[thread overview]
Message-ID: <875xjgihwr.fsf@bootlin.com> (raw)
In-Reply-To: <20250405143348.2477-1-vulab@iscas.ac.cn> (Wentao Liang's message of "Sat, 5 Apr 2025 22:33:47 +0800")

Hello Liang,

On 05/04/2025 at 22:33:47 +08, Wentao Liang <vulab@iscas.ac.cn> wrote:

> The bcm47xxnflash_ops_bcm4706_cmd_ctrl() calls
> bcm47xxnflash_ops_bcm4706_ctl_cmd() without printing error message. A
> proper implementation can be found in
> bcm47xxnflash_ops_bcm4706_write().

This is a problem with cmd_ctrl, but this interface has been deprecated
~5y ago and people encouraged to transition to ->exec_op(). Did you
personally experience issues with that or is this just code analysis? If
you are actually using this driver, I'd highly suggest to update the
driver.

> Add error log to the bcm47xxnflash_ops_bcm4706_ctl_cmd() to prevent
> silent failure.
>
> Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
> ---
>  drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c b/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
> index 6487dfc64258..c89129588bb6 100644
> --- a/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
> +++ b/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
> @@ -182,7 +182,8 @@ static void bcm47xxnflash_ops_bcm4706_cmd_ctrl(struct nand_chip *nand_chip,
>  	if (cmd != NAND_CMD_RESET)
>  		code |= NCTL_CSA;
>  
> -	bcm47xxnflash_ops_bcm4706_ctl_cmd(b47n->cc, code);
> +	if (bcm47xxnflash_ops_bcm4706_ctl_cmd(b47n->cc, code))
> +		pr_err("%s ctl_cmd didn't work!\n", __func__);

dev_err() is probably better
__func__ is not needed
If we want to make it a bit usable, I'd say that the return code shall
be there as well.

>  }
>  
>  /* Default nand_select_chip calls cmd_ctrl, which is not used in BCM4706 */

Thanks,
Miquèl


  reply	other threads:[~2025-04-07  6:51 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-04-05 14:33 [PATCH v2] mtd: bcm47xxnflash: Add error handling for bcm47xxnflash_ops_bcm4706_ctl_cmd() Wentao Liang
2025-04-05 14:33 ` Wentao Liang
2025-04-07  6:51 ` Miquel Raynal [this message]
2025-04-07  6:51   ` Miquel Raynal

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=875xjgihwr.fsf@bootlin.com \
    --to=miquel.raynal@bootlin.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=richard@nod.at \
    --cc=vigneshr@ti.com \
    --cc=vulab@iscas.ac.cn \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.