[PATCH] crypto: amlogic - Save a few bytes of memory

[PATCH] crypto: amlogic - Save a few bytes of memory

[Christophe JAILLET]

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

diff --git a/drivers/crypto/amlogic/amlogic-gxl-core.c b/drivers/crypto/amlogic/amlogic-gxl-core.c
index 6e7ae896717c..937187027ad5 100644
--- a/drivers/crypto/amlogic/amlogic-gxl-core.c
+++ b/drivers/crypto/amlogic/amlogic-gxl-core.c
@@ -237,7 +237,6 @@ static int meson_crypto_probe(struct platform_device *pdev)
 		return err;
 	}
 
-	mc->irqs = devm_kcalloc(mc->dev, MAXFLOW, sizeof(int), GFP_KERNEL);
 	for (i = 0; i < MAXFLOW; i++) {
 		mc->irqs[i] = platform_get_irq(pdev, i);
 		if (mc->irqs[i] < 0)
diff --git a/drivers/crypto/amlogic/amlogic-gxl.h b/drivers/crypto/amlogic/amlogic-gxl.h
index dc0f142324a3..8c0746a1d6d4 100644
--- a/drivers/crypto/amlogic/amlogic-gxl.h
+++ b/drivers/crypto/amlogic/amlogic-gxl.h
@@ -95,7 +95,7 @@ struct meson_dev {
 	struct device *dev;
 	struct meson_flow *chanlist;
 	atomic_t flow;
-	int *irqs;
+	int irqs[MAXFLOW];
 #ifdef CONFIG_CRYPTO_DEV_AMLOGIC_GXL_DEBUG
 	struct dentry *dbgfs_dir;
 #endif
-- 
2.34.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH] crypto: amlogic - Save a few bytes of memory

[Martin Blumenstingl]

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

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH] crypto: amlogic - Save a few bytes of memory

[Christophe JAILLET]

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
> 


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH] crypto: amlogic - Save a few bytes of memory

[Christophe JAILLET]

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

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

RE: [PATCH] crypto: amlogic - Save a few bytes of memory

[Elliott, Robert (Servers)]

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

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH] crypto: amlogic - Save a few bytes of memory

[Herbert Xu]

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.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic