crypto: amlogic - Save a few bytes of memory

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

Posted by Christophe JAILLET 3 years, 4 months ago

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

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

Posted by Herbert Xu 3 years, 4 months ago

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

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

Posted by Martin Blumenstingl 3 years, 4 months ago

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

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

Posted by Christophe JAILLET 3 years, 4 months ago

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

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

Posted by Elliott, Robert (Servers) 3 years, 4 months ago


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

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

Posted by Christophe JAILLET 3 years, 4 months ago

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
>