[OPW,kernel,1/2] Staging: btmtk_usb: Fix Sparse Warning of incorrect casting
diff mbox

Message ID 0396e074bd30ae8f9f11f1518ac7c4881961286f.1381739782.git.rashika.kheria@gmail.com
State Changes Requested
Headers show

Commit Message

Rashika Oct. 14, 2013, 8:46 a.m. UTC
This patch fixes the following Sparse Warnings in btmtk_usb.c:

drivers/staging/btmtk_usb/btmtk_usb.c:110:16: warning: cast to restricted __le32
drivers/staging/btmtk_usb/btmtk_usb.c:299:23: warning: cast to restricted __le16

Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
---
 drivers/staging/btmtk_usb/btmtk_usb.c |    4 ----
 1 file changed, 4 deletions(-)

Comments

Josh Triplett Oct. 14, 2013, 3:10 p.m. UTC | #1
On Mon, Oct 14, 2013 at 02:16:43PM +0530, Rashika Kheria wrote:
> This patch fixes the following Sparse Warnings in btmtk_usb.c:
> 
> drivers/staging/btmtk_usb/btmtk_usb.c:110:16: warning: cast to restricted __le32
> drivers/staging/btmtk_usb/btmtk_usb.c:299:23: warning: cast to restricted __le16
[...]
> --- a/drivers/staging/btmtk_usb/btmtk_usb.c
> +++ b/drivers/staging/btmtk_usb/btmtk_usb.c
> @@ -107,8 +107,6 @@ static int btmtk_usb_io_read32(struct btmtk_usb_data *data, u32 reg, u32 *val)
>  
>  	memmove(val, data->io_buf, 4);
>  
> -	*val = le32_to_cpu(*val);
> -
>  	if (ret > 0)
>  		ret = 0;
>  
> @@ -296,8 +294,6 @@ static u16 btmtk_usb_get_crc(struct btmtk_usb_data *data)
>  
>  		memmove(&crc, data->io_buf, 2);
>  
> -		crc = le16_to_cpu(crc);
> -

This isn't the right way to fix endian warnings like this.  The two
calls look correct, and removing them would be wrong.  Sparse is
complaining about calling le32_to_cpu on something that isn't an le32_t;
it wants u32 used only for native-endian data, and le32 used for
little-endian data.

You'll need to split each of these variables into two separate
variables: one to hold the little-endian version apparently provided as
in put through io_buf, and one to hold the native-endian version used
later in the function.  For example, you could make the u16 crc be an
le16 crc_le, declare a separate local u16 crc, and then write:

crc = le16_to_cpu(crc_le)

You could then use crc in the remainder of the function as currently
done.

Does that make sense?

- Josh Triplett
Josh Triplett Oct. 14, 2013, 3:15 p.m. UTC | #2
On Mon, Oct 14, 2013 at 08:10:49AM -0700, Josh Triplett wrote:
> On Mon, Oct 14, 2013 at 02:16:43PM +0530, Rashika Kheria wrote:
> > This patch fixes the following Sparse Warnings in btmtk_usb.c:
> > 
> > drivers/staging/btmtk_usb/btmtk_usb.c:110:16: warning: cast to restricted __le32
> > drivers/staging/btmtk_usb/btmtk_usb.c:299:23: warning: cast to restricted __le16
> [...]
> > --- a/drivers/staging/btmtk_usb/btmtk_usb.c
> > +++ b/drivers/staging/btmtk_usb/btmtk_usb.c
> > @@ -107,8 +107,6 @@ static int btmtk_usb_io_read32(struct btmtk_usb_data *data, u32 reg, u32 *val)
> >  
> >  	memmove(val, data->io_buf, 4);
> >  
> > -	*val = le32_to_cpu(*val);
> > -
> >  	if (ret > 0)
> >  		ret = 0;
> >  
> > @@ -296,8 +294,6 @@ static u16 btmtk_usb_get_crc(struct btmtk_usb_data *data)
> >  
> >  		memmove(&crc, data->io_buf, 2);
> >  
> > -		crc = le16_to_cpu(crc);
> > -
> 
> This isn't the right way to fix endian warnings like this.  The two
> calls look correct, and removing them would be wrong.  Sparse is
> complaining about calling le32_to_cpu on something that isn't an le32_t;

Sorry, that should have said "le32", as in the next line; too much time
in userspace. :)

> it wants u32 used only for native-endian data, and le32 used for
> little-endian data.
> 
> You'll need to split each of these variables into two separate
> variables: one to hold the little-endian version apparently provided as
> in put through io_buf, and one to hold the native-endian version used
> later in the function.  For example, you could make the u16 crc be an
> le16 crc_le, declare a separate local u16 crc, and then write:
> 
> crc = le16_to_cpu(crc_le)
> 
> You could then use crc in the remainder of the function as currently
> done.
> 
> Does that make sense?
> 
> - Josh Triplett
Rashika Oct. 14, 2013, 3:38 p.m. UTC | #3
On Mon, Oct 14, 2013 at 8:45 PM, Josh Triplett <josh@joshtriplett.org>wrote:

> On Mon, Oct 14, 2013 at 08:10:49AM -0700, Josh Triplett wrote:
> > On Mon, Oct 14, 2013 at 02:16:43PM +0530, Rashika Kheria wrote:
> > > This patch fixes the following Sparse Warnings in btmtk_usb.c:
> > >
> > > drivers/staging/btmtk_usb/btmtk_usb.c:110:16: warning: cast to
> restricted __le32
> > > drivers/staging/btmtk_usb/btmtk_usb.c:299:23: warning: cast to
> restricted __le16
> > [...]
> > > --- a/drivers/staging/btmtk_usb/btmtk_usb.c
> > > +++ b/drivers/staging/btmtk_usb/btmtk_usb.c
> > > @@ -107,8 +107,6 @@ static int btmtk_usb_io_read32(struct
> btmtk_usb_data *data, u32 reg, u32 *val)
> > >
> > >     memmove(val, data->io_buf, 4);
> > >
> > > -   *val = le32_to_cpu(*val);
> > > -
> > >     if (ret > 0)
> > >             ret = 0;
> > >
> > > @@ -296,8 +294,6 @@ static u16 btmtk_usb_get_crc(struct btmtk_usb_data
> *data)
> > >
> > >             memmove(&crc, data->io_buf, 2);
> > >
> > > -           crc = le16_to_cpu(crc);
> > > -
> >
> > This isn't the right way to fix endian warnings like this.  The two
> > calls look correct, and removing them would be wrong.  Sparse is
> > complaining about calling le32_to_cpu on something that isn't an le32_t;
>
> Sorry, that should have said "le32", as in the next line; too much time
> in userspace. :)
>
> > it wants u32 used only for native-endian data, and le32 used for
> > little-endian data.
> >
> > You'll need to split each of these variables into two separate
> > variables: one to hold the little-endian version apparently provided as
> > in put through io_buf, and one to hold the native-endian version used
> > later in the function.  For example, you could make the u16 crc be an
> > le16 crc_le, declare a separate local u16 crc, and then write:
> >
> > crc = le16_to_cpu(crc_le)
> >
> > You could then use crc in the remainder of the function as currently
> > done.
> >
> > Does that make sense?
> >
> > - Josh Triplett
>

I will make the required changes and resend the patch.


Thanks
Rashika Oct. 14, 2013, 3:59 p.m. UTC | #4
On Mon, Oct 14, 2013 at 9:08 PM, Rashika Kheria <rashika.kheria@gmail.com>wrote:

>
>
>
> On Mon, Oct 14, 2013 at 8:45 PM, Josh Triplett <josh@joshtriplett.org>wrote:
>
>> On Mon, Oct 14, 2013 at 08:10:49AM -0700, Josh Triplett wrote:
>> > On Mon, Oct 14, 2013 at 02:16:43PM +0530, Rashika Kheria wrote:
>> > > This patch fixes the following Sparse Warnings in btmtk_usb.c:
>> > >
>> > > drivers/staging/btmtk_usb/btmtk_usb.c:110:16: warning: cast to
>> restricted __le32
>> > > drivers/staging/btmtk_usb/btmtk_usb.c:299:23: warning: cast to
>> restricted __le16
>> > [...]
>> > > --- a/drivers/staging/btmtk_usb/btmtk_usb.c
>> > > +++ b/drivers/staging/btmtk_usb/btmtk_usb.c
>> > > @@ -107,8 +107,6 @@ static int btmtk_usb_io_read32(struct
>> btmtk_usb_data *data, u32 reg, u32 *val)
>> > >
>> > >     memmove(val, data->io_buf, 4);
>> > >
>> > > -   *val = le32_to_cpu(*val);
>> > > -
>> > >     if (ret > 0)
>> > >             ret = 0;
>> > >
>> > > @@ -296,8 +294,6 @@ static u16 btmtk_usb_get_crc(struct
>> btmtk_usb_data *data)
>> > >
>> > >             memmove(&crc, data->io_buf, 2);
>> > >
>> > > -           crc = le16_to_cpu(crc);
>> > > -
>> >
>> > This isn't the right way to fix endian warnings like this.  The two
>> > calls look correct, and removing them would be wrong.  Sparse is
>> > complaining about calling le32_to_cpu on something that isn't an le32_t;
>>
>> Sorry, that should have said "le32", as in the next line; too much time
>> in userspace. :)
>>
>> > it wants u32 used only for native-endian data, and le32 used for
>> > little-endian data.
>> >
>> > You'll need to split each of these variables into two separate
>> > variables: one to hold the little-endian version apparently provided as
>> > in put through io_buf, and one to hold the native-endian version used
>> > later in the function.  For example, you could make the u16 crc be an
>> > le16 crc_le, declare a separate local u16 crc, and then write:
>> >
>> > crc = le16_to_cpu(crc_le)
>> >
>> > You could then use crc in the remainder of the function as currently
>> > done.
>> >
>> > Does that make sense?
>> >
>>
> Hi Josh,

I am confused whether to replace crc used above the conversion (as in
memmove() ) to crc_le?


Will it not change the interpretation of the code?


Thanks,

Patch
diff mbox

diff --git a/drivers/staging/btmtk_usb/btmtk_usb.c b/drivers/staging/btmtk_usb/btmtk_usb.c
index 1b0f993..e17efd5 100644
--- a/drivers/staging/btmtk_usb/btmtk_usb.c
+++ b/drivers/staging/btmtk_usb/btmtk_usb.c
@@ -107,8 +107,6 @@  static int btmtk_usb_io_read32(struct btmtk_usb_data *data, u32 reg, u32 *val)
 
 	memmove(val, data->io_buf, 4);
 
-	*val = le32_to_cpu(*val);
-
 	if (ret > 0)
 		ret = 0;
 
@@ -296,8 +294,6 @@  static u16 btmtk_usb_get_crc(struct btmtk_usb_data *data)
 
 		memmove(&crc, data->io_buf, 2);
 
-		crc = le16_to_cpu(crc);
-
 		if (crc != 0xFFFF)
 			break;