Received-SPF: pass (google.com: domain of linux-kernel+bounces-74873-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <54afa5e8-fb5e-4d90-8897-8f3c5a684418@hartkopp.net>
Date: Wed, 21 Feb 2024 15:14:02 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] can: netlink: Fix TDCO calculation using the old data
 bittiming
To: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>,
 Maxime Jayat <maxime.jayat@mobile-devices.fr>
Cc: Wolfgang Grandegger <wg@grandegger.com>,
 Marc Kleine-Budde <mkl@pengutronix.de>, linux-can@vger.kernel.org,
 netdev@vger.kernel.org, linux-kernel@vger.kernel.org
References: <40579c18-63c0-43a4-8d4c-f3a6c1c0b417@munic.io>
 <CAMZ6Rq+10m=yQ9Cc9gZQegwD=6iCU=s1r78+ogJ4PV0f5_s+tQ@mail.gmail.com>
Content-Language: en-US
From: Oliver Hartkopp <socketcan@hartkopp.net>
In-Reply-To: <CAMZ6Rq+10m=yQ9Cc9gZQegwD=6iCU=s1r78+ogJ4PV0f5_s+tQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

Hi all,

I have an old PCAN USB adapter (Classical CAN) which uses the pcan_usb 
driver and wanted to set a 50kbit/s bitrate:

ip link set can0 up txqueuelen 500 type can bitrate 50000 sjw 4

First it complained about the SJW having a higher value than some 
phase-seg value which was 2.

Error: sjw: 4 greater than phase-seg2: 2.

I always thought the driver automatically adapts the SJW value to the 
highest possible and SJW=4 could always be set. Did this change at a 
certain point?

Anyway, then I reduced the given SJW value and the ip command did not 
give any error message.

But finally there was not CAN traffic possible with my "always working 
setup".

I'm running 6.8.0-rc4-00433-g92a355464776 from Linus' tree.

Reverting this patch fixed my issue.

Best regards,
Oliver


On 07.11.23 03:26, Vincent MAILHOL wrote:
> On Tue. 7 Nov. 2023 at 03:02, Maxime Jayat
> <maxime.jayat@mobile-devices.fr> wrote:
>> The TDCO calculation was done using the currently applied data bittiming,
>> instead of the newly computed data bittiming, which means that the TDCO
>> had an invalid value unless setting the same data bittiming twice.
> 
> Nice catch!
> 
> Moving the can_calc_tdco() before the memcpy(&priv->data_bittiming,
> &dbt, sizeof(dbt)) was one of the last changes I made. And the last
> batch of tests did not catch that. Thanks for the patch!
> 
>> Fixes: d99755f71a80 ("can: netlink: add interface for CAN-FD Transmitter Delay Compensation (TDC)")
>> Signed-off-by: Maxime Jayat <maxime.jayat@mobile-devices.fr>
> 
> Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> 
>> ---
>>   drivers/net/can/dev/netlink.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
>> index 036d85ef07f5..dfdc039d92a6 100644
>> --- a/drivers/net/can/dev/netlink.c
>> +++ b/drivers/net/can/dev/netlink.c
>> @@ -346,7 +346,7 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
>>                          /* Neither of TDC parameters nor TDC flags are
>>                           * provided: do calculation
>>                           */
>> -                       can_calc_tdco(&priv->tdc, priv->tdc_const, &priv->data_bittiming,
>> +                       can_calc_tdco(&priv->tdc, priv->tdc_const, &dbt,
>>                                        &priv->ctrlmode, priv->ctrlmode_supported);
>>                  } /* else: both CAN_CTRLMODE_TDC_{AUTO,MANUAL} are explicitly
>>                     * turned off. TDC is disabled: do nothing
>> --
>> 2.34.1
>>
>