AW: [PATCH v2 05/12] kern_tc.c: Replace FREEBSD event mechanism by adding pointers to function

[Sebastian Huber]

On 04/05/2022 08:05, Gabriel.Moyano at dlr.de wrote:
>> On 29/04/2022 09:20, Gabriel Moyano wrote:
>>> ---
>>>    cpukit/include/sys/timepps.h |  6 ++++++
>>>    cpukit/score/src/kern_tc.c   | 25 +++++++++++++++++++++++++
>>>    2 files changed, 31 insertions(+)
>>>
>>> diff --git a/cpukit/include/sys/timepps.h
>>> b/cpukit/include/sys/timepps.h index 5703381ffa..0d666a4f2e 100644
>>> --- a/cpukit/include/sys/timepps.h
>>> +++ b/cpukit/include/sys/timepps.h
>>> @@ -26,6 +26,7 @@
>>>    #include <sys/time.h>
>>>    #ifdef __rtems__
>>>    #include <rtems/score/atomic.h>
>>> +#include <rtems/rtems/types.h>
>>>    #endif /* __rtems__ */
>>>
>>>    #define PPS_API_VERS_1	1
>>> @@ -164,6 +165,11 @@ struct pps_state {
>>>    	int		ppscap;
>>>    	struct timecounter *ppstc;
>>>    	unsigned	ppscount[3];
>>> +#ifdef __rtems__
>>> +    rtems_id task_waiting;
>> You don't need task_waiting, this is an implementation detail of potential wait/wakeup implementations.
> What do you think here of replacing it with a void* because some variable is required to save the tasks waiting

You probably need device-specific state anyway, so you can use

struct my_pps_device {
   struct pps_state pps;
   rtems_id task_waiting;
};

and

dev = RTEMS_CONTAINER_OF( state, struct my_pps_device, pps );

> 
>>> +    int (*wait)(struct pps_state *pps, struct timespec timeout); /* Wait for an event. Called internally when time_pps_fetch() is
>> used. It shall not be NULL*/
>>> +    void (*wakeup)(struct pps_state *pps); /* Used to wakeup tasks
>>> + waiting for an event. It shall not be NULL*/
>> Please fix the formatting and take care that you keep the line limit.
>>
>>> +#endif /* __rtems__ */
>>>    	/*
>>>    	 * The following fields are valid if the driver calls pps_init_abi().
>>>    	 */
>>> diff --git a/cpukit/score/src/kern_tc.c b/cpukit/score/src/kern_tc.c
>>> index f7d0a0b4ba..76e3e056de 100644
>>> --- a/cpukit/score/src/kern_tc.c
>>> +++ b/cpukit/score/src/kern_tc.c
>>> @@ -1917,9 +1917,15 @@ abi_aware(struct pps_state *pps, int vers)
>>>    static int
>>>    pps_fetch(struct pps_fetch_args *fapi, struct pps_state *pps)
>>>    {
>>> +#ifndef __rtems__
>>>    	int err, timo;
>>> +#else /* __rtems__ */
>>> +	int err;
>>> +#endif /* __rtems__ */
>>>    	pps_seq_t aseq, cseq;
>>> +#ifndef __rtems__
>>>    	struct timeval tv;
>>> +#endif /* __rtems__ */
>>>
>>>    	if (fapi->tsformat && fapi->tsformat != PPS_TSFMT_TSPEC)
>>>    		return (EINVAL);
>>> @@ -1932,6 +1938,7 @@ pps_fetch(struct pps_fetch_args *fapi, struct pps_state *pps)
>>>    	 * sleep a long time.
>>>    	 */
>>>    	if (fapi->timeout.tv_sec || fapi->timeout.tv_nsec) {
>>> +#ifndef __rtems__
>>>    		if (fapi->timeout.tv_sec == -1)
>>>    			timo = 0x7fffffff;
>>>    		else {
>>> @@ -1939,10 +1946,12 @@ pps_fetch(struct pps_fetch_args *fapi, struct pps_state *pps)
>>>    			tv.tv_usec = fapi->timeout.tv_nsec / 1000;
>>>    			timo = tvtohz(&tv);
>>>    		}
>>> +#endif /* __rtems__ */
>>>    		aseq = atomic_load_int(&pps->ppsinfo.assert_sequence);
>>>    		cseq = atomic_load_int(&pps->ppsinfo.clear_sequence);
>>>    		while (aseq == atomic_load_int(&pps->ppsinfo.assert_sequence) &&
>>>    		    cseq == atomic_load_int(&pps->ppsinfo.clear_sequence)) {
>>> +#ifndef __rtems__
>>>    			if (abi_aware(pps, 1) && pps->driver_mtx != NULL) {
>>>    				if (pps->flags & PPSFLAG_MTX_SPIN) {
>>>    					err = msleep_spin(pps, pps->driver_mtx, @@ -1954,6 +1963,12 @@
>>> pps_fetch(struct pps_fetch_args *fapi, struct pps_state *pps)
>>>    			} else {
>>>    				err = tsleep(pps, PCATCH, "ppsfch", timo);
>>>    			}
>>> +#else /* __rtems__ */
>>> +            if (pps->wait != NULL)
>>> +                err = (*pps->wait)(pps, fapi->timeout);
>>> +            else
>>> +                err = EAGAIN;
>>> +#endif /* __rtems__ */
>> Replace the NULL check with an _Assert().  Check in pps_init() that the handler are not NULL, otherwise generate a new fatal error.
>> You need tests for the fatal errors.
> Actually the synchronization can work without these functions; wait() is called in pps_time_fetch() and wakeup() wakes up the waiting task(s). This is also why there is one _Assert() in pps_init() but if you consider better to add other for wait(), it can be done.

This "if" doubles the testing effort in this somewhat complex function. 
Just provide wait/wake default implementations which do nothing. This 
approach is used elsewhere in RTEMS as well.

-- 
embedded brains GmbH
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.huber at embedded-brains.de
phone: +49-89-18 94 741 - 16
fax:   +49-89-18 94 741 - 08

Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/