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
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAH9NwWcYSkr5ygFEvmqtD=ZjunXhw7E4Ek7_KAmwCUYpUrJqGQ@mail.gmail.com>
Date:	Wed, 14 Dec 2011 09:10:07 +0100
From:	Christian Gmeiner <christian.gmeiner@...il.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	linux-kernel@...r.kernel.org,
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	rpurdie@...ys.net
Subject: Re: [PATCH RESEND] add led driver for Bachmann's ot200

Hi Andrew

2011/12/13 Andrew Morton <akpm@...ux-foundation.org>:
> On Tue, 13 Dec 2011 10:57:30 +0100
> Christian Gmeiner <christian.gmeiner@...il.com> wrote:
>
>> >From a7fecf3426ef98fdd19e9d2610665b9d1ce358a0 Mon Sep 17 00:00:00 2001
>> From: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
>> Date: Fri, 18 Nov 2011 10:21:48 +0100
>> Subject: [PATCH] add led driver for Bachmann's ot200
>>
>> This patch adds support for leds on Bachmann's ot200 visualisation device.
>> The device has three leds on the back panel (led_err, led_init and led_run)
>> and can handle up to seven leds on the front panel.
>>
>> The driver was written by Linutronix on behalf of
>> Bachmann electronic GmbH.
>
> The code uses no tabs and indents with seven-spaces.  Please convert it
> to use hard tabs in the conventional fashion?  In the Makefile and
> Kconfig, too.

okay

>
>>
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index ff203a4..6cf0dd6 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -387,6 +387,14 @@ config LEDS_RENESAS_TPU
>>          pin function. The latter to support brightness control.
>>          Brightness control is supported but hardware blinking is not.
>>
>> +config LEDS_OT200
>> +       tristate "LED support for Bachmann OT200"
>> +       depends on LEDS_CLASS
>> +       help
>> +         This option enables support for the LEDs on the Bachmann OT200. The
>> +         LEDs are controlled through LPC bus.
>> +         Say Y to enable LEDs on the Bachmann OT200.
>> +
>>  config LEDS_TRIGGERS
>>        bool "LED Trigger support"
>>        depends on LEDS_CLASS
>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>> index e4f6bf5..0814d42 100644
>> --- a/drivers/leds/Makefile
>> +++ b/drivers/leds/Makefile
>> @@ -43,6 +43,7 @@ obj-$(CONFIG_LEDS_NS2)                        += leds-ns2.o
>>  obj-$(CONFIG_LEDS_NETXBIG)             += leds-netxbig.o
>>  obj-$(CONFIG_LEDS_ASIC3)               += leds-asic3.o
>>  obj-$(CONFIG_LEDS_RENESAS_TPU)         += leds-renesas-tpu.o
>> +obj-$(CONFIG_LEDS_OT200)               += leds-ot200.o
>>
>>  # LED SPI Drivers
>>  obj-$(CONFIG_LEDS_DAC124S085)          += leds-dac124s085.o
>> diff --git a/drivers/leds/leds-ot200.c b/drivers/leds/leds-ot200.c
>> new file mode 100644
>> index 0000000..60f118b
>> --- /dev/null
>> +++ b/drivers/leds/leds-ot200.c
>> @@ -0,0 +1,204 @@
>> +/*
>> + * Bachmann ot200 leds driver.
>> + *
>> + * Author: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
>> + * License: GPL as published by the FSF.
>> + */
>> +
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/leds.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +
>> +struct ot200_led {
>> +       struct led_classdev cdev;
>> +       char name[8];
>> +       unsigned int num;
>> +};
>> +
>> +static u8 old_val;
>> +static u8 old_val_back;
>
> What's going on here?  The driver is using eight bits to store the
> current control setting, but the changelog says the driver can handle
> ten LEDs.
>
> <looks>
>
> OK, so we're using seven bits in old_val and three in old_val_back.
>

Exactly.. I have not found a better way to store the led state. Any
recommendations?


>> +DEFINE_SPINLOCK(value_lock);
>
> Should be static.
>

okay

>> +static void ot200_led_set(struct led_classdev *led_cdev,
>> +                       enum led_brightness value)
>> +{
>> +       struct ot200_led *led = container_of(led_cdev, struct ot200_led, cdev);
>> +       u8 val;
>> +       u8 mask;
>> +       unsigned long flags;
>> +
>> +       mask = 1 << led->num;
>> +
>> +       spin_lock_irqsave(&value_lock, flags);
>> +       val = old_val;
>> +
>> +       if (value == LED_OFF)
>> +               val &= ~mask;
>> +       else
>> +               val |= mask;
>> +
>> +       old_val = val;
>> +       spin_unlock_irqrestore(&value_lock, flags);
>> +       outb(val, 0x49);
>> +}
>
> It's a bit worrisome doing the outb() outside the locked section, but
> all the races I can think of result from userspace doing things which
> were unavoidably indeterminate anyway.
>

I think that it is really no problem to do outb() in the looked area.
Will change
this for v2 of this patch.

>> +static void ot200_led_back_set(struct led_classdev *led_cdev,
>> +                       enum led_brightness value)
>> +{
>> +       struct ot200_led *led = container_of(led_cdev, struct ot200_led, cdev);
>> +       u8 val;
>> +       u8 mask;
>> +       unsigned long flags;
>> +
>> +       mask = 1 << led->num;
>> +
>> +       spin_lock_irqsave(&value_lock, flags);
>> +       val = old_val_back;
>> +
>> +       if (value == LED_OFF)
>> +               val &= ~mask;
>> +       else
>> +               val |= mask;
>> +
>> +       old_val_back = val;
>> +       spin_unlock_irqrestore(&value_lock, flags);
>> +       outb(val, 0x5a);
>> +}
>> +
>> +static int __devinit create_ot200_led(struct platform_device *pdev, int num,
>> +               struct ot200_led *led)
>> +{
>> +       int ret;
>> +
>> +       num += 1;
>> +       snprintf(led->name, sizeof(led->name), "led%d", num);
>> +       led->num = 7 - num;
>> +       led->cdev.name = led->name;
>> +       led->cdev.default_trigger = NULL;
>> +       led->cdev.blink_set = NULL;
>> +       led->cdev.brightness_set = ot200_led_set;
>> +
>> +       ret = led_classdev_register(&pdev->dev, &led->cdev);
>> +       if (ret < 0)
>> +               goto out;
>> +
>> +       dev_set_drvdata(led->cdev.dev, led);
>> +       return 0;
>> +
>> +       led_classdev_unregister(&led->cdev);
>> +out:
>> +       return ret;
>> +}
>> +
>> +static int __devinit create_ot200_led_back(struct platform_device *pdev,
>> +               int num, struct ot200_led *led)
>> +{
>> +       char *led_name;
>> +       int ret;
>> +
>> +       switch (num) {
>> +       case 0:
>> +               led_name = "run";
>> +               break;
>> +       case 1:
>> +               led_name = "init";
>> +               break;
>> +       case 2:
>> +               led_name = "err";
>> +               break;
>> +       default:
>> +               BUG();
>> +       };
>> +
>> +       snprintf(led->name, sizeof(led->name), "led_%s", led_name);
>> +       led->num = num;
>> +       led->cdev.name = led->name;
>> +       led->cdev.blink_set = NULL;
>> +       led->cdev.brightness_set = ot200_led_back_set;
>> +
>> +       ret = led_classdev_register(&pdev->dev, &led->cdev);
>> +       if (ret < 0)
>> +               goto out;
>> +
>> +       dev_set_drvdata(led->cdev.dev, led);
>> +       return 0;
>> +
>> +       led_classdev_unregister(&led->cdev);
>> +out:
>> +       return ret;
>> +
>> +}
>
> This is implementing a userspace API (led_run, led_init, led_err) but
> that API isn't documented anywhere.  Can we add a few words of
> Documentation so our users know how to use this driver?

To be exact the driver uses the led subsystem - led_classdev_register() - and
creates these led devices. So we are not creating any new API, which should
be documented - or?

# ls /sys/class/leds/ -l
total 0
lrwxrwxrwx 1 root root 0 Dec 14 07:57 led1 ->
../../devices/platform/leds-ot200.0/leds/led1
lrwxrwxrwx 1 root root 0 Dec 14 07:57 led2 ->
../../devices/platform/leds-ot200.0/leds/led2
lrwxrwxrwx 1 root root 0 Dec 14 07:57 led3 ->
../../devices/platform/leds-ot200.0/leds/led3
lrwxrwxrwx 1 root root 0 Dec 14 07:57 led4 ->
../../devices/platform/leds-ot200.0/leds/led4
lrwxrwxrwx 1 root root 0 Dec 14 07:57 led5 ->
../../devices/platform/leds-ot200.0/leds/led5
lrwxrwxrwx 1 root root 0 Dec 14 07:57 led6 ->
../../devices/platform/leds-ot200.0/leds/led6
lrwxrwxrwx 1 root root 0 Dec 14 07:57 led7 ->
../../devices/platform/leds-ot200.0/leds/led7
lrwxrwxrwx 1 root root 0 Dec 14 07:57 led_err ->
../../devices/platform/leds-ot200.0/leds/led_err
lrwxrwxrwx 1 root root 0 Dec 14 07:57 led_ini ->
../../devices/platform/leds-ot200.0/leds/led_ini
lrwxrwxrwx 1 root root 0 Dec 14 07:57 led_run ->
../../devices/platform/leds-ot200.0/leds/led_run

Thanks for your review.
--
Christian Gmeiner, MSc
--
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

Powered by Openwall GNU/*/Linux Powered by OpenVZ