linux-kernel - RE: [PATCH] ASoC: Intel: sst: ipc command timeout

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <BN6PR1101MB2132D23B042284DDA667642A97AC0@BN6PR1101MB2132.namprd11.prod.outlook.com>
Date:   Tue, 28 Apr 2020 17:29:08 +0000
From:   "Lu, Brent" <brent.lu@...el.com>
To:     Amadeusz Sławiński 
        <amadeuszx.slawinski@...ux.intel.com>,
        "Rojewski, Cezary" <cezary.rojewski@...el.com>,
        Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>
CC:     Kate Stewart <kstewart@...uxfoundation.org>,
        Richard Fontana <rfontana@...hat.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Jie Yang" <yang.jie@...ux.intel.com>,
        Takashi Iwai <tiwai@...e.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Liam Girdwood <liam.r.girdwood@...ux.intel.com>,
        "clang-built-linux@...glegroups.com" 
        <clang-built-linux@...glegroups.com>,
        Mark Brown <broonie@...nel.org>,
        "Thomas Gleixner" <tglx@...utronix.de>,
        Allison Randal <allison@...utok.net>
Subject: RE: [PATCH] ASoC: Intel: sst: ipc command timeout

> 
> I've looked at the code and byt_is_dsp_busy seems suspicious to me.
> Can you check if following change fixes problem for you:
> 
> diff --git a/sound/soc/intel/baytrail/sst-baytrail-ipc.c
> b/sound/soc/intel/baytrail/sst-baytrail-ipc.c
> index 74274bd38f7a..34746fd871b0 100644
> --- a/sound/soc/intel/baytrail/sst-baytrail-ipc.c
> +++ b/sound/soc/intel/baytrail/sst-baytrail-ipc.c
> @@ -666,8 +666,8 @@ static bool byt_is_dsp_busy(struct sst_dsp *dsp)
>   {
>          u64 ipcx;
> 
> -       ipcx = sst_dsp_shim_read_unlocked(dsp, SST_IPCX);
> -       return (ipcx & (SST_IPCX_BUSY | SST_IPCX_DONE));
> +       ipcx = sst_dsp_shim_read64_unlocked(dsp, SST_IPCX);
> +       return (ipcx & (SST_BYT_IPCX_BUSY | SST_BYT_IPCX_DONE));
>   }
> 
>   int sst_byt_dsp_init(struct device *dev, struct sst_pdata *pdata)
> 
> We seem to treat SST_IPCX as 32 bit register instead of 64 one, which may
> explain wrong behaviour. (Specification says it is 64 bit register).
> 
> Thanks,
> Amadeusz

Hi Amadeusz,

The patch does not work but I managed to create a workaround just for
reference. Still don't know why first read in sst_byt_irq_thread returns
incorrect value.

diff --git a/sound/soc/intel/baytrail/sst-baytrail-ipc.c b/sound/soc/intel/baytrail/sst-baytrail-ipc.c
index 5bbaa667bec1..56c557cb722d 100644
--- a/sound/soc/intel/baytrail/sst-baytrail-ipc.c
+++ b/sound/soc/intel/baytrail/sst-baytrail-ipc.c
@@ -12,6 +12,7 @@
  * more details.
  */

+#define DEBUG
 #include <linux/types.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
@@ -313,7 +314,7 @@ static irqreturn_t sst_byt_irq_thread(int irq, void *context)
        struct sst_dsp *sst = (struct sst_dsp *) context;
        struct sst_byt *byt = sst_dsp_get_thread_context(sst);
        struct sst_generic_ipc *ipc = &byt->ipc;
-       u64 header;
+       u64 header, mask, old, new;
        unsigned long flags;

        spin_lock_irqsave(&sst->spinlock, flags);
@@ -332,10 +333,32 @@ static irqreturn_t sst_byt_irq_thread(int irq, void *context)
                 * processed the message and can accept new. Clear data part
                 * of the header
                 */
-               sst_dsp_shim_update_bits64_unlocked(sst, SST_IPCD,
-                       SST_BYT_IPCD_DONE | SST_BYT_IPCD_BUSY |
-                       IPC_HEADER_DATA(IPC_HEADER_DATA_MASK),
-                       SST_BYT_IPCD_DONE);
+               /* inline the sst_dsp_shim_update_bits64_unlocked function */
+               mask = SST_BYT_IPCD_DONE | SST_BYT_IPCD_BUSY |
+                       IPC_HEADER_DATA(IPC_HEADER_DATA_MASK);
+               old = sst_dsp_shim_read64_unlocked(sst, SST_IPCD);
+               new = (old & (~mask)) | (SST_BYT_IPCD_DONE & mask);
+
+               if (old != new)
+                       sst_dsp_shim_write64_unlocked(sst, SST_IPCD, new);
+
+               /*
+                * workaround, give it a second chance if the SST_IPCD
+                * changes
+                */
+               if (old != header) {
+                       dev_dbg(ipc->dev, "%s: header 0x%llx old 0x%llx\n",
+                               __func__, header, old);
+                       if (old & SST_BYT_IPCD_BUSY) {
+                               if (old & IPC_NOTIFICATION) {
+                                       /* message from ADSP */
+                                       sst_byt_process_notification(byt, &flags);
+                               } else {
+                                       /* reply from ADSP */
+                                       sst_byt_process_reply(byt, old);
+                               }
+                       }
+               }
                /* unmask message request interrupts */
                sst_dsp_shim_update_bits64_unlocked(sst, SST_IMRX,
                        SST_BYT_IMRX_REQUEST, 0);