Re: [PATCH v9 10/39] drm/i915: Implement HDCP2.2 receiver authentication

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: "daniel.vetter@ffwll.ch" <daniel.vetter@ffwll.ch>,
	"Winkler, Tomas" <tomas.winkler@intel.com>,
	"seanpaul@chromium.org" <seanpaul@chromium.org>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH v9 10/39] drm/i915: Implement HDCP2.2 receiver authentication
Date: Fri, 21 Dec 2018 20:06:09 +0200	[thread overview]
Message-ID: <20181221180609.GM20097@intel.com> (raw)
In-Reply-To: <20181220145527.GN21184@phenom.ffwll.local>

On Thu, Dec 20, 2018 at 03:55:27PM +0100, Daniel Vetter wrote:
> On Thu, Dec 20, 2018 at 02:28:55PM +0000, Winkler, Tomas wrote:
> > 
> > 
> > > On Wed, 19 Dec 2018, "Winkler, Tomas" <tomas.winkler@intel.com> wrote:
> > > >>
> > > >> On Wed, 19 Dec 2018, "C, Ramalingam" <ramalingam.c@intel.com> wrote:
> > > >> > On 12/19/2018 8:05 PM, Daniel Vetter wrote:
> > > >> >> On Thu, Dec 13, 2018 at 09:31:12AM +0530, Ramalingam C wrote:
> > > >> >>>   struct intel_hdcp {
> > > >> >>> @@ -414,6 +430,24 @@ struct intel_hdcp {
> > > >> >>>   	 */
> > > >> >>>   	u8 content_type;
> > > >> >>>   	struct hdcp_port_data port_data;
> > > >> >>> +
> > > >> >>> +	u8 is_paired;
> > > >> >>> +	u8 is_repeater;
> > > >> >> Make these two bool, will simplify the code a bunch.
> > > >> >
> > > >> > Seems there is a movement for not to use the bool in structures.
> > > >>
> > > >> No. Please use bools in structs when it makes sense. Avoid bools in
> > > >> structs when you need to care about memory footprint or alignment or
> > > >> packing or the like. This is not one of those cases.
> > > >>
> > > >> > Thats why I have changed these from bool to u8 from v8 onwards.
> > > >> > Checkpatch also complains on this
> > > >>
> > > >> Sorry to say, checkpatch is not the authority although we do send out
> > > >> automated checkpatch results.
> > > >
> > > > I believe it was Linus' call to not use  bool in structs at all
> > > > https://lkml.org/lkml/2017/11/21/384
> > > 
> > > I don't care. That's a valid judgement in the context referenced, but the
> > > conclusion "no bools in structs at all" isn't. In this case, I think bools are the
> > > better option, and anything else makes the code worse.
> > 
> > The solution was to use bit fields, 
> >  unsinged int is_paired:1;
> > unsinged int is_repeter:1
> 
> This doesn't work with READ_ONCE/WRITE_ONCE, and it generates terrible
> assembly (at least gcc is well known for struggling with these, compared
> to open-coded bitops). So depending upon what you want to do, and where
> youre space/performance tradeoff lies, doing this unconditionally is just
> wrong.

Another annoying downside with non-bool bitfields:

unsigned int foo:1;
foo = 2;
-> foo == 0

So you'll need to remmber to convert everything to
0/1 before the assignment.

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