Re: Usb to can driver

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 26.04.2013 01:35, Max S. wrote:

> On Wed, 2013-04-24 at 23:24 +0200, Marc Kleine-Budde wrote:
>> On 04/24/2013 07:40 PM, Oliver Hartkopp wrote:
>>>>> assumptions that are made:
>>>>> * The sizes of the struct can_frame members are id:u32 dlc:u8 data:u64 .
>>>>> * It is also assumed that existing defines like CAN_EFF_FLAG in can.h
>>>>> and can/error.h don't change, as they are used by the device to
>>>>> construct the can_id field.
>>>>
>>>> You cannot rely in your firmware, that the struct can_frame and
>>>> CAN_*_FLAG doesn't change. Please define your own struct.
>>>>
>>>
>>>
>>> Hm - i really appreciate the memcopy-only approach which can cope with the
>>> host byte order directly. This is a real improvement on the host side.
>>>
>>> The struct can_frame and the error message content is official Kernel API and
>>> therefore can be assumed to be fix.
>>
>> Yes but no. The struct can_frame to the _userspace_ is official Kernel
>> API/ABI, but within the kernel the is no stable API (see
>> stable_api_nonsense.txt). But if we go this way we should make compile
>> time checks, so that the compilation breaks if the struct can_frame changes.
> 
> ok. tell me if i understood correctly.
> A user-space program can safely assume:
> sizeof((struct can_frame *)NULL)->can_id) == 4
> and:
> CAN_EFF_FLAG == 0x80000000U 
> 
> but a kernel-space driver can not?


The point is, that exposed data structures to the userspace have to be fixed.

Data structures in the kernel might be changed, when it's needed.
BUT: The interface between the network layer (PF_CAN / PF_PACKET) and the
CAN netdevice driver is the transfer of a struct can(fd)_frame.

This interface is not 'guaranteed' to be fix but it will definitely depend on
the exposed data structures for the CAN frame.

> 
> If I cannot make sufficient assumptions about the struct can_frame and
> CAN_*_FLAGs It will make the code harder to maintain in the long run.
> the speed gains derived from a direct memcpy/assignment will later be
> eaten up by the code designed to convert the old struct can_frame
> received from the device to the new struct can_frame found in the
> kernel.
> 
> The question is in the end, where should i draw the line.
> 
> One option would be to define a new struct ss_frame to standardize
> communication between device & host, but still use the device_config to
> allow the changing of byte order.
> The driver would then need to do three assignments to rearrange padding,
> and adjust any difference in the id flags:
> 
> * arrange for correct byte order beforehand...
> 
> can_frame->can_id  = CONVERT_ME_FLAGS(ss_frame->can_id);
> can_frame->can_dlc = ss_frame->can_dlc;
> can_frame->data    = ss_frame->data;
> 
> In this case the 'line' would be explicitly between the struct ss_frame
> and the struct can_frame not floating somewhere vague between the kernel
> and firmware...
> Naturally you will no longer be able to simply:
> *can_frame = frame_wrapper->can_frame;
> As i am doing now.
> 
> What do you think?


Indeed we had the same problem when introducing the CAN FD support last year.

The solution was to define a 'backward compatible' struct canfd_frame, that
allows to transport either a CAN frame and a CAN FD frame in the same data
structure.

See picture in:

http://can-newsletter.org/engineering/standardization/nr_stand_can-fd_linux3.6_120703/ 

The information whether it is a CAN FD frame or not is defined in the length
of the data structure. This structure length information would need to be
transported apart from the struct can(fd)_frame itself.

IMHO using the struct can(fd)_frame can considered to be stable.
The 'stable API nonsense' mainly focuses kernel internal function definitions
and structures that may change in names and variables - or be removed at all.

Struct can(fd)_frame is a CAN specific data structure. And CAN is an
ISO standard that even Linux hackers are not able to change :-)

Regards,
Oliver