diff mbox series

[v3,3/5] mtd: rawnand: brcmnand: Fix crash during the panic_write

Message ID 20230627193738.19596-4-william.zhang@broadcom.com
State New
Headers
Series mtd: rawnand: brcmnand: driver and doc updates |

Commit Message

William Zhang June 27, 2023, 7:37 p.m. UTC 
  During the panic write path to execute another nand command, if
there is a pending command, we should wait for the command instead of
calling BUG_ON so we don't crash while crashing.

Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")
Signed-off-by: William Zhang <william.zhang@broadcom.com>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Reviewed-by: Kursad Oney <kursad.oney@broadcom.com>
Reviewed-by: Kamal Dasu <kamal.dasu@broadcom.com>
---

Changes in v3: None
Changes in v2: None

 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Comments

Miquel Raynal July 4, 2023, 3:23 p.m. UTC | #1 
Hi William,

william.zhang@broadcom.com wrote on Tue, 27 Jun 2023 12:37:36 -0700:

> During the panic write path to execute another nand command, if

"When executing a NAND command within the panic write path, wait for any
pending command instead of calling BUG_ON to avoid crashing while
(already) crashing."

> there is a pending command, we should wait for the command instead of
> calling BUG_ON so we don't crash while crashing.
> 
> Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")
> Signed-off-by: William Zhang <william.zhang@broadcom.com>
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> Reviewed-by: Kursad Oney <kursad.oney@broadcom.com>
> Reviewed-by: Kamal Dasu <kamal.dasu@broadcom.com>
> ---
> 
> Changes in v3: None
> Changes in v2: None
> 
>  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> index 37c2c7cfa00e..ea03104692bf 100644
> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> @@ -1608,7 +1608,17 @@ static void brcmnand_send_cmd(struct brcmnand_host *host, int cmd)
>  
>  	dev_dbg(ctrl->dev, "send native cmd %d addr 0x%llx\n", cmd, cmd_addr);
>  
> -	BUG_ON(ctrl->cmd_pending != 0);
> +	/*
> +	 * If we came here through _panic_write and there is a pending
> +	 * command, try to wait for it. If it times out, rather than
> +	 * hitting BUG_ON, just return so we don't crash while crashing.
> +	 */
> +	if (oops_in_progress) {
> +		if (ctrl->cmd_pending &&
> +			bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY, NAND_CTRL_RDY, 0))
> +			return;
> +	} else
> +		BUG_ON(ctrl->cmd_pending != 0);
>  	ctrl->cmd_pending = cmd;
>  
>  	ret = bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY, NAND_CTRL_RDY, 0);

LGTM otherwise.

Thanks,
Miquèl
Miquel Raynal July 4, 2023, 3:26 p.m. UTC | #2 
Hi William,

william.zhang@broadcom.com wrote on Tue, 27 Jun 2023 12:37:36 -0700:

> During the panic write path to execute another nand command, if
> there is a pending command, we should wait for the command instead of
> calling BUG_ON so we don't crash while crashing.
> 
> Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")

The Fixes tag looks wrong.

> Signed-off-by: William Zhang <william.zhang@broadcom.com>
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> Reviewed-by: Kursad Oney <kursad.oney@broadcom.com>
> Reviewed-by: Kamal Dasu <kamal.dasu@broadcom.com>
> ---
> 
> Changes in v3: None
> Changes in v2: None
> 
>  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> index 37c2c7cfa00e..ea03104692bf 100644
> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> @@ -1608,7 +1608,17 @@ static void brcmnand_send_cmd(struct brcmnand_host *host, int cmd)
>  
>  	dev_dbg(ctrl->dev, "send native cmd %d addr 0x%llx\n", cmd, cmd_addr);
>  
> -	BUG_ON(ctrl->cmd_pending != 0);
> +	/*
> +	 * If we came here through _panic_write and there is a pending
> +	 * command, try to wait for it. If it times out, rather than
> +	 * hitting BUG_ON, just return so we don't crash while crashing.
> +	 */
> +	if (oops_in_progress) {
> +		if (ctrl->cmd_pending &&
> +			bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY, NAND_CTRL_RDY, 0))
> +			return;
> +	} else
> +		BUG_ON(ctrl->cmd_pending != 0);
>  	ctrl->cmd_pending = cmd;
>  
>  	ret = bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY, NAND_CTRL_RDY, 0);


Thanks,
Miquèl
William Zhang July 5, 2023, 12:40 a.m. UTC | #3 
Hi Miquel,

On 07/04/2023 08:26 AM, Miquel Raynal wrote:
> Hi William,
> 
> william.zhang@broadcom.com wrote on Tue, 27 Jun 2023 12:37:36 -0700:
> 
>> During the panic write path to execute another nand command, if
>> there is a pending command, we should wait for the command instead of
>> calling BUG_ON so we don't crash while crashing.
>>
>> Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")
> 
> The Fixes tag looks wrong.
> 
The brcmnand_send_cmd function and BUG_ON line were added by this commit 
and the function didn't changed much since then. Not sure why you think 
it is wrong?

>> Signed-off-by: William Zhang <william.zhang@broadcom.com>
>> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
>> Reviewed-by: Kursad Oney <kursad.oney@broadcom.com>
>> Reviewed-by: Kamal Dasu <kamal.dasu@broadcom.com>
>> ---
>>
>> Changes in v3: None
>> Changes in v2: None
>>
>>   drivers/mtd/nand/raw/brcmnand/brcmnand.c | 12 +++++++++++-
>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> index 37c2c7cfa00e..ea03104692bf 100644
>> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> @@ -1608,7 +1608,17 @@ static void brcmnand_send_cmd(struct brcmnand_host *host, int cmd)
>>   
>>   	dev_dbg(ctrl->dev, "send native cmd %d addr 0x%llx\n", cmd, cmd_addr);
>>   
>> -	BUG_ON(ctrl->cmd_pending != 0);
>> +	/*
>> +	 * If we came here through _panic_write and there is a pending
>> +	 * command, try to wait for it. If it times out, rather than
>> +	 * hitting BUG_ON, just return so we don't crash while crashing.
>> +	 */
>> +	if (oops_in_progress) {
>> +		if (ctrl->cmd_pending &&
>> +			bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY, NAND_CTRL_RDY, 0))
>> +			return;
>> +	} else
>> +		BUG_ON(ctrl->cmd_pending != 0);
>>   	ctrl->cmd_pending = cmd;
>>   
>>   	ret = bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY, NAND_CTRL_RDY, 0);
> 
> 
> Thanks,
> Miquèl
>
Miquel Raynal July 5, 2023, 7:09 a.m. UTC | #4 
Hi William,

william.zhang@broadcom.com wrote on Tue, 4 Jul 2023 17:40:03 -0700:

> Hi Miquel,
> 
> On 07/04/2023 08:26 AM, Miquel Raynal wrote:
> > Hi William,
> > 
> > william.zhang@broadcom.com wrote on Tue, 27 Jun 2023 12:37:36 -0700:
> >   
> >> During the panic write path to execute another nand command, if
> >> there is a pending command, we should wait for the command instead of
> >> calling BUG_ON so we don't crash while crashing.
> >>
> >> Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")  
> > 
> > The Fixes tag looks wrong.
> >   
> The brcmnand_send_cmd function and BUG_ON line were added by this commit and the function didn't changed much since then. Not sure why you think it is wrong?

Ok, the title of that commit let me think it was moving code rather
than adding it. Alright.

> >> Signed-off-by: William Zhang <william.zhang@broadcom.com>
> >> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> >> Reviewed-by: Kursad Oney <kursad.oney@broadcom.com>
> >> Reviewed-by: Kamal Dasu <kamal.dasu@broadcom.com>
> >> ---
> >>
> >> Changes in v3: None
> >> Changes in v2: None
> >>
> >>   drivers/mtd/nand/raw/brcmnand/brcmnand.c | 12 +++++++++++-
> >>   1 file changed, 11 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >> index 37c2c7cfa00e..ea03104692bf 100644
> >> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >> @@ -1608,7 +1608,17 @@ static void brcmnand_send_cmd(struct brcmnand_host *host, int cmd)  
> >>   >>   	dev_dbg(ctrl->dev, "send native cmd %d addr 0x%llx\n", cmd, cmd_addr);
> >>   >> -	BUG_ON(ctrl->cmd_pending != 0);  
> >> +	/*
> >> +	 * If we came here through _panic_write and there is a pending
> >> +	 * command, try to wait for it. If it times out, rather than
> >> +	 * hitting BUG_ON, just return so we don't crash while crashing.
> >> +	 */
> >> +	if (oops_in_progress) {
> >> +		if (ctrl->cmd_pending &&
> >> +			bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY, NAND_CTRL_RDY, 0))
> >> +			return;
> >> +	} else
> >> +		BUG_ON(ctrl->cmd_pending != 0);
> >>   	ctrl->cmd_pending = cmd;  
> >>   >>   	ret = bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY, NAND_CTRL_RDY, 0);  
> > 
> > 
> > Thanks,
> > Miquèl
> >   


Thanks,
Miquèl
diff mbox series

Patch

diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index 37c2c7cfa00e..ea03104692bf 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -1608,7 +1608,17 @@  static void brcmnand_send_cmd(struct brcmnand_host *host, int cmd)
 
 	dev_dbg(ctrl->dev, "send native cmd %d addr 0x%llx\n", cmd, cmd_addr);
 
-	BUG_ON(ctrl->cmd_pending != 0);
+	/*
+	 * If we came here through _panic_write and there is a pending
+	 * command, try to wait for it. If it times out, rather than
+	 * hitting BUG_ON, just return so we don't crash while crashing.
+	 */
+	if (oops_in_progress) {
+		if (ctrl->cmd_pending &&
+			bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY, NAND_CTRL_RDY, 0))
+			return;
+	} else
+		BUG_ON(ctrl->cmd_pending != 0);
 	ctrl->cmd_pending = cmd;
 
 	ret = bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY, NAND_CTRL_RDY, 0);