Message ID | 1432842380-4187-1-git-send-email-u.kleine-koenig@pengutronix.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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
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.
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
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.
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
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
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(+)