| Message ID | 20171216011230.107527-1-briannorris@chromium.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 509bf3a7d43ab173abc354df2a859229ede043c0 |
| Headers | show |
+ 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 >
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 > > >
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
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
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.
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
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);
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 sound/soc/codecs/rt5514-spi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)