On Fri, Apr 26, 2019 at 04:03:47PM -0500, Pierre-Louis Bossart wrote:

On 4/26/19 11:47 AM, Ross Zwisler wrote:

...

Currently in sst_dsp_new() if we get an error return from sst_dma_new() we just print an error message and then still complete the function successfully. This means that we are trying to run without sst->dma properly set up, which will result in NULL pointer dereference when sst->dma is later used. This was happening for me in sst_dsp_dma_get_channel():

     struct sst_dma *dma = dsp->dma;

... dma->ch = dma_request_channel(mask, dma_chan_filter, dsp);

This resulted in:

BUG: unable to handle kernel NULL pointer dereference at 0000000000000018
IP: sst_dsp_dma_get_channel+0x4f/0x125 [snd_soc_sst_firmware]

Fix this by adding proper error handling for the case where we fail to set up DMA.

Signed-off-by: Ross Zwisler zwisler@google.com Cc: stable@vger.kernel.org

---

sound/soc/intel/common/sst-firmware.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/sound/soc/intel/common/sst-firmware.c b/sound/soc/intel/common/sst-firmware.c index 1e067504b6043..9be3a793a55e3 100644 --- a/sound/soc/intel/common/sst-firmware.c +++ b/sound/soc/intel/common/sst-firmware.c @@ -1251,11 +1251,15 @@ struct sst_dsp *sst_dsp_new(struct device *dev, goto irq_err; err = sst_dma_new(sst);

• if (err)

• if (err) { dev_warn(dev, "sst_dma_new failed %d\n", err);

• goto dma_err;
  

• }

Thanks for the patch. The fix looks correct, but does it make sense to keep a dev_warn() here? Should it be changed to dev_err() instead since as you mentioned it's fatal to keep going. Also you may want to mention in the commit message that this should only impact Broadwell and maybe the legacy Baytrail driver. IIRC we don't use the DMAs in other cases.

Sure, I'll address both of these in a v2. Thank you for the quick review.

On 4/29/19 1:25 PM, Ross Zwisler wrote:

Currently in sst_dsp_new() if we get an error return from sst_dma_new() we just print an error message and then still complete the function successfully. This means that we are trying to run without sst->dma properly set up, which will result in NULL pointer dereference when sst->dma is later used. This was happening for me in sst_dsp_dma_get_channel():

     struct sst_dma *dma = dsp->dma;

... dma->ch = dma_request_channel(mask, dma_chan_filter, dsp);

This resulted in:

BUG: unable to handle kernel NULL pointer dereference at 0000000000000018
IP: sst_dsp_dma_get_channel+0x4f/0x125 [snd_soc_sst_firmware]

Fix this by adding proper error handling for the case where we fail to set up DMA.

This change only affects Haswell and Broadwell systems. Baytrail systems explicilty opt-out of DMA via sst->pdata->resindex_dma_base being set to -1.

Signed-off-by: Ross Zwisler zwisler@google.com Cc: stable@vger.kernel.org

Acked-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com

Thanks Ross!

FWIW we should start deprecating this driver now and transition to SOF. I'll double-check how the upcoming 1.3 release works on my Pixel 2015/Samus device later this week.

---

Changes in v2:

• Upgraded the sst_dma_new() failure message from dev_warn() to dev_err() (Pierre-Louis).
• Noted in the changelog that this change only affects Haswell and Broadwell (Pierre-Louis).

---

sound/soc/intel/common/sst-firmware.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/sound/soc/intel/common/sst-firmware.c b/sound/soc/intel/common/sst-firmware.c index 1e067504b6043..f830e59f93eaa 100644 --- a/sound/soc/intel/common/sst-firmware.c +++ b/sound/soc/intel/common/sst-firmware.c @@ -1251,11 +1251,15 @@ struct sst_dsp *sst_dsp_new(struct device *dev, goto irq_err;

err = sst_dma_new(sst);

• if (err)

• dev_warn(dev, "sst_dma_new failed %d\n", err);
  

• if (err) {

• dev_err(dev, "sst_dma_new failed %d\n", err);
  

• goto dma_err;
  

• }

  return sst;

+dma_err:

• free_irq(sst->irq, sst); irq_err: if (sst->ops->free) sst->ops->free(sst);