[v2,1/4] ASoC: cs35l45: Checks index of cs35l45_irqs[]

Commit Message

Vlad Karpovich Aug. 30, 2023, 7:55 p.m. UTC

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 <[e-mail]>
Signed-off-by: Vlad Karpovich <[e-mail]>
---
 sound/soc/codecs/cs35l45.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Ricardo Rivera-Matos Aug. 31, 2023, 2:56 p.m. UTC | #1

Hello Mark,

> On Aug 30, 2023, at 3:59 PM, Mark Brown <[e-mail]> wrote:
> 
> On Wed, Aug 30, 2023 at 02:55:33PM -0500, Vlad Karpovich wrote:
>> 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 <[e-mail]>
>> Signed-off-by: Vlad Karpovich <[e-mail]>
> 
> Who actually wrote this patch?

I am the original author, allow me to clarify how and why this is supposed to work.

How:

static const struct cs35l45_irq cs35l45_irqs[] = {
	CS35L45_IRQ(AMP_SHORT_ERR, "Amplifier short error", cs35l45_spk_safe_err),
	CS35L45_IRQ(UVLO_VDDBATT_ERR, "VDDBATT undervoltage error", cs35l45_spk_safe_err),
	CS35L45_IRQ(BST_SHORT_ERR, "Boost inductor error", cs35l45_spk_safe_err),
	CS35L45_IRQ(BST_UVP_ERR, "Boost undervoltage error", cs35l45_spk_safe_err),
	CS35L45_IRQ(TEMP_ERR, "Overtemperature error", cs35l45_spk_safe_err),
	CS35L45_IRQ(AMP_CAL_ERR, "Amplifier calibration error", cs35l45_spk_safe_err),
	CS35L45_IRQ(UVLO_VDDLV_ERR, "LV threshold detector error", cs35l45_spk_safe_err),
	CS35L45_IRQ(GLOBAL_ERROR, "Global error", cs35l45_global_err),
	CS35L45_IRQ(DSP_WDT_EXPIRE, "DSP Watchdog Timer", cs35l45_dsp_wdt_expire),
	CS35L45_IRQ(PLL_UNLOCK_FLAG_RISE, "PLL unlock flag rise", cs35l45_pll_unlock),
	CS35L45_IRQ(PLL_LOCK_FLAG, "PLL lock", cs35l45_pll_lock),
	CS35L45_IRQ(DSP_VIRT2_MBOX, "DSP virtual MBOX 2 write flag", cs35l45_dsp_virt2_mbox_cb),
};

static irqreturn_t cs35l45_spk_safe_err(int irq, void *data)
{
	struct cs35l45_private *cs35l45 = data;
	int i;

	i = irq - regmap_irq_get_virq(cs35l45->irq_data, 0);

	if (i < 0 || i > 6)
		dev_err(cs35l45->dev, "Unspecified global error condition (%d) detected!\n", irq);
	else
		dev_err(cs35l45->dev, "%s condition detected!\n", cs35l45_irqs[i].name);

	return IRQ_HANDLED;
}

This snippet here is from the OoT CS35L45 driver. There are only seven root causes for a speaker safe error and when one of those root cause bits are set, we enter the common handler to print the root cause. Using the IRQ and the VIRQ number we do some math and print the name of the error as a dev_err.

Why:

Originally these root cause bits were treated as general bits and simply checked in the cs35l45_global_err() handler. A problem arose when the CS35L45 would come out of hibernation and the root cause bits would be masked by default. Treating the root cause bits as IRQs ensured that, like the other IRQ bits, the root cause bits would be unmasked before an IRQ could be serviced.

Further notes:

static const struct cs35l45_irq cs35l45_irqs[] = {
<snip>
	CS35L45_IRQ(GLOBAL_ERROR, "Global error", cs35l45_spk_safe_err),
	CS35L45_IRQ(DSP_WDT_EXPIRE, "DSP Watchdog Timer", cs35l45_spk_safe_err),
<snip>
};

This is from next-20230831. I am not sure how this happened, but these IRQs are not pointing to cs35l45_global_err and cs35l45_dsp_wdt_expire respectively. Maybe a bad cherry-pick. We will address this shortly.

I hope this addresses any confusion, please let me know if I can offer any further details.

Thanks,
Ricardo

Richard Fitzgerald Aug. 31, 2023, 3:22 p.m. UTC | #2

On 31/8/23 15:56, Rivera-Matos, Ricardo wrote:
> Hello Mark,
> 
>> On Aug 30, 2023, at 3:59 PM, Mark Brown <[e-mail]> wrote:
>>
>> On Wed, Aug 30, 2023 at 02:55:33PM -0500, Vlad Karpovich wrote:
>>> 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 <[e-mail]>
>>> Signed-off-by: Vlad Karpovich <[e-mail]>
>>
>> Who actually wrote this patch?
> 
> I am the original author, allow me to clarify how and why this is supposed to work.
> 

I think Mark's comment is that the Signed-off-by chain doesn't match
the patch author. The s-o-b order implies that Ricardo wrote it and
Vlad is upstreaming it - but in that case this email should start with
a From: line to say that the author is Ricardo.

Mark Brown Aug. 31, 2023, 4:05 p.m. UTC | #3

On Thu, Aug 31, 2023 at 04:22:52PM +0100, Richard Fitzgerald wrote:

> I think Mark's comment is that the Signed-off-by chain doesn't match
> the patch author. The s-o-b order implies that Ricardo wrote it and
> Vlad is upstreaming it - but in that case this email should start with
> a From: line to say that the author is Ricardo.

Yes, that's what I'm querying.

Vlad Karpovich Aug. 31, 2023, 4:08 p.m. UTC | #4

On 8/31/23 11:05, Mark Brown wrote:
> On Thu, Aug 31, 2023 at 04:22:52PM +0100, Richard Fitzgerald wrote:
>
>> I think Mark's comment is that the Signed-off-by chain doesn't match
>> the patch author. The s-o-b order implies that Ricardo wrote it and
>> Vlad is upstreaming it - but in that case this email should start with
>> a From: line to say that the author is Ricardo.
> Yes, that's what I'm querying.
Ok. I will update the author

Patch

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))
+		dev_err(cs35l45->dev, "Unspecified global error condition (%d) detected!\n", irq);
+	else
+		dev_err(cs35l45->dev, "%s condition detected!\n", cs35l45_irqs[i].name);
 
 	return IRQ_HANDLED;
 }