| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <B152A2ECB7F1E847B5E0A8CFDF6F369002E1F8@008-AM1MPN1-022.mgdnok.nokia.com>
Date: Fri, 17 Dec 2010 12:45:44 +0000
From: <ilkka.koskinen@...ia.com>
To: <samu.p.onkalo@...ia.com>, <rpurdie@...ys.net>,
<akpm@...ux-foundation.org>
CC: <linux-kernel@...r.kernel.org>, <arun.murthy@...ricsson.com>,
<ankur.vaish@...ricsson.com>
Subject: RE: [PATCH 2/3] leds: lp5523: Fix circular locking
Hi,
>From: Onkalo Samu.P (Nokia-MS/Tampere)
>Sent: 17 December, 2010 12:19
>Subject: [PATCH 2/3] leds: lp5523: Fix circular locking
>
>Driver contained possibility for circular locking.
>One lock is held by sysfs-core and another one by the driver itself.
>This happened when the driver created or removed sysfs entries
>dynamically.
>There is no real need to do those operations. Now all the sysfs entries
>are created at probe and removed at removal. Engine load and mux
>configuration
>sysfs entries are now visible all the time. However, access to the
>entries
>fails if the engine is disabled or running.
>
>Signed-off-by: Samu Onkalo <samu.p.onkalo@...ia.com>
>---
> drivers/leds/leds-lp5523.c | 56 ++++++++++++-------------------------
>------
> 1 files changed, 16 insertions(+), 40 deletions(-)
>
>diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
>index 6cb4160..d0c4068 100644
>--- a/drivers/leds/leds-lp5523.c
>+++ b/drivers/leds/leds-lp5523.c
>@@ -105,7 +105,6 @@
> #define SHIFT_MASK(id) (((id) - 1) * 2)
>
> struct lp5523_engine {
>- const struct attribute_group *attributes;
> int id;
> u8 mode;
> u8 prog_page;
>@@ -403,14 +402,23 @@ static ssize_t store_engine_leds(struct device
>*dev,
> struct i2c_client *client = to_i2c_client(dev);
> struct lp5523_chip *chip = i2c_get_clientdata(client);
> u16 mux = 0;
>+ ssize_t ret;
>
> if (lp5523_mux_parse(buf, &mux, len))
> return -EINVAL;
>
>+ mutex_lock(&chip->lock);
>+ ret = -EINVAL;
You could move this assignment to declaration. But otherwise
looks good to me. So,
Reviewed-by: Ilkka Koskinen <ilkka.koskinen@...ia.com>
Cheers,
>+ if (chip->engines[nr - 1].mode != LP5523_CMD_LOAD)
>+ goto leave;
>+
> if (lp5523_load_mux(&chip->engines[nr - 1], mux))
>- return -EINVAL;
>+ goto leave;
>
>- return len;
>+ ret = len;
>+leave:
>+ mutex_unlock(&chip->lock);
>+ return ret;
> }
>
> #define store_leds(nr) \
>@@ -556,7 +564,11 @@ static int lp5523_do_store_load(struct
>lp5523_engine *engine,
>
> mutex_lock(&chip->lock);
>
>- ret = lp5523_load_program(engine, pattern);
>+ if (engine->mode == LP5523_CMD_LOAD)
>+ ret = lp5523_load_program(engine, pattern);
>+ else
>+ ret = -EINVAL;
>+
> mutex_unlock(&chip->lock);
>
> if (ret) {
>@@ -737,37 +749,18 @@ static struct attribute *lp5523_attributes[] = {
> &dev_attr_engine2_mode.attr,
> &dev_attr_engine3_mode.attr,
> &dev_attr_selftest.attr,
>- NULL
>-};
>-
>-static struct attribute *lp5523_engine1_attributes[] = {
> &dev_attr_engine1_load.attr,
> &dev_attr_engine1_leds.attr,
>- NULL
>-};
>-
>-static struct attribute *lp5523_engine2_attributes[] = {
> &dev_attr_engine2_load.attr,
> &dev_attr_engine2_leds.attr,
>- NULL
>-};
>-
>-static struct attribute *lp5523_engine3_attributes[] = {
> &dev_attr_engine3_load.attr,
> &dev_attr_engine3_leds.attr,
>- NULL
> };
>
> static const struct attribute_group lp5523_group = {
> .attrs = lp5523_attributes,
> };
>
>-static const struct attribute_group lp5523_engine_group[] = {
>- {.attrs = lp5523_engine1_attributes },
>- {.attrs = lp5523_engine2_attributes },
>- {.attrs = lp5523_engine3_attributes },
>-};
>-
> static int lp5523_register_sysfs(struct i2c_client *client)
> {
> struct device *dev = &client->dev;
>@@ -788,10 +781,6 @@ static void lp5523_unregister_sysfs(struct
>i2c_client *client)
>
> sysfs_remove_group(&dev->kobj, &lp5523_group);
>
>- for (i = 0; i < ARRAY_SIZE(chip->engines); i++)
>- if (chip->engines[i].mode == LP5523_CMD_LOAD)
>- sysfs_remove_group(&dev->kobj,
>&lp5523_engine_group[i]);
>-
> for (i = 0; i < chip->num_leds; i++)
> sysfs_remove_group(&chip->leds[i].cdev.dev->kobj,
> &lp5523_led_attribute_group);
>@@ -802,10 +791,6 @@ static void lp5523_unregister_sysfs(struct
>i2c_client *client)
> /*--------------------------------------------------------------*/
> static int lp5523_set_mode(struct lp5523_engine *engine, u8 mode)
> {
>- /* engine to chip */
>- struct lp5523_chip *chip = engine_to_lp5523(engine);
>- struct i2c_client *client = chip->client;
>- struct device *dev = &client->dev;
> int ret = 0;
>
> /* if in that mode already do nothing, except for run */
>@@ -817,18 +802,10 @@ static int lp5523_set_mode(struct lp5523_engine
>*engine, u8 mode)
> } else if (mode == LP5523_CMD_LOAD) {
> lp5523_set_engine_mode(engine, LP5523_CMD_DISABLED);
> lp5523_set_engine_mode(engine, LP5523_CMD_LOAD);
>-
>- ret = sysfs_create_group(&dev->kobj, engine->attributes);
>- if (ret)
>- return ret;
> } else if (mode == LP5523_CMD_DISABLED) {
> lp5523_set_engine_mode(engine, LP5523_CMD_DISABLED);
> }
>
>- /* remove load attribute from sysfs if not in load mode */
>- if (engine->mode == LP5523_CMD_LOAD && mode != LP5523_CMD_LOAD)
>- sysfs_remove_group(&dev->kobj, engine->attributes);
>-
> engine->mode = mode;
>
> return ret;
>@@ -845,7 +822,6 @@ static int __init lp5523_init_engine(struct
>lp5523_engine *engine, int id)
> engine->engine_mask = LP5523_ENG_MASK_BASE >> SHIFT_MASK(id);
> engine->prog_page = id - 1;
> engine->mux_page = id + 2;
>- engine->attributes = &lp5523_engine_group[id - 1];
>
> return 0;
> }
>--
>1.7.0.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists