diff mbox series

[v8,4/6] leds: ledtrig-tty: replace mutex with completion

Message ID 20231109085038.371977-5-fe@dev.tdt.de
State New
Headers show
Series ledtrig-tty: add additional tty state evaluation | expand

Commit Message

Florian Eckert Nov. 9, 2023, 8:50 a.m. UTC
With this commit, the mutex handling is replaced by the completion
handling. When handling mutex, it must always be ensured that the held
mutex is also released again. This is more error-prone should the number
of code paths increase.

This is a preparatory commit to make the trigger more configurable via
additional sysfs parameters. With this change, the worker always runs and
is no longer stopped if no ttyname is set.

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
---
 drivers/leds/trigger/ledtrig-tty.c | 60 +++++++++++++++---------------
 1 file changed, 31 insertions(+), 29 deletions(-)

Comments

Greg Kroah-Hartman Nov. 23, 2023, 2:10 p.m. UTC | #1
On Thu, Nov 09, 2023 at 09:50:36AM +0100, Florian Eckert wrote:
> With this commit, the mutex handling is replaced by the completion
> handling. When handling mutex, it must always be ensured that the held
> mutex is also released again. This is more error-prone should the number
> of code paths increase.
> 
> This is a preparatory commit to make the trigger more configurable via
> additional sysfs parameters. With this change, the worker always runs and
> is no longer stopped if no ttyname is set.
> 
> Signed-off-by: Florian Eckert <fe@dev.tdt.de>
> ---
>  drivers/leds/trigger/ledtrig-tty.c | 60 +++++++++++++++---------------
>  1 file changed, 31 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
> index 86595add72cd..3badf74fa420 100644
> --- a/drivers/leds/trigger/ledtrig-tty.c
> +++ b/drivers/leds/trigger/ledtrig-tty.c
> @@ -1,5 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>  
> +#include <linux/completion.h>
>  #include <linux/delay.h>
>  #include <linux/leds.h>
>  #include <linux/module.h>
> @@ -12,15 +13,24 @@
>  struct ledtrig_tty_data {
>  	struct led_classdev *led_cdev;
>  	struct delayed_work dwork;
> -	struct mutex mutex;
> +	struct completion sysfs;
>  	const char *ttyname;
>  	struct tty_struct *tty;
>  	int rx, tx;
>  };
>  
> -static void ledtrig_tty_restart(struct ledtrig_tty_data *trigger_data)
> +static int ledtrig_tty_waitforcompletion(struct device *dev)

Nit, you might want to add a few more '_' characters, right:
	ledtrig_tty_wait_for_completion()
to match up with the call to wait_for_completion_timeout() it makes.

>  {
> -	schedule_delayed_work(&trigger_data->dwork, 0);
> +	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
> +	int ret;
> +
> +	ret = wait_for_completion_timeout(&trigger_data->sysfs,
> +					  msecs_to_jiffies(LEDTRIG_TTY_INTERVAL * 20));
> +

Nit, no blank line needed here, if you happen to redo this patch.

thanks,

greg "naming is hard" k-h
diff mbox series

Patch

diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
index 86595add72cd..3badf74fa420 100644
--- a/drivers/leds/trigger/ledtrig-tty.c
+++ b/drivers/leds/trigger/ledtrig-tty.c
@@ -1,5 +1,6 @@ 
 // SPDX-License-Identifier: GPL-2.0
 
+#include <linux/completion.h>
 #include <linux/delay.h>
 #include <linux/leds.h>
 #include <linux/module.h>
@@ -12,15 +13,24 @@ 
 struct ledtrig_tty_data {
 	struct led_classdev *led_cdev;
 	struct delayed_work dwork;
-	struct mutex mutex;
+	struct completion sysfs;
 	const char *ttyname;
 	struct tty_struct *tty;
 	int rx, tx;
 };
 
-static void ledtrig_tty_restart(struct ledtrig_tty_data *trigger_data)
+static int ledtrig_tty_waitforcompletion(struct device *dev)
 {
-	schedule_delayed_work(&trigger_data->dwork, 0);
+	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
+	int ret;
+
+	ret = wait_for_completion_timeout(&trigger_data->sysfs,
+					  msecs_to_jiffies(LEDTRIG_TTY_INTERVAL * 20));
+
+	if (ret == 0)
+		return -ETIMEDOUT;
+
+	return ret;
 }
 
 static ssize_t ttyname_show(struct device *dev,
@@ -28,14 +38,16 @@  static ssize_t ttyname_show(struct device *dev,
 {
 	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
 	ssize_t len = 0;
+	int completion;
 
-	mutex_lock(&trigger_data->mutex);
+	reinit_completion(&trigger_data->sysfs);
+	completion = ledtrig_tty_waitforcompletion(dev);
+	if (completion < 0)
+		return completion;
 
 	if (trigger_data->ttyname)
 		len = sprintf(buf, "%s\n", trigger_data->ttyname);
 
-	mutex_unlock(&trigger_data->mutex);
-
 	return len;
 }
 
@@ -46,7 +58,7 @@  static ssize_t ttyname_store(struct device *dev,
 	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
 	char *ttyname;
 	ssize_t ret = size;
-	bool running;
+	int completion;
 
 	if (size > 0 && buf[size - 1] == '\n')
 		size -= 1;
@@ -59,9 +71,10 @@  static ssize_t ttyname_store(struct device *dev,
 		ttyname = NULL;
 	}
 
-	mutex_lock(&trigger_data->mutex);
-
-	running = trigger_data->ttyname != NULL;
+	reinit_completion(&trigger_data->sysfs);
+	completion = ledtrig_tty_waitforcompletion(dev);
+	if (completion < 0)
+		return completion;
 
 	kfree(trigger_data->ttyname);
 	tty_kref_put(trigger_data->tty);
@@ -69,11 +82,6 @@  static ssize_t ttyname_store(struct device *dev,
 
 	trigger_data->ttyname = ttyname;
 
-	mutex_unlock(&trigger_data->mutex);
-
-	if (ttyname && !running)
-		ledtrig_tty_restart(trigger_data);
-
 	return ret;
 }
 static DEVICE_ATTR_RW(ttyname);
@@ -86,13 +94,8 @@  static void ledtrig_tty_work(struct work_struct *work)
 	struct led_classdev *led_cdev = trigger_data->led_cdev;
 	int ret;
 
-	mutex_lock(&trigger_data->mutex);
-
-	if (!trigger_data->ttyname) {
-		/* exit without rescheduling */
-		mutex_unlock(&trigger_data->mutex);
-		return;
-	}
+	if (!trigger_data->ttyname)
+		goto out;
 
 	/* try to get the tty corresponding to $ttyname */
 	if (!trigger_data->tty) {
@@ -117,11 +120,8 @@  static void ledtrig_tty_work(struct work_struct *work)
 	}
 
 	ret = tty_get_icount(trigger_data->tty, &icount);
-	if (ret) {
-		dev_warn(led_cdev->dev, "Failed to get icount, stop polling\n");
-		mutex_unlock(&trigger_data->mutex);
-		return;
-	}
+	if (ret)
+		goto out;
 
 	if (icount.rx != trigger_data->rx ||
 	    icount.tx != trigger_data->tx) {
@@ -134,7 +134,7 @@  static void ledtrig_tty_work(struct work_struct *work)
 	}
 
 out:
-	mutex_unlock(&trigger_data->mutex);
+	complete_all(&trigger_data->sysfs);
 	schedule_delayed_work(&trigger_data->dwork,
 			      msecs_to_jiffies(LEDTRIG_TTY_INTERVAL * 2));
 }
@@ -157,7 +157,9 @@  static int ledtrig_tty_activate(struct led_classdev *led_cdev)
 
 	INIT_DELAYED_WORK(&trigger_data->dwork, ledtrig_tty_work);
 	trigger_data->led_cdev = led_cdev;
-	mutex_init(&trigger_data->mutex);
+	init_completion(&trigger_data->sysfs);
+
+	schedule_delayed_work(&trigger_data->dwork, 0);
 
 	return 0;
 }