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
