lkml.org 
[lkml]   [2019]   [Apr]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] ASoC: Intel: avoid Oops if DMA setup fails
    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.

    \
     
     \ /
      Last update: 2019-04-29 19:03    [W:2.808 / U:0.700 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site