diff mbox series

[v2,mvebu,+,mvebu/dt64,1/6] firmware: turris-mox-rwtm: fix reply status decoding function

Message ID 20210429083636.22560-1-pali@kernel.org (mailing list archive)
State New, archived
Headers show
Series [v2,mvebu,+,mvebu/dt64,1/6] firmware: turris-mox-rwtm: fix reply status decoding function | expand

Commit Message

Pali Rohár April 29, 2021, 8:36 a.m. UTC 
From: Marek Behún <kabel@kernel.org>

The status decoding function mox_get_status() currently contains a dead
code path: if the error status is not MBOX_STS_SUCCESS, it always
returns -EIO, so the comparison to MBOX_STS_FAIL is never executed and
we don't get the actual error code sent by the firmware.

Fix this.

Signed-off-by: Marek Behún <kabel@kernel.org>
Fixes: 389711b37493 ("firmware: Add Turris Mox rWTM firmware driver")
---
 drivers/firmware/turris-mox-rwtm.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Andrew Lunn May 3, 2021, 12:22 p.m. UTC | #1 
On Thu, Apr 29, 2021 at 10:36:31AM +0200, Pali Rohár wrote:
> From: Marek Behún <kabel@kernel.org>
> 
> The status decoding function mox_get_status() currently contains a dead
> code path: if the error status is not MBOX_STS_SUCCESS, it always
> returns -EIO, so the comparison to MBOX_STS_FAIL is never executed and
> we don't get the actual error code sent by the firmware.
> 
> Fix this.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> Fixes: 389711b37493 ("firmware: Add Turris Mox rWTM firmware driver")

You have put a fixes tag here, meaning you want it in stable? How does
dead code elimination fulfil the stable requirements?

Do any of these changes contain real fixes?

   Andrew
Marek Behún May 5, 2021, 4:04 p.m. UTC | #2 
On Mon, 3 May 2021 14:22:49 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Thu, Apr 29, 2021 at 10:36:31AM +0200, Pali Rohár wrote:
> > From: Marek Behún <kabel@kernel.org>
> > 
> > The status decoding function mox_get_status() currently contains a dead
> > code path: if the error status is not MBOX_STS_SUCCESS, it always
> > returns -EIO, so the comparison to MBOX_STS_FAIL is never executed and
> > we don't get the actual error code sent by the firmware.
> > 
> > Fix this.
> > 
> > Signed-off-by: Marek Behún <kabel@kernel.org>
> > Fixes: 389711b37493 ("firmware: Add Turris Mox rWTM firmware driver")  
> 
> You have put a fixes tag here, meaning you want it in stable? How does
> dead code elimination fulfil the stable requirements?
> 
> Do any of these changes contain real fixes?
> 
>    Andrew

Andrew, this is not dead code elimination. Rather it is that there is
dead code path due to an incorrect check. By correcting the check, the
dead code path becomes alive and starts reporting errors correctly.
This fix is nedeed in stable so that stable will report errors
correctly.

Marek
Andrew Lunn May 5, 2021, 4:20 p.m. UTC | #3 
On Wed, May 05, 2021 at 06:04:33PM +0200, Marek Behún wrote:
> On Mon, 3 May 2021 14:22:49 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > On Thu, Apr 29, 2021 at 10:36:31AM +0200, Pali Rohár wrote:
> > > From: Marek Behún <kabel@kernel.org>
> > > 
> > > The status decoding function mox_get_status() currently contains a dead
> > > code path: if the error status is not MBOX_STS_SUCCESS, it always
> > > returns -EIO, so the comparison to MBOX_STS_FAIL is never executed and
> > > we don't get the actual error code sent by the firmware.
> > > 
> > > Fix this.
> > > 
> > > Signed-off-by: Marek Behún <kabel@kernel.org>
> > > Fixes: 389711b37493 ("firmware: Add Turris Mox rWTM firmware driver")  
> > 
> > You have put a fixes tag here, meaning you want it in stable? How does
> > dead code elimination fulfil the stable requirements?
> > 
> > Do any of these changes contain real fixes?
> > 
> >    Andrew
> 
> Andrew, this is not dead code elimination.

Please word you commit message differently.

The status decoding function mox_get_status() currently contains an
incorrect check: ...

	  Andrew
Pali Rohár May 11, 2021, 9:46 p.m. UTC | #4 
On Wednesday 05 May 2021 18:20:46 Andrew Lunn wrote:
> On Wed, May 05, 2021 at 06:04:33PM +0200, Marek Behún wrote:
> > On Mon, 3 May 2021 14:22:49 +0200
> > Andrew Lunn <andrew@lunn.ch> wrote:
> > 
> > > On Thu, Apr 29, 2021 at 10:36:31AM +0200, Pali Rohár wrote:
> > > > From: Marek Behún <kabel@kernel.org>
> > > > 
> > > > The status decoding function mox_get_status() currently contains a dead
> > > > code path: if the error status is not MBOX_STS_SUCCESS, it always
> > > > returns -EIO, so the comparison to MBOX_STS_FAIL is never executed and
> > > > we don't get the actual error code sent by the firmware.
> > > > 
> > > > Fix this.
> > > > 
> > > > Signed-off-by: Marek Behún <kabel@kernel.org>
> > > > Fixes: 389711b37493 ("firmware: Add Turris Mox rWTM firmware driver")  
> > > 
> > > You have put a fixes tag here, meaning you want it in stable? How does
> > > dead code elimination fulfil the stable requirements?
> > > 
> > > Do any of these changes contain real fixes?
> > > 
> > >    Andrew
> > 
> > Andrew, this is not dead code elimination.
> 
> Please word you commit message differently.
> 
> The status decoding function mox_get_status() currently contains an
> incorrect check: ...
> 
> 	  Andrew

Andrew, Marek has already updated commit message and I have sent a new
version v3 of this patch series with this update. It is OK now?
diff mbox series

Patch

diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
index 62f0d1a5dd32..f85acdb3130c 100644
--- a/drivers/firmware/turris-mox-rwtm.c
+++ b/drivers/firmware/turris-mox-rwtm.c
@@ -147,11 +147,14 @@  MOX_ATTR_RO(pubkey, "%s\n", pubkey);
 
 static int mox_get_status(enum mbox_cmd cmd, u32 retval)
 {
-	if (MBOX_STS_CMD(retval) != cmd ||
-	    MBOX_STS_ERROR(retval) != MBOX_STS_SUCCESS)
+	if (MBOX_STS_CMD(retval) != cmd)
 		return -EIO;
 	else if (MBOX_STS_ERROR(retval) == MBOX_STS_FAIL)
 		return -(int)MBOX_STS_VALUE(retval);
+	else if (MBOX_STS_ERROR(retval) == MBOX_STS_BADCMD)
+		return -ENOSYS;
+	else if (MBOX_STS_ERROR(retval) != MBOX_STS_SUCCESS)
+		return -EIO;
 	else
 		return MBOX_STS_VALUE(retval);
 }