Re: aspeed-adc driver kpanic

[Peter Delevoryas <pdel@fb.com>]

> On Oct 4, 2021, at 11:54 AM, Patrick Williams <patrick@stwcx.xyz> wrote:
> 
> On Mon, Oct 04, 2021 at 01:34:54PM -0500, Patrick Williams wrote:
>> Hi Billy,
>> 
>> When I run the latest linux-5.14 on QEMU with the Witherspoon config, I end up
>> with a kernel panic[1].  I think there is an ordering problem in the aspeed_adc
>> driver.  
>> 
>> See [2,3].  The code registers with devm a pointer to the prescaler object which
>> is not yet created.  I think it is possible that the struct value contains
>> uninitialized data as well.  Can you please take a look at this?
>> 
>> 1. https://gist.github.com/williamspatrick/4a0f0d1e0ca6f54816461a8df09e6cb8
>> 2. https://github.com/openbmc/linux/blob/dev-5.14/drivers/iio/adc/aspeed_adc.c#L513
>> 3. https://github.com/openbmc/linux/blob/dev-5.14/drivers/iio/adc/aspeed_adc.c#L527
>> 
>> -- 
>> Patrick Williams
> 
> Also, Peter D. has been working on getting the QEMU code for the ADC working
> and I cherry-picked his commits[1] and the code gets farther but crashes with
> what seems like a memory alignment issue in a read at [2].  New gist of kernel panic
> at [3].

Oh yeah, this is probably not the driver’s fault, this is the fault of my QEMU
patches. I only allowed 32-bit aligned reads. I bet if you apply this additional
diff, it won’t crash, but it’ll probably return the channel 0 values when you’re
trying to read channel 1 values, channel 2 instead of channel 3, etc. I think only
the data registers require the 16-bit read access, because 2 channels are packed
in each 32-bit data register, but the bounds and hysteresis registers are
1 channel per register.

diff --git a/hw/adc/aspeed_adc.c b/hw/adc/aspeed_adc.c
index fcd93d6853..58e3f18c6c 100644
--- a/hw/adc/aspeed_adc.c
+++ b/hw/adc/aspeed_adc.c
@@ -234,9 +234,9 @@ static const MemoryRegionOps aspeed_adc_engine_ops = {
     .write = aspeed_adc_engine_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
     .valid = {
-        .min_access_size = 4,
+        .min_access_size = 1,
         .max_access_size = 4,
-        .unaligned = false,
+        .unaligned = true,
     },
 };

I intend to resolve this by resubmitting Andrew Jeffery’s patch for supporting
16-bit and 8-bit reads transparently to the QEMU model, but maybe I’ll also
revise my patch to support the 16-bit reads (even without Andrew’s special
memory patch).

https://lore.kernel.org/qemu-devel/20170630030058.28943-1-andrew@aj.id.au/

Cedric just approved what I had, but it hasn’t been pulled yet: if you want,
feel free to comment, I’ll probably comment about it myself too.

https://lore.kernel.org/qemu-devel/4d7c55d4-25fd-520c-97aa-98036fe6fd1a@kaod.org/

Thanks!
Peter

> 
> 1. https://github.com/peterdelevoryas/qemu/tree/adc-support-v2
> 2. https://github.com/openbmc/linux/blob/dev-5.14/drivers/iio/adc/aspeed_adc.c#L248
> 3. https://gist.github.com/williamspatrick/76827c99e2db8fce385b9a87f7526d33
> 
> -- 
> Patrick Williams