| Message ID | c797dc5e9248498918916a6eeedaa25de2196e8c.1669154149.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: amlogic - Save a few bytes of memory | expand |
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.
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
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(-)