AW: [PATCH v3 04/11] kern_tc.c: Replace FREEBSD event mechanism by adding pointers to function
Gabriel.Moyano at dlr.de
Gabriel.Moyano at dlr.de
Fri May 20 08:02:55 UTC 2022
Thx, I'll send a new version of the commits today. The issue in the NTP test will also be resolved.
> FREEBSD should be FreeBSD.
>
> On 04/05/2022 14:12, Gabriel Moyano wrote:
> > ---
> > cpukit/include/sys/timepps.h | 21 ++++++++++++++++++++
> > cpukit/score/src/kern_tc.c | 38 ++++++++++++++++++++++++++++++++++++
> > 2 files changed, 59 insertions(+)
> >
> > diff --git a/cpukit/include/sys/timepps.h
> > b/cpukit/include/sys/timepps.h index 5703381ffa..b734c6f841 100644
> > --- a/cpukit/include/sys/timepps.h
> > +++ b/cpukit/include/sys/timepps.h
> > @@ -164,6 +164,27 @@ struct pps_state {
> > int ppscap;
> > struct timecounter *ppstc;
> > unsigned ppscount[3];
> > +#ifdef __rtems__
> > + /**
> > + * @brief Wait for an event.
> > + *
> > + * Called internally when time_pps_fetch() is used.
> > + * It is initialized with an empty function by default.
>
> initialized by pps_init() to a handler which just returns 0?
>
> Is 0 a good default? You will end up in an endless loop I guess. Why not return ETIMEDOUT by default? Please add a test case for this
> scenario.
>
> > + *
> > + * @param pps is the pointer to the object.
> > + *
> > + * @param timeout
> > + *
> > + * @return 0 if no error otherwise a negative value.
>
> Please list all valid status codes. Probably:
>
> @retval 0 A wakeup event was received.
>
> @retval ETIMEDOUT A timeout occurred while waiting for the event.
>
> > + */
> > + int (*wait)(struct pps_state *pps, struct timespec timeout);
>
> Add a blank line here.
>
> Please review the entire patch set so that all changes are done in the coding style of the original file. For the details see:
>
> https://www.freebsd.org/cgi/man.cgi?query=style&sektion=9
>
> In your patches this is mostly space vs. tabs.
>
> > + /**
> > + * @brief Wakeup the tasks waiting for an event.
> > + *
> > + * @param pps is the pointer to the object.
> > + */
> > + void (*wakeup)(struct pps_state *pps); #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..6ca408e4ab 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,10 @@
> > pps_fetch(struct pps_fetch_args *fapi, struct pps_state *pps)
> > } else {
> > err = tsleep(pps, PCATCH, "ppsfch", timo);
> > }
> > +#else /* __rtems__ */
> > + _Assert(pps->wait != NULL);
> > + err = (*pps->wait)(pps, fapi->timeout); #endif /*
> > +__rtems__ */
> > if (err == EWOULDBLOCK) {
> > if (fapi->timeout.tv_sec == -1) {
> > continue;
>
> I would remove the err == EWOULDBLOCK ... block if you remove the block above, since the wait handler has to deal with the
> timeout.tv_sec == -1 condition in this case.
>
> > @@ -2058,9 +2071,29 @@ pps_ioctl(u_long cmd, caddr_t data, struct pps_state *pps)
> > }
> > }
> >
> > +#ifdef __rtems__
> > +static int
> > +default_wait(struct pps_state *pps, struct timespec timeout) {
>
> Add a blank line.
>
> > + (void) pps;
>
> (void)pps;
>
> See FreeBSD style guide.
>
> > + (void) timeout;
> > +
> > + return 0;
>
> return (0);
>
> > +}
> > +
> > +static void
> > +default_wakeup(struct pps_state *pps) {
> > + (void) pps;
> > +}
> > +#endif /* __rtems__ */
> > void
> > pps_init(struct pps_state *pps)
> > {
> > +#ifdef __rtems__
> > + pps->wait = default_wait;
> > + pps->wakeup = default_wakeup;
> > +#endif /* __rtems__ */
> > pps->ppscap |= PPS_TSFMT_TSPEC | PPS_CANWAIT;
> > if (pps->ppscap & PPS_CAPTUREASSERT)
> > pps->ppscap |= PPS_OFFSETASSERT;
> > @@ -2227,7 +2260,12 @@ pps_event(struct pps_state *pps, int event)
> > #endif
> >
> > /* Wakeup anyone sleeping in pps_fetch(). */
> > +#ifndef __rtems__
> > wakeup(pps);
> > +#else /* __rtems__ */
> > + _Assert(pps->wakeup != NULL);
> > + (*pps->wakeup)(pps);
> > +#endif /* __rtems__ */
> > }
> > #else /* __rtems__ */
> > /* FIXME: https://devel.rtems.org/ticket/2349 */
>
More information about the devel
mailing list