diff mbox series

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

Message ID 20240613161045.29606-4-kabel@kernel.org (mailing list archive)
State Superseded
Headers show
Series Updates for turris-mox-rwtm driver | expand

Commit Message

Marek Behún June 13, 2024, 4:10 p.m. UTC 
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(-)

Comments

Ilpo Järvinen June 13, 2024, 4:15 p.m. UTC | #1 
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>
Andy Shevchenko June 13, 2024, 5:51 p.m. UTC | #2 
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.
Arnd Bergmann June 14, 2024, 5:44 a.m. UTC | #3 
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
Marek Behún June 17, 2024, 10:57 a.m. UTC | #4 
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() :)
Andy Shevchenko June 17, 2024, 11:01 a.m. UTC | #5 
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().
diff mbox series

Patch

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;