Message ID | cover.1545784679.git.fthain@telegraphics.com.au (mailing list archive) |
---|---|
Headers | show |
Series | Re-use nvram module | expand |
On Wed, Dec 26, 2018 at 1:43 AM Finn Thain <fthain@telegraphics.com.au> wrote: > This allows for removal of drivers/char/generic_nvram.c as well as some > duplicated code in arch/powerpc/kernel/nvram_64.c. By reducing the number > of /dev/nvram char misc device implementations, the number of bugs and > inconsistencies is also reduced. > > This patch series reduces inconsistencies between PPC32 and PPC64, and > between PPC_PMAC and MAC. A uniform API has benefits for userspace. > For example, some error codes for some ioctl calls become consistent > across PowerPC platforms. The uniform API can potentially benefit > bootloaders that work across different platforms which all have XPRAM > (e.g. Emile). > > I think there are two reasonable merge strategies for this patch series. > The char misc maintainer could take the entire series. Alternatively the > m68k maintainer could take patches 1 thru 14, and after those patches > reach mainline the powerpc maintainer could take 15 thru 25 (even though > patch 21 is not powerpc-related). I had a look at the complete series now, and I think this is a great cleanup. I replied with a couple of minor comments that you may or may not want to address first. The one thing I would like to see resolved (I hope this doesn't bring back an old discussion you had already concluded) is regarding the use of a global exported structure of function pointers, as opposed to using either directly exported functions (with a consistent interface) or a boot-time selectable structure like dma_map_ops or ppc_md. Arnd
On Sat, 29 Dec 2018, Arnd Bergmann wrote: > I had a look at the complete series now, and I think this is a great > cleanup. I replied with a couple of minor comments that you may or may > not want to address first. > Thanks for reviewing this. > The one thing I would like to see resolved (I hope this doesn't bring > back an old discussion you had already concluded) is regarding the use > of a global exported structure of function pointers, as opposed to using > either directly exported functions (with a consistent interface) or a > boot-time selectable structure like dma_map_ops or ppc_md. > If I understand correctly, /dev/nvram was made obsolete by the nvmem subsystem (?). If so, there won't be new /dev/nvram users, and the refactoring here only has to be sufficiently flexible to meet the needs of existing users. I'm not opposed to exported functions in place of a singleton ops struct. Other things being equal I'm inclined toward the ops struct, perhaps because I like encapsulation or perhaps because I don't like excess generality. (That design decision was made years ago and I don't remember the reasoning.) All the arch_nvram_ops structs that I've defined in these patches have the 'const' properly: const struct nvram_ops arch_nvram_ops = { .read_byte = nvram_read_byte, .write_byte = nvram_write_byte, .read = nvram_read, .write = nvram_write, .get_size = nvram_get_size, .set_checksum = nvram_set_checksum, .initialize = nvram_initialize, }; EXPORT_SYMBOL(arch_nvram_ops); This is because there's no need to do any run-time reconfiguration. Is a collection of exported functions a better fit here?
On Sun, 30 Dec 2018, I wrote: > > I'm not opposed to exported functions in place of a singleton ops > struct. Other things being equal I'm inclined toward the ops struct, > perhaps because I like encapsulation or perhaps because I don't like > excess generality. (That design decision was made years ago and I don't > remember the reasoning.) The rationale for the ops struct was that it offers introspection. It turns out that PPC64 device drivers don't care about byte-at-a-time accessors and X86 device drivers don't care about checksum validation. But that only gets us so far. We still needed a way to find out whether the arch has provided byte-at-a-time accessors (i.e. PPC32 and M68K Mac) or byte range accessors (i.e. PPC64 and those platforms with checksummed NVRAM like X86 and M68K Atari). You can't resolve this question at build time for a multi-platform kernel binary, so pre-processor tricks don't help. Device drivers tend to want to access NVRAM one byte at a time. With this patch series, those platforms which need checksum validation always set byte-at-a-time methods to NULL. (Hence the atari_scsi changes in patch 3.) The char misc driver is quite different to the usual device drivers, because the struct file_operations methods always access a byte range. The NULL methods in the ops struct allow the nvram.c misc device to avoid inefficient byte-at-a-time accessors where possible, just as arch/powerpc/kernel/nvram_64.c presently does. --
On Sun, Dec 30, 2018 at 5:05 AM Finn Thain <fthain@telegraphics.com.au> wrote: > > On Sun, 30 Dec 2018, I wrote: > > > > > I'm not opposed to exported functions in place of a singleton ops > > struct. Other things being equal I'm inclined toward the ops struct, > > perhaps because I like encapsulation or perhaps because I don't like > > excess generality. (That design decision was made years ago and I don't > > remember the reasoning.) > > The rationale for the ops struct was that it offers introspection. > > It turns out that PPC64 device drivers don't care about byte-at-a-time > accessors and X86 device drivers don't care about checksum validation. > But that only gets us so far. > > We still needed a way to find out whether the arch has provided > byte-at-a-time accessors (i.e. PPC32 and M68K Mac) or byte range accessors > (i.e. PPC64 and those platforms with checksummed NVRAM like X86 and M68K > Atari). > > You can't resolve this question at build time for a multi-platform kernel > binary, so pre-processor tricks don't help. > > Device drivers tend to want to access NVRAM one byte at a time. With this > patch series, those platforms which need checksum validation always set > byte-at-a-time methods to NULL. (Hence the atari_scsi changes in patch 3.) > > The char misc driver is quite different to the usual device drivers, > because the struct file_operations methods always access a byte range. > > The NULL methods in the ops struct allow the nvram.c misc device to avoid > inefficient byte-at-a-time accessors where possible, just as > arch/powerpc/kernel/nvram_64.c presently does. Ok, I see. That sounds absolutely reasonable, so let's stay with the structure as you proposed. Arnd
On Sun, 30 Dec 2018, I wrote: > > The rationale for the ops struct was that it offers introspection. > > [...] those platforms which need checksum validation always set > byte-at-a-time methods to NULL. > > [...] The NULL methods in the ops struct allow the nvram.c misc device > to avoid inefficient byte-at-a-time accessors where possible, just as > arch/powerpc/kernel/nvram_64.c presently does. > Hopefully my message makes more sense with the tangential irrelevancies removed. I will document these considerations in nvram.h for the next revision. --