[Intel-gfx] [PATCH] drm/i915: Rename global i915 to i915_modparams

[Jani Nikula]

On Tue, 19 Sep 2017, Joonas Lahtinen <joonas.lahtinen at linux.intel.com> wrote:
> On Tue, 2017-09-19 at 16:15 +0300, Ville Syrjälä wrote:
>> On Tue, Sep 19, 2017 at 04:07:17PM +0300, Joonas Lahtinen wrote:
>> > On Tue, 2017-09-19 at 13:22 +0300, Ville Syrjälä wrote:
>> > > On Tue, Sep 19, 2017 at 11:24:41AM +0300, Joonas Lahtinen wrote:
>> > > > On Mon, 2017-09-18 at 22:07 +0200, Michal Wajdeczko wrote:
>> > > > > On Mon, 18 Sep 2017 21:11:40 +0200, Jani Nikula <jani.nikula at intel.com>  
>> > > > > wrote:
>> > > > > 
>> > > > > > On Mon, 18 Sep 2017, Michal Wajdeczko <michal.wajdeczko at intel.com> wrote:
>> > > > > > > Our global struct with params is named exactly the same way
>> > > > > > > as new preferred name for the drm_i915_private function parameter.
>> > > > > > > To avoid such name reuse lets use different name for the global.
>> > > > > > > 
>> > > > > > > v4: introduction of mkwrite()
>> > > > > > 
>> > > > > > Why?
>> > > > > > 
>> > > > > > I don't know what you're trying to achieve with the mkwrite() stuff (the
>> > > > > 
>> > > > > I was trying to buy at least one more vote, as discussed on IRC
>> > > > > 
>> > > > > <quote>
>> > > > > [14:23:36] <dolphin> I'll be glad to vote for i915_modparams +  
>> > > > > i915_modparams_mkwrite()
>> > > > > <quote/>
>> > > > > 
>> > > > > > commit message would be the perfect place to explain that) but no matter
>> > > > > > what it should IMO be a separate patch.
>> > > > > > 
>> > > > > > I think the simple s/i915/i915_modparams/ would be fine, and we could
>> > > > > > move on.
>> > > > > 
>> > > > > Note that it all started with this idea.
>> > > > > See https://patchwork.freedesktop.org/patch/176409/
>> > > > > 
>> > > > 
>> > > > I agree with Jani that the pure rename should be its own patch. That'll
>> > > > make review much easier. Then have a follow-up that introduces
>> > > > _mkwrite() and as a bonus makes the struct const or at least makes
>> > > > sparse complain.
>> > > 
>> > > I know we abuse the const+mkwrite type of thing for the device info, but
>> > > I'm not sure how safe that actually is on account of the compiler being
>> > > free to assume that const stuff doesn't generally change. I guess if the
>> > > mkwrite thing happens at some early controlled point it's going to be OK,
>> > > but if it starts happening at some randomish times we might not be so
>> > > lucky.
>> > 
>> > I see this more as a reason to introduce it.
>> 
>> Introduce what exactly? A bug due to compiler optimizing away some read of
>> the variable because it can assume that it didn't change?
>
> "Introduce enforcement of taking special action to acknowledge
> consideration before writing to i915_modparams, to avoid bugs due to
> writing to variables that are not expected to change."
>
>   -- Joonas Lahtinen, 2017
>
> If somebody is still unclear what I'm after, I'm out of words.

I think the communication bug here is that tou keep arguing for your
goal, we keep arguing against your approach to achieve that goal.

I agree changing the module parameters (both within the driver and via
sysfs) should be managed better.

I don't think we can make the struct i915_params variable const.

We could make the struct i915_params static within i915_params.c, and
add a driver global const pointer to it. But that doesn't help with
e.g. parameter changes originating from sysfs. I also don't particularly
like the driver changing parameters such that the default -1 gets
"sanitized" to something or the other. I'd expect the users to see in
sysfs whatever they set in kernel command line or at probe time or in
the sysfs. Adding constness doesn't help that either.

BR,
Jani.


>
>> I think this needs to be well thought out to make sure we don't end up
>> with some impossible looking bugs.
>
> That's why we're discussing it. My bet is most of the potentially
> troublesome variables belong to to the device state anyway (vs. module
> state). It's just a dumping ground of global variables currently.
>
> Regards, Joonas

-- 
Jani Nikula, Intel Open Source Technology Center