RE: [PATCH 3/3] leds: lp5521: Fix circular locking

[ilkka.koskinen]

Hi,

>From: Samu Onkalo [mailto:samu.p.onkalo@xxxxxxxxx]
>Sent: 17 December, 2010 12:19
>Subject: [PATCH 3/3] leds: lp5521: 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 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@xxxxxxxxx>

Reviewed-by: Ilkka Koskinen <ilkka.koskinen@xxxxxxxxx>


>---
> drivers/leds/leds-lp5521.c |   52 ++++++-------------------------------
>------
> 1 files changed, 8 insertions(+), 44 deletions(-)
>
>diff --git a/drivers/leds/leds-lp5521.c b/drivers/leds/leds-lp5521.c
>index 745ad99..eda99b9 100644
>--- a/drivers/leds/leds-lp5521.c
>+++ b/drivers/leds/leds-lp5521.c
>@@ -98,7 +98,6 @@
> #define LP5521_EXT_CLK_USED		0x08
>
> struct lp5521_engine {
>-	const struct attribute_group *attributes;
> 	int		id;
> 	u8		mode;
> 	u8		prog_page;
>@@ -225,25 +224,22 @@ static int lp5521_set_led_current(struct
>lp5521_chip *chip, int led, u8 curr)
> 		    curr);
> }
>
>-static void lp5521_init_engine(struct lp5521_chip *chip,
>-			const struct attribute_group *attr_group)
>+static void lp5521_init_engine(struct lp5521_chip *chip)
> {
> 	int i;
> 	for (i = 0; i < ARRAY_SIZE(chip->engines); i++) {
> 		chip->engines[i].id = i + 1;
> 		chip->engines[i].engine_mask = LP5521_ENG_MASK_BASE >> (i *
>2);
> 		chip->engines[i].prog_page = i;
>-		chip->engines[i].attributes = &attr_group[i];
> 	}
> }
>
>-static int lp5521_configure(struct i2c_client *client,
>-			const struct attribute_group *attr_group)
>+static int lp5521_configure(struct i2c_client *client)
> {
> 	struct lp5521_chip *chip = i2c_get_clientdata(client);
> 	int ret;
>
>-	lp5521_init_engine(chip, attr_group);
>+	lp5521_init_engine(chip);
>
> 	/* Set all PWMs to direct control mode */
> 	ret = lp5521_write(client, LP5521_REG_OP_MODE, 0x3F);
>@@ -329,9 +325,6 @@ static int lp5521_detect(struct i2c_client *client)
> /* Set engine mode and create appropriate sysfs attributes, if
>required. */
> static int lp5521_set_mode(struct lp5521_engine *engine, u8 mode)
> {
>-	struct lp5521_chip *chip = engine_to_lp5521(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 */
>@@ -343,18 +336,10 @@ static int lp5521_set_mode(struct lp5521_engine
>*engine, u8 mode)
> 	} else if (mode == LP5521_CMD_LOAD) {
> 		lp5521_set_engine_mode(engine, LP5521_CMD_DISABLED);
> 		lp5521_set_engine_mode(engine, LP5521_CMD_LOAD);
>-
>-		ret = sysfs_create_group(&dev->kobj, engine->attributes);
>-		if (ret)
>-			return ret;
> 	} else if (mode == LP5521_CMD_DISABLED) {
> 		lp5521_set_engine_mode(engine, LP5521_CMD_DISABLED);
> 	}
>
>-	/* remove load attribute from sysfs if not in load mode */
>-	if (engine->mode == LP5521_CMD_LOAD && mode != LP5521_CMD_LOAD)
>-		sysfs_remove_group(&dev->kobj, engine->attributes);
>-
> 	engine->mode = mode;
>
> 	return ret;
>@@ -387,7 +372,10 @@ static int lp5521_do_store_load(struct
>lp5521_engine *engine,
> 		goto fail;
>
> 	mutex_lock(&chip->lock);
>-	ret = lp5521_load_program(engine, pattern);
>+	if (engine->mode == LP5521_CMD_LOAD)
>+		ret = lp5521_load_program(engine, pattern);
>+	else
>+		ret = -EINVAL;
> 	mutex_unlock(&chip->lock);
>
> 	if (ret) {
>@@ -574,20 +562,8 @@ static struct attribute *lp5521_attributes[] = {
> 	&dev_attr_engine2_mode.attr,
> 	&dev_attr_engine3_mode.attr,
> 	&dev_attr_selftest.attr,
>-	NULL
>-};
>-
>-static struct attribute *lp5521_engine1_attributes[] = {
> 	&dev_attr_engine1_load.attr,
>-	NULL
>-};
>-
>-static struct attribute *lp5521_engine2_attributes[] = {
> 	&dev_attr_engine2_load.attr,
>-	NULL
>-};
>-
>-static struct attribute *lp5521_engine3_attributes[] = {
> 	&dev_attr_engine3_load.attr,
> 	NULL
> };
>@@ -596,12 +572,6 @@ static const struct attribute_group lp5521_group =
>{
> 	.attrs = lp5521_attributes,
> };
>
>-static const struct attribute_group lp5521_engine_group[] = {
>-	{.attrs = lp5521_engine1_attributes },
>-	{.attrs = lp5521_engine2_attributes },
>-	{.attrs = lp5521_engine3_attributes },
>-};
>-
> static int lp5521_register_sysfs(struct i2c_client *client)
> {
> 	struct device *dev = &client->dev;
>@@ -616,12 +586,6 @@ static void lp5521_unregister_sysfs(struct
>i2c_client *client)
>
> 	sysfs_remove_group(&dev->kobj, &lp5521_group);
>
>-	for (i = 0; i <  ARRAY_SIZE(chip->engines); i++) {
>-		if (chip->engines[i].mode == LP5521_CMD_LOAD)
>-			sysfs_remove_group(&dev->kobj,
>-					chip->engines[i].attributes);
>-	}
>-
> 	for (i = 0; i < chip->num_leds; i++)
> 		sysfs_remove_group(&chip->leds[i].cdev.dev->kobj,
> 				&lp5521_led_attribute_group);
>@@ -724,7 +688,7 @@ static int lp5521_probe(struct i2c_client *client,
>
> 	dev_info(&client->dev, "%s programmable led chip found\n", id-
>>name);
>
>-	ret = lp5521_configure(client, lp5521_engine_group);
>+	ret = lp5521_configure(client);
> 	if (ret < 0) {
> 		dev_err(&client->dev, "error configuring chip\n");
> 		goto fail2;
>--
>1.7.0.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/