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

From: "Ville Syrjälä" <ville.syrjala@intel.com>
To: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v6 3/3] drm/i915: Make i915_modparams members const
Date: Wed, 20 Sep 2017 14:43:14 +0300	[thread overview]
Message-ID: <20170920114314.GY4914@intel.com> (raw)
In-Reply-To: <op.y6uy4d1zxaggs7@mwajdecz-mobl1.ger.corp.intel.com>

On Wed, Sep 20, 2017 at 11:54:03AM +0200, Michal Wajdeczko wrote:
> On Wed, 20 Sep 2017 10:34:43 +0200, Jani Nikula <jani.nikula@intel.com>  
> wrote:
> 
> > On Tue, 19 Sep 2017, Michal Wajdeczko <michal.wajdeczko@intel.com> 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.
> >
> > Checkpatch is the least of all reasons to not make the modparams struct
> > const.
> >
> > We can get away with having some fields (such as device info within
> > dev_priv) const, even if that's dubious.
> >
> > IIUC modifying const data is undefined behaviour at best, could cause
> > subtle bugs through compiler optimizing reads of the data away because
> > it assumes no modifications, and the data gets placed in rodata at
> > worst.
> 
> Note that we're forcing modparams to be stored in dedicated section:
> 
> #define __read_mostly __attribute__((__section__(".data..read_mostly")))
> 
> >
> > I kinda like the union trick in this patch, but IMO we need to double
> > check what the standard says about it. Making the fellow developers
> > check the standard is always a bad sign, even if it turns out to be fine
> > after all.
> 
> ISO/IEC 9899: 6.5.2.3
> 
> If the member used to access the contents of a union object is not the same
> as the member last used to store a value in the object, the appropriate  
> part
> of the object representation of the value is reinterpreted as an object
> representation in the new type as described in 6.2.6 (a process sometimes
> called "type punning").

There's a bunch of text in 6.7.3 which may or may not make that part
irrelevant. Very hard to say since there's no explicit example of
this kind in the spec.

-- 
Ville Syrjälä
Intel OTC
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx