diff mbox series

[2/2] staging: iio: cdc: ad7746: avoid overwrite of num_channels

Message ID 09e65d3a235febfc4c3ee172b573ba8c9cde94b8.1620766020.git.lucas.p.stankus@gmail.com (mailing list archive)
State New
Headers show
Series staging: iio: cdc: ad7746: initial effort to move out of staging | expand

Commit Message

Lucas Stankus May 11, 2021, 8:54 p.m. UTC
AD7745 devices don't have the CIN2 pins and therefore can't handle related
channels. Forcing the number of AD7746 channels may lead to enabling more
channels than what the hardware actually supports.
Avoid num_channels being overwritten after first assignment.

Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
---
 drivers/staging/iio/cdc/ad7746.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Alexandru Ardelean May 12, 2021, 5:20 p.m. UTC | #1
On Tue, May 11, 2021 at 11:55 PM Lucas Stankus
<lucas.p.stankus@gmail.com> wrote:
>
> AD7745 devices don't have the CIN2 pins and therefore can't handle related
> channels. Forcing the number of AD7746 channels may lead to enabling more
> channels than what the hardware actually supports.
> Avoid num_channels being overwritten after first assignment.
>
> Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
> ---
>  drivers/staging/iio/cdc/ad7746.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> index e03d010b2f4c..9e0da43b2871 100644
> --- a/drivers/staging/iio/cdc/ad7746.c
> +++ b/drivers/staging/iio/cdc/ad7746.c
> @@ -693,7 +693,6 @@ static int ad7746_probe(struct i2c_client *client,
>                 indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);
>         else
>                 indio_dev->num_channels =  ARRAY_SIZE(ad7746_channels) - 2;
> -       indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);

ohh; good catch

this falls into the category of a fix, so a Fixes tag is required;
this looks so old, that i did not bother tracking it before
83e416f458d53  [which is 2011]

so, maybe something like:

Fixes: 83e416f458d53 ("staging: iio: adc: Replace, rewrite ad7745 from
scratch.")

>         indio_dev->modes = INDIO_DIRECT_MODE;
>
>         if (pdata) {
> --
> 2.31.1
>
Jonathan Cameron May 13, 2021, 3:52 p.m. UTC | #2
On Wed, 12 May 2021 20:20:02 +0300
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Tue, May 11, 2021 at 11:55 PM Lucas Stankus
> <lucas.p.stankus@gmail.com> wrote:
> >
> > AD7745 devices don't have the CIN2 pins and therefore can't handle related
> > channels. Forcing the number of AD7746 channels may lead to enabling more
> > channels than what the hardware actually supports.
> > Avoid num_channels being overwritten after first assignment.
> >
> > Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
> > ---
> >  drivers/staging/iio/cdc/ad7746.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> > index e03d010b2f4c..9e0da43b2871 100644
> > --- a/drivers/staging/iio/cdc/ad7746.c
> > +++ b/drivers/staging/iio/cdc/ad7746.c
> > @@ -693,7 +693,6 @@ static int ad7746_probe(struct i2c_client *client,
> >                 indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);
> >         else
> >                 indio_dev->num_channels =  ARRAY_SIZE(ad7746_channels) - 2;
> > -       indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);  
> 
> ohh; good catch
> 
> this falls into the category of a fix, so a Fixes tag is required;
> this looks so old, that i did not bother tracking it before
> 83e416f458d53  [which is 2011]
> 
> so, maybe something like:
> 
> Fixes: 83e416f458d53 ("staging: iio: adc: Replace, rewrite ad7745 from
> scratch.")

ouch.  Given I was queuing up some fixes I've added this one to the fixes-togreg
branch of iio.git and marked it for stable.

So drop this one from your v2 series with the changes requested in patch 1.

Thanks,

Jonathan

> 
> >         indio_dev->modes = INDIO_DIRECT_MODE;
> >
> >         if (pdata) {
> > --
> > 2.31.1
> >
Lucas Stankus May 18, 2021, 12:49 a.m. UTC | #3
On Thu, May 13, 2021 at 12:51 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Wed, 12 May 2021 20:20:02 +0300
> Alexandru Ardelean <ardeleanalex@gmail.com> wrote:
>
> > On Tue, May 11, 2021 at 11:55 PM Lucas Stankus
> > <lucas.p.stankus@gmail.com> wrote:
> > >
> > > AD7745 devices don't have the CIN2 pins and therefore can't handle related
> > > channels. Forcing the number of AD7746 channels may lead to enabling more
> > > channels than what the hardware actually supports.
> > > Avoid num_channels being overwritten after first assignment.
> > >
> > > Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
> > > ---
> > >  drivers/staging/iio/cdc/ad7746.c | 1 -
> > >  1 file changed, 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> > > index e03d010b2f4c..9e0da43b2871 100644
> > > --- a/drivers/staging/iio/cdc/ad7746.c
> > > +++ b/drivers/staging/iio/cdc/ad7746.c
> > > @@ -693,7 +693,6 @@ static int ad7746_probe(struct i2c_client *client,
> > >                 indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);
> > >         else
> > >                 indio_dev->num_channels =  ARRAY_SIZE(ad7746_channels) - 2;
> > > -       indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);
> >
> > ohh; good catch
> >
> > this falls into the category of a fix, so a Fixes tag is required;
> > this looks so old, that i did not bother tracking it before
> > 83e416f458d53  [which is 2011]
> >
> > so, maybe something like:
> >
> > Fixes: 83e416f458d53 ("staging: iio: adc: Replace, rewrite ad7745 from
> > scratch.")
>
> ouch.  Given I was queuing up some fixes I've added this one to the fixes-togreg
> branch of iio.git and marked it for stable.
>
> So drop this one from your v2 series with the changes requested in patch 1.
>
> Thanks,
>
> Jonathan

No problems, but I think I should've better checked the mailing list before
sending the patch, it would have avoided the noise.

Anyway, thanks for the review :)

>
> >
> > >         indio_dev->modes = INDIO_DIRECT_MODE;
> > >
> > >         if (pdata) {
> > > --
> > > 2.31.1
> > >
>
Lucas Stankus May 18, 2021, 12:55 a.m. UTC | #4
On Wed, May 12, 2021 at 2:20 PM Alexandru Ardelean
<ardeleanalex@gmail.com> wrote:
>
> On Tue, May 11, 2021 at 11:55 PM Lucas Stankus
> <lucas.p.stankus@gmail.com> wrote:
> >
> > AD7745 devices don't have the CIN2 pins and therefore can't handle related
> > channels. Forcing the number of AD7746 channels may lead to enabling more
> > channels than what the hardware actually supports.
> > Avoid num_channels being overwritten after first assignment.
> >
> > Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
> > ---
> >  drivers/staging/iio/cdc/ad7746.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> > index e03d010b2f4c..9e0da43b2871 100644
> > --- a/drivers/staging/iio/cdc/ad7746.c
> > +++ b/drivers/staging/iio/cdc/ad7746.c
> > @@ -693,7 +693,6 @@ static int ad7746_probe(struct i2c_client *client,
> >                 indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);
> >         else
> >                 indio_dev->num_channels =  ARRAY_SIZE(ad7746_channels) - 2;
> > -       indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);
>
> ohh; good catch
>
> this falls into the category of a fix, so a Fixes tag is required;
> this looks so old, that i did not bother tracking it before
> 83e416f458d53  [which is 2011]

As Jonathan said, this bug was already fixed and the patch will be dropped,
but thank you for the review.

This was my first bug fix in the kernel, so sorry for the absence of a
Fixes tag, I'll make sure to add one next time.


>
> so, maybe something like:
>
> Fixes: 83e416f458d53 ("staging: iio: adc: Replace, rewrite ad7745 from
> scratch.")
>
> >         indio_dev->modes = INDIO_DIRECT_MODE;
> >
> >         if (pdata) {
> > --
> > 2.31.1
> >
Jonathan Cameron May 18, 2021, 12:08 p.m. UTC | #5
On Mon, 17 May 2021 21:55:20 -0300
Lucas Stankus <lucas.p.stankus@gmail.com> wrote:

> On Wed, May 12, 2021 at 2:20 PM Alexandru Ardelean
> <ardeleanalex@gmail.com> wrote:
> >
> > On Tue, May 11, 2021 at 11:55 PM Lucas Stankus
> > <lucas.p.stankus@gmail.com> wrote:  
> > >
> > > AD7745 devices don't have the CIN2 pins and therefore can't handle related
> > > channels. Forcing the number of AD7746 channels may lead to enabling more
> > > channels than what the hardware actually supports.
> > > Avoid num_channels being overwritten after first assignment.
> > >
> > > Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
> > > ---
> > >  drivers/staging/iio/cdc/ad7746.c | 1 -
> > >  1 file changed, 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> > > index e03d010b2f4c..9e0da43b2871 100644
> > > --- a/drivers/staging/iio/cdc/ad7746.c
> > > +++ b/drivers/staging/iio/cdc/ad7746.c
> > > @@ -693,7 +693,6 @@ static int ad7746_probe(struct i2c_client *client,
> > >                 indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);
> > >         else
> > >                 indio_dev->num_channels =  ARRAY_SIZE(ad7746_channels) - 2;
> > > -       indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);  
> >
> > ohh; good catch
> >
> > this falls into the category of a fix, so a Fixes tag is required;
> > this looks so old, that i did not bother tracking it before
> > 83e416f458d53  [which is 2011]  
> 
> As Jonathan said, this bug was already fixed and the patch will be dropped,
> but thank you for the review.
> 
> This was my first bug fix in the kernel, so sorry for the absence of a
> Fixes tag, I'll make sure to add one next time.
> 

Wasn't already fixed - I just applied this patch without PATCH 1/2
so now it is ;)

Jonathan

> 
> >
> > so, maybe something like:
> >
> > Fixes: 83e416f458d53 ("staging: iio: adc: Replace, rewrite ad7745 from
> > scratch.")
> >  
> > >         indio_dev->modes = INDIO_DIRECT_MODE;
> > >
> > >         if (pdata) {
> > > --
> > > 2.31.1
> > >
diff mbox series

Patch

diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
index e03d010b2f4c..9e0da43b2871 100644
--- a/drivers/staging/iio/cdc/ad7746.c
+++ b/drivers/staging/iio/cdc/ad7746.c
@@ -693,7 +693,6 @@  static int ad7746_probe(struct i2c_client *client,
 		indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);
 	else
 		indio_dev->num_channels =  ARRAY_SIZE(ad7746_channels) - 2;
-	indio_dev->num_channels = ARRAY_SIZE(ad7746_channels);
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	if (pdata) {