Date:   Sun, 19 Mar 2023 15:32:03 +0000
From:   Jonathan Cameron <jic23@kernel.org>
To:     Patrik =?UTF-8?B?RGFobHN0csO2bQ==?= <risca@dalakolonin.se>
Cc:     lars@metafoo.de, linux-iio@vger.kernel.org,
        linux-kernel@vger.kernel.org, hns@goldelico.com
Subject: Re: [PATCH] iio: adc: palmas_gpadc: fix NULL dereference on rmmod
Message-ID: <20230319153203.54a40887@jic23-huawei>
In-Reply-To: <20230318192253.GB3605556@dalakolonin.se>
References: <20230313205029.1881745-1-risca@dalakolonin.se>
        <20230318163033.161d6fd5@jic23-huawei>
        <20230318192253.GB3605556@dalakolonin.se>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sat, 18 Mar 2023 20:22:53 +0100
Patrik Dahlstr=C3=B6m <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=C3=B6m <risca@dalakolonin.se> wrote:
> >  =20
> > > Calling dev_to_iio_dev() on a platform device pointer is undefined and
> > > will make adc NULL.
> > >=20
> > > Signed-off-by: Patrik Dahlstr=C3=B6m <risca@dalakolonin.se> =20
> >=20
> > Hi Patrik,
> >=20
> > Looks good so applied to the fixes-togreg branch of iio.git.
> >=20
> > 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().
> >=20
> > 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. =20
> I got the patch and it looks good, but it will take a few days before I
> have the time to test it.
>=20
> 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 compatibili=
ty
> with the adc_wakeupX_data platform data. However, I don't see this platfo=
rm
> 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 some=
one
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.

> >=20
> > Thanks,
> >=20
> > Jonathan =20
>=20
> 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

> >=20
> >  =20
> > > ---
> > >  drivers/iio/adc/palmas_gpadc.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >=20
> > > 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_dev=
ice *pdev)
> > > =20
> > >  static int palmas_gpadc_remove(struct platform_device *pdev)
> > >  {
> > > -	struct iio_dev *indio_dev =3D dev_to_iio_dev(&pdev->dev);
> > > +	struct iio_dev *indio_dev =3D dev_get_drvdata(&pdev->dev);
> > >  	struct palmas_gpadc *adc =3D iio_priv(indio_dev);
> > > =20
> > >  	if (adc->wakeup1_enable || adc->wakeup2_enable) =20
> >  =20