linux-kernel - Re: [PATCH 04/12] iio: adc: max9611: Fix misuse of GENMASK macro

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190714143756.2hqmncnbx7nftj7w@uno.localdomain>
Date:   Sun, 14 Jul 2019 16:37:56 +0200
From:   Jacopo Mondi <jacopo@...ndi.org>
To:     Joe Perches <joe@...ches.com>
Cc:     Jonathan Cameron <jic23@...nel.org>,
        Jacopo Mondi <jacopo+renesas@...ndi.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, Hartmut Knaack <knaack.h@....de>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        linux-iio@...r.kernel.org
Subject: Re: [PATCH 04/12] iio: adc: max9611: Fix misuse of GENMASK macro

Hi Joe, Jonathan,

On Sun, Jul 14, 2019 at 05:19:32AM -0700, Joe Perches wrote:
> On Sun, 2019-07-14 at 12:54 +0100, Jonathan Cameron wrote:
> > On Tue,  9 Jul 2019 22:04:17 -0700
> > Joe Perches <joe@...ches.com> wrote:
> >
> > > Arguments are supposed to be ordered high then low.
> > >
> > > Signed-off-by: Joe Perches <joe@...ches.com>
> >
> > Applied to the fixes-togreg branch of iio.git and marked for
> > stable etc.
>
> This mask is used in an init function called from a probe.
>
> I don't have this hardware but it looks as if it could
> never have worked so I doubt the driver and the hardware
> have ever been tested.
>

Ups, this is embarassing! Thanks for noticing

I actually tested the device before sending the driver of course

> Does anyone have this device in actual use?
>

The driver is currently in use in Renesas Gen3 Salvator boards:
arch/arm64/boot/dts/renesas/salvator-common.dtsi:466

I might need some time before I can actually test and tell what's
happening though. It might work by pure chance. Fortunately the mask
is only used for validation of the temperature reading at probe time,
and I can tell this passes (we would have noticed otherwise, Salvator
is one of the reference Gen3 platforms for upstream development).

As said I might need some time before I can test this, but the change
is indeed correct.

Thanks
   j

>
> 	regval = ret & MAX9611_TEMP_MASK;
>
> 	if ((regval > MAX9611_TEMP_MAX_POS &&
> 	     regval < MAX9611_TEMP_MIN_NEG) ||
> 	     regval > MAX9611_TEMP_MAX_NEG) {
> 		dev_err(max9611->dev,
> 			"Invalid value received from ADC 0x%4x: aborting\n",
> 			regval);
> 		return -EIO;
> 	}
>
>
> > Thanks,
> >
> > Jonathan
> >
> > > ---
> > >  drivers/iio/adc/max9611.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iio/adc/max9611.c b/drivers/iio/adc/max9611.c
> > > index 917223d5ff5b..0e3c6529fc4c 100644
> > > --- a/drivers/iio/adc/max9611.c
> > > +++ b/drivers/iio/adc/max9611.c
> > > @@ -83,7 +83,7 @@
> > >  #define MAX9611_TEMP_MAX_POS		0x7f80
> > >  #define MAX9611_TEMP_MAX_NEG		0xff80
> > >  #define MAX9611_TEMP_MIN_NEG		0xd980
> > > -#define MAX9611_TEMP_MASK		GENMASK(7, 15)
> > > +#define MAX9611_TEMP_MASK		GENMASK(15, 7)
> > >  #define MAX9611_TEMP_SHIFT		0x07
> > >  #define MAX9611_TEMP_RAW(_r)		((_r) >> MAX9611_TEMP_SHIFT)
> > >  #define MAX9611_TEMP_SCALE_NUM		1000000
>

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)