[RESEND PATCH 3/4] leds: leds-ns2: handle can_sleep GPIOs

Simon Guinot simon.guinot at sequanux.org
Fri Jun 26 10:10:28 PDT 2015

On Wed, Jun 24, 2015 at 04:18:29PM +0200, Jacek Anaszewski wrote:
> On 06/18/2015 05:17 PM, Simon Guinot wrote:
> >On the board n090401 (Seagate NAS 4-Bay), some of the LEDs are handled
> >by the leds-ns2 driver. This LEDs are connected to an I2C GPIO expander
> >(PCA95554PW) which means that GPIO access may sleep. This patch makes
> >leds-ns2 compatible with such GPIOs by using the *_cansleep() variant of
> >the GPIO functions. As a drawback this functions can't be used safely in
> >a timer context (with the timer LED trigger for example). To fix this
> >issue, a workqueue mechanism (copied from the leds-gpio driver) is used.
> >
> >Note that this patch also updates slightly the ns2_led_sata_store
> >function. The LED state is now retrieved from cached values instead of
> >reading the GPIOs previously. This prevents ns2_led_sata_store from
> >working with a stale LED state (which may happen when a delayed work
> >is pending).
> >
> >Signed-off-by: Simon Guinot <simon.guinot at sequanux.org>
> >Signed-off-by: Vincent Donnefort <vdonnefort at gmail.com>
> >---
> >  drivers/leds/leds-ns2.c | 56 ++++++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 42 insertions(+), 14 deletions(-)
> >
> >
> >+static void ns2_led_work(struct work_struct *work)
> >+{
> >+	struct ns2_led_data *led_dat =
> >+		container_of(work, struct ns2_led_data, work);
> >+	int i = led_dat->new_mode_index;
> >+
> >+	write_lock(&led_dat->rw_lock);
> >+
> >+	gpio_set_value_cansleep(led_dat->cmd, led_dat->modval[i].cmd_level);
> >+	gpio_set_value_cansleep(led_dat->slow, led_dat->modval[i].slow_level);
> >+
> >+	write_unlock(&led_dat->rw_lock);
> >+}
> >+
> I've just realized that this can break one of the basic rules:
> no sleeping should occur while holding a spinlock. Did you
> consider this?

Well, if I did, I can't say I have done a good job here :/

You have to know that this code is used on a large number of boards.
Thus, I have to thank you for spotting this bug. As a relief, this don't
actually lead to a bug with the configuration we are using: UP machine

It should be simple to fix it because using a spinlock in ns2_led_work()
is not needed. The GPIO writing calls are protected by the workqueue
itself: a single instance is running at a time. We are only let with the
new_mode_index reading which must be made coherent.

Note that the very same issue also applies to ns2_led_get_mode(). And
again using a lock here is not needed either. This function is only
called once at probe time and there is no possible concurrency.

I'll fix all this issues with the v2.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150626/40279a2e/attachment.sig>

More information about the linux-arm-kernel mailing list