Oliver Hartkopp wrote:
> Wolfgang Grandegger wrote:
>> Marc Kleine-Budde wrote:
>
>>>> + if (unlikely(dlc > 8)) {
>>>> +
>>>> + static int print_dlc_err;
>>>> +
>>>> + if (!print_dlc_err) {
>>>> + dev_err(dev->dev.parent, "illegal dlc %d\n", dlc);
>>>> + print_dlc_err = 1;
>>>> + }
>>> You should use something like "WARN_ONCE" instead of open-coding it.
>> WARN_ONCE is OK if it's acceptable that dlc > 8 happens.
>
> Yes. I also thought about something like
>
> if (WARN_ONCE(dlc > 8, "%s: Illegal dlc %d\n", DRV_NAME, dlc))
> dlc = 8;
>
> Would this be ok?
>
>
>>> The general question is should a driver print this message or not. The
>>> at91 silently limits the dlc to 8. I'm not sure what the other mainline
>>> drivers do.
>> It depends. If it's due to a malfunctioning of the hardware (or
>> software), we should inform the user. Unfortunately, the drivers do not
>> yet handle that case consistently. I have realized:
>>
>> - ignore dlc > 8
>> - dlc &= 0xf
>> - print error message (or oops)
>> - min(dlc, 8)
>>
>
> The problem is to define what a dlc > 8 provided by the CAN controller (which
> IS a BUG inside the CAN controller!) should mean to the rest of the data
> inside the registers containing the received CAN frame:
>
> - do we assume the rest to be a valid CAN frame?
> - should we drop the frame ?
>
> Is this something that depends on the CAN controller? E.g. is a wrong dlc
> tolerable for SJA1000 but not for at91_can ??
>
> Once we've checked that, we should clean this up.
Yep. Are there SJA1000 chips delivering invalid dlc's?
>> This is just for incoming messages. The dlc of out-going messages could
>> be check by the upper layer. Is that already done? We need to fix that
>> issue consistently for all drivers.
>
> The CAN protocols that are using the PF_CAN core should use can_send() which
> does this check:
>
> int can_send(struct sk_buff *skb, int loop)
> {
> struct sk_buff *newskb = NULL;
> struct can_frame *cf = (struct can_frame *)skb->data;
> int err;
>
> if (skb->len != sizeof(struct can_frame) || cf->can_dlc > 8) {
> kfree_skb(skb);
> return -EINVAL;
> }
> (..)
Ok, then we can also drop dlc checks in the driver's xmit function.
Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
