Re: [Qemu-devel] [PATCH 0/7] CAN bus support for QEMU (SJA1000 PCI so far)

[Pavel Pisa]

Hello Konrad,

thanks for review.

On Friday 12 of January 2018 11:43:18 KONRAD Frederic wrote:
> You should add that to the title as well:
>
> git format-patch ... --subject-prefix="PATCH V3" ...
>
> to avoid any confusion.

OK, I add V4.

> You need to run the ./scripts/checkpatch.pl on your patches to
> check the coding-style before submitting.

I have run ./scripts/checkpatch.pl on patches but I have
ignored warnings for comments overflowing column 80,
because of better readability, same for

//#define DEBUG_CAN

which seems to be used quite often in the rest of the QEMU
code. But I (try hard) switch to the strict mode for next
version.

I follow all your comments. I have send e-mail to Deniz Eren
to provide his signed-off. I have received plain patches
from him without this line. I trust that they have been
intended to be integrated/released under GPL but
I am not sure if rules allows to add the line myself.

As for the inline can_bus_filter_match

> > +static inline
> > +int can_bus_filter_match(struct qemu_can_filter *filter, qemu_canid_t
> > can_id) +{
> > +    int m;
> > +    if (((can_id | filter->can_mask) & QEMU_CAN_ERR_FLAG)) {
> > +        return (filter->can_mask & QEMU_CAN_ERR_FLAG) != 0;
> > +    }
> > +    m = (can_id & filter->can_mask) == (filter->can_id &
> > filter->can_mask); +    return filter->can_id & QEMU_CAN_INV_FILTER ? !m
> > : m;
> > +}

> Is it really required to inline this?

I agree that it size is on upper size for inline.
My initial version has next form which would be better
to be inlined

int can_bus_filter_match(struct qemu_can_filter *filter, qemu_canid_t can_id)
{
    return (can_id & filter->can_mask) == (filter->can_id & filter->can_mask);
}

but I decided then to add more SocketCAN like behavior
to can use it for CAN bus clients filters list in the future.
Function call cost is not so high today.
The function can be sometimes optimized to basic form when
it is inlined and actual masks computation eliminates
some cases, but again branch predictor should take care
about that.

> > +void can_sja_hardware_reset(CanSJA1000State *s)

> Is something able to reset the chip outside?

I have found and supported more SJA1000 based
CAN cards with reset option during my LinCAN driver
development in the past. The card have special
register or address which allows to activate
RESET pin of SJA1000. So even that reset cannot
be activated externally on currently supported cards,
I would keep that function available for future
boards emulation if there is no objection.

Best wishes,

Pavel