From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v3 12/13] xen/iommu: smmu: Add Xen specific code to be able to use the driver Date: Fri, 20 Feb 2015 14:01:55 +0000 Message-ID: <54E73E53.9030505@linaro.org> References: <1422643768-23614-1-git-send-email-julien.grall@linaro.org> <1422643768-23614-13-git-send-email-julien.grall@linaro.org> <54D8D4D1.102@linaro.org> <1424438999.30924.231.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YOo9x-000452-I2 for xen-devel@lists.xenproject.org; Fri, 20 Feb 2015 14:02:25 +0000 Received: by wesu56 with SMTP id u56so5744388wes.10 for ; Fri, 20 Feb 2015 06:02:23 -0800 (PST) In-Reply-To: <1424438999.30924.231.camel@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: xen-devel@lists.xenproject.org, tim@xen.org, stefano.stabellini@citrix.com, Stefano Stabellini List-Id: xen-devel@lists.xenproject.org On 20/02/15 13:29, Ian Campbell wrote: > On Mon, 2015-02-09 at 23:40 +0800, Julien Grall wrote: >> Hi Stefano, >> >> On 06/02/2015 21:20, Stefano Stabellini wrote: >>> On Fri, 30 Jan 2015, Julien Grall wrote: >>>> -static int force_stage; >>>> -module_param_named(force_stage, force_stage, int, S_IRUGO | S_IWUSR); >>>> -MODULE_PARM_DESC(force_stage, >>>> - "Force SMMU mappings to be installed at a particular stage of translation. A value of '1' or '2' forces the corresponding stage. All other values are ignored (i.e. no stage is forced). Note that selecting a specific stage will disable support for nested translation."); >>> >>> for the sake of minimizing the diff, I would add >>> >>> #define module_param_named(a, b, c, d) >>> #define MODULE_PARM_DESC(a, b) >>> >>> to the Xen definitions above >> >> We still have to drop force_stage because it will get unused and won't >> compile. >> So it's not too bad to remove the 2 other lines. > > You currently redefine it as a const int -- is it really unused? This variable is only read to force the driver to use the specify stage (1 or 2). On Xen, we only want to use stage-2. The driver will bail out if the SMMU doesn't support it. >> All the Xen code is beginning with /* Xen: */. Though, I could add a /* >> Xen: End */ to make the separation clearer. > > BTW, I was thinking to suggest replacing all the #if 0 with #ifndef > CONFIG_XEN or something, which might make some of the comments shorter > or even mean they aren't really needed. > > Either way I would write it as: > #if 0 /* Xen: Comment comment */ > (similarly any if (0) ...) I prefer the #if 0 /* Xen: */ >>> For the sake of minimizing the changes to Linux functions, could you >>> write a wrapper around this function instead? That might allow you to >>> remove the changes related to the return value. >> >> We should avoid to try to minimize the code against the clarity of the >> code... >> >> The change you suggest would require to modify the caller of this >> function, which is less clearer than this one. > > Given an original function foo can you do > > static void foo(void) > { > original code, unmodified > } > static void foo_wrapper(void) > { > wrapper > foo_orig > wrapper > } > #define foo foo_wrapper > > or something along those lines? There is some case where we need to modify the original function. So I'm not sure it worth to add more line just for a wrapper. Regards, -- Julien Grall