Re: [PATCH v2] mtd: bcm47xxnflash: Add error handling for bcm47xxnflash_ops_bcm4706_ctl_cmd()

[Miquel Raynal]

Hello Liang,

On 05/04/2025 at 22:33:47 +08, Wentao Liang <vulab@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> ---
>  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