Re: [PATCH] ib_core: Enable and expose force_mr module parameter

From: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
To: Chuck Lever <chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Cc: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>,
	linux-rdma <linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Subject: Re: [PATCH] ib_core: Enable and expose force_mr module parameter
Date: Wed, 28 Jun 2017 18:41:12 +0300	[thread overview]
Message-ID: <20170628154112.GH1248@mtr-leonro.local> (raw)
In-Reply-To: <373E6A00-8505-468C-A974-DCDAC15A3B7B-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>

[-- Attachment #1: Type: text/plain, Size: 1924 bytes --]

On Wed, Jun 28, 2017 at 10:49:00AM -0400, Chuck Lever wrote:
>
> > On Jun 28, 2017, at 5:56 AM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >
> > On Tue, Jun 27, 2017 at 12:24:20PM +0300, Sagi Grimberg wrote:
> >>
> >>>> It will be "marked" as UAPI and we won't be able to change it,
> >>>> which IMHO bad for the debug. It also invites users to set it
> >>>> and in perfect world, we should test our HCAs with this flag
> >>>> on and off, because curious user can set it.
> >>>
> >>> We have loads of legacy debugging interfaces.
> >>>
> >>> And yes, ULP developers should be aware of it and test with
> >>> both settings.
> >>>
> >>> I'm not convinced any of this is a problem for this particular
> >>> setting. However, I guess the thing for you to do is propose a
> >>> patch.
> >>
> >> Are you guys seriously still on this? Its simply a trivial
> >> debug flag, whats all the fuss really about?
> >
> > I hope that we are done.
> > https://patchwork.kernel.org/patch/9808615/
>
> Somehow this got by me.
>
> The patch description says "exposed to regular users." The
> force_mr option is not exposed to regular users. Module
> parameters are visible only to system administrators.
>
> Also, just looking at the patch, it's not obvious why "The
> force_mr module parameter wasn't exposed ... from the
> beginning". IMO an explanation of what has prevented
> exposure is needed. Commit a060b5629ab0 got the fourth
> parameter of module_param_named wrong. Maybe a Fixes:
> tag is needed too.
>
> But frankly there is no visible change from this patch.
> Before the patch, force_mr is not visible, and after the
> patch it is not visible. Why then is this change necessary?
> It would be helpful to cite a policy about why removal is
> preferred over fixing the incorrect parameter.
>
> Therefore IMO the patch description is not adequate.

I'll update it.

Thanks

>
>
> --
> Chuck Lever
>
>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]