aspeed-adc driver kpanic

aspeed-adc driver kpanic

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 685 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: aspeed-adc driver kpanic

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 1291 bytes --]

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].

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: aspeed-adc driver kpanic

[Peter Delevoryas]

> 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

Re: aspeed-adc driver kpanic

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 973 bytes --]

On Mon, Oct 04, 2021 at 07:26:04PM +0000, Peter Delevoryas wrote:

> 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

FWIW, applying this did get me past the kernel panic.  I don't really care about
any of the data right now.  I was trying to test something out in userspace.

> 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,
>      },
>  };

-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: aspeed-adc driver kpanic

[Joel Stanley]

On Mon, 4 Oct 2021 at 18:35, Patrick Williams <patrick@stwcx.xyz> 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?

I merged v6 of Billy's patches, but he sent a v7 version that
contained a fix this issue:

--- a/drivers/iio/adc/aspeed_adc.c
+++ b/drivers/iio/adc/aspeed_adc.c
@@ -492,8 +492,8 @@ static int aspeed_adc_probe(struct platform_device *pdev)

        data = iio_priv(indio_dev);
        data->dev = &pdev->dev;
-       data->model_data = of_device_get_match_data(&pdev->dev);
        platform_set_drvdata(pdev, indio_dev);
+       data->model_data = of_device_get_match_data(&pdev->dev);

        data->base = devm_platform_ioremap_resource(pdev, 0);
        if (IS_ERR(data->base))
@@ -512,7 +512,7 @@ static int aspeed_adc_probe(struct platform_device *pdev)

        ret = devm_add_action_or_reset(data->dev,
                                       aspeed_adc_unregister_fixed_divider,
-                                      data->clk_prescaler);
+                                      data->fixed_div_clk);
        if (ret)
                return ret;
        snprintf(clk_parent_name, ARRAY_SIZE(clk_parent_name), clk_name);


>
> 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