Re: [PATCH for-4.15] ASoC: rt5514: don't assume rt5514 component was "attached"

Re: [PATCH for-4.15] ASoC: rt5514: don't assume rt5514 component was "attached"

[Brian Norris]

+ others

On Fri, Dec 15, 2017 at 05:12:30PM -0800, Brian Norris wrote:
> I've found that on Google's "Kevin" Chromebook, the rt5514 codec might
> not be set up completely, yet its device is still present, and therefore
> its PM suspend/resume is called. This hits a NULL pointer exception,
> since we never had the chance to set our drvdata pointer.
> 
> This resolves crashes seen when trying to resume my system.
> 
> Fixes: e9c50aa6bd39 ("ASoC: rt5514-spi: check irq status to schedule data copy in resume function")
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
> This is a v4.15-rc1 regression

I see that this is probably a bug on its own, which should be fixed on
its own (e.g., with the $subject patch), but FYI I was also pointed at
this patch, which also "resolves" this problem by fixing the rt5514 DAI
setup:

https://patchwork.kernel.org/patch/10067725/
[PATCH] ASoC: rockchip: Use dummy_dai for rt5514 dsp dailink

Would be nice to see it get handled too, possibly in 4.15 as well (I
think that's a regression too?).

Brian

> 
>  sound/soc/codecs/rt5514-spi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/soc/codecs/rt5514-spi.c b/sound/soc/codecs/rt5514-spi.c
> index 2df91db765ac..9255afcf2c3a 100644
> --- a/sound/soc/codecs/rt5514-spi.c
> +++ b/sound/soc/codecs/rt5514-spi.c
> @@ -482,7 +482,7 @@ static int __maybe_unused rt5514_resume(struct device *dev)
>  	if (device_may_wakeup(dev))
>  		disable_irq_wake(irq);
>  
> -	if (rt5514_dsp->substream) {
> +	if (rt5514_dsp && rt5514_dsp->substream) {
>  		rt5514_spi_burst_read(RT5514_IRQ_CTRL, (u8 *)&buf, sizeof(buf));
>  		if (buf[0] & RT5514_IRQ_STATUS_BIT)
>  			rt5514_schedule_copy(rt5514_dsp);
> -- 
> 2.15.1.504.g5279b80103-goog
> 

Re: [PATCH for-4.15] ASoC: rt5514: don't assume rt5514 component was "attached"

[Cheng-yi Chiang]

Hi Brian,
  Oder has posted the same fix : https://patchwork.kernel.org/
patch/10066257/ and it has been applied.
Perhaps you can cherry-pick it to v4.15 ?

Thanks!

On Sat, Dec 16, 2017 at 11:17 AM, Brian Norris <briannorris@chromium.org>
wrote:

> + others
>
> On Fri, Dec 15, 2017 at 05:12:30PM -0800, Brian Norris wrote:
> > I've found that on Google's "Kevin" Chromebook, the rt5514 codec might
> > not be set up completely, yet its device is still present, and therefore
> > its PM suspend/resume is called. This hits a NULL pointer exception,
> > since we never had the chance to set our drvdata pointer.
> >
> > This resolves crashes seen when trying to resume my system.
> >
> > Fixes: e9c50aa6bd39 ("ASoC: rt5514-spi: check irq status to schedule
> data copy in resume function")
> > Signed-off-by: Brian Norris <briannorris@chromium.org>
> > ---
> > This is a v4.15-rc1 regression
>
> I see that this is probably a bug on its own, which should be fixed on
> its own (e.g., with the $subject patch), but FYI I was also pointed at
> this patch, which also "resolves" this problem by fixing the rt5514 DAI
> setup:
>
> https://patchwork.kernel.org/patch/10067725/
> [PATCH] ASoC: rockchip: Use dummy_dai for rt5514 dsp dailink
>
> Would be nice to see it get handled too, possibly in 4.15 as well (I
> think that's a regression too?).
>
> Brian
>
> >
> >  sound/soc/codecs/rt5514-spi.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/sound/soc/codecs/rt5514-spi.c
> b/sound/soc/codecs/rt5514-spi.c
> > index 2df91db765ac..9255afcf2c3a 100644
> > --- a/sound/soc/codecs/rt5514-spi.c
> > +++ b/sound/soc/codecs/rt5514-spi.c
> > @@ -482,7 +482,7 @@ static int __maybe_unused rt5514_resume(struct
> device *dev)
> >       if (device_may_wakeup(dev))
> >               disable_irq_wake(irq);
> >
> > -     if (rt5514_dsp->substream) {
> > +     if (rt5514_dsp && rt5514_dsp->substream) {
> >               rt5514_spi_burst_read(RT5514_IRQ_CTRL, (u8 *)&buf,
> sizeof(buf));
> >               if (buf[0] & RT5514_IRQ_STATUS_BIT)
> >                       rt5514_schedule_copy(rt5514_dsp);
> > --
> > 2.15.1.504.g5279b80103-goog
> >
>

Re: [PATCH for-4.15] ASoC: rt5514: don't assume rt5514 component was "attached"

[Brian Norris]

Hi!

(By the way, your mail is HTML and likely will get rejected by many
mailing lists and/or people reading these mailing lists.)

On Mon, Dec 18, 2017 at 12:23:18PM +0800, Cheng-yi Chiang wrote:
>    Hi Brian,
>      Oder has posted the same fix : [1]https://patchwork.kernel.org/
>    patch/10066257/ and it has been applied.

OK cool. Obviously I'm biased, but I prefer mine, as it has less
needless whitespace, and is appropriately documented ;) But it should be
a fine replacement.

>    Perhaps you can cherry-pick it to v4.15 ?

I have no say in that; that would be up to Mark, I think. It's most
certainly a regression in 4.15-rc1, so IMO it should be given a proper
'Fixes: e9c50aa6bd39 ("ASoC: rt5514-spi: check irq status to schedule
data copy in resume function")' tag and sent for 4.15, not 4.16.

Brian

Re: [PATCH for-4.15] ASoC: rt5514: don't assume rt5514 component was "attached"

[Cheng-yi Chiang]

Hi Brian, I am sorry for not using the plain text mode in the previous mail.
I agree with you on other points.

On Tue, Dec 19, 2017 at 1:42 AM, Brian Norris <briannorris@chromium.org> wrote:
> Hi!
>
> (By the way, your mail is HTML and likely will get rejected by many
> mailing lists and/or people reading these mailing lists.)
>
> On Mon, Dec 18, 2017 at 12:23:18PM +0800, Cheng-yi Chiang wrote:
>>    Hi Brian,
>>      Oder has posted the same fix : [1]https://patchwork.kernel.org/
>>    patch/10066257/ and it has been applied.
>
> OK cool. Obviously I'm biased, but I prefer mine, as it has less
> needless whitespace, and is appropriately documented ;) But it should be
> a fine replacement.
>
>>    Perhaps you can cherry-pick it to v4.15 ?
>
> I have no say in that; that would be up to Mark, I think. It's most
> certainly a regression in 4.15-rc1, so IMO it should be given a proper
> 'Fixes: e9c50aa6bd39 ("ASoC: rt5514-spi: check irq status to schedule
> data copy in resume function")' tag and sent for 4.15, not 4.16.
>
> Brian

Re: [PATCH for-4.15] ASoC: rt5514: don't assume rt5514 component was "attached"

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 644 bytes --]

On Mon, Dec 18, 2017 at 09:42:36AM -0800, Brian Norris wrote:
> On Mon, Dec 18, 2017 at 12:23:18PM +0800, Cheng-yi Chiang wrote:

> >    Hi Brian,
> >      Oder has posted the same fix : [1]https://patchwork.kernel.org/
> >    patch/10066257/ and it has been applied.

> >    Perhaps you can cherry-pick it to v4.15 ?

> I have no say in that; that would be up to Mark, I think. It's most
> certainly a regression in 4.15-rc1, so IMO it should be given a proper
> 'Fixes: e9c50aa6bd39 ("ASoC: rt5514-spi: check irq status to schedule
> data copy in resume function")' tag and sent for 4.15, not 4.16.

It's been applied as a fix for some time.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH for-4.15] ASoC: rt5514: don't assume rt5514 component was "attached"

[Brian Norris]

On Tue, Dec 19, 2017 at 10:58:18AM +0000, Mark Brown wrote:
> It's been applied as a fix for some time.

Indeed it has. Sorry for missing that. I look forward to seeing it in a
release candidate, so my system will again work on mainline :)

Brian