Finn Thain <fth...@telegraphics.com.au> writes: > On Mon, 7 Jan 2019, Michael Ellerman wrote: > >> Arnd Bergmann <a...@arndb.de> writes: >> > On Wed, Dec 26, 2018 at 1:43 AM Finn Thain <fth...@telegraphics.com.au> >> > wrote: >> > >> >> +static ssize_t ppc_nvram_get_size(void) >> >> +{ >> >> + if (ppc_md.nvram_size) >> >> + return ppc_md.nvram_size(); >> >> + return -ENODEV; >> >> +} >> > >> >> +const struct nvram_ops arch_nvram_ops = { >> >> + .read = ppc_nvram_read, >> >> + .write = ppc_nvram_write, >> >> + .get_size = ppc_nvram_get_size, >> >> + .sync = ppc_nvram_sync, >> >> +}; >> > >> > Coming back to this after my comment on the m68k side, I notice that >> > there is now a double indirection through function pointers. Have you >> > considered completely removing the operations from ppc_md instead >> > by having multiple copies of nvram_ops? >> > >> > With the current method, it does seem odd to have a single >> > per-architecture instance of the exported structure containing >> > function pointers. This doesn't give us the flexibility of having >> > multiple copies in the kernel the way that ppc_md does, but it adds >> > overhead compared to simply exporting the functions directly. >> >> Yeah TBH I'm not convinced the arch ops is the best solution. >> >> Why can't each arch just implement the required ops functions? On ppc >> we'd still use ppc_md but that would be a ppc detail. >> >> Optional ops are fairly easy to support by providing a default >> implementation, eg. instead of: >> >> + if (arch_nvram_ops.get_size == NULL) >> + return -ENODEV; >> + >> + nvram_size = arch_nvram_ops.get_size(); >> + if (nvram_size < 0) >> + return nvram_size; >> >> >> We do in some header: >> >> #ifndef arch_nvram_get_size >> static inline int arch_nvram_get_size(void) >> { >> return -ENODEV; >> } >> #endif >> >> And then: >> >> nvram_size = arch_nvram_get_size(); >> if (nvram_size < 0) >> return nvram_size; >> >> >> But I haven't digested the whole series so maybe I'm missing something? >> > > The reason why that doesn't work boils down to introspection. (This was > mentioned elsewhere in this email thread.) For example, we presently have > code like this, > > ssize_t nvram_get_size(void) > { > if (ppc_md.nvram_size) > return ppc_md.nvram_size(); > return -1; > } > EXPORT_SYMBOL(nvram_get_size); > > This construction means we get to decide at run-time which of the NVRAM > functions should be used. (Whereas your example makes a build-time decision.)
Right, but we only need to make a runtime decision on powerpc (right?). So it seems to me we should isolate that in the powerpc code. > The purpose of arch_nvram_ops is much the same. That is, it does for m68k > and x86 what ppc_md already does for ppc32 and ppc64. (And once these > platforms share an API like this, they can share more driver code, and > reduce duplicated code.) > The approach taken in this series was to push the arch_nvram_ops approach > as far as possible, because by making everything fit into that regime it > immediately became apparent where architectures and platforms have > diverged, creating weird inconsistencies like the differences in sync > ioctl behaviour between ppc32 and ppc64 for core99. (Early revisions of > this series exposed more issues like bugs and dead code that got addressed > elsewhere.) I just don't see the advantage of having arch_nvram_ops which is a structure of function pointers that are always the same on a given arch, vs some static inlines that implement the same ops for that arch. > Problem is, as Arnd pointed out, powerpc doesn't need both kinds of ops > struct. So I'm rewriting this series in such a way that powerpc doesn't > have to implement both. This rewrite is going to look totally different > for powerpc (though not for x86 or m68k) so you might want to wait for me > to post v9 before spending more time on code review. OK. I know you've been working on this series for a long time and I don't want to roadblock it, so at the end of the day I don't feel that strongly about it as long as the code works. I'll wait for v9 and have another look then. cheers