[PATCH 1/2] leds: trigger: Fix error path to not unlock the unlocked mutex

[PATCH 1/2] leds: trigger: Fix error path to not unlock the unlocked mutex

[Uwe Kleine-König]

ttyname is allocated before the mutex is taken, so it must not be
unlocked in the error path.

Reported-by: Pavel Machek <pavel@ucw.cz>
Fixes: fd4a641ac88f ("leds: trigger: implement a tty trigger")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/leds/trigger/ledtrig-tty.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
index d2ab6ab080ac..68ed2c87a65c 100644
--- a/drivers/leds/trigger/ledtrig-tty.c
+++ b/drivers/leds/trigger/ledtrig-tty.c
@@ -51,10 +51,8 @@ static ssize_t ttyname_store(struct device *dev,
 
 	if (size) {
 		ttyname = kmemdup_nul(buf, size, GFP_KERNEL);
-		if (!ttyname) {
-			ret = -ENOMEM;
-			goto out_unlock;
-		}
+		if (!ttyname)
+			return -ENOMEM;
 	} else {
 		ttyname = NULL;
 	}
-- 
2.29.2

[PATCH 2/2] leds: trigger/tty: Use led_set_brightness_sync() from workqueue

[Uwe Kleine-König]

led_set_brightness() involves scheduling a workqueue. As here the led's
brightness setting is done in context of the trigger's workqueue this is
unjustified overhead and it's more sensible to use
led_set_brightness_sync().

Reported-by: Pavel Machek <pavel@ucw.cz>
Fixes: fd4a641ac88f ("leds: trigger: implement a tty trigger")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/leds/trigger/ledtrig-tty.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
index 68ed2c87a65c..ee57b5b95959 100644
--- a/drivers/leds/trigger/ledtrig-tty.c
+++ b/drivers/leds/trigger/ledtrig-tty.c
@@ -123,12 +123,12 @@ static void ledtrig_tty_work(struct work_struct *work)
 
 	if (icount.rx != trigger_data->rx ||
 	    icount.tx != trigger_data->tx) {
-		led_set_brightness(trigger_data->led_cdev, LED_ON);
+		led_set_brightness_sync(trigger_data->led_cdev, LED_ON);
 
 		trigger_data->rx = icount.rx;
 		trigger_data->tx = icount.tx;
 	} else {
-		led_set_brightness(trigger_data->led_cdev, LED_OFF);
+		led_set_brightness_sync(trigger_data->led_cdev, LED_OFF);
 	}
 
 out:
-- 
2.29.2

Re: [PATCH 1/2] leds: trigger: Fix error path to not unlock the unlocked mutex

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 246 bytes --]

On Fri 2021-02-19 09:29:54, Uwe Kleine-König wrote:
> ttyname is allocated before the mutex is taken, so it must not be
> unlocked in the error path.
> 

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 2/2] leds: trigger/tty: Use led_set_brightness_sync() from workqueue

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 377 bytes --]

On Fri 2021-02-19 09:29:55, Uwe Kleine-König wrote:
> led_set_brightness() involves scheduling a workqueue. As here the led's
> brightness setting is done in context of the trigger's workqueue this is
> unjustified overhead and it's more sensible to use
> led_set_brightness_sync().

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 2/2] leds: trigger/tty: Use led_set_brightness_sync() from workqueue

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 703 bytes --]

On Fri, Feb 19, 2021 at 10:46:32AM +0100, Pavel Machek wrote:
> On Fri 2021-02-19 09:29:55, Uwe Kleine-König wrote:
> > led_set_brightness() involves scheduling a workqueue. As here the led's
> > brightness setting is done in context of the trigger's workqueue this is
> > unjustified overhead and it's more sensible to use
> > led_set_brightness_sync().
> 
> Acked-by: Pavel Machek <pavel@ucw.cz>

Who is expected to pick these two up? I assume Greg as the commit that
is fixed here is only in his try for now?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] leds: trigger/tty: Use led_set_brightness_sync() from workqueue

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 698 bytes --]

On Fri 2021-02-19 10:52:54, Uwe Kleine-König wrote:
> On Fri, Feb 19, 2021 at 10:46:32AM +0100, Pavel Machek wrote:
> > On Fri 2021-02-19 09:29:55, Uwe Kleine-König wrote:
> > > led_set_brightness() involves scheduling a workqueue. As here the led's
> > > brightness setting is done in context of the trigger's workqueue this is
> > > unjustified overhead and it's more sensible to use
> > > led_set_brightness_sync().
> > 
> > Acked-by: Pavel Machek <pavel@ucw.cz>
> 
> Who is expected to pick these two up? I assume Greg as the commit that
> is fixed here is only in his try for now?

I can't reasonably pick them up.

									Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 1/2] leds: trigger: Fix error path to not unlock the unlocked mutex

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4839 bytes --]

Hi "Uwe,

I love your patch! Perhaps something to improve:

[auto build test WARNING on tty/tty-testing]
[also build test WARNING on next-20210218]
[cannot apply to pavel-linux-leds/for-next staging/staging-testing v5.11]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Uwe-Kleine-K-nig/leds-trigger-Fix-error-path-to-not-unlock-the-unlocked-mutex/20210219-163217
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: powerpc-randconfig-r032-20210219 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project c9439ca36342fb6013187d0a69aef92736951476)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install powerpc cross compiling tool for clang build
        # apt-get install binutils-powerpc-linux-gnu
        # https://github.com/0day-ci/linux/commit/f13a575b98965827ab968d1c88d98c6b044d4492
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Uwe-Kleine-K-nig/leds-trigger-Fix-error-path-to-not-unlock-the-unlocked-mutex/20210219-163217
        git checkout f13a575b98965827ab968d1c88d98c6b044d4492
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/leds/trigger/ledtrig-tty.c:70:1: warning: unused label 'out_unlock' [-Wunused-label]
   out_unlock:
   ^~~~~~~~~~~
   1 warning generated.


vim +/out_unlock +70 drivers/leds/trigger/ledtrig-tty.c

fd4a641ac88fbb Uwe Kleine-König 2021-01-13  39  
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  40  static ssize_t ttyname_store(struct device *dev,
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  41  			     struct device_attribute *attr, const char *buf,
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  42  			     size_t size)
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  43  {
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  44  	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  45  	char *ttyname;
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  46  	ssize_t ret = size;
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  47  	bool running;
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  48  
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  49  	if (size > 0 && buf[size - 1] == '\n')
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  50  		size -= 1;
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  51  
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  52  	if (size) {
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  53  		ttyname = kmemdup_nul(buf, size, GFP_KERNEL);
f13a575b989658 Uwe Kleine-König 2021-02-19  54  		if (!ttyname)
f13a575b989658 Uwe Kleine-König 2021-02-19  55  			return -ENOMEM;
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  56  	} else {
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  57  		ttyname = NULL;
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  58  	}
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  59  
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  60  	mutex_lock(&trigger_data->mutex);
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  61  
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  62  	running = trigger_data->ttyname != NULL;
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  63  
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  64  	kfree(trigger_data->ttyname);
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  65  	tty_kref_put(trigger_data->tty);
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  66  	trigger_data->tty = NULL;
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  67  
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  68  	trigger_data->ttyname = ttyname;
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  69  
fd4a641ac88fbb Uwe Kleine-König 2021-01-13 @70  out_unlock:
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  71  	mutex_unlock(&trigger_data->mutex);
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  72  
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  73  	if (ttyname && !running)
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  74  		ledtrig_tty_restart(trigger_data);
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  75  
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  76  	return ret;
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  77  }
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  78  static DEVICE_ATTR_RW(ttyname);
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  79  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 36144 bytes --]

Re: [PATCH 1/2] leds: trigger: Fix error path to not unlock the unlocked mutex

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4967 bytes --]

Hi "Uwe,

I love your patch! Perhaps something to improve:

[auto build test WARNING on tty/tty-testing]
[also build test WARNING on next-20210218]
[cannot apply to pavel-linux-leds/for-next staging/staging-testing v5.11]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Uwe-Kleine-K-nig/leds-trigger-Fix-error-path-to-not-unlock-the-unlocked-mutex/20210219-163217
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: powerpc-randconfig-r032-20210219 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project c9439ca36342fb6013187d0a69aef92736951476)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install powerpc cross compiling tool for clang build
        # apt-get install binutils-powerpc-linux-gnu
        # https://github.com/0day-ci/linux/commit/f13a575b98965827ab968d1c88d98c6b044d4492
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Uwe-Kleine-K-nig/leds-trigger-Fix-error-path-to-not-unlock-the-unlocked-mutex/20210219-163217
        git checkout f13a575b98965827ab968d1c88d98c6b044d4492
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/leds/trigger/ledtrig-tty.c:70:1: warning: unused label 'out_unlock' [-Wunused-label]
   out_unlock:
   ^~~~~~~~~~~
   1 warning generated.


vim +/out_unlock +70 drivers/leds/trigger/ledtrig-tty.c

fd4a641ac88fbb Uwe Kleine-König 2021-01-13  39  
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  40  static ssize_t ttyname_store(struct device *dev,
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  41  			     struct device_attribute *attr, const char *buf,
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  42  			     size_t size)
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  43  {
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  44  	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  45  	char *ttyname;
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  46  	ssize_t ret = size;
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  47  	bool running;
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  48  
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  49  	if (size > 0 && buf[size - 1] == '\n')
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  50  		size -= 1;
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  51  
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  52  	if (size) {
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  53  		ttyname = kmemdup_nul(buf, size, GFP_KERNEL);
f13a575b989658 Uwe Kleine-König 2021-02-19  54  		if (!ttyname)
f13a575b989658 Uwe Kleine-König 2021-02-19  55  			return -ENOMEM;
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  56  	} else {
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  57  		ttyname = NULL;
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  58  	}
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  59  
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  60  	mutex_lock(&trigger_data->mutex);
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  61  
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  62  	running = trigger_data->ttyname != NULL;
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  63  
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  64  	kfree(trigger_data->ttyname);
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  65  	tty_kref_put(trigger_data->tty);
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  66  	trigger_data->tty = NULL;
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  67  
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  68  	trigger_data->ttyname = ttyname;
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  69  
fd4a641ac88fbb Uwe Kleine-König 2021-01-13 @70  out_unlock:
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  71  	mutex_unlock(&trigger_data->mutex);
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  72  
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  73  	if (ttyname && !running)
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  74  		ledtrig_tty_restart(trigger_data);
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  75  
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  76  	return ret;
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  77  }
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  78  static DEVICE_ATTR_RW(ttyname);
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  79  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 36144 bytes --]

Re: [PATCH 2/2] leds: trigger/tty: Use led_set_brightness_sync() from workqueue

[Greg Kroah-Hartman]

On Fri, Feb 19, 2021 at 10:52:54AM +0100, Uwe Kleine-König wrote:
> On Fri, Feb 19, 2021 at 10:46:32AM +0100, Pavel Machek wrote:
> > On Fri 2021-02-19 09:29:55, Uwe Kleine-König wrote:
> > > led_set_brightness() involves scheduling a workqueue. As here the led's
> > > brightness setting is done in context of the trigger's workqueue this is
> > > unjustified overhead and it's more sensible to use
> > > led_set_brightness_sync().
> > 
> > Acked-by: Pavel Machek <pavel@ucw.cz>
> 
> Who is expected to pick these two up? I assume Greg as the commit that
> is fixed here is only in his try for now?

I will do so once Linus takes my existing pull request, don't worry,
they will make it into 5.12-final.

thanks,

greg k-h

Re: [PATCH 2/2] leds: trigger/tty: Use led_set_brightness_sync() from workqueue

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 974 bytes --]

On Fri 2021-02-19 11:41:49, Greg Kroah-Hartman wrote:
> On Fri, Feb 19, 2021 at 10:52:54AM +0100, Uwe Kleine-König wrote:
> > On Fri, Feb 19, 2021 at 10:46:32AM +0100, Pavel Machek wrote:
> > > On Fri 2021-02-19 09:29:55, Uwe Kleine-König wrote:
> > > > led_set_brightness() involves scheduling a workqueue. As here the led's
> > > > brightness setting is done in context of the trigger's workqueue this is
> > > > unjustified overhead and it's more sensible to use
> > > > led_set_brightness_sync().
> > > 
> > > Acked-by: Pavel Machek <pavel@ucw.cz>
> > 
> > Who is expected to pick these two up? I assume Greg as the commit that
> > is fixed here is only in his try for now?
> 
> I will do so once Linus takes my existing pull request, don't worry,
> they will make it into 5.12-final.

It seems like the label is now unused, so this causes warnings... see
the bot output.

Best regards,
									Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]