Skip to contentSkip to conversation

Updates for turris-mox-rwtm driver

PATCH Superseded Marek Behún 2 participants 27 messages Jun 17, 2024 – Jul 12, 2024 Arm/cz.nic turris support

v3 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Marek Behún

author maintainer
#0 lore raw

v3 cover Updates for turris-mox-rwtm driver

Hi Gregory, Arnd, Andy et al.

this is v3 of series of fixes, refactors and cleanups for the turris-mox-rwtm driver.

v1 and v2 can be found at:
https://patchwork.kernel.org/project/linux-soc/list/?series=861215
https://patchwork.kernel.org/project/linux-soc/list/?series=861716

Changes since v2:
- use SZ_4K instead of PAGE_SIZE for size of dma buffer
- other small fixes that Andy suggested, like:
- use if(ret) instead of if(ret<0) if return value can't be positive
- use max() from minmax.h
- removed check for -EPROFE_DEFER, it is done by dev_err_probe() internally
- ...

Marek

Marek Behún (17):
firmware: turris-mox-rwtm: Do not complete if there are no waiters
firmware: turris-mox-rwtm: Fix checking return value of
wait_for_completion_timeout()
firmware: turris-mox-rwtm: Use macro constant instead of hardcoded

    4096

firmware: turris-mox-rwtm: Use ETH_ALEN instead of hardcoded 6
firmware: turris-mox-rwtm: Use the boolean type where appropriate
firmware: turris-mox-rwtm: Hide signature related constants behind
macros
firmware: turris-mox-rwtm: Fix driver includes
firmware: turris-mox-rwtm: Don't create own kobject type
firmware: turris-mox-rwtm: Simplify debugfs code
firmware: turris-mox-rwtm: Convert rest to devm_* and get rid of
driver .remove()
firmware: turris-mox-rwtm: Use dev_err_probe() where possible
firmware: turris-mox-rwtm: Initialize completion before mailbox
firmware: turris-mox-rwtm: Drop redundant device pointer
firmware: turris-mox-rwtm: Use devm_mutex_init() instead of
mutex_init()
firmware: turris-mox-rwtm: Use container_of() instead of hwrng .priv
member
firmware: turris-mox-rwtm: Use EOPNOTSUPP instead of ENOSYS
firmware: turris-mox-rwtm: Deduplicate command execution code

 drivers/firmware/turris-mox-rwtm.c | 384 ++++++++++++-----------------
 1 file changed, 155 insertions(+), 229 deletions(-)
Signature · 1 line
2.44.2
author maintainer
#1 lore raw

PATCH v3 1/17 firmware: turris-mox-rwtm: Do not complete if there are no waiters

Do not complete the command done completion if there are no waiters. This can happen if a wait_for_completion timed out or was interrupted.

Fixes 389711b37493 ("firmware: Add Turris Mox rWTM firmware driver")
---
 drivers/firmware/turris-mox-rwtm.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/firmware/turris-mox-rwtm.c +4 -1
index 31d962cdd6eb..f1f9160c4195 100644
@@ -2,7 +2,7 @@
 /*
  * Turris Mox rWTM firmware driver
  *
- * Copyright (C) 2019 Marek Behún <[email protected]>
+ * Copyright (C) 2019, 2024 Marek Behún <[email protected]>
  */
 
 #include <linux/armada-37xx-rwtm-mailbox.h>
@@ -174,6 +174,9 @@ static void mox_rwtm_rx_callback(struct mbox_client *cl, void *data)
 	struct mox_rwtm *rwtm = dev_get_drvdata(cl->dev);
 	struct armada_37xx_rwtm_rx_msg *msg = data;
 
+	if (completion_done(&rwtm->cmd_done))
+		return;
+
 	rwtm->reply = *msg;
 	complete(&rwtm->cmd_done);
 }
Signature · 1 line
2.44.2
#2 lore raw

Re: [PATCH v3 01/17] firmware: turris-mox-rwtm: Do not complete if there are no waiters

Quoted 2 lines · Marek Behún

Do not complete the command done completion if there are no waiters.

"command done"

(perhaps in double quotes, since it's a kind of a name)

Quoted 1 line

This can happen if a wait_for_completion timed out or was interrupted.

wait_for_completion()

...

In case you need a new version, no need to resend because of the above nit-picks.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author maintainer
#3 lore raw

PATCH v3 2/17 firmware: turris-mox-rwtm: Fix checking return value of wait_for_completion_timeout()

In reply to Marek Behún

The wait_for_completion_timeout() function returns 0 if timed out, and a positive value if completed. Fix the usage of this function.

Fixes 389711b37493 ("firmware: Add Turris Mox rWTM firmware driver")
Fixes 2eab59cf0d20 ("firmware: turris-mox-rwtm: fail probing when firmware does not support hwrng")
---
 drivers/firmware/turris-mox-rwtm.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)
drivers/firmware/turris-mox-rwtm.c +6 -9
index f1f9160c4195..3f4758e03c81 100644
@@ -202,9 +202,8 @@ static int mox_get_board_info(struct mox_rwtm *rwtm)
 	if (ret < 0)
 		return ret;
 
-	ret = wait_for_completion_timeout(&rwtm->cmd_done, HZ / 2);
-	if (ret < 0)
-		return ret;
+	if (!wait_for_completion_timeout(&rwtm->cmd_done, HZ / 2))
+		return -ETIMEDOUT;
 
 	ret = mox_get_status(MBOX_CMD_BOARD_INFO, reply->retval);
 	if (ret == -ENODATA) {
@@ -238,9 +237,8 @@ static int mox_get_board_info(struct mox_rwtm *rwtm)
 	if (ret < 0)
 		return ret;
 
-	ret = wait_for_completion_timeout(&rwtm->cmd_done, HZ / 2);
-	if (ret < 0)
-		return ret;
+	if (!wait_for_completion_timeout(&rwtm->cmd_done, HZ / 2))
+		return -ETIMEDOUT;
 
 	ret = mox_get_status(MBOX_CMD_ECDSA_PUB_KEY, reply->retval);
 	if (ret == -ENODATA) {
@@ -277,9 +275,8 @@ static int check_get_random_support(struct mox_rwtm *rwtm)
 	if (ret < 0)
 		return ret;
 
-	ret = wait_for_completion_timeout(&rwtm->cmd_done, HZ / 2);
-	if (ret < 0)
-		return ret;
+	if (!wait_for_completion_timeout(&rwtm->cmd_done, HZ / 2))
+		return -ETIMEDOUT;
 
 	return mox_get_status(MBOX_CMD_GET_RANDOM, rwtm->reply.retval);
 }
Signature · 1 line
2.44.2
author maintainer
#4 lore raw

PATCH v3 3/17 firmware: turris-mox-rwtm: Use macro constant instead of hardcoded 4096

In reply to Marek Behún

The 4096 bytes limit in mox_hwrng_read() is due to the DMA buffer being allocated to one PAGE_SIZE bytes. Use new local macro constant RWTM_DMA_BUFFER_SIZE at allocation time and when used in mox_hwrng_read().

Use SZ_4K instead of PAGE_SIZE. Although PAGE_SIZE is never set to a larger value on Armada 3720, it theoretically could, and this would be a waste of space.

---
 drivers/firmware/turris-mox-rwtm.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
drivers/firmware/turris-mox-rwtm.c +7 -4
index 3f4758e03c81..aafd747543f6 100644
@@ -11,14 +11,18 @@
 #include <linux/dma-mapping.h>
 #include <linux/hw_random.h>
 #include <linux/mailbox_client.h>
+#include <linux/minmax.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/sizes.h>
 #include <linux/slab.h>
 
 #define DRIVER_NAME		"turris-mox-rwtm"
 
+#define RWTM_DMA_BUFFER_SIZE	SZ_4K
+
 /*
  * The macros and constants below come from Turris Mox's rWTM firmware code.
  * This firmware is open source and it's sources can be found at
@@ -287,8 +291,7 @@ static int mox_hwrng_read(struct hwrng *rng, void *data, size_t max, bool wait)
 	struct armada_37xx_rwtm_tx_msg msg;
 	int ret;
 
-	if (max > 4096)
-		max = 4096;
+	max = min(max, RWTM_DMA_BUFFER_SIZE);
 
 	msg.command = MBOX_CMD_GET_RANDOM;
 	msg.args[0] = 1;
@@ -479,8 +482,8 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	rwtm->dev = dev;
-	rwtm->buf = dmam_alloc_coherent(dev, PAGE_SIZE, &rwtm->buf_phys,
-					GFP_KERNEL);
+	rwtm->buf = dmam_alloc_coherent(dev, RWTM_DMA_BUFFER_SIZE,
+					&rwtm->buf_phys, GFP_KERNEL);
 	if (!rwtm->buf)
 		return -ENOMEM;
 
Signature · 1 line
2.44.2
author maintainer
#5 lore raw

PATCH v3 4/17 firmware: turris-mox-rwtm: Use ETH_ALEN instead of hardcoded 6

In reply to Marek Behún

Use the ETH_ALEN macro instead of hardcoded 6 for MAC address length.

---
 drivers/firmware/turris-mox-rwtm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/firmware/turris-mox-rwtm.c +2 -1
index aafd747543f6..f7f9859b8f61 100644
@@ -10,6 +10,7 @@
 #include <linux/debugfs.h>
 #include <linux/dma-mapping.h>
 #include <linux/hw_random.h>
+#include <linux/if_ether.h>
 #include <linux/mailbox_client.h>
 #include <linux/minmax.h>
 #include <linux/module.h>
@@ -69,7 +70,7 @@ struct mox_rwtm {
 	int has_board_info;
 	u64 serial_number;
 	int board_version, ram_size;
-	u8 mac_address1[6], mac_address2[6];
+	u8 mac_address1[ETH_ALEN], mac_address2[ETH_ALEN];
 
 	/* public key burned in eFuse */
 	int has_pubkey;
Signature · 1 line
2.44.2
author maintainer
#6 lore raw

PATCH v3 5/17 firmware: turris-mox-rwtm: Use the boolean type where appropriate

In reply to Marek Behún

Use the boolean type for has_board_info, has_pubkey and last_sig_done members of the driver's private structure.

---
 drivers/firmware/turris-mox-rwtm.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)
drivers/firmware/turris-mox-rwtm.c +8 -7
index f7f9859b8f61..5ed480ac5146 100644
@@ -19,6 +19,7 @@
 #include <linux/platform_device.h>
 #include <linux/sizes.h>
 #include <linux/slab.h>
+#include <linux/types.h>
 
 #define DRIVER_NAME		"turris-mox-rwtm"
 
@@ -67,13 +68,13 @@ struct mox_rwtm {
 	struct completion cmd_done;
 
 	/* board information */
-	int has_board_info;
+	bool has_board_info;
 	u64 serial_number;
 	int board_version, ram_size;
 	u8 mac_address1[ETH_ALEN], mac_address2[ETH_ALEN];
 
 	/* public key burned in eFuse */
-	int has_pubkey;
+	bool has_pubkey;
 	u8 pubkey[135];
 
 #ifdef CONFIG_DEBUG_FS
@@ -85,7 +86,7 @@ struct mox_rwtm {
 	 */
 	struct dentry *debugfs_root;
 	u32 last_sig[34];
-	int last_sig_done;
+	bool last_sig_done;
 #endif
 };
 
@@ -229,7 +230,7 @@ static int mox_get_board_info(struct mox_rwtm *rwtm)
 				  reply->status[5]);
 		reply_to_mac_addr(rwtm->mac_address2, reply->status[6],
 				  reply->status[7]);
-		rwtm->has_board_info = 1;
+		rwtm->has_board_info = true;
 
 		pr_info("Turris Mox serial number %016llX\n",
 			rwtm->serial_number);
@@ -256,7 +257,7 @@ static int mox_get_board_info(struct mox_rwtm *rwtm)
 	} else {
 		u32 *s = reply->status;
 
-		rwtm->has_pubkey = 1;
+		rwtm->has_pubkey = true;
 		sprintf(rwtm->pubkey,
 			"%06x%08x%08x%08x%08x%08x%08x%08x%08x%08x%08x%08x%08x%08x%08x%08x%08x",
 			ret, s[0], s[1], s[2], s[3], s[4], s[5], s[6], s[7],
@@ -352,7 +353,7 @@ static ssize_t do_sign_read(struct file *file, char __user *buf, size_t len,
 
 	/* 2 arrays of 17 32-bit words are 136 bytes */
 	ret = simple_read_from_buffer(buf, len, ppos, rwtm->last_sig, 136);
-	rwtm->last_sig_done = 0;
+	rwtm->last_sig_done = false;
 
 	return ret;
 }
@@ -418,7 +419,7 @@ static ssize_t do_sign_write(struct file *file, const char __user *buf,
 	 */
 	memcpy(rwtm->last_sig, rwtm->buf + 68, 136);
 	cpu_to_be32_array(rwtm->last_sig, rwtm->last_sig, 34);
-	rwtm->last_sig_done = 1;
+	rwtm->last_sig_done = true;
 
 	mutex_unlock(&rwtm->busy);
 	return len;
Signature · 1 line
2.44.2
#7 lore raw

Re: [PATCH v3 05/17] firmware: turris-mox-rwtm: Use the boolean type where appropriate

Quoted 3 lines · Marek Behún

Use the boolean type for has_board_info, has_pubkey and last_sig_done members of the driver's private structure.

...

Quoted 17 lines
drivers/firmware/turris-mox-rwtm.c +2 -2
@@ -67,13 +68,13 @@ struct mox_rwtm {
        struct completion cmd_done;
 
        /* board information */
-       int has_board_info;
+       bool has_board_info;
        u64 serial_number;
        int board_version, ram_size;
        u8 mac_address1[ETH_ALEN], mac_address2[ETH_ALEN];
 
        /* public key burned in eFuse */
-       int has_pubkey;
+       bool has_pubkey;
        u8 pubkey[135];
 
 #ifdef CONFIG_DEBUG_FS

drivers/firmware/turris-mox-rwtm.c struct mox_rwtm { · line 91

Quoted 7 lines
drivers/firmware/turris-mox-rwtm.c +1 -1
@@ -85,7 +86,7 @@ struct mox_rwtm {
         */
        struct dentry *debugfs_root;
        u32 last_sig[34];
-       int last_sig_done;
+       bool last_sig_done;
 #endif
 };

A side note: At some point it might make sense to look at the structure layout with help of pahole tool and maybe compress it a bit to save a few bytes (it might be feasible after this change).

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author maintainer
#8 lore raw

PATCH v3 6/17 firmware: turris-mox-rwtm: Hide signature related constants behind macros

In reply to Marek Behún

Hide signature generation related constants behind macros instead of hardcoding the values.

Use SHA512_DIGEST_SIZE from crypto/sha2.h instead of hardcoded 64 as the message size.

---
 drivers/firmware/turris-mox-rwtm.c | 34 +++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 12 deletions(-)
drivers/firmware/turris-mox-rwtm.c +22 -12
index 5ed480ac5146..59bb9245e04f 100644
@@ -5,6 +5,7 @@
  * Copyright (C) 2019, 2024 Marek Behún <[email protected]>
  */
 
+#include <crypto/sha2.h>
 #include <linux/armada-37xx-rwtm-mailbox.h>
 #include <linux/completion.h>
 #include <linux/debugfs.h>
@@ -31,6 +32,12 @@
  * https://gitlab.labs.nic.cz/turris/mox-boot-builder/tree/master/wtmi.
  */
 
+#define MOX_ECC_NUMBER_WORDS	17
+#define MOX_ECC_NUMBER_LEN	(MOX_ECC_NUMBER_WORDS * sizeof(u32))
+
+#define MOX_ECC_SIGNATURE_WORDS	(2 * MOX_ECC_NUMBER_WORDS)
+#define MOX_ECC_SIGNATURE_LEN	(MOX_ECC_NUMBER_WORDS * sizeof(u32))
+
 #define MBOX_STS_SUCCESS	(0 << 30)
 #define MBOX_STS_FAIL		(1 << 30)
 #define MBOX_STS_BADCMD		(2 << 30)
@@ -85,7 +92,7 @@ struct mox_rwtm {
 	 * from userspace.
 	 */
 	struct dentry *debugfs_root;
-	u32 last_sig[34];
+	u32 last_sig[MOX_ECC_SIGNATURE_WORDS];
 	bool last_sig_done;
 #endif
 };
@@ -345,14 +352,15 @@ static ssize_t do_sign_read(struct file *file, char __user *buf, size_t len,
 	if (*ppos != 0)
 		return 0;
 
-	if (len < 136)
+	if (len < MOX_ECC_SIGNATURE_LEN)
 		return -EINVAL;
 
 	if (!rwtm->last_sig_done)
 		return -ENODATA;
 
 	/* 2 arrays of 17 32-bit words are 136 bytes */
-	ret = simple_read_from_buffer(buf, len, ppos, rwtm->last_sig, 136);
+	ret = simple_read_from_buffer(buf, len, ppos, rwtm->last_sig,
+				      MOX_ECC_SIGNATURE_LEN);
 	rwtm->last_sig_done = false;
 
 	return ret;
@@ -367,8 +375,7 @@ static ssize_t do_sign_write(struct file *file, const char __user *buf,
 	loff_t dummy = 0;
 	ssize_t ret;
 
-	/* the input is a SHA-512 hash, so exactly 64 bytes have to be read */
-	if (len != 64)
+	if (len != SHA512_DIGEST_SIZE)
 		return -EINVAL;
 
 	/* if last result is not zero user has not read that information yet */
@@ -389,17 +396,18 @@ static ssize_t do_sign_write(struct file *file, const char __user *buf,
 	 *   3. Address of the buffer where ECDSA signature value S shall be
 	 *      stored by the rWTM firmware.
 	 */
-	memset(rwtm->buf, 0, 4);
-	ret = simple_write_to_buffer(rwtm->buf + 4, 64, &dummy, buf, len);
+	memset(rwtm->buf, 0, sizeof(u32));
+	ret = simple_write_to_buffer(rwtm->buf + sizeof(u32),
+				     SHA512_DIGEST_SIZE, &dummy, buf, len);
 	if (ret < 0)
 		goto unlock_mutex;
-	be32_to_cpu_array(rwtm->buf, rwtm->buf, 17);
+	be32_to_cpu_array(rwtm->buf, rwtm->buf, MOX_ECC_NUMBER_WORDS);
 
 	msg.command = MBOX_CMD_SIGN;
 	msg.args[0] = 1;
 	msg.args[1] = rwtm->buf_phys;
-	msg.args[2] = rwtm->buf_phys + 68;
-	msg.args[3] = rwtm->buf_phys + 2 * 68;
+	msg.args[2] = rwtm->buf_phys + MOX_ECC_NUMBER_LEN;
+	msg.args[3] = rwtm->buf_phys + 2 * MOX_ECC_NUMBER_LEN;
 	ret = mbox_send_message(rwtm->mbox, &msg);
 	if (ret < 0)
 		goto unlock_mutex;
@@ -417,8 +425,10 @@ static ssize_t do_sign_write(struct file *file, const char __user *buf,
 	 * computed by the rWTM firmware and convert their words from
 	 * LE to BE.
 	 */
-	memcpy(rwtm->last_sig, rwtm->buf + 68, 136);
-	cpu_to_be32_array(rwtm->last_sig, rwtm->last_sig, 34);
+	memcpy(rwtm->last_sig, rwtm->buf + MOX_ECC_NUMBER_LEN,
+	       MOX_ECC_SIGNATURE_LEN);
+	cpu_to_be32_array(rwtm->last_sig, rwtm->last_sig,
+			  MOX_ECC_SIGNATURE_WORDS);
 	rwtm->last_sig_done = true;
 
 	mutex_unlock(&rwtm->busy);
Signature · 1 line
2.44.2
#9 lore raw

Re: [PATCH v3 06/17] firmware: turris-mox-rwtm: Hide signature related constants behind macros

Quoted 6 lines · Marek Behún

Hide signature generation related constants behind macros instead of hardcoding the values.

Use SHA512_DIGEST_SIZE from crypto/sha2.h instead of hardcoded 64 as the message size.

...

Quoted 1 line
        /* 2 arrays of 17 32-bit words are 136 bytes */

Hmm... Comment now is a bit too precise. But okay :-)

drivers/firmware/turris-mox-rwtm.c static ssize_t do_sign_read(struct file *file, char __user *buf, size_t len, · line 363

Quoted 3 lines
drivers/firmware/turris-mox-rwtm.c +2 -1
-       ret = simple_read_from_buffer(buf, len, ppos, rwtm->last_sig, 136);
+       ret = simple_read_from_buffer(buf, len, ppos, rwtm->last_sig,
+                                     MOX_ECC_SIGNATURE_LEN);

I think sizeof() here is more robust.
No need to resend due to this.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author maintainer
#10 lore raw

PATCH v3 7/17 firmware: turris-mox-rwtm: Fix driver includes

In reply to Marek Behún

Drop including of.h, include several other headers that are used but not included directly.

---
 drivers/firmware/turris-mox-rwtm.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/firmware/turris-mox-rwtm.c +6 -1
index 59bb9245e04f..9c66bda2f976 100644
@@ -8,18 +8,23 @@
 #include <crypto/sha2.h>
 #include <linux/armada-37xx-rwtm-mailbox.h>
 #include <linux/completion.h>
+#include <linux/container_of.h>
 #include <linux/debugfs.h>
+#include <linux/device.h>
 #include <linux/dma-mapping.h>
+#include <linux/err.h>
+#include <linux/fs.h>
 #include <linux/hw_random.h>
 #include <linux/if_ether.h>
+#include <linux/kobject.h>
 #include <linux/mailbox_client.h>
 #include <linux/minmax.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
-#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/sizes.h>
 #include <linux/slab.h>
+#include <linux/sysfs.h>
 #include <linux/types.h>
 
 #define DRIVER_NAME		"turris-mox-rwtm"
Signature · 1 line
2.44.2
author maintainer
#11 lore raw

PATCH v3 8/17 firmware: turris-mox-rwtm: Don't create own kobject type

In reply to Marek Behún

In order to create attribute files in /sys/firmware/turris-mox-rwtm, this driver creates it's own kobject type.

Simplify this by dropping this own kobject creation, and instead creating standard device attribute files.

For backwards compatibility with sysfs ABI, create a symlink /sys/firmware/turris-mox-rwtm, pointing to this device's sysfs directory.

---
 drivers/firmware/turris-mox-rwtm.c | 108 ++++++++---------------------
 1 file changed, 30 insertions(+), 78 deletions(-)
drivers/firmware/turris-mox-rwtm.c +30 -78
index 9c66bda2f976..9c857ba427d0 100644
@@ -23,7 +23,6 @@
 #include <linux/mutex.h>
 #include <linux/platform_device.h>
 #include <linux/sizes.h>
-#include <linux/slab.h>
 #include <linux/sysfs.h>
 #include <linux/types.h>
 
@@ -62,13 +61,10 @@ enum mbox_cmd {
 	MBOX_CMD_OTP_WRITE	= 8,
 };
 
-struct mox_kobject;
-
 struct mox_rwtm {
 	struct device *dev;
 	struct mbox_client mbox_client;
 	struct mbox_chan *mbox;
-	struct mox_kobject *kobj;
 	struct hwrng hwrng;
 
 	struct armada_37xx_rwtm_rx_msg reply;
@@ -102,59 +98,17 @@ struct mox_rwtm {
 #endif
 };
 
-struct mox_kobject {
-	struct kobject kobj;
-	struct mox_rwtm *rwtm;
-};
-
-static inline struct kobject *rwtm_to_kobj(struct mox_rwtm *rwtm)
-{
-	return &rwtm->kobj->kobj;
-}
-
-static inline struct mox_rwtm *to_rwtm(struct kobject *kobj)
-{
-	return container_of(kobj, struct mox_kobject, kobj)->rwtm;
-}
-
-static void mox_kobj_release(struct kobject *kobj)
-{
-	kfree(to_rwtm(kobj)->kobj);
-}
-
-static const struct kobj_type mox_kobj_ktype = {
-	.release	= mox_kobj_release,
-	.sysfs_ops	= &kobj_sysfs_ops,
-};
-
-static int mox_kobj_create(struct mox_rwtm *rwtm)
-{
-	rwtm->kobj = kzalloc(sizeof(*rwtm->kobj), GFP_KERNEL);
-	if (!rwtm->kobj)
-		return -ENOMEM;
-
-	kobject_init(rwtm_to_kobj(rwtm), &mox_kobj_ktype);
-	if (kobject_add(rwtm_to_kobj(rwtm), firmware_kobj, "turris-mox-rwtm")) {
-		kobject_put(rwtm_to_kobj(rwtm));
-		return -ENXIO;
-	}
-
-	rwtm->kobj->rwtm = rwtm;
-
-	return 0;
-}
-
 #define MOX_ATTR_RO(name, format, cat)				\
 static ssize_t							\
-name##_show(struct kobject *kobj, struct kobj_attribute *a,	\
+name##_show(struct device *dev, struct device_attribute *a,	\
 	    char *buf)						\
 {								\
-	struct mox_rwtm *rwtm = to_rwtm(kobj);	\
+	struct mox_rwtm *rwtm = dev_get_drvdata(dev);		\
 	if (!rwtm->has_##cat)					\
 		return -ENODATA;				\
 	return sprintf(buf, format, rwtm->name);		\
 }								\
-static struct kobj_attribute mox_attr_##name = __ATTR_RO(name)
+static DEVICE_ATTR_RO(name)
 
 MOX_ATTR_RO(serial_number, "%016llX\n", board_info);
 MOX_ATTR_RO(board_version, "%i\n", board_info);
@@ -163,6 +117,17 @@ MOX_ATTR_RO(mac_address1, "%pM\n", board_info);
 MOX_ATTR_RO(mac_address2, "%pM\n", board_info);
 MOX_ATTR_RO(pubkey, "%s\n", pubkey);
 
+static struct attribute *turris_mox_rwtm_attrs[] = {
+	&dev_attr_serial_number.attr,
+	&dev_attr_board_version.attr,
+	&dev_attr_ram_size.attr,
+	&dev_attr_mac_address1.attr,
+	&dev_attr_mac_address2.attr,
+	&dev_attr_pubkey.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(turris_mox_rwtm);
+
 static int mox_get_status(enum mbox_cmd cmd, u32 retval)
 {
 	if (MBOX_STS_CMD(retval) != cmd)
@@ -177,16 +142,6 @@ static int mox_get_status(enum mbox_cmd cmd, u32 retval)
 		return MBOX_STS_VALUE(retval);
 }
 
-static const struct attribute *mox_rwtm_attrs[] = {
-	&mox_attr_serial_number.attr,
-	&mox_attr_board_version.attr,
-	&mox_attr_ram_size.attr,
-	&mox_attr_mac_address1.attr,
-	&mox_attr_mac_address2.attr,
-	&mox_attr_pubkey.attr,
-	NULL
-};
-
 static void mox_rwtm_rx_callback(struct mbox_client *cl, void *data)
 {
 	struct mox_rwtm *rwtm = dev_get_drvdata(cl->dev);
@@ -488,6 +443,11 @@ static inline void rwtm_unregister_debugfs(struct mox_rwtm *rwtm)
 }
 #endif
 
+static void rwtm_firmware_symlink_drop(void *parent)
+{
+	sysfs_remove_link(parent, DRIVER_NAME);
+}
+
 static int turris_mox_rwtm_probe(struct platform_device *pdev)
 {
 	struct mox_rwtm *rwtm;
@@ -504,18 +464,6 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 	if (!rwtm->buf)
 		return -ENOMEM;
 
-	ret = mox_kobj_create(rwtm);
-	if (ret < 0) {
-		dev_err(dev, "Cannot create turris-mox-rwtm kobject!\n");
-		return ret;
-	}
-
-	ret = sysfs_create_files(rwtm_to_kobj(rwtm), mox_rwtm_attrs);
-	if (ret < 0) {
-		dev_err(dev, "Cannot create sysfs files!\n");
-		goto put_kobj;
-	}
-
 	platform_set_drvdata(pdev, rwtm);
 
 	mutex_init(&rwtm->busy);
@@ -529,7 +477,7 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 		if (ret != -EPROBE_DEFER)
 			dev_err(dev, "Cannot request mailbox channel: %i\n",
 				ret);
-		goto remove_files;
+		return ret;
 	}
 
 	init_completion(&rwtm->cmd_done);
@@ -563,14 +511,19 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 
 	dev_info(dev, "HWRNG successfully registered\n");
 
+	/*
+	 * For sysfs ABI compatibility, create symlink
+	 * /sys/firmware/turris-mox-rwtm to this device's sysfs directory.
+	 */
+	ret = sysfs_create_link(firmware_kobj, &dev->kobj, DRIVER_NAME);
+	if (!ret)
+		devm_add_action_or_reset(dev, rwtm_firmware_symlink_drop,
+					 firmware_kobj);
+
 	return 0;
 
 free_channel:
 	mbox_free_channel(rwtm->mbox);
-remove_files:
-	sysfs_remove_files(rwtm_to_kobj(rwtm), mox_rwtm_attrs);
-put_kobj:
-	kobject_put(rwtm_to_kobj(rwtm));
 	return ret;
 }
 
@@ -579,8 +532,6 @@ static void turris_mox_rwtm_remove(struct platform_device *pdev)
 	struct mox_rwtm *rwtm = platform_get_drvdata(pdev);
 
 	rwtm_unregister_debugfs(rwtm);
-	sysfs_remove_files(rwtm_to_kobj(rwtm), mox_rwtm_attrs);
-	kobject_put(rwtm_to_kobj(rwtm));
 	mbox_free_channel(rwtm->mbox);
 }
 
@@ -598,6 +549,7 @@ static struct platform_driver turris_mox_rwtm_driver = {
 	.driver	= {
 		.name		= DRIVER_NAME,
 		.of_match_table	= turris_mox_rwtm_match,
+		.dev_groups	= turris_mox_rwtm_groups,
 	},
 };
 module_platform_driver(turris_mox_rwtm_driver);
Signature · 1 line
2.44.2
#12 lore raw

Re: [PATCH v3 08/17] firmware: turris-mox-rwtm: Don't create own kobject type

Quoted 10 lines · Marek Behún

In order to create attribute files in /sys/firmware/turris-mox-rwtm, this driver creates it's own kobject type.

Simplify this by dropping this own kobject creation, and instead creating standard device attribute files.

For backwards compatibility with sysfs ABI, create a symlink /sys/firmware/turris-mox-rwtm, pointing to this device's sysfs directory.

...

drivers/firmware/turris-mox-rwtm.c struct mox_rwtm { · line 109

Quoted 11 lines

#define MOX_ATTR_RO(name, format, cat) \
static ssize_t \

drivers/firmware/turris-mox-rwtm.c +2 -2
-name##_show(struct kobject *kobj, struct kobj_attribute *a,    \
+name##_show(struct device *dev, struct device_attribute *a,    \
            char *buf)                                          \
 {                                                              \
-       struct mox_rwtm *rwtm = to_rwtm(kobj);  \
+       struct mox_rwtm *rwtm = dev_get_drvdata(dev);           \
        if (!rwtm->has_##cat)                                   \
                return -ENODATA;                                \
        return sprintf(buf, format, rwtm->name);                \

At some point this should be replaced with sysfs_emit()

Quoted 3 lines

} \

drivers/firmware/turris-mox-rwtm.c +1 -1
-static struct kobj_attribute mox_attr_##name = __ATTR_RO(name)
+static DEVICE_ATTR_RO(name)
Signature · 2 lines
With Best Regards,
Andy Shevchenko
author maintainer
#13 lore raw

PATCH v3 9/17 firmware: turris-mox-rwtm: Simplify debugfs code

In reply to Marek Behún

Simplify debugfs code: do not check for errors, as debugfs errors should be ignored, and use devm action for dropping the debugfs directory.

---
 drivers/firmware/turris-mox-rwtm.c | 44 ++++++++----------------------
 1 file changed, 11 insertions(+), 33 deletions(-)
drivers/firmware/turris-mox-rwtm.c +11 -33
index 9c857ba427d0..77fdd5c66971 100644
@@ -92,7 +92,6 @@ struct mox_rwtm {
 	 * It should be rewritten via crypto API once akcipher API is available
 	 * from userspace.
 	 */
-	struct dentry *debugfs_root;
 	u32 last_sig[MOX_ECC_SIGNATURE_WORDS];
 	bool last_sig_done;
 #endif
@@ -406,39 +405,23 @@ static const struct file_operations do_sign_fops = {
 	.llseek	= no_llseek,
 };
 
-static int rwtm_register_debugfs(struct mox_rwtm *rwtm)
+static void rwtm_debugfs_release(void *root)
 {
-	struct dentry *root, *entry;
-
-	root = debugfs_create_dir("turris-mox-rwtm", NULL);
-
-	if (IS_ERR(root))
-		return PTR_ERR(root);
-
-	entry = debugfs_create_file_unsafe("do_sign", 0600, root, rwtm,
-					   &do_sign_fops);
-	if (IS_ERR(entry))
-		goto err_remove;
-
-	rwtm->debugfs_root = root;
-
-	return 0;
-err_remove:
 	debugfs_remove_recursive(root);
-	return PTR_ERR(entry);
 }
 
-static void rwtm_unregister_debugfs(struct mox_rwtm *rwtm)
+static void rwtm_register_debugfs(struct mox_rwtm *rwtm)
 {
-	debugfs_remove_recursive(rwtm->debugfs_root);
+	struct dentry *root;
+
+	root = debugfs_create_dir("turris-mox-rwtm", NULL);
+
+	debugfs_create_file_unsafe("do_sign", 0600, root, rwtm, &do_sign_fops);
+
+	devm_add_action_or_reset(rwtm->dev, rwtm_debugfs_release, root);
 }
 #else
-static inline int rwtm_register_debugfs(struct mox_rwtm *rwtm)
-{
-	return 0;
-}
-
-static inline void rwtm_unregister_debugfs(struct mox_rwtm *rwtm)
+static inline void rwtm_register_debugfs(struct mox_rwtm *rwtm)
 {
 }
 #endif
@@ -503,11 +486,7 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 		goto free_channel;
 	}
 
-	ret = rwtm_register_debugfs(rwtm);
-	if (ret < 0) {
-		dev_err(dev, "Failed creating debugfs entries: %i\n", ret);
-		goto free_channel;
-	}
+	rwtm_register_debugfs(rwtm);
 
 	dev_info(dev, "HWRNG successfully registered\n");
 
@@ -531,7 +510,6 @@ static void turris_mox_rwtm_remove(struct platform_device *pdev)
 {
 	struct mox_rwtm *rwtm = platform_get_drvdata(pdev);
 
-	rwtm_unregister_debugfs(rwtm);
 	mbox_free_channel(rwtm->mbox);
 }
 
Signature · 1 line
2.44.2
#14 lore raw

Re: [PATCH v3 09/17] firmware: turris-mox-rwtm: Simplify debugfs code

Quoted 3 lines · Marek Behún

Simplify debugfs code: do not check for errors, as debugfs errors should be ignored, and use devm action for dropping the debugfs directory.

Actually you may do that without devm. There is a lookup method or so in debugfs APIs that can be used. Can be amended later on, though.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author maintainer
#15 lore raw

Re: [PATCH v3 09/17] firmware: turris-mox-rwtm: Simplify debugfs code

Quoted 7 lines · Andy Shevchenko
Quoted 3 lines · Marek Behún

Simplify debugfs code: do not check for errors, as debugfs errors should be ignored, and use devm action for dropping the debugfs directory.

Actually you may do that without devm. There is a lookup method or so in debugfs APIs that can be used. Can be amended later on, though.

But I want to get rid of driver .remove() method altogehter in a subsequent patch. How can I use debugfs_lookup_and_remove() that way without devm?

Marek

author maintainer
#16 lore raw

PATCH v3 10/17 firmware: turris-mox-rwtm: Convert rest to devm_* and get rid of driver .remove()

In reply to Marek Behún

Use devm resource management for driver's mailbox. This allows us to get rid of the driver's .remove() method and the gotos in .probe().

---
 drivers/firmware/turris-mox-rwtm.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)
drivers/firmware/turris-mox-rwtm.c +11 -14
index 77fdd5c66971..9a68784a9619 100644
@@ -426,6 +426,11 @@ static inline void rwtm_register_debugfs(struct mox_rwtm *rwtm)
 }
 #endif
 
+static void rwtm_devm_mbox_release(void *mbox)
+{
+	mbox_free_channel(mbox);
+}
+
 static void rwtm_firmware_symlink_drop(void *parent)
 {
 	sysfs_remove_link(parent, DRIVER_NAME);
@@ -463,6 +468,10 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	ret = devm_add_action_or_reset(dev, rwtm_devm_mbox_release, rwtm->mbox);
+	if (ret)
+		return ret;
+
 	init_completion(&rwtm->cmd_done);
 
 	ret = mox_get_board_info(rwtm);
@@ -473,7 +482,7 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 	if (ret < 0) {
 		dev_notice(dev,
 			   "Firmware does not support the GET_RANDOM command\n");
-		goto free_channel;
+		return ret;
 	}
 
 	rwtm->hwrng.name = DRIVER_NAME "_hwrng";
@@ -483,7 +492,7 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 	ret = devm_hwrng_register(dev, &rwtm->hwrng);
 	if (ret < 0) {
 		dev_err(dev, "Cannot register HWRNG: %i\n", ret);
-		goto free_channel;
+		return ret;
 	}
 
 	rwtm_register_debugfs(rwtm);
@@ -500,17 +509,6 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 					 firmware_kobj);
 
 	return 0;
-
-free_channel:
-	mbox_free_channel(rwtm->mbox);
-	return ret;
-}
-
-static void turris_mox_rwtm_remove(struct platform_device *pdev)
-{
-	struct mox_rwtm *rwtm = platform_get_drvdata(pdev);
-
-	mbox_free_channel(rwtm->mbox);
 }
 
 static const struct of_device_id turris_mox_rwtm_match[] = {
@@ -523,7 +521,6 @@ MODULE_DEVICE_TABLE(of, turris_mox_rwtm_match);
 
 static struct platform_driver turris_mox_rwtm_driver = {
 	.probe	= turris_mox_rwtm_probe,
-	.remove_new = turris_mox_rwtm_remove,
 	.driver	= {
 		.name		= DRIVER_NAME,
 		.of_match_table	= turris_mox_rwtm_match,
Signature · 1 line
2.44.2
author maintainer
#17 lore raw

PATCH v3 11/17 firmware: turris-mox-rwtm: Use dev_err_probe() where possible

In reply to Marek Behún

Use dev_err_probe() where possible in the driver's .probe() method.

---
 drivers/firmware/turris-mox-rwtm.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)
drivers/firmware/turris-mox-rwtm.c +5 -11
index 9a68784a9619..397175fafbe0 100644
@@ -460,13 +460,9 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 	rwtm->mbox_client.rx_callback = mox_rwtm_rx_callback;
 
 	rwtm->mbox = mbox_request_channel(&rwtm->mbox_client, 0);
-	if (IS_ERR(rwtm->mbox)) {
-		ret = PTR_ERR(rwtm->mbox);
-		if (ret != -EPROBE_DEFER)
-			dev_err(dev, "Cannot request mailbox channel: %i\n",
-				ret);
-		return ret;
-	}
+	if (IS_ERR(rwtm->mbox))
+		return dev_err_probe(dev, PTR_ERR(rwtm->mbox),
+				     "Cannot request mailbox channel!\n");
 
 	ret = devm_add_action_or_reset(dev, rwtm_devm_mbox_release, rwtm->mbox);
 	if (ret)
@@ -490,10 +486,8 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 	rwtm->hwrng.priv = (unsigned long) rwtm;
 
 	ret = devm_hwrng_register(dev, &rwtm->hwrng);
-	if (ret < 0) {
-		dev_err(dev, "Cannot register HWRNG: %i\n", ret);
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "Cannot register HWRNG!\n");
 
 	rwtm_register_debugfs(rwtm);
 
Signature · 1 line
2.44.2
author maintainer
#18 lore raw

PATCH v3 12/17 firmware: turris-mox-rwtm: Initialize completion before mailbox

In reply to Marek Behún

Initialize the completion before the mailbox channel is requested.

Fixes 389711b37493 ("firmware: Add Turris Mox rWTM firmware driver")
---
 drivers/firmware/turris-mox-rwtm.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/firmware/turris-mox-rwtm.c +1 -2
index 397175fafbe0..2c43af69783c 100644
@@ -455,6 +455,7 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, rwtm);
 
 	mutex_init(&rwtm->busy);
+	init_completion(&rwtm->cmd_done);
 
 	rwtm->mbox_client.dev = dev;
 	rwtm->mbox_client.rx_callback = mox_rwtm_rx_callback;
@@ -468,8 +469,6 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	init_completion(&rwtm->cmd_done);
-
 	ret = mox_get_board_info(rwtm);
 	if (ret < 0)
 		dev_warn(dev, "Cannot read board information: %i\n", ret);
Signature · 1 line
2.44.2
author maintainer
#19 lore raw

PATCH v3 13/17 firmware: turris-mox-rwtm: Drop redundant device pointer

In reply to Marek Behún

Drop redundant device pointer from driver's private structure.

---
 drivers/firmware/turris-mox-rwtm.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)
drivers/firmware/turris-mox-rwtm.c +11 -7
index 2c43af69783c..0b93040167b6 100644
@@ -62,7 +62,6 @@ enum mbox_cmd {
 };
 
 struct mox_rwtm {
-	struct device *dev;
 	struct mbox_client mbox_client;
 	struct mbox_chan *mbox;
 	struct hwrng hwrng;
@@ -97,6 +96,11 @@ struct mox_rwtm {
 #endif
 };
 
+static inline struct device *rwtm_dev(struct mox_rwtm *rwtm)
+{
+	return rwtm->mbox_client.dev;
+}
+
 #define MOX_ATTR_RO(name, format, cat)				\
 static ssize_t							\
 name##_show(struct device *dev, struct device_attribute *a,	\
@@ -165,6 +169,7 @@ static void reply_to_mac_addr(u8 *mac, u32 t1, u32 t2)
 
 static int mox_get_board_info(struct mox_rwtm *rwtm)
 {
+	struct device *dev = rwtm_dev(rwtm);
 	struct armada_37xx_rwtm_tx_msg msg;
 	struct armada_37xx_rwtm_rx_msg *reply = &rwtm->reply;
 	int ret;
@@ -179,10 +184,10 @@ static int mox_get_board_info(struct mox_rwtm *rwtm)
 
 	ret = mox_get_status(MBOX_CMD_BOARD_INFO, reply->retval);
 	if (ret == -ENODATA) {
-		dev_warn(rwtm->dev,
+		dev_warn(dev,
 			 "Board does not have manufacturing information burned!\n");
 	} else if (ret == -ENOSYS) {
-		dev_notice(rwtm->dev,
+		dev_notice(dev,
 			   "Firmware does not support the BOARD_INFO command\n");
 	} else if (ret < 0) {
 		return ret;
@@ -214,9 +219,9 @@ static int mox_get_board_info(struct mox_rwtm *rwtm)
 
 	ret = mox_get_status(MBOX_CMD_ECDSA_PUB_KEY, reply->retval);
 	if (ret == -ENODATA) {
-		dev_warn(rwtm->dev, "Board has no public key burned!\n");
+		dev_warn(dev, "Board has no public key burned!\n");
 	} else if (ret == -ENOSYS) {
-		dev_notice(rwtm->dev,
+		dev_notice(dev,
 			   "Firmware does not support the ECDSA_PUB_KEY command\n");
 	} else if (ret < 0) {
 		return ret;
@@ -418,7 +423,7 @@ static void rwtm_register_debugfs(struct mox_rwtm *rwtm)
 
 	debugfs_create_file_unsafe("do_sign", 0600, root, rwtm, &do_sign_fops);
 
-	devm_add_action_or_reset(rwtm->dev, rwtm_debugfs_release, root);
+	devm_add_action_or_reset(rwtm_dev(rwtm), rwtm_debugfs_release, root);
 }
 #else
 static inline void rwtm_register_debugfs(struct mox_rwtm *rwtm)
@@ -446,7 +451,6 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 	if (!rwtm)
 		return -ENOMEM;
 
-	rwtm->dev = dev;
 	rwtm->buf = dmam_alloc_coherent(dev, RWTM_DMA_BUFFER_SIZE,
 					&rwtm->buf_phys, GFP_KERNEL);
 	if (!rwtm->buf)
Signature · 1 line
2.44.2
author maintainer
#20 lore raw

PATCH v3 14/17 firmware: turris-mox-rwtm: Use devm_mutex_init() instead of mutex_init()

In reply to Marek Behún

Use devm_mutex_init() instead of mutex_init(), to properly call mutex_destroy() on probe failure / driver unbind.

---
 drivers/firmware/turris-mox-rwtm.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/firmware/turris-mox-rwtm.c +4 -1
index 0b93040167b6..5a850dc27fe3 100644
@@ -458,7 +458,10 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, rwtm);
 
-	mutex_init(&rwtm->busy);
+	ret = devm_mutex_init(dev, &rwtm->busy);
+	if (ret)
+		return ret;
+
 	init_completion(&rwtm->cmd_done);
 
 	rwtm->mbox_client.dev = dev;
Signature · 1 line
2.44.2
author maintainer
#21 lore raw

PATCH v3 15/17 firmware: turris-mox-rwtm: Use container_of() instead of hwrng .priv member

In reply to Marek Behún

Use container_of() to get driver private structure from hwnrg structure, instead of the hwrng's .priv member, as suggested by Herbert for another driver [1].

[1] https://lore.kernel.org/soc/[email protected]/

---
 drivers/firmware/turris-mox-rwtm.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/firmware/turris-mox-rwtm.c +1 -2
index 5a850dc27fe3..135adfa9fb1f 100644
@@ -260,7 +260,7 @@ static int check_get_random_support(struct mox_rwtm *rwtm)
 
 static int mox_hwrng_read(struct hwrng *rng, void *data, size_t max, bool wait)
 {
-	struct mox_rwtm *rwtm = (struct mox_rwtm *) rng->priv;
+	struct mox_rwtm *rwtm = container_of(rng, struct mox_rwtm, hwrng);
 	struct armada_37xx_rwtm_tx_msg msg;
 	int ret;
 
@@ -489,7 +489,6 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 
 	rwtm->hwrng.name = DRIVER_NAME "_hwrng";
 	rwtm->hwrng.read = mox_hwrng_read;
-	rwtm->hwrng.priv = (unsigned long) rwtm;
 
 	ret = devm_hwrng_register(dev, &rwtm->hwrng);
 	if (ret)
Signature · 1 line
2.44.2
#22 lore raw

Re: [PATCH v3 15/17] firmware: turris-mox-rwtm: Use container_of() instead of hwrng .priv member

Quoted 7 lines · Marek Behún

Use container_of() to get driver private structure from hwnrg structure, instead of the hwrng's .priv member, as suggested by Herbert for another driver [1].

[1] https://lore.kernel.org/soc/[email protected]/

You can make this to be a Link tag.

Link ...URL... [1]
Signature · 2 lines
With Best Regards,
Andy Shevchenko
author maintainer
#23 lore raw

PATCH v3 16/17 firmware: turris-mox-rwtm: Use EOPNOTSUPP instead of ENOSYS

In reply to Marek Behún

Checkpatch warns agains -ENOSYS:
WARNING: ENOSYS means 'invalid syscall nr' and nothing else
Use EOPNOTSUPP instead.

---
 drivers/firmware/turris-mox-rwtm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/firmware/turris-mox-rwtm.c +3 -3
index 135adfa9fb1f..9197e0549c6d 100644
@@ -138,7 +138,7 @@ static int mox_get_status(enum mbox_cmd cmd, u32 retval)
 	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;
+		return -EOPNOTSUPP;
 	else if (MBOX_STS_ERROR(retval) != MBOX_STS_SUCCESS)
 		return -EIO;
 	else
@@ -186,7 +186,7 @@ static int mox_get_board_info(struct mox_rwtm *rwtm)
 	if (ret == -ENODATA) {
 		dev_warn(dev,
 			 "Board does not have manufacturing information burned!\n");
-	} else if (ret == -ENOSYS) {
+	} else if (ret == -EOPNOTSUPP) {
 		dev_notice(dev,
 			   "Firmware does not support the BOARD_INFO command\n");
 	} else if (ret < 0) {
@@ -220,7 +220,7 @@ static int mox_get_board_info(struct mox_rwtm *rwtm)
 	ret = mox_get_status(MBOX_CMD_ECDSA_PUB_KEY, reply->retval);
 	if (ret == -ENODATA) {
 		dev_warn(dev, "Board has no public key burned!\n");
-	} else if (ret == -ENOSYS) {
+	} else if (ret == -EOPNOTSUPP) {
 		dev_notice(dev,
 			   "Firmware does not support the ECDSA_PUB_KEY command\n");
 	} else if (ret < 0) {
Signature · 1 line
2.44.2
author maintainer
#24 lore raw

PATCH v3 17/17 firmware: turris-mox-rwtm: Deduplicate command execution code

In reply to Marek Behún

Deduplicate rWTM command execution calls
mbox_send_message()
wait_for_completion()
mox_get_status()
to one function
mox_rwtm_exec()

---
 drivers/firmware/turris-mox-rwtm.c | 98 ++++++++++++------------------
 1 file changed, 39 insertions(+), 59 deletions(-)
drivers/firmware/turris-mox-rwtm.c +39 -59
index 9197e0549c6d..52e5cc1cf48f 100644
@@ -157,6 +157,34 @@ static void mox_rwtm_rx_callback(struct mbox_client *cl, void *data)
 	complete(&rwtm->cmd_done);
 }
 
+static int mox_rwtm_exec(struct mox_rwtm *rwtm, enum mbox_cmd cmd,
+			 struct armada_37xx_rwtm_tx_msg *msg,
+			 bool interruptible)
+{
+	struct armada_37xx_rwtm_tx_msg _msg = {};
+	int ret;
+
+	if (!msg)
+		msg = &_msg;
+
+	msg->command = cmd;
+
+	ret = mbox_send_message(rwtm->mbox, msg);
+	if (ret < 0)
+		return ret;
+
+	if (interruptible) {
+		ret = wait_for_completion_interruptible(&rwtm->cmd_done);
+		if (ret < 0)
+			return ret;
+	} else {
+		if (!wait_for_completion_timeout(&rwtm->cmd_done, HZ / 2))
+			return -ETIMEDOUT;
+	}
+
+	return mox_get_status(cmd, rwtm->reply.retval);
+}
+
 static void reply_to_mac_addr(u8 *mac, u32 t1, u32 t2)
 {
 	mac[0] = t1 >> 8;
@@ -170,19 +198,10 @@ static void reply_to_mac_addr(u8 *mac, u32 t1, u32 t2)
 static int mox_get_board_info(struct mox_rwtm *rwtm)
 {
 	struct device *dev = rwtm_dev(rwtm);
-	struct armada_37xx_rwtm_tx_msg msg;
 	struct armada_37xx_rwtm_rx_msg *reply = &rwtm->reply;
 	int ret;
 
-	msg.command = MBOX_CMD_BOARD_INFO;
-	ret = mbox_send_message(rwtm->mbox, &msg);
-	if (ret < 0)
-		return ret;
-
-	if (!wait_for_completion_timeout(&rwtm->cmd_done, HZ / 2))
-		return -ETIMEDOUT;
-
-	ret = mox_get_status(MBOX_CMD_BOARD_INFO, reply->retval);
+	ret = mox_rwtm_exec(rwtm, MBOX_CMD_BOARD_INFO, NULL, false);
 	if (ret == -ENODATA) {
 		dev_warn(dev,
 			 "Board does not have manufacturing information burned!\n");
@@ -209,15 +228,7 @@ static int mox_get_board_info(struct mox_rwtm *rwtm)
 		pr_info("           burned RAM size %i MiB\n", rwtm->ram_size);
 	}
 
-	msg.command = MBOX_CMD_ECDSA_PUB_KEY;
-	ret = mbox_send_message(rwtm->mbox, &msg);
-	if (ret < 0)
-		return ret;
-
-	if (!wait_for_completion_timeout(&rwtm->cmd_done, HZ / 2))
-		return -ETIMEDOUT;
-
-	ret = mox_get_status(MBOX_CMD_ECDSA_PUB_KEY, reply->retval);
+	ret = mox_rwtm_exec(rwtm, MBOX_CMD_ECDSA_PUB_KEY, NULL, false);
 	if (ret == -ENODATA) {
 		dev_warn(dev, "Board has no public key burned!\n");
 	} else if (ret == -EOPNOTSUPP) {
@@ -240,37 +251,23 @@ static int mox_get_board_info(struct mox_rwtm *rwtm)
 
 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;
-
-	if (!wait_for_completion_timeout(&rwtm->cmd_done, HZ / 2))
-		return -ETIMEDOUT;
+	struct armada_37xx_rwtm_tx_msg msg = {
+		.args = { 1, rwtm->buf_phys, 4 },
+	};
 
-	return mox_get_status(MBOX_CMD_GET_RANDOM, rwtm->reply.retval);
+	return mox_rwtm_exec(rwtm, MBOX_CMD_GET_RANDOM, &msg, false);
 }
 
 static int mox_hwrng_read(struct hwrng *rng, void *data, size_t max, bool wait)
 {
 	struct mox_rwtm *rwtm = container_of(rng, struct mox_rwtm, hwrng);
-	struct armada_37xx_rwtm_tx_msg msg;
+	struct armada_37xx_rwtm_tx_msg msg = {
+		.args = { 1, rwtm->buf_phys, (max + 3) & ~3 },
+	};
 	int ret;
 
 	max = min(max, RWTM_DMA_BUFFER_SIZE);
 
-	msg.command = MBOX_CMD_GET_RANDOM;
-	msg.args[0] = 1;
-	msg.args[1] = rwtm->buf_phys;
-	msg.args[2] = (max + 3) & ~3;
-
 	if (!wait) {
 		if (!mutex_trylock(&rwtm->busy))
 			return -EBUSY;
@@ -278,15 +275,7 @@ static int mox_hwrng_read(struct hwrng *rng, void *data, size_t max, bool wait)
 		mutex_lock(&rwtm->busy);
 	}
 
-	ret = mbox_send_message(rwtm->mbox, &msg);
-	if (ret < 0)
-		goto unlock_mutex;
-
-	ret = wait_for_completion_interruptible(&rwtm->cmd_done);
-	if (ret < 0)
-		goto unlock_mutex;
-
-	ret = mox_get_status(MBOX_CMD_GET_RANDOM, rwtm->reply.retval);
+	ret = mox_rwtm_exec(rwtm, MBOX_CMD_GET_RANDOM, &msg, true);
 	if (ret < 0)
 		goto unlock_mutex;
 
@@ -334,7 +323,6 @@ static ssize_t do_sign_write(struct file *file, const char __user *buf,
 			     size_t len, loff_t *ppos)
 {
 	struct mox_rwtm *rwtm = file->private_data;
-	struct armada_37xx_rwtm_rx_msg *reply = &rwtm->reply;
 	struct armada_37xx_rwtm_tx_msg msg;
 	loff_t dummy = 0;
 	ssize_t ret;
@@ -367,23 +355,15 @@ static ssize_t do_sign_write(struct file *file, const char __user *buf,
 		goto unlock_mutex;
 	be32_to_cpu_array(rwtm->buf, rwtm->buf, MOX_ECC_NUMBER_WORDS);
 
-	msg.command = MBOX_CMD_SIGN;
 	msg.args[0] = 1;
 	msg.args[1] = rwtm->buf_phys;
 	msg.args[2] = rwtm->buf_phys + MOX_ECC_NUMBER_LEN;
 	msg.args[3] = rwtm->buf_phys + 2 * MOX_ECC_NUMBER_LEN;
-	ret = mbox_send_message(rwtm->mbox, &msg);
-	if (ret < 0)
-		goto unlock_mutex;
 
-	ret = wait_for_completion_interruptible(&rwtm->cmd_done);
+	ret = mox_rwtm_exec(rwtm, MBOX_CMD_SIGN, &msg, true);
 	if (ret < 0)
 		goto unlock_mutex;
 
-	ret = MBOX_STS_VALUE(reply->retval);
-	if (MBOX_STS_ERROR(reply->retval) != MBOX_STS_SUCCESS)
-		goto unlock_mutex;
-
 	/*
 	 * Here we read the R and S values of the ECDSA signature
 	 * computed by the rWTM firmware and convert their words from
Signature · 1 line
2.44.2
#25 lore raw

Re: [PATCH v3 17/17] firmware: turris-mox-rwtm: Deduplicate command execution code

Quoted 7 lines · Marek Behún

Deduplicate rWTM command execution calls
mbox_send_message()
wait_for_completion()
mox_get_status()
to one function
mox_rwtm_exec()

...

Review

Quoted 2 lines
(quoted diff) +2 -0
+       struct armada_37xx_rwtm_tx_msg msg = {
+               .args = { 1, rwtm->buf_phys, (max + 3) & ~3 },

I see that's in the original code, but perhaps later you want to switch this to use ALIGN() macro.

Quoted 1 line

+ };

Signature · 2 lines
With Best Regards,
Andy Shevchenko
Reviewed-by on the v3 series
#26 lore raw
In reply to Marek Behún
Quoted 5 lines · Marek Behún

Hi Gregory, Arnd, Andy et al.

this is v3 of series of fixes, refactors and cleanups for the turris-mox-rwtm driver.

As far as I'm concerned this looks good enough to proceed. FWIW,

In case of a new version, please address a few nit-picks.

Signature · 2 lines
With Best Regards,
Andy Shevchenko