Re: [PATCH 4/7] platform/surface: aggregator_cdev: Add support for forwarding events to user-space

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 6/4/21 1:32 PM, Hans de Goede wrote:
Hi,

I've one review remark inline below.

On 6/4/21 1:45 AM, Maximilian Luz wrote:

[...]

+static int ssam_cdev_device_open(struct inode *inode, struct file *filp)
+{
+	struct miscdevice *mdev = filp->private_data;
+	struct ssam_cdev_client *client;
+	struct ssam_cdev *cdev = container_of(mdev, struct ssam_cdev, mdev);
+
+	/* Initialize client */
+	client = vzalloc(sizeof(*client));
+	if (!client)
+		return -ENOMEM;
+
+	client->cdev = ssam_cdev_get(cdev);
+
+	INIT_LIST_HEAD(&client->node);
+
+	mutex_init(&client->notifier_lock);
+
+	mutex_init(&client->read_lock);
+	mutex_init(&client->write_lock);
+	INIT_KFIFO(client->buffer);
+	init_waitqueue_head(&client->waitq);
+
+	filp->private_data = client;
+
+	/* Attach client. */
+	down_write(&cdev->client_lock);
+
+	if (test_bit(SSAM_CDEV_DEVICE_SHUTDOWN_BIT, &cdev->flags)) {
+		up_write(&cdev->client_lock);
+		ssam_cdev_put(client->cdev);

You are missing the mutex_destroy() calls here which you are
doing in ssam_cdev_device_release().

Thank you for noticing this! This code is based on Surface DTX code
which has the same problem, I'll send in a fix for that shortly.

Or maybe move the mutex_init calls below this check
(before the up_write()) since I don't think the client can
be accessed by any code until the up_write is done?

Yes, that would also be possible, but I'd prefer adding the
mutex_destroy() calls in the failure path. To me that seems a bit easier
to reason about.

Thanks,
Max



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux