Re: [PATCH] hpet: factor timer allocate from open

From: Andrew Morton
Date: Mon Mar 22 2010 - 18:21:57 EST


On Thu, 18 Mar 2010 21:06:58 -0700 (PDT)
Magnus Lynch <maglyx@xxxxxxxxx> wrote:

> Andrew Morton wrote:
> > Please always retain and maintain the changelog with each version of a patch.
> >
> > Please resend this patch with a complete changelog.
>
> OK, here's my description from the original posting:
> <<
> The current implementation of the /dev/hpet driver couples opening the
> device with allocating one of the (scarce) timers (aka comparators).
> This is a limitation in that the main counter may be valuable to
> applications seeking a high-resolution timer who have no use for the
> interrupt generating functionality of the comparators.
>
> This patch alters the open semantics so that when the device is
> opened, no timer is allocated. Operations that depend on a timer being
> in context implicitly attempt allocating a timer, to maintain backward
> compatibility. There is also an IOCTL (HPET_ALLOC_TIMER _IO) added so
> that the allocation may be done explicitly. (I prefer the explicit
> open then allocate pattern but don't know how practical it would be to
> require all existing code to be changed.)

A stylistic nit:

> @@ -384,6 +408,10 @@ static int hpet_fasync(int fd, struct file *file, int on)
> {
> struct hpet_dev *devp;
>
> + int r = hpet_alloc_timer(file);
> + if (r < 0)
> + return r;
> +
> devp = file->private_data;
>
> if (fasync_helper(fd, file, on, &devp->hd_async_queue) >= 0)
>
> ...
>
> @@ -438,6 +469,10 @@ hpet_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
> {
> struct hpet_dev *devp;
>
> + int r = hpet_alloc_timer(file);
> + if (r < 0)
> + return r;
> +
> devp = file->private_data;
> return hpet_ioctl_common(devp, cmd, arg, 0);
> }

The above constructs make it harder for people to modify the code
later. If they want to add a new local, where to put it? If they want
to add more code, where to put it? Plus there are risks that people
will accidentally turn the code into c99-style definitions.

One could do

{
struct hpet_dev *devp;
int r = hpet_alloc_timer(file);

if (r < 0)
return r;


but that's not terribly good either: it adds risk that someone will
later add a leak.

Better is the plain old simple approach:


{
struct hpet_dev *devp;
int r;

r = hpet_alloc_timer(file);
if (r < 0)
return r;


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/