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 <[email protected]>
Cc: Rod Whitby <[email protected]>
---
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;
}
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 <[email protected]>
> Cc: Rod Whitby <[email protected]>
Acked-by: Rod Whitby <[email protected]>
Thanks.
-- Rod
On Sun, 27 Jul 2008 20:30:59 +0200 (CEST) Sven Wegener <[email protected]> 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..
On Wed, 30 Jul 2008, Andrew Morton wrote:
> On Sun, 27 Jul 2008 20:30:59 +0200 (CEST) Sven Wegener <[email protected]> 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;