Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] iio:adc:ti-adc084s021: Endian casting tidy ups.
@ 2019-10-13  9:47 jic23
  2019-11-04 15:08 ` Ardelean, Alexandru
  0 siblings, 1 reply; 4+ messages in thread
From: jic23 @ 2019-10-13  9:47 UTC (permalink / raw)
  To: linux-iio; +Cc: Jonathan Cameron, Mårten Lindahl

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Highlighted by sparse:
CHECK   drivers/iio/adc/ti-adc084s021.c
drivers/iio/adc/ti-adc084s021.c:79:26: warning: incorrect type in assignment (different base types)
drivers/iio/adc/ti-adc084s021.c:79:26:    expected unsigned short [unsigned] [short] [usertype] <noident>
drivers/iio/adc/ti-adc084s021.c:79:26:    got restricted __be16 <noident>
drivers/iio/adc/ti-adc084s021.c:110:24: warning: cast to restricted __be16
drivers/iio/adc/ti-adc084s021.c:110:24: warning: cast to restricted __be16
drivers/iio/adc/ti-adc084s021.c:110:24: warning: cast to restricted __be16
drivers/iio/adc/ti-adc084s021.c:110:24: warning: cast to restricted __be16

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Mårten Lindahl <martenli@axis.com>
---
 drivers/iio/adc/ti-adc084s021.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/ti-adc084s021.c b/drivers/iio/adc/ti-adc084s021.c
index bdedf456ee05..42966f2eb3d8 100644
--- a/drivers/iio/adc/ti-adc084s021.c
+++ b/drivers/iio/adc/ti-adc084s021.c
@@ -68,7 +68,7 @@ static int adc084s021_adc_conversion(struct adc084s021 *adc, void *data)
 {
 	int n_words = (adc->spi_trans.len >> 1) - 1; /* Discard first word */
 	int ret, i = 0;
-	u16 *p = data;
+	__be16 *p = data;
 
 	/* Do the transfer */
 	ret = spi_sync(adc->spi, &adc->message);
@@ -87,6 +87,7 @@ static int adc084s021_read_raw(struct iio_dev *indio_dev,
 {
 	struct adc084s021 *adc = iio_priv(indio_dev);
 	int ret;
+	__be16 value;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
@@ -101,13 +102,13 @@ static int adc084s021_read_raw(struct iio_dev *indio_dev,
 		}
 
 		adc->tx_buf[0] = channel->channel << 3;
-		ret = adc084s021_adc_conversion(adc, val);
+		ret = adc084s021_adc_conversion(adc, &value);
 		iio_device_release_direct_mode(indio_dev);
 		regulator_disable(adc->reg);
 		if (ret < 0)
 			return ret;
 
-		*val = be16_to_cpu(*val);
+		*val = be16_to_cpu(value);
 		*val = (*val >> channel->scan_type.shift) & 0xff;
 
 		return IIO_VAL_INT;
-- 
2.23.0


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] iio:adc:ti-adc084s021: Endian casting tidy ups.
  2019-10-13  9:47 [PATCH] iio:adc:ti-adc084s021: Endian casting tidy ups jic23
@ 2019-11-04 15:08 ` Ardelean, Alexandru
  2019-11-09 12:11   ` Jonathan Cameron
  0 siblings, 1 reply; 4+ messages in thread
From: Ardelean, Alexandru @ 2019-11-04 15:08 UTC (permalink / raw)
  To: jic23, linux-iio; +Cc: Jonathan.Cameron, martenli

On Sun, 2019-10-13 at 10:47 +0100, jic23@kernel.org wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Highlighted by sparse:
> CHECK   drivers/iio/adc/ti-adc084s021.c
> drivers/iio/adc/ti-adc084s021.c:79:26: warning: incorrect type in
> assignment (different base types)
> drivers/iio/adc/ti-adc084s021.c:79:26:    expected unsigned short
> [unsigned] [short] [usertype] <noident>
> drivers/iio/adc/ti-adc084s021.c:79:26:    got restricted __be16 <noident>
> drivers/iio/adc/ti-adc084s021.c:110:24: warning: cast to restricted
> __be16
> drivers/iio/adc/ti-adc084s021.c:110:24: warning: cast to restricted
> __be16
> drivers/iio/adc/ti-adc084s021.c:110:24: warning: cast to restricted
> __be16
> drivers/iio/adc/ti-adc084s021.c:110:24: warning: cast to restricted
> __be16
> 

This one looks a bit tricky.
And looks like it could use a bit more cleanup than this.
Otherwise sparse may come along and complain about more stuff.

One thing that would be good, would be to change:

int adc084s021_adc_conversion(struct adc084s021 *adc, void *data)

to

int adc084s021_adc_conversion(struct adc084s021 *adc, __be16 *data, int
buf_size)   [1]


> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Mårten Lindahl <martenli@axis.com>
> ---
>  drivers/iio/adc/ti-adc084s021.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-adc084s021.c b/drivers/iio/adc/ti-
> adc084s021.c
> index bdedf456ee05..42966f2eb3d8 100644
> --- a/drivers/iio/adc/ti-adc084s021.c
> +++ b/drivers/iio/adc/ti-adc084s021.c
> @@ -68,7 +68,7 @@ static int adc084s021_adc_conversion(struct adc084s021
> *adc, void *data)
>  {
>  	int n_words = (adc->spi_trans.len >> 1) - 1; /* Discard first word
> */
>  	int ret, i = 0;
> -	u16 *p = data;
> +	__be16 *p = data;
>  
>  	/* Do the transfer */
>  	ret = spi_sync(adc->spi, &adc->message);
> @@ -87,6 +87,7 @@ static int adc084s021_read_raw(struct iio_dev
> *indio_dev,
>  {
>  	struct adc084s021 *adc = iio_priv(indio_dev);
>  	int ret;
> +	__be16 value;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> @@ -101,13 +102,13 @@ static int adc084s021_read_raw(struct iio_dev
> *indio_dev,
>  		}
>  
>  		adc->tx_buf[0] = channel->channel << 3;
> -		ret = adc084s021_adc_conversion(adc, val);
> +		ret = adc084s021_adc_conversion(adc, &value);

Following [1], this could be called with  "adc084s021_adc_conversion(adc,
&value, 1)" to make sure it's not doing any stack corruption. I can't tell
if this is doing any or not; the code is a bit fuzzy to me.

The neat part is that memcpy() could be used to then access the data on
rx_buf.


>  		iio_device_release_direct_mode(indio_dev);
>  		regulator_disable(adc->reg);
>  		if (ret < 0)
>  			return ret;
>  
> -		*val = be16_to_cpu(*val);
> +		*val = be16_to_cpu(value);
>  		*val = (*val >> channel->scan_type.shift) & 0xff;
>  
>  		return IIO_VAL_INT;

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] iio:adc:ti-adc084s021: Endian casting tidy ups.
  2019-11-04 15:08 ` Ardelean, Alexandru
@ 2019-11-09 12:11   ` Jonathan Cameron
  2019-11-11  9:22     ` Ardelean, Alexandru
  0 siblings, 1 reply; 4+ messages in thread
From: Jonathan Cameron @ 2019-11-09 12:11 UTC (permalink / raw)
  To: Ardelean, Alexandru; +Cc: linux-iio, Jonathan.Cameron, martenli

On Mon, 4 Nov 2019 15:08:12 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Sun, 2019-10-13 at 10:47 +0100, jic23@kernel.org wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Highlighted by sparse:
> > CHECK   drivers/iio/adc/ti-adc084s021.c
> > drivers/iio/adc/ti-adc084s021.c:79:26: warning: incorrect type in
> > assignment (different base types)
> > drivers/iio/adc/ti-adc084s021.c:79:26:    expected unsigned short
> > [unsigned] [short] [usertype] <noident>
> > drivers/iio/adc/ti-adc084s021.c:79:26:    got restricted __be16 <noident>
> > drivers/iio/adc/ti-adc084s021.c:110:24: warning: cast to restricted
> > __be16
> > drivers/iio/adc/ti-adc084s021.c:110:24: warning: cast to restricted
> > __be16
> > drivers/iio/adc/ti-adc084s021.c:110:24: warning: cast to restricted
> > __be16
> > drivers/iio/adc/ti-adc084s021.c:110:24: warning: cast to restricted
> > __be16
> >   
> 
Thanks for looking at this. I'd missed entirely that a void *
was hiding some more nastiness!

> This one looks a bit tricky.
> And looks like it could use a bit more cleanup than this.
> Otherwise sparse may come along and complain about more stuff.
> 
> One thing that would be good, would be to change:
> 
> int adc084s021_adc_conversion(struct adc084s021 *adc, void *data)
> 
> to
> 
> int adc084s021_adc_conversion(struct adc084s021 *adc, __be16 *data, int
> buf_size)   [1]
> 
> 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Mårten Lindahl <martenli@axis.com>
> > ---
> >  drivers/iio/adc/ti-adc084s021.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ti-adc084s021.c b/drivers/iio/adc/ti-
> > adc084s021.c
> > index bdedf456ee05..42966f2eb3d8 100644
> > --- a/drivers/iio/adc/ti-adc084s021.c
> > +++ b/drivers/iio/adc/ti-adc084s021.c
> > @@ -68,7 +68,7 @@ static int adc084s021_adc_conversion(struct adc084s021
> > *adc, void *data)
> >  {
> >  	int n_words = (adc->spi_trans.len >> 1) - 1; /* Discard first word
> > */
> >  	int ret, i = 0;
> > -	u16 *p = data;
> > +	__be16 *p = data;
> >  
> >  	/* Do the transfer */
> >  	ret = spi_sync(adc->spi, &adc->message);
> > @@ -87,6 +87,7 @@ static int adc084s021_read_raw(struct iio_dev
> > *indio_dev,
> >  {
> >  	struct adc084s021 *adc = iio_priv(indio_dev);
> >  	int ret;
> > +	__be16 value;
> >  
> >  	switch (mask) {
> >  	case IIO_CHAN_INFO_RAW:
> > @@ -101,13 +102,13 @@ static int adc084s021_read_raw(struct iio_dev
> > *indio_dev,
> >  		}
> >  
> >  		adc->tx_buf[0] = channel->channel << 3;
> > -		ret = adc084s021_adc_conversion(adc, val);
> > +		ret = adc084s021_adc_conversion(adc, &value);  
> 
> Following [1], this could be called with  "adc084s021_adc_conversion(adc,
> &value, 1)" to make sure it's not doing any stack corruption. I can't tell
> if this is doing any or not; the code is a bit fuzzy to me.

I'm fairly sure current code is safe as IIO_CHAN_INFO_RAW reads occur
with protection against buffered mode and when not in buffered mode the
magic length is 4 which is divided by 2 and has 1 subtracted giving a
safe value of 1.  This dance is ensure there is only one place where
the length is recorded and avoid recomputing it in *_buffer_trigger_handler.
We could stash it in the private data though for easier to read code.

Agreed that the type change to __be16 * definitely makes sense though.
Will respin with that.

> 
> The neat part is that memcpy() could be used to then access the data on
> rx_buf.

We could do that, but as it's not part of the fix really I'd rather leave
that for another day.  Not sure anything stops us doing this whilst doing
this tidy up even without passing in the size.

Thanks,

Jonathan

> 
> 
> >  		iio_device_release_direct_mode(indio_dev);
> >  		regulator_disable(adc->reg);
> >  		if (ret < 0)
> >  			return ret;
> >  
> > -		*val = be16_to_cpu(*val);
> > +		*val = be16_to_cpu(value);
> >  		*val = (*val >> channel->scan_type.shift) & 0xff;
> >  
> >  		return IIO_VAL_INT;  


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] iio:adc:ti-adc084s021: Endian casting tidy ups.
  2019-11-09 12:11   ` Jonathan Cameron
@ 2019-11-11  9:22     ` Ardelean, Alexandru
  0 siblings, 0 replies; 4+ messages in thread
From: Ardelean, Alexandru @ 2019-11-11  9:22 UTC (permalink / raw)
  To: jic23; +Cc: Jonathan.Cameron, linux-iio, martenli

On Sat, 2019-11-09 at 12:11 +0000, Jonathan Cameron wrote:
> [External]
> 
> On Mon, 4 Nov 2019 15:08:12 +0000
> "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
> 
> > On Sun, 2019-10-13 at 10:47 +0100, jic23@kernel.org wrote:
> > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > 
> > > Highlighted by sparse:
> > > CHECK   drivers/iio/adc/ti-adc084s021.c
> > > drivers/iio/adc/ti-adc084s021.c:79:26: warning: incorrect type in
> > > assignment (different base types)
> > > drivers/iio/adc/ti-adc084s021.c:79:26:    expected unsigned short
> > > [unsigned] [short] [usertype] <noident>
> > > drivers/iio/adc/ti-adc084s021.c:79:26:    got restricted __be16
> > > <noident>
> > > drivers/iio/adc/ti-adc084s021.c:110:24: warning: cast to restricted
> > > __be16
> > > drivers/iio/adc/ti-adc084s021.c:110:24: warning: cast to restricted
> > > __be16
> > > drivers/iio/adc/ti-adc084s021.c:110:24: warning: cast to restricted
> > > __be16
> > > drivers/iio/adc/ti-adc084s021.c:110:24: warning: cast to restricted
> > > __be16
> > >   
> Thanks for looking at this. I'd missed entirely that a void *
> was hiding some more nastiness!
> 
> > This one looks a bit tricky.
> > And looks like it could use a bit more cleanup than this.
> > Otherwise sparse may come along and complain about more stuff.
> > 
> > One thing that would be good, would be to change:
> > 
> > int adc084s021_adc_conversion(struct adc084s021 *adc, void *data)
> > 
> > to
> > 
> > int adc084s021_adc_conversion(struct adc084s021 *adc, __be16 *data, int
> > buf_size)   [1]
> > 
> > 
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > Cc: Mårten Lindahl <martenli@axis.com>
> > > ---
> > >  drivers/iio/adc/ti-adc084s021.c | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/ti-adc084s021.c b/drivers/iio/adc/ti-
> > > adc084s021.c
> > > index bdedf456ee05..42966f2eb3d8 100644
> > > --- a/drivers/iio/adc/ti-adc084s021.c
> > > +++ b/drivers/iio/adc/ti-adc084s021.c
> > > @@ -68,7 +68,7 @@ static int adc084s021_adc_conversion(struct
> > > adc084s021
> > > *adc, void *data)
> > >  {
> > >  	int n_words = (adc->spi_trans.len >> 1) - 1; /* Discard first word
> > > */
> > >  	int ret, i = 0;
> > > -	u16 *p = data;
> > > +	__be16 *p = data;
> > >  
> > >  	/* Do the transfer */
> > >  	ret = spi_sync(adc->spi, &adc->message);
> > > @@ -87,6 +87,7 @@ static int adc084s021_read_raw(struct iio_dev
> > > *indio_dev,
> > >  {
> > >  	struct adc084s021 *adc = iio_priv(indio_dev);
> > >  	int ret;
> > > +	__be16 value;
> > >  
> > >  	switch (mask) {
> > >  	case IIO_CHAN_INFO_RAW:
> > > @@ -101,13 +102,13 @@ static int adc084s021_read_raw(struct iio_dev
> > > *indio_dev,
> > >  		}
> > >  
> > >  		adc->tx_buf[0] = channel->channel << 3;
> > > -		ret = adc084s021_adc_conversion(adc, val);
> > > +		ret = adc084s021_adc_conversion(adc, &value);  
> > 
> > Following [1], this could be called
> > with  "adc084s021_adc_conversion(adc,
> > &value, 1)" to make sure it's not doing any stack corruption. I can't
> > tell
> > if this is doing any or not; the code is a bit fuzzy to me.
> 
> I'm fairly sure current code is safe as IIO_CHAN_INFO_RAW reads occur
> with protection against buffered mode and when not in buffered mode the
> magic length is 4 which is divided by 2 and has 1 subtracted giving a
> safe value of 1.  This dance is ensure there is only one place where
> the length is recorded and avoid recomputing it in
> *_buffer_trigger_handler.
> We could stash it in the private data though for easier to read code.

No idea what's best here.
I am mostly interested in getting the sparse checks up-n-running, so that
we don't have another dance with Greg [as the ones with the ADIS patches].

> 
> Agreed that the type change to __be16 * definitely makes sense though.
> Will respin with that.
> 
> > The neat part is that memcpy() could be used to then access the data on
> > rx_buf.
> 
> We could do that, but as it's not part of the fix really I'd rather leave
> that for another day.  Not sure anything stops us doing this whilst doing
> this tidy up even without passing in the size.

Let's leave things here as-is for now.
sparse checks [or any new checks we're adding] feel more important right
now.

Thanks
Alex

> 
> Thanks,
> 
> Jonathan
> 
> > 
> > >  		iio_device_release_direct_mode(indio_dev);
> > >  		regulator_disable(adc->reg);
> > >  		if (ret < 0)
> > >  			return ret;
> > >  
> > > -		*val = be16_to_cpu(*val);
> > > +		*val = be16_to_cpu(value);
> > >  		*val = (*val >> channel->scan_type.shift) & 0xff;
> > >  
> > >  		return IIO_VAL_INT;  

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-13  9:47 [PATCH] iio:adc:ti-adc084s021: Endian casting tidy ups jic23
2019-11-04 15:08 ` Ardelean, Alexandru
2019-11-09 12:11   ` Jonathan Cameron
2019-11-11  9:22     ` Ardelean, Alexandru

Linux-IIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iio/0 linux-iio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iio linux-iio/ https://lore.kernel.org/linux-iio \
		linux-iio@vger.kernel.org
	public-inbox-index linux-iio

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-iio


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git