[PATCH] iio: adc: palmas_gpadc: fix NULL dereference on rmmod

[PATCH] iio: adc: palmas_gpadc: fix NULL dereference on rmmod

[Patrik Dahlström]

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

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

Re: [PATCH] iio: adc: palmas_gpadc: fix NULL dereference on rmmod

[Jonathan Cameron]

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)

Re: [PATCH] iio: adc: palmas_gpadc: fix NULL dereference on rmmod

[Patrik Dahlström]

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

Re: [PATCH] iio: adc: palmas_gpadc: fix NULL dereference on rmmod

[Jonathan Cameron]

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

Re: [PATCH] iio: adc: palmas_gpadc: fix NULL dereference on rmmod

[H. Nikolaus Schaller]

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