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

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

[Marek Behún]

Hi Gregory, Arnd, Andy et al.

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

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

Changes since v1:
- complete dropping of driver's own kobject type

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 | 381 ++++++++++++-----------------
 1 file changed, 155 insertions(+), 226 deletions(-)

-- 
2.44.2

[PATCH v2 01/17] 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 v2 02/17] 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>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 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 v2 03/17] firmware: turris-mox-rwtm: Use macro constant 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. Use new local macro constant
RWTM_DMA_BUFFER_SIZE at allocation time and when used in mox_hwrng_read().

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

diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
index 3f4758e03c81..b8deb13aed98 100644
--- a/drivers/firmware/turris-mox-rwtm.c
+++ b/drivers/firmware/turris-mox-rwtm.c
@@ -19,6 +19,8 @@
 
 #define DRIVER_NAME		"turris-mox-rwtm"
 
+#define RWTM_DMA_BUFFER_SIZE	PAGE_SIZE
+
 /*
  * 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 +289,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 > RWTM_DMA_BUFFER_SIZE)
+		max = RWTM_DMA_BUFFER_SIZE;
 
 	msg.command = MBOX_CMD_GET_RANDOM;
 	msg.args[0] = 1;
@@ -479,8 +481,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;
 
-- 
2.44.2

Re: [PATCH v2 03/17] firmware: turris-mox-rwtm: Use macro constant instead of hardcoded 4096

[Ilpo Järvinen]

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

On Thu, 13 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. Use new local macro constant
> RWTM_DMA_BUFFER_SIZE at allocation time and when used in mox_hwrng_read().
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>  drivers/firmware/turris-mox-rwtm.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
> index 3f4758e03c81..b8deb13aed98 100644
> --- a/drivers/firmware/turris-mox-rwtm.c
> +++ b/drivers/firmware/turris-mox-rwtm.c
> @@ -19,6 +19,8 @@
>  
>  #define DRIVER_NAME		"turris-mox-rwtm"
>  
> +#define RWTM_DMA_BUFFER_SIZE	PAGE_SIZE
> +
>  /*
>   * 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 +289,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 > RWTM_DMA_BUFFER_SIZE)
> +		max = RWTM_DMA_BUFFER_SIZE;
>  
>  	msg.command = MBOX_CMD_GET_RANDOM;
>  	msg.args[0] = 1;
> @@ -479,8 +481,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;

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

-- 
 i.

Re: [PATCH v2 03/17] firmware: turris-mox-rwtm: Use macro constant instead of hardcoded 4096

[Andy Shevchenko]

On Thu, Jun 13, 2024 at 6:10 PM Marek Behún <kabel@kernel.org> wrote:
>
> 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().

...

> +#define RWTM_DMA_BUFFER_SIZE   PAGE_SIZE

Is it guaranteed to work on any possible PAGE_SIZE settings? We have
some architectures that may have it quite different to 4k. Even if
this driver will never be run in such an environment, strictly
speaking it might not be a good idea to replace 4096 with PAGE_SIZE.

...

> +       if (max > RWTM_DMA_BUFFER_SIZE)
> +               max = RWTM_DMA_BUFFER_SIZE;

You probably want max() from minmax.c.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 03/17] firmware: turris-mox-rwtm: Use macro constant instead of hardcoded 4096

[Arnd Bergmann]

On Thu, Jun 13, 2024, at 19:51, Andy Shevchenko wrote:
> On Thu, Jun 13, 2024 at 6:10 PM Marek Behún <kabel@kernel.org> wrote:
>>
>> 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().
>
> ...
>
>> +#define RWTM_DMA_BUFFER_SIZE   PAGE_SIZE
>
> Is it guaranteed to work on any possible PAGE_SIZE settings? We have
> some architectures that may have it quite different to 4k. Even if
> this driver will never be run in such an environment, strictly
> speaking it might not be a good idea to replace 4096 with PAGE_SIZE.

This is a Cortex-A53, so it does support 64KB page size and the
driver should be written to work with that even if it's silly
to use that configuration in practice.

I did not check if using PAGE_SIZE or SZ_4K is the correct
choice for CONFIG_PAGE_SIZE_64KB kernel here.

      Arnd

Re: [PATCH v2 03/17] firmware: turris-mox-rwtm: Use macro constant instead of hardcoded 4096

[Marek Behún]

On Thu, 13 Jun 2024 19:51:56 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Thu, Jun 13, 2024 at 6:10 PM Marek Behún <kabel@kernel.org> wrote:
> >
> > 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().  
> 
> ...
> 
> > +#define RWTM_DMA_BUFFER_SIZE   PAGE_SIZE  
> 
> Is it guaranteed to work on any possible PAGE_SIZE settings? We have
> some architectures that may have it quite different to 4k. Even if
> this driver will never be run in such an environment, strictly
> speaking it might not be a good idea to replace 4096 with PAGE_SIZE.

It would work with 64K, but I will use SZ_4K from sizes.h.

> ...
> 
> > +       if (max > RWTM_DMA_BUFFER_SIZE)
> > +               max = RWTM_DMA_BUFFER_SIZE;  
> 
> You probably want max() from minmax.c.

You mean min() :)

Re: [PATCH v2 03/17] firmware: turris-mox-rwtm: Use macro constant instead of hardcoded 4096

[Andy Shevchenko]

On Mon, Jun 17, 2024 at 12:58 PM Marek Behún <kabel@kernel.org> wrote:
> On Thu, 13 Jun 2024 19:51:56 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Thu, Jun 13, 2024 at 6:10 PM Marek Behún <kabel@kernel.org> wrote:

...

> > > +       if (max > RWTM_DMA_BUFFER_SIZE)
> > > +               max = RWTM_DMA_BUFFER_SIZE;
> >
> > You probably want max() from minmax.c.
>
> You mean min() :)

Right, or even clamp().

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 04/17] 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>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 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 b8deb13aed98..3a7969d15d27 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>
@@ -67,7 +68,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 v2 05/17] 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 3a7969d15d27..703b05c404e4 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"
 
@@ -65,13 +66,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
@@ -83,7 +84,7 @@ struct mox_rwtm {
 	 */
 	struct dentry *debugfs_root;
 	u32 last_sig[34];
-	int last_sig_done;
+	bool last_sig_done;
 #endif
 };
 
@@ -227,7 +228,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);
@@ -254,7 +255,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],
@@ -351,7 +352,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;
 }
@@ -417,7 +418,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 v2 06/17] 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 703b05c404e4..fb263f9bbab9 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>
@@ -29,6 +30,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)
@@ -83,7 +90,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
 };
@@ -344,14 +351,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;
@@ -366,8 +374,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 */
@@ -388,17 +395,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;
@@ -416,8 +424,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 v2 07/17] 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 fb263f9bbab9..75a98386a2b0 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 v2 08/17] firmware: turris-mox-rwtm: Don't create own kobject type

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

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

diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
index 75a98386a2b0..d17dc0679439 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>
 
@@ -60,13 +59,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;
@@ -100,59 +96,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);
@@ -161,6 +115,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)
@@ -175,16 +140,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);
@@ -487,6 +442,11 @@ static inline void rwtm_unregister_debugfs(struct mox_rwtm *rwtm)
 }
 #endif
 
+static void rwtm_firmware_symlink_drop(void *)
+{
+	sysfs_remove_link(firmware_kobj, DRIVER_NAME);
+}
+
 static int turris_mox_rwtm_probe(struct platform_device *pdev)
 {
 	struct mox_rwtm *rwtm;
@@ -503,18 +463,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);
@@ -528,7 +476,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);
@@ -562,14 +510,18 @@ 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, NULL);
+
 	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;
 }
 
@@ -578,8 +530,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);
 }
 
@@ -597,6 +547,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);
-- 
2.44.2

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

[Ilpo Järvinen]

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

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

> 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.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>

Much better, thanks.

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

-- 
 i.

> ---
>  drivers/firmware/turris-mox-rwtm.c | 107 ++++++++---------------------
>  1 file changed, 29 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
> index 75a98386a2b0..d17dc0679439 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>
>  
> @@ -60,13 +59,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;
> @@ -100,59 +96,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);
> @@ -161,6 +115,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)
> @@ -175,16 +140,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);
> @@ -487,6 +442,11 @@ static inline void rwtm_unregister_debugfs(struct mox_rwtm *rwtm)
>  }
>  #endif
>  
> +static void rwtm_firmware_symlink_drop(void *)
> +{
> +	sysfs_remove_link(firmware_kobj, DRIVER_NAME);
> +}
> +
>  static int turris_mox_rwtm_probe(struct platform_device *pdev)
>  {
>  	struct mox_rwtm *rwtm;
> @@ -503,18 +463,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);
> @@ -528,7 +476,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);
> @@ -562,14 +510,18 @@ 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, NULL);
> +
>  	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;
>  }
>  
> @@ -578,8 +530,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);
>  }
>  
> @@ -597,6 +547,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,
>  	},

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

[Andy Shevchenko]

On Thu, Jun 13, 2024 at 6:11 PM Marek Behún <kabel@kernel.org> wrote:
>
> In order to create attribute files in /sys/firmware/turris-mox-rwtm,
> this driver creates it's own kobject type.

its

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

...


> +static void rwtm_firmware_symlink_drop(void *)

Interesting, can we actually name the parameter, like "unused"?

> +{
> +       sysfs_remove_link(firmware_kobj, DRIVER_NAME);

But why not provide a fimware_kobj pointer as a parameter?

> +}

...

> +       /*
> +        * 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, NULL);

This means that it will remove the link in case devm_add_action()
fails. Is it okay?

-- 
With Best Regards,
Andy Shevchenko

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

[Arnd Bergmann]

On Thu, Jun 13, 2024, at 22:32, Andy Shevchenko wrote:
> On Thu, Jun 13, 2024 at 6:11 PM Marek Behún <kabel@kernel.org> wrote:
>
>
>> +static void rwtm_firmware_symlink_drop(void *)
>
> Interesting, can we actually name the parameter, like "unused"?

Agreed.

I was surprised that the compiler doesn't warn about it and
checked: apparently this is a c23 language feature, so
newer versions of gcc now treats it as a gnu extension for
--std=gnu11 as well. gcc-10 and older versions however fail:

error: parameter name omitted

and clang-19 still warns 

warning: omitting the parameter name in a function definition is a C2x extension [-Wc2x-extensions]

    Arnd

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

[Marek Behún]

On Thu, 13 Jun 2024 22:32:28 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Thu, Jun 13, 2024 at 6:11 PM Marek Behún <kabel@kernel.org> wrote:
> >
> > In order to create attribute files in /sys/firmware/turris-mox-rwtm,
> > this driver creates it's own kobject type.  
> 
> its
> 
> > 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.  
> 
> ...
> 
> 
> > +static void rwtm_firmware_symlink_drop(void *)  
> 
> Interesting, can we actually name the parameter, like "unused"?
> 
> > +{
> > +       sysfs_remove_link(firmware_kobj, DRIVER_NAME);  
> 
> But why not provide a fimware_kobj pointer as a parameter?
> 
> > +}  
> 
> ...
> 
> > +       /*
> > +        * 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, NULL);  
> 
> This means that it will remove the link in case devm_add_action()
> fails. Is it okay?

IMO rather to drop the symlink (which is created for backwards
compatibility anyway) in the improbable case that
devm_add_action() fails, then than to leave it dangling there on driver
unbind.

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

[Marek Behún]

On Thu, 13 Jun 2024 22:32:28 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Thu, Jun 13, 2024 at 6:11 PM Marek Behún <kabel@kernel.org> wrote:
> >
> > In order to create attribute files in /sys/firmware/turris-mox-rwtm,
> > this driver creates it's own kobject type.  
> 
> its
> 
> > 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.  
> 
> ...
> 
> 
> > +static void rwtm_firmware_symlink_drop(void *)  
> 
> Interesting, can we actually name the parameter, like "unused"?
> 
> > +{
> > +       sysfs_remove_link(firmware_kobj, DRIVER_NAME);  
> 
> But why not provide a fimware_kobj pointer as a parameter?
> 
> > +}  
> 
> ...
> 
> > +       /*
> > +        * 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, NULL);  
> 
> This means that it will remove the link in case devm_add_action()
> fails. Is it okay?
> 

Alternatively I could fail .probe() in case devm_add_action() fails. It
is improbable anyway.

[PATCH v2 09/17] 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 d17dc0679439..a536c9c461a7 100644
--- a/drivers/firmware/turris-mox-rwtm.c
+++ b/drivers/firmware/turris-mox-rwtm.c
@@ -90,7 +90,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
@@ -405,39 +404,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
@@ -502,11 +485,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");
 
@@ -529,7 +508,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);
 }
 
-- 
2.44.2

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

[Andy Shevchenko]

On Thu, Jun 13, 2024 at 6:11 PM Marek Behún <kabel@kernel.org> wrote:
>
> Simplify debugfs code: do not check for errors, as debugfs errors should
> be ignored, and use devm action for dropping the debugfs directory.

...

> -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);

This is incorrect. If devm_add_action() fails, the root will be removed...

> +       debugfs_create_file_unsafe("do_sign", 0600, root, rwtm, &do_sign_fops);

...and this most likely will use the dangling pointer.

>  }


-- 
With Best Regards,
Andy Shevchenko

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

[Marek Behún]

On Thu, 13 Jun 2024 22:37:39 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Thu, Jun 13, 2024 at 6:11 PM Marek Behún <kabel@kernel.org> wrote:
> >
> > Simplify debugfs code: do not check for errors, as debugfs errors should
> > be ignored, and use devm action for dropping the debugfs directory.  
> 
> ...
> 
> > -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);  
> 
> This is incorrect. If devm_add_action() fails, the root will be removed...
> 
> > +       debugfs_create_file_unsafe("do_sign", 0600, root, rwtm, &do_sign_fops);  
> 
> ...and this most likely will use the dangling pointer.

OK, I will first creae the file and then add the action.

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

[Marek Behún]

Use devm resource management for driver's 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 | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
index a536c9c461a7..3017a8c19005 100644
--- a/drivers/firmware/turris-mox-rwtm.c
+++ b/drivers/firmware/turris-mox-rwtm.c
@@ -425,6 +425,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 *)
 {
 	sysfs_remove_link(firmware_kobj, DRIVER_NAME);
@@ -462,6 +467,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 < 0)
+		return ret;
+
 	init_completion(&rwtm->cmd_done);
 
 	ret = mox_get_board_info(rwtm);
@@ -472,7 +481,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";
@@ -482,7 +491,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);
@@ -498,17 +507,6 @@ static int turris_mox_rwtm_probe(struct platform_device *pdev)
 		devm_add_action_or_reset(dev, rwtm_firmware_symlink_drop, NULL);
 
 	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[] = {
@@ -521,7 +519,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

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

[Andy Shevchenko]

On Thu, Jun 13, 2024 at 6:11 PM Marek Behún <kabel@kernel.org> wrote:
>
> Use devm resource management for driver's mailbox. This allows us to get
> rid of driver's .remove() method and gotos in .probe().

of the driver's

...

> +       ret = devm_add_action_or_reset(dev, rwtm_devm_mbox_release, rwtm->mbox);
> +       if (ret < 0)

Can it return positive codes? If not, why ' < 0'?

> +               return ret;


-- 
With Best Regards,
Andy Shevchenko

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

[Marek Behún]

On Thu, 13 Jun 2024 22:39:33 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Thu, Jun 13, 2024 at 6:11 PM Marek Behún <kabel@kernel.org> wrote:
> >
> > Use devm resource management for driver's mailbox. This allows us to get
> > rid of driver's .remove() method and gotos in .probe().  
> 
> of the driver's
> 
> ...
> 
> > +       ret = devm_add_action_or_reset(dev, rwtm_devm_mbox_release, rwtm->mbox);
> > +       if (ret < 0)  
> 
> Can it return positive codes? If not, why ' < 0'?
>

Right...

[PATCH v2 11/17] 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 | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
index 3017a8c19005..81a82b1ef515 100644
--- a/drivers/firmware/turris-mox-rwtm.c
+++ b/drivers/firmware/turris-mox-rwtm.c
@@ -461,10 +461,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);
@@ -489,10 +490,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

Re: [PATCH v2 11/17] firmware: turris-mox-rwtm: Use dev_err_probe() where possible

[Andy Shevchenko]

On Thu, Jun 13, 2024 at 6:11 PM Marek Behún <kabel@kernel.org> wrote:
>
> Use dev_err_probe() where possible in the driver's .probe() method.

...

>         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;

What;s the point of this check, please?

> +               return dev_err_probe(dev, ret,
> +                                    "Cannot request mailbox channel!\n");
>         }


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 11/17] firmware: turris-mox-rwtm: Use dev_err_probe() where possible

[Marek Behún]

On Thu, 13 Jun 2024 22:49:18 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Thu, Jun 13, 2024 at 6:11 PM Marek Behún <kabel@kernel.org> wrote:
> >
> > Use dev_err_probe() where possible in the driver's .probe() method.  
> 
> ...
> 
> >         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;  
> 
> What;s the point of this check, please?
> 
> > +               return dev_err_probe(dev, ret,
> > +                                    "Cannot request mailbox channel!\n");
> >         }  
> 
> 

The point is to not print the error message if we just need to wait for
the mailbox driver.

Re: [PATCH v2 11/17] firmware: turris-mox-rwtm: Use dev_err_probe() where possible

[Andy Shevchenko]

On Mon, Jun 17, 2024 at 1:14 PM Marek Behún <kabel@kernel.org> wrote:
> On Thu, 13 Jun 2024 22:49:18 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Thu, Jun 13, 2024 at 6:11 PM Marek Behún <kabel@kernel.org> wrote:

...

> > >         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;
> >
> > What;s the point of this check, please?
> >
> > > +               return dev_err_probe(dev, ret,
> > > +                                    "Cannot request mailbox channel!\n");
> > >         }
>
> The point is to not print the error message if we just need to wait for
> the mailbox driver.

Right, but that was the initial idea behind dev_err_probe(). Have you
checked its implementation (and I hope it's documented as well)?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 11/17] firmware: turris-mox-rwtm: Use dev_err_probe() where possible

[Marek Behún]

On Mon, 17 Jun 2024 14:26:00 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Mon, Jun 17, 2024 at 1:14 PM Marek Behún <kabel@kernel.org> wrote:
> > On Thu, 13 Jun 2024 22:49:18 +0200
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> > > On Thu, Jun 13, 2024 at 6:11 PM Marek Behún <kabel@kernel.org> wrote:  
> 
> ...
> 
> > > >         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;  
> > >
> > > What;s the point of this check, please?
> > >  
> > > > +               return dev_err_probe(dev, ret,
> > > > +                                    "Cannot request mailbox channel!\n");
> > > >         }  
> >
> > The point is to not print the error message if we just need to wait for
> > the mailbox driver.  
> 
> Right, but that was the initial idea behind dev_err_probe(). Have you
> checked its implementation (and I hope it's documented as well)?
> 

OK I am blushing now :-) Thanks, I'll drop the check

[PATCH v2 12/17] firmware: turris-mox-rwtm: Initialize completion before mailbox

[Marek Behún]

Initialize the completion before the mailbox channel is requested.

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 81a82b1ef515..35c2a899caab 100644
--- a/drivers/firmware/turris-mox-rwtm.c
+++ b/drivers/firmware/turris-mox-rwtm.c
@@ -454,6 +454,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;
@@ -472,8 +473,6 @@ 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);
-- 
2.44.2

Re: [PATCH v2 12/17] firmware: turris-mox-rwtm: Initialize completion before mailbox

[Andy Shevchenko]

On Thu, Jun 13, 2024 at 6:11 PM Marek Behún <kabel@kernel.org> wrote:
>
> Initialize the completion before the mailbox channel is requested.

Sounds like a fix, does it need the Fixes tag?


-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 13/17] 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 | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/firmware/turris-mox-rwtm.c b/drivers/firmware/turris-mox-rwtm.c
index 35c2a899caab..e1142083186c 100644
--- a/drivers/firmware/turris-mox-rwtm.c
+++ b/drivers/firmware/turris-mox-rwtm.c
@@ -60,7 +60,6 @@ enum mbox_cmd {
 };
 
 struct mox_rwtm {
-	struct device *dev;
 	struct mbox_client mbox_client;
 	struct mbox_chan *mbox;
 	struct hwrng hwrng;
@@ -95,6 +94,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,	\
@@ -163,6 +167,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;
@@ -177,10 +182,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;
@@ -212,9 +217,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;
@@ -415,7 +420,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);
 }
@@ -445,7 +450,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)
-- 
2.44.2

[PATCH v2 14/17] 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 e1142083186c..a9a3a189ea47 100644
--- a/drivers/firmware/turris-mox-rwtm.c
+++ b/drivers/firmware/turris-mox-rwtm.c
@@ -457,7 +457,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 v2 15/17] 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 a9a3a189ea47..4c445f6b8140 100644
--- a/drivers/firmware/turris-mox-rwtm.c
+++ b/drivers/firmware/turris-mox-rwtm.c
@@ -258,7 +258,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;
 
@@ -493,7 +493,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 v2 16/17] 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 4c445f6b8140..2ea378fec414 100644
--- a/drivers/firmware/turris-mox-rwtm.c
+++ b/drivers/firmware/turris-mox-rwtm.c
@@ -136,7 +136,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
@@ -184,7 +184,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) {
@@ -218,7 +218,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) {
-- 
2.44.2

[PATCH v2 17/17] 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 2ea378fec414..f4b56269f507 100644
--- a/drivers/firmware/turris-mox-rwtm.c
+++ b/drivers/firmware/turris-mox-rwtm.c
@@ -155,6 +155,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;
@@ -168,19 +196,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");
@@ -207,15 +226,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) {
@@ -238,38 +249,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 > RWTM_DMA_BUFFER_SIZE)
 		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;
@@ -277,15 +274,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;
 
@@ -333,7 +322,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;
@@ -366,23 +354,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