public inbox for linux-can@vger.kernel.org
 help / color / mirror / Atom feed
* re: can: usb: PEAK-System Technik PCAN-USB specific part
@ 2012-03-06 11:21 Dan Carpenter
  2012-03-07  8:56 ` Stephane Grosjean
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Dan Carpenter @ 2012-03-06 11:21 UTC (permalink / raw)
  To: s.grosjean; +Cc: linux-can

Hello Stephane Grosjean,

The patch 46be265d3388: "can: usb: PEAK-System Technik PCAN-USB 
specific part" from Mar 2, 2012, leads to the following warning:

drivers/net/can/usb/peak_usb/pcan_usb.c:751 pcan_usb_encode_msg()
	 error: wrong number of bits for 'cpu_to_le32' (16 vs 32)

drivers/net/can/usb/peak_usb/pcan_usb.c
   742          /* can id */
   743          if (cf->can_id & CAN_EFF_FLAG) {
   744                  __le32 tmp32 = cpu_to_le32(cf->can_id & CAN_ERR_MASK);
   745  
   746                  tmp32 <<= 3;
   747                  *pc |= PCAN_USB_STATUSLEN_EXT_ID;
   748                  memcpy(++pc, &tmp32, 4);
   749                  pc += 4;
   750          } else {
   751                  __le16 tmp16 = cpu_to_le32(cf->can_id & CAN_ERR_MASK);
                        ^^^^^^^^^^^^
A little endian 32 bit can't fit here.

   752  
   753                  tmp16 <<= 5;
   754                  memcpy(++pc, &tmp16, 2);
   755                  pc += 2;

regards,
dan carpenter


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

* Re: can: usb: PEAK-System Technik PCAN-USB specific part
  2012-03-06 11:21 can: usb: PEAK-System Technik PCAN-USB specific part Dan Carpenter
@ 2012-03-07  8:56 ` Stephane Grosjean
  2012-03-07  9:24   ` Marc Kleine-Budde
  2012-03-07  9:05 ` Stephane Grosjean
  2012-03-07 12:21 ` Dan Carpenter
  2 siblings, 1 reply; 14+ messages in thread
From: Stephane Grosjean @ 2012-03-07  8:56 UTC (permalink / raw)
  To: linux-can@vger.kernel.org

Hi everybody,

Happy to have seen some of you last Monday, at iCC, it was great. It's 
always easier when we're able to attach a face to a name.

Back to work now:

Le 06/03/2012 12:21, Dan Carpenter a écrit :
> Hello Stephane Grosjean,
>
> The patch 46be265d3388: "can: usb: PEAK-System Technik PCAN-USB
> specific part" from Mar 2, 2012, leads to the following warning:
>
> drivers/net/can/usb/peak_usb/pcan_usb.c:751 pcan_usb_encode_msg()
> 	 error: wrong number of bits for 'cpu_to_le32' (16 vs 32)
>
> drivers/net/can/usb/peak_usb/pcan_usb.c
>     742          /* can id */
>     743          if (cf->can_id&  CAN_EFF_FLAG) {
>     744                  __le32 tmp32 = cpu_to_le32(cf->can_id&  CAN_ERR_MASK);
>     745
>     746                  tmp32<<= 3;
>     747                  *pc |= PCAN_USB_STATUSLEN_EXT_ID;
>     748                  memcpy(++pc,&tmp32, 4);
>     749                  pc += 4;
>     750          } else {
>     751                  __le16 tmp16 = cpu_to_le32(cf->can_id&  CAN_ERR_MASK);
>                          ^^^^^^^^^^^^
> A little endian 32 bit can't fit here.


How may this error happen while it didn't before? Is this is due to some 
(new) options of gcc, or because of a new version used? In the future, 
how could I avoid that? I mean, how to synchronize with the final GCC 
options/version?

FYI, the original code was:

> >  +	else {
> } else {
> >  +		u16 tmp16 = (u16 )*cpu_to_le32*(cf->can_id&  CAN_ERR_MASK);
>                              ^^^^^^^^^^^^^^^^^

cf->can_id is a 32-bits field which I want to keep only the lo-word part...

How to do that in the correct way, please?

Anyway, what next? I suppose I have to submit a new patch, but comparing 
to what?

Thanks for your help and regards,

Stéphane
--
PEAK-System Technik GmbH, Otto-Roehm-Strasse 69, D-64293 Darmstadt 
Geschaeftsleitung: A.Gach/U.Wilhelm,St.Nr.:007/241/13586 FA Darmstadt 
HRB-9183 Darmstadt, Ust.IdNr.:DE 202220078, WEE-Reg.-Nr.: DE39305391 
Tel.+49 (0)6151-817320 / Fax:+49 (0)6151-817329, info@peak-system.com

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

* Re: can: usb: PEAK-System Technik PCAN-USB specific part
  2012-03-06 11:21 can: usb: PEAK-System Technik PCAN-USB specific part Dan Carpenter
  2012-03-07  8:56 ` Stephane Grosjean
@ 2012-03-07  9:05 ` Stephane Grosjean
  2012-03-07 12:21 ` Dan Carpenter
  2 siblings, 0 replies; 14+ messages in thread
From: Stephane Grosjean @ 2012-03-07  9:05 UTC (permalink / raw)
  To: linux-can@vger.kernel.org

Same but without the stars around "cpu_to_le32()"... (Sorry for that 
wrong copy/paste)

> FYI, the original code was:
>
>> >  +	else {
>> } else {
>> >  +		u16 tmp16 = (u16 )cpu_to_le32(cf->can_id & CAN_ERR_MASK);
>>                              ^^^^^^^^^^^^^^^^^

Regards,

Stéphane
--
PEAK-System Technik GmbH, Otto-Roehm-Strasse 69, D-64293 Darmstadt 
Geschaeftsleitung: A.Gach/U.Wilhelm,St.Nr.:007/241/13586 FA Darmstadt 
HRB-9183 Darmstadt, Ust.IdNr.:DE 202220078, WEE-Reg.-Nr.: DE39305391 
Tel.+49 (0)6151-817320 / Fax:+49 (0)6151-817329, info@peak-system.com

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

* Re: can: usb: PEAK-System Technik PCAN-USB specific part
  2012-03-07  8:56 ` Stephane Grosjean
@ 2012-03-07  9:24   ` Marc Kleine-Budde
  2012-03-07 11:16     ` Stephane Grosjean
  0 siblings, 1 reply; 14+ messages in thread
From: Marc Kleine-Budde @ 2012-03-07  9:24 UTC (permalink / raw)
  To: Stephane Grosjean; +Cc: linux-can@vger.kernel.org

[-- Attachment #1: Type: text/plain, Size: 2391 bytes --]

On 03/07/2012 09:56 AM, Stephane Grosjean wrote:
> Happy to have seen some of you last Monday, at iCC, it was great. It's
> always easier when we're able to attach a face to a name.

Yes, I always like that occasions.

> Back to work now:
> 
> Le 06/03/2012 12:21, Dan Carpenter a écrit :
>> Hello Stephane Grosjean,
>>
>> The patch 46be265d3388: "can: usb: PEAK-System Technik PCAN-USB
>> specific part" from Mar 2, 2012, leads to the following warning:
>>
>> drivers/net/can/usb/peak_usb/pcan_usb.c:751 pcan_usb_encode_msg()
>>      error: wrong number of bits for 'cpu_to_le32' (16 vs 32)
>>
>> drivers/net/can/usb/peak_usb/pcan_usb.c
>>     742          /* can id */
>>     743          if (cf->can_id&  CAN_EFF_FLAG) {
>>     744                  __le32 tmp32 = cpu_to_le32(cf->can_id& 
>> CAN_ERR_MASK);
>>     745
>>     746                  tmp32<<= 3;
>>     747                  *pc |= PCAN_USB_STATUSLEN_EXT_ID;
>>     748                  memcpy(++pc,&tmp32, 4);
>>     749                  pc += 4;
>>     750          } else {
>>     751                  __le16 tmp16 = cpu_to_le32(cf->can_id& 
>> CAN_ERR_MASK);
>>                          ^^^^^^^^^^^^
>> A little endian 32 bit can't fit here.
> 
> 
> How may this error happen while it didn't before? Is this is due to some
> (new) options of gcc, or because of a new version used? In the future,
> how could I avoid that? I mean, how to synchronize with the final GCC
> options/version?

There's no final gcc version. Gcc and the kernel will always evolve.

> 
> FYI, the original code was:
> 
>> >  +    else {
>> } else {
>> >  +        u16 tmp16 = (u16 )*cpu_to_le32*(cf->can_id&  CAN_ERR_MASK);
>>                              ^^^^^^^^^^^^^^^^^
> 
> cf->can_id is a 32-bits field which I want to keep only the lo-word part...
> 
> How to do that in the correct way, please?
> 
> Anyway, what next? I suppose I have to submit a new patch, but comparing
> to what?

Please post your patch against the can-next git repo.

https://gitorious.org/linux-can/linux-can-next

Regards, Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

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

* Re: can: usb: PEAK-System Technik PCAN-USB specific part
  2012-03-07  9:24   ` Marc Kleine-Budde
@ 2012-03-07 11:16     ` Stephane Grosjean
  0 siblings, 0 replies; 14+ messages in thread
From: Stephane Grosjean @ 2012-03-07 11:16 UTC (permalink / raw)
  To: Marc Kleine-Budde; +Cc: linux-can@vger.kernel.org

Le 07/03/2012 10:24, Marc Kleine-Budde a écrit :
> On 03/07/2012 09:56 AM, Stephane Grosjean wrote:
> Le 06/03/2012 12:21, Dan Carpenter a écrit :
>>> Hello Stephane Grosjean,
>>>
>>> The patch 46be265d3388: "can: usb: PEAK-System Technik PCAN-USB
>>> specific part" from Mar 2, 2012, leads to the following warning:
>>>
>>> drivers/net/can/usb/peak_usb/pcan_usb.c:751 pcan_usb_encode_msg()
>>>       error: wrong number of bits for 'cpu_to_le32' (16 vs 32)
>>>
>>> drivers/net/can/usb/peak_usb/pcan_usb.c
>>>      742          /* can id */
>>>      743          if (cf->can_id&   CAN_EFF_FLAG) {
>>>      744                  __le32 tmp32 = cpu_to_le32(cf->can_id&
>>> CAN_ERR_MASK);
>>>      745
>>>      746                  tmp32<<= 3;
>>>      747                  *pc |= PCAN_USB_STATUSLEN_EXT_ID;
>>>      748                  memcpy(++pc,&tmp32, 4);
>>>      749                  pc += 4;
>>>      750          } else {
>>>      751                  __le16 tmp16 = cpu_to_le32(cf->can_id&
>>> CAN_ERR_MASK);
>>>                           ^^^^^^^^^^^^
>>> A little endian 32 bit can't fit here.
>>
>> How may this error happen while it didn't before? Is this is due to some
>> (new) options of gcc, or because of a new version used? In the future,
>> how could I avoid that? I mean, how to synchronize with the final GCC
>> options/version?
> There's no final gcc version. Gcc and the kernel will always evolve.


Yes I know, this is Ok... But the question is: how did that guy manage 
to get that error? I "git pull"ed my own linux-can-next then rebuilt my 
drivers and don't even see that error!
I suppose that he set some flags somewhere in the Kernel Makefile(s), 
but where? How can I know whether my next patch will fail or not in his 
configuration?

Thanks for your reply,

Stéphane


--
PEAK-System Technik GmbH, Otto-Roehm-Strasse 69, D-64293 Darmstadt 
Geschaeftsleitung: A.Gach/U.Wilhelm,St.Nr.:007/241/13586 FA Darmstadt 
HRB-9183 Darmstadt, Ust.IdNr.:DE 202220078, WEE-Reg.-Nr.: DE39305391 
Tel.+49 (0)6151-817320 / Fax:+49 (0)6151-817329, info@peak-system.com

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

* Re: can: usb: PEAK-System Technik PCAN-USB specific part
  2012-03-06 11:21 can: usb: PEAK-System Technik PCAN-USB specific part Dan Carpenter
  2012-03-07  8:56 ` Stephane Grosjean
  2012-03-07  9:05 ` Stephane Grosjean
@ 2012-03-07 12:21 ` Dan Carpenter
  2012-03-07 12:29   ` Marc Kleine-Budde
  2 siblings, 1 reply; 14+ messages in thread
From: Dan Carpenter @ 2012-03-07 12:21 UTC (permalink / raw)
  To: s.grosjean; +Cc: linux-can

[-- Attachment #1: Type: text/plain, Size: 872 bytes --]

On Tue, Mar 06, 2012 at 02:21:08PM +0300, Dan Carpenter wrote:
> Hello Stephane Grosjean,
> 
> The patch 46be265d3388: "can: usb: PEAK-System Technik PCAN-USB 
> specific part" from Mar 2, 2012, leads to the following warning:
> 
> drivers/net/can/usb/peak_usb/pcan_usb.c:751 pcan_usb_encode_msg()
> 	 error: wrong number of bits for 'cpu_to_le32' (16 vs 32)
> 
> drivers/net/can/usb/peak_usb/pcan_usb.c
>    742          /* can id */
>    743          if (cf->can_id & CAN_EFF_FLAG) {
>    744                  __le32 tmp32 = cpu_to_le32(cf->can_id & CAN_ERR_MASK);
>    745  
>    746                  tmp32 <<= 3;

Also we can't be doing this shift on little endian data.

>    747                  *pc |= PCAN_USB_STATUSLEN_EXT_ID;
>    748                  memcpy(++pc, &tmp32, 4);
>    749                  pc += 4;

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

* Re: can: usb: PEAK-System Technik PCAN-USB specific part
  2012-03-07 12:21 ` Dan Carpenter
@ 2012-03-07 12:29   ` Marc Kleine-Budde
  2012-03-07 13:17     ` Dan Carpenter
  0 siblings, 1 reply; 14+ messages in thread
From: Marc Kleine-Budde @ 2012-03-07 12:29 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: s.grosjean, linux-can

[-- Attachment #1: Type: text/plain, Size: 1264 bytes --]

On 03/07/2012 01:21 PM, Dan Carpenter wrote:
> On Tue, Mar 06, 2012 at 02:21:08PM +0300, Dan Carpenter wrote:
>> Hello Stephane Grosjean,
>>
>> The patch 46be265d3388: "can: usb: PEAK-System Technik PCAN-USB 
>> specific part" from Mar 2, 2012, leads to the following warning:
>>
>> drivers/net/can/usb/peak_usb/pcan_usb.c:751 pcan_usb_encode_msg()
>> 	 error: wrong number of bits for 'cpu_to_le32' (16 vs 32)
>>
>> drivers/net/can/usb/peak_usb/pcan_usb.c
>>    742          /* can id */
>>    743          if (cf->can_id & CAN_EFF_FLAG) {
>>    744                  __le32 tmp32 = cpu_to_le32(cf->can_id & CAN_ERR_MASK);
>>    745  
>>    746                  tmp32 <<= 3;
> 
> Also we can't be doing this shift on little endian data.

This should be first shift, then convert to little endian, isn't it?

>>    747                  *pc |= PCAN_USB_STATUSLEN_EXT_ID;
>>    748                  memcpy(++pc, &tmp32, 4);
>>    749                  pc += 4;

Marc
-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

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

* Re: can: usb: PEAK-System Technik PCAN-USB specific part
  2012-03-07 12:29   ` Marc Kleine-Budde
@ 2012-03-07 13:17     ` Dan Carpenter
  2012-03-07 13:26       ` Marc Kleine-Budde
  0 siblings, 1 reply; 14+ messages in thread
From: Dan Carpenter @ 2012-03-07 13:17 UTC (permalink / raw)
  To: Marc Kleine-Budde; +Cc: s.grosjean, linux-can

[-- Attachment #1: Type: text/plain, Size: 1366 bytes --]

On Wed, Mar 07, 2012 at 01:29:19PM +0100, Marc Kleine-Budde wrote:
> On 03/07/2012 01:21 PM, Dan Carpenter wrote:
> > On Tue, Mar 06, 2012 at 02:21:08PM +0300, Dan Carpenter wrote:
> >> Hello Stephane Grosjean,
> >>
> >> The patch 46be265d3388: "can: usb: PEAK-System Technik PCAN-USB 
> >> specific part" from Mar 2, 2012, leads to the following warning:
> >>
> >> drivers/net/can/usb/peak_usb/pcan_usb.c:751 pcan_usb_encode_msg()
> >> 	 error: wrong number of bits for 'cpu_to_le32' (16 vs 32)
> >>
> >> drivers/net/can/usb/peak_usb/pcan_usb.c
> >>    742          /* can id */
> >>    743          if (cf->can_id & CAN_EFF_FLAG) {
> >>    744                  __le32 tmp32 = cpu_to_le32(cf->can_id & CAN_ERR_MASK);
> >>    745  
> >>    746                  tmp32 <<= 3;
> > 
> > Also we can't be doing this shift on little endian data.
> 
> This should be first shift, then convert to little endian, isn't it?
> 

That's my guess too.
		__le32 tmp32 = cpu_to_le32((cf->can_id & CAN_ERR_MASK) << 3);

Btw, it's slightly odd to me that CAN_EFF_MASK and CAN_ERR_MASK are
the same.  I wouldn't comment on it except that I was wondering if
it might be a copy and paste bug.

#define CAN_EFF_MASK 0x1FFFFFFFU /* extended frame format (EFF) */
#define CAN_ERR_MASK 0x1FFFFFFFU /* omit EFF, RTR, ERR flags */

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

* Re: can: usb: PEAK-System Technik PCAN-USB specific part
  2012-03-07 13:17     ` Dan Carpenter
@ 2012-03-07 13:26       ` Marc Kleine-Budde
  2012-03-07 13:36         ` Dan Carpenter
  2012-03-07 14:05         ` Stephane Grosjean
  0 siblings, 2 replies; 14+ messages in thread
From: Marc Kleine-Budde @ 2012-03-07 13:26 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: s.grosjean, linux-can

[-- Attachment #1: Type: text/plain, Size: 1920 bytes --]

On 03/07/2012 02:17 PM, Dan Carpenter wrote:
> On Wed, Mar 07, 2012 at 01:29:19PM +0100, Marc Kleine-Budde wrote:
>> On 03/07/2012 01:21 PM, Dan Carpenter wrote:
>>> On Tue, Mar 06, 2012 at 02:21:08PM +0300, Dan Carpenter wrote:
>>>> Hello Stephane Grosjean,
>>>>
>>>> The patch 46be265d3388: "can: usb: PEAK-System Technik PCAN-USB 
>>>> specific part" from Mar 2, 2012, leads to the following warning:
>>>>
>>>> drivers/net/can/usb/peak_usb/pcan_usb.c:751 pcan_usb_encode_msg()
>>>> 	 error: wrong number of bits for 'cpu_to_le32' (16 vs 32)
>>>>
>>>> drivers/net/can/usb/peak_usb/pcan_usb.c
>>>>    742          /* can id */
>>>>    743          if (cf->can_id & CAN_EFF_FLAG) {
>>>>    744                  __le32 tmp32 = cpu_to_le32(cf->can_id & CAN_ERR_MASK);
>>>>    745  
>>>>    746                  tmp32 <<= 3;
>>>
>>> Also we can't be doing this shift on little endian data.
>>
>> This should be first shift, then convert to little endian, isn't it?
>>
> 
> That's my guess too.
> 		__le32 tmp32 = cpu_to_le32((cf->can_id & CAN_ERR_MASK) << 3);

Stéphane, do you want to make that patch?
Add my Acked-by and a
"Reported-by: Dan Carpenter <dan.carpenter@oracle.com>"

> Btw, it's slightly odd to me that CAN_EFF_MASK and CAN_ERR_MASK are
> the same.  I wouldn't comment on it except that I was wondering if
> it might be a copy and paste bug.

You're right, it odd, but it's intended to be the same. Do you want to
make a patch to add an annotation?

> #define CAN_EFF_MASK 0x1FFFFFFFU /* extended frame format (EFF) */
> #define CAN_ERR_MASK 0x1FFFFFFFU /* omit EFF, RTR, ERR flags */

Marc
-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

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

* Re: can: usb: PEAK-System Technik PCAN-USB specific part
  2012-03-07 13:26       ` Marc Kleine-Budde
@ 2012-03-07 13:36         ` Dan Carpenter
  2012-03-07 13:43           ` Marc Kleine-Budde
  2012-03-07 14:05         ` Stephane Grosjean
  1 sibling, 1 reply; 14+ messages in thread
From: Dan Carpenter @ 2012-03-07 13:36 UTC (permalink / raw)
  To: Marc Kleine-Budde; +Cc: s.grosjean, linux-can

[-- Attachment #1: Type: text/plain, Size: 350 bytes --]

On Wed, Mar 07, 2012 at 02:26:10PM +0100, Marc Kleine-Budde wrote:
> You're right, it odd, but it's intended to be the same. Do you want to
> make a patch to add an annotation?
> 

No.  I was just curious.  I should have noticed how old the
definitions were and realized that they were well tested at this
point.

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

* Re: can: usb: PEAK-System Technik PCAN-USB specific part
  2012-03-07 13:36         ` Dan Carpenter
@ 2012-03-07 13:43           ` Marc Kleine-Budde
  0 siblings, 0 replies; 14+ messages in thread
From: Marc Kleine-Budde @ 2012-03-07 13:43 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: s.grosjean, linux-can

[-- Attachment #1: Type: text/plain, Size: 743 bytes --]

On 03/07/2012 02:36 PM, Dan Carpenter wrote:
> On Wed, Mar 07, 2012 at 02:26:10PM +0100, Marc Kleine-Budde wrote:
>> You're right, it odd, but it's intended to be the same. Do you want to
>> make a patch to add an annotation?
>>
> 
> No.  I was just curious.  I should have noticed how old the
> definitions were and realized that they were well tested at this
> point.

It's always good to ask, old code doesn't automatically mean it's tested :)

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

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

* Re: can: usb: PEAK-System Technik PCAN-USB specific part
  2012-03-07 13:26       ` Marc Kleine-Budde
  2012-03-07 13:36         ` Dan Carpenter
@ 2012-03-07 14:05         ` Stephane Grosjean
  2012-03-07 14:07           ` Marc Kleine-Budde
  1 sibling, 1 reply; 14+ messages in thread
From: Stephane Grosjean @ 2012-03-07 14:05 UTC (permalink / raw)
  To: Marc Kleine-Budde; +Cc: Dan Carpenter, linux-can



Le 07/03/2012 14:26, Marc Kleine-Budde a écrit :
> On 03/07/2012 02:17 PM, Dan Carpenter wrote:
>> On Wed, Mar 07, 2012 at 01:29:19PM +0100, Marc Kleine-Budde wrote:
>>> On 03/07/2012 01:21 PM, Dan Carpenter wrote:
>>>> On Tue, Mar 06, 2012 at 02:21:08PM +0300, Dan Carpenter wrote:
>>>>> Hello Stephane Grosjean,
>>>>>
>>>>> The patch 46be265d3388: "can: usb: PEAK-System Technik PCAN-USB
>>>>> specific part" from Mar 2, 2012, leads to the following warning:
>>>>>
>>>>> drivers/net/can/usb/peak_usb/pcan_usb.c:751 pcan_usb_encode_msg()
>>>>> 	 error: wrong number of bits for 'cpu_to_le32' (16 vs 32)
>>>>>
>>>>> drivers/net/can/usb/peak_usb/pcan_usb.c
>>>>>     742          /* can id */
>>>>>     743          if (cf->can_id&  CAN_EFF_FLAG) {
>>>>>     744                  __le32 tmp32 = cpu_to_le32(cf->can_id&  CAN_ERR_MASK);
>>>>>     745
>>>>>     746                  tmp32<<= 3;
>>>> Also we can't be doing this shift on little endian data.
>>> This should be first shift, then convert to little endian, isn't it?
>>>
>> That's my guess too.
>> 		__le32 tmp32 = cpu_to_le32((cf->can_id&  CAN_ERR_MASK)<<  3);
> Stéphane, do you want to make that patch?
> Add my Acked-by and a
> "Reported-by: Dan Carpenter<dan.carpenter@oracle.com>"

Ok I'll do it as soon as I did small tests to check.

FYI, the patch will also contain the same fix but for the tmp16 local, 
declared into the "else" block:

     __le16 tmp16 = cpu_to_le16((cf->can_id & CAN_ERR_MASK) << 5);


Regards,

Stéphane


--
PEAK-System Technik GmbH, Otto-Roehm-Strasse 69, D-64293 Darmstadt 
Geschaeftsleitung: A.Gach/U.Wilhelm,St.Nr.:007/241/13586 FA Darmstadt 
HRB-9183 Darmstadt, Ust.IdNr.:DE 202220078, WEE-Reg.-Nr.: DE39305391 
Tel.+49 (0)6151-817320 / Fax:+49 (0)6151-817329, info@peak-system.com

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

* Re: can: usb: PEAK-System Technik PCAN-USB specific part
  2012-03-07 14:05         ` Stephane Grosjean
@ 2012-03-07 14:07           ` Marc Kleine-Budde
  2012-03-07 14:13             ` Stephane Grosjean
  0 siblings, 1 reply; 14+ messages in thread
From: Marc Kleine-Budde @ 2012-03-07 14:07 UTC (permalink / raw)
  To: Stephane Grosjean; +Cc: Dan Carpenter, linux-can

[-- Attachment #1: Type: text/plain, Size: 918 bytes --]

On 03/07/2012 03:05 PM, Stephane Grosjean wrote:
>>> That's my guess too.
>>>         __le32 tmp32 = cpu_to_le32((cf->can_id&  CAN_ERR_MASK)<<  3);
>> Stéphane, do you want to make that patch?
>> Add my Acked-by and a
>> "Reported-by: Dan Carpenter<dan.carpenter@oracle.com>"
> 
> Ok I'll do it as soon as I did small tests to check.

Have you got a big endian machine (powerpc or an ARM IXP in Big Endian
Mode) somewhere?

> FYI, the patch will also contain the same fix but for the tmp16 local,
> declared into the "else" block:
> 
>     __le16 tmp16 = cpu_to_le16((cf->can_id & CAN_ERR_MASK) << 5);

good point,

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

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

* Re: can: usb: PEAK-System Technik PCAN-USB specific part
  2012-03-07 14:07           ` Marc Kleine-Budde
@ 2012-03-07 14:13             ` Stephane Grosjean
  0 siblings, 0 replies; 14+ messages in thread
From: Stephane Grosjean @ 2012-03-07 14:13 UTC (permalink / raw)
  To: Marc Kleine-Budde; +Cc: Dan Carpenter, linux-can

Le 07/03/2012 15:07, Marc Kleine-Budde a écrit :
> On 03/07/2012 03:05 PM, Stephane Grosjean wrote:
>>>> That's my guess too.
>>>>          __le32 tmp32 = cpu_to_le32((cf->can_id&   CAN_ERR_MASK)<<   3);
>>> Stéphane, do you want to make that patch?
>>> Add my Acked-by and a
>>> "Reported-by: Dan Carpenter<dan.carpenter@oracle.com>"
>> Ok I'll do it as soon as I did small tests to check.
> Have you got a big endian machine (powerpc or an ARM IXP in Big Endian
> Mode) somewhere?

No sorry, I only have little-endian machines here (x86 and ARMle). I 
just talked about small non-regression tests.

Regards,

Stéphane


--
PEAK-System Technik GmbH, Otto-Roehm-Strasse 69, D-64293 Darmstadt 
Geschaeftsleitung: A.Gach/U.Wilhelm,St.Nr.:007/241/13586 FA Darmstadt 
HRB-9183 Darmstadt, Ust.IdNr.:DE 202220078, WEE-Reg.-Nr.: DE39305391 
Tel.+49 (0)6151-817320 / Fax:+49 (0)6151-817329, info@peak-system.com

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

end of thread, other threads:[~2012-03-07 14:13 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-03-06 11:21 can: usb: PEAK-System Technik PCAN-USB specific part Dan Carpenter
2012-03-07  8:56 ` Stephane Grosjean
2012-03-07  9:24   ` Marc Kleine-Budde
2012-03-07 11:16     ` Stephane Grosjean
2012-03-07  9:05 ` Stephane Grosjean
2012-03-07 12:21 ` Dan Carpenter
2012-03-07 12:29   ` Marc Kleine-Budde
2012-03-07 13:17     ` Dan Carpenter
2012-03-07 13:26       ` Marc Kleine-Budde
2012-03-07 13:36         ` Dan Carpenter
2012-03-07 13:43           ` Marc Kleine-Budde
2012-03-07 14:05         ` Stephane Grosjean
2012-03-07 14:07           ` Marc Kleine-Budde
2012-03-07 14:13             ` Stephane Grosjean

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox