On Thu, 16 Dec 2010, Richard Cochran wrote:

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);

Please start a new obj-y += line instead
  

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