[v3,02/27] staging: iio: resolver: ad2s1210: fix use before initialization

Commit Message

David Lechner Sept. 29, 2023, 5:23 p.m. UTC

From: David Lechner <[e-mail]>

From: David Lechner <[e-mail]>

This fixes a use before initialization in ad2s1210_probe(). The
ad2s1210_setup_gpios() function uses st->sdev but it was being called
before this field was initialized.

Signed-off-by: David Lechner <[e-mail]>
---

v3 changes:
* This is a new patch split out from "staging: iio: resolver: ad2s1210:
 fix probe"

 drivers/staging/iio/resolver/ad2s1210.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Jonathan Cameron Sept. 30, 2023, 2:28 p.m. UTC | #1

On Fri, 29 Sep 2023 12:23:07 -0500
David Lechner <[e-mail]> wrote:

> From: David Lechner <[e-mail]>
> 
> From: David Lechner <[e-mail]>
> 
> This fixes a use before initialization in ad2s1210_probe(). The
> ad2s1210_setup_gpios() function uses st->sdev but it was being called
> before this field was initialized.
> 
> Signed-off-by: David Lechner <[e-mail]>
Applied to the togreg banch of iio.git and pushed out as testing for 0-day to
poke at it.

I didn't pull this out as a fix to upstream quicker because it would
make a mess of the rest of applying the rest of the series.

Maybe we want to consider backporting some of these at somepoint.

Jonathan

> ---
> 
> v3 changes:
> * This is a new patch split out from "staging: iio: resolver: ad2s1210:
>  fix probe"
> 
>  drivers/staging/iio/resolver/ad2s1210.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> index f695ca0547e4..3f08b59f4e19 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -658,9 +658,6 @@ static int ad2s1210_probe(struct spi_device *spi)
>  	if (!indio_dev)
>  		return -ENOMEM;
>  	st = iio_priv(indio_dev);
> -	ret = ad2s1210_setup_gpios(st);
> -	if (ret < 0)
> -		return ret;
>  
>  	spi_set_drvdata(spi, indio_dev);
>  
> @@ -671,6 +668,10 @@ static int ad2s1210_probe(struct spi_device *spi)
>  	st->resolution = 12;
>  	st->fexcit = AD2S1210_DEF_EXCIT;
>  
> +	ret = ad2s1210_setup_gpios(st);
> +	if (ret < 0)
> +		return ret;
> +
>  	indio_dev->info = &ad2s1210_info;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->channels = ad2s1210_channels;
>

Dan Carpenter Oct. 2, 2023, 8:07 a.m. UTC | #2

On Fri, Sep 29, 2023 at 12:23:07PM -0500, David Lechner wrote:
> From: David Lechner <[e-mail]>
> 
> From: David Lechner <[e-mail]>
> 
> This fixes a use before initialization in ad2s1210_probe(). The
> ad2s1210_setup_gpios() function uses st->sdev but it was being called
> before this field was initialized.
> 
> Signed-off-by: David Lechner <[e-mail]>
> ---
> 

Fixes: b19e9ad5e2cb ("staging:iio:resolver:ad2s1210 general driver cleanup.")

This would crash the driver right away, on probe.  It's amazing no one
filed a bug report even though the bug is 12 years old.

regards,
dan carpenter

Jonathan Cameron Oct. 2, 2023, 9:17 a.m. UTC | #3

On Mon, 2 Oct 2023 11:07:15 +0300
Dan Carpenter <[e-mail]> wrote:

> On Fri, Sep 29, 2023 at 12:23:07PM -0500, David Lechner wrote:
> > From: David Lechner <[e-mail]>
> > 
> > From: David Lechner <[e-mail]>
> > 
> > This fixes a use before initialization in ad2s1210_probe(). The
> > ad2s1210_setup_gpios() function uses st->sdev but it was being called
> > before this field was initialized.
> > 
> > Signed-off-by: David Lechner <[e-mail]>
> > ---
> >   
> 
> Fixes: b19e9ad5e2cb ("staging:iio:resolver:ad2s1210 general driver cleanup.")
Thanks but nope, not that one.

At that point ad2s1210_setup_gpios, didn't use st->sdev.
I think this went wrong when the platform data was removed in 

I 'think' it was
Fixes: f356dc6ec26b ("staging: iio: ad2s1210: Switch to the gpio descriptor interface")



> 
> This would crash the driver right away, on probe.  It's amazing no one
> filed a bug report even though the bug is 12 years old.
Only 5 years :)

Welcome to the long tail of IIO Devices and the long term availability of the
hardware - this is still a production part.  Clearly no one was using the
upstream driver for 5 + years, but here comes David who is not only fixing
the bugs but cleaning it up.

Hmm. What happened to roadtest? I was hoping that would solve this sort
of issue by allowing simple testing of basic functionality... Hope it
is still headed for a new version / upstream!

Jonathan

> regards,
> dan carpenter

Vincent Whitchurch Oct. 6, 2023, 2:48 p.m. UTC | #4

On Mon, 2023-10-02 at 10:17 +0100, Jonathan Cameron wrote:
> Hmm. What happened to roadtest?  I was hoping that would solve this sort
> of issue by allowing simple testing of basic functionality...

Roadtest is alive and well.  Several of my coworkers have been using it
for development and testing of new drivers[0][1][2][3][4] and
patches[5][6], and this has resulted in easier testing and refactoring
during development, more robust code, and of course the ability to
easily detect regressions after the patches are merged.

[0] https://lore.kernel.org/lkml/[e-mail]/
[1] https://lore.kernel.org/lkml/d218[e-mail]/
[2] https://lore.kernel.org/lkml/7b85[e-mail]/
[3] https://lore.kernel.org/lkml/[e-mail]/
[4] https://lore.kernel.org/lkml/[e-mail]/
[5] https://lore.kernel.org/lkml/[e-mail]/
[6] https://lore.kernel.org/lkml/[e-mail]/

In fact, by running our roadtests on newer kernels we have found
numerous bugs[10][12][14] and regressions[7][8][9][11][15] in mainline,
including subsystem-level issues affecting other drivers too.

[7] https://lore.kernel.org/lkml/[e-mail]/
[8] https://lore.kernel.org/lkml/[e-mail]/
[9] https://lore.kernel.org/lkml/[e-mail]/
[10] https://lore.kernel.org/lkml/[e-mail]/
[11] https://lore.kernel.org/lkml/[e-mail]/
[12] https://lore.kernel.org/linux-iio/[e-mail]/
[13] https://lore.kernel.org/linux-iio/[e-mail]/
[14] https://lore.kernel.org/linux-iio/[e-mail]/
[15] https://lore.kernel.org/linux-spi/[e-mail]/

(The above lists are not exhaustive.)

> Hope it is still headed for a new version / upstream!

I pushed out an update with a squash of (most parts of) our internal
version out to the following repo, it's based on v6.6-rc4.

  https://github.com/vwax/linux/tree/roadtest/devel

(There are currently 6 lines of --diff-filter=M against v6.6-rc4 on the
 linked repo.  Two of those are from a patch which is posted and waiting
 for review on the lists, and the rest are for enabling regmap debugfs
 writes which are used from some of the newer tests.)

Since roadtest itself does not require any patches to the kernel or any
out-of-tree modules, the maintenance of the framework would not really
be simplified by putting it in the upstream tree.  However, there is of
course a potentially large benefit to the quality of many kinds of
kernel drivers if roadtest gets used by others, and having it in-tree
could facilitate that.  And it would potentially allow regressions like
the ones we're finding to be caught _before_ they go in, since anyone
can run the tests without special hardware.

The idea of having to maintain it in-tree and doing all the work that
goes along with that (dealing with the expectations of maintainers,
wrangling patches from mailing lists, etc), is something I personally
have had a hard time warming up to, but I have some coworkers who may
potentially be interested in that kind of work, so I wouldn't rule out
another posting of the patch set targeting upstream sometime in the
future.

Jonathan Cameron Oct. 10, 2023, 9:29 a.m. UTC | #5

On Fri, 6 Oct 2023 14:48:29 +0000
Vincent Whitchurch <[e-mail]> wrote:

Hi Vincent

Thanks for the update,

> On Mon, 2023-10-02 at 10:17 +0100, Jonathan Cameron wrote:
> > Hmm. What happened to roadtest?  I was hoping that would solve this sort
> > of issue by allowing simple testing of basic functionality...  
> 
> Roadtest is alive and well.  Several of my coworkers have been using it
> for development and testing of new drivers[0][1][2][3][4] and
> patches[5][6], and this has resulted in easier testing and refactoring
> during development, more robust code, and of course the ability to
> easily detect regressions after the patches are merged.
> 
> [0] https://lore.kernel.org/lkml/[e-mail]/
> [1] https://lore.kernel.org/lkml/d218[e-mail]/
> [2] https://lore.kernel.org/lkml/7b85[e-mail]/
> [3] https://lore.kernel.org/lkml/[e-mail]/
> [4] https://lore.kernel.org/lkml/[e-mail]/
> [5] https://lore.kernel.org/lkml/[e-mail]/
> [6] https://lore.kernel.org/lkml/[e-mail]/
> 
> In fact, by running our roadtests on newer kernels we have found
> numerous bugs[10][12][14] and regressions[7][8][9][11][15] in mainline,
> including subsystem-level issues affecting other drivers too.
> 
> [7] https://lore.kernel.org/lkml/[e-mail]/
> [8] https://lore.kernel.org/lkml/[e-mail]/
> [9] https://lore.kernel.org/lkml/[e-mail]/
> [10] https://lore.kernel.org/lkml/[e-mail]/
> [11] https://lore.kernel.org/lkml/[e-mail]/
> [12] https://lore.kernel.org/linux-iio/[e-mail]/
> [13] https://lore.kernel.org/linux-iio/[e-mail]/
> [14] https://lore.kernel.org/linux-iio/[e-mail]/
> [15] https://lore.kernel.org/linux-spi/[e-mail]/
> 
> (The above lists are not exhaustive.)
> 

Great stuff!

> > Hope it is still headed for a new version / upstream!  
> 
> I pushed out an update with a squash of (most parts of) our internal
> version out to the following repo, it's based on v6.6-rc4.
> 
>   https://github.com/vwax/linux/tree/roadtest/devel

Thanks.

> 
> (There are currently 6 lines of --diff-filter=M against v6.6-rc4 on the
>  linked repo.  Two of those are from a patch which is posted and waiting
>  for review on the lists, and the rest are for enabling regmap debugfs
>  writes which are used from some of the newer tests.)
> 
> Since roadtest itself does not require any patches to the kernel or any
> out-of-tree modules, the maintenance of the framework would not really
> be simplified by putting it in the upstream tree.  However, there is of
> course a potentially large benefit to the quality of many kinds of
> kernel drivers if roadtest gets used by others, and having it in-tree
> could facilitate that.  And it would potentially allow regressions like
> the ones we're finding to be caught _before_ they go in, since anyone
> can run the tests without special hardware.

Exactly  - my main interest is the dream of getting to the point where
new drivers typically also come with roadtest tests, with the aim that
they will be used for regression testing. For IIO I might lean on
/ ask nicely  few of the bigger contributors to add fairly comprehensive
tests for say one in 3 of their drivers, providing a canary for any
subsystem level problems that might sneak in. The stability gained for
those drivers might also prove it's own benefit to push people to add tests.
At somepoint in the longer term I might even make it a requirement for
upstreaming a new driver + slowly tackle the backlog of existing ones.
From my experiments with it last year, this is a trivial burden fo

> 
> The idea of having to maintain it in-tree and doing all the work that
> goes along with that (dealing with the expectations of maintainers,
> wrangling patches from mailing lists, etc), is something I personally
> have had a hard time warming up to, but I have some coworkers who may
> potentially be interested in that kind of work, so I wouldn't rule out
> another posting of the patch set targeting upstream sometime in the
> future.

I fully appreciate your concern.  I just really like roadtest and want
a smooth way to integrate using it with my upstream maintenance (and occasional
development) process...  I of course can't expect you to commit to anything
though - I'd be delighted if someone else wants to take this forwards but
that would be very much their decision to make!

Having not yet waded into the latest code, how 'stable' is it from the point
of view of modifications to tests?  I can rebase the ones I have out of tree
and see, but I'm after an assessment that incorporates what you are
planning to change in future.

I guess the nasty stuff is if you have a few hundred additional drivers
in the test set, any modification to the way they interact with the core
of roadtest becomes very painful to push into those tests.

One starting point would be to separate the tests directory from the
directories containing roadtest frameworks etc as that would help to
limit scope of responsibility.

If a potential upstream roadtest maintainer is primarily concerned about
review + handling of the actual tests, other than potentially letting in
some ugly code, I'd imagine any subsystem maintainer who opts into this
will take that burden on - perhaps with the occasional question heading
your way. I'd certainly not expect you to have to deal with high patch flows
and would ensure that didn't happen for any IIO tests (any review people
have time for is of course welcome!)

+CC a few maintainers of other subsystems who may be interested (I know
one of them is ;)

Jonathan

Patch

diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index f695ca0547e4..3f08b59f4e19 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -658,9 +658,6 @@  static int ad2s1210_probe(struct spi_device *spi)
 	if (!indio_dev)
 		return -ENOMEM;
 	st = iio_priv(indio_dev);
-	ret = ad2s1210_setup_gpios(st);
-	if (ret < 0)
-		return ret;
 
 	spi_set_drvdata(spi, indio_dev);
 
@@ -671,6 +668,10 @@  static int ad2s1210_probe(struct spi_device *spi)
 	st->resolution = 12;
 	st->fexcit = AD2S1210_DEF_EXCIT;
 
+	ret = ad2s1210_setup_gpios(st);
+	if (ret < 0)
+		return ret;
+
 	indio_dev->info = &ad2s1210_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->channels = ad2s1210_channels;