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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel