Re: [PATCH] can: kvaser_usb: handle rx msg correctly

[Marc Kleine-Budde <mkl@pengutronix.de>]

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

On 05/02/2013 08:06 PM, Olivier Sobrie wrote:
> Hi Marc,
> 
> On Thu, May 02, 2013 at 12:35:53PM +0200, Marc Kleine-Budde wrote:
>> On 04/30/2013 11:40 PM, Olivier Sobrie wrote:
>>> --- a/drivers/net/can/usb/kvaser_usb.c
>>> +++ b/drivers/net/can/usb/kvaser_usb.c
>>> @@ -834,22 +834,51 @@ static void kvaser_usb_rx_can_msg(const struct kvaser_usb *dev,
>>>  		return;
>>>  	}
>>>  
>>> -	cf->can_id = ((msg->u.rx_can.msg[0] & 0x1f) << 6) |
>>> -		     (msg->u.rx_can.msg[1] & 0x3f);
>>> -	cf->can_dlc = get_can_dlc(msg->u.rx_can.msg[5]);
>>> +	switch (msg->id) {
>>> +	case CMD_LOG_MESSAGE:
>>> +		cf->can_id = msg->u.log_message.id;
>>
>> No need to mangle the can_id like the CMD_RX_EXT_MESSAGE or
>> CMD_RX_STD_MESSAGE case? What about the extended messages flag?
> 
> No not needed but I forgot a le32_to_cpu() for the can_id.
> The hw set the CAN_EFF_FLAG when it is an extended frame.

The hardware uses the same bit to mask an extended flag as the linux
kernel does it with CAN_EFF_FLAG? I feel more confident about this if
you add a flag that describes the hard EFF_FLAG and the do the "is EFF
flag set, mask 29 bit and set CAN_EFF_FLAG otherwise mask 11 bit" dance.

Can you please check the driver for other missing leXX_to_cpu and
cpu_to_leXX?

>>> +		cf->can_dlc = get_can_dlc(msg->u.log_message.dlc);
>>> +
>>> +		if (msg->u.log_message.flags & MSG_FLAG_REMOTE_FRAME)
>>> +			cf->can_id |= CAN_RTR_FLAG;
>>> +		else
>>> +			memcpy(cf->data, &msg->u.log_message.data,
>>> +			       cf->can_dlc);
>>
>> Is this endianness save? But the memcpy was already here. Keep it until
>> someone with a ppc or sparc complains.
> 
> msg->u.log_message.dlc is an u8 and I don't see why the memcpy would be
> a problem.

It's not the dlc I'm talking about. I mean the data. There might be a
problem if you have a dlc of 5. Where is byte 5 located? But I think
it's safe as it is.

>>> +		break;
>>>  
>>> -	if (msg->id == CMD_RX_EXT_MESSAGE) {
>>> +	case CMD_RX_EXT_MESSAGE:
>>>  		cf->can_id <<= 18;
>>
>> who sets the can_id in the first place?
> 
> Damn I shouldn't have changed this by a switch case.
> I fix this and send you an updated patch.

Thanks,
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: 263 bytes --]