Re: [PATCH v6 3/3] drm/i915: Make i915_modparams members const

[Jani Nikula <jani.nikula@intel.com>]

On Wed, 20 Sep 2017, Joonas Lahtinen <joonas.lahtinen@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@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.

It's not just that.

> 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.

Please figure out the alternatives to changing module parameters
first. The hurdles introduced in patch 3 aren't going to stop anyone
from adding new ones. I'm not going to tell anyone to stop doing that
until I have a better idea what to do *instead*.

Overall I'm more concerned about the insane total number of module
parameters that we have. We have them primarily because they are
*convenient*, and they are pretty much the only way to force some
behaviour on module probe. Anything you put in i915 debugfs or sysfs is
going to be much less convenient for debugging.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx