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

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

[Sven Wegener]

On initialization, we first do the ioremap and then register the led devices.
On deinitialization, we do it in reverse order. This prevents someone calling
into the brightness_set functions with an invalid latch_address.

Signed-off-by: Sven Wegener <sven.wegener@stealer.net>
Cc: Rod Whitby <rod@whitby.id.au>
---
 drivers/leds/leds-fsg.c |   28 ++++++++++++++--------------
 1 files changed, 14 insertions(+), 14 deletions(-)

I do not own a FSG-3, but it's probably a single CPU device and the chance 
of hitting this race condition is unlikely, but we should do it right 
nontheless. Further looking at the code, we probably also need a lock to 
make changing latch_value fully atomic, but again it is unlikely to hit 
this.

diff --git a/drivers/leds/leds-fsg.c b/drivers/leds/leds-fsg.c
index a7421b8..a26fe36 100644
--- 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;
+
 	ret = led_classdev_register(&pdev->dev, &fsg_wlan_led);
 	if (ret < 0)
 		goto failwlan;
@@ -185,20 +195,8 @@ static int fsg_led_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto failring;
 
-	/* 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;
-
 	return ret;
 
- failremap:
-	led_classdev_unregister(&fsg_ring_led);
  failring:
 	led_classdev_unregister(&fsg_sync_led);
  failsync:
@@ -210,14 +208,14 @@ static int fsg_led_probe(struct platform_device *pdev)
  failwan:
 	led_classdev_unregister(&fsg_wlan_led);
  failwlan:
+	iounmap(latch_address);
+ failremap:
 
 	return ret;
 }
 
 static int fsg_led_remove(struct platform_device *pdev)
 {
-	iounmap(latch_address);
-
 	led_classdev_unregister(&fsg_wlan_led);
 	led_classdev_unregister(&fsg_wan_led);
 	led_classdev_unregister(&fsg_sata_led);
@@ -225,6 +223,8 @@ static int fsg_led_remove(struct platform_device *pdev)
 	led_classdev_unregister(&fsg_sync_led);
 	led_classdev_unregister(&fsg_ring_led);
 
+	iounmap(latch_address);
+
 	return 0;
 }
 

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

[Rod Whitby]

Sven Wegener wrote:
> On initialization, we first do the ioremap and then register the led devices.
> On deinitialization, we do it in reverse order. This prevents someone calling
> into the brightness_set functions with an invalid latch_address.
> 
> Signed-off-by: Sven Wegener <sven.wegener@stealer.net>
> Cc: Rod Whitby <rod@whitby.id.au>

Acked-by: Rod Whitby <rod@whitby.id.au>

Thanks.

-- Rod

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

[Andrew Morton]

On Sun, 27 Jul 2008 20:30:59 +0200 (CEST) Sven Wegener <sven.wegener@stealer.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..

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

[Sven Wegener]

On Wed, 30 Jul 2008, Andrew Morton wrote:

> On Sun, 27 Jul 2008 20:30:59 +0200 (CEST) Sven Wegener <sven.wegener@stealer.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;