Re: [Intel-gfx] [PATCH v2] drm/i915/display: Use unions per platform in intel_dpll_hw_state

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Feb 24, 2022 at 01:49:36PM +0000, Souza, Jose wrote:
> On Thu, 2022-02-24 at 15:25 +0200, Ville Syrjälä wrote:
> > On Thu, Feb 24, 2022 at 01:17:35PM +0000, Souza, Jose wrote:
> > > On Thu, 2022-02-24 at 12:20 +0200, Ville Syrjälä wrote:
> > > > On Wed, Feb 23, 2022 at 12:55:51PM -0800, José Roberto de Souza wrote:
> > > > <snip>
> > > > > +	union {
> > > > > +		/* icl+ TC */
> > > > > +		struct {
> > > > > +			u32 mg_refclkin_ctl;
> > > > > +			u32 mg_clktop2_coreclkctl1;
> > > > > +			u32 mg_clktop2_hsclkctl;
> > > > > +			u32 mg_pll_div0;
> > > > > +			u32 mg_pll_div1;
> > > > > +			u32 mg_pll_lf;
> > > > > +			u32 mg_pll_frac_lock;
> > > > > +			u32 mg_pll_ssc;
> > > > > +			u32 mg_pll_bias;
> > > > > +			u32 mg_pll_tdc_coldst_bias;
> > > > > +			u32 mg_pll_bias_mask;
> > > > > +			u32 mg_pll_tdc_coldst_bias_mask;
> > > > > +		};
> > > > > +
> > > > > +		/* bxt */
> > > > > +		struct {
> > > > > +			/* bxt */
> > > > > +			u32 ebb0;
> > > > > +			u32 ebb4;
> > > > > +			u32 pll0;
> > > > > +			u32 pll1;
> > > > > +			u32 pll2;
> > > > > +			u32 pll3;
> > > > > +			u32 pll6;
> > > > > +			u32 pll8;
> > > > > +			u32 pll9;
> > > > > +			u32 pll10;
> > > > > +			u32 pcsdw12;
> > > > > +		};
> > > > 
> > > > Wasn't there some funny compiler bug around anonymous unions?
> > > > git log --grep='anon.*union' seems to agree. Please double check
> > > > that stuff to make sure this is actually safe.
> > > 
> > > I don't see any patch referring to compiler issues with 'git log --grep='anon.*union', what I see is other subsystems making use of it too.
> > > Can you share the commit hash that you are referring to?
> > 
> > $ git log --format=oneline --grep='anon.*union' -- drivers/gpu/drm/i915
> > 
> 
> I see issues with initialization of anonymous union but we don't initialize intel_dpll_hw_state.
> Also it was fixed on GCC 4.6 that is older than current GCC requirement to build kernel(GCC 5.1).
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=10676

OK. However, after pondering this a bit I think naming things wpild
probably be better here. You have a bunch of if ladders now where each
branch only operates on one of the structs inside the union. IMO the
anonymity is making it rather hard to see if the code is even correct.

-- 
Ville Syrjälä
Intel