crypto: amlogic - Save a few bytes of memory

  There is no real point in allocating dedicated memory for the irqs array.
MAXFLOW is only 2, so it is easier to allocated the needed space
directly within the 'meson_dev' structure.

This saves some memory allocation and avoids an indirection when using the
irqs array.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/crypto/amlogic/amlogic-gxl-core.c | 1 -
 drivers/crypto/amlogic/amlogic-gxl.h      | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)
  

On Tue, Nov 22, 2022 at 10:57 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> There is no real point in allocating dedicated memory for the irqs array.
> MAXFLOW is only 2, so it is easier to allocated the needed space
> directly within the 'meson_dev' structure.
>
> This saves some memory allocation and avoids an indirection when using the
> irqs array.
..and it even fixes a missing devm_kcalloc error check

Personally I prefer this approach over a patch that was sent earlier today: [0]
Corentin, Christophe, what do you think?


Best regards,
Martin


[0] https://lore.kernel.org/linux-crypto/0df30bbf-3b7e-ed20-e316-41192bf3cc2b@linaro.org/T/#m6a45b44206c282f106d379b01d19027823c5d79b
  

Le 22/11/2022 à 23:02, Martin Blumenstingl a écrit :
> On Tue, Nov 22, 2022 at 10:57 PM Christophe JAILLET
> <christophe.jaillet@wanadoo.fr> wrote:
>>
>> There is no real point in allocating dedicated memory for the irqs array.
>> MAXFLOW is only 2, so it is easier to allocated the needed space
>> directly within the 'meson_dev' structure.
>>
>> This saves some memory allocation and avoids an indirection when using the
>> irqs array.
> ..and it even fixes a missing devm_kcalloc error check
> 
> Personally I prefer this approach over a patch that was sent earlier today: [0]

Funny.
A file untouched for about 18 months and 2 patches around the same line, 
... the same day!

> Corentin, Christophe, what do you think?

Obviously, mine is better :)

More seriously, I think it is mostly a mater of taste and that both are 
fine. Neither one will make a real difference IRL.

I guess that memory allocation failure in probe are unlikely and saving 
64 bytes (40 for devm_ + 2 x 4 = 48, rounded to 64 bytes) won't make any 
real difference.

Up to you.

CJ
> 
> 
> Best regards,
> Martin
> 
> 
> [0] https://lore.kernel.org/linux-crypto/0df30bbf-3b7e-ed20-e316-41192bf3cc2b@linaro.org/T/#m6a45b44206c282f106d379b01d19027823c5d79b
>
  

Le 22/11/2022 à 23:02, Martin Blumenstingl a écrit :
> On Tue, Nov 22, 2022 at 10:57 PM Christophe JAILLET
> <christophe.jaillet@wanadoo.fr> wrote:
>>
>> There is no real point in allocating dedicated memory for the irqs array.
>> MAXFLOW is only 2, so it is easier to allocated the needed space
>> directly within the 'meson_dev' structure.
>>
>> This saves some memory allocation and avoids an indirection when using the
>> irqs array.
> ..and it even fixes a missing devm_kcalloc error check
> 
> Personally I prefer this approach over a patch that was sent earlier today: [0]
> Corentin, Christophe, what do you think?
> 
> 
> Best regards,
> Martin
> 
> 
> [0] https://lore.kernel.org/linux-crypto/0df30bbf-3b7e-ed20-e316-41192bf3cc2b@linaro.org/T/#m6a45b44206c282f106d379b01d19027823c5d79b
> 

Unrelated, but I think that meson_irq_handler() needs a small ajustement 
to avoid printing a spurious message if readl() returns 0.

Maybe something like that?:


@@ -33,9 +33,10 @@ static irqreturn_t meson_irq_handler(int irq, void *data)
  				writel_relaxed(0xF, mc->base + ((0x4 + flow) << 2));
  				mc->chanlist[flow].status = 1;
  				complete(&mc->chanlist[flow].complete);
-				return IRQ_HANDLED;
+			} else {
+				dev_err(mc->dev, "%s %d Got irq for flow %d but ctrl is empty\n", 
__func__, irq, flow);
  			}
-			dev_err(mc->dev, "%s %d Got irq for flow %d but ctrl is empty\n", 
__func__, irq, flow);
+			return IRQ_HANDLED;
  		}
  	}


CJ
  

> -----Original Message-----
> From: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> Sent: Tuesday, November 22, 2022 4:37 PM
> Subject: Re: [PATCH] crypto: amlogic - Save a few bytes of memory


...
> Unrelated, but I think that meson_irq_handler() needs a small ajustement
> to avoid printing a spurious message if readl() returns 0.
> 
> Maybe something like that?:
> 
> @@ -33,9 +33,10 @@ static irqreturn_t meson_irq_handler(int irq, void
> *data)
>   				writel_relaxed(0xF, mc->base + ((0x4 + flow) << 2));
>   				mc->chanlist[flow].status = 1;
>   				complete(&mc->chanlist[flow].complete);
> -				return IRQ_HANDLED;
> +			} else {
> +				dev_err(mc->dev, "%s %d Got irq for flow %d but ctrl is empty\n", __func__, irq, flow);
>   			}
> -			dev_err(mc->dev, "%s %d Got irq for flow %d but ctrl is empty\n", __func__, irq, flow);
> +			return IRQ_HANDLED;
>   		}
>   	}

You might want to reconsider allowing this irq handler to do any prints.
80 characters take 5 ms to print on a 115 kbps serial port, which can
lead to RCU stalls, soft lockups, etc.

If kept, dev_err_ratelimited would be a little safer.

In some systems "spurious interrupts" are routine and are not really a
problem unless you overreact to them.
  

On Tue, Nov 22, 2022 at 10:56:19PM +0100, Christophe JAILLET wrote:
> There is no real point in allocating dedicated memory for the irqs array.
> MAXFLOW is only 2, so it is easier to allocated the needed space
> directly within the 'meson_dev' structure.
> 
> This saves some memory allocation and avoids an indirection when using the
> irqs array.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  drivers/crypto/amlogic/amlogic-gxl-core.c | 1 -
>  drivers/crypto/amlogic/amlogic-gxl.h      | 2 +-
>  2 files changed, 1 insertion(+), 2 deletions(-)

Patch applied.  Thanks.