linux-kernel - Re: [PATCH] leds-fsg: Change order of initialization and deinitialization

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] [day] [month] [year] [list]

Date:	Wed, 30 Jul 2008 17:42:27 +0200 (CEST)
From:	Sven Wegener <sven.wegener@...aler.net>
To:	Andrew Morton <akpm@...ux-foundation.org>
cc:	Richard Purdie <rpurdie@...ys.net>, Rod Whitby <rod@...tby.id.au>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] leds-fsg: Change order of initialization and
 deinitialization

On Wed, 30 Jul 2008, Andrew Morton wrote:

> On Sun, 27 Jul 2008 20:30:59 +0200 (CEST) Sven Wegener <sven.wegener@...aler.net> wrote:
> 
> > --- a/drivers/leds/leds-fsg.c
> > +++ b/drivers/leds/leds-fsg.c
> > @@ -161,6 +161,16 @@ static int fsg_led_probe(struct platform_device *pdev)
> >  {
> >  	int ret;
> >  
> > +	/* Map the LED chip select address space */
> > +	latch_address = (unsigned short *) ioremap(IXP4XX_EXP_BUS_BASE(2), 512);
> > +	if (!latch_address) {
> > +		ret = -ENOMEM;
> > +		goto failremap;
> > +	}
> > +
> > +	latch_value = 0xffff;
> > +	*latch_address = latch_value;
> 
> Mutter.  Shouldn't this driver be using readw/writew?  It's not even
> using a volatile pointer, so there's a decent risk that the compiler
> will optimise away important things..

I don't own a FSG-3, so I can't test my changes. Currently I don't have a 
arm cross toolchain, so I even can't compile-test anything. But you mean 
something along the lines below on top of the patch? Don't know if we need 
to writew an initial value or if the hardware sets it up properly. Using 
__raw functions to avoid the cpu_to_le16 conversion as the conversion 
isn't there in the current code.

diff --git a/drivers/leds/leds-fsg.c b/drivers/leds/leds-fsg.c
index a26fe36..42fb09d 100644
--- a/drivers/leds/leds-fsg.c
+++ b/drivers/leds/leds-fsg.c
@@ -22,80 +22,55 @@
 #include <asm/arch/hardware.h>
 #include <asm/io.h>
 
-static short __iomem *latch_address;
-static unsigned short latch_value;
+static unsigned short __iomem *latch_address;
 
 
+static void fsg_led_set(enum led_brightness value, int bit)
+{
+	unsigned short latch_value = __raw_readw(latch_address);
+
+	if (value)
+		latch_value &= ~(1 << bit);
+	else
+		latch_value |=  (1 << bit);
+
+	__raw_writew(latch_value, latch_address);
+}
+
 static void fsg_led_wlan_set(struct led_classdev *led_cdev,
 			     enum led_brightness value)
 {
-	if (value) {
-		latch_value &= ~(1 << FSG_LED_WLAN_BIT);
-		*latch_address = latch_value;
-	} else {
-		latch_value |=  (1 << FSG_LED_WLAN_BIT);
-		*latch_address = latch_value;
-	}
+	fsg_led_set(value, FSG_LED_WLAN_BIT);
 }
 
 static void fsg_led_wan_set(struct led_classdev *led_cdev,
 			    enum led_brightness value)
 {
-	if (value) {
-		latch_value &= ~(1 << FSG_LED_WAN_BIT);
-		*latch_address = latch_value;
-	} else {
-		latch_value |=  (1 << FSG_LED_WAN_BIT);
-		*latch_address = latch_value;
-	}
+	fsg_led_set(value, FSG_LED_WAN_BIT);
 }
 
 static void fsg_led_sata_set(struct led_classdev *led_cdev,
 			     enum led_brightness value)
 {
-	if (value) {
-		latch_value &= ~(1 << FSG_LED_SATA_BIT);
-		*latch_address = latch_value;
-	} else {
-		latch_value |=  (1 << FSG_LED_SATA_BIT);
-		*latch_address = latch_value;
-	}
+	fsg_led_set(value, FSG_LED_SATA_BIT);
 }
 
 static void fsg_led_usb_set(struct led_classdev *led_cdev,
 			    enum led_brightness value)
 {
-	if (value) {
-		latch_value &= ~(1 << FSG_LED_USB_BIT);
-		*latch_address = latch_value;
-	} else {
-		latch_value |=  (1 << FSG_LED_USB_BIT);
-		*latch_address = latch_value;
-	}
+	fsg_led_set(value, FSG_LED_USB_BIT);
 }
 
 static void fsg_led_sync_set(struct led_classdev *led_cdev,
 			     enum led_brightness value)
 {
-	if (value) {
-		latch_value &= ~(1 << FSG_LED_SYNC_BIT);
-		*latch_address = latch_value;
-	} else {
-		latch_value |=  (1 << FSG_LED_SYNC_BIT);
-		*latch_address = latch_value;
-	}
+	fsg_led_set(value, FSG_LED_SYNC_BIT);
 }
 
 static void fsg_led_ring_set(struct led_classdev *led_cdev,
 			     enum led_brightness value)
 {
-	if (value) {
-		latch_value &= ~(1 << FSG_LED_RING_BIT);
-		*latch_address = latch_value;
-	} else {
-		latch_value |=  (1 << FSG_LED_RING_BIT);
-		*latch_address = latch_value;
-	}
+	fsg_led_set(value, FSG_LED_RING_BIT);
 }
 
 
@@ -168,9 +143,6 @@ static int fsg_led_probe(struct platform_device *pdev)
 		goto failremap;
 	}
 
-	latch_value = 0xffff;
-	*latch_address = latch_value;
-
 	ret = led_classdev_register(&pdev->dev, &fsg_wlan_led);
 	if (ret < 0)
 		goto failwlan;
--
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/