Re: [Xenomai-core] [PATCH] RT-Socket-CAN, TX loopback and further improvements

[Wolfgang Grandegger <wg@domain.hid>]

Jan Kiszka wrote:
> Wolfgang Grandegger wrote:
>>>> @@ -894,6 +937,12 @@
>>>>      /* We got access */
>>>>  
>>>>  
>>>> +#ifdef CONFIG_XENO_DRIVERS_RTCAN_TX_LOOPBACK
>>>> +    /* Push message onto stack for loopback when TX done */
>>>> +    if (sock->tx_loopback)
>>>> +    rtcan_tx_push(dev, sock, frame);
>>>> +#endif /* CONFIG_XENO_DRIVERS_RTCAN_TX_LOOPBACK */
>>> Hmm, I would prefer to define something like
>>>
>>> if (rtcan_tx_loopback_enabled(sock))
>>>     rtcan_tx_push(dev, sock, frame);
>>>
>>> with rtcan_tx_loopback_enabled resolving to 0 in the configured-out
>>> case. Thus some #ifdefs could be moved from the code to central places
>>> in header files.
>> OK, here I can avoid the #ifdef's ...
>>
>>>> Index: ksrc/drivers/can/mscan/rtcan_mscan.c
>>>> ===================================================================
>>>> --- ksrc/drivers/can/mscan/rtcan_mscan.c    (revision 1834)
>>>> +++ ksrc/drivers/can/mscan/rtcan_mscan.c    (working copy)
>>>> @@ -251,6 +251,21 @@
>>>>      regs->cantier = 0;
>>>>      /* Wake up a sender */
>>>>      rtdm_sem_up(&dev->tx_sem);
>>>> +
>>>> +#ifdef CONFIG_XENO_DRIVERS_RTCAN_TX_LOOPBACK
>>>> +    if (dev->tx_socket) {
>>> Same here. A macro like rtcan_tx_loopback_pending(dev) would avoid the
>>> #ifdef.
>> ...but not here. Or does the optimizer remove the if block?
> 
> AFAIK, all relevant gcc versions kill blocks like
> 
> if (0) {
> 	...
> }
> 
> from the binary. But one problem might appear: the block content must be
> compilable in any case...

I'm going to checks this.

>>> I think the timestamp should rather be passed to rtcan_tc_loopback and
>>> set there. Or, if passing that value increases the code size needlessly,
>>> rtcan_tx_loopback should be defined like
>>>
>>> static inline void
>>> rtcan_tx_loopback(struct rtcan_device *dev, nanosecs_abs_t timestamp)
>>> {
>>>     <set timestamp>
>>>     __rtcan_tx_loopback(dev);
>>> }
>>>
>>> where __rtcan_tx_loopback is the uninlined code. That makes the driver
>>> code leaner. Locking will hopefully change anyway, so nothing to do on
>>> this part for now.
>> Or do it directly in rtcan_tx_loopback() and rtcan_recv(). Would it be
>> OK to read the time in these functions as well or should it be done, as
>> is, at the beginning of the ISR? I have already planned some similar
>> rewrite when Xenomai 2.3. is out, hopefully together with the new locking.
> 
> A few cycles offset of the timestamp doesn't matter here. We should just
> avoid reading it more than once as this can be a costly operation on
> some systems.

Hm, what does "costly operation" mean? Does it make sense then to read 
the time only when really necessary e.g. not for pure TX done interrupts 
and if nobody request them?

Wolfgang.

> Jan
>