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

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

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?

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx