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