Re: [PATCH v2 7/8] thunderbolt: Add support for Intel Ice Lake

[Mika Westerberg]

On Tue, Aug 13, 2019 at 06:10:38PM +0200, Lukas Wunner wrote:
> On Mon, Aug 12, 2019 at 03:38:46PM +0300, Mika Westerberg wrote:
> > +static void icm_veto_begin(struct tb *tb)
> > +{
> > +	struct icm *icm = tb_priv(tb);
> > +
> > +	if (!icm->veto) {
> > +		icm->veto = true;
> > +		/* Keep the domain powered while veto is in effect */
> > +		pm_runtime_get(&tb->dev);
> > +	}
> > +}
> 
> Hm, don't you need memory barriers when accessing icm->veto?

AFAICT it does not need them, it is protected under mutex where there
can be concurrency.

> If so, I'd suggest:
> 
> 	/* Keep the domain powered while veto is in effect */
> 	if (cmpxchg(&icm->veto, false, true))
> 		pm_runtime_get(&tb->dev);
> 
> You'll have to declare icm->veto unsigned int instead of bool
> because thunderbolt.ko is compiled if CONFIG_COMPILE_TEST is
> enabled and there are arches which do not support cmpxchg for
> a size of 1 byte.
> 
> The other bools in struct icm should likewise be unsigned int
> per Linus' dictum:
> https://lkml.org/lkml/2017/11/21/384

Yeah, it probably wastes some space but I like them because IMHO they
are more readable than bitfields. We have a bunch of other bools in the
driver structures so if we are going to convert struct icm we should do
the same for others to keep things consistent. Probably should be a
separate cleanup patch.

> > --- a/drivers/thunderbolt/nhi.c
> > +++ b/drivers/thunderbolt/nhi.c
> > +/* Ice Lake specific NHI operations */
> > +
> 
> Again, can't this be moved to a separate file for maintainability?

Sure.