Re: ni_usb: bug when setting kernel timer

I've succeeded in compiling the kernel portion now. I'm far more familiar
with C++ than C so here's my findings and I hope someone can take it the
last mile
The timer_setup function is already defined in 4.14 so it needs to be
surrounded by a LINUX_VERSION_CODE < KERNEL_VERSION(4, 14, 0) check.

The issue is that the timer_list function prototype is:
// Kernel <= 4.14
void            (*function)(unsigned long);
// Kernel > 4.14
void            (*function)(struct timer_list *);

But that timer_setup
// Kernel < 4.14
Supplied by compat layer
timer_setup(struct timer_list *timer, void (*func)(COMPAT_TIMER_ARG_TYPE),
unsigned int flags)
// Kernel >= 4.14
timer_setup(struct timer_list *timer, void (*callback)(struct timer_list
*), unsigned int flags)

So < 4.14 and > 4.14 are clear straightforward. 4.14 is the edge case where
the timer_list function prototype does not match timer_setup.

The GPIB code is inconsistent in how the timer functions are used. For 4.14
COMPAT_TIMER_ARG_TYPE needs to be (struct timer_list *) in most cases, but
pseudo_irq_handler and watchdog_timeout are both directly assigned to a
timer_list function so need to have an unsigned long as their function
argument. This is the part I'm not sure how to most cleanly resolve. I've
attached my patch which is ok except for the pseudo_irq_handler and
watchdog_timeout function prototypes being hardcoded to unsigned long.

Regards,
Derek


On Mon, May 28, 2018 at 9:41 PM, Derek Kozel <[email protected]> wrote:

> My change to the kernel version requirement was incorrect. Kernel 4.14.0
> has unsigned long as the timer_list function argument type.
>
> https://elixir.bootlin.com/linux/v4.14/source/include/linux/timer.h#L20
> https://sourceforge.net/p/linux-gpib/code/1752/tree//
> trunk/linux-gpib-kernel/compat/include/linux/timer.h#l29
>
> I'll look more into the redefinition error. Comparing 4.13, 4.14, and 4.15
> declarations of setup_timer it looks like there were a few changes between
> each so I maybe just particularly unlucky in kernel versions.
>
> https://elixir.bootlin.com/linux/v4.13/source/include/linux/timer.h#L154
> https://elixir.bootlin.com/linux/v4.14/source/include/linux/timer.h#L175
> https://elixir.bootlin.com/linux/v4.15/source/include/linux/timer.h#L131
>
> Regards,
> Derek
>
> On Mon, May 28, 2018 at 7:49 PM, Derek Kozel <[email protected]>
> wrote:
>
>> Hello,
>>
>> I've just had a chance to try the newly separated source trees and
>> encountered an error with the setup_timer compat header. The make log is
>> attached as linux-gpib-r1752-setup_timer-error.txt.
>>
>> I'm running Ubuntu 16.04 with kernel 4.14.0-041400-generic. I have SVN
>> rev 1752.
>>
>> As the error was setup_timer being redefined I changed the kernel version
>> check to < 4.14 and attempted to recompile. Many drivers succeeded, but
>> then I encountered an incompatible pointer type error. The log for that is
>> also attached, I reran make after the first error to abbreviate the output.
>> My guess is that I'll have to try understanding the code a bit more to fix
>> this error, but I'm also slightly surprised it hasn't been encountered by
>> anyone else. Can anyone shed light on it?
>>
>> Many thanks,
>> Derek
>>
>> On Tue, May 8, 2018 at 3:41 PM, Frank Mori Hess <[email protected]> wrote:
>>
>>> On Mon, May 7, 2018 at 10:46 AM, Stefan Mahr <[email protected]>
>>> wrote:
>>> > Hi,
>>> >
>>> > There's a timer initialisation bug in ni_usb driver, especially
>>> > for NI_USB_B. The timer is setup after first use of mod_timer
>>> > (ni_usb_b_read_serial_number->usb_send_bulk_msg), so kernel throws a
>>> > bug in timer.c: BUG_ON(!timer->function);
>>> >
>>> > Attached patch should fix this issue.
>>> >
>>>
>>> Thanks, I'm going to try implementing timer_setup using setup_timer in
>>> a compatibility header later today, to get rid of needing to #ifdef
>>> every timer setup.  I'll incorporate your change into that.
>>>
>>>
>>> --
>>> Frank
>>>
>>> ------------------------------------------------------------
>>> ------------------
>>> Check out the vibrant tech community on one of the world's most
>>> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>>> _______________________________________________
>>> Linux-gpib-general mailing list
>>> [email protected]
>>> https://lists.sourceforge.net/lists/listinfo/linux-gpib-general
>>>
>>
>>
>

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

_______________________________________________
Linux-gpib-general mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-gpib-general

Index: compat/include/linux/timer.h
===================================================================
--- compat/include/linux/timer.h	(revision 1752)
+++ compat/include/linux/timer.h	(working copy)
@@ -24,12 +24,13 @@
 
 #include_next <linux/timer.h>
 
-#if LINUX_VERSION_CODE < KERNEL_VERSION(4, 15, 0)
-
+#if LINUX_VERSION_CODE < KERNEL_VERSION(4, 14, 0)
 #define COMPAT_TIMER_ARG_TYPE unsigned long
-#define COMPAT_FROM_TIMER(var, callback_timer, timer_fieldname) \
-	container_of((struct timer_list *)(callback_timer), typeof(*var), timer_fieldname)
+#else
+#define COMPAT_TIMER_ARG_TYPE struct timer_list *
+#endif
 
+#if LINUX_VERSION_CODE < KERNEL_VERSION(4, 14, 0)
 static inline void timer_setup(struct timer_list *timer,
 	void (*func)(COMPAT_TIMER_ARG_TYPE), unsigned int flags)
 {
@@ -38,6 +39,13 @@
 	
 	setup_timer(timer, func, (COMPAT_TIMER_ARG_TYPE)timer);
 }
+#endif
+
+#if LINUX_VERSION_CODE < KERNEL_VERSION(4, 15, 0)
+
+#define COMPAT_FROM_TIMER(var, callback_timer, timer_fieldname) \
+	container_of((struct timer_list *)(callback_timer), typeof(*var), timer_fieldname)
+
 #define timer_setup_on_stack(timer, func, flags) timer_setup(timer, func, flags)
 #define destroy_timer_on_stack(timer)
 
Index: drivers/gpib/sys/ostimer.c
===================================================================
--- drivers/gpib/sys/ostimer.c	(revision 1752)
+++ drivers/gpib/sys/ostimer.c	(working copy)
@@ -23,7 +23,7 @@
 
 /* Watchdog timeout routine */
 
-void watchdog_timeout( COMPAT_TIMER_ARG_TYPE t )
+void watchdog_timeout( unsigned long t )
 {
 	gpib_board_t *board = COMPAT_FROM_TIMER(board, t, timer);
 	smp_mb__before_atomic();
Index: drivers/gpib/sys/osutil.c
===================================================================
--- drivers/gpib/sys/osutil.c	(revision 1752)
+++ drivers/gpib/sys/osutil.c	(working copy)
@@ -49,7 +49,7 @@
 	return (HZ + 99) / 100;
 }
 
-void pseudo_irq_handler(COMPAT_TIMER_ARG_TYPE t)
+void pseudo_irq_handler(unsigned long t)
 {
 	struct gpib_pseudo_irq *pseudo_irq = COMPAT_FROM_TIMER(pseudo_irq, t, timer);
 	if(pseudo_irq->handler)