linux-kernel - Re: [PATCH 1/2] leds:arch/sh/boards/landisk LEDs supports

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1178628889.6061.33.camel@localhost.localdomain>
Date:	Tue, 08 May 2007 13:54:48 +0100
From:	Richard Purdie <rpurdie@...ys.net>
To:	kogiidena@...plant.ddo.jp
Cc:	linux-kernel@...r.kernel.org, lethal@...ux-sh.org
Subject: Re: [PATCH 1/2] leds:arch/sh/boards/landisk LEDs supports

> On Tue, 2007-05-08 at 21:26 +0900, kogiidena wrote:
> To:Richard-san
> CC:all
> 
> LED driver of I-O DATA LANDISK and USL-5P
> Please apply following patch
> 
> Signed-off-by: kogiidena <kogiidena@...plant.ddo.jp>
> 
[...]
> +/* HDD-access-LED setting at landisk status LED */
> +static void landisk_disk_trig_activate(struct led_classdev *led_cdev)
> +{
> +        unsigned long flags;
> +        spin_lock_irqsave(&landisk_led_lock, flags);
> +        ctrl_outb((ctrl_inb(PA_LED) & ~0x0c) | 0x04, PA_LED);
> +        spin_unlock_irqrestore(&landisk_led_lock, flags);
> +}
> +
> +static void landisk_disk_trig_deactivate(struct led_classdev *led_cdev)
> +{
> +        unsigned long flags;
> +        spin_lock_irqsave(&landisk_led_lock, flags);
> +        ctrl_outb((ctrl_inb(PA_LED) & ~0x0c) | 0x0c, PA_LED);
> +        spin_unlock_irqrestore(&landisk_led_lock, flags);
> +}
> +
> +static struct led_trigger landisk_disk_led_trigger = {
> +        .name = "disk",
> +        .activate = landisk_disk_trig_activate,
> +        .deactivate = landisk_disk_trig_deactivate,
> +};

You can't do this since the trigger will appear for all LEDs and it only
applies to a single LED. There has been previous discussion of LED
specific triggers and we'll need that support before you can make
something like this work. Nobody has sent me a patch for that yet and I
haven't had time to write one...

+static int __init landisk_led_init(void)
> +{
> +        u8 orig, test;
> +
> +        orig = ctrl_inb(PA_LED);
> +        ctrl_outb(0x40, PA_LED);
> +
> +        test = ctrl_inb(PA_LED);
> +        ctrl_outb(orig, PA_LED);
> +
> +        landisk_arch = (test == 0x40);
> +
> +        if (landisk_arch == 0) {
> +                /* arch == landisk */
> +                ctrl_outb(orig | 0x07, PA_LED);
> +                landisk_leds[1].default_trigger = "disk";
> +                led_trigger_register(&landisk_disk_led_trigger);
> +        } else {
> +                /* arch == usl-5p */
> +                ctrl_outb(orig | 0x03, PA_LED);
> +        }
> +        return platform_driver_register(&landisk_led_driver);

Broken error handling - you should check the return code of
led_trigger_register().

> +static void __exit landisk_led_exit(void)
> +{
> +        if (landisk_arch == 0)
> +                led_trigger_unregister(&landisk_disk_led_trigger);
> +        return platform_driver_unregister(&landisk_led_driver);
> +}

void functions don't return.

Other than those issues, it looks ok.

Richard


-
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/