linux-kernel - Re: [RFC PATCH 08/22] thunderbolt: Add downstream PCIe port mappings for Alpine and Titan Ridge

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: <20191001132702.GQ2714@lahna.fi.intel.com>
Date:   Tue, 1 Oct 2019 16:27:02 +0300
From:   Mika Westerberg <mika.westerberg@...ux.intel.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     linux-usb@...r.kernel.org,
        Andreas Noever <andreas.noever@...il.com>,
        Michael Jamet <michael.jamet@...el.com>,
        Yehezkel Bernat <YehezkelShB@...il.com>,
        Rajmohan Mani <rajmohan.mani@...el.com>,
        Nicholas Johnson <nicholas.johnson-opensource@...look.com.au>,
        Lukas Wunner <lukas@...ner.de>,
        Alan Stern <stern@...land.harvard.edu>,
        Mario.Limonciello@...l.com,
        Anthony Wong <anthony.wong@...onical.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 08/22] thunderbolt: Add downstream PCIe port mappings
 for Alpine and Titan Ridge

On Tue, Oct 01, 2019 at 02:40:54PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Oct 01, 2019 at 02:38:16PM +0300, Mika Westerberg wrote:
> > In order to keep PCIe hierarchies consistent across hotplugs, add
> > hard-coded PCIe downstream port to Thunderbolt port for Alpine Ridge and
> > Titan Ridge as well.
> 
> Oh, that makes me nervous, how could a hard-coded pcie port ever get set
> up incorrectly :)
> 
> How do we "know" that this is correct?  Is there any ACPI guarantees
> that this "always will be so"?  If not, we all know someone will mess
> this up...

For Alpine Ridge and Titan Ridge the PCIe ports are always the same.

Basically what this is about is that you have up to two Thunderbolt
ports (type-C ports). When you plug in Thunderbolt device and PCIe gets
tunneled, the PCIe hierarchy always is always extended from the same
PCIe downstream port.

If we don't do this then the PCIe device may be changing its address
each plug/unplug. Also for older generations (that code is already in
mainline) there are PCIe downstream ports that do not have enough
resources for additional devices.

> > Signed-off-by: Mika Westerberg <mika.westerberg@...ux.intel.com>
> > ---
> >  drivers/thunderbolt/tb.c |  4 +++-
> >  drivers/thunderbolt/tb.h | 25 +++++++++++++++++++++++++
> >  2 files changed, 28 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> > index dbbe9afb9fb7..704455a4f763 100644
> > --- a/drivers/thunderbolt/tb.c
> > +++ b/drivers/thunderbolt/tb.c
> > @@ -344,10 +344,12 @@ static struct tb_port *tb_find_pcie_down(struct tb_switch *sw,
> >  		 * Hard-coded Thunderbolt port to PCIe down port mapping
> >  		 * per controller.
> >  		 */
> > -		if (tb_switch_is_cr(sw))
> > +		if (tb_switch_is_cr(sw) || tb_switch_is_ar(sw))
> >  			index = !phy_port ? 6 : 7;
> >  		else if (tb_switch_is_fr(sw))
> >  			index = !phy_port ? 6 : 8;
> > +		else if (tb_switch_is_tr(sw))
> > +			index = !phy_port ? 8 : 9;
> >  		else
> >  			goto out;
> >  
> > diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> > index e641dcebd50a..dbab06551eaa 100644
> > --- a/drivers/thunderbolt/tb.h
> > +++ b/drivers/thunderbolt/tb.h
> > @@ -632,6 +632,31 @@ static inline bool tb_switch_is_fr(const struct tb_switch *sw)
> >  	}
> >  }
> >  
> > +static inline bool tb_switch_is_ar(const struct tb_switch *sw)
> 
> "ar"?  Can you spell it out?

You mean call this tb_switch_is_alpine_ridge()? Sure,  I will then do
the same for tb_switch_is_fr() and the existing ones.

> 
> > +{
> > +	switch (sw->config.device_id) {
> > +	case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_2C_BRIDGE:
> > +	case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_BRIDGE:
> > +	case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_BRIDGE:
> > +	case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_2C_BRIDGE:
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +
> > +static inline bool tb_switch_is_tr(const struct tb_switch *sw)
> 
> Same for "tr" please.

Sure.