[PATCH 00/19] Updates for turris-mox-rwtm driver

[PATCH 00/19] Updates for turris-mox-rwtm driver

[Marek Behún]

Hi Gregory, Arnd, Andy et al.

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

Marek

Marek Behún (19):
  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 PAGE_SIZE 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: Use kobj_type's default_groups member for
    automatic sysfs files creation
  firmware: turris-mox-rwtm: Simplify debugfs code
  firmware: turris-mox-rwtm: Simplify driver kobject code
  firmware: turris-mox-rwtm: Return true error code if kobject_add()
    fails
  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: Rearrange probe calls
  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: Deduplicate command execution code
  firmware: turris-mox-rwtm: Use EOPNOTSUPP instead of ENOSYS

 drivers/firmware/turris-mox-rwtm.c | 390 +++++++++++++----------------
 1 file changed, 172 insertions(+), 218 deletions(-)

-- 
2.44.2

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

[Marek Behún]

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")
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/firmware/turris-mox-rwtm.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
index 31d962cdd6eb..f1f9160c4195 100644
--- a/drivers/firmware/turris-mox-rwtm.c
+++ b/drivers/firmware/turris-mox-rwtm.c
@@ -2,7 +2,7 @@
 /*
  * Turris Mox rWTM firmware driver
  *
- * Copyright (C) 2019 Marek Behún <kabel@kernel.org>
+ * Copyright (C) 2019, 2024 Marek Behún <kabel@kernel.org>
  */
 
 #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);
 }
-- 
2.44.2

[PATCH 02/19] firmware: turris-mox-rwtm: Fix checking return value of wait_for_completion_timeout()

[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")
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/firmware/turris-mox-rwtm.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
index f1f9160c4195..3f4758e03c81 100644
--- a/drivers/firmware/turris-mox-rwtm.c
+++ b/drivers/firmware/turris-mox-rwtm.c
@@ -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);
 }
-- 
2.44.2

[PATCH 03/19] firmware: turris-mox-rwtm: Use PAGE_SIZE instead of hardcoded 4096

[Marek Behún]

The 4096 bytes limit in mox_hwrng_read() is due to the DMA buffer being
allocated to one PAGE_SIZE bytes. The PAGE_SIZE macro is used when
allocating the buffer, use it in mox_hwrng_read() as well.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/firmware/turris-mox-rwtm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
index 3f4758e03c81..5acdde1bb6d9 100644
--- a/drivers/firmware/turris-mox-rwtm.c
+++ b/drivers/firmware/turris-mox-rwtm.c
@@ -287,8 +287,8 @@ 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;
+	if (max > PAGE_SIZE)
+		max = PAGE_SIZE;
 
 	msg.command = MBOX_CMD_GET_RANDOM;
 	msg.args[0] = 1;
-- 
2.44.2

[PATCH 04/19] firmware: turris-mox-rwtm: Use ETH_ALEN instead of hardcoded 6

[Marek Behún]

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

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/firmware/turris-mox-rwtm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
index 5acdde1bb6d9..1b708a0760e6 100644
--- a/drivers/firmware/turris-mox-rwtm.c
+++ b/drivers/firmware/turris-mox-rwtm.c
@@ -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/module.h>
 #include <linux/mutex.h>
@@ -65,7 +66,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;
-- 
2.44.2

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

[Marek Behún]

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

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/firmware/turris-mox-rwtm.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
index 1b708a0760e6..b7212bdc301d 100644
--- a/drivers/firmware/turris-mox-rwtm.c
+++ b/drivers/firmware/turris-mox-rwtm.c
@@ -17,6 +17,7 @@
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
+#include <linux/types.h>
 
 #define DRIVER_NAME		"turris-mox-rwtm"
 
@@ -63,13 +64,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
@@ -81,7 +82,7 @@ struct mox_rwtm {
 	 */
 	struct dentry *debugfs_root;
 	u32 last_sig[34];
-	int last_sig_done;
+	bool last_sig_done;
 #endif
 };
 
@@ -225,7 +226,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);
@@ -252,7 +253,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],
@@ -349,7 +350,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;
 }
@@ -415,7 +416,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;
-- 
2.44.2

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

[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.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/firmware/turris-mox-rwtm.c | 34 +++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
index b7212bdc301d..254629437f59 100644
--- a/drivers/firmware/turris-mox-rwtm.c
+++ b/drivers/firmware/turris-mox-rwtm.c
@@ -5,6 +5,7 @@
  * Copyright (C) 2019, 2024 Marek Behún <kabel@kernel.org>
  */
 
+#include <crypto/sha2.h>
 #include <linux/armada-37xx-rwtm-mailbox.h>
 #include <linux/completion.h>
 #include <linux/debugfs.h>
@@ -27,6 +28,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)
@@ -81,7 +88,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
 };
@@ -342,14 +349,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;
@@ -364,8 +372,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 */
@@ -386,17 +393,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;
@@ -414,8 +422,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);
-- 
2.44.2

[PATCH 07/19] firmware: turris-mox-rwtm: Fix driver includes

[Marek Behún]

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

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/firmware/turris-mox-rwtm.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
index 254629437f59..5675f94a3ff2 100644
--- a/drivers/firmware/turris-mox-rwtm.c
+++ b/drivers/firmware/turris-mox-rwtm.c
@@ -8,16 +8,21 @@
 #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/module.h>
 #include <linux/mutex.h>
-#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
+#include <linux/sysfs.h>
 #include <linux/types.h>
 
 #define DRIVER_NAME		"turris-mox-rwtm"
-- 
2.44.2

[PATCH 08/19] firmware: turris-mox-rwtm: Use kobj_type's default_groups member for automatic sysfs files creation

[Marek Behún]

Put the turris-mox-rwtm attribute group into the .default_groups
member of the underlying kobject type and drop the manual
sysfs_create_files() / sysfs_remove_files(). The kobject library will
take care of this in it's internal code.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/firmware/turris-mox-rwtm.c | 71 ++++++++++++++----------------
 1 file changed, 32 insertions(+), 39 deletions(-)

diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
index 5675f94a3ff2..441409fefc59 100644
--- a/drivers/firmware/turris-mox-rwtm.c
+++ b/drivers/firmware/turris-mox-rwtm.c
@@ -113,6 +113,36 @@ static inline struct mox_rwtm *to_rwtm(struct kobject *kobj)
 	return container_of(kobj, struct mox_kobject, kobj)->rwtm;
 }
 
+#define MOX_ATTR_RO(name, format, cat)				\
+static ssize_t							\
+name##_show(struct kobject *kobj, struct kobj_attribute *a,	\
+	    char *buf)						\
+{								\
+	struct mox_rwtm *rwtm = to_rwtm(kobj);	\
+	if (!rwtm->has_##cat)					\
+		return -ENODATA;				\
+	return sprintf(buf, format, rwtm->name);		\
+}								\
+static struct kobj_attribute mox_attr_##name = __ATTR_RO(name)
+
+MOX_ATTR_RO(serial_number, "%016llX\n", board_info);
+MOX_ATTR_RO(board_version, "%i\n", board_info);
+MOX_ATTR_RO(ram_size, "%i\n", board_info);
+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 *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
+};
+ATTRIBUTE_GROUPS(mox_rwtm);
+
 static void mox_kobj_release(struct kobject *kobj)
 {
 	kfree(to_rwtm(kobj)->kobj);
@@ -121,6 +151,7 @@ static void mox_kobj_release(struct kobject *kobj)
 static const struct kobj_type mox_kobj_ktype = {
 	.release	= mox_kobj_release,
 	.sysfs_ops	= &kobj_sysfs_ops,
+	.default_groups	= mox_rwtm_groups,
 };
 
 static int mox_kobj_create(struct mox_rwtm *rwtm)
@@ -140,25 +171,6 @@ static int mox_kobj_create(struct mox_rwtm *rwtm)
 	return 0;
 }
 
-#define MOX_ATTR_RO(name, format, cat)				\
-static ssize_t							\
-name##_show(struct kobject *kobj, struct kobj_attribute *a,	\
-	    char *buf)						\
-{								\
-	struct mox_rwtm *rwtm = to_rwtm(kobj);	\
-	if (!rwtm->has_##cat)					\
-		return -ENODATA;				\
-	return sprintf(buf, format, rwtm->name);		\
-}								\
-static struct kobj_attribute mox_attr_##name = __ATTR_RO(name)
-
-MOX_ATTR_RO(serial_number, "%016llX\n", board_info);
-MOX_ATTR_RO(board_version, "%i\n", board_info);
-MOX_ATTR_RO(ram_size, "%i\n", board_info);
-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 int mox_get_status(enum mbox_cmd cmd, u32 retval)
 {
 	if (MBOX_STS_CMD(retval) != cmd)
@@ -173,16 +185,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);
@@ -507,12 +509,6 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 		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);
@@ -526,7 +522,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;
+		goto put_kobj;
 	}
 
 	init_completion(&rwtm->cmd_done);
@@ -564,8 +560,6 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 
 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;
@@ -576,7 +570,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);
 }
-- 
2.44.2

[PATCH 09/19] firmware: turris-mox-rwtm: Simplify debugfs code

[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.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/firmware/turris-mox-rwtm.c | 44 ++++++++----------------------
 1 file changed, 11 insertions(+), 33 deletions(-)

diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
index 441409fefc59..6d1e0b1dd2b4 100644
--- a/drivers/firmware/turris-mox-rwtm.c
+++ b/drivers/firmware/turris-mox-rwtm.c
@@ -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
@@ -450,39 +449,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);
+
+	devm_add_action_or_reset(rwtm->dev, rwtm_debugfs_release, root);
+
+	debugfs_create_file_unsafe("do_sign", 0600, root, rwtm, &do_sign_fops);
 }
 #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
@@ -548,11 +531,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");
 
@@ -569,7 +548,6 @@ static void turris_mox_rwtm_remove(struct platform_device *pdev)
 {
 	struct mox_rwtm *rwtm = platform_get_drvdata(pdev);
 
-	rwtm_unregister_debugfs(rwtm);
 	kobject_put(rwtm_to_kobj(rwtm));
 	mbox_free_channel(rwtm->mbox);
 }
-- 
2.44.2

[PATCH 10/19] firmware: turris-mox-rwtm: Simplify driver kobject code

[Marek Behún]

Drop the mox_kobject wrapper that needs to be allocated, instead put the
kobject directly into the driver private structure. This allows us to
drop one kzalloc() call.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/firmware/turris-mox-rwtm.c | 36 ++++++++----------------------
 1 file changed, 9 insertions(+), 27 deletions(-)

diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
index 6d1e0b1dd2b4..84ec72575c4d 100644
--- a/drivers/firmware/turris-mox-rwtm.c
+++ b/drivers/firmware/turris-mox-rwtm.c
@@ -21,7 +21,6 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/platform_device.h>
-#include <linux/slab.h>
 #include <linux/sysfs.h>
 #include <linux/types.h>
 
@@ -58,13 +57,11 @@ 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 kobject kobj;
 	struct hwrng hwrng;
 
 	struct armada_37xx_rwtm_rx_msg reply;
@@ -97,19 +94,9 @@ 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;
+	return container_of(kobj, struct mox_rwtm, kobj);
 }
 
 #define MOX_ATTR_RO(name, format, cat)				\
@@ -142,9 +129,8 @@ static struct attribute *mox_rwtm_attrs[] = {
 };
 ATTRIBUTE_GROUPS(mox_rwtm);
 
-static void mox_kobj_release(struct kobject *kobj)
+static void mox_kobj_release(struct kobject *)
 {
-	kfree(to_rwtm(kobj)->kobj);
 }
 
 static const struct kobj_type mox_kobj_ktype = {
@@ -155,18 +141,14 @@ static const struct kobj_type mox_kobj_ktype = {
 
 static int mox_kobj_create(struct mox_rwtm *rwtm)
 {
-	rwtm->kobj = kzalloc(sizeof(*rwtm->kobj), GFP_KERNEL);
-	if (!rwtm->kobj)
-		return -ENOMEM;
+	struct kobject *kobj = &rwtm->kobj;
 
-	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));
+	kobject_init(kobj, &mox_kobj_ktype);
+	if (kobject_add(kobj, firmware_kobj, "turris-mox-rwtm")) {
+		kobject_put(kobj);
 		return -ENXIO;
 	}
 
-	rwtm->kobj->rwtm = rwtm;
-
 	return 0;
 }
 
@@ -540,7 +522,7 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 free_channel:
 	mbox_free_channel(rwtm->mbox);
 put_kobj:
-	kobject_put(rwtm_to_kobj(rwtm));
+	kobject_put(&rwtm->kobj);
 	return ret;
 }
 
@@ -548,7 +530,7 @@ static void turris_mox_rwtm_remove(struct platform_device *pdev)
 {
 	struct mox_rwtm *rwtm = platform_get_drvdata(pdev);
 
-	kobject_put(rwtm_to_kobj(rwtm));
+	kobject_put(&rwtm->kobj);
 	mbox_free_channel(rwtm->mbox);
 }
 
-- 
2.44.2

[PATCH 11/19] firmware: turris-mox-rwtm: Return true error code if kobject_add() fails

[Marek Behún]

Return the error code from kobject_add() if it fails, instead of -ENXIO.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/firmware/turris-mox-rwtm.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
index 84ec72575c4d..cdbe244be694 100644
--- a/drivers/firmware/turris-mox-rwtm.c
+++ b/drivers/firmware/turris-mox-rwtm.c
@@ -142,14 +142,15 @@ static const struct kobj_type mox_kobj_ktype = {
 static int mox_kobj_create(struct mox_rwtm *rwtm)
 {
 	struct kobject *kobj = &rwtm->kobj;
+	int ret;
 
 	kobject_init(kobj, &mox_kobj_ktype);
-	if (kobject_add(kobj, firmware_kobj, "turris-mox-rwtm")) {
+
+	ret = kobject_add(kobj, firmware_kobj, "turris-mox-rwtm");
+	if (ret)
 		kobject_put(kobj);
-		return -ENXIO;
-	}
 
-	return 0;
+	return ret;
 }
 
 static int mox_get_status(enum mbox_cmd cmd, u32 retval)
-- 
2.44.2

[PATCH 12/19] firmware: turris-mox-rwtm: Convert rest to devm_* and get rid of driver .remove()

[Marek Behún]

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

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/firmware/turris-mox-rwtm.c | 41 +++++++++++++++---------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
index cdbe244be694..153772721901 100644
--- a/drivers/firmware/turris-mox-rwtm.c
+++ b/drivers/firmware/turris-mox-rwtm.c
@@ -139,6 +139,11 @@ static const struct kobj_type mox_kobj_ktype = {
 	.default_groups	= mox_rwtm_groups,
 };
 
+static void mox_devm_kobj_release(void *kobj)
+{
+	kobject_put(kobj);
+}
+
 static int mox_kobj_create(struct mox_rwtm *rwtm)
 {
 	struct kobject *kobj = &rwtm->kobj;
@@ -146,11 +151,11 @@ static int mox_kobj_create(struct mox_rwtm *rwtm)
 
 	kobject_init(kobj, &mox_kobj_ktype);
 
-	ret = kobject_add(kobj, firmware_kobj, "turris-mox-rwtm");
+	ret = devm_add_action_or_reset(rwtm->dev, mox_devm_kobj_release, kobj);
 	if (ret)
-		kobject_put(kobj);
+		return ret;
 
-	return ret;
+	return kobject_add(kobj, firmware_kobj, "turris-mox-rwtm");
 }
 
 static int mox_get_status(enum mbox_cmd cmd, u32 retval)
@@ -453,6 +458,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 int turris_mox_rwtm_probe(struct platform_device *pdev)
 {
 	struct mox_rwtm *rwtm;
@@ -488,9 +498,13 @@ 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 put_kobj;
+		return ret;
 	}
 
+	ret = devm_add_action_or_reset(dev, rwtm_devm_mbox_release, rwtm->mbox);
+	if (ret < 0)
+		return ret;
+
 	init_completion(&rwtm->cmd_done);
 
 	ret = mox_get_board_info(rwtm);
@@ -501,7 +515,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";
@@ -511,7 +525,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);
@@ -519,20 +533,6 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 	dev_info(dev, "HWRNG successfully registered\n");
 
 	return 0;
-
-free_channel:
-	mbox_free_channel(rwtm->mbox);
-put_kobj:
-	kobject_put(&rwtm->kobj);
-	return ret;
-}
-
-static void turris_mox_rwtm_remove(struct platform_device *pdev)
-{
-	struct mox_rwtm *rwtm = platform_get_drvdata(pdev);
-
-	kobject_put(&rwtm->kobj);
-	mbox_free_channel(rwtm->mbox);
 }
 
 static const struct of_device_id turris_mox_rwtm_match[] = {
@@ -545,7 +545,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,
-- 
2.44.2

[PATCH 13/19] firmware: turris-mox-rwtm: Use dev_err_probe() where possible

[Marek Behún]

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

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/firmware/turris-mox-rwtm.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
index 153772721901..3c3f8ae23809 100644
--- a/drivers/firmware/turris-mox-rwtm.c
+++ b/drivers/firmware/turris-mox-rwtm.c
@@ -480,10 +480,9 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	ret = mox_kobj_create(rwtm);
-	if (ret < 0) {
-		dev_err(dev, "Cannot create turris-mox-rwtm kobject!\n");
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "Cannot create turris-mox-rwtm kobject!\n");
 
 	platform_set_drvdata(pdev, rwtm);
 
@@ -495,10 +494,11 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 	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 (ret == -EPROBE_DEFER)
+			return ret;
+
+		return dev_err_probe(dev, ret,
+				     "Cannot request mailbox channel!\n");
 	}
 
 	ret = devm_add_action_or_reset(dev, rwtm_devm_mbox_release, rwtm->mbox);
@@ -523,10 +523,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 < 0)
+		return dev_err_probe(dev, ret, "Cannot register HWRNG!\n");
 
 	rwtm_register_debugfs(rwtm);
 
-- 
2.44.2

[PATCH 14/19] firmware: turris-mox-rwtm: Rearrange probe calls

[Marek Behún]

Rearrange probe calls:
- create the kobject (and corresponding sysfs files) only after the
  mailbox is created and board info is read
- initialize the completion before mailbox channel is requested

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/firmware/turris-mox-rwtm.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
index 3c3f8ae23809..b9be3c806695 100644
--- a/drivers/firmware/turris-mox-rwtm.c
+++ b/drivers/firmware/turris-mox-rwtm.c
@@ -479,14 +479,10 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 	if (!rwtm->buf)
 		return -ENOMEM;
 
-	ret = mox_kobj_create(rwtm);
-	if (ret < 0)
-		return dev_err_probe(dev, ret,
-				     "Cannot create turris-mox-rwtm kobject!\n");
-
 	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;
@@ -505,12 +501,15 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 	if (ret < 0)
 		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);
 
+	ret = mox_kobj_create(rwtm);
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "Cannot create turris-mox-rwtm kobject!\n");
+
 	ret = check_get_random_support(rwtm);
 	if (ret < 0) {
 		dev_notice(dev,
-- 
2.44.2

[PATCH 15/19] firmware: turris-mox-rwtm: Drop redundant device pointer

[Marek Behún]

Drop redundant device pointer from driver's private structure.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/firmware/turris-mox-rwtm.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
index b9be3c806695..5f4dd919ce2e 100644
--- a/drivers/firmware/turris-mox-rwtm.c
+++ b/drivers/firmware/turris-mox-rwtm.c
@@ -58,7 +58,6 @@ enum mbox_cmd {
 };
 
 struct mox_rwtm {
-	struct device *dev;
 	struct mbox_client mbox_client;
 	struct mbox_chan *mbox;
 	struct kobject kobj;
@@ -94,6 +93,11 @@ struct mox_rwtm {
 #endif
 };
 
+static inline struct device *rwtm_dev(struct mox_rwtm *rwtm)
+{
+	return rwtm->mbox_client.dev;
+}
+
 static inline struct mox_rwtm *to_rwtm(struct kobject *kobj)
 {
 	return container_of(kobj, struct mox_rwtm, kobj);
@@ -151,7 +155,8 @@ static int mox_kobj_create(struct mox_rwtm *rwtm)
 
 	kobject_init(kobj, &mox_kobj_ktype);
 
-	ret = devm_add_action_or_reset(rwtm->dev, mox_devm_kobj_release, kobj);
+	ret = devm_add_action_or_reset(rwtm_dev(rwtm), mox_devm_kobj_release,
+				       kobj);
 	if (ret)
 		return ret;
 
@@ -196,6 +201,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;
@@ -210,10 +216,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;
@@ -245,9 +251,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;
@@ -448,7 +454,7 @@ static void rwtm_register_debugfs(struct mox_rwtm *rwtm)
 
 	root = debugfs_create_dir("turris-mox-rwtm", NULL);
 
-	devm_add_action_or_reset(rwtm->dev, rwtm_debugfs_release, root);
+	devm_add_action_or_reset(rwtm_dev(rwtm), rwtm_debugfs_release, root);
 
 	debugfs_create_file_unsafe("do_sign", 0600, root, rwtm, &do_sign_fops);
 }
@@ -473,7 +479,6 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 	if (!rwtm)
 		return -ENOMEM;
 
-	rwtm->dev = dev;
 	rwtm->buf = dmam_alloc_coherent(dev, PAGE_SIZE, &rwtm->buf_phys,
 					GFP_KERNEL);
 	if (!rwtm->buf)
-- 
2.44.2

[PATCH 16/19] firmware: turris-mox-rwtm: Use devm_mutex_init() instead of mutex_init()

[Marek Behún]

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

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/firmware/turris-mox-rwtm.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
index 5f4dd919ce2e..f753e98f1ca7 100644
--- a/drivers/firmware/turris-mox-rwtm.c
+++ b/drivers/firmware/turris-mox-rwtm.c
@@ -486,7 +486,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 < 0)
+		return ret;
+
 	init_completion(&rwtm->cmd_done);
 
 	rwtm->mbox_client.dev = dev;
-- 
2.44.2

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

[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/ZmLhQBdmg613KdET@gondor.apana.org.au/

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/firmware/turris-mox-rwtm.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
index f753e98f1ca7..5991434f62a3 100644
--- a/drivers/firmware/turris-mox-rwtm.c
+++ b/drivers/firmware/turris-mox-rwtm.c
@@ -292,7 +292,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;
 
@@ -527,7 +527,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 < 0)
-- 
2.44.2

[PATCH 18/19] firmware: turris-mox-rwtm: Deduplicate command execution code

[Marek Behún]

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

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/firmware/turris-mox-rwtm.c | 98 ++++++++++++------------------
 1 file changed, 39 insertions(+), 59 deletions(-)

diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
index 5991434f62a3..ea5ff1827d08 100644
--- a/drivers/firmware/turris-mox-rwtm.c
+++ b/drivers/firmware/turris-mox-rwtm.c
@@ -189,6 +189,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;
@@ -202,19 +230,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");
@@ -241,15 +260,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 == -ENOSYS) {
@@ -272,38 +283,24 @@ 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;
 
 	if (max > PAGE_SIZE)
 		max = PAGE_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;
@@ -311,15 +308,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;
 
@@ -367,7 +356,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;
@@ -400,23 +388,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
-- 
2.44.2

[PATCH 19/19] firmware: turris-mox-rwtm: Use EOPNOTSUPP instead of ENOSYS

[Marek Behún]

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

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/firmware/turris-mox-rwtm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
index ea5ff1827d08..d1e5ee37aca4 100644
--- a/drivers/firmware/turris-mox-rwtm.c
+++ b/drivers/firmware/turris-mox-rwtm.c
@@ -170,7 +170,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
@@ -237,7 +237,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) {
@@ -263,7 +263,7 @@ static int mox_get_board_info(struct mox_rwtm *rwtm)
 	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 == -ENOSYS) {
+	} else if (ret == -EOPNOTSUPP) {
 		dev_notice(dev,
 			   "Firmware does not support the ECDSA_PUB_KEY command\n");
 	} else if (ret < 0) {
-- 
2.44.2

Re: [PATCH 03/19] firmware: turris-mox-rwtm: Use PAGE_SIZE instead of hardcoded 4096

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1003 bytes --]

On Wed, 12 Jun 2024, Marek Behún wrote:

> The 4096 bytes limit in mox_hwrng_read() is due to the DMA buffer being
> allocated to one PAGE_SIZE bytes. The PAGE_SIZE macro is used when
> allocating the buffer, use it in mox_hwrng_read() as well.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>  drivers/firmware/turris-mox-rwtm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
> index 3f4758e03c81..5acdde1bb6d9 100644
> --- a/drivers/firmware/turris-mox-rwtm.c
> +++ b/drivers/firmware/turris-mox-rwtm.c
> @@ -287,8 +287,8 @@ 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;
> +	if (max > PAGE_SIZE)
> +		max = PAGE_SIZE;

Wouldn't it be better to bind these to the alloc side with a local define 
that is set to PAGE_SIZE?

-- 
 i.

Re: [PATCH 04/19] firmware: turris-mox-rwtm: Use ETH_ALEN instead of hardcoded 6

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1043 bytes --]

On Wed, 12 Jun 2024, Marek Behún wrote:

> Use the ETH_ALEN macro instead of hardcoded 6 for MAC address length.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>  drivers/firmware/turris-mox-rwtm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
> index 5acdde1bb6d9..1b708a0760e6 100644
> --- a/drivers/firmware/turris-mox-rwtm.c
> +++ b/drivers/firmware/turris-mox-rwtm.c
> @@ -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/module.h>
>  #include <linux/mutex.h>
> @@ -65,7 +66,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];

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH 02/19] firmware: turris-mox-rwtm: Fix checking return value of wait_for_completion_timeout()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1954 bytes --]

On Wed, 12 Jun 2024, Marek Behún wrote:

> 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")
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>  drivers/firmware/turris-mox-rwtm.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
> index f1f9160c4195..3f4758e03c81 100644
> --- a/drivers/firmware/turris-mox-rwtm.c
> +++ b/drivers/firmware/turris-mox-rwtm.c
> @@ -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;

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH 08/19] firmware: turris-mox-rwtm: Use kobj_type's default_groups member for automatic sysfs files creation

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2439 bytes --]

On Wed, 12 Jun 2024, Marek Behún wrote:

> Put the turris-mox-rwtm attribute group into the .default_groups
> member of the underlying kobject type and drop the manual
> sysfs_create_files() / sysfs_remove_files(). The kobject library will
> take care of this in it's internal code.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>  drivers/firmware/turris-mox-rwtm.c | 71 ++++++++++++++----------------
>  1 file changed, 32 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
> index 5675f94a3ff2..441409fefc59 100644
> --- a/drivers/firmware/turris-mox-rwtm.c
> +++ b/drivers/firmware/turris-mox-rwtm.c
> @@ -113,6 +113,36 @@ static inline struct mox_rwtm *to_rwtm(struct kobject *kobj)
>  	return container_of(kobj, struct mox_kobject, kobj)->rwtm;
>  }
>  
> +#define MOX_ATTR_RO(name, format, cat)				\
> +static ssize_t							\
> +name##_show(struct kobject *kobj, struct kobj_attribute *a,	\
> +	    char *buf)						\
> +{								\
> +	struct mox_rwtm *rwtm = to_rwtm(kobj);	\

Fix \ alignment to match the others while you move the code around.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

> +	if (!rwtm->has_##cat)					\
> +		return -ENODATA;				\
> +	return sprintf(buf, format, rwtm->name);		\
> +}								\
> +static struct kobj_attribute mox_attr_##name = __ATTR_RO(name)
> +
> +MOX_ATTR_RO(serial_number, "%016llX\n", board_info);
> +MOX_ATTR_RO(board_version, "%i\n", board_info);
> +MOX_ATTR_RO(ram_size, "%i\n", board_info);
> +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 *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
> +};
> +ATTRIBUTE_GROUPS(mox_rwtm);
> +
>  static void mox_kobj_release(struct kobject *kobj)
>  {
>  	kfree(to_rwtm(kobj)->kobj);
> @@ -121,6 +151,7 @@ static void mox_kobj_release(struct kobject *kobj)
>  static const struct kobj_type mox_kobj_ktype = {
>  	.release	= mox_kobj_release,
>  	.sysfs_ops	= &kobj_sysfs_ops,
> +	.default_groups	= mox_rwtm_groups,
>  };
>  
>  static int mox_kobj_create(struct mox_rwtm *rwtm)

Re: [PATCH 10/19] firmware: turris-mox-rwtm: Simplify driver kobject code

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3386 bytes --]

On Wed, 12 Jun 2024, Marek Behún wrote:

> Drop the mox_kobject wrapper that needs to be allocated, instead put the
> kobject directly into the driver private structure. This allows us to
> drop one kzalloc() call.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>  drivers/firmware/turris-mox-rwtm.c | 36 ++++++++----------------------
>  1 file changed, 9 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
> index 6d1e0b1dd2b4..84ec72575c4d 100644
> --- a/drivers/firmware/turris-mox-rwtm.c
> +++ b/drivers/firmware/turris-mox-rwtm.c
> @@ -21,7 +21,6 @@
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/platform_device.h>
> -#include <linux/slab.h>
>  #include <linux/sysfs.h>
>  #include <linux/types.h>
>  
> @@ -58,13 +57,11 @@ 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 kobject kobj;
>  	struct hwrng hwrng;
>  
>  	struct armada_37xx_rwtm_rx_msg reply;
> @@ -97,19 +94,9 @@ 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;
> +	return container_of(kobj, struct mox_rwtm, kobj);
>  }
>  
>  #define MOX_ATTR_RO(name, format, cat)				\
> @@ -142,9 +129,8 @@ static struct attribute *mox_rwtm_attrs[] = {
>  };
>  ATTRIBUTE_GROUPS(mox_rwtm);
>  
> -static void mox_kobj_release(struct kobject *kobj)
> +static void mox_kobj_release(struct kobject *)
>  {
> -	kfree(to_rwtm(kobj)->kobj);
>  }

Is empty .release necessary at all? I found some kobj_type structs without 
.release so I'd expect it to be unnecessary.

-- 
 i.

>  static const struct kobj_type mox_kobj_ktype = {
> @@ -155,18 +141,14 @@ static const struct kobj_type mox_kobj_ktype = {
>  
>  static int mox_kobj_create(struct mox_rwtm *rwtm)
>  {
> -	rwtm->kobj = kzalloc(sizeof(*rwtm->kobj), GFP_KERNEL);
> -	if (!rwtm->kobj)
> -		return -ENOMEM;
> +	struct kobject *kobj = &rwtm->kobj;
>  
> -	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));
> +	kobject_init(kobj, &mox_kobj_ktype);
> +	if (kobject_add(kobj, firmware_kobj, "turris-mox-rwtm")) {
> +		kobject_put(kobj);
>  		return -ENXIO;
>  	}
>  
> -	rwtm->kobj->rwtm = rwtm;
> -
>  	return 0;
>  }
>  
> @@ -540,7 +522,7 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
>  free_channel:
>  	mbox_free_channel(rwtm->mbox);
>  put_kobj:
> -	kobject_put(rwtm_to_kobj(rwtm));
> +	kobject_put(&rwtm->kobj);
>  	return ret;
>  }
>  
> @@ -548,7 +530,7 @@ static void turris_mox_rwtm_remove(struct platform_device *pdev)
>  {
>  	struct mox_rwtm *rwtm = platform_get_drvdata(pdev);
>  
> -	kobject_put(rwtm_to_kobj(rwtm));
> +	kobject_put(&rwtm->kobj);
>  	mbox_free_channel(rwtm->mbox);
>  }
>  
> 

Re: [PATCH 03/19] firmware: turris-mox-rwtm: Use PAGE_SIZE instead of hardcoded 4096

[Marek Behún]

On Thu, 13 Jun 2024 11:01:42 +0300 (EEST)
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> On Wed, 12 Jun 2024, Marek Behún wrote:
> 
> > The 4096 bytes limit in mox_hwrng_read() is due to the DMA buffer being
> > allocated to one PAGE_SIZE bytes. The PAGE_SIZE macro is used when
> > allocating the buffer, use it in mox_hwrng_read() as well.
> > 
> > Signed-off-by: Marek Behún <kabel@kernel.org>
> > ---
> >  drivers/firmware/turris-mox-rwtm.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
> > index 3f4758e03c81..5acdde1bb6d9 100644
> > --- a/drivers/firmware/turris-mox-rwtm.c
> > +++ b/drivers/firmware/turris-mox-rwtm.c
> > @@ -287,8 +287,8 @@ 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;
> > +	if (max > PAGE_SIZE)
> > +		max = PAGE_SIZE;  
> 
> Wouldn't it be better to bind these to the alloc side with a local define 
> that is set to PAGE_SIZE?

OK.

Re: [PATCH 10/19] firmware: turris-mox-rwtm: Simplify driver kobject code

[Marek Behún]

On Thu, 13 Jun 2024 11:31:43 +0300 (EEST)
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> Is empty .release necessary at all? I found some kobj_type structs without 
> .release so I'd expect it to be unnecessary.

lib/kobject.c function kobject_cleanup() has the following:

  if (t && !t->release)
    pr_debug("'%s' (%p): does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.\n",

Marek

Re: [PATCH 10/19] firmware: turris-mox-rwtm: Simplify driver kobject code

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 678 bytes --]

On Thu, 13 Jun 2024, Marek Behún wrote:

> On Thu, 13 Jun 2024 11:31:43 +0300 (EEST)
> Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> 
> > Is empty .release necessary at all? I found some kobj_type structs without 
> > .release so I'd expect it to be unnecessary.
> 
> lib/kobject.c function kobject_cleanup() has the following:
> 
>   if (t && !t->release)
>     pr_debug("'%s' (%p): does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.\n",

Hmm, the plot thickens... that documentation file says:

'Do not try to get rid of this warning by providing an "empty" release 
function.'

?

-- 
 i.

Re: [PATCH 10/19] firmware: turris-mox-rwtm: Simplify driver kobject code

[Marek Behún]

On Thu, 13 Jun 2024 13:19:47 +0300 (EEST)
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> On Thu, 13 Jun 2024, Marek Behún wrote:
> 
> > On Thu, 13 Jun 2024 11:31:43 +0300 (EEST)
> > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> >   
> > > Is empty .release necessary at all? I found some kobj_type structs without 
> > > .release so I'd expect it to be unnecessary.  
> > 
> > lib/kobject.c function kobject_cleanup() has the following:
> > 
> >   if (t && !t->release)
> >     pr_debug("'%s' (%p): does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.\n",  
> 
> Hmm, the plot thickens... that documentation file says:
> 
> 'Do not try to get rid of this warning by providing an "empty" release 
> function.'
> 
> ?

This whole thing stinks. I will rewrite it so that the attributes will
be under the device's kobject, as they should be. This way I can get
rid of the whole own kobject type.

Then I will add a symlink from /sys/firmware/turris-mox-rwtm to the
device, for sysfs ABI compatibility.

Marek

Re: [PATCH 10/19] firmware: turris-mox-rwtm: Simplify driver kobject code

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1484 bytes --]

On Thu, 13 Jun 2024, Marek Behún wrote:

> On Thu, 13 Jun 2024 13:19:47 +0300 (EEST)
> Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> 
> > On Thu, 13 Jun 2024, Marek Behún wrote:
> > 
> > > On Thu, 13 Jun 2024 11:31:43 +0300 (EEST)
> > > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> > >   
> > > > Is empty .release necessary at all? I found some kobj_type structs without 
> > > > .release so I'd expect it to be unnecessary.  
> > > 
> > > lib/kobject.c function kobject_cleanup() has the following:
> > > 
> > >   if (t && !t->release)
> > >     pr_debug("'%s' (%p): does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.\n",  
> > 
> > Hmm, the plot thickens... that documentation file says:
> > 
> > 'Do not try to get rid of this warning by providing an "empty" release 
> > function.'
> > 
> > ?
> 
> This whole thing stinks. I will rewrite it so that the attributes will
> be under the device's kobject, as they should be. This way I can get
> rid of the whole own kobject type.

Yeah, I didn't really understand why they weren't there in the first 
place.

> Then I will add a symlink from /sys/firmware/turris-mox-rwtm to the
> device, for sysfs ABI compatibility.


BTW (unrelated to any of your patches unless I missed something)...
You might want to check one unlock_mutex: label that seemed to be 100% 
duplicate the tail of the return path.


-- 
 i.

Re: [PATCH 10/19] firmware: turris-mox-rwtm: Simplify driver kobject code

[Marek Behún]

On Thu, 13 Jun 2024 16:37:23 +0300 (EEST)
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> On Thu, 13 Jun 2024, Marek Behún wrote:
> 
> > On Thu, 13 Jun 2024 13:19:47 +0300 (EEST)
> > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> >   
> > > On Thu, 13 Jun 2024, Marek Behún wrote:
> > >   
> > > > On Thu, 13 Jun 2024 11:31:43 +0300 (EEST)
> > > > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> > > >     
> > > > > Is empty .release necessary at all? I found some kobj_type structs without 
> > > > > .release so I'd expect it to be unnecessary.    
> > > > 
> > > > lib/kobject.c function kobject_cleanup() has the following:
> > > > 
> > > >   if (t && !t->release)
> > > >     pr_debug("'%s' (%p): does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.\n",    
> > > 
> > > Hmm, the plot thickens... that documentation file says:
> > > 
> > > 'Do not try to get rid of this warning by providing an "empty" release 
> > > function.'
> > > 
> > > ?  
> > 
> > This whole thing stinks. I will rewrite it so that the attributes will
> > be under the device's kobject, as they should be. This way I can get
> > rid of the whole own kobject type.  
> 
> Yeah, I didn't really understand why they weren't there in the first 
> place.
> 
> > Then I will add a symlink from /sys/firmware/turris-mox-rwtm to the
> > device, for sysfs ABI compatibility.  
> 
> 
> BTW (unrelated to any of your patches unless I missed something)...
> You might want to check one unlock_mutex: label that seemed to be 100% 
> duplicate the tail of the return path.

Do you mean this?

          mutex_unlock(&rwtm->busy);
          return len;
  unlock_mutex:
          mutex_unlock(&rwtm->busy);
          return ret;

It's not 100% duplicate, but yes, I could do ret = len.

The thing is that this whole debugfs code is going away. I wanted to
clean the driver up a little before converting this debugfs code to
keyctl (which is the correct API for the thing that is now exposed to
userspace via debugfs).

Marek

Re: [PATCH 10/19] firmware: turris-mox-rwtm: Simplify driver kobject code

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2347 bytes --]

On Thu, 13 Jun 2024, Marek Behún wrote:

> On Thu, 13 Jun 2024 16:37:23 +0300 (EEST)
> Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> 
> > On Thu, 13 Jun 2024, Marek Behún wrote:
> > 
> > > On Thu, 13 Jun 2024 13:19:47 +0300 (EEST)
> > > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> > >   
> > > > On Thu, 13 Jun 2024, Marek Behún wrote:
> > > >   
> > > > > On Thu, 13 Jun 2024 11:31:43 +0300 (EEST)
> > > > > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> > > > >     
> > > > > > Is empty .release necessary at all? I found some kobj_type structs without 
> > > > > > .release so I'd expect it to be unnecessary.    
> > > > > 
> > > > > lib/kobject.c function kobject_cleanup() has the following:
> > > > > 
> > > > >   if (t && !t->release)
> > > > >     pr_debug("'%s' (%p): does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.\n",    
> > > > 
> > > > Hmm, the plot thickens... that documentation file says:
> > > > 
> > > > 'Do not try to get rid of this warning by providing an "empty" release 
> > > > function.'
> > > > 
> > > > ?  
> > > 
> > > This whole thing stinks. I will rewrite it so that the attributes will
> > > be under the device's kobject, as they should be. This way I can get
> > > rid of the whole own kobject type.  
> > 
> > Yeah, I didn't really understand why they weren't there in the first 
> > place.
> > 
> > > Then I will add a symlink from /sys/firmware/turris-mox-rwtm to the
> > > device, for sysfs ABI compatibility.  
> > 
> > 
> > BTW (unrelated to any of your patches unless I missed something)...
> > You might want to check one unlock_mutex: label that seemed to be 100% 
> > duplicate the tail of the return path.
> 
> Do you mean this?
> 
>           mutex_unlock(&rwtm->busy);
>           return len;
>   unlock_mutex:
>           mutex_unlock(&rwtm->busy);
>           return ret;
> 
> It's not 100% duplicate, but yes, I could do ret = len.

Ah sorry, I didn't realize the variable name was different.

> The thing is that this whole debugfs code is going away. I wanted to
> clean the driver up a little before converting this debugfs code to
> keyctl (which is the correct API for the thing that is now exposed to
> userspace via debugfs).


-- 
 i.