Re: [PATCH v16 07/14] counter: Add character device interface

On Mon, 20 Sep 2021 19:09:13 +0900
William Breathitt Gray [off-list ref] wrote:

On Sun, Sep 12, 2021 at 05:18:42PM +0100, Jonathan Cameron wrote:

quoted

On Fri, 27 Aug 2021 12:47:51 +0900
William Breathitt Gray [off-list ref] wrote:
  

quoted

This patch introduces a character device interface for the Counter
subsystem. Device data is exposed through standard character device read
operations. Device data is gathered when a Counter event is pushed by
the respective Counter device driver. Configuration is handled via ioctl
operations on the respective Counter character device node.

Cc: David Lechner <david@lechnology.com>
Cc: Gwendal Grignou <redacted>
Cc: Dan Carpenter <redacted>
Cc: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: William Breathitt Gray <redacted>  

Hi William,

Why the bit based lock?  It feels like a mutex_trylock() type approach or
spinlock_trylock() would be a more common solution to this problem.
There is precedence for doing what you have here though so I'm not that
worried about it.  

Hi Jonathan,

We originally used a mutex for this, but Jarkko discovered that this
produced a warning because chrdev_lock would be held when returning to
user space:
https://lore.kernel.org/linux-arm-kernel/YOq19zTsOzKA8v7c@shinobu/T/#m6072133d418d598a5f368bb942c945e46cfab9a5 (local)

Following David Lechner's suggestion, I decided to reimplement
chrdev_lock as a bitmap using an atomic flag.

Ok.  I'm not sure bit lock was quite what was intended (as there is only one of them)
but I suppose it doesn't greatly matter.

quoted

There are a few more things inline.

I've now been through this patch with as fine toothed comb as I'm likely to
do so.  Hence I won't do another review unless there are substantial changes.
I nearly applied it as it stands, but given we aren't in a rush (merge window
open), it's worth just a little more time to tidy up loose ends.

Jonathan  

Responses follow below.

...
...

quoted

quoted

+static int counter_chrdev_release(struct inode *inode, struct file *filp)
+{
+	struct counter_device *const counter = filp->private_data;
+	int ret = 0;
+
+	mutex_lock(&counter->ops_exist_lock);
+
+	if (!counter->ops) {  

This needs a comment to explain how you would get here and
why these two lists need cleaning up here if we do.

Superficially it feels to me like you could just add a counter->ops check in
counter_disable_events() and then call that directly.  I'm guessing
I am missing a deadlock or similar however.  

I don't believe there is a risk of a deadlock, I just felt this
conditional check isn't really related to the counter_disable_events()
path so I kept it seperate; the events lists are freed in
counter_disable_events() but that's incidental rather than the purpose
of the function. My reasoning for the separation is that there are two
scenarios where counter_chrdev_release is called: the first is when a
user closes the chrdev, while the second is when the Counter driver is
removed.

For the first scenario, the counter_disable_events() function is called
to stop events from firing off when noone has the chrdev open. In the
second scenario, we are not interested in disabling events, we know
we're no longer going to be interacting with this device again so we
want to free any held memory.

I want to keep the intention of these code paths clear because the
distinction is important if we start managing additional memory in the
future (i.e. memory unrelated to the events list that would not be freed
in counter_disable_events()), so I'll add a comment explaining what's
happening in this path. Alternatively, I could define a
counter_chrdev_free() function and toss those list free calls into
there, but perhaps that would be overkill right now for just two calls.

A comment would be enough for now I think.

quoted

quoted

+		counter_events_list_free(&counter->events_list);
+		counter_events_list_free(&counter->next_events_list);
+		ret = -ENODEV;
+		goto out_unlock;
+	}
+
+	ret = counter_disable_events(counter);
+	if (ret < 0) {
+		mutex_unlock(&counter->ops_exist_lock);
+		return ret;
+	}
+
+out_unlock:
+	mutex_unlock(&counter->ops_exist_lock);
+
+	put_device(&counter->dev);
+	clear_bit_unlock(0, counter->chrdev_lock);
+
+	return ret;
+}
+
+static const struct file_operations counter_fops = {
+	.owner = THIS_MODULE,
+	.llseek = no_llseek,
+	.read = counter_chrdev_read,
+	.poll = counter_chrdev_poll,
+	.unlocked_ioctl = counter_chrdev_ioctl,
+	.open = counter_chrdev_open,
+	.release = counter_chrdev_release,
+};
+
+int counter_chrdev_add(struct counter_device *const counter)  

To think about: This isn't doing the add. So would counter_chrdev_init() be more
appropriate? The fact it can fail due to the kfifo_alloc makes that
naming less than ideal though.  

I've been using the "add" here to refer to adding the chrdev to the
counter_device structure rather than to the rest of the system. I used a
similar naming convention for the counter-sysfs.c file so I think
"counter_chrdev_add" here should be all right and is consistent with the
existing "counter_sysfs_add" function.

Hmm. I'll go with maybe and reserve the right to say I told you so if
it ever causes problem (and I can remember this discussion which is
fairly unlikely!).

...


...

quoted

quoted

 
 /**

@@ -260,6 +289,16 @@ struct counter_ops {
  * @num_ext:		number of Counter device extensions specified in @ext
  * @priv:		optional private data supplied by driver
  * @dev:		internal device structure
+ * @chrdev:		internal character device structure
+ * @events_list:	list of current watching Counter events
+ * @events_list_lock:	lock to protect Counter events list operations
+ * @next_events_list:	list of next watching Counter events
+ * @n_events_list_lock:	lock to protect Counter next events list operations
+ * @events:		queue of detected Counter events
+ * @events_wait:	wait queue to allow blocking reads of Counter events
+ * @events_lock:	lock to protect Counter events queue read operations
+ * @chrdev_lock:	lock to limit chrdev to a single open at a time
+ * @ops_exist_lock:	lock to prevent use during removal
  */
 struct counter_device {
 	const char *name;

@@ -278,12 +317,24 @@ struct counter_device {
 	void *priv;
 
 	struct device dev;
+	struct cdev chrdev;
+	struct list_head events_list;
+	spinlock_t events_list_lock;
+	struct list_head next_events_list;
+	struct mutex n_events_list_lock;
+	DECLARE_KFIFO_PTR(events, struct counter_event);
+	wait_queue_head_t events_wait;
+	struct mutex events_lock;
+	DECLARE_BITMAP(chrdev_lock, 1);  

Why use a bitmap for this rather than any of the other lock types?  

We can't use a mutex due to the problems mentioned in
https://lore.kernel.org/linux-arm-kernel/YOq19zTsOzKA8v7c@shinobu/T/#m6072133d418d598a5f368bb942c945e46cfab9a5 (local)
so I think a bitmap might be the most straightforward solution here. It
goes without saying that I'm open to any other suggestions as well if
there is a better solution for this case.

Add a comment here so no one tries to refactor it in the future without
understanding  your reasoning.

William Breathitt Gray

Thanks,

Jonathan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel