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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190813164930.GA18281@lahna.fi.intel.com>
Date:   Tue, 13 Aug 2019 19:49:30 +0300
From:   Mika Westerberg <mika.westerberg@...ux.intel.com>
To:     Lukas Wunner <lukas@...ner.de>
Cc:     linux-kernel@...r.kernel.org,
        Andreas Noever <andreas.noever@...il.com>,
        Michael Jamet <michael.jamet@...el.com>,
        Yehezkel Bernat <YehezkelShB@...il.com>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Len Brown <lenb@...nel.org>, Mario.Limonciello@...l.com,
        Anthony Wong <anthony.wong@...onical.com>,
        Rajmohan Mani <rajmohan.mani@...el.com>,
        Raanan Avargil <raanan.avargil@...el.com>,
        David Laight <David.Laight@...lab.com>,
        linux-acpi@...r.kernel.org
Subject: Re: [PATCH v2 7/8] thunderbolt: Add support for Intel Ice Lake

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.