[PATCH 1/2] Input: yealink - use driver core to instantiate device attributes

[PATCH 1/2] Input: yealink - use driver core to instantiate device attributes

[Dmitry Torokhov]

Instead of manually creating driver-specific device attributes
set struct usb_driver->dev_groups pointer to have the driver core
do it.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/yealink.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/input/misc/yealink.c b/drivers/input/misc/yealink.c
index c3221b960a75..435a46baad9d 100644
--- a/drivers/input/misc/yealink.c
+++ b/drivers/input/misc/yealink.c
@@ -771,7 +771,7 @@ static DEVICE_ATTR(show_icon	, _M220, NULL		, show_icon	);
 static DEVICE_ATTR(hide_icon	, _M220, NULL		, hide_icon	);
 static DEVICE_ATTR(ringtone	, _M220, NULL		, store_ringtone);
 
-static struct attribute *yld_attributes[] = {
+static struct attribute *yld_attrs[] = {
 	&dev_attr_line1.attr,
 	&dev_attr_line2.attr,
 	&dev_attr_line3.attr,
@@ -782,10 +782,7 @@ static struct attribute *yld_attributes[] = {
 	&dev_attr_ringtone.attr,
 	NULL
 };
-
-static const struct attribute_group yld_attr_group = {
-	.attrs = yld_attributes
-};
+ATTRIBUTE_GROUPS(yld);
 
 /*******************************************************************************
  * Linux interface and usb initialisation
@@ -842,7 +839,6 @@ static void usb_disconnect(struct usb_interface *intf)
 
 	down_write(&sysfs_rwsema);
 	yld = usb_get_intfdata(intf);
-	sysfs_remove_group(&intf->dev.kobj, &yld_attr_group);
 	usb_set_intfdata(intf, NULL);
 	up_write(&sysfs_rwsema);
 
@@ -975,8 +971,6 @@ static int usb_probe(struct usb_interface *intf, const struct usb_device_id *id)
 	store_line3(&intf->dev, NULL,
 			DRIVER_VERSION, sizeof(DRIVER_VERSION));
 
-	/* Register sysfs hooks (don't care about failure) */
-	ret = sysfs_create_group(&intf->dev.kobj, &yld_attr_group);
 	return 0;
 }
 
@@ -985,6 +979,7 @@ static struct usb_driver yealink_driver = {
 	.probe		= usb_probe,
 	.disconnect	= usb_disconnect,
 	.id_table	= usb_table,
+	.dev_groups	= yld_groups,
 };
 
 module_usb_driver(yealink_driver);
-- 
2.45.2.803.g4e1b14247a-goog

[PATCH 2/2] Input: yealink - simplify locking in sysfs attribute handling

[Dmitry Torokhov]

The locking rules in the driver came from era when sysfs attributes
could live past the point of time when device would be unbound from
the driver, and so used module-global semaphore (potentially shared
between multiple yealink devices). Thankfully these times are long
gone and attributes will not be accessible once they are removed.

Simplify the logic by moving to per-device mutex, stop checking if
there is driver data instance attached to the interface, and use
guard notation to acquire the mutex.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/yealink.c | 72 ++++++++++--------------------------
 1 file changed, 20 insertions(+), 52 deletions(-)

diff --git a/drivers/input/misc/yealink.c b/drivers/input/misc/yealink.c
index 435a46baad9d..8866bf65d347 100644
--- a/drivers/input/misc/yealink.c
+++ b/drivers/input/misc/yealink.c
@@ -36,7 +36,7 @@
 #include <linux/kernel.h>
 #include <linux/slab.h>
 #include <linux/module.h>
-#include <linux/rwsem.h>
+#include <linux/mutex.h>
 #include <linux/usb/input.h>
 #include <linux/map_to_7segment.h>
 
@@ -103,6 +103,8 @@ struct yealink_dev {
 	u8 lcdMap[ARRAY_SIZE(lcdMap)];	/* state of LCD, LED ... */
 	int key_code;			/* last reported key	 */
 
+	struct mutex sysfs_mutex;
+
 	unsigned int shutdown:1;
 
 	int	stat_ix;
@@ -548,8 +550,6 @@ static void input_close(struct input_dev *dev)
  * sysfs interface
  ******************************************************************************/
 
-static DECLARE_RWSEM(sysfs_rwsema);
-
 /* Interface to the 7-segments translation table aka. char set.
  */
 static ssize_t show_map(struct device *dev, struct device_attribute *attr,
@@ -580,15 +580,10 @@ static ssize_t store_map(struct device *dev, struct device_attribute *attr,
  */
 static ssize_t show_line(struct device *dev, char *buf, int a, int b)
 {
-	struct yealink_dev *yld;
+	struct yealink_dev *yld = dev_get_drvdata(dev);
 	int i;
 
-	down_read(&sysfs_rwsema);
-	yld = dev_get_drvdata(dev);
-	if (yld == NULL) {
-		up_read(&sysfs_rwsema);
-		return -ENODEV;
-	}
+	guard(mutex)(&yld->sysfs_mutex);
 
 	for (i = a; i < b; i++)
 		*buf++ = lcdMap[i].type;
@@ -598,7 +593,6 @@ static ssize_t show_line(struct device *dev, char *buf, int a, int b)
 	*buf++ = '\n';
 	*buf = 0;
 
-	up_read(&sysfs_rwsema);
 	return 3 + ((b - a) << 1);
 }
 
@@ -630,22 +624,16 @@ static ssize_t show_line3(struct device *dev, struct device_attribute *attr,
 static ssize_t store_line(struct device *dev, const char *buf, size_t count,
 		int el, size_t len)
 {
-	struct yealink_dev *yld;
+	struct yealink_dev *yld = dev_get_drvdata(dev);
 	int i;
 
-	down_write(&sysfs_rwsema);
-	yld = dev_get_drvdata(dev);
-	if (yld == NULL) {
-		up_write(&sysfs_rwsema);
-		return -ENODEV;
-	}
+	guard(mutex)(&yld->sysfs_mutex);
 
 	if (len > count)
 		len = count;
 	for (i = 0; i < len; i++)
 		setChar(yld, el++, buf[i]);
 
-	up_write(&sysfs_rwsema);
 	return count;
 }
 
@@ -675,15 +663,10 @@ static ssize_t store_line3(struct device *dev, struct device_attribute *attr,
 static ssize_t get_icons(struct device *dev, struct device_attribute *attr,
 			char *buf)
 {
-	struct yealink_dev *yld;
+	struct yealink_dev *yld = dev_get_drvdata(dev);
 	int i, ret = 1;
 
-	down_read(&sysfs_rwsema);
-	yld = dev_get_drvdata(dev);
-	if (yld == NULL) {
-		up_read(&sysfs_rwsema);
-		return -ENODEV;
-	}
+	guard(mutex)(&yld->sysfs_mutex);
 
 	for (i = 0; i < ARRAY_SIZE(lcdMap); i++) {
 		if (lcdMap[i].type != '.')
@@ -692,7 +675,7 @@ static ssize_t get_icons(struct device *dev, struct device_attribute *attr,
 				yld->lcdMap[i] == ' ' ? "  " : "on",
 				lcdMap[i].u.p.name);
 	}
-	up_read(&sysfs_rwsema);
+
 	return ret;
 }
 
@@ -700,15 +683,10 @@ static ssize_t get_icons(struct device *dev, struct device_attribute *attr,
 static ssize_t set_icon(struct device *dev, const char *buf, size_t count,
 			int chr)
 {
-	struct yealink_dev *yld;
+	struct yealink_dev *yld = dev_get_drvdata(dev);
 	int i;
 
-	down_write(&sysfs_rwsema);
-	yld = dev_get_drvdata(dev);
-	if (yld == NULL) {
-		up_write(&sysfs_rwsema);
-		return -ENODEV;
-	}
+	guard(mutex)(&yld->sysfs_mutex);
 
 	for (i = 0; i < ARRAY_SIZE(lcdMap); i++) {
 		if (lcdMap[i].type != '.')
@@ -719,7 +697,6 @@ static ssize_t set_icon(struct device *dev, const char *buf, size_t count,
 		}
 	}
 
-	up_write(&sysfs_rwsema);
 	return count;
 }
 
@@ -739,22 +716,16 @@ static ssize_t hide_icon(struct device *dev, struct device_attribute *attr,
  */
 
 /* Stores raw ringtone data in the phone */
-static ssize_t store_ringtone(struct device *dev,
-		struct device_attribute *attr,
-		const char *buf, size_t count)
+static ssize_t store_ringtone(struct device *dev, struct device_attribute *attr,
+			      const char *buf, size_t count)
 {
-	struct yealink_dev *yld;
+	struct yealink_dev *yld = dev_get_drvdata(dev);
 
-	down_write(&sysfs_rwsema);
-	yld = dev_get_drvdata(dev);
-	if (yld == NULL) {
-		up_write(&sysfs_rwsema);
-		return -ENODEV;
-	}
+	guard(mutex)(&yld->sysfs_mutex);
 
 	/* TODO locking with async usb control interface??? */
 	yealink_set_ringtone(yld, (char *)buf, count);
-	up_write(&sysfs_rwsema);
+
 	return count;
 }
 
@@ -835,14 +806,10 @@ static int usb_cleanup(struct yealink_dev *yld, int err)
 
 static void usb_disconnect(struct usb_interface *intf)
 {
-	struct yealink_dev *yld;
-
-	down_write(&sysfs_rwsema);
-	yld = usb_get_intfdata(intf);
-	usb_set_intfdata(intf, NULL);
-	up_write(&sysfs_rwsema);
+	struct yealink_dev *yld = usb_get_intfdata(intf);
 
 	usb_cleanup(yld, 0);
+	usb_set_intfdata(intf, NULL);
 }
 
 static int usb_probe(struct usb_interface *intf, const struct usb_device_id *id)
@@ -870,6 +837,7 @@ static int usb_probe(struct usb_interface *intf, const struct usb_device_id *id)
 
 	yld->udev = udev;
 	yld->intf = intf;
+	mutex_init(&yld->sysfs_mutex);
 
 	yld->idev = input_dev = input_allocate_device();
 	if (!input_dev)
-- 
2.45.2.803.g4e1b14247a-goog

Re: [PATCH 1/2] Input: yealink - use driver core to instantiate device attributes

[Greg KH]

On Wed, Jul 10, 2024 at 04:48:53PM -0700, Dmitry Torokhov wrote:
> Instead of manually creating driver-specific device attributes
> set struct usb_driver->dev_groups pointer to have the driver core
> do it.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---

Thanks for doing these cleanups!

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 2/2] Input: yealink - simplify locking in sysfs attribute handling

[Greg KH]

On Wed, Jul 10, 2024 at 04:48:54PM -0700, Dmitry Torokhov wrote:
> The locking rules in the driver came from era when sysfs attributes
> could live past the point of time when device would be unbound from
> the driver, and so used module-global semaphore (potentially shared
> between multiple yealink devices). Thankfully these times are long
> gone and attributes will not be accessible once they are removed.
> 
> Simplify the logic by moving to per-device mutex, stop checking if
> there is driver data instance attached to the interface, and use
> guard notation to acquire the mutex.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>