netdev - Re: [PATCH 2/4] net: can: ifi: Fix TX DLC configuration

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <56D6C25E.4000407@denx.de>
Date:	Wed, 02 Mar 2016 11:37:18 +0100
From:	Marek Vasut <marex@...x.de>
To:	Oliver Hartkopp <socketcan@...tkopp.net>, linux-can@...r.kernel.org
CC:	netdev@...r.kernel.org, Marc Kleine-Budde <mkl@...gutronix.de>,
	Mark Rutland <mark.rutland@....com>,
	Wolfgang Grandegger <wg@...ndegger.com>
Subject: Re: [PATCH 2/4] net: can: ifi: Fix TX DLC configuration

On 03/02/2016 07:12 AM, Oliver Hartkopp wrote:
> 
> 
> On 03/01/2016 10:27 PM, Marek Vasut wrote:
>> On 03/01/2016 07:11 PM, Oliver Hartkopp wrote:
>>
>> Hi!
>>
>>> On 02/29/2016 08:59 PM, Marek Vasut wrote:
>>>> The TX DLC, the transmission length information, was not written
>>>> into the transmit configuration register. When using the CAN core
>>>> with different CAN controller, the receiving CAN controller will
>>>> receive only the ID part of the CAN frame, but no data at all.
>>>>
>>>> This patch adds the TX DLC into the register to fix this issue.
>>>>
>>>> Signed-off-by: Marek Vasut <marex@...x.de>
>>>> Cc: Marc Kleine-Budde <mkl@...gutronix.de>
>>>> Cc: Mark Rutland <mark.rutland@....com>
>>>> Cc: Oliver Hartkopp <socketcan@...tkopp.net>
>>>> Cc: Wolfgang Grandegger <wg@...ndegger.com>
>>>> ---
>>>>  drivers/net/can/ifi_canfd/ifi_canfd.c | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c b/drivers/net/can/ifi_canfd/ifi_canfd.c
>>>> index 72f5205..82a33bd 100644
>>>> --- a/drivers/net/can/ifi_canfd/ifi_canfd.c
>>>> +++ b/drivers/net/can/ifi_canfd/ifi_canfd.c
>>>> @@ -774,10 +774,15 @@ static netdev_tx_t ifi_canfd_start_xmit(struct sk_buff *skb,
>>>>  
>>>>  	if (priv->can.ctrlmode & (CAN_CTRLMODE_FD | CAN_CTRLMODE_FD_NON_ISO)) {
>>>>  		if (can_is_canfd_skb(skb)) {
>>>> +			txdlc |= can_len2dlc(cf->len);
>>>>  			txdlc |= IFI_CANFD_TXFIFO_DLC_EDL;
>>>>  			if (cf->flags & CANFD_BRS)
>>>>  				txdlc |= IFI_CANFD_TXFIFO_DLC_BRS;
>>>> +		} else {
>>>> +			txdlc |= cf->len;
>>>>  		}
>>>> +	} else {
>>>> +		txdlc |= cf->len;
>>>>  	}
>>>
>>> Please use
>>>
>>> 	txdlc |= can_len2dlc(cf->len);
>>>
>>> by default (it works for CAN and CAN FD).
>>>
>>> So that it looks more like:
>>>
>>> 	txdlc |= can_len2dlc(cf->len);
>>
>> Roger.
>>
>>> 	if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) && can_is_canfd_skb(skb)) {
>>> 		txdlc |= IFI_CANFD_TXFIFO_DLC_EDL;
>>> 		if (cf->flags & CANFD_BRS)
>>> 			txdlc |= IFI_CANFD_TXFIFO_DLC_BRS;
>>> 	}
>>>
>>> Testing against CAN_CTRLMODE_FD_NON_ISO is wrong!
>>> This configuration bit is just for the protocol on the wire and is no
>>> distinction for CAN / CAN FD.
>>
>> So CAN_CTRLMODE_FD is always set if the system operates in CAN/FD mode.
> 
> Not the 'system' but this specific CAN netdevice.

Ooops, of course, sorry.

>> And in addition to that, if the system operates in CAN/FD BOSCH mode,
>> the CAN_CTRLMODE_FD_NON_ISO is set. Do I understand it correctly ?
> 
> Yep! ('the CAN netdev')

Thanks for the clarification!

-- 
Best regards,
Marek Vasut