| Message ID | 66ed571883695314ace53080df8f6dfb132a8b4d.1545784679.git.fthain@telegraphics.com.au (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Re-use nvram module | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | warning | next/apply_patch Patch failed to apply |
| snowpatch_ozlabs/apply_patch | fail | Failed to apply to any branch |
> + > +const struct nvram_ops arch_nvram_ops = { > + .read_byte = nvram_read_byte, > + .write_byte = nvram_write_byte, > + .get_size = nvram_get_size, > +}; > +EXPORT_SYMBOL(arch_nvram_ops); I think something this internal should always be EXPORT_SYMBOL_GPL.
On Thu, 3 Jan 2019, Christoph Hellwig wrote: > > + > > +const struct nvram_ops arch_nvram_ops = { > > + .read_byte = nvram_read_byte, > > + .write_byte = nvram_write_byte, > > + .get_size = nvram_get_size, > > +}; > > +EXPORT_SYMBOL(arch_nvram_ops); > > I think something this internal should always be EXPORT_SYMBOL_GPL. > By "internal" do you mean "not involving interests outside of the Linux Foundation"? TBH, I don't know who's affected. Anyway, this patch series is mostly refactoring. So the policy it enforces towards the use of arch_nvram_ops just reflects the policy already in place: $ git grep -w EXPORT_SYMBOL_GPL | grep nvram $ git grep -w EXPORT_SYMBOL | grep nvram ... arch/powerpc/kernel/setup_32.c:EXPORT_SYMBOL(nvram_read_byte); arch/powerpc/kernel/setup_32.c:EXPORT_SYMBOL(nvram_write_byte); arch/powerpc/kernel/setup_32.c:EXPORT_SYMBOL(nvram_get_size); arch/powerpc/kernel/setup_32.c:EXPORT_SYMBOL(nvram_sync); arch/powerpc/platforms/powermac/nvram.c:EXPORT_SYMBOL(pmac_get_partition); arch/powerpc/platforms/powermac/nvram.c:EXPORT_SYMBOL(pmac_xpram_read); arch/powerpc/platforms/powermac/nvram.c:EXPORT_SYMBOL(pmac_xpram_write); drivers/char/nvram.c:EXPORT_SYMBOL(__nvram_read_byte); drivers/char/nvram.c:EXPORT_SYMBOL(nvram_read_byte); drivers/char/nvram.c:EXPORT_SYMBOL(__nvram_write_byte); drivers/char/nvram.c:EXPORT_SYMBOL(nvram_write_byte); drivers/char/nvram.c:EXPORT_SYMBOL(__nvram_check_checksum); drivers/char/nvram.c:EXPORT_SYMBOL(nvram_check_checksum); ... $ --
On Fri, Jan 04, 2019 at 09:08:13AM +1100, Finn Thain wrote: > On Thu, 3 Jan 2019, Christoph Hellwig wrote: > > > > + > > > +const struct nvram_ops arch_nvram_ops = { > > > + .read_byte = nvram_read_byte, > > > + .write_byte = nvram_write_byte, > > > + .get_size = nvram_get_size, > > > +}; > > > +EXPORT_SYMBOL(arch_nvram_ops); > > > > I think something this internal should always be EXPORT_SYMBOL_GPL. > > > > By "internal" do you mean "not involving interests outside of the Linux > Foundation"? TBH, I don't know who's affected. Linux Foundation doesn't matter. But as far as I can see you export the struct from arch code for nvram.ko to use them and no one else, which is clearly internal.
On Fri, 4 Jan 2019, Christoph Hellwig wrote: > On Fri, Jan 04, 2019 at 09:08:13AM +1100, Finn Thain wrote: > > On Thu, 3 Jan 2019, Christoph Hellwig wrote: > > > > > > + > > > > +const struct nvram_ops arch_nvram_ops = { > > > > + .read_byte = nvram_read_byte, > > > > + .write_byte = nvram_write_byte, > > > > + .get_size = nvram_get_size, > > > > +}; > > > > +EXPORT_SYMBOL(arch_nvram_ops); > > > > > > I think something this internal should always be EXPORT_SYMBOL_GPL. > > > > > > > By "internal" do you mean "not involving interests outside of the Linux > > Foundation"? TBH, I don't know who's affected. > > Linux Foundation doesn't matter. > > But as far as I can see you export the struct from arch code > for nvram.ko to use them and no one else, which is clearly internal. > No, arch_nvram_ops is not "internal" in that sense. The struct is used whereever the nvram_* exports are presently used: $ git grep -e "EXPORT_SYMBOL.*nvram" v4.20 ... v4.20:arch/powerpc/kernel/setup_32.c:EXPORT_SYMBOL(nvram_read_byte); v4.20:arch/powerpc/kernel/setup_32.c:EXPORT_SYMBOL(nvram_write_byte); v4.20:arch/powerpc/kernel/setup_32.c:EXPORT_SYMBOL(nvram_get_size); v4.20:arch/powerpc/kernel/setup_32.c:EXPORT_SYMBOL(nvram_sync); v4.20:drivers/char/nvram.c:EXPORT_SYMBOL(__nvram_read_byte); v4.20:drivers/char/nvram.c:EXPORT_SYMBOL(nvram_read_byte); v4.20:drivers/char/nvram.c:EXPORT_SYMBOL(__nvram_write_byte); v4.20:drivers/char/nvram.c:EXPORT_SYMBOL(nvram_write_byte); v4.20:drivers/char/nvram.c:EXPORT_SYMBOL(__nvram_check_checksum); v4.20:drivers/char/nvram.c:EXPORT_SYMBOL(nvram_check_checksum); ... $ git grep -wle "nvram_read_byte|nvram_write_byte|nvram_get_size|nvram_sync|__nvram_read_byte|__nvram_write_byte|__nvram_check_checksum" arch/powerpc/include/asm/machdep.h arch/powerpc/include/asm/nvram.h arch/powerpc/kernel/setup_32.c arch/powerpc/platforms/powermac/nvram.c drivers/char/generic_nvram.c drivers/char/nvram.c drivers/platform/x86/thinkpad_acpi.c drivers/scsi/atari_scsi.c drivers/video/fbdev/controlfb.c drivers/video/fbdev/imsttfb.c drivers/video/fbdev/matrox/matroxfb_base.c drivers/video/fbdev/platinumfb.c drivers/video/fbdev/valkyriefb.c include/linux/nvram.h sound/ppc/awacs.c $ --
diff --git a/drivers/char/nvram.c b/drivers/char/nvram.c index c660cff9faf4..00897daa0643 100644 --- a/drivers/char/nvram.c +++ b/drivers/char/nvram.c @@ -52,9 +52,11 @@ static DEFINE_MUTEX(nvram_mutex); static DEFINE_SPINLOCK(nvram_state_lock); static int nvram_open_cnt; /* #times opened */ static int nvram_open_mode; /* special open modes */ +static ssize_t nvram_size; #define NVRAM_WRITE 1 /* opened for writing (exclusive) */ #define NVRAM_EXCL 2 /* opened with O_EXCL */ +#ifdef CONFIG_X86 /* * These functions are provided to be called internally or by other parts of * the kernel. It's up to the caller to ensure correct checksum before reading @@ -162,6 +164,19 @@ void nvram_set_checksum(void) } #endif /* 0 */ +static ssize_t nvram_get_size(void) +{ + return NVRAM_BYTES; +} + +const struct nvram_ops arch_nvram_ops = { + .read_byte = nvram_read_byte, + .write_byte = nvram_write_byte, + .get_size = nvram_get_size, +}; +EXPORT_SYMBOL(arch_nvram_ops); +#endif /* CONFIG_X86 */ + /* * The are the file operation function for user access to /dev/nvram */ @@ -169,7 +184,7 @@ void nvram_set_checksum(void) static loff_t nvram_misc_llseek(struct file *file, loff_t offset, int origin) { return generic_file_llseek_size(file, offset, origin, MAX_LFS_FILESIZE, - NVRAM_BYTES); + nvram_size); } static ssize_t nvram_misc_read(struct file *file, char __user *buf, @@ -320,8 +335,7 @@ static int nvram_misc_release(struct inode *inode, struct file *file) return 0; } -#ifdef CONFIG_PROC_FS - +#if defined(CONFIG_X86) && defined(CONFIG_PROC_FS) static const char * const floppy_types[] = { "none", "5.25'' 360k", "5.25'' 1.2M", "3.5'' 720k", "3.5'' 1.44M", "3.5'' 2.88M", "3.5'' 2.88M" @@ -411,7 +425,7 @@ static int nvram_proc_read(struct seq_file *seq, void *offset) return 0; } -#endif /* CONFIG_PROC_FS */ +#endif /* CONFIG_X86 && CONFIG_PROC_FS */ static const struct file_operations nvram_misc_fops = { .owner = THIS_MODULE, @@ -433,13 +447,20 @@ static int __init nvram_module_init(void) { int ret; + if (arch_nvram_ops.get_size == NULL) + return -ENODEV; + + nvram_size = arch_nvram_ops.get_size(); + if (nvram_size < 0) + return nvram_size; + ret = misc_register(&nvram_misc); if (ret) { pr_err("nvram: can't misc_register on minor=%d\n", NVRAM_MINOR); return ret; } -#ifdef CONFIG_PROC_FS +#if defined(CONFIG_X86) && defined(CONFIG_PROC_FS) if (!proc_create_single("driver/nvram", 0, NULL, nvram_proc_read)) { pr_err("nvram: can't create /proc/driver/nvram\n"); misc_deregister(&nvram_misc); @@ -453,7 +474,7 @@ static int __init nvram_module_init(void) static void __exit nvram_module_exit(void) { -#ifdef CONFIG_PROC_FS +#if defined(CONFIG_X86) && defined(CONFIG_PROC_FS) remove_proc_entry("driver/nvram", NULL); #endif misc_deregister(&nvram_misc); diff --git a/include/linux/nvram.h b/include/linux/nvram.h index 4f78147e74d9..d1bdee50d6a8 100644 --- a/include/linux/nvram.h +++ b/include/linux/nvram.h @@ -15,6 +15,8 @@ extern int nvram_check_checksum(void); 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); };
Different platforms and architectures offer different NVRAM sizes and access methods. E.g. PPC32 has byte-at-a-time accessor functions whereas PPC64 has byte-range accessor functions. Adopt the nvram_ops struct so the nvram module can call such functions as are defined by the various platforms and architectures. Signed-off-by: Finn Thain <fthain@telegraphics.com.au> --- The procfs code here could be moved to arch/x86 (like the earlier patch did for m68k code that was here). The nvram_ops struct could be implemented and exported by the rtc-cmos driver instead, to eliminate the remaining #ifdefs. --- drivers/char/nvram.c | 33 +++++++++++++++++++++++++++------ include/linux/nvram.h | 2 ++ 2 files changed, 29 insertions(+), 6 deletions(-)