Messages in this thread | | | Date | Thu, 16 Dec 2010 21:56:38 +0100 (CET) | From | Thomas Gleixner <> | Subject | Re: [PATCH V7 3/8] posix clocks: introduce dynamic clocks |
| |
On Thu, 16 Dec 2010, Richard Cochran wrote:
> --- /dev/null > +++ b/include/linux/posix-clock.h > +/** > + * struct posix_clock_fops - character device operations > + * > + * Every posix clock is represented by a character device. Drivers may > + * optionally offer extended capabilities by implementing these > + * functions. The character device file operations are first handled > + * by the clock device layer, then passed on to the driver by calling > + * these functions. > + * > + * The clock device layer already uses fp->private_data, so drivers > + * are provided their private data via the 'priv' paramenter. > + */ > +void *posix_clock_private(struct file *fp);
Leftover ? There is neither a caller nor an implementation
> +struct posix_clock_fops { > + int (*fasync) (void *priv, int fd, struct file *file, int on); > + int (*mmap) (void *priv, struct vm_area_struct *vma); > + int (*open) (void *priv, fmode_t f_mode); > + int (*release) (void *priv); > + long (*ioctl) (void *priv, unsigned int cmd, unsigned long arg); > + long (*compat_ioctl) (void *priv, unsigned int cmd, unsigned long arg);
Do we really need a compat_ioctl ?
> + ssize_t (*read) (void *priv, uint flags, char __user *buf, size_t cnt); > + unsigned int (*poll) (void *priv, struct file *file, poll_table *wait);
> --- a/kernel/time/Makefile > +++ b/kernel/time/Makefile > @@ -1,4 +1,5 @@ > -obj-y += timekeeping.o ntp.o clocksource.o jiffies.o timer_list.o timecompare.o timeconv.o > +obj-y += timekeeping.o ntp.o clocksource.o jiffies.o timer_list.o \ > +timecompare.o timeconv.o posix-clock.o
Please start a new obj-y += line instead
> --- /dev/null > +++ b/kernel/time/posix-clock.c > + > +#define MAX_CLKDEV BITS_PER_LONG
Please put some more space between the MAX_CLKDEV and BITS_PER_LONG. I had to look three times to not read it as a single word.
> +static DECLARE_BITMAP(clocks_map, MAX_CLKDEV); > +static DEFINE_MUTEX(clocks_mutex); /* protects 'clocks_map' */
Please avoid tail comments
> +struct posix_clock { > + struct posix_clock_operations *ops; > + struct cdev cdev; > + struct kref kref; > + struct mutex mux; > + void *priv;
You can get away with that private pointer and all the void * arguments to the various posix_clock_operations, if you mandate that the posix_clock_operations are embedded into a clock specific data structure.
So void *priv would become struct posix_clock_operations *clkops and you can get your private data in the clock implementation with container_of().
> + int index; > + bool zombie;
Ths field is only set, but nowhere else used. What's the purpose ? Leftover ?
> +}; > +
> +static void delete_clock(struct kref *kref); > + > + > +static int posix_clock_open(struct inode *inode, struct file *fp) > +{ > + struct posix_clock *clk = > + container_of(inode->i_cdev, struct posix_clock, cdev); > + > + kref_get(&clk->kref); > + fp->private_data = clk;
fp->private_data should only be set on success. And this will leak a ref count when the clock open function fails.
What's that kref protecting here ?
> + > + if (clk->ops->fops.open) > + return clk->ops->fops.open(clk->priv, fp->f_mode); > + else > + return 0; > +} > + > +static int posix_clock_release(struct inode *inode, struct file *fp) > +{ > + struct posix_clock *clk = fp->private_data; > + int err = 0;
fp->private_data should be set to NULL in the release function.
> + if (clk->ops->fops.release) > + err = clk->ops->fops.release(clk->priv); > + > + kref_put(&clk->kref, delete_clock);
> +struct posix_clock *posix_clock_create(struct posix_clock_operations *cops, > + dev_t devid, void *priv) > +{ > + struct posix_clock *clk; > + int err; > + > + mutex_lock(&clocks_mutex); > + > + err = -ENOMEM; > + clk = kzalloc(sizeof(*clk), GFP_KERNEL); > + if (!clk) > + goto no_memory; > + > + err = -EBUSY; > + clk->index = find_first_zero_bit(clocks_map, MAX_CLKDEV); > + if (clk->index < MAX_CLKDEV) > + set_bit(clk->index, clocks_map); > + else > + goto no_index;
if (clk->index >= MAX_CLKDEV) goto no_index;
set_bit(clk->index, clocks_map);
Makes it better readable.
> +static void delete_clock(struct kref *kref) > +{ > + struct posix_clock *clk = > + container_of(kref, struct posix_clock, kref); > + > + mutex_lock(&clocks_mutex); > + clear_bit(clk->index, clocks_map); > + mutex_unlock(&clocks_mutex); > + > + mutex_destroy(&clk->mux); > + kfree(clk); > +} > + > +void posix_clock_destroy(struct posix_clock *clk) > +{ > + cdev_del(&clk->cdev); > + > + mutex_lock(&clk->mux); > + clk->zombie = true; > + mutex_unlock(&clk->mux); > + > + kref_put(&clk->kref, delete_clock);
I still have some headache to understand that kref / delete_clock magic here.
Thanks,
tglx
| |