firmware: turris-mox-rwtm: fix reply status decoding function

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.

---
 drivers/firmware/turris-mox-rwtm.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

index 50bb2a6d6ccf..54b98642ee1b 100644
@@ -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);
 }

2.26.2


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Report a notice level message if a command is not supported by the rWTM firmware.

This should not be an error, merely a notice, because the firmware can be used on non-CZ.NIC boards that do not have manufacturing information burned.

---
 drivers/firmware/turris-mox-rwtm.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

index 54b98642ee1b..0f9e40a28fb0 100644
@@ -204,11 +204,14 @@ static int mox_get_board_info(struct mox_rwtm *rwtm)
 		return ret;
 
 	ret = mox_get_status(MBOX_CMD_BOARD_INFO, reply->retval);
-	if (ret < 0 && ret != -ENODATA) {
-		return ret;
-	} else if (ret == -ENODATA) {
+	if (ret == -ENODATA) {
 		dev_warn(rwtm->dev,
 			 "Board does not have manufacturing information burned!\n");
+	} else if (ret == -ENOSYS) {
+		dev_notice(rwtm->dev,
+			   "Firmware does not support the BOARD_INFO command\n");
+	} else if (ret < 0) {
+		return ret;
 	} else {
 		rwtm->serial_number = reply->status[1];
 		rwtm->serial_number <<= 32;

@@ -237,10 +240,13 @@ static int mox_get_board_info(struct mox_rwtm *rwtm)
 		return ret;
 
 	ret = mox_get_status(MBOX_CMD_ECDSA_PUB_KEY, reply->retval);
-	if (ret < 0 && ret != -ENODATA) {
-		return ret;
-	} else if (ret == -ENODATA) {
+	if (ret == -ENODATA) {
 		dev_warn(rwtm->dev, "Board has no public key burned!\n");
+	} else if (ret == -ENOSYS) {
+		dev_notice(rwtm->dev,
+			   "Firmware does not support the ECDSA_PUB_KEY command\n");
+	} else if (ret < 0) {
+		return ret;
 	} else {
 		u32 *s = reply->status;
 

2.26.2


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

From: Pali Rohár <[email protected]>

When Marvell's rWTM firmware, which does not support the GET_RANDOM command, is used, kernel prints an error message
hwrng: no data available
every 10 seconds.

Fail probing of this driver if the rWTM firmware does not support the GET_RANDOM command.

This makes it possible to put this driver's compatible into generic armada-37xx device tree, to be available for other Armada 3720 devices besides Turris MOX. If they use the rWTM firmware from CZ.NIC, they will have HWRNG available, and if not, the driver won't be complaining.

---
 drivers/firmware/turris-mox-rwtm.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

index 0f9e40a28fb0..2b56dd05961f 100644
@@ -260,6 +260,27 @@ static int mox_get_board_info(struct mox_rwtm *rwtm)
 	return 0;
 }
 
+static int check_get_random_support(struct mox_rwtm *rwtm)
+{
+	struct armada_37xx_rwtm_tx_msg msg;
+	int ret;
+
+	msg.command = MBOX_CMD_GET_RANDOM;
+	msg.args[0] = 1;
+	msg.args[1] = rwtm->buf_phys;
+	msg.args[2] = 4;
+
+	ret = mbox_send_message(rwtm->mbox, &msg);
+	if (ret < 0)
+		return ret;
+
+	ret = wait_for_completion_timeout(&rwtm->cmd_done, HZ / 2);
+	if (ret < 0)
+		return ret;
+
+	return mox_get_status(MBOX_CMD_GET_RANDOM, rwtm->reply.retval);
+}
+
 static int mox_hwrng_read(struct hwrng *rng, void *data, size_t max, bool wait)
 {
 	struct mox_rwtm *rwtm = (struct mox_rwtm *) rng->priv;

@@ -497,6 +518,13 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 	if (ret < 0)
 		dev_warn(dev, "Cannot read board information: %i\n", ret);
 
+	ret = check_get_random_support(rwtm);
+	if (ret < 0) {
+		dev_notice(dev,
+			   "Firmware does not support the GET_RANDOM command\n");
+		goto free_channel;
+	}
+
 	rwtm->hwrng.name = DRIVER_NAME "_hwrng";
 	rwtm->hwrng.read = mox_hwrng_read;
 	rwtm->hwrng.priv = (unsigned long) rwtm;

2.26.2


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

From: Pali Rohár <[email protected]>

Move the turris-mox-rwtm firmware node from Turris MOX' device tree into the generic armada-37xx.dtsi file.

The Turris MOX rWTM firmware can be used on any Armada 37xx device, giving them access to the rWTM hardware random number generator, which is otherwise unavailable.

This change allows Linux to load the turris-mox-rwtm.ko module on these boards.

Tested on ESPRESSObin v5 with both default Marvell WTMI firmware and CZ.NIC's firmware. With default WTMI firmware the turris-mox-rwtm fails to probe, while with CZ.NIC's firmware it registers the HW random number generator.

---
 arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts | 8 --------
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi           | 8 ++++++++
 2 files changed, 8 insertions(+), 8 deletions(-)

index d239ab70ed99..8447f303a294 100644
@@ -107,14 +107,6 @@ sfp: sfp {
 		/* enabled by U-Boot if SFP module is present */
 		status = "disabled";
 	};
-
-	firmware {
-		turris-mox-rwtm {
-			compatible = "cznic,turris-mox-rwtm";
-			mboxes = <&rwtm 0>;
-			status = "okay";
-		};
-	};
 };
 
 &i2c0 {

index 7a2df148c6a3..31126f1ffe5b 100644
@@ -503,4 +503,12 @@ pcie_intc: interrupt-controller {
 			};
 		};
 	};
+
+	firmware {
+		turris-mox-rwtm {
+			compatible = "cznic,turris-mox-rwtm";
+			mboxes = <&rwtm 0>;
+			status = "okay";
+		};
+	};
 };

2.26.2

Hello Marek,

I disagree with this patch. This firmware is specific to Turris MOX so it is not something that should be exposed to all the Armada 3700 based boards.

If you want you still can create an dtsi for this and include it when needed.

Gregory

Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

Gregory, we are planning to send pull-request for TF-A documentation, adding information that people can compile the firmware with CZ.NIC's firmware.

Since this firmware exposes HW random number generator, it is possible that people will start using it for espressobin.

In that case this won't be specific for Turris MOX anymore.

Maybe we should add another compatible to the turris-mox-rwtm driver?

Marek

Part of the problem is that it looks specific to the Turris MOX.

But please help me understand the big picture first. How is the firmware distributed? Is the binary part of linux-firmware? How does it get loaded? Does the firmware contain anything which is specific to the Turris MOX? Could the hardware number generator part be split out into a more generic sounding name blob?

Andrew

Hello Andrew,

The WTMI firmware is loaded before kernel. This firmware is loaded by BootROM, and it is this firmware that does DDR training before loading TF-A + U-Boot.

For example for ESPRESSObin you have several repositories you need to create a final flash-image.bin containing this WTMI firmware + TF-A + U-Boot. These repositories are:
trusted-firmware-a (contains documentation how to build all this) A3700-utils-marvell
u-boot
mv-ddr-marvell
From these sources you are able to create a final flash-image.bin that you can flash onto the SPI-NOR (or eMMC or other devices which A3720 can boot from).

The A3700-utils-marvell repository contains the code of the WTMI firmware.

On Turris MOX this is a little bit different, because
- we have implemented the WTMI firmware differently (more mailbox commands, HW crypto, ...)
- it supports retrieving MOX board information (MAC addresses, serial number) stored in eFuses (this information is stored in a specific way that in only true for MOX)
- the firmware binary must be signed by our private key in order to boot on MOX.

This is because the secure firmware has access to an ECDSA engine with a private key storage in eFuses (each MOX has its own private key generated and stored into the eFuses when manufactured) In order to disallow hackers to somehow extract the private key, the firmware must be signed so that they cannot load arbitrary firmware into the secure processor.

BUT
- since this firmware is able to provide HWRNG, we wanted to make it available for other Armada 3720 boards
- we updated the code so that users can build it for non-MOX devices
- it does not have to be signed for other devices
- it does not contain MOX specific stuff for non-MOX devices

So currently when users build the flash-image.bin binary containing WTMI firmware, they are using code from A3700-utils-marvell. This code is split into 2 parts:
- sys_init - does HW and DDR initialization and execution of an "app"
- efuse - default "app" which is loaded by sys_init
The way it is written is that user can select a different "app" when building, and we have updated Turris MOX firmware code to be loadable as this "app" for sys_init. (And we have renamed it from "Turris MOX secure firmware" to "CZ.NIC's Armada 3720 secure firmware").

It basically is. As I have written above, when users build the flash-image.bin with CZ.NIC's firmware, the prompt does not say anything about Turris MOX. Instead it says something like
CZ.NIC's Armada 3720 Secure Firmware version build date Running on ESPRESSObin
and currently provides only the random number generator command.

So theoretically the turris-mox-rwtm driver can be renamed into something else and we can add a different compatible in order not to sound so turris-mox specific.

Marek

That would be a good idea. And if possible, try to push the hardware random number code upstream in the firmware repos, so everybody gets it by default, not just those using your build. Who is responsible for upstream? Marvell?

Andrew

Hello Andrew! The issue is that upstream Marvell repository contains only 'fuse.bin' application which is suitable only for fuse programming. I think it is not correct if this Marvell fuse application start providing other functionality not relevant to fuse programming. And Marvell does not provide any other application (publicly). So it would be needed to send it as another application, not part of 'fuse.bin'. And then it complicates build system and compile options, which is already too complicated (you need to set tons of TF-A options and prepare two sets of cross compile toolchains).

But because application / firmware for MOX / Armada 3720 is actively developed on different place, I do not think that it make sense to send every change to two different locations (and wait for Marvell until review every change and include it into their repository). Such thing just increase maintenance cost at both sides.

For me it looks like a better solution to provide 'wtmi_app.bin' application with HW number generator from separate repository, where it is currently developed and where it is available for a longer time.

We are planning to send documentation update to Trusted-Firmware project to specify how to build Armada 3720 firmware image with our application. So people who are building Armada 3720 firmware would be able to switch from Marvell's 'fuse.bin' application to our 'wtmi_app.bin'.

Why not? We can rename it to fuse+hwrng and implement hwrng there. Maybe Konstantin will agree with such a change :)

This is a little bit better argument than the previous one. But I think Andrew may be right in that for end-users it just complicates things if they have more options. Better to give them one option.

Hello Andrew!

I do not think that renaming driver is the best option. For future it would complicate backporting patches to stable kernel and also it would make usage of 'gitk' harder as this tool cannot automatically track file renames.

I saw that kernel drivers lot of times got support for a new HW but driver name was not changed (maybe for backward compatibility, maybe to simplify things).

Andrew, now when you got a full picture of this problem, would you be willing to accept these patches? Or do you require some changes?

Hi Pali

I'm not suggesting renaming the .c file.

What would be good is to add additional compatible strings. Add a more generic compatible. What goes into the .dtsi should use the generic name. Also, the node names should also be generic, since the node name is probably not used anywhere, just the compatible string. Keep the current compatible in the driver, for backwards compatibility with older DT blobs.

Andrew

Ok! What about compatible string "marvell,armada-3700-rwtm-firmware"?

Mailbox layer which is used by this driver has compatible string "marvell,armada-3700-rwtm-mailbox", so name is similar.

Yes, that is O.K.

Andrew

From: Marek Behún <[email protected]>

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.

---
 drivers/firmware/turris-mox-rwtm.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

index 62f0d1a5dd32..f85acdb3130c 100644
@@ -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);
 }

2.20.1

From: Marek Behún <[email protected]>

Report a notice level message if a command is not supported by the rWTM firmware.

This should not be an error, merely a notice, because the firmware can be used on non-CZ.NIC boards that do not have manufacturing information burned.

---
 drivers/firmware/turris-mox-rwtm.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

index f85acdb3130c..d7e3489e4bf2 100644
@@ -204,11 +204,14 @@ static int mox_get_board_info(struct mox_rwtm *rwtm)
 		return ret;
 
 	ret = mox_get_status(MBOX_CMD_BOARD_INFO, reply->retval);
-	if (ret < 0 && ret != -ENODATA) {
-		return ret;
-	} else if (ret == -ENODATA) {
+	if (ret == -ENODATA) {
 		dev_warn(rwtm->dev,
 			 "Board does not have manufacturing information burned!\n");
+	} else if (ret == -ENOSYS) {
+		dev_notice(rwtm->dev,
+			   "Firmware does not support the BOARD_INFO command\n");
+	} else if (ret < 0) {
+		return ret;
 	} else {
 		rwtm->serial_number = reply->status[1];
 		rwtm->serial_number <<= 32;

@@ -237,10 +240,13 @@ static int mox_get_board_info(struct mox_rwtm *rwtm)
 		return ret;
 
 	ret = mox_get_status(MBOX_CMD_ECDSA_PUB_KEY, reply->retval);
-	if (ret < 0 && ret != -ENODATA) {
-		return ret;
-	} else if (ret == -ENODATA) {
+	if (ret == -ENODATA) {
 		dev_warn(rwtm->dev, "Board has no public key burned!\n");
+	} else if (ret == -ENOSYS) {
+		dev_notice(rwtm->dev,
+			   "Firmware does not support the ECDSA_PUB_KEY command\n");
+	} else if (ret < 0) {
+		return ret;
 	} else {
 		u32 *s = reply->status;
 

2.20.1

When Marvell's rWTM firmware, which does not support the GET_RANDOM command, is used, kernel prints an error message
hwrng: no data available
every 10 seconds.

Fail probing of this driver if the rWTM firmware does not support the GET_RANDOM command.

This makes it possible to put this driver's compatible into generic armada-37xx device tree, to be available for other Armada 3720 devices besides Turris MOX. If they use the rWTM firmware from CZ.NIC, they will have HWRNG available, and if not, the driver won't be complaining.

---
 drivers/firmware/turris-mox-rwtm.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

index d7e3489e4bf2..3ef9687dddca 100644
@@ -260,6 +260,27 @@ static int mox_get_board_info(struct mox_rwtm *rwtm)
 	return 0;
 }
 
+static int check_get_random_support(struct mox_rwtm *rwtm)
+{
+	struct armada_37xx_rwtm_tx_msg msg;
+	int ret;
+
+	msg.command = MBOX_CMD_GET_RANDOM;
+	msg.args[0] = 1;
+	msg.args[1] = rwtm->buf_phys;
+	msg.args[2] = 4;
+
+	ret = mbox_send_message(rwtm->mbox, &msg);
+	if (ret < 0)
+		return ret;
+
+	ret = wait_for_completion_timeout(&rwtm->cmd_done, HZ / 2);
+	if (ret < 0)
+		return ret;
+
+	return mox_get_status(MBOX_CMD_GET_RANDOM, rwtm->reply.retval);
+}
+
 static int mox_hwrng_read(struct hwrng *rng, void *data, size_t max, bool wait)
 {
 	struct mox_rwtm *rwtm = (struct mox_rwtm *) rng->priv;

@@ -497,6 +518,13 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 	if (ret < 0)
 		dev_warn(dev, "Cannot read board information: %i\n", ret);
 
+	ret = check_get_random_support(rwtm);
+	if (ret < 0) {
+		dev_notice(dev,
+			   "Firmware does not support the GET_RANDOM command\n");
+		goto free_channel;
+	}
+
 	rwtm->hwrng.name = DRIVER_NAME "_hwrng";
 	rwtm->hwrng.read = mox_hwrng_read;
 	rwtm->hwrng.priv = (unsigned long) rwtm;

2.20.1

Currently it is hard to determinate if on Armada 3720 device is HWRNG by running kernel accessible or not. So print information message into dmesg when HWRNG is available and registration was successful.

---
 drivers/firmware/turris-mox-rwtm.c | 2 ++
 1 file changed, 2 insertions(+)

index 3ef9687dddca..1cf4f1087492 100644
@@ -542,6 +542,8 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 		goto free_channel;
 	}
 
+	dev_info(dev, "HWRNG successfully registered\n");
+
 	return 0;
 
 free_channel:

2.20.1

Add more generic compatible string 'marvell,armada-3700-rwtm-firmware' for this driver, since it can also be used on other Armada 3720 devices.

Current compatible string 'cznic,turris-mox-rwtm' is kept for backward compatibility.

---
We are also planning to work on extending this driver to support accessing OTP, which will also work with Marvell's default WTMI firmware.

---
 drivers/firmware/turris-mox-rwtm.c | 1 +
 1 file changed, 1 insertion(+)

index 1cf4f1087492..c2d34dc8ba46 100644
@@ -569,6 +569,7 @@ static int turris_mox_rwtm_remove(struct platform_device *pdev)
 
 static const struct of_device_id turris_mox_rwtm_match[] = {
 	{ .compatible = "cznic,turris-mox-rwtm", },
+	{ .compatible = "marvell,armada-3700-rwtm-firmware", },
 	{ },
 };
 

2.20.1

Move the turris-mox-rwtm firmware node from Turris MOX' device tree into the generic armada-37xx.dtsi file and use the generic compatible string 'marvell,armada-3700-rwtm-firmware' instead of the current one.

The Turris MOX rWTM firmware can be used on any Armada 37xx device, giving them access to the rWTM hardware random number generator, which is otherwise unavailable.

This change allows Linux to load the turris-mox-rwtm.ko module on these boards.

Tested on ESPRESSObin v5 with both default Marvell WTMI firmware and CZ.NIC's firmware. With default WTMI firmware the turris-mox-rwtm fails to probe, while with CZ.NIC's firmware it registers the HW random number generator.

---
 arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts | 8 --------
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi           | 8 ++++++++
 2 files changed, 8 insertions(+), 8 deletions(-)

index 0753cc489638..ebb0ddf8d306 100644
@@ -107,14 +107,6 @@
 		/* enabled by U-Boot if SFP module is present */
 		status = "disabled";
 	};
-
-	firmware {
-		turris-mox-rwtm {
-			compatible = "cznic,turris-mox-rwtm";
-			mboxes = <&rwtm 0>;
-			status = "okay";
-		};
-	};
 };
 
 &i2c0 {

index 1b7f43e27589..847a2d12a4be 100644
@@ -505,4 +505,12 @@
 			};
 		};
 	};
+
+	firmware {
+		armada-3700-rwtm {
+			compatible = "marvell,armada-3700-rwtm-firmware";
+			mboxes = <&rwtm 0>;
+			status = "okay";
+		};
+	};
 };

2.20.1

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

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?

From: Marek Behún <[email protected]>

The status decoding function mox_get_status() currently contains an incorrect check: 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.

---
 drivers/firmware/turris-mox-rwtm.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

index 62f0d1a5dd32..f85acdb3130c 100644
@@ -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);
 }

2.20.1

From: Marek Behún <[email protected]>

Report a notice level message if a command is not supported by the rWTM firmware.

This should not be an error, merely a notice, because the firmware can be used on non-CZ.NIC boards that do not have manufacturing information burned.

---
 drivers/firmware/turris-mox-rwtm.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

index f85acdb3130c..d7e3489e4bf2 100644
@@ -204,11 +204,14 @@ static int mox_get_board_info(struct mox_rwtm *rwtm)
 		return ret;
 
 	ret = mox_get_status(MBOX_CMD_BOARD_INFO, reply->retval);
-	if (ret < 0 && ret != -ENODATA) {
-		return ret;
-	} else if (ret == -ENODATA) {
+	if (ret == -ENODATA) {
 		dev_warn(rwtm->dev,
 			 "Board does not have manufacturing information burned!\n");
+	} else if (ret == -ENOSYS) {
+		dev_notice(rwtm->dev,
+			   "Firmware does not support the BOARD_INFO command\n");
+	} else if (ret < 0) {
+		return ret;
 	} else {
 		rwtm->serial_number = reply->status[1];
 		rwtm->serial_number <<= 32;

@@ -237,10 +240,13 @@ static int mox_get_board_info(struct mox_rwtm *rwtm)
 		return ret;
 
 	ret = mox_get_status(MBOX_CMD_ECDSA_PUB_KEY, reply->retval);
-	if (ret < 0 && ret != -ENODATA) {
-		return ret;
-	} else if (ret == -ENODATA) {
+	if (ret == -ENODATA) {
 		dev_warn(rwtm->dev, "Board has no public key burned!\n");
+	} else if (ret == -ENOSYS) {
+		dev_notice(rwtm->dev,
+			   "Firmware does not support the ECDSA_PUB_KEY command\n");
+	} else if (ret < 0) {
+		return ret;
 	} else {
 		u32 *s = reply->status;
 

2.20.1

When Marvell's rWTM firmware, which does not support the GET_RANDOM command, is used, kernel prints an error message
hwrng: no data available
every 10 seconds.

Fail probing of this driver if the rWTM firmware does not support the GET_RANDOM command.

This makes it possible to put this driver's compatible into generic armada-37xx device tree, to be available for other Armada 3720 devices besides Turris MOX. If they use the rWTM firmware from CZ.NIC, they will have HWRNG available, and if not, the driver won't be complaining.

---
 drivers/firmware/turris-mox-rwtm.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

index d7e3489e4bf2..3ef9687dddca 100644
@@ -260,6 +260,27 @@ static int mox_get_board_info(struct mox_rwtm *rwtm)
 	return 0;
 }
 
+static int check_get_random_support(struct mox_rwtm *rwtm)
+{
+	struct armada_37xx_rwtm_tx_msg msg;
+	int ret;
+
+	msg.command = MBOX_CMD_GET_RANDOM;
+	msg.args[0] = 1;
+	msg.args[1] = rwtm->buf_phys;
+	msg.args[2] = 4;
+
+	ret = mbox_send_message(rwtm->mbox, &msg);
+	if (ret < 0)
+		return ret;
+
+	ret = wait_for_completion_timeout(&rwtm->cmd_done, HZ / 2);
+	if (ret < 0)
+		return ret;
+
+	return mox_get_status(MBOX_CMD_GET_RANDOM, rwtm->reply.retval);
+}
+
 static int mox_hwrng_read(struct hwrng *rng, void *data, size_t max, bool wait)
 {
 	struct mox_rwtm *rwtm = (struct mox_rwtm *) rng->priv;

@@ -497,6 +518,13 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 	if (ret < 0)
 		dev_warn(dev, "Cannot read board information: %i\n", ret);
 
+	ret = check_get_random_support(rwtm);
+	if (ret < 0) {
+		dev_notice(dev,
+			   "Firmware does not support the GET_RANDOM command\n");
+		goto free_channel;
+	}
+
 	rwtm->hwrng.name = DRIVER_NAME "_hwrng";
 	rwtm->hwrng.read = mox_hwrng_read;
 	rwtm->hwrng.priv = (unsigned long) rwtm;

2.20.1

We start getting into questions of if this is relevant for stable. Running Turris MOX with plain Marvell rWTM will spam the log. So i would say it is a fix, for that situation.

However, i would drop the second part of the description. Making the Turris MOX firmware work on none Turris MOX hardware is a new feature, and so not applicable to stable.

Andrew

Done in v4.

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Currently it is hard to determinate if on Armada 3720 device is HWRNG by running kernel accessible or not. So print information message into dmesg when HWRNG is available and registration was successful.

---
 drivers/firmware/turris-mox-rwtm.c | 2 ++
 1 file changed, 2 insertions(+)

index 3ef9687dddca..1cf4f1087492 100644
@@ -542,6 +542,8 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 		goto free_channel;
 	}
 
+	dev_info(dev, "HWRNG successfully registered\n");
+
 	return 0;
 
 free_channel:

2.20.1

Add more generic compatible string 'marvell,armada-3700-rwtm-firmware' for this driver, since it can also be used on other Armada 3720 devices.

Current compatible string 'cznic,turris-mox-rwtm' is kept for backward compatibility.

---
We are also planning to work on extending this driver to support accessing OTP, which will also work with Marvell's default WTMI firmware.

---
 drivers/firmware/turris-mox-rwtm.c | 1 +
 1 file changed, 1 insertion(+)

index 1cf4f1087492..c2d34dc8ba46 100644
@@ -569,6 +569,7 @@ static int turris_mox_rwtm_remove(struct platform_device *pdev)
 
 static const struct of_device_id turris_mox_rwtm_match[] = {
 	{ .compatible = "cznic,turris-mox-rwtm", },
+	{ .compatible = "marvell,armada-3700-rwtm-firmware", },
 	{ },
 };
 

2.20.1

Move the turris-mox-rwtm firmware node from Turris MOX' device tree into the generic armada-37xx.dtsi file and use the generic compatible string 'marvell,armada-3700-rwtm-firmware' instead of the current one.

The Turris MOX rWTM firmware can be used on any Armada 37xx device, giving them access to the rWTM hardware random number generator, which is otherwise unavailable.

This change allows Linux to load the turris-mox-rwtm.ko module on these boards.

Tested on ESPRESSObin v5 with both default Marvell WTMI firmware and CZ.NIC's firmware. With default WTMI firmware the turris-mox-rwtm fails to probe, while with CZ.NIC's firmware it registers the HW random number generator.

---
 arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts | 8 --------
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi           | 8 ++++++++
 2 files changed, 8 insertions(+), 8 deletions(-)

index 0753cc489638..ebb0ddf8d306 100644
@@ -107,14 +107,6 @@
 		/* enabled by U-Boot if SFP module is present */
 		status = "disabled";
 	};
-
-	firmware {
-		turris-mox-rwtm {
-			compatible = "cznic,turris-mox-rwtm";
-			mboxes = <&rwtm 0>;
-			status = "okay";
-		};
-	};
 };
 
 &i2c0 {

index 1b7f43e27589..847a2d12a4be 100644
@@ -505,4 +505,12 @@
 			};
 		};
 	};
+
+	firmware {
+		armada-3700-rwtm {
+			compatible = "marvell,armada-3700-rwtm-firmware";
+			mboxes = <&rwtm 0>;
+			status = "okay";
+		};
+	};
 };

2.20.1

This is a new feature, not a fix. Please split this patchset up into fixes, which get applied to stable, and new features which will get merged during the next merge window.

Thanks
Andrew

Done, I have split patches into two patch series, so they can be reviewed and merged separately.

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Andrew

These 4 patches are just fixups. Per Andrew's request I have split them from V3 series, so they can be applied to stable.

Marek Behún (2):
firmware: turris-mox-rwtm: fix reply status decoding function
firmware: turris-mox-rwtm: report failures better

Pali Rohár (2):
firmware: turris-mox-rwtm: fail probing when firmware does not support
hwrng
firmware: turris-mox-rwtm: show message about HWRNG registration

 drivers/firmware/turris-mox-rwtm.c | 55 +++++++++++++++++++++++++-----
 1 file changed, 47 insertions(+), 8 deletions(-)

2.20.1


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

From: Marek Behún <[email protected]>

The status decoding function mox_get_status() currently contains an incorrect check: 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.

---
 drivers/firmware/turris-mox-rwtm.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

index 62f0d1a5dd32..f85acdb3130c 100644
@@ -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);
 }

2.20.1


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

From: Marek Behún <[email protected]>

Report a notice level message if a command is not supported by the rWTM firmware.

This should not be an error, merely a notice, because the firmware can be used on boards that do not have manufacturing information burned.

---
 drivers/firmware/turris-mox-rwtm.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

index f85acdb3130c..d7e3489e4bf2 100644
@@ -204,11 +204,14 @@ static int mox_get_board_info(struct mox_rwtm *rwtm)
 		return ret;
 
 	ret = mox_get_status(MBOX_CMD_BOARD_INFO, reply->retval);
-	if (ret < 0 && ret != -ENODATA) {
-		return ret;
-	} else if (ret == -ENODATA) {
+	if (ret == -ENODATA) {
 		dev_warn(rwtm->dev,
 			 "Board does not have manufacturing information burned!\n");
+	} else if (ret == -ENOSYS) {
+		dev_notice(rwtm->dev,
+			   "Firmware does not support the BOARD_INFO command\n");
+	} else if (ret < 0) {
+		return ret;
 	} else {
 		rwtm->serial_number = reply->status[1];
 		rwtm->serial_number <<= 32;

@@ -237,10 +240,13 @@ static int mox_get_board_info(struct mox_rwtm *rwtm)
 		return ret;
 
 	ret = mox_get_status(MBOX_CMD_ECDSA_PUB_KEY, reply->retval);
-	if (ret < 0 && ret != -ENODATA) {
-		return ret;
-	} else if (ret == -ENODATA) {
+	if (ret == -ENODATA) {
 		dev_warn(rwtm->dev, "Board has no public key burned!\n");
+	} else if (ret == -ENOSYS) {
+		dev_notice(rwtm->dev,
+			   "Firmware does not support the ECDSA_PUB_KEY command\n");
+	} else if (ret < 0) {
+		return ret;
 	} else {
 		u32 *s = reply->status;
 

2.20.1


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Andrew

When Marvell's rWTM firmware, which does not support the GET_RANDOM command, is used, kernel prints an error message
hwrng: no data available
every 10 seconds.

Fail probing of this driver if the rWTM firmware does not support the GET_RANDOM command.

---
 drivers/firmware/turris-mox-rwtm.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

index d7e3489e4bf2..3ef9687dddca 100644
@@ -260,6 +260,27 @@ static int mox_get_board_info(struct mox_rwtm *rwtm)
 	return 0;
 }
 
+static int check_get_random_support(struct mox_rwtm *rwtm)
+{
+	struct armada_37xx_rwtm_tx_msg msg;
+	int ret;
+
+	msg.command = MBOX_CMD_GET_RANDOM;
+	msg.args[0] = 1;
+	msg.args[1] = rwtm->buf_phys;
+	msg.args[2] = 4;
+
+	ret = mbox_send_message(rwtm->mbox, &msg);
+	if (ret < 0)
+		return ret;
+
+	ret = wait_for_completion_timeout(&rwtm->cmd_done, HZ / 2);
+	if (ret < 0)
+		return ret;
+
+	return mox_get_status(MBOX_CMD_GET_RANDOM, rwtm->reply.retval);
+}
+
 static int mox_hwrng_read(struct hwrng *rng, void *data, size_t max, bool wait)
 {
 	struct mox_rwtm *rwtm = (struct mox_rwtm *) rng->priv;

@@ -497,6 +518,13 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 	if (ret < 0)
 		dev_warn(dev, "Cannot read board information: %i\n", ret);
 
+	ret = check_get_random_support(rwtm);
+	if (ret < 0) {
+		dev_notice(dev,
+			   "Firmware does not support the GET_RANDOM command\n");
+		goto free_channel;
+	}
+
 	rwtm->hwrng.name = DRIVER_NAME "_hwrng";
 	rwtm->hwrng.read = mox_hwrng_read;
 	rwtm->hwrng.priv = (unsigned long) rwtm;

2.20.1


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Andrew

Currently it is hard to determinate if on Armada 3720 device is HWRNG by running kernel accessible or not. So print information message into dmesg when HWRNG is available and registration was successful.

---
 drivers/firmware/turris-mox-rwtm.c | 2 ++
 1 file changed, 2 insertions(+)

index 3ef9687dddca..1cf4f1087492 100644
@@ -542,6 +542,8 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 		goto free_channel;
 	}
 
+	dev_info(dev, "HWRNG successfully registered\n");
+
 	return 0;
 
 free_channel:

2.20.1


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Andrew

Hello,

Series applied on mvebu/fixes

Thanks,

Gregory

Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

Hello Gregory!

I see that you put this patch into mvebu/fixes branch and tagged it for 5.13 kernel:
https://git.kernel.org/pub/scm/linux/kernel/git/gclement/mvebu.git/tag/?h=mvebu-fixes-5.13-1

But it looks like it was not merged into 5.13. Are you going to re-send all pending patches to 5.14?

Hello Pali,

They have been merged in 5.14 during merged windows and they are now also in 5.13.4. So I think everything is OK now.

Gregory

Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Yes! I already figured out. So everything is fine now.

These two patches add support for using turris-mox-rwtm driver on any Armada 3720 board. They are just adding a new compatible string.

Per Andrew's request I have split them from V3 series, so they can be reviewed separately.

These patches are intended for next merge window.

I suppose that in future these patches may be backported into stable kernel releases as in my opinion they match stable rules condition:
"New device IDs and quirks are also accepted". New compatible string is basically new device id. Therefore I put into these patches also Fixes/CC:stable tags. If you have different opinion on this, let me know.

Pali Rohár (2):
firmware: turris-mox-rwtm: add marvell,armada-3700-rwtm-firmware
compatible string
arm64: dts: marvell: armada-37xx: move firmware node to generic dtsi
file

 arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts | 6 ++----
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi           | 8 ++++++++
 drivers/firmware/turris-mox-rwtm.c                     | 1 +
 3 files changed, 11 insertions(+), 4 deletions(-)

2.20.1


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Add more generic compatible string 'marvell,armada-3700-rwtm-firmware' for this driver, since it can also be used on other Armada 3720 devices.

Current compatible string 'cznic,turris-mox-rwtm' is kept for backward compatibility.

---
 drivers/firmware/turris-mox-rwtm.c | 1 +
 1 file changed, 1 insertion(+)

index 1cf4f1087492..c2d34dc8ba46 100644
@@ -569,6 +569,7 @@ static int turris_mox_rwtm_remove(struct platform_device *pdev)
 
 static const struct of_device_id turris_mox_rwtm_match[] = {
 	{ .compatible = "cznic,turris-mox-rwtm", },
+	{ .compatible = "marvell,armada-3700-rwtm-firmware", },
 	{ },
 };
 

2.20.1


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Since this is intended for the next merge window, it is clearly not a fix. Please drop the fixes tag.

Otherwise:

Andrew

Hello,

Applied on mvebu/drivers (I removed the Fixes line)

Thanks,

Gregory

Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

Move the turris-mox-rwtm firmware node from Turris MOX' device tree into the generic armada-37xx.dtsi file and use the generic compatible string 'marvell,armada-3700-rwtm-firmware' instead of the current one.

Turris MOX DTS file contains also old compatible string for backward compatibility.

The Turris MOX rWTM firmware can be used on any Armada 37xx device, giving them access to the rWTM hardware random number generator, which is otherwise unavailable.

This change allows Linux to load the turris-mox-rwtm.ko module on these boards.

Tested on ESPRESSObin v5 with both default Marvell WTMI firmware and CZ.NIC's firmware. With default WTMI firmware the turris-mox-rwtm fails to probe, while with CZ.NIC's firmware it registers the HW random number generator.

---
 arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts | 6 ++----
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi           | 8 ++++++++
 2 files changed, 10 insertions(+), 4 deletions(-)

index 0753cc489638..6700f2212b61 100644
@@ -109,10 +109,8 @@
 	};
 
 	firmware {
-		turris-mox-rwtm {
-			compatible = "cznic,turris-mox-rwtm";
-			mboxes = <&rwtm 0>;
-			status = "okay";
+		armada-3700-rwtm {
+			compatible = "marvell,armada-3700-rwtm-firmware", "cznic,turris-mox-rwtm";
 		};
 	};
 };

index 1b7f43e27589..847a2d12a4be 100644
@@ -505,4 +505,12 @@
 			};
 		};
 	};
+
+	firmware {
+		armada-3700-rwtm {
+			compatible = "marvell,armada-3700-rwtm-firmware";
+			mboxes = <&rwtm 0>;
+			status = "okay";
+		};
+	};
 };

2.20.1


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Please drop stable. It is a new feature, not a fix.

Otherwise

Andrew

Hello,

Applied on mvebu/dt64 (I removed the Fixes line)

Thanks,

Gregory

Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com