Page MenuHomeFreeBSD

linuxkpi: Fix mod_timer and del_timer_sync
ClosedPublic

Authored by manu on May 24 2020, 4:11 PM.

Details

Summary

mod_timer is supposed to return 1 if the modified timer was pending, which
is exactly what callout_reset does so return this value directly.
The only not handled case is when callout_reset fails, for now returns 0 as if the timer
wasn't modified.
del_timer_sync returns int so add a function and handle that.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

manu requested review of this revision.May 24 2020, 4:11 PM
sys/compat/linuxkpi/common/include/linux/timer.h
86 ↗(On Diff #72189)

This is wrong. callout_drain() is not the same like callout_stop() as used by del_timer(). Please read the callout implementation carefully.

sys/compat/linuxkpi/common/src/linux_compat.c
1911 ↗(On Diff #72189)

Please read the callout_reset() code that you return exactly what you want!

sys/compat/linuxkpi/common/include/linux/timer.h
86 ↗(On Diff #72189)

oops sorry, I'll change that.

sys/compat/linuxkpi/common/src/linux_compat.c
1911 ↗(On Diff #72189)

?

sys/compat/linuxkpi/common/src/linux_compat.c
1911 ↗(On Diff #72189)

From what I know callout functions can return 1,0,-1. Are you sure callout reset only returns 1/0 ?

sys/compat/linuxkpi/common/src/linux_compat.c
1911 ↗(On Diff #72189)

Oh mhm, right, I guess I should just ignore <0 like we do now.
I'll change that.

manu edited the summary of this revision. (Show Details)
manu marked 2 inline comments as done.May 25 2020, 8:57 AM
sys/compat/linuxkpi/common/src/linux_compat.c
1908 ↗(On Diff #72220)

Callout reset never fails. Please remove or update this comment.

Either it:
starts timer from clear (0)
or re-starts timer (1)

MPASS(ret == 0 || ret == 1);

return (ret == 1);

sys/compat/linuxkpi/common/src/linux_compat.c
1908 ↗(On Diff #72220)

So my original code was right ?

sys/compat/linuxkpi/common/src/linux_compat.c
1908 ↗(On Diff #72220)

Yes, looks like, except for only testing ret == 1.

Add an assert for the sake of future changes in callout APIs.

Beware that older versions of FreeBSD behave different w.r.t the return values from FreeBSD callout APIs. Be careful when MFC-ing to 11 and older.

This revision is now accepted and ready to land.May 25 2020, 9:41 AM
This revision was automatically updated to reflect the committed changes.