Re: [PATCH] char: nvram: disable on ARM

Re: [PATCH] char: nvram: disable on ARM

[Arnd Bergmann]

On Tue, Feb 6, 2018 at 11:05 PM, Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
> /dev/nvram was never meant to be used alongside the RTC CMOS driver from
> drivers/rtc as it already expose the NVRAM through another interface..
> Anyway, the last defconfig to enable it properly was removed in 2010 so
> prevent ARM users from selecting it.
>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> ---
>  drivers/char/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
> index c28dca0c613d..9bcac927cfd0 100644
> --- a/drivers/char/Kconfig
> +++ b/drivers/char/Kconfig
> @@ -264,7 +264,7 @@ source "drivers/char/hw_random/Kconfig"
>
>  config NVRAM
>         tristate "/dev/nvram support"
> -       depends on ATARI || X86 || (ARM && RTC_DRV_CMOS) || GENERIC_NVRAM
> +       depends on ATARI || X86 || GENERIC_NVRAM
>         ---help---
>           If you say Y here and create a character special file /dev/nvram
>           with major number 10 and minor number 144 using mknod ("man mknod"),

The change looks good:

Acked-by: Arnd Bergmann <arnd@arndb.de>

but taking a closer look raises a few other points:

* arch/arm/kernel/time.c has this code

#if defined(CONFIG_RTC_DRV_CMOS) || defined(CONFIG_RTC_DRV_CMOS_MODULE) || \
    defined(CONFIG_NVRAM) || defined(CONFIG_NVRAM_MODULE)
/* this needs a better home */
DEFINE_SPINLOCK(rtc_lock);
EXPORT_SYMBOL(rtc_lock);
#endif  /* pc-style 'CMOS' RTC support */

That can be adapted now, or maybe we could move all definitions into
a common place (that needs some more planning).

* similarly, this line in nvram.c can be simplified:
#if defined(CONFIG_ATARI)
#  define MACH ATARI
#elif defined(__i386__) || defined(__x86_64__) || defined(__arm__)  /* and ?? */
#  define MACH PC
#else
#  error Cannot build nvram driver for this machine configuration.
#endif

* GENERIC_NVRAM is not really generic, instead this seems to be the
  chardev that is used for 32-bit powerpc (powermac, 85xx, 86xx), while
  64-bit powerpc (cell, maple, opal, pseries) use code from
  arch/powerpc/kernel/nvram_64.c, with the same underlying arch hooks.
  The nvram_64 code appears to be mostly a superset of the 32-bit
  generic_nvram one.

* The code in drivers/char/nvram is not used at all when
   GENERIC_NVRAM is set, and half the code in there is different
   between x86 and atari.

* most of the external interface in include/linux/nvram.h is
  unused, the rest tends to be architecture specific

* The procfs file appears to be completely useless on any 64-bit
   x86 machine, this is what I see:

$ cat /proc/driver/nvram
Checksum status: valid
# floppies     : 0
Floppy 0 type  : none
Floppy 1 type  : none
HD 0 type      : none
HD 1 type      : none
HD type 48 data: 0/0/0 C/H/S, precomp 0, lz 0
HD type 49 data: 156/0/0 C/H/S, precomp 0, lz 0
DOS base memory: 635 kB
Extended memory: 65535 kB (configured), 65535 kB (tested)
Gfx adapter    : EGA, VGA, ... (with BIOS)
FPU            : not installed

      Arnd

Re: [PATCH] char: nvram: disable on ARM

[Alexandre Belloni]

On 06/02/2018 at 23:55:02 +0100, Arnd Bergmann wrote:
> * arch/arm/kernel/time.c has this code
> 
> #if defined(CONFIG_RTC_DRV_CMOS) || defined(CONFIG_RTC_DRV_CMOS_MODULE) || \
>     defined(CONFIG_NVRAM) || defined(CONFIG_NVRAM_MODULE)
> /* this needs a better home */
> DEFINE_SPINLOCK(rtc_lock);
> EXPORT_SYMBOL(rtc_lock);
> #endif  /* pc-style 'CMOS' RTC support */
> 
> That can be adapted now, or maybe we could move all definitions into
> a common place (that needs some more planning).
> 

Yes, on arm, the rtc_lock is mostly there to please
drivers/rtc/rtc-cmos.c. Maybe we could make the locking in this driver
x86 and PPC specific.

If we can get rid of arch/powerpc/platforms/chrp/time.c and
arch/powerpc/platforms/maple/time.c (so much duplicated code), then it
is x86 only.

> * similarly, this line in nvram.c can be simplified:
> #if defined(CONFIG_ATARI)
> #  define MACH ATARI
> #elif defined(__i386__) || defined(__x86_64__) || defined(__arm__)  /* and ?? */
> #  define MACH PC
> #else
> #  error Cannot build nvram driver for this machine configuration.
> #endif
> 
> * GENERIC_NVRAM is not really generic, instead this seems to be the
>   chardev that is used for 32-bit powerpc (powermac, 85xx, 86xx), while
>   64-bit powerpc (cell, maple, opal, pseries) use code from
>   arch/powerpc/kernel/nvram_64.c, with the same underlying arch hooks.
>   The nvram_64 code appears to be mostly a superset of the 32-bit
>   generic_nvram one.
> 
> * The code in drivers/char/nvram is not used at all when
>    GENERIC_NVRAM is set, and half the code in there is different
>    between x86 and atari.
> 
> * most of the external interface in include/linux/nvram.h is
>   unused, the rest tends to be architecture specific
> 
> * The procfs file appears to be completely useless on any 64-bit
>    x86 machine, this is what I see:
> 
> $ cat /proc/driver/nvram
> Checksum status: valid
> # floppies     : 0
> Floppy 0 type  : none
> Floppy 1 type  : none
> HD 0 type      : none
> HD 1 type      : none
> HD type 48 data: 0/0/0 C/H/S, precomp 0, lz 0
> HD type 49 data: 156/0/0 C/H/S, precomp 0, lz 0
> DOS base memory: 635 kB
> Extended memory: 65535 kB (configured), 65535 kB (tested)
> Gfx adapter    : EGA, VGA, ... (with BIOS)
> FPU            : not installed
> 

I really don't think anyone is using that but I don't really know much
about x86 and the specification this may be part of.

I see the info may be used in drivers/video/fbdev/ and
drivers/platform/x86/thinkpad_acpi.c

-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH] char: nvram: disable on ARM

[Arnd Bergmann]

On Wed, Feb 7, 2018 at 2:55 AM, Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
> On 06/02/2018 at 23:55:02 +0100, Arnd Bergmann wrote:
>> * arch/arm/kernel/time.c has this code
>>
>> #if defined(CONFIG_RTC_DRV_CMOS) || defined(CONFIG_RTC_DRV_CMOS_MODULE) || \
>>     defined(CONFIG_NVRAM) || defined(CONFIG_NVRAM_MODULE)
>> /* this needs a better home */
>> DEFINE_SPINLOCK(rtc_lock);
>> EXPORT_SYMBOL(rtc_lock);
>> #endif  /* pc-style 'CMOS' RTC support */
>>
>> That can be adapted now, or maybe we could move all definitions into
>> a common place (that needs some more planning).
>>
>
> Yes, on arm, the rtc_lock is mostly there to please
> drivers/rtc/rtc-cmos.c. Maybe we could make the locking in this driver
> x86 and PPC specific.
>
> If we can get rid of arch/powerpc/platforms/chrp/time.c and
> arch/powerpc/platforms/maple/time.c (so much duplicated code), then it
> is x86 only.

What about these:

arch/alpha/kernel/rtc.c:        spin_lock(&rtc_lock);
arch/alpha/kernel/rtc.c:        spin_unlock(&rtc_lock);
arch/alpha/kernel/time.c:DEFINE_SPINLOCK(rtc_lock);
arch/alpha/kernel/time.c:EXPORT_SYMBOL(rtc_lock);
arch/arm/kernel/time.c:DEFINE_SPINLOCK(rtc_lock);
arch/arm/kernel/time.c:EXPORT_SYMBOL(rtc_lock);
arch/m32r/kernel/time.c:DEFINE_SPINLOCK(rtc_lock);
arch/m32r/kernel/time.c:EXPORT_SYMBOL(rtc_lock);
arch/m68k/atari/time.c:DEFINE_SPINLOCK(rtc_lock);
arch/m68k/atari/time.c:EXPORT_SYMBOL_GPL(rtc_lock);
arch/mn10300/kernel/rtc.c:DEFINE_SPINLOCK(rtc_lock);
arch/mn10300/kernel/rtc.c:EXPORT_SYMBOL(rtc_lock);
arch/powerpc/kernel/time.c:DEFINE_SPINLOCK(rtc_lock);
arch/powerpc/kernel/time.c:EXPORT_SYMBOL_GPL(rtc_lock);
arch/sparc/kernel/time_32.c:DEFINE_SPINLOCK(rtc_lock);
arch/sparc/kernel/time_32.c:EXPORT_SYMBOL(rtc_lock);
arch/sparc/kernel/time_64.c:DEFINE_SPINLOCK(rtc_lock);

Are they all obsolete?

>> * similarly, this line in nvram.c can be simplified:
>> #if defined(CONFIG_ATARI)
>> #  define MACH ATARI
>> #elif defined(__i386__) || defined(__x86_64__) || defined(__arm__)  /* and ?? */
>> #  define MACH PC
>> #else
>> #  error Cannot build nvram driver for this machine configuration.
>> #endif
>>
>> * GENERIC_NVRAM is not really generic, instead this seems to be the
>>   chardev that is used for 32-bit powerpc (powermac, 85xx, 86xx), while
>>   64-bit powerpc (cell, maple, opal, pseries) use code from
>>   arch/powerpc/kernel/nvram_64.c, with the same underlying arch hooks.
>>   The nvram_64 code appears to be mostly a superset of the 32-bit
>>   generic_nvram one.
>>
>> * The code in drivers/char/nvram is not used at all when
>>    GENERIC_NVRAM is set, and half the code in there is different
>>    between x86 and atari.
>>
>> * most of the external interface in include/linux/nvram.h is
>>   unused, the rest tends to be architecture specific
>>
>> * The procfs file appears to be completely useless on any 64-bit
>>    x86 machine, this is what I see:
>>
>> $ cat /proc/driver/nvram
>> Checksum status: valid
>> # floppies     : 0
>> Floppy 0 type  : none
>> Floppy 1 type  : none
>> HD 0 type      : none
>> HD 1 type      : none
>> HD type 48 data: 0/0/0 C/H/S, precomp 0, lz 0
>> HD type 49 data: 156/0/0 C/H/S, precomp 0, lz 0
>> DOS base memory: 635 kB
>> Extended memory: 65535 kB (configured), 65535 kB (tested)
>> Gfx adapter    : EGA, VGA, ... (with BIOS)
>> FPU            : not installed
>>
>
> I really don't think anyone is using that but I don't really know much
> about x86 and the specification this may be part of.
>
> I see the info may be used in drivers/video/fbdev/ and
> drivers/platform/x86/thinkpad_acpi.c

The thinkpad_acpi driver seems to look at some other bytes
in the nvram, which have a platform specific meaning.

For drivers/video/fbdev/, these appear to all be for pre-x86
Apple Macintosh (m68k or powerpc).

       Arnd

Re: [PATCH] char: nvram: disable on ARM

[Alexandre Belloni]

On 07/02/2018 at 11:33:55 +0100, Arnd Bergmann wrote:
> On Wed, Feb 7, 2018 at 2:55 AM, Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
> > On 06/02/2018 at 23:55:02 +0100, Arnd Bergmann wrote:
> >> * arch/arm/kernel/time.c has this code
> >>
> >> #if defined(CONFIG_RTC_DRV_CMOS) || defined(CONFIG_RTC_DRV_CMOS_MODULE) || \
> >>     defined(CONFIG_NVRAM) || defined(CONFIG_NVRAM_MODULE)
> >> /* this needs a better home */
> >> DEFINE_SPINLOCK(rtc_lock);
> >> EXPORT_SYMBOL(rtc_lock);
> >> #endif  /* pc-style 'CMOS' RTC support */
> >>
> >> That can be adapted now, or maybe we could move all definitions into
> >> a common place (that needs some more planning).
> >>
> >
> > Yes, on arm, the rtc_lock is mostly there to please
> > drivers/rtc/rtc-cmos.c. Maybe we could make the locking in this driver
> > x86 and PPC specific.
> >
> > If we can get rid of arch/powerpc/platforms/chrp/time.c and
> > arch/powerpc/platforms/maple/time.c (so much duplicated code), then it
> > is x86 only.
> 
> What about these:
> 
> arch/alpha/kernel/rtc.c:        spin_lock(&rtc_lock);
> arch/alpha/kernel/rtc.c:        spin_unlock(&rtc_lock);
> arch/alpha/kernel/time.c:DEFINE_SPINLOCK(rtc_lock);
> arch/alpha/kernel/time.c:EXPORT_SYMBOL(rtc_lock);
> arch/arm/kernel/time.c:DEFINE_SPINLOCK(rtc_lock);
> arch/arm/kernel/time.c:EXPORT_SYMBOL(rtc_lock);
> arch/m32r/kernel/time.c:DEFINE_SPINLOCK(rtc_lock);
> arch/m32r/kernel/time.c:EXPORT_SYMBOL(rtc_lock);
> arch/m68k/atari/time.c:DEFINE_SPINLOCK(rtc_lock);
> arch/m68k/atari/time.c:EXPORT_SYMBOL_GPL(rtc_lock);
> arch/mn10300/kernel/rtc.c:DEFINE_SPINLOCK(rtc_lock);
> arch/mn10300/kernel/rtc.c:EXPORT_SYMBOL(rtc_lock);
> arch/powerpc/kernel/time.c:DEFINE_SPINLOCK(rtc_lock);
> arch/powerpc/kernel/time.c:EXPORT_SYMBOL_GPL(rtc_lock);
> arch/sparc/kernel/time_32.c:DEFINE_SPINLOCK(rtc_lock);
> arch/sparc/kernel/time_32.c:EXPORT_SYMBOL(rtc_lock);
> arch/sparc/kernel/time_64.c:DEFINE_SPINLOCK(rtc_lock);
> 
> Are they all obsolete?
> 

Yes and no. For those architecture, the spinlock is only used to make
the driver compile. It is probably not actually needed or at least it
can be made local to the driver.

For alpha, I just realized I never sent a patch removing the spinlock
usage, I'll do that this cycle:
https://github.com/alexandrebelloni/linux/commit/e79e2a754a3a67f2d7e906bfda0042f9dcf66a0b

> >> $ cat /proc/driver/nvram
> >> Checksum status: valid
> >> # floppies     : 0
> >> Floppy 0 type  : none
> >> Floppy 1 type  : none
> >> HD 0 type      : none
> >> HD 1 type      : none
> >> HD type 48 data: 0/0/0 C/H/S, precomp 0, lz 0
> >> HD type 49 data: 156/0/0 C/H/S, precomp 0, lz 0
> >> DOS base memory: 635 kB
> >> Extended memory: 65535 kB (configured), 65535 kB (tested)
> >> Gfx adapter    : EGA, VGA, ... (with BIOS)
> >> FPU            : not installed
> >>
> >
> > I really don't think anyone is using that but I don't really know much
> > about x86 and the specification this may be part of.
> >
> > I see the info may be used in drivers/video/fbdev/ and
> > drivers/platform/x86/thinkpad_acpi.c
> 
> The thinkpad_acpi driver seems to look at some other bytes
> in the nvram, which have a platform specific meaning.
> 

Yeah, I was more concerned that they need drivers/char/nvram.c for
nvram_read_byte so we can't simply remove the driver.

> For drivers/video/fbdev/, these appear to all be for pre-x86
> Apple Macintosh (m68k or powerpc).
> 

-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH] char: nvram: disable on ARM

[Arnd Bergmann]

On Wed, Feb 7, 2018 at 1:48 PM, Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
> On 07/02/2018 at 11:33:55 +0100, Arnd Bergmann wrote:
>> On Wed, Feb 7, 2018 at 2:55 AM, Alexandre Belloni

>> >> $ cat /proc/driver/nvram
>> >> Checksum status: valid
>> >> # floppies     : 0
>> >> Floppy 0 type  : none
>> >> Floppy 1 type  : none
>> >> HD 0 type      : none
>> >> HD 1 type      : none
>> >> HD type 48 data: 0/0/0 C/H/S, precomp 0, lz 0
>> >> HD type 49 data: 156/0/0 C/H/S, precomp 0, lz 0
>> >> DOS base memory: 635 kB
>> >> Extended memory: 65535 kB (configured), 65535 kB (tested)
>> >> Gfx adapter    : EGA, VGA, ... (with BIOS)
>> >> FPU            : not installed
>> >>
>> >
>> > I really don't think anyone is using that but I don't really know much
>> > about x86 and the specification this may be part of.
>> >
>> > I see the info may be used in drivers/video/fbdev/ and
>> > drivers/platform/x86/thinkpad_acpi.c
>>
>> The thinkpad_acpi driver seems to look at some other bytes
>> in the nvram, which have a platform specific meaning.
>>
>
> Yeah, I was more concerned that they need drivers/char/nvram.c for
> nvram_read_byte so we can't simply remove the driver.

Ok, so the procfs interface may be obsolete, but we still need an
interface into the CMOS NVRAM data.

I see that the x86 version of nvram_read_byte is just a wrapper
around CMOS_READ(14 + addr). We also have some drivers
that call the low-level function directly:

arch/x86/include/asm/floppy.h:  val = CMOS_READ(0x10) & 15;
arch/x86/kernel/bootflag.c:     v = CMOS_READ(sbf_port);
drivers/char/mwave/smapi.c:     usSmapiID = CMOS_READ(0x7C);
drivers/input/misc/wistron_btns.c:              qlen = CMOS_READ(cmos_address);

I suppose we could make the thinkpad driver do the same,
or provide a 'static inline' version of nvram_read_byte somewhere.

      Arnd

Re: [PATCH] char: nvram: disable on ARM

[Alexandre Belloni]

On 07/02/2018 at 15:00:04 +0100, Arnd Bergmann wrote:
> On Wed, Feb 7, 2018 at 1:48 PM, Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
> > On 07/02/2018 at 11:33:55 +0100, Arnd Bergmann wrote:
> >> On Wed, Feb 7, 2018 at 2:55 AM, Alexandre Belloni
> 
> >> >> $ cat /proc/driver/nvram
> >> >> Checksum status: valid
> >> >> # floppies     : 0
> >> >> Floppy 0 type  : none
> >> >> Floppy 1 type  : none
> >> >> HD 0 type      : none
> >> >> HD 1 type      : none
> >> >> HD type 48 data: 0/0/0 C/H/S, precomp 0, lz 0
> >> >> HD type 49 data: 156/0/0 C/H/S, precomp 0, lz 0
> >> >> DOS base memory: 635 kB
> >> >> Extended memory: 65535 kB (configured), 65535 kB (tested)
> >> >> Gfx adapter    : EGA, VGA, ... (with BIOS)
> >> >> FPU            : not installed
> >> >>
> >> >
> >> > I really don't think anyone is using that but I don't really know much
> >> > about x86 and the specification this may be part of.
> >> >
> >> > I see the info may be used in drivers/video/fbdev/ and
> >> > drivers/platform/x86/thinkpad_acpi.c
> >>
> >> The thinkpad_acpi driver seems to look at some other bytes
> >> in the nvram, which have a platform specific meaning.
> >>
> >
> > Yeah, I was more concerned that they need drivers/char/nvram.c for
> > nvram_read_byte so we can't simply remove the driver.
> 
> Ok, so the procfs interface may be obsolete, but we still need an
> interface into the CMOS NVRAM data.
> 

Actually, I just found
https://unix.stackexchange.com/questions/331419/is-dev-nvram-dangerous-to-write-to

So it seems to have real values for some people (even if they are
wrong).

That also points to https://sourceforge.net/projects/nvram-wakeup/ but I
don't think it is necessary. The RTC driver should be able to wakeup an
x86 platform.

All the other uses of /dev/nvram I could find with a simple google
search (i.e. saving and restoring CMOS settings) could just use
/sys/class/rtc/rtc0/device/nvram

> I see that the x86 version of nvram_read_byte is just a wrapper
> around CMOS_READ(14 + addr). We also have some drivers
> that call the low-level function directly:
> 
> arch/x86/include/asm/floppy.h:  val = CMOS_READ(0x10) & 15;
> arch/x86/kernel/bootflag.c:     v = CMOS_READ(sbf_port);
> drivers/char/mwave/smapi.c:     usSmapiID = CMOS_READ(0x7C);
> drivers/input/misc/wistron_btns.c:              qlen = CMOS_READ(cmos_address);
> 
> I suppose we could make the thinkpad driver do the same,
> or provide a 'static inline' version of nvram_read_byte somewhere.
> 

I guess we can do that, provided we take rtc_lock before using
CMOS_READ.

Thinking of it, I think this means we don't need the lock for powerpc as
nvram_read_byte doesn't take it. So I guess it is only needed on x86.

-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH] char: nvram: disable on ARM

[Alexandre Belloni]

On 07/02/2018 at 16:47:00 +0100, Alexandre Belloni wrote:
> > >> > I really don't think anyone is using that but I don't really know much
> > >> > about x86 and the specification this may be part of.
> > >> >
> > >> > I see the info may be used in drivers/video/fbdev/ and
> > >> > drivers/platform/x86/thinkpad_acpi.c
> > >>
> > >> The thinkpad_acpi driver seems to look at some other bytes
> > >> in the nvram, which have a platform specific meaning.
> > >>
> > >
> > > Yeah, I was more concerned that they need drivers/char/nvram.c for
> > > nvram_read_byte so we can't simply remove the driver.
> > 
> > Ok, so the procfs interface may be obsolete, but we still need an
> > interface into the CMOS NVRAM data.
> > 
> 
> Actually, I just found
> https://unix.stackexchange.com/questions/331419/is-dev-nvram-dangerous-to-write-to
> 
> So it seems to have real values for some people (even if they are
> wrong).
> 
> That also points to https://sourceforge.net/projects/nvram-wakeup/ but I
> don't think it is necessary. The RTC driver should be able to wakeup an
> x86 platform.
> 
> All the other uses of /dev/nvram I could find with a simple google
> search (i.e. saving and restoring CMOS settings) could just use
> /sys/class/rtc/rtc0/device/nvram
> 

Ok, the chromeos guys are using it for verified boot it seems:
https://chromium.googlesource.com/chromiumos/platform/vboot_reference

I'm wondering whether they really care about the checksum though.

-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com