| Message ID | 20230313205029.1881745-1-risca@dalakolonin.se (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | iio: adc: palmas_gpadc: fix NULL dereference on rmmod | expand |
On Mon, 13 Mar 2023 21:50:29 +0100 Patrik Dahlström <risca@dalakolonin.se> wrote: > Calling dev_to_iio_dev() on a platform device pointer is undefined and > will make adc NULL. > > Signed-off-by: Patrik Dahlström <risca@dalakolonin.se> Hi Patrik, Looks good so applied to the fixes-togreg branch of iio.git. Whilst we are here, this would be a trivial driver to take fully device managed. The only slightly messy bit is that it would need a devm_add_action_or_reset() + custom callback to handle the device_wakeup_enable(). On the off chance you can test it I'll send a patch in a few mins. Note that will depend on this one going up stream first and that I haven't done more than build test it. Thanks, Jonathan > --- > drivers/iio/adc/palmas_gpadc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/iio/adc/palmas_gpadc.c b/drivers/iio/adc/palmas_gpadc.c > index 61e80bf3d05e..6db6f3bc768a 100644 > --- a/drivers/iio/adc/palmas_gpadc.c > +++ b/drivers/iio/adc/palmas_gpadc.c > @@ -638,7 +638,7 @@ static int palmas_gpadc_probe(struct platform_device *pdev) > > static int palmas_gpadc_remove(struct platform_device *pdev) > { > - struct iio_dev *indio_dev = dev_to_iio_dev(&pdev->dev); > + struct iio_dev *indio_dev = dev_get_drvdata(&pdev->dev); > struct palmas_gpadc *adc = iio_priv(indio_dev); > > if (adc->wakeup1_enable || adc->wakeup2_enable)
On Sat, Mar 18, 2023 at 04:30:33PM +0000, Jonathan Cameron wrote: > On Mon, 13 Mar 2023 21:50:29 +0100 > Patrik Dahlström <risca@dalakolonin.se> wrote: > > > Calling dev_to_iio_dev() on a platform device pointer is undefined and > > will make adc NULL. > > > > Signed-off-by: Patrik Dahlström <risca@dalakolonin.se> > > Hi Patrik, > > Looks good so applied to the fixes-togreg branch of iio.git. > > Whilst we are here, this would be a trivial driver to take fully device > managed. The only slightly messy bit is that it would need > a devm_add_action_or_reset() + custom callback to handle the > device_wakeup_enable(). > > On the off chance you can test it I'll send a patch in a few mins. > Note that will depend on this one going up stream first and that > I haven't done more than build test it. I got the patch and it looks good, but it will take a few days before I have the time to test it. I have some more patches coming for this driver to configure the adc thresholds from userspace, employing the iio channel event subsystem, but they need a bit more work. In particular, to ensure backwards compatibility with the adc_wakeupX_data platform data. However, I don't see this platform data being used by anyone. How important is it to retain support for adc_wakeupX_data? > > Thanks, > > Jonathan Thank you for going the extra mile :) > > > > --- > > drivers/iio/adc/palmas_gpadc.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/iio/adc/palmas_gpadc.c b/drivers/iio/adc/palmas_gpadc.c > > index 61e80bf3d05e..6db6f3bc768a 100644 > > --- a/drivers/iio/adc/palmas_gpadc.c > > +++ b/drivers/iio/adc/palmas_gpadc.c > > @@ -638,7 +638,7 @@ static int palmas_gpadc_probe(struct platform_device *pdev) > > > > static int palmas_gpadc_remove(struct platform_device *pdev) > > { > > - struct iio_dev *indio_dev = dev_to_iio_dev(&pdev->dev); > > + struct iio_dev *indio_dev = dev_get_drvdata(&pdev->dev); > > struct palmas_gpadc *adc = iio_priv(indio_dev); > > > > if (adc->wakeup1_enable || adc->wakeup2_enable) >
On Sat, 18 Mar 2023 20:22:53 +0100 Patrik Dahlström <risca@dalakolonin.se> wrote: > On Sat, Mar 18, 2023 at 04:30:33PM +0000, Jonathan Cameron wrote: > > On Mon, 13 Mar 2023 21:50:29 +0100 > > Patrik Dahlström <risca@dalakolonin.se> wrote: > > > > > Calling dev_to_iio_dev() on a platform device pointer is undefined and > > > will make adc NULL. > > > > > > Signed-off-by: Patrik Dahlström <risca@dalakolonin.se> > > > > Hi Patrik, > > > > Looks good so applied to the fixes-togreg branch of iio.git. > > > > Whilst we are here, this would be a trivial driver to take fully device > > managed. The only slightly messy bit is that it would need > > a devm_add_action_or_reset() + custom callback to handle the > > device_wakeup_enable(). > > > > On the off chance you can test it I'll send a patch in a few mins. > > Note that will depend on this one going up stream first and that > > I haven't done more than build test it. > I got the patch and it looks good, but it will take a few days before I > have the time to test it. > > I have some more patches coming for this driver to configure the adc > thresholds from userspace, employing the iio channel event subsystem, but > they need a bit more work. In particular, to ensure backwards compatibility > with the adc_wakeupX_data platform data. However, I don't see this platform > data being used by anyone. > How important is it to retain support for adc_wakeupX_data? It's a somewhat unusual feature, so I doubt it was implemented without someone needing it. However as you observe there is no upstream user. As it is causing you problems, I'd just rip out the palmas_adc_platform_data completely and see if anyone objects. You can do that as a standalone patch prior to posting your events stuff if you like. Or hopefully H. Nikolaus Schaller might be able to give us some background on why that feature is there but not used. > > > > Thanks, > > > > Jonathan > > Thank you for going the extra mile :) No problem. I jumped on the opportunity to get it tested - takes way longer than writing a little patch like that ;) Jonathan > > > > > > > --- > > > drivers/iio/adc/palmas_gpadc.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/iio/adc/palmas_gpadc.c b/drivers/iio/adc/palmas_gpadc.c > > > index 61e80bf3d05e..6db6f3bc768a 100644 > > > --- a/drivers/iio/adc/palmas_gpadc.c > > > +++ b/drivers/iio/adc/palmas_gpadc.c > > > @@ -638,7 +638,7 @@ static int palmas_gpadc_probe(struct platform_device *pdev) > > > > > > static int palmas_gpadc_remove(struct platform_device *pdev) > > > { > > > - struct iio_dev *indio_dev = dev_to_iio_dev(&pdev->dev); > > > + struct iio_dev *indio_dev = dev_get_drvdata(&pdev->dev); > > > struct palmas_gpadc *adc = iio_priv(indio_dev); > > > > > > if (adc->wakeup1_enable || adc->wakeup2_enable) > >
Hi there, > Am 19.03.2023 um 16:32 schrieb Jonathan Cameron <jic23@kernel.org>: > > On Sat, 18 Mar 2023 20:22:53 +0100 > Patrik Dahlström <risca@dalakolonin.se> wrote: > >> On Sat, Mar 18, 2023 at 04:30:33PM +0000, Jonathan Cameron wrote: >>> On Mon, 13 Mar 2023 21:50:29 +0100 >>> Patrik Dahlström <risca@dalakolonin.se> wrote: >>> >>>> Calling dev_to_iio_dev() on a platform device pointer is undefined and >>>> will make adc NULL. >>>> >>>> Signed-off-by: Patrik Dahlström <risca@dalakolonin.se> >>> >>> Hi Patrik, >>> >>> Looks good so applied to the fixes-togreg branch of iio.git. >>> >>> Whilst we are here, this would be a trivial driver to take fully device >>> managed. The only slightly messy bit is that it would need >>> a devm_add_action_or_reset() + custom callback to handle the >>> device_wakeup_enable(). >>> >>> On the off chance you can test it I'll send a patch in a few mins. >>> Note that will depend on this one going up stream first and that >>> I haven't done more than build test it. >> I got the patch and it looks good, but it will take a few days before I >> have the time to test it. >> >> I have some more patches coming for this driver to configure the adc >> thresholds from userspace, Yes, that is a useful feature. >> employing the iio channel event subsystem, but >> they need a bit more work. In particular, to ensure backwards compatibility >> with the adc_wakeupX_data platform data. However, I don't see this platform >> data being used by anyone. >> How important is it to retain support for adc_wakeupX_data? > > It's a somewhat unusual feature, so I doubt it was implemented without someone > needing it. However as you observe there is no upstream user. > > As it is causing you problems, I'd just rip out the palmas_adc_platform_data > completely and see if anyone objects. You can do that as a standalone patch > prior to posting your events stuff if you like. Or hopefully > H. Nikolaus Schaller might be able to give us some background on why > that feature is there but not used. I also have no idea. The original author was Pradeep Goudagunta <pgoudagunta@nvidia.com> and I just copied it from https://android.googlesource.com/kernel/tegra/+/aaabb2e045f31e5a970109ffdaae900dd403d17e/drivers/staging/iio/adc/palmas_gpadc.c polished the code and made it compile & work some years ago. So it may have been useful in a some Tegra Android kernel from 2013. Maybe it is for special power management (at least that is how I interpret the "wakeup"). But I found some hint which device it is good for: https://lore.kernel.org/all/1379509642-19227-2-git-send-email-ldewangan@nvidia.com/T/ "PALMAS PMIC is used on Dalmore platform." https://docs.nvidia.com/gameworks/content/devices/dalmore_devkit_main.htm And there seems to be an upstream DTS: arch/arm/boot/dts/tegra114-dalmore.dts but without gpadc support. That is quite common that upstream DTS are incomplete so we can't deduce that there are no users of a feature. BR, Nikolaus > >>> >>> Thanks, >>> >>> Jonathan >> >> Thank you for going the extra mile :) > > No problem. I jumped on the opportunity to get it tested - takes way longer > than writing a little patch like that ;) > > Jonathan > >>> >>> >>>> --- >>>> drivers/iio/adc/palmas_gpadc.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/iio/adc/palmas_gpadc.c b/drivers/iio/adc/palmas_gpadc.c >>>> index 61e80bf3d05e..6db6f3bc768a 100644 >>>> --- a/drivers/iio/adc/palmas_gpadc.c >>>> +++ b/drivers/iio/adc/palmas_gpadc.c >>>> @@ -638,7 +638,7 @@ static int palmas_gpadc_probe(struct platform_device *pdev) >>>> >>>> static int palmas_gpadc_remove(struct platform_device *pdev) >>>> { >>>> - struct iio_dev *indio_dev = dev_to_iio_dev(&pdev->dev); >>>> + struct iio_dev *indio_dev = dev_get_drvdata(&pdev->dev); >>>> struct palmas_gpadc *adc = iio_priv(indio_dev); >>>> >>>> if (adc->wakeup1_enable || adc->wakeup2_enable) >>> >
diff --git a/drivers/iio/adc/palmas_gpadc.c b/drivers/iio/adc/palmas_gpadc.c index 61e80bf3d05e..6db6f3bc768a 100644 --- a/drivers/iio/adc/palmas_gpadc.c +++ b/drivers/iio/adc/palmas_gpadc.c @@ -638,7 +638,7 @@ static int palmas_gpadc_probe(struct platform_device *pdev) static int palmas_gpadc_remove(struct platform_device *pdev) { - struct iio_dev *indio_dev = dev_to_iio_dev(&pdev->dev); + struct iio_dev *indio_dev = dev_get_drvdata(&pdev->dev); struct palmas_gpadc *adc = iio_priv(indio_dev); if (adc->wakeup1_enable || adc->wakeup2_enable)
Calling dev_to_iio_dev() on a platform device pointer is undefined and will make adc NULL. Signed-off-by: Patrik Dahlström <risca@dalakolonin.se> --- drivers/iio/adc/palmas_gpadc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)