[Intel-gfx] [PATCH v6 3/3] drm/i915: Make i915_modparams members const

[Michal Wajdeczko]

On Wed, 20 Sep 2017 15:07:50 +0200, Joonas Lahtinen  
<joonas.lahtinen at linux.intel.com> wrote:

> On Wed, 2017-09-20 at 15:01 +0300, Jani Nikula wrote:
>> On Wed, 20 Sep 2017, Joonas Lahtinen <joonas.lahtinen at linux.intel.com>  
>> wrote:
>> > On Tue, 2017-09-19 at 19:38 +0000, Michal Wajdeczko wrote:
>> > > We should discourage developers from modifying modparams.
>> > > Introduce special macro for easier tracking of changes done
>> > > in modparams and enforce its use by defining existing modparams
>> > > members as const. Note that defining whole modparams struct
>> > > as const makes checkpatch unhappy.
>> > >
>> > > v2: rebased
>> > >
>> > > Credits-to: Coccinelle
>> > >
>> > > @@
>> > > identifier n;
>> > > expression e;
>> > > @@
>> > > (
>> > > -	i915_modparams.n = e;
>> > > +	i915_modparams_set(n, e);
>> >
>> > Not cool with such a brief name, it really needs to be something more
>> > standing out to make the developer think they've failed design if
>> > they're calling the function.
>> >
>> > 'i915_modparams_force_write' is my current favourite.
>> >
>> > And we need huge kerneldoc comment for the function about the concerns
>> > expressed by Jani, me and Ville. There must be no potential readers  
>> for
>> > the variables while they're being changed, compiler optimizations need
>> > to be watched for etc.
>> >
>> > Because really, if we change a module parameter variable while  
>> somebody
>> > is for example running a loop based on it, we're in deep problems.
>> >
>> > Might be worthwhile having a i915_modparams_lock to be taken when
>> > sanitization of options begins, and asserting that lock is held when
>> > _force_write() is being called. rw_semaphore sounds like the right
>> > choice here. Many can read but only one can write.
>> >
>> > Any opinions on that?
>>
>> It can't protect against users changing the parameters via sysfs, and I
>> think fixing that at the moment would have an air of overengineering.
>>
>> I'm thinking review and merge patch 1 to fix the i915 name collision,
>> and forget about the rest for now.
>
> Agreed on merging, disagreeing on forgetting next steps.
>
>> Too much controversy, no real rush or
>> pressure to do anything right now beyond patch 1. Don't just do
>> something, stand there.
>
> The controversy seemed to be around compiler optimizations, and that
> doesn't seem to be a worry. The other thing is how to name the
> function, and that's not too bad discussion. It naturally shouldn't
> block merging the first patch.

If there is an agreement on merging first patch, can someone give it
r-b and merge ? Note that this patch is prone to rebase conflicts.

Michal

>
> Reviewing the places where the modparams get written/read may only lead
> to improvements as I see it. Any troublesome variables should get moved
> to device state instead of module state. For example while sanitizing
> enable_ppgtt and other user requested kernel parameters, we should copy
> the state to relevant dynamic structures where it'll have an effect, if
> we actually intend to support changing the parameters on the fly.
>
> Regards, Joonas