Messages in this thread | | | Date | Mon, 22 Mar 2010 15:21:03 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH] hpet: factor timer allocate from open |
| |
On Thu, 18 Mar 2010 21:06:58 -0700 (PDT) Magnus Lynch <maglyx@gmail.com> 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;
| |