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

[Jan Kiszka <jan.kiszka@domain.hid>]

[-- Attachment #1: Type: text/plain, Size: 2529 bytes --]

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 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.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 249 bytes --]