* aspeed-adc driver kpanic @ 2021-10-04 18:34 Patrick Williams 2021-10-04 18:54 ` Patrick Williams 2021-10-05 0:00 ` Joel Stanley 0 siblings, 2 replies; 5+ messages in thread From: Patrick Williams @ 2021-10-04 18:34 UTC (permalink / raw) To: Billy Tsai; +Cc: OpenBMC List [-- 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 --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: aspeed-adc driver kpanic 2021-10-04 18:34 aspeed-adc driver kpanic Patrick Williams @ 2021-10-04 18:54 ` Patrick Williams 2021-10-04 19:26 ` Peter Delevoryas 2021-10-05 0:00 ` Joel Stanley 1 sibling, 1 reply; 5+ messages in thread From: Patrick Williams @ 2021-10-04 18:54 UTC (permalink / raw) To: Billy Tsai; +Cc: OpenBMC List, Peter Delevoryas [-- 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 --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: aspeed-adc driver kpanic 2021-10-04 18:54 ` Patrick Williams @ 2021-10-04 19:26 ` Peter Delevoryas 2021-10-04 22:25 ` Patrick Williams 0 siblings, 1 reply; 5+ messages in thread From: Peter Delevoryas @ 2021-10-04 19:26 UTC (permalink / raw) Cc: Billy Tsai, OpenBMC List > 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: aspeed-adc driver kpanic 2021-10-04 19:26 ` Peter Delevoryas @ 2021-10-04 22:25 ` Patrick Williams 0 siblings, 0 replies; 5+ messages in thread From: Patrick Williams @ 2021-10-04 22:25 UTC (permalink / raw) To: Peter Delevoryas; +Cc: Billy Tsai, OpenBMC List [-- 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 --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: aspeed-adc driver kpanic 2021-10-04 18:34 aspeed-adc driver kpanic Patrick Williams 2021-10-04 18:54 ` Patrick Williams @ 2021-10-05 0:00 ` Joel Stanley 1 sibling, 0 replies; 5+ messages in thread From: Joel Stanley @ 2021-10-05 0:00 UTC (permalink / raw) To: Patrick Williams; +Cc: Billy Tsai, OpenBMC List 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-10-05 0:02 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-04 18:34 aspeed-adc driver kpanic Patrick Williams 2021-10-04 18:54 ` Patrick Williams 2021-10-04 19:26 ` Peter Delevoryas 2021-10-04 22:25 ` Patrick Williams 2021-10-05 0:00 ` Joel Stanley
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).