[OPW,kernel,2/2] Staging: btmtk_usb: Fix Sparse Warning of incorrect type in assignment
diff mbox

Message ID 014944d3a5978ac9a69124b9e39067d76d0afe4b.1381739782.git.rashika.kheria@gmail.com
State Changes Requested
Headers show

Commit Message

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

drivers/staging/btmtk_usb/btmtk_usb.c:676:39: warning: incorrect type in assignment (different base types)
drivers/staging/btmtk_usb/btmtk_usb.c:676:39:    expected unsigned int [unsigned] [usertype] packet_header
drivers/staging/btmtk_usb/btmtk_usb.c:676:39:    got restricted __le32 [usertype] <noident>

drivers/staging/btmtk_usb/btmtk_usb.c:736:31: warning: incorrect type in assignment (different base types)
drivers/staging/btmtk_usb/btmtk_usb.c:736:31:    expected unsigned int [unsigned] [addressable] [usertype] packet_header
drivers/staging/btmtk_usb/btmtk_usb.c:736:31:    got restricted __le32 [usertype] <noident>

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

Comments

Greg KH Oct. 14, 2013, 3:59 p.m. UTC | #1
On Mon, Oct 14, 2013 at 02:18:42PM +0530, Rashika Kheria wrote:
> This patch fixes the following Sparse Warnings in btmtk_usb.c:
> 
> drivers/staging/btmtk_usb/btmtk_usb.c:676:39: warning: incorrect type in assignment (different base types)
> drivers/staging/btmtk_usb/btmtk_usb.c:676:39:    expected unsigned int [unsigned] [usertype] packet_header
> drivers/staging/btmtk_usb/btmtk_usb.c:676:39:    got restricted __le32 [usertype] <noident>
> 
> drivers/staging/btmtk_usb/btmtk_usb.c:736:31: warning: incorrect type in assignment (different base types)
> drivers/staging/btmtk_usb/btmtk_usb.c:736:31:    expected unsigned int [unsigned] [addressable] [usertype] packet_header
> drivers/staging/btmtk_usb/btmtk_usb.c:736:31:    got restricted __le32 [usertype] <noident>
> 
> Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
> ---
>  drivers/staging/btmtk_usb/btmtk_usb.c |   16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/btmtk_usb/btmtk_usb.c b/drivers/staging/btmtk_usb/btmtk_usb.c
> index e17efd5..e03d805 100644
> --- a/drivers/staging/btmtk_usb/btmtk_usb.c
> +++ b/drivers/staging/btmtk_usb/btmtk_usb.c
> @@ -559,13 +559,13 @@ static int btmtk_usb_load_fw(struct btmtk_usb_data *data)
>  	struct urb *urb;
>  	void *buf;
>  	u32 cur_len = 0;
> -	u32 packet_header = 0;
> +	__le32 packet_header = 0;
>  	u32 value;
>  	u32 ilm_len = 0, dlm_len = 0;
>  	u16 fw_ver, build_ver;
>  	u32 loop = 0;
>  	dma_addr_t data_dma;
> -	int ret = 0, sent_len;
> +	int ret = 0, sent_len, sent_bytes;
>  	struct completion sent_to_mcu_done;
>  	unsigned int pipe;
>  	pipe = usb_sndbulkpipe(data->udev, data->bulk_tx_ep->bEndpointAddress);
> @@ -671,9 +671,9 @@ loadfw_protect:
>  		sent_len = min_t(s32, (ilm_len - cur_len), 14336);
>  
>  		if (sent_len > 0) {
> -			packet_header &= ~(0xffffffff);
> -			packet_header |= (sent_len << 16);
> -			packet_header = cpu_to_le32(packet_header);
> +			sent_bytes = sent_len << 16;  
> +			sent_bytes |= __le32_to_cpu(packet_header) & ~(0xffffffff);  
> +			packet_header = cpu_to_le32(sent_bytes);

Please always run your patches through scripts/checkpatch.pl so I don't
have to point out the obvious problems with it :)

Also, are you _sure_ you aren't changing the logic of what is happening
here?  Please be very careful when changing the logic of code that you
don't modify the original intention / logic.  As it is, I do not think
this change is correct, but please prove me wrong.

thanks,

greg k-h
Rashika Oct. 14, 2013, 4:10 p.m. UTC | #2
On Mon, Oct 14, 2013 at 9:29 PM, Greg KH <greg@kroah.com> wrote:

> On Mon, Oct 14, 2013 at 02:18:42PM +0530, Rashika Kheria wrote:
> > This patch fixes the following Sparse Warnings in btmtk_usb.c:
> >
> > drivers/staging/btmtk_usb/btmtk_usb.c:676:39: warning: incorrect type in
> assignment (different base types)
> > drivers/staging/btmtk_usb/btmtk_usb.c:676:39:    expected unsigned int
> [unsigned] [usertype] packet_header
> > drivers/staging/btmtk_usb/btmtk_usb.c:676:39:    got restricted __le32
> [usertype] <noident>
> >
> > drivers/staging/btmtk_usb/btmtk_usb.c:736:31: warning: incorrect type in
> assignment (different base types)
> > drivers/staging/btmtk_usb/btmtk_usb.c:736:31:    expected unsigned int
> [unsigned] [addressable] [usertype] packet_header
> > drivers/staging/btmtk_usb/btmtk_usb.c:736:31:    got restricted __le32
> [usertype] <noident>
> >
> > Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
> > ---
> >  drivers/staging/btmtk_usb/btmtk_usb.c |   16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/staging/btmtk_usb/btmtk_usb.c
> b/drivers/staging/btmtk_usb/btmtk_usb.c
> > index e17efd5..e03d805 100644
> > --- a/drivers/staging/btmtk_usb/btmtk_usb.c
> > +++ b/drivers/staging/btmtk_usb/btmtk_usb.c
> > @@ -559,13 +559,13 @@ static int btmtk_usb_load_fw(struct btmtk_usb_data
> *data)
> >       struct urb *urb;
> >       void *buf;
> >       u32 cur_len = 0;
> > -     u32 packet_header = 0;
> > +     __le32 packet_header = 0;
> >       u32 value;
> >       u32 ilm_len = 0, dlm_len = 0;
> >       u16 fw_ver, build_ver;
> >       u32 loop = 0;
> >       dma_addr_t data_dma;
> > -     int ret = 0, sent_len;
> > +     int ret = 0, sent_len, sent_bytes;
> >       struct completion sent_to_mcu_done;
> >       unsigned int pipe;
> >       pipe = usb_sndbulkpipe(data->udev,
> data->bulk_tx_ep->bEndpointAddress);
> > @@ -671,9 +671,9 @@ loadfw_protect:
> >               sent_len = min_t(s32, (ilm_len - cur_len), 14336);
> >
> >               if (sent_len > 0) {
> > -                     packet_header &= ~(0xffffffff);
> > -                     packet_header |= (sent_len << 16);
> > -                     packet_header = cpu_to_le32(packet_header);
> > +                     sent_bytes = sent_len << 16;
> > +                     sent_bytes |= __le32_to_cpu(packet_header) &
> ~(0xffffffff);
> > +                     packet_header = cpu_to_le32(sent_bytes);
>
> Please always run your patches through scripts/checkpatch.pl so I don't
> have to point out the obvious problems with it :)
>
> Also, are you _sure_ you aren't changing the logic of what is happening
> here?  Please be very careful when changing the logic of code that you
> don't modify the original intention / logic.  As it is, I do not think
> this change is correct, but please prove me wrong.
>
> thanks,
>
> greg k-h
>
> Hi Greg,

The logic used here is based on the following understanding -


 > -                     packet_header &= ~(0xffffffff);
> -                     packet_header |= (sent_len << 16);
> -                     packet_header = cpu_to_le32(packet_header);

packet_header's last 2 bytes are ANDed to 0 and then it is ORed with
(sent_len << 16) to store the last 2 bytes of sent_len as it's first 2
bytes. After that it is converted from u32 to le32. (The error because
packet_header must store values of type u32).

What I have done is:

> +                     sent_bytes = sent_len << 16;
> +                     sent_bytes |= __le32_to_cpu(packet_header) &
~(0xffffffff);
> +                     packet_header = cpu_to_le32(sent_bytes);

declared a new variable sent_bytes of type int which will contain the last
2 bytes of sent_len.  Now this value be ORed with the AND of
packet_header(type : u32) and mask(0xffffffff). Thus again yielding
sent_bytes containing first 2 bytes as sent_len last 2 bytes and the last 2
as 0 (as in case of packet_header in the previous case). Thus,
packet_header's datatype is corrected and it store the same data.

Kindly correct me if I misunderstood.

Thanks,
Greg Kroah-Hartman Oct. 14, 2013, 5:34 p.m. UTC | #3
On Mon, Oct 14, 2013 at 09:40:57PM +0530, Rashika Kheria wrote:
> The logic used here is based on the following understanding -
> 
>  > -                     packet_header &= ~(0xffffffff);
> > -                     packet_header |= (sent_len << 16);
> > -                     packet_header = cpu_to_le32(packet_header);
> 
> packet_header's last 2 bytes are ANDed to 0

32 bits (4 bytes) are being affected here, not 2 bytes.

> and then it is ORed with (sent_len << 16) to store the last 2 bytes of
> sent_len as it's first 2 bytes. After that it is converted from u32 to
> le32. (The error because packet_header must store values of type u32).
> 
> What I have done is:
> 
> > +                     sent_bytes = sent_len << 16;
> > +                     sent_bytes |= __le32_to_cpu(packet_header) & ~(0xffffffff);
> > +                     packet_header = cpu_to_le32(sent_bytes);
> 
> declared a new variable sent_bytes of type int

Don't use int, use the specific size, or odd things might happen.

> which will contain the last 2 bytes of sent_len.  Now this value be
> ORed with the AND of packet_header(type : u32) and mask(0xffffffff).
> Thus again yielding sent_bytes containing first 2 bytes as sent_len
> last 2 bytes and the last 2 as 0 (as in case of packet_header in the
> previous case). Thus, packet_header's datatype is corrected and it
> store the same data.
> 
> Kindly correct me if I misunderstood.

Try using some "real" numbers here and see what happens.

The issue is this line:

> +                     sent_bytes |= __le32_to_cpu(packet_header) & ~(0xffffffff);

Will always resolve to:
	sent_bytes |= 0;
right?

So why have it at all?

Yes, the original line of:
	packet_header &= ~(0xffffffff);
is horrible, and someone is thinking it will be faster than just setting
the value to 0, so don't continue this "style" of coding, it's not
needed at all (USB is _slow_, so no need to mess around with bit shifts
and the like here trying to make things "faster", let the compiler do
the correct thing, it will almost always be faster in the end.)

thanks,

greg k-h
Rashika Oct. 14, 2013, 5:44 p.m. UTC | #4
On Mon, Oct 14, 2013 at 11:04 PM, Greg KH <gregkh@linuxfoundation.org>wrote:

> On Mon, Oct 14, 2013 at 09:40:57PM +0530, Rashika Kheria wrote:
> > The logic used here is based on the following understanding -
> >
> >  > -                     packet_header &= ~(0xffffffff);
> > > -                     packet_header |= (sent_len << 16);
> > > -                     packet_header = cpu_to_le32(packet_header);
> >
> > packet_header's last 2 bytes are ANDed to 0
>
> 32 bits (4 bytes) are being affected here, not 2 bytes.
>
> > and then it is ORed with (sent_len << 16) to store the last 2 bytes of
> > sent_len as it's first 2 bytes. After that it is converted from u32 to
> > le32. (The error because packet_header must store values of type u32).
> >
> > What I have done is:
> >
> > > +                     sent_bytes = sent_len << 16;
> > > +                     sent_bytes |= __le32_to_cpu(packet_header) &
> ~(0xffffffff);
> > > +                     packet_header = cpu_to_le32(sent_bytes);
> >
> > declared a new variable sent_bytes of type int
>
> Don't use int, use the specific size, or odd things might happen.
>
> > which will contain the last 2 bytes of sent_len.  Now this value be
> > ORed with the AND of packet_header(type : u32) and mask(0xffffffff).
> > Thus again yielding sent_bytes containing first 2 bytes as sent_len
> > last 2 bytes and the last 2 as 0 (as in case of packet_header in the
> > previous case). Thus, packet_header's datatype is corrected and it
> > store the same data.
> >
> > Kindly correct me if I misunderstood.
>
> Try using some "real" numbers here and see what happens.
>
> The issue is this line:
>
> > +                     sent_bytes |= __le32_to_cpu(packet_header) &
> ~(0xffffffff);
>
> Will always resolve to:
>         sent_bytes |= 0;
> right?
>
> So why have it at all?
>
> Yes, the original line of:
>         packet_header &= ~(0xffffffff);
> is horrible, and someone is thinking it will be faster than just setting
> the value to 0, so don't continue this "style" of coding, it's not
> needed at all (USB is _slow_, so no need to mess around with bit shifts
> and the like here trying to make things "faster", let the compiler do
> the correct thing, it will almost always be faster in the end.)
>
> thanks,
>
> greg k-h
>
> So, I should just leave it as it is and do the changes according to what
is given?
Greg Kroah-Hartman Oct. 14, 2013, 5:50 p.m. UTC | #5
On Mon, Oct 14, 2013 at 11:14:21PM +0530, Rashika Kheria wrote:
> 

Please fix your email client to not send out html email, Linux kernel
mailing lists will reject those types of emails automatically...

> On Mon, Oct 14, 2013 at 11:04 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> 
>     On Mon, Oct 14, 2013 at 09:40:57PM +0530, Rashika Kheria wrote:
>     > The logic used here is based on the following understanding -
>     >
>     >  > -                     packet_header &= ~(0xffffffff);
>     > > -                     packet_header |= (sent_len << 16);
>     > > -                     packet_header = cpu_to_le32(packet_header);
>     >
>     > packet_header's last 2 bytes are ANDed to 0
> 
>     32 bits (4 bytes) are being affected here, not 2 bytes.
> 
>     > and then it is ORed with (sent_len << 16) to store the last 2 bytes of
>     > sent_len as it's first 2 bytes. After that it is converted from u32 to
>     > le32. (The error because packet_header must store values of type u32).
>     >
>     > What I have done is:
>     >
>     > > +                     sent_bytes = sent_len << 16;
>     > > +                     sent_bytes |= __le32_to_cpu(packet_header) & ~
>     (0xffffffff);
>     > > +                     packet_header = cpu_to_le32(sent_bytes);
>     >
>     > declared a new variable sent_bytes of type int
> 
>     Don't use int, use the specific size, or odd things might happen.
> 
>     > which will contain the last 2 bytes of sent_len.  Now this value be
>     > ORed with the AND of packet_header(type : u32) and mask(0xffffffff).
>     > Thus again yielding sent_bytes containing first 2 bytes as sent_len
>     > last 2 bytes and the last 2 as 0 (as in case of packet_header in the
>     > previous case). Thus, packet_header's datatype is corrected and it
>     > store the same data.
>     >
>     > Kindly correct me if I misunderstood.
> 
>     Try using some "real" numbers here and see what happens.
> 
>     The issue is this line:
> 
>     > +                     sent_bytes |= __le32_to_cpu(packet_header) & ~
>     (0xffffffff);
> 
>     Will always resolve to:
>             sent_bytes |= 0;
>     right?
> 
>     So why have it at all?
> 
>     Yes, the original line of:
>             packet_header &= ~(0xffffffff);
>     is horrible, and someone is thinking it will be faster than just setting
>     the value to 0, so don't continue this "style" of coding, it's not
>     needed at all (USB is _slow_, so no need to mess around with bit shifts
>     and the like here trying to make things "faster", let the compiler do
>     the correct thing, it will almost always be faster in the end.)
> 
>     thanks,
> 
>     greg k-h
> 
> 
> So, I should just leave it as it is and do the changes according to what is
> given?

Yes, please do.

thanks,

greg k-h

Patch
diff mbox

diff --git a/drivers/staging/btmtk_usb/btmtk_usb.c b/drivers/staging/btmtk_usb/btmtk_usb.c
index e17efd5..e03d805 100644
--- a/drivers/staging/btmtk_usb/btmtk_usb.c
+++ b/drivers/staging/btmtk_usb/btmtk_usb.c
@@ -559,13 +559,13 @@  static int btmtk_usb_load_fw(struct btmtk_usb_data *data)
 	struct urb *urb;
 	void *buf;
 	u32 cur_len = 0;
-	u32 packet_header = 0;
+	__le32 packet_header = 0;
 	u32 value;
 	u32 ilm_len = 0, dlm_len = 0;
 	u16 fw_ver, build_ver;
 	u32 loop = 0;
 	dma_addr_t data_dma;
-	int ret = 0, sent_len;
+	int ret = 0, sent_len, sent_bytes;
 	struct completion sent_to_mcu_done;
 	unsigned int pipe;
 	pipe = usb_sndbulkpipe(data->udev, data->bulk_tx_ep->bEndpointAddress);
@@ -671,9 +671,9 @@  loadfw_protect:
 		sent_len = min_t(s32, (ilm_len - cur_len), 14336);
 
 		if (sent_len > 0) {
-			packet_header &= ~(0xffffffff);
-			packet_header |= (sent_len << 16);
-			packet_header = cpu_to_le32(packet_header);
+			sent_bytes = sent_len << 16;  
+			sent_bytes |= __le32_to_cpu(packet_header) & ~(0xffffffff);  
+			packet_header = cpu_to_le32(sent_bytes);
 
 			memmove(buf, &packet_header, 4);
 			memmove(buf + 4, data->firmware->data + 32 + cur_len,
@@ -731,9 +731,9 @@  loadfw_protect:
 		if (sent_len <= 0)
 			break;
 
-		packet_header &= ~(0xffffffff);
-		packet_header |= (sent_len << 16);
-		packet_header = cpu_to_le32(packet_header);
+		sent_bytes = sent_len << 16;  
+		sent_bytes |= __le32_to_cpu(packet_header) & ~(0xffffffff);  
+		packet_header = cpu_to_le32(sent_bytes);
 
 		memmove(buf, &packet_header, 4);
 		memmove(buf + 4,