Received: by 2002:ad5:474a:0:0:0:0:0 with SMTP id i10csp3164982imu; Sun, 6 Jan 2019 20:53:42 -0800 (PST) X-Google-Smtp-Source: ALg8bN6CqZPEFpLmrfIbUaXfeoxD5/jV7mclydGbPFEbVF4uKjN8qUfQVqi4ObKDRPdYXza4YTyj X-Received: by 2002:a17:902:42e4:: with SMTP id h91mr60948886pld.18.1546836822429; Sun, 06 Jan 2019 20:53:42 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1546836822; cv=none; d=google.com; s=arc-20160816; b=AEe9KtxAK5Z8NAg4aSmuZQ7trDPFAyeo9NOy50gdZWIrzgKVq8/YOMluK1anX4jzg5 VOr3adgfUE4cU6ZkWOKEtQLqqz20rCriAEvoYVktjfOYe3PeE1QRS+WHtzPngjfuutVo VLZCMUEJdTK6J+jV3WB2EeZyIx7n8wmFb1qth4gi+43Rucd0nZdYPLbptFP/+K2gvSLy khU+mmLfg1zyjzYg0ATFHaeJ/xeJNIDJDbTC3+mso/otC/EpEOC+BC0G75BgyZJOEzDC UaozRX2sbIawnia5Uh+SznOT2AUHlnPL3hSdA0urvsMhWaCW9VsJG0BWqNTzK4Y1G39e KhUw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:mime-version:references:message-id :in-reply-to:subject:cc:to:from:date; bh=3ff0bGp7KL+pLrOsFpeIVyVVJZAgL3F2JtInHd/GPBs=; b=MlhA6NqUIGUOdSukHjydumb2wH6yUmZ7I8PqJC6DWMxVw+tn2WPc7Rn8ym0XTHpqit TAbfUpstOtRu6vQIPEb6zu0nxQpFqaO6AKRE6vTIWRqv7BGXgNxvO3zMdRjM4k+/XlYH 5+tpuDZSW0WudEdkdhuSd5ZPbWgWJ37BL4i7Wfi8gvG0QuuBBRrDJTdn+un+U3InU2QQ 7E/k/JaIfvcZuOxa9B34gxZMTfICJRF6nV5BA6ba25QUJhQZbvqp4k6MXpGlvR64MPBz YY+xO7OFugttuYFCHwcX9w7bQVZogRHQmpTUoax+i5IPiaSFotOL+12l3JfUJAgBCLx1 2hzQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id v3si36507677pga.556.2019.01.06.20.53.23; Sun, 06 Jan 2019 20:53:42 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726425AbfAGEwK (ORCPT + 99 others); Sun, 6 Jan 2019 23:52:10 -0500 Received: from kvm5.telegraphics.com.au ([98.124.60.144]:45578 "EHLO kvm5.telegraphics.com.au" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726287AbfAGEwK (ORCPT ); Sun, 6 Jan 2019 23:52:10 -0500 Received: from localhost (localhost.localdomain [127.0.0.1]) by kvm5.telegraphics.com.au (Postfix) with ESMTP id 3632828DE0; Sun, 6 Jan 2019 23:52:02 -0500 (EST) Date: Mon, 7 Jan 2019 15:52:38 +1100 (AEDT) From: Finn Thain To: Michael Ellerman cc: Arnd Bergmann , Greg Kroah-Hartman , Benjamin Herrenschmidt , Paul Mackerras , Linux Kernel Mailing List , linux-m68k , linuxppc-dev Subject: Re: [PATCH v8 24/25] powerpc: Adopt nvram module for PPC64 In-Reply-To: <8736q5xst1.fsf@concordia.ellerman.id.au> Message-ID: References: <2fe2b8e6395aeacfafcbde590a50922d4e632189.1545784679.git.fthain@telegraphics.com.au> <8736q5xst1.fsf@concordia.ellerman.id.au> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 7 Jan 2019, Michael Ellerman wrote: > Arnd Bergmann writes: > > On Wed, Dec 26, 2018 at 1:43 AM Finn Thain 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.) 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.) 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. Thanks. -- > cheers >