Re: [Qemu-devel] [PATCH V4 2/7] CAN bus support to connect bust to Linux host SocketCAN interface.

[Pavel Pisa]

Hello Philippe,

On Tuesday 16 of January 2018 01:12:09 Philippe Mathieu-Daudé wrote:
> On 01/15/2018 06:29 PM, Pavel Pisa wrote:
> > Hello Philippe,
> >
> > thanks for review.
> >
> > I have updated patch series in can-pci branch in
> >
> >   https://gitlab.fel.cvut.cz/canbus/qemu-canbus
> >
> > I would wait some day if there is some remark
> > from other developers and socket ones especially.
>
> I'll have a look.

if there are no more comments, I prepare version 5
of the patches at start of the next week.

Already implemented changes for version 5:

Assertions to check match to Linux struct can_frame
has been moved out of the function.
The case statements use ranges.
min_access_size=1 removed.
"for loops" are used to process operations
for the first and the second chip where
appropriate in PCM-3680I and MIOe-3680 patches.
Deniz Eren reported that updated version
works correctly.

I have not proceed with next two remarks yet:

Inlining in can_sja_single_filter and can_sja_dual_filter
functions. There is only single group of repeated four lines
of code which could be inlined, others are groups of two
lines each. 

>     if (extended) {
vvvvvvvvvvvvvvv
>         filter->can_id = (uint32_t)acr[0] << 21;
>         filter->can_id |= (uint32_t)acr[1] << 13;
>         filter->can_id |= (uint32_t)acr[2] << 5;
>         filter->can_id |= (uint32_t)acr[3] >> 3;
^^^^^^^^^^^^^^^
>         if (acr[3] & 4) {
>             filter->can_id |= QEMU_CAN_RTR_FLAG;
>         }
>
vvvvvvvvvvvvvvv
>         filter->can_mask = (uint32_t)amr[0] << 21;
>         filter->can_mask |= (uint32_t)amr[1] << 13;
>         filter->can_mask |= (uint32_t)amr[2] << 5;
>         filter->can_mask |= (uint32_t)amr[3] >> 3;
^^^^^^^^^^^^^^^
>         filter->can_mask = ~filter->can_mask & QEMU_CAN_EFF_MASK;
>         if (!(amr[3] & 4)) {
>             filter->can_mask |= QEMU_CAN_RTR_FLAG;
>         }

Only these marked parts repeats.
Rest is similar but not material for simple inline.
Same for others two line sequences.
All these bit level manipulations are moved from frame
processing and another more top level functions into these
short can_sja_{single,dual}_filter functions.

> /* PeliCAN mode */
> enum SJA1000_PeliCAN_regs {
>         SJA_MOD      = 0x00,
> /* Command register */
>         SJA_CMR      = 0x01,
....
> /* TX Error Counter */
>         SJA_TXERR0   = 0x0e,
>         SJA_TXERR1   = 0x0f,
> /* Rx Message Counter (number of msgs. in RX FIFO */
>         SJA_RMC      = 0x1d,
> /* Rx Buffer Start Addr. (address of current MSG) */
>         SJA_RBSA     = 0x1e,
> /* Transmit Buffer (write) Receive Buffer (read) Frame Information */
>         SJA_FRM      = 0x10,
> /*
>  * ID bytes (11 bits in 0 and 1 for standard message or
>  *          16 bits in 0,1 and 13 bits in 2,3 for extended message)
>  */
>         SJA_ID0      = 0x11, SJA_ID1 = 0x12,
> /* ID cont. for extended frames */
>         SJA_ID2      = 0x13, SJA_ID3 = 0x14,

Proposed formating

           SJA_MOD      = 0x00, /* Command */
           SJA_CMR      = 0x01, /* Status */

this is really better for short comments.
But how format long ones then to keep lines max 80 characters

           SJA_FRM      = 0x10, /* Transmit Buffer (write) Receive Buffer
                                 * (read) Frame Information
                                 */

The SJA_ID0 is would look even worse.

But if the second format is preferred then I update the patch.

> > Trace events seems as nice feature. But simple text printf
> > like output has been enough till now for CAN testing.
> > There is no debugging output in production build.
> > So I would add tracing infrastructure later if needed.
>
> They are as useful as console printf, but less invasive and more
> powerful: you can use a much precise timing, select which set of events
> to display without having to recompile.
> The default backend behaves as console printf.
>
> You should try them :)

I have tried them on 

  pci_update_mappings_del
  pci_update_mappings_add
  pci_cfg_write

and they work great. They would be nice for SJA1000
register accesses, PCI boards configuration etc.
I am not sure how to use them for CAN messages
which has a variable length data field.
Any of debugging outputs is active (all is optimized out)
in regular build.

> >>> +    /* open socket */
> >>> +    s = socket(PF_CAN, SOCK_RAW, CAN_RAW);
> >>
> >> I never used it, but I think QEMU uses his socket API: "qemu/sockets.h"
> >
> > The SocketCAN host connection code is Linux specific,
> > but I can switch to qemu_socket() if it is preferred.
> > But address family has to be from Linux header file anyway.
>
> qemu_socket() sockets are heavily tested and already solve many things,
> like async I/O and error handling.

The CAN socket works with RAW packets read and written as 16 bytes.
So some higher level of optimizations used for stream or larger packets
have no significant effect there and SocketCAN API is Linux kernel
specific so generic abstraction layers has no effect there as well.

I have question about reporting fatal error.

Our code uses error_report() and exit(1) for now.

        error_report("CAN host device \"%s\" connect to bus \"%s\" failed",
                      host_dev_name, bus->name);
        exit(1);

Is that correct or there is better solution/function instead of exit?
I have seen this combination in many other places of QEMU code
in past but may it be there is some proposed change already.

Best wishes,

Pavel Pisa