Re: [PATCH v2 06/10] can: netlink: add CAN_CTRLMODE_XL_ERR_SIGNAL

[Vincent Mailhol <mailhol@kernel.org>]

On 06/11/2025 at 09:50, Oliver Hartkopp wrote:
> On 21.10.25 17:47, Vincent Mailhol wrote:
>> Classical CAN and CAN FD must generate error frames on the CAN bus
>> when detecting a protocol violation.
>>
>> CAN XL's error signaling is different and works as follows:
>>
>>    - In interoperability mode (both FD and XL), error signaling must be
>>      on.
>>
>>    - When operating a CAN controller in CAN XL only mode but with TMS
>>      off, the user can decide whether the error signalling is enabled
>>      or disabled.
>>
>>    - On the contrary, when using TMS, error signalling must be off.
>>
>> Introduce the new CAN_CTRLMODE_XL_ERR_SIGNAL control mode. This new
>> option is only made available for CAN XL, so despite the error
>> signalling being always on for Classical CAN and CAN FD, forbid the
>> use of this flag when CAN XL is off.
>>
>> If the user provides the error signalling flag, check its validity. If
>> the flag is omitted, activate error signalling by default whenever
>> possible. This is summarized in below table:
>>
>>             CAN_CTRLMODE_XL_ERR_SIGNAL
>>     -------------------------------------------
>>     CC/FD        option not available
>>     CC/FD/XL    on
> 
> Yes. This is the 'mixed-mode'
> I would propose to use the 'mixed-mode' expression in the patch description.

Ack!

>>     XL TMS off    configurable (default on)
> 
> Good default.
> 
>>     XL TMS on    off
>>
>> Suggested-by: Oliver Hartkopp <socketcan@hartkopp.net>
>> Link: https://lore.kernel.org/linux-can/20250527195625.65252-9-
>> socketcan@hartkopp.net/
>> Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
>> ---
>>   drivers/net/can/dev/dev.c        |  2 ++
>>   drivers/net/can/dev/netlink.c    | 29 +++++++++++++++++++++++++++--
>>   include/uapi/linux/can/netlink.h |  1 +
>>   3 files changed, 30 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c
>> index 1de5babcc4f3..0c16d0174f7f 100644
>> --- a/drivers/net/can/dev/dev.c
>> +++ b/drivers/net/can/dev/dev.c
>> @@ -125,6 +125,8 @@ const char *can_get_ctrlmode_str(u32 ctrlmode)
>>           return "xl-tdc-manual";
>>       case CAN_CTRLMODE_XL_TMS:
>>           return "xl-tms";
>> +    case CAN_CTRLMODE_XL_ERR_SIGNAL:
>> +        return "xl-error-signalling";
>>       default:
>>           return "<unknown>";
>>       }
>> diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
>> index 8afd2baa03cf..6126b191fea0 100644
>> --- a/drivers/net/can/dev/netlink.c
>> +++ b/drivers/net/can/dev/netlink.c
>> @@ -191,7 +191,8 @@ static int can_validate_xl_flags(struct netlink_ext_ack
>> *extack,
>>           }
>>           if (masked_flags & CAN_CTRLMODE_XL_TMS) {
>>               const u32 tms_conflicts_mask = CAN_CTRLMODE_FD |
>> -                CAN_CTRLMODE_XL_TDC_MASK;
>> +                CAN_CTRLMODE_XL_TDC_MASK |
>> +                CAN_CTRLMODE_XL_ERR_SIGNAL;
>>               u32 tms_conflicts = masked_flags & tms_conflicts_mask;
>>                 if (tms_conflicts) {
>> @@ -201,11 +202,23 @@ static int can_validate_xl_flags(struct netlink_ext_ack
>> *extack,
>>                   return -EOPNOTSUPP;
>>               }
>>           }
>> +        if ((masked_flags & CAN_CTRLMODE_FD) &&
>> +            (mask & CAN_CTRLMODE_XL_ERR_SIGNAL) &&
>> +            !(masked_flags & CAN_CTRLMODE_XL_ERR_SIGNAL)) {
>> +            NL_SET_ERR_MSG(extack,
>> +                       "When using both CAN FD and XL, error signalling must
>> be on");

I changed that error message to:

	NL_SET_ERR_MSG(extack, "Mixed mode requires error signalling");

> This implicitly tells us that mixed-mode is CC/FD/XL ;-)

I was under the assumption that Classical CAN was always allowed, even under
TMS. The arbitration still uses the nominal bittiming anyway, so I still have
some issue understanding why an XL nodes operating under TMS wouldn't be able to
send a classical CAN frame.

The restriction seems rather arbitrary to me. I would be curious to understand
what the issue would be to allow Classical CAN under TMS.

>  > +            return -EOPNOTSUPP;
>> +        }
>>       } else {
>>           if (mask & CAN_CTRLMODE_XL_TMS) {
>>               NL_SET_ERR_MSG(extack, "TMS requires CAN XL");
>>               return -EOPNOTSUPP;
>>           }
>> +        if (mask & CAN_CTRLMODE_XL_ERR_SIGNAL) {
>> +            NL_SET_ERR_MSG(extack,
>> +                       "Error signalling is only configurable with CAN XL");
>> +            return -EOPNOTSUPP;
>> +        }
>>       }
>>         return 0;
>> @@ -310,6 +323,11 @@ static int can_ctrlmode_changelink(struct net_device *dev,
>>                          "TMS can not be activated while CAN FD is on");
>>               return -EOPNOTSUPP;
>>           }
>> +        if (deactivated & CAN_CTRLMODE_XL_ERR_SIGNAL) {
>> +            NL_SET_ERR_MSG(extack,
>> +                       "Error signalling can not be deactivated while CAN FD
>> is on");
>> +            return -EOPNOTSUPP;
>> +        }
>>       }
>>         /* If a top dependency flag is provided, reset all its dependencies */
>> @@ -317,12 +335,19 @@ static int can_ctrlmode_changelink(struct net_device *dev,
>>           priv->ctrlmode &= ~CAN_CTRLMODE_FD_TDC_MASK;
>>       if (cm->mask & CAN_CTRLMODE_XL)
>>           priv->ctrlmode &= ~(CAN_CTRLMODE_XL_TDC_MASK |
>> -                    CAN_CTRLMODE_XL_TMS);
>> +                    CAN_CTRLMODE_XL_TMS |
>> +                    CAN_CTRLMODE_XL_ERR_SIGNAL);
>>         /* clear bits to be modified and copy the flag values */
>>       priv->ctrlmode &= ~cm->mask;
>>       priv->ctrlmode |= maskedflags;
>>   +    /* If omitted, set error signalling on if possible */
>> +    if ((maskedflags & CAN_CTRLMODE_XL) &&
>> +        !(cm->mask & CAN_CTRLMODE_XL_ERR_SIGNAL) &&
>> +        !(priv->ctrlmode & CAN_CTRLMODE_XL_TMS))
>> +        priv->ctrlmode |= CAN_CTRLMODE_XL_ERR_SIGNAL;
>> +
>>       /* Wipe potential leftovers from previous CAN FD/XL config */
>>       if (!(priv->ctrlmode & CAN_CTRLMODE_FD)) {
>>           memset(&priv->fd.data_bittiming, 0,
>> diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
>> index ebafb091d80f..30d446921dc4 100644
>> --- a/include/uapi/linux/can/netlink.h
>> +++ b/include/uapi/linux/can/netlink.h
>> @@ -108,6 +108,7 @@ struct can_ctrlmode {
>>   #define CAN_CTRLMODE_XL_TDC_AUTO    0x2000    /* XL transceiver
>> automatically calculates TDCV */
>>   #define CAN_CTRLMODE_XL_TDC_MANUAL    0x4000    /* XL TDCV is manually set
>> up by user */
>>   #define CAN_CTRLMODE_XL_TMS        0x8000    /* Transceiver Mode Switching */
>> +#define CAN_CTRLMODE_XL_ERR_SIGNAL    0x10000    /* XL error signalling */
>>     /*
>>    * CAN device statistics
>>
> Reviewed-by: Oliver Hartkopp <socketcan@hartkopp.net>

Thanks!


Yours sincerely,
Vincent Mailhol