LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Al Viro <viro@ZenIV.linux.org.uk>
To: Rusty Russell <rusty@rustcorp.com.au>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	linux-kernel@vger.kernel.org, Jeff Garzik <jeff@garzik.org>
Subject: Re: [PATCH 5/5] typesafe: TIMER_INITIALIZER and setup_timer
Date: Thu, 6 Mar 2008 10:40:35 +0000	[thread overview]
Message-ID: <20080306104034.GB27894@ZenIV.linux.org.uk> (raw)
In-Reply-To: <200803051355.36502.rusty@rustcorp.com.au>

On Wed, Mar 05, 2008 at 01:55:36PM +1100, Rusty Russell wrote:

> AFAICT you posted an untested code fragment which won't actually compile 
> because the callback in this case returns void.

Callbacks for timers *do* return void.  FWIW,

extern void decay_to_pointer(void const volatile *);
#define check_callback_type(f,x) (sizeof((0 ? decay_to_pointer(x),(f)(x) : (void)0), 0))
#define __setup_timer(timer, func, arg) \
do {    \
	struct timer_list *__timer = (timer);   \
	__timer->function = (void (*)(unsigned long))(func); \
	__timer->data = (unsigned long)(arg);   \
	(void)check_callback_type(func, arg); \
} while(0)

#define setup_timer(timer, func, arg) \
do {    \
	struct timer_list *__timer = (timer);   \
	__timer->function = (void (*)(unsigned long))(func); \
	__timer->data = (unsigned long)(arg);   \
	init_timer(__timer);    \
	(void)check_callback_type(func, arg); \
} while(0)

#define DECLARE_TIMER(_name, _function, _data) \
        struct timer_list _name = { \
                .base = &boot_tvec_bases, \
                .function = (void (*)(unsigned long))(_function), \
                .data = (unsigned long)(_data) + \
                        0 * check_callback_type(_function, _data) \
        }

is what the timer series ended up using.  Note that this is _after_
conversion of setup_timer() users to natural argument types.  And yes,
it went timer-by-timer.
 
> > > ; normal C constructs are quite sufficient, TYVM.
> 
> Your code didn't check the return type, except that it's a pointer or 
> int-compatible.

Huh?  It certainly does _not_ accept return of int or pointer - ? : with
(void) 0 in the other branch is there to give a warning on those.  Note
that any timer callback that does anything of that kind is simply bogus -
there is nothing the caller could do about whatever return values we might
have, anyway.

> We can do better, as this patch showed.

> If you have a "saner way" of doing this in general which is just as effective, 
> I look forward to it.  Otherwise you're just managed to delay these 
> improvements for another release.

See above.  The point is, we don't bloody need to convert them all at once
and we can eliminate void (unsigned long) ones as we go.

Let me put it that way: if you actually look at these suckers, you'll see
that there's less than a dozen instances that actually want an integer.
Out of several hundreds.  And out of those only two, IIRC, were not trivially
converted by "pass pointer to array element instead of index in array".
Something in arch/sparc and arch/sparc64 - I can dig the details out if
you wish, but (a) it'll obviously need search from scratch to verify that
new pathology didn't show up and (b) it's extremely unlikely that the total
amount had become considerable.

So for timer callbacks the right answer is "just convert them to pointers,
these two cases are not worth the trouble".  And we _can_ do that in bisectable
way, TYVM - it's not that we can't do conversion stepwise.

A _very_ long series (we've a lot of these suckers) and I had two of its
incarnations bitrot on me by now, but it's not particulary tricky.

FWIW, I've finally rescued another bitrot victim (bdev API conversion and
related cleanups and fixes; sent to Jens for review last week, might be
actually mergable next cycle), so the timer one is near the top of the pile
right now; I can certainly bump it up and post the infrastructure + several
hundred instances converted by the next week.  Finishing vfs-2.6 tree should
take a couple of days (there are several _old_ pending mini-series that might
go there; currently the tree on kernel.org still has only immediate fixes +
ro-bind, all under vfs-fixes.b1 / ro-bind.b1), then I'm free for that...

PS: apologies for missing your previous mail; ETOOBIGMBOX ;-/

  reply	other threads:[~2008-03-06 10:40 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-04 12:11 [PATCH 0/5] typesafe callbacks Rusty Russell
2008-02-04 12:14 ` [PATCH 1/5] cast_if_type: allow macros functions which take more than one type Rusty Russell
2008-02-04 12:16   ` [PATCH 2/5] typesafe: Convert stop_machine Rusty Russell
2008-02-04 12:17     ` [PATCH 3/5] typesafe: kthread_create and kthread_run Rusty Russell
2008-02-04 12:18       ` [PATCH 4/5] typesafe: request_irq and devm_request_irq Rusty Russell
2008-02-04 12:19         ` [PATCH 5/5] typesafe: TIMER_INITIALIZER and setup_timer Rusty Russell
2008-02-04 14:57           ` Al Viro
2008-02-05  3:41             ` Rusty Russell
2008-03-05  2:55               ` Rusty Russell
2008-03-06 10:40                 ` Al Viro [this message]
2008-03-10  1:07                   ` Rusty Russell
2008-03-10  2:03                     ` Al Viro
2008-03-10  3:42                       ` Rusty Russell

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20080306104034.GB27894@ZenIV.linux.org.uk \
    --to=viro@zeniv.linux.org.uk \
    --cc=jeff@garzik.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=rusty@rustcorp.com.au \
    --cc=torvalds@linux-foundation.org \
    --subject='Re: [PATCH 5/5] typesafe: TIMER_INITIALIZER and setup_timer' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).