[PATCH] mtd: dc21285: use raw spinlock functions for nw_gpio_lock

[PATCH] mtd: dc21285: use raw spinlock functions for nw_gpio_lock

[Uwe Kleine-König]

Since commit bd31b85960a7 (which is in 3.2-rc1) nw_gpio_lock is a raw spinlock
that needs usage of the corresponding raw functions.

This fixes:

  drivers/mtd/maps/dc21285.c: In function 'nw_en_write':
  drivers/mtd/maps/dc21285.c:41:340: warning: passing argument 1 of 'spinlock_check' from incompatible pointer type
    spin_lock_irqsave(&nw_gpio_lock, flags);

  In file included from include/linux/seqlock.h:35:0,
                   from include/linux/time.h:5,
                   from include/linux/stat.h:18,
                   from include/linux/module.h:10,
                   from drivers/mtd/maps/dc21285.c:8:
  include/linux/spinlock.h:299:102: note: expected 'struct spinlock_t *' but argument is of type 'struct raw_spinlock_t *'
   static inline raw_spinlock_t *spinlock_check(spinlock_t *lock)
                                                                                                        ^
  drivers/mtd/maps/dc21285.c:43:25: warning: passing argument 1 of 'spin_unlock_irqrestore' from incompatible pointer type
    spin_unlock_irqrestore(&nw_gpio_lock, flags);
                           ^
  In file included from include/linux/seqlock.h:35:0,
                   from include/linux/time.h:5,
                   from include/linux/stat.h:18,
                   from include/linux/module.h:10,
                   from drivers/mtd/maps/dc21285.c:8:
  include/linux/spinlock.h:370:91: note: expected 'struct spinlock_t *' but argument is of type 'struct raw_spinlock_t *'
   static inline void spin_unlock_irqrestore(spinlock_t *lock, unsigned long flags)

Fixes: bd31b85960a7 ("locking, ARM: Annotate low level hw locks as raw")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/mtd/maps/dc21285.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/maps/dc21285.c b/drivers/mtd/maps/dc21285.c
index f8a7dd14cee0..70a3db3ab856 100644
--- a/drivers/mtd/maps/dc21285.c
+++ b/drivers/mtd/maps/dc21285.c
@@ -38,9 +38,9 @@ static void nw_en_write(void)
 	 * we want to write a bit pattern XXX1 to Xilinx to enable
 	 * the write gate, which will be open for about the next 2ms.
 	 */
-	spin_lock_irqsave(&nw_gpio_lock, flags);
+	raw_spin_lock_irqsave(&nw_gpio_lock, flags);
 	nw_cpld_modify(CPLD_FLASH_WR_ENABLE, CPLD_FLASH_WR_ENABLE);
-	spin_unlock_irqrestore(&nw_gpio_lock, flags);
+	raw_spin_unlock_irqrestore(&nw_gpio_lock, flags);
 
 	/*
 	 * let the ISA bus to catch on...
-- 
2.1.4

Re: [PATCH] mtd: dc21285: use raw spinlock functions for nw_gpio_lock

[Brian Norris]

On Thu, May 28, 2015 at 10:22:10AM +0200, Uwe Kleine-König wrote:
> Since commit bd31b85960a7 (which is in 3.2-rc1) nw_gpio_lock is a raw spinlock
> that needs usage of the corresponding raw functions.
> 
> This fixes:
> 
>   drivers/mtd/maps/dc21285.c: In function 'nw_en_write':
>   drivers/mtd/maps/dc21285.c:41:340: warning: passing argument 1 of 'spinlock_check' from incompatible pointer type
>     spin_lock_irqsave(&nw_gpio_lock, flags);
> 
>   In file included from include/linux/seqlock.h:35:0,
>                    from include/linux/time.h:5,
>                    from include/linux/stat.h:18,
>                    from include/linux/module.h:10,
>                    from drivers/mtd/maps/dc21285.c:8:
>   include/linux/spinlock.h:299:102: note: expected 'struct spinlock_t *' but argument is of type 'struct raw_spinlock_t *'
>    static inline raw_spinlock_t *spinlock_check(spinlock_t *lock)
>                                                                                                         ^
>   drivers/mtd/maps/dc21285.c:43:25: warning: passing argument 1 of 'spin_unlock_irqrestore' from incompatible pointer type
>     spin_unlock_irqrestore(&nw_gpio_lock, flags);
>                            ^
>   In file included from include/linux/seqlock.h:35:0,
>                    from include/linux/time.h:5,
>                    from include/linux/stat.h:18,
>                    from include/linux/module.h:10,
>                    from drivers/mtd/maps/dc21285.c:8:
>   include/linux/spinlock.h:370:91: note: expected 'struct spinlock_t *' but argument is of type 'struct raw_spinlock_t *'
>    static inline void spin_unlock_irqrestore(spinlock_t *lock, unsigned long flags)
> 
> Fixes: bd31b85960a7 ("locking, ARM: Annotate low level hw locks as raw")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Applied to l2-mtd.git. Apparently nobody builds this driver.

Brian

[PATCH] ARM: footbridge: enable dc21285 mtd map in defconfig

[Uwe Kleine-König]

This driver fails to build since release 3.2. Improve compile coverage
by enabling this driver in the footbridge defconfig

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
 arch/arm/configs/footbridge_defconfig | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/configs/footbridge_defconfig b/arch/arm/configs/footbridge_defconfig
index 87e020f303ab..9aebf4b910ca 100644
--- a/arch/arm/configs/footbridge_defconfig
+++ b/arch/arm/configs/footbridge_defconfig
@@ -37,6 +37,10 @@ CONFIG_IRDA_CACHE_LAST_LSAP=y
 CONFIG_IRDA_FAST_RR=y
 CONFIG_IRDA_DEBUG=y
 CONFIG_WINBOND_FIR=m
+CONFIG_MTD=m
+CONFIG_MTD_CFI=m
+CONFIG_MTD_COMPLEX_MAPPINGS=y
+CONFIG_MTD_DC21285=m
 CONFIG_PARPORT=y
 CONFIG_PARPORT_PC=y
 CONFIG_PARPORT_PC_FIFO=y
-- 
2.1.4

[PATCH] ARM: footbridge: enable dc21285 mtd map in defconfig

[Brian Norris]

On Thu, May 28, 2015 at 09:46:20PM +0200, Uwe Kleine-K?nig wrote:
> This driver fails to build since release 3.2. Improve compile coverage
> by enabling this driver in the footbridge defconfig

Actually, it did build, but it just had alarming warnings. I don't think
those warnings actually affected anything at runtime, except for lock
debugging (if enabled).

But anyway, the change looks good, and I'm doing this in my local build
tests now, so:

> Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>

Acked-by: Brian Norris <computersforpeace@gmail.com>

> ---
>  arch/arm/configs/footbridge_defconfig | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm/configs/footbridge_defconfig b/arch/arm/configs/footbridge_defconfig
> index 87e020f303ab..9aebf4b910ca 100644
> --- a/arch/arm/configs/footbridge_defconfig
> +++ b/arch/arm/configs/footbridge_defconfig
> @@ -37,6 +37,10 @@ CONFIG_IRDA_CACHE_LAST_LSAP=y
>  CONFIG_IRDA_FAST_RR=y
>  CONFIG_IRDA_DEBUG=y
>  CONFIG_WINBOND_FIR=m
> +CONFIG_MTD=m
> +CONFIG_MTD_CFI=m
> +CONFIG_MTD_COMPLEX_MAPPINGS=y
> +CONFIG_MTD_DC21285=m
>  CONFIG_PARPORT=y
>  CONFIG_PARPORT_PC=y
>  CONFIG_PARPORT_PC_FIFO=y

[PATCH] ARM: footbridge: enable dc21285 mtd map in defconfig

[Russell King - ARM Linux]

On Thu, May 28, 2015 at 12:55:58PM -0700, Brian Norris wrote:
> On Thu, May 28, 2015 at 09:46:20PM +0200, Uwe Kleine-K?nig wrote:
> > This driver fails to build since release 3.2. Improve compile coverage
> > by enabling this driver in the footbridge defconfig
> 
> Actually, it did build, but it just had alarming warnings. I don't think
> those warnings actually affected anything at runtime, except for lock
> debugging (if enabled).
> 
> But anyway, the change looks good, and I'm doing this in my local build
> tests now, so:
> 
> > Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> 
> Acked-by: Brian Norris <computersforpeace@gmail.com>

I'd say no to this - some boards have old (28F008) flash on them, which
(iirc) doesn't take kindly to being probed by MTD.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH] ARM: footbridge: enable dc21285 mtd map in defconfig

[Uwe Kleine-König]

On Thu, May 28, 2015 at 10:05:20PM +0100, Russell King - ARM Linux wrote:
> On Thu, May 28, 2015 at 12:55:58PM -0700, Brian Norris wrote:
> > On Thu, May 28, 2015 at 09:46:20PM +0200, Uwe Kleine-K?nig wrote:
> > > This driver fails to build since release 3.2. Improve compile coverage
> > > by enabling this driver in the footbridge defconfig
> > 
> > Actually, it did build, but it just had alarming warnings. I don't think
> > those warnings actually affected anything at runtime, except for lock
> > debugging (if enabled).
> > 
> > But anyway, the change looks good, and I'm doing this in my local build
> > tests now, so:
> > 
> > > Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> > 
> > Acked-by: Brian Norris <computersforpeace@gmail.com>
> 
> I'd say no to this - some boards have old (28F008) flash on them, which
> (iirc) doesn't take kindly to being probed by MTD.
Is this a bug? I'd say keeping the driver disabled in the defconfig just
papers over a problem.
Maybe modify the driver to not probe unless a kernel parameter is given?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] ARM: footbridge: enable dc21285 mtd map in defconfig

[Russell King - ARM Linux]

On Fri, May 29, 2015 at 08:49:42AM +0200, Uwe Kleine-K?nig wrote:
> On Thu, May 28, 2015 at 10:05:20PM +0100, Russell King - ARM Linux wrote:
> > On Thu, May 28, 2015 at 12:55:58PM -0700, Brian Norris wrote:
> > > On Thu, May 28, 2015 at 09:46:20PM +0200, Uwe Kleine-K?nig wrote:
> > > > This driver fails to build since release 3.2. Improve compile coverage
> > > > by enabling this driver in the footbridge defconfig
> > > 
> > > Actually, it did build, but it just had alarming warnings. I don't think
> > > those warnings actually affected anything at runtime, except for lock
> > > debugging (if enabled).
> > > 
> > > But anyway, the change looks good, and I'm doing this in my local build
> > > tests now, so:
> > > 
> > > > Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> > > 
> > > Acked-by: Brian Norris <computersforpeace@gmail.com>
> > 
> > I'd say no to this - some boards have old (28F008) flash on them, which
> > (iirc) doesn't take kindly to being probed by MTD.
> Is this a bug? I'd say keeping the driver disabled in the defconfig just
> papers over a problem.

Yes it is papering over a bug, but...

(1) I didn't add the driver.
(2) I didn't see the addition of the driver.
(3) I don't have the resources to be able to recover from a screwed up
    flash bricking my footbridge platforms.

So, I wouldn't even like to /try/ enabling it to see whether it doesn't
have the behaviour I referred to.

> Maybe modify the driver to not probe unless a kernel parameter is given?

Possibly an alternative solution.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH] ARM: footbridge: enable dc21285 mtd map in defconfig

[Brian Norris]

On Fri, May 29, 2015 at 1:42 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, May 29, 2015 at 08:49:42AM +0200, Uwe Kleine-K?nig wrote:
>> On Thu, May 28, 2015 at 10:05:20PM +0100, Russell King - ARM Linux wrote:
>> > On Thu, May 28, 2015 at 12:55:58PM -0700, Brian Norris wrote:
>> > > On Thu, May 28, 2015 at 09:46:20PM +0200, Uwe Kleine-K?nig wrote:
>> > > > This driver fails to build since release 3.2. Improve compile coverage
>> > > > by enabling this driver in the footbridge defconfig
>> > >
>> > > Actually, it did build, but it just had alarming warnings. I don't think
>> > > those warnings actually affected anything at runtime, except for lock
>> > > debugging (if enabled).
>> > >
>> > > But anyway, the change looks good, and I'm doing this in my local build
>> > > tests now, so:
>> > >
>> > > > Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
>> > >
>> > > Acked-by: Brian Norris <computersforpeace@gmail.com>

^^ Sorry, in retrospect that was dumb.

>> > I'd say no to this - some boards have old (28F008) flash on them, which
>> > (iirc) doesn't take kindly to being probed by MTD.
>> Is this a bug? I'd say keeping the driver disabled in the defconfig just
>> papers over a problem.

I believe there are several ancient MTD drivers like this that don't
use any typical platform or PCI driver infrastructure, and so are much
more likely to just go and start assuming and poking things as soon as
their init code is run. A random sampling of drivers/mtd/maps/ turns
up things like this driver: drivers/mtd/maps/impa7.c. It just assumes
a few fixed physical addresses and hammers away...

> Yes it is papering over a bug, but...
>
> (1) I didn't add the driver.
> (2) I didn't see the addition of the driver.
> (3) I don't have the resources to be able to recover from a screwed up
>     flash bricking my footbridge platforms.
>
> So, I wouldn't even like to /try/ enabling it to see whether it doesn't
> have the behaviour I referred to.

This driver is so old and unloved (and semi-broken for >19 releases)
that I'd bet nobody actually uses or cares for it. So it really
doesn't make sense to be adding it to a defconfig. It'll just be
relegated to my build tests, so maybe I'll catch obvious build
regressions.

I don't really know what to do with such drivers, though. For now I
tend to leave well enough alone (I know no history on many of these to
judge much otherwise), but it seems like some of these could
potentially cause people harm if they somehow manage to load one of
them. Maybe some of them should be removed, or marked BROKEN and see
if anyone complains.

>> Maybe modify the driver to not probe unless a kernel parameter is given?
>
> Possibly an alternative solution.

Brian