Re: [PATCH] arch: m68k: mac: misc.c: Remove some unused functions

Re: [PATCH] arch: m68k: mac: misc.c: Remove some unused functions

[Finn Thain]

On Thu, 1 Jan 2015, Rickard Strandqvist wrote:

> Removes some functions that are not used anywhere:
> mac_pram_write() mac_pram_read()
> 

If you remove those functions, you'd then find that all of the called 
functions become unused:

maciisi_read_pram
maciisi_write_pram
pmu_read_pram
pmu_write_pram
cuda_read_pram
cuda_write_pram
via_read_pram
via_write_pram

I'd rather not remove all of this code. Better to finish the 
implementation.

If we provide access to the PRAM (i.e. NVRAM) from userspace then the 
EMILE bootloader would be able to assign the boot device, for example.

Within the kernel, the CUDA PRAM functions could be used by valkyriefb to 
determine the correct default video mode (see comment in that driver).

Would it be acceptable to utilize drivers/char/generic_nvram.c and 
CONFIG_GENERIC_NVRAM? This is the PowerMac PRAM driver but looks generic 
enough that it may not need any modification for 68k Macs.

-- 

> This was partially found by using a static code analysis program called 
> cppcheck.
> 
> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> ---
>  arch/m68k/mac/misc.c |   46 ----------------------------------------------
>  1 file changed, 46 deletions(-)
> 
> diff --git a/arch/m68k/mac/misc.c b/arch/m68k/mac/misc.c
> index 707b61a..6fe7eb6 100644
> --- a/arch/m68k/mac/misc.c
> +++ b/arch/m68k/mac/misc.c
> @@ -453,52 +453,6 @@ void pmu_shutdown(void)
>   *-------------------------------------------------------------------
>   */
>  
> -void mac_pram_read(int offset, __u8 *buffer, int len)
> -{
> -	__u8 (*func)(int);
> -	int i;
> -
> -	switch(macintosh_config->adb_type) {
> -	case MAC_ADB_IISI:
> -		func = maciisi_read_pram; break;
> -	case MAC_ADB_PB1:
> -	case MAC_ADB_PB2:
> -		func = pmu_read_pram; break;
> -	case MAC_ADB_CUDA:
> -		func = cuda_read_pram; break;
> -	default:
> -		func = via_read_pram;
> -	}
> -	if (!func)
> -		return;
> -	for (i = 0 ; i < len ; i++) {
> -		buffer[i] = (*func)(offset++);
> -	}
> -}
> -
> -void mac_pram_write(int offset, __u8 *buffer, int len)
> -{
> -	void (*func)(int, __u8);
> -	int i;
> -
> -	switch(macintosh_config->adb_type) {
> -	case MAC_ADB_IISI:
> -		func = maciisi_write_pram; break;
> -	case MAC_ADB_PB1:
> -	case MAC_ADB_PB2:
> -		func = pmu_write_pram; break;
> -	case MAC_ADB_CUDA:
> -		func = cuda_write_pram; break;
> -	default:
> -		func = via_write_pram;
> -	}
> -	if (!func)
> -		return;
> -	for (i = 0 ; i < len ; i++) {
> -		(*func)(offset++, buffer[i]);
> -	}
> -}
> -
>  void mac_poweroff(void)
>  {
>  	/*
> 

Re: [PATCH] arch: m68k: mac: misc.c: Remove some unused functions

[Geert Uytterhoeven]

On Sun, Jan 4, 2015 at 8:21 AM, Finn Thain <fthain@telegraphics.com.au> wrote:
> On Thu, 1 Jan 2015, Rickard Strandqvist wrote:
>> Removes some functions that are not used anywhere:
>> mac_pram_write() mac_pram_read()
>
> If you remove those functions, you'd then find that all of the called
> functions become unused:
>
> maciisi_read_pram
> maciisi_write_pram
> pmu_read_pram
> pmu_write_pram
> cuda_read_pram
> cuda_write_pram
> via_read_pram
> via_write_pram
>
> I'd rather not remove all of this code. Better to finish the
> implementation.

Indeed.

> Would it be acceptable to utilize drivers/char/generic_nvram.c and
> CONFIG_GENERIC_NVRAM? This is the PowerMac PRAM driver but looks generic
> enough that it may not need any modification for 68k Macs.

Yes, that would be great.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

nvram and generic_nvram modules are problematic, was Re: [PATCH] arch: m68k: mac: misc.c: Remove some unused functions

[Finn Thain]

On Sun, 4 Jan 2015, Geert Uytterhoeven wrote:

> On Sun, Jan 4, 2015 at 8:21 AM, Finn Thain <fthain@telegraphics.com.au> wrote:
> > On Thu, 1 Jan 2015, Rickard Strandqvist wrote:
> > > Removes some functions that are not used anywhere:
> > > mac_pram_write() mac_pram_read()
> >
> > ... I'd rather not remove all of this code. Better to finish the 
> > implementation.
> 
> Indeed.
> 
> > Would it be acceptable to utilize drivers/char/generic_nvram.c and 
> > CONFIG_GENERIC_NVRAM? This is the PowerMac PRAM driver but looks 
> > generic enough that it may not need any modification for 68k Macs.
> 
> Yes, that would be great.
> 

Unfortunately, it seems to be unworkable.

The only user of generic_nvram is PPC32 (PPC64 could benefit though). 
PPC32 uses the driver by defining both CONFIG_NVRAM and 
CONFIG_GENERIC_NVRAM.

I tried to simplify this so that CONFIG_GENERIC_NVRAM would build 
drivers/char/generic_nvram, while CONFIG_NVRAM would build 
drivers/char/nvram, in order that it would become possible to have a 
multi-platform kernel with both, either, or niether.

But that approach brings new problems:

- An m68k multi-platform kernel would need to contain both modules, and 
  both modules would need MODULE_ALIAS_MISCDEV(NVRAM_MINOR). This isn't 
  going to work. (It's likely that other platforms might also want to use 
  generic_nvram and the same problem would apply to x86 and ARM.)

- The misc device code in drivers/char/nvram is duplicated in 
  generic_nvram. To avoid this duplication, the nvram module could 
  leverage the generic_nvram module instead. This doesn't work either.  
  Systems like mine (Gentoo) have "alias char-major-10-144 nvram" in 
  /etc/modprobe.d/i386.conf, which means that accessing /dev/nvram causes 
  the wrong module to load.

In the end I concluded that the only plausible "generic" driver is 
actually drivers/char/nvram itself. Otherwise it would be under an arch/ 
directory and not under drivers/.

So the nvram module should be the one with 
MODULE_ALIAS_MISCDEV(NVRAM_MAJOR), and it should work on any architecture 
that needs to use it. (Sure enough, drivers/char/generic_nvram lacks the 
MODULE_ALIAS.)

So I believe that the solution is to eliminate drivers/char/generic_nvram 
altogether, and move the architecture-specific code out of 
drivers/char/nvram, so the nvram module can be re-used more easily. I 
think that PPC32, PPC64 and m68k could readily re-use it.

Drivers themselves all test for CONFIG_NVRAM; never CONFIG_GENERIC_NVRAM. 
This is another indication that the generic_nvram driver is surplus to 
requirement.

The CONFIG_PROC_FS support (/proc/driver/nvram) in the drivers/char/nvram 
module is inherently architecture-specific. I suspect that the 
Atari-specific code should move to arch/m68k/atari/ and the x86-specific 
code should move to arch/x86/.

I find the ARM support in drivers/char/nvram to be surprising, not to say 
questionable. The /proc/driver/nvram implementation, given 
defined(__arm__), decodes the NVRAM contents in exactly the same format as 
when defined(__i386__) || defined(__x86_64__). Whereas, only MIPS and 
PowerPC defconfigs set CONFIG_RTC_DRV_CMOS at all, and without that symbol 
the driver will never be built for ARM. This raises the question, does 
/proc/driver/nvram do anything useful on any ARM platforms?

Some guidance on this problem would be appreciated; all the approaches I 
tried led to unsatisfactory compromises. I don't want to keep re-writing 
these patches without a workable plan.

-- 

Re: nvram and generic_nvram modules are problematic, was Re: [PATCH] arch: m68k: mac: misc.c: Remove some unused functions

[Russell King - ARM Linux]

On Sun, Feb 01, 2015 at 02:39:42PM +1100, Finn Thain wrote:
> I find the ARM support in drivers/char/nvram to be surprising, not to say 
> questionable. The /proc/driver/nvram implementation, given 
> defined(__arm__), decodes the NVRAM contents in exactly the same format as 
> when defined(__i386__) || defined(__x86_64__).

That's because it's used on the Netwinder and EBSA285 platforms, which
are PCI-like, complete with a southbridge which makes them look like a
PC.

> Whereas, only MIPS and 
> PowerPC defconfigs set CONFIG_RTC_DRV_CMOS at all, and without that symbol 
> the driver will never be built for ARM. This raises the question, does 
> /proc/driver/nvram do anything useful on any ARM platforms?

Yes.

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

Re: nvram and generic_nvram modules are problematic, was Re: [PATCH] arch: m68k: mac: misc.c: Remove some unused functions

[Finn Thain]

On Sun, 1 Feb 2015, Russell King - ARM Linux wrote:

> On Sun, Feb 01, 2015 at 02:39:42PM +1100, Finn Thain wrote:
> > I find the ARM support in drivers/char/nvram to be surprising, not to 
> > say questionable. The /proc/driver/nvram implementation, given 
> > defined(__arm__), decodes the NVRAM contents in exactly the same 
> > format as when defined(__i386__) || defined(__x86_64__).
> 
> That's because it's used on the Netwinder and EBSA285 platforms, which 
> are PCI-like, complete with a southbridge which makes them look like a 
> PC.

Well, that explains the presence of RTC NVRAM, but the question was also 
about the format of that NVRAM.

The the code in question in drivers/char/nvram.c decodes RTC NVRAM on ARM 
as if it took the same format as x86 PC RTC NVRAM. Apparently they are not 
in the same format (for EBSA-285 at least).

I downloaded the EBSA-285 BIOS,
ftp://ftp.arm.linux.org.uk/pub/armlinux/source/boot/bios-1.11.tar.gz

This is from bios/init/cfg.c --

        img_nr = rtc_read_cmos(0);
        root_dev = rtc_read_cmos(2) | rtc_read_cmos(3) << 8;

        for (i = 0; i < 80; i++) {
                extra_args[i] = rtc_read_cmos(128+i);
                if (!extra_args[i])
                        break;
        }
        extra_args[79] = '\0';

(Like /dev/nvram, file offset 0 is actually device offset 14.)

This EBSA-285 BIOS code uses bytes 128 through 208 as "extra_args", 
whereas /dev/nvram has only 114 bytes in total. The EBSA-285 BIOS doesn't 
read or write any checksum.

The first two bytes aren't reported in /proc/driver/nvram, whereas 
EBSA-285 BIOS uses them for img_nr and root_dev. The next one is reported 
as "floppy type" on x86 PC, though the EBSA-285 BIOS uses it for 
root_dev >> 8.

This is from an x86 PC, for example:

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

My original question was whether the CONFIG_PROC_FS support found in 
drivers/char/nvram.c should be moved to arch/x86 and arch/m68k.

So that question now seems to hinge on the Netwinder ROM ("nettrom") which 
I gather is proprietary (I didn't find any source code). Does anyone know 
what format the Netwinder NVRAM takes?

If NVRAM on Netwinder has a different format to x86 PC, then the 
CONFIG_PROC_FS code in drivers/char/nvram should go elsewhere.

And if the NVRAM on Netwinder had no checksum, drivers/char/nvram.c would 
become a generic /dev/nvram misc device, and drivers/char/generic_nvram.c 
could go away, along with its inherent problems.

-- 

Re: nvram and generic_nvram modules are problematic, was Re: [PATCH] arch: m68k: mac: misc.c: Remove some unused functions

[Geert Uytterhoeven]

On Sun, Feb 1, 2015 at 4:39 AM, Finn Thain <fthain@telegraphics.com.au> wrote:
> On Sun, 4 Jan 2015, Geert Uytterhoeven wrote:
>
>> On Sun, Jan 4, 2015 at 8:21 AM, Finn Thain <fthain@telegraphics.com.au> wrote:
>> > On Thu, 1 Jan 2015, Rickard Strandqvist wrote:
>> > > Removes some functions that are not used anywhere:
>> > > mac_pram_write() mac_pram_read()
>> >
>> > ... I'd rather not remove all of this code. Better to finish the
>> > implementation.
>>
>> Indeed.
>>
>> > Would it be acceptable to utilize drivers/char/generic_nvram.c and
>> > CONFIG_GENERIC_NVRAM? This is the PowerMac PRAM driver but looks
>> > generic enough that it may not need any modification for 68k Macs.
>>
>> Yes, that would be great.
>>
>
> Unfortunately, it seems to be unworkable.

An alternative could be to just provide an nvram attribute file in sysfs,
like many RTC drivers do.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: nvram and generic_nvram modules are problematic, was Re: [PATCH] arch: m68k: mac: misc.c: Remove some unused functions

[Finn Thain]

On Sun, 1 Feb 2015, Geert Uytterhoeven wrote:

> On Sun, Feb 1, 2015 at 4:39 AM, Finn Thain wrote:
> > On Sun, 4 Jan 2015, Geert Uytterhoeven wrote:
> > > On Sun, Jan 4, 2015 at 8:21 AM, Finn Thain wrote:
> > > > On Thu, 1 Jan 2015, Rickard Strandqvist wrote:
> > > > > Removes some functions that are not used anywhere: 
> > > > > mac_pram_write() mac_pram_read()
> > > >
> > > > ... I'd rather not remove all of this code. Better to finish the 
> > > > implementation.
> > >
> > > Indeed.
> > >
> > > > Would it be acceptable to utilize drivers/char/generic_nvram.c and 
> > > > CONFIG_GENERIC_NVRAM? This is the PowerMac PRAM driver but looks 
> > > > generic enough that it may not need any modification for 68k Macs.
> > >
> > > Yes, that would be great.
> > >
> >
> > Unfortunately, it seems to be unworkable.
> 
> An alternative could be to just provide an nvram attribute file in 
> sysfs, like many RTC drivers do.
> 

Are attribute files seekable? Even if userspace could use "/dev/nvram" and 
"/sys/whatever/nvram" interchangably, wouldn't it be better if PPC Macs 
and 68k Macs offered a consistent API to userspace?

And your suggestion doesn't solve the problem, that is, to be able to 
build a multi-platform kernel binary in which drivers can access NVRAM.

The __nvram_read_byte(), nvram_read_byte() etc functions defined in 
drivers/char/nvram.c, if allowed to proliferate because random 
architectures might like to use a generic /dev/nvram API, would further 
uglify that file.

If the m68k Mac kernel doesn't define the nvram_read_byte() routine then 
valkyriefb can't use it. (fbdev drivers are apparently the reason why 
powerpc defines them.)

drivers/char/nvram.c has two sets of these routines for PC RTC NVRAM; one 
for m68k (Atari) and one for arm/x86. We don't want to introduce more code 
into drivers/char/nvram.c to support all four configurations:
1) arm/x86
2) atari
3) atari + mac
4) mac

So we'd end up having to move m68k-specific code out of 
drivers/char/nvram.c, to make it generic. And that then begs all of the 
questions in my previous message.

BTW, my experimental patches replaced all of those __nvram_* and nvram_* 
functions with an ops struct. E.g.

$ cat include/linux/nvram.h 
#ifndef _LINUX_NVRAM_H
#define _LINUX_NVRAM_H

#include <uapi/linux/nvram.h>

struct nvram_ops {
        ssize_t         (*read)(char *, size_t, loff_t *);
        ssize_t         (*write)(char *, size_t, loff_t *);
        unsigned char   (*read_byte)(int);
        void            (*write_byte)(unsigned char, int);
        ssize_t         (*get_size)(void);
#ifdef CONFIG_PPC
        void            (*sync)(void);
#else
        long            (*set_checksum)(void);
        long            (*initialize)(void);
#endif
};

extern const struct nvram_ops arch_nvram_ops;

extern const struct nvram_ops rtc_nvram_ops;

#endif  /* _LINUX_NVRAM_H */


This experiment has m68k implement arch_nvram_ops that dispatch to Atari 
or Mac methods (at compile-time for a single-platform kernel, or at 
run-time for a multi-platform kernel binary).

But this implies modifications to fbdev drivers, PPC32 and PPC64, nvram 
and generic_nvram modules. And any work at all done on generic_nvram seems 
to be misguided, unless it is removal.

--