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: <c8309abf-cbfb-a3db-5aa7-2e2f748a6d34@intel.com>
Date:   Tue, 14 Apr 2020 21:53:33 +0200
From:   Cezary Rojewski <cezary.rojewski@...el.com>
To:     "Lu, Brent" <brent.lu@...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>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "clang-built-linux@...glegroups.com" 
        <clang-built-linux@...glegroups.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Takashi Iwai <tiwai@...e.com>,
        Jie Yang <yang.jie@...ux.intel.com>,
        Liam Girdwood <liam.r.girdwood@...ux.intel.com>,
        Richard Fontana <rfontana@...hat.com>,
        Mark Brown <broonie@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Allison Randal <allison@...utok.net>,
        "amadeuszx.slawinski@...ux.intel.com" 
        <amadeuszx.slawinski@...ux.intel.com>
Subject: Re: [PATCH] ASoC: Intel: sst: ipc command timeout

On 2020-04-14 18:20, Lu, Brent wrote:
>>
>> I have mixed feelings about this.
>>
>> One one hand, this looks simple enough.
>>
>> But on the other hand we have other users of memcpy_fromio(), including
>> SOF drivers, so what are the odds we have the same problems in other
>> places? Wouldn't it be safer to either change this function so that it's
>> behavior is not ambiguous or compiler-dependent, or fix the compiler?
>>
> 
> Hi Pierre and Amadeusz,
> 
> I have to admit that I didn't dig into clang's __builtin_memcpy to see what's
> happening inside so I don't have direct evidence to say it's clang's problem.
> What I know is kernel built by clang10 works fine but have this issue once
> changed to clang11. At first I also suspect that it's a timing issue so I checked
> the command transaction. The transaction is simple, host writes command
> in SST_IPCX register, the DSP then writes reply in SST_IPCD register and
> trigger an interrupt. Finally the irq thread sst_byt_irq_thread() reads the
> SST_IPCD register to complete the transaction. I added some debug messages
> to see if there is something wrong in the transaction but it all looks good.
> 
> I am also confused that why this only happens to BYT but not BDW since they
> share the same register accessing code in sst-dsp.c. I checked the code and
> realized that in BDW, the irq thread (hsw_irq_thread) performs 32-bit register
> read instead of 64-bit in BYT platform. Therefore I change the code in BYT to
> use two readl() calls and found the problem is gone. My best guess is it's
> related to the implementation of __builtin_memcpy() but not sure it's the
> timing or implementing cause this problem.
> 
> 
> Regards,
> Brent
> 

Regs width difference between BDW and BYT comes from specification. BDW 
has IPC registers which are 32 wide. This fact ain't exactly the reason 
to modify sst_shim32_read64.

I'm sharing Amadeo's point of view. Your change should slow down 
execution a bit - but that might be just what handlers needed to make 
everything work again. Debug prints also slow down the execution what 
could have prevented you from spotting the real problem.
Let's ignore the memcpy stuff for a moment - could you focus on 
elaborating the scenario where such issue occurs? Your initial commit 
message also skips important bits such as platform used when reproducing 
and so on, please add them.

Thanks,
Czarek