Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 1 Sep 2023 08:48:43 +0000
From:   Charles Keepax <ckeepax@opensource.cirrus.com>
To:     Vlad Karpovich <vkarpovi@opensource.cirrus.com>
CC:     James Schulman <james.schulman@cirrus.com>,
        David Rhodes <david.rhodes@cirrus.com>,
        Richard Fitzgerald <rf@opensource.cirrus.com>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>,
        Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>,
        Rob Herring <robh+dt@kernel.org>,
        <alsa-devel@alsa-project.org>, <patches@opensource.cirrus.com>,
        <linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>,
        Ricardo Rivera-Matos <rriveram@opensource.cirrus.com>
Subject: Re: [PATCH v3 1/4] ASoC: cs35l45: Checks index of cs35l45_irqs[]
Message-ID: <20230901084843.GZ103419@ediswmail.ad.cirrus.com>
References: <20230831162042.471801-1-vkarpovi@opensource.cirrus.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230831162042.471801-1-vkarpovi@opensource.cirrus.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

On Thu, Aug 31, 2023 at 11:20:39AM -0500, Vlad Karpovich wrote:
> From: Ricardo Rivera-Matos <rriveram@opensource.cirrus.com>
> 
> Checks the index computed by the virq offset before printing the
> error condition in cs35l45_spk_safe_err() handler.
> 
> Signed-off-by: Ricardo Rivera-Matos <rriveram@opensource.cirrus.com>
> Signed-off-by: Vlad Karpovich <vkarpovi@opensource.cirrus.com>
> ---
>  sound/soc/codecs/cs35l45.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/codecs/cs35l45.c b/sound/soc/codecs/cs35l45.c
> index 2ac4612402eb..02b1172d2647 100644
> --- a/sound/soc/codecs/cs35l45.c
> +++ b/sound/soc/codecs/cs35l45.c
> @@ -1023,7 +1023,10 @@ static irqreturn_t cs35l45_spk_safe_err(int irq, void *data)
>  
>  	i = irq - regmap_irq_get_virq(cs35l45->irq_data, 0);
>  
> -	dev_err(cs35l45->dev, "%s condition detected!\n", cs35l45_irqs[i].name);
> +	if (i < 0 || i >= ARRAY_SIZE(cs35l45_irqs))

I am happy enough for this to be merged, since it clearly does
no harm. So:

Acked-by: Charles Keepax <ckeepax@opensource.cirrus.com>

But I do still have a slight reservation of what is the point
of this error check?  This handler is static and can only be
called from within cs35l45.c and the only code that registers
IRQs goes through the cs35l45_irqs array and registers IRQs
from there, so how does this ever end up with i being out of
bounds?

And whilst I would not add this to this patch. I do also think
perhaps Ricardo had a point in his email, the IRQ handler
should probably be renamed, since it handles more than just
the spk_safe_err's, perhaps something like cs35l45_report_err.
On why the watchdog and global error call this as well, that
was a review comment on an earlier patch since the handlers for
those errors only printed a message, they might as well be
combined with the spk_safe error that also only printed a
message. If at some point separate handling is added for them
they can be split out.

Thanks,
Charles