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 +, 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

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 +, 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

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

[Mark Brown]

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.


signature.asc
Description: PGP signature

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

[Mark Brown]

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.


signature.asc
Description: PGP signature

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  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"

[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  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"

[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"

[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"

[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 
> ---
> 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 *), 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]

+ 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 
> ---
> 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 *), sizeof(buf));
>   if (buf[0] & RT5514_IRQ_STATUS_BIT)
>   rt5514_schedule_copy(rt5514_dsp);
> -- 
> 2.15.1.504.g5279b80103-goog
> 

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

[Brian Norris]

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 
---
This is a v4.15-rc1 regression

 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 *), sizeof(buf));
if (buf[0] & RT5514_IRQ_STATUS_BIT)
rt5514_schedule_copy(rt5514_dsp);
-- 
2.15.1.504.g5279b80103-goog

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

[Brian Norris]

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 
---
This is a v4.15-rc1 regression

 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 *), sizeof(buf));
if (buf[0] & RT5514_IRQ_STATUS_BIT)
rt5514_schedule_copy(rt5514_dsp);
-- 
2.15.1.504.g5279b80103-goog