| Message ID | 3ba1dd965c1097e00463eafe7c7d5fd93bbed836.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 |
> --- a/drivers/char/nvram.c > +++ b/drivers/char/nvram.c > @@ -48,6 +48,10 @@ > #include <linux/mutex.h> > #include <linux/pagemap.h> > > +#ifdef CONFIG_PPC > +#include <asm/nvram.h> > +#include <asm/machdep.h> > +#endif > > static DEFINE_MUTEX(nvram_mutex); > static DEFINE_SPINLOCK(nvram_state_lock); > @@ -331,6 +335,37 @@ static long nvram_misc_ioctl(struct file *file, unsigned int cmd, > long ret = -ENOTTY; > > switch (cmd) { > +#ifdef CONFIG_PPC > + case OBSOLETE_PMAC_NVRAM_GET_OFFSET: > + pr_warn("nvram: Using obsolete PMAC_NVRAM_GET_OFFSET ioctl\n"); > + /* fall through */ > + case IOC_NVRAM_GET_OFFSET: > + ret = -EINVAL; > +#ifdef CONFIG_PPC_PMAC I think it would make be nicer here to keep the ppc bits in arch/ppc, and instead add a .ioctl() callback to nvram_ops. > @@ -369,12 +405,14 @@ static int nvram_misc_open(struct inode *inode, struct file *file) > return -EBUSY; > } > > +#ifndef CONFIG_PPC > /* Prevent multiple writers if the set_checksum ioctl is implemented. */ > if ((arch_nvram_ops.set_checksum != NULL) && > (file->f_mode & FMODE_WRITE) && (nvram_open_mode & NVRAM_WRITE)) { > spin_unlock(&nvram_state_lock); > return -EBUSY; > } > +#endif > > diff --git a/include/linux/nvram.h b/include/linux/nvram.h > index b7bfaec60a43..24a57675dba1 100644 > --- a/include/linux/nvram.h > +++ b/include/linux/nvram.h > @@ -18,8 +18,12 @@ struct nvram_ops { > unsigned char (*read_byte)(int); > void (*write_byte)(unsigned char, int); > ssize_t (*get_size)(void); > +#ifdef CONFIG_PPC > + long (*sync)(void); > +#else > long (*set_checksum)(void); > long (*initialize)(void); > +#endif > }; Maybe just leave all entries visible here, and avoid the above #ifdef checks. Arnd
On Sat, 29 Dec 2018, Arnd Bergmann wrote: > > --- a/drivers/char/nvram.c > > +++ b/drivers/char/nvram.c > > @@ -48,6 +48,10 @@ > > #include <linux/mutex.h> > > #include <linux/pagemap.h> > > > > +#ifdef CONFIG_PPC > > +#include <asm/nvram.h> > > +#include <asm/machdep.h> > > +#endif > > > > static DEFINE_MUTEX(nvram_mutex); > > static DEFINE_SPINLOCK(nvram_state_lock); > > @@ -331,6 +335,37 @@ static long nvram_misc_ioctl(struct file *file, unsigned int cmd, > > long ret = -ENOTTY; > > > > switch (cmd) { > > +#ifdef CONFIG_PPC > > + case OBSOLETE_PMAC_NVRAM_GET_OFFSET: > > + pr_warn("nvram: Using obsolete PMAC_NVRAM_GET_OFFSET ioctl\n"); > > + /* fall through */ > > + case IOC_NVRAM_GET_OFFSET: > > + ret = -EINVAL; > > +#ifdef CONFIG_PPC_PMAC > > I think it would make be nicer here to keep the ppc bits in arch/ppc, > and instead add a .ioctl() callback to nvram_ops. > The problem with having an nvram_ops.ioctl() method is the code in the !PPC branch. That code would get duplicated because it's needed by both X86 and M68K, to implement the checksum ioctls. > > @@ -369,12 +405,14 @@ static int nvram_misc_open(struct inode *inode, struct file *file) > > return -EBUSY; > > } > > > > +#ifndef CONFIG_PPC > > /* Prevent multiple writers if the set_checksum ioctl is implemented. */ > > if ((arch_nvram_ops.set_checksum != NULL) && > > (file->f_mode & FMODE_WRITE) && (nvram_open_mode & NVRAM_WRITE)) { > > spin_unlock(&nvram_state_lock); > > return -EBUSY; > > } > > +#endif > > > > diff --git a/include/linux/nvram.h b/include/linux/nvram.h > > index b7bfaec60a43..24a57675dba1 100644 > > --- a/include/linux/nvram.h > > +++ b/include/linux/nvram.h > > @@ -18,8 +18,12 @@ struct nvram_ops { > > unsigned char (*read_byte)(int); > > void (*write_byte)(unsigned char, int); > > ssize_t (*get_size)(void); > > +#ifdef CONFIG_PPC > > + long (*sync)(void); > > +#else > > long (*set_checksum)(void); > > long (*initialize)(void); > > +#endif > > }; > > Maybe just leave all entries visible here, and avoid the above #ifdef checks. > The #ifdef isn't there just to save a few bytes, though it does do that. It's really meant to cause a build failure when I mess up somewhere. But I'm happy to change it if you can see a reason to do so (?)
On Sun, 30 Dec 2018, Finn Thain wrote: > > > diff --git a/include/linux/nvram.h b/include/linux/nvram.h > > > index b7bfaec60a43..24a57675dba1 100644 > > > --- a/include/linux/nvram.h > > > +++ b/include/linux/nvram.h > > > @@ -18,8 +18,12 @@ struct nvram_ops { > > > unsigned char (*read_byte)(int); > > > void (*write_byte)(unsigned char, int); > > > ssize_t (*get_size)(void); > > > +#ifdef CONFIG_PPC > > > + long (*sync)(void); > > > +#else > > > long (*set_checksum)(void); > > > long (*initialize)(void); > > > +#endif > > > }; > > > > Maybe just leave all entries visible here, and avoid the above #ifdef > > checks. > > > > The #ifdef isn't there just to save a few bytes, though it does do that. > It's really meant to cause a build failure when I mess up somewhere. But > I'm happy to change it if you can see a reason to do so (?) > I think the problem with these #ifdef conditionals is that they don't express the correct constraints. So, at the end of this series I'd prefer to see, 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); #if defined(CONFIG_PPC) long (*sync)(void); int (*get_partition)(int); #elif defined(CONFIG_X86) || defined(CONFIG_M68K) long (*set_checksum)(void); long (*initialize)(void); #endif }; Is that okay with you? --
On Sun, Dec 30, 2018 at 8:25 AM Finn Thain <fthain@telegraphics.com.au> wrote: > > On Sat, 29 Dec 2018, Arnd Bergmann wrote: > > > > --- a/drivers/char/nvram.c > > > +++ b/drivers/char/nvram.c > > > @@ -48,6 +48,10 @@ > > > #include <linux/mutex.h> > > > #include <linux/pagemap.h> > > > > > > +#ifdef CONFIG_PPC > > > +#include <asm/nvram.h> > > > +#include <asm/machdep.h> > > > +#endif > > > > > > static DEFINE_MUTEX(nvram_mutex); > > > static DEFINE_SPINLOCK(nvram_state_lock); > > > @@ -331,6 +335,37 @@ static long nvram_misc_ioctl(struct file *file, unsigned int cmd, > > > long ret = -ENOTTY; > > > > > > switch (cmd) { > > > +#ifdef CONFIG_PPC > > > + case OBSOLETE_PMAC_NVRAM_GET_OFFSET: > > > + pr_warn("nvram: Using obsolete PMAC_NVRAM_GET_OFFSET ioctl\n"); > > > + /* fall through */ > > > + case IOC_NVRAM_GET_OFFSET: > > > + ret = -EINVAL; > > > +#ifdef CONFIG_PPC_PMAC > > > > I think it would make be nicer here to keep the ppc bits in arch/ppc, > > and instead add a .ioctl() callback to nvram_ops. > > > > The problem with having an nvram_ops.ioctl() method is the code in the > !PPC branch. That code would get duplicated because it's needed by both > X86 and M68K, to implement the checksum ioctls. I was thinking you'd just have a common ioctl function that falls back to the .ioctl callback for any unhandled commands like switch (cmd) { case NVRAM_INIT: ... break; case ...: break; default: if (ops->ioctl) return ops->ioctl(...); return -EINVAL; } Would that work? Arnd
On Mon, Dec 31, 2018 at 12:13 AM Finn Thain <fthain@telegraphics.com.au> wrote: > On Sun, 30 Dec 2018, Finn Thain wrote: > > > > diff --git a/include/linux/nvram.h b/include/linux/nvram.h > > > > index b7bfaec60a43..24a57675dba1 100644 > > > > --- a/include/linux/nvram.h > > > > +++ b/include/linux/nvram.h > > > > @@ -18,8 +18,12 @@ struct nvram_ops { > > > > unsigned char (*read_byte)(int); > > > > void (*write_byte)(unsigned char, int); > > > > ssize_t (*get_size)(void); > > > > +#ifdef CONFIG_PPC > > > > + long (*sync)(void); > > > > +#else > > > > long (*set_checksum)(void); > > > > long (*initialize)(void); > > > > +#endif > > > > }; > > > > > > Maybe just leave all entries visible here, and avoid the above #ifdef > > > checks. > > > > > > > The #ifdef isn't there just to save a few bytes, though it does do that. > > It's really meant to cause a build failure when I mess up somewhere. But > > I'm happy to change it if you can see a reason to do so (?) > > > > I think the problem with these #ifdef conditionals is that they don't > express the correct constraints. So, at the end of this series I'd prefer > to see, > > 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); > #if defined(CONFIG_PPC) > long (*sync)(void); > int (*get_partition)(int); > #elif defined(CONFIG_X86) || defined(CONFIG_M68K) > long (*set_checksum)(void); > long (*initialize)(void); > #endif > }; > > Is that okay with you? My preference would be no #ifdef here, but the compile time error you mention is a good enough reason, so I'm fine with either version you pick. Arnd
On Mon, 31 Dec 2018, Arnd Bergmann wrote: > On Sun, Dec 30, 2018 at 8:25 AM Finn Thain <fthain@telegraphics.com.au> wrote: > > > > On Sat, 29 Dec 2018, Arnd Bergmann wrote: > > > > > > --- a/drivers/char/nvram.c > > > > +++ b/drivers/char/nvram.c > > > > @@ -48,6 +48,10 @@ > > > > #include <linux/mutex.h> > > > > #include <linux/pagemap.h> > > > > > > > > +#ifdef CONFIG_PPC > > > > +#include <asm/nvram.h> > > > > +#include <asm/machdep.h> > > > > +#endif > > > > > > > > static DEFINE_MUTEX(nvram_mutex); > > > > static DEFINE_SPINLOCK(nvram_state_lock); > > > > @@ -331,6 +335,37 @@ static long nvram_misc_ioctl(struct file *file, unsigned int cmd, > > > > long ret = -ENOTTY; > > > > > > > > switch (cmd) { > > > > +#ifdef CONFIG_PPC > > > > + case OBSOLETE_PMAC_NVRAM_GET_OFFSET: > > > > + pr_warn("nvram: Using obsolete PMAC_NVRAM_GET_OFFSET ioctl\n"); > > > > + /* fall through */ > > > > + case IOC_NVRAM_GET_OFFSET: > > > > + ret = -EINVAL; > > > > +#ifdef CONFIG_PPC_PMAC > > > > > > I think it would make be nicer here to keep the ppc bits in arch/ppc, > > > and instead add a .ioctl() callback to nvram_ops. > > > > > > > The problem with having an nvram_ops.ioctl() method is the code in the > > !PPC branch. That code would get duplicated because it's needed by > > both X86 and M68K, to implement the checksum ioctls. > > I was thinking you'd just have a common ioctl function that falls back > to the .ioctl callback for any unhandled commands like > > switch (cmd) { > case NVRAM_INIT: > ... > break; > case ...: > break; > default: > if (ops->ioctl) > return ops->ioctl(...); > return -EINVAL; > } > > Would that work? > There are no ioctls common to all architectures. So your example becomes, static long nvram_misc_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { if (ops->ioctl) return ops->ioctl(file, cmd, arg); return -ENOTTY; } And then my objection is the same: m68k and x86 now have to duplicate their common ops->ioctl() implementation. Here's a compromise that avoids some code duplication. switch (cmd) { #if defined(CONFIG_X86) || defined(CONFIG_M68K) case NVRAM_INIT: ... break; case NVRAM_SETCKS: ... break; #endif default: if (ops->ioctl) return ops->ioctl(...); return -EINVAL; } But PPC64 and PPC32 also need to share their ops->ioctl() implementation. It's not clear to me where that code would go. Personally, I prefer the present patch series, or something similar, with it's symmetry between nvram.c and nvram.h: static long nvram_misc_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { long ret = -ENOTTY; switch (cmd) { #if defined(CONFIG_PPC) case OBSOLETE_PMAC_NVRAM_GET_OFFSET: ... case IOC_NVRAM_GET_OFFSET: ... break; case IOC_NVRAM_SYNC: ... break; #elif defined(CONFIG_X86) || defined(CONFIG_M68K) case NVRAM_INIT: ... break; case NVRAM_SETCKS: ... break; #endif } return ret; } ... versus the struct definition in 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); #if defined(CONFIG_PPC) long (*sync)(void); int (*get_partition)(int); #elif defined(CONFIG_X86) || defined(CONFIG_M68K) long (*set_checksum)(void); long (*initialize)(void); #endif }; Which of these alternatives do you prefer? Is there a better way?
On Tue, 1 Jan 2019, I wrote: > > There are no [nvram] ioctls common to all architectures. So your example > becomes, > > static long nvram_misc_ioctl(struct file *file, unsigned int cmd, > unsigned long arg) > { > if (ops->ioctl) > return ops->ioctl(file, cmd, arg); > return -ENOTTY; > } > > And then my objection is the same: m68k and x86 now have to duplicate > their common ops->ioctl() implementation. > Perhaps code duplication is inevitable. Either you punt the ioctl implementation from the char misc driver (and duplicate that ioctl implementation under arch/) or else you duplicate the char misc driver. Maybe this dilemma explains the situation we have now* which is duplicated drivers (drivers/char/nvram.c and drivers/char/generic_nvram.c). But this explanation doesn't seem to offer any solution. Re-using either of the existing drivers seems to be impossible. Different interpretations of the NVRAM Kconfig symbol accross the tree are not helping. And having separate Kconfig symbols (NVRAM and GENERIC_NVRAM) for the two drivers doesn't help either. But maybe the NVRAM symbol can be dropped from arch/powerpc and all of the powerpc drivers... * Actually, we presently have duplicated misc device drivers AND duplicated ioctl implementations too, for good measure. See arch/powerpc/kernel/nvram_64.c and drivers/char/generic_nvram.c. --
diff --git a/arch/powerpc/include/asm/nvram.h b/arch/powerpc/include/asm/nvram.h index 56a388da9c4f..629a5cdcc865 100644 --- a/arch/powerpc/include/asm/nvram.h +++ b/arch/powerpc/include/asm/nvram.h @@ -78,9 +78,6 @@ extern int pmac_get_partition(int partition); extern u8 pmac_xpram_read(int xpaddr); extern void pmac_xpram_write(int xpaddr, u8 data); -/* Synchronize NVRAM */ -extern void nvram_sync(void); - /* Initialize NVRAM OS partition */ extern int __init nvram_init_os_partition(struct nvram_os_partition *part); diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c index ee91bba0805d..e0d045677472 100644 --- a/arch/powerpc/kernel/setup_32.c +++ b/arch/powerpc/kernel/setup_32.c @@ -152,7 +152,6 @@ __setup("l3cr=", ppc_setup_l3cr); #ifdef CONFIG_GENERIC_NVRAM -/* Generic nvram hooks used by drivers/char/gen_nvram.c */ unsigned char nvram_read_byte(int addr) { if (ppc_md.nvram_read_val) @@ -175,15 +174,16 @@ static ssize_t ppc_nvram_get_size(void) return -ENODEV; } -void nvram_sync(void) +static long ppc_nvram_sync(void) { if (ppc_md.nvram_sync) ppc_md.nvram_sync(); + return 0; } -EXPORT_SYMBOL(nvram_sync); const struct nvram_ops arch_nvram_ops = { .get_size = ppc_nvram_get_size, + .sync = ppc_nvram_sync, }; EXPORT_SYMBOL(arch_nvram_ops); diff --git a/drivers/char/generic_nvram.c b/drivers/char/generic_nvram.c index a7dfde734897..f32d5663de95 100644 --- a/drivers/char/generic_nvram.c +++ b/drivers/char/generic_nvram.c @@ -96,7 +96,7 @@ static int nvram_ioctl(struct file *file, unsigned int cmd, unsigned long arg) } #endif /* CONFIG_PPC_PMAC */ case IOC_NVRAM_SYNC: - nvram_sync(); + arch_nvram_ops.sync(); break; default: return -EINVAL; diff --git a/drivers/char/nvram.c b/drivers/char/nvram.c index b77c27d68762..466e1fb02052 100644 --- a/drivers/char/nvram.c +++ b/drivers/char/nvram.c @@ -48,6 +48,10 @@ #include <linux/mutex.h> #include <linux/pagemap.h> +#ifdef CONFIG_PPC +#include <asm/nvram.h> +#include <asm/machdep.h> +#endif static DEFINE_MUTEX(nvram_mutex); static DEFINE_SPINLOCK(nvram_state_lock); @@ -331,6 +335,37 @@ static long nvram_misc_ioctl(struct file *file, unsigned int cmd, long ret = -ENOTTY; switch (cmd) { +#ifdef CONFIG_PPC + case OBSOLETE_PMAC_NVRAM_GET_OFFSET: + pr_warn("nvram: Using obsolete PMAC_NVRAM_GET_OFFSET ioctl\n"); + /* fall through */ + case IOC_NVRAM_GET_OFFSET: + ret = -EINVAL; +#ifdef CONFIG_PPC_PMAC + if (machine_is(powermac)) { + int part, offset; + + if (copy_from_user(&part, (void __user *)arg, + sizeof(part)) != 0) + return -EFAULT; + if (part < pmac_nvram_OF || part > pmac_nvram_NR) + return -EINVAL; + offset = pmac_get_partition(part); + if (copy_to_user((void __user *)arg, + &offset, sizeof(offset)) != 0) + return -EFAULT; + ret = 0; + } +#endif + break; + case IOC_NVRAM_SYNC: + if (arch_nvram_ops.sync != NULL) { + mutex_lock(&nvram_mutex); + ret = arch_nvram_ops.sync(); + mutex_unlock(&nvram_mutex); + } + break; +#else /* !CONFIG_PPC */ case NVRAM_INIT: /* initialize NVRAM contents and checksum */ if (!capable(CAP_SYS_ADMIN)) @@ -354,6 +389,7 @@ static long nvram_misc_ioctl(struct file *file, unsigned int cmd, mutex_unlock(&nvram_mutex); } break; +#endif /* CONFIG_PPC */ } return ret; } @@ -369,12 +405,14 @@ static int nvram_misc_open(struct inode *inode, struct file *file) return -EBUSY; } +#ifndef CONFIG_PPC /* Prevent multiple writers if the set_checksum ioctl is implemented. */ if ((arch_nvram_ops.set_checksum != NULL) && (file->f_mode & FMODE_WRITE) && (nvram_open_mode & NVRAM_WRITE)) { spin_unlock(&nvram_state_lock); return -EBUSY; } +#endif if (file->f_flags & O_EXCL) nvram_open_mode |= NVRAM_EXCL; diff --git a/include/linux/nvram.h b/include/linux/nvram.h index b7bfaec60a43..24a57675dba1 100644 --- a/include/linux/nvram.h +++ b/include/linux/nvram.h @@ -18,8 +18,12 @@ struct nvram_ops { unsigned char (*read_byte)(int); void (*write_byte)(unsigned char, int); ssize_t (*get_size)(void); +#ifdef CONFIG_PPC + long (*sync)(void); +#else long (*set_checksum)(void); long (*initialize)(void); +#endif }; extern const struct nvram_ops arch_nvram_ops;