Re: [PATCH 4/4] thunderbolt: Export IOMMU based DMA protection support to userspace
On Tue, Nov 13, 2018 at 1:40 PM Mika Westerberg wrote: > > On Tue, Nov 13, 2018 at 01:13:31PM +0200, Yehezkel Bernat wrote: > > On Tue, Nov 13, 2018 at 12:56 PM Mika Westerberg > > wrote: > > > > > > > Just one point: > > > > Have you considered the option to add this property per (TBT?) device? > > > > > > No. ;-) > > > > > > You mean that one device uses security levels and another IOMMU? I don't > > > think it is possible without having some sort of table in the IOMMU > > > driver telling which devices it needs identity map and which not. Also > > > not sure what would be the benefit? > > > > For performance, of course. If some devices are considered safe (maybe a > > list > > communicated by platform firmware), the kernel may decide to configure them > > to > > passthrough the IOMMU (I think I remember there is such an option, but > > maybe I'm > > wrong.) > > At least I'm not aware of such an option. Windows for example enables > IOMMU for everything and I think macOS does the same. In Linux (with > these patches) we put all internal devices already passthrough mode so > things like internal graphics should not be affected. eGPUs are > different thing, though. So your point here is "currently we do the IOMMU decisions system-wide; we can always add a per-device attribute if needed"? Fair enough. So for this patch, Reviewed-by: Yehezkel Bernat
Re: [PATCH 5/5] thunderbolt: Add support for runtime PM
On Sun, Jul 8, 2018 at 10:31 AM Mika Westerberg wrote: > > On Sat, Jul 07, 2018 at 11:14:01PM +0200, Lukas Wunner wrote: > > > > Because I'm fairly certain that > > I do not get a PME for the Light Ridge in my MacBook Pro, but I'll test > > this once more and modify negotiate_os_control() to grant PME control > > to the OS. > > I think in case of Apple hardware, they handle the in some different > means than PME (possibly part of chipset driver or ACPI method/event). In addition to what already mentioned, many things have changed around power management during Alpine Ridge development, some of them came later as FW updates (and BIOS changes). Comparing Alpine ridge to Light Ridge here is comparing oranges to, well, Apples.
Re: [PATCH] thunderbolt: Notify userspace when boot_acl is changed
On Mon, Jun 18, 2018 at 5:44 PM Mika Westerberg
wrote:
>
> The commit 9aaa3b8b4c56 ("thunderbolt: Add support for preboot ACL")
> introduced boot_acl attribute but missed the fact that now userspace
> needs to poll the attribute constantly to find out whether it has
> changed or not. Fix this by sending notification to the userspace
> whenever the boot_acl attribute is changed.
>
> Fixes: 9aaa3b8b4c56 ("thunderbolt: Add support for preboot ACL")
> Reported-by: Christian Kellner
> Signed-off-by: Mika Westerberg
> ---
> drivers/thunderbolt/domain.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/thunderbolt/domain.c b/drivers/thunderbolt/domain.c
> index 6281266b8ec0..a923ebdeb73c 100644
> --- a/drivers/thunderbolt/domain.c
> +++ b/drivers/thunderbolt/domain.c
> @@ -213,6 +213,10 @@ static ssize_t boot_acl_store(struct device *dev, struct
> device_attribute *attr,
> goto err_free_acl;
> }
> ret = tb->cm_ops->set_boot_acl(tb, acl, tb->nboot_acl);
> + if (!ret) {
> + /* Notify userspace about the change */
> + kobject_uevent(&tb->dev.kobj, KOBJ_CHANGE);
> + }
> mutex_unlock(&tb->lock);
>
> err_free_acl:
> --
> 2.17.1
>
Acked-by: Yehezkel Bernat
Re: [PATCH] thunderbolt: Fix a missing-check bug
On Sat, Oct 20, 2018 at 12:25 AM Wenwen Wang wrote: > > On Thu, Oct 18, 2018 at 4:13 AM Mika Westerberg > wrote: > > > > Hi Wenwen, > > > > On Wed, Oct 17, 2018 at 09:00:29AM -0500, Wenwen Wang wrote: > > > In tb_cfg_copy(), the header of the received control package, which is in > > > the buffer 'pkg->buffer', is firstly parsed through parse_header() to make > > > sure the header is in the expected format. In parse_header(), the header > > > is > > > actually checked by invoking check_header(), which checks the 'unknown' > > > field of the header and the response route acquired through > > > 'tb_cfg_get_route(header)'. If there is no error in this checking process, > > > the package, including the header, is then copied to 'req->response' in > > > tb_cfg_copy() through memcpy() and the parsing result is saved to > > > 'req->result'. > > > > > > The problem here is that the whole parsing and checking process is > > > conducted directly on the buffer 'pkg->buffer', which is actually a DMA > > > region and allocated through dma_pool_alloc() in tb_ctl_pkg_alloc(). Given > > > that the DMA region can also be accessed directly by a device at any time, > > > it is possible that a malicious device can race to modify the package data > > > after the parsing and checking process but before memcpy() is invoked in > > > tb_cfg_copy(). Through this way, the attacker can bypass the parsing and > > > checking process and inject malicious data. This can potentially cause > > > undefined behavior of the kernel and introduce unexpected security risk. > > > > Here the device doing DMA is the Thunderbolt host controller which is > > soldered on the motherboard (not anything connected via the TBT ports). > > In addition the buffers we are dealing here are already marked ready by > > the host controller hardware so it is not expected to touch them anymore > > (if it did, then it would be a quite nasty bug). > > > > What kind of use-case you had in mind that could possibly inject > > malicious data to these buffers? > > Hi Mika, > > Thanks for your response. The current version of the code assumes that > the Thunderbolt controller behaves as expected, e.g., the host > controller should not touch the data after it is marked ready. > However, it is not impossible that the controller is exploited by an > attacker through a security vulnerability, even though it is soldered > on the motherboard. In that case, the controller may behave in an > unexpected way and this bug will offer more opportunities for the > attacker. [Re-sending as plain text. Mobile clients aren't good at that, apparently... Sorry.] If a device that can do DMA (i.e. accessing the whole physical memory directly) is compromised, incorrect read length is the least of the troubles the user should worry about. On the other hand, this is a performance-sensitive path of the code and copying each DMA buffer will hurt the performance very much for sure. I agree with Mika that it doesn't make much sense to degrade the performance here just for covering this theoretical attack that can do much worse things anyway.
Re: [PATCH] thunderbolt: Handle NULL boot ACL entries properly
On Fri, Apr 27, 2018 at 6:58 PM Mika Westerberg <
mika.westerb...@linux.intel.com> wrote:
> If the boot ACL entry is already NULL we should not fill in the upper
> two DWs with 0xf. Otherwise they are not shown as empty entries
> when the sysfs attribute is read.
> Fixes: 9aaa3b8b4c56 ("thunderbolt: Add support for preboot ACL")
> Signed-off-by: Mika Westerberg
> ---
>drivers/thunderbolt/icm.c | 2 +-
>1 file changed, 1 insertion(+), 1 deletion(-)
> diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c
> index 2d2ceda9aa26..aec6a82989d8 100644
> --- a/drivers/thunderbolt/icm.c
> +++ b/drivers/thunderbolt/icm.c
> @@ -1255,7 +1255,7 @@ static int icm_ar_get_boot_acl(struct tb *tb,
uuid_t *uuids, size_t nuuids)
> /* Map empty entries to null UUID */
> uuid[0] = 0;
> uuid[1] = 0;
> - } else {
> + } else if (uuid[0] != 0 && uuid[1] != 0) {
Maybe || instead of &&?
I'd think it's enough for one of them to be non-zero to be a valid UUID.
> /* Upper two DWs are always one's */
> uuid[2] = 0x;
> uuid[3] = 0x;
> --
> 2.17.0
Re: [PATCH v2] thunderbolt: Handle NULL boot ACL entries properly
On Mon, Apr 30, 2018 at 2:12 PM Mika Westerberg <
mika.westerb...@linux.intel.com> wrote:
> If the boot ACL entry is already NULL we should not fill in the upper
> two DWs with 0xf. Otherwise they are not shown as empty entries
> when the sysfs attribute is read.
> Fixes: 9aaa3b8b4c56 ("thunderbolt: Add support for preboot ACL")
> Signed-off-by: Mika Westerberg
> ---
> Changes from v1 (https://lkml.org/lkml/2018/4/27/845):
> * Use || instead of && to make sure UUIDs with only one zero DW
> are still treated as valid.
> drivers/thunderbolt/icm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c
> index 2d2ceda9aa26..500911f16498 100644
> --- a/drivers/thunderbolt/icm.c
> +++ b/drivers/thunderbolt/icm.c
> @@ -1255,7 +1255,7 @@ static int icm_ar_get_boot_acl(struct tb *tb,
uuid_t *uuids, size_t nuuids)
> /* Map empty entries to null UUID */
> uuid[0] = 0;
> uuid[1] = 0;
> - } else {
> + } else if (uuid[0] != 0 || uuid[1] != 0) {
> /* Upper two DWs are always one's */
> uuid[2] = 0xffff;
> uuid[3] = 0x;
> --
> 2.17.0
Acked-by: Yehezkel Bernat
Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4
On Wed, Oct 2, 2019 at 6:09 PM wrote: > > > -Original Message- > > From: Mika Westerberg > > Sent: Wednesday, October 2, 2019 3:39 AM > > To: Limonciello, Mario > > Cc: linux-...@vger.kernel.org; andreas.noe...@gmail.com; > > michael.ja...@intel.com; yehezkel...@gmail.com; rajmohan.m...@intel.com; > > nicholas.johnson-opensou...@outlook.com.au; lu...@wunner.de; > > gre...@linuxfoundation.org; st...@rowland.harvard.edu; > > anthony.w...@canonical.com; linux-kernel@vger.kernel.org > > Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4 > > > > > > [EXTERNAL EMAIL] > > > > On Tue, Oct 01, 2019 at 06:14:23PM +, mario.limoncie...@dell.com wrote: > > > One more thought; would you consider exporting to sysfs sw- > > >config.vendor_id? > > > Maybe an attribute that is switch_vendor? > > > > > > Userland fwupd also does validation on the NVM and will need to follow > > > this. > > > The same check will go into fwupd to match the vendor and lack of > > nvm_non_active0 > > > to mark the device as not updatable. When the checks in the kernel get > > relaxed, > > > some NVM parsing will have to make it over to fwupd too to relax the > > > check at > > that point. > > > > The original idea was that the kernel does the basic validation and > > userspace then does more complex checks. Currently you can compare the > > two NVM images (active one and the new) and find that information there. > > I think fwupd is doing just that already. Is that not enough? > > I guess for fwupd we can do this without the extra attribute: > > 1) If no NVM files or nvm_version: means unsupported vendor -show that > messaging somewhere. > This means kernel doesn't know the vendor. > > 2) If NVM file and nvm_version: means kernel knows it > *fwupd checks the vendor offset for intel. > -> intel: good, proceed > ->something else: fwupd needs to learn the format for the new vendor, show > messaging > > There is the unlikely case that kernel knows new vendor format and vendor NVM > happened to have > same value as Intel vendor ID in same location of Intel NVM but another > meaning. > Hopefully that doesn't happen :) It's not even "same location - another meaning", the vendor ID comes from the DROM section, so it takes a few internal jumps inside the NVM to find the location. One of the "pointers" or section headers will be broken for sure. And after this, we need to find the NVM in LVFS and it has to pass validation in a few other locations. The chances are so low that I'd think it isn't worth worrying about it.
Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4
On Fri, Oct 4, 2019 at 10:54 AM Mika Westerberg wrote: > > On Thu, Oct 03, 2019 at 02:41:11PM +, mario.limoncie...@dell.com wrote: > > > -Original Message- > > > From: Mika Westerberg > > > Sent: Thursday, October 3, 2019 3:00 AM > > > To: Limonciello, Mario > > > Cc: yehezkel...@gmail.com; linux-...@vger.kernel.org; > > > andreas.noe...@gmail.com; michael.ja...@intel.com; > > > rajmohan.m...@intel.com; nicholas.johnson-opensou...@outlook.com.au; > > > lu...@wunner.de; gre...@linuxfoundation.org; st...@rowland.harvard.edu; > > > anthony.w...@canonical.com; linux-kernel@vger.kernel.org > > > Subject: Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4 > > > > > > > > > [EXTERNAL EMAIL] > > > > > > On Wed, Oct 02, 2019 at 04:00:55PM +, mario.limoncie...@dell.com > > > wrote: > > > > > It's not even "same location - another meaning", the vendor ID comes > > > > > from > > > the > > > > > DROM section, so it takes a few internal jumps inside the NVM to find > > > > > the > > > > > location. One of the "pointers" or section headers will be broken for > > > > > sure. > > > > > > > > > > And after this, we need to find the NVM in LVFS and it has to pass > > > > > validation > > > in > > > > > a few other locations. The chances are so low that I'd think it isn't > > > > > worth > > > > > worrying about it. > > > > > > > > And now I remember why the back of my mind was having this thought of > > > wanting > > > > sysfs attribute in the first place. The multiple jumps means that a > > > > lot more of > > > the > > > > NVM has to be dumped to get that data, which slows down fwupd startup > > > significantly. > > > > > > IIRC currently fwupd does two reads of total 128 bytes from the active > > > NVM. Is that really slowing down fwupd startup significantly? > > > > Yeah, I timed it with fwupd. Here's the averages: > > > > Without doing the reads to jump to this it's 0:00.06 seconds to probe a > > tree of > > Host controller and dock plugged in. > > > > With doing the reads and just host controller: > > 0:04.40 seconds > > > > With doing the reads and host controller and dock plugged in: > > 0:10.73 seconds > > OK, it clearly takes time to read them. I wonder if this includes > powering up the controller? > > Also if you can get the hw_vendor_id and hw_product_id from the kernel > does that mean you don't need to do the two reads or you still need > those? Are those the chip vendor or the OEM, in case they are different? Thinking about it again, I'd guess it shouldn't matter much, if the chip is from Intel, the FW supports NVM upgrade, isn't it?
Re: [RFC PATCH 17/22] thunderbolt: Add initial support for USB4
On Fri, Oct 4, 2019 at 11:19 AM Mika Westerberg wrote: > > On Fri, Oct 04, 2019 at 11:07:34AM +0300, Yehezkel Bernat wrote: > > > Also if you can get the hw_vendor_id and hw_product_id from the kernel > > > does that mean you don't need to do the two reads or you still need > > > those? > > > > Are those the chip vendor or the OEM, in case they are different? > > Those are the actual USB4 hardware maker values, directly from > ROUTER_CS_0 (p. 287 in the USB4 spec). This almost certainly differ from > the OEM values from DROM we currently expose. Makes sense to me. Userspace can learn the relevant IDs that their NVM format is known. > > > Thinking about it again, I'd guess it shouldn't matter much, if the chip is > > from > > Intel, the FW supports NVM upgrade, isn't it? > > So the bottom line is that if the kernel thinks the router supports NVM > upgrade it exposes the nvm_active/nvm_non_active files etc. I think > fwupd uses this information to display user whether the device can be > upgraded or not (for example ICL cannot as the NVM is part of BIOS). Yes, fwupd already takes this into account, but the question here is how to handle cases that NVM is available but the format isn't known to userspace (yet). > > Exposing hw_vendor_id and hw_product_id may speed up fwupd because it > does not need to go over the active NVM to figure out whether the new > image is for the correct controller. It's not about finding the relevant image for upgrade (which must be searched for by looking in the DROM vendor/product values), but about the question if the NVM format is known to userspace and skip the parsing work if it's anyway going to fail. So yes, I think exposing vendor ID (and maybe also product ID) can improve the situation. Thanks!
Re: [PATCH v1] thunderbolt: Switch to use device_property_count_uXX()
On Tue, Jul 23, 2019 at 10:21 PM Andy Shevchenko wrote: > > Use use device_property_count_uXX() directly, that makes code neater. > > Signed-off-by: Andy Shevchenko Acked-by: Yehezkel Bernat
Re: [PATCH 2/8] thunderbolt: Move NVM upgrade support flag to struct icm
On Fri, Jul 5, 2019 at 12:58 PM Mika Westerberg
wrote:
>
> @@ -1913,12 +1915,7 @@ static int icm_start(struct tb *tb)
> if (IS_ERR(tb->root_switch))
> return PTR_ERR(tb->root_switch);
>
> - /*
> -* NVM upgrade has not been tested on Apple systems and they
> -* don't provide images publicly either. To be on the safe side
> -* prevent root switch NVM upgrade on Macs for now.
> -*/
> - tb->root_switch->no_nvm_upgrade = x86_apple_machine;
> + tb->root_switch->no_nvm_upgrade = !icm->can_upgrade_nvm;
> tb->root_switch->rpm = icm->rpm;
>
> ret = tb_switch_add(tb->root_switch);
> @@ -2021,6 +2018,7 @@ struct tb *icm_probe(struct tb_nhi *nhi)
> switch (nhi->pdev->device) {
> case PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_NHI:
> case PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_NHI:
> + icm->can_upgrade_nvm = true;
> icm->is_supported = icm_fr_is_supported;
> icm->get_route = icm_fr_get_route;
> icm->save_devices = icm_fr_save_devices;
> @@ -2038,6 +2036,13 @@ struct tb *icm_probe(struct tb_nhi *nhi)
> case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_NHI:
> case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_2C_NHI:
> icm->max_boot_acl = ICM_AR_PREBOOT_ACL_ENTRIES;
> + /*
> +* NVM upgrade has not been tested on Apple systems and
> +* they don't provide images publicly either. To be on
> +* the safe side prevent root switch NVM upgrade on Macs
> +* for now.
> +*/
> + icm->can_upgrade_nvm = !x86_apple_machine;
> icm->is_supported = icm_ar_is_supported;
> icm->cio_reset = icm_ar_cio_reset;
> icm->get_mode = icm_ar_get_mode;
> @@ -2054,6 +2059,7 @@ struct tb *icm_probe(struct tb_nhi *nhi)
> case PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_NHI:
> case PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_NHI:
> icm->max_boot_acl = ICM_AR_PREBOOT_ACL_ENTRIES;
> + icm->can_upgrade_nvm = true;
Shouldn't this be also !x86_apple_machine just like AR?
(For FR, we don't use ICM on Apple machines, as much as I remember, so it's fine
to enable it there unconditionally for ICM code path.)
> icm->is_supported = icm_ar_is_supported;
> icm->cio_reset = icm_tr_cio_reset;
> icm->get_mode = icm_ar_get_mode;
> --
> 2.20.1
>
Re: [PATCH 3/8] thunderbolt: Use 32-bit writes when writing ring producer/consumer
On Fri, Jul 5, 2019 at 12:58 PM Mika Westerberg
wrote:
>
> The register access should be using 32-bit reads/writes according to the
> datasheet. With the previous generation hardware 16-bit writes have been
> working but starting with ICL this is not the case anymore so fix
> producer/consumer register update to use correct width register address.
>
> Signed-off-by: Mika Westerberg
> ---
> drivers/thunderbolt/nhi.c | 26 ++
> 1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> index 27fbe62c7ddd..09242653da67 100644
> --- a/drivers/thunderbolt/nhi.c
> +++ b/drivers/thunderbolt/nhi.c
> @@ -143,9 +143,24 @@ static void __iomem *ring_options_base(struct tb_ring
> *ring)
> return io;
> }
>
> -static void ring_iowrite16desc(struct tb_ring *ring, u32 value, u32 offset)
> +static void ring_iowrite_prod(struct tb_ring *ring, u16 prod)
> {
> - iowrite16(value, ring_desc_base(ring) + offset);
> + u32 val;
> +
> + val = ioread32(ring_desc_base(ring) + 8);
> + val &= 0x;
> + val |= prod << 16;
> + iowrite32(val, ring_desc_base(ring) + 8);
> +}
> +
> +static void ring_iowrite_cons(struct tb_ring *ring, u16 cons)
> +{
> + u32 val;
> +
> + val = ioread32(ring_desc_base(ring) + 8);
> + val &= 0x;
> + val |= cons;
> + iowrite32(val, ring_desc_base(ring) + 8);
> }
>
> static void ring_iowrite32desc(struct tb_ring *ring, u32 value, u32 offset)
> @@ -197,7 +212,10 @@ static void ring_write_descriptors(struct tb_ring *ring)
> descriptor->sof = frame->sof;
> }
> ring->head = (ring->head + 1) % ring->size;
> - ring_iowrite16desc(ring, ring->head, ring->is_tx ? 10 : 8);
> + if (ring->is_tx)
> + ring_iowrite_prod(ring, ring->head);
> + else
> + ring_iowrite_cons(ring, ring->head);
Really a matter of taste, but maybe you want to consider having a single
function, with a 3rd parameter, bool is_tx.
The calls here will be unified to:
ring_iowrite(ring, ring->head, ring->is_tx);
(No condition is needed here).
The implementation uses the new parameter to decide which part of the register
to mask, reducing the code duplication (in my eyes):
val = ioread32(ring_desc_base(ring) + 8);
if (is_tx) {
val &= 0x;
val |= value << 16;
} else {
val &= 0x;
val |= value;
}
iowrite32(val, ring_desc_base(ring) + 8);
I'm not sure if it improves the readability or makes it worse. Your call.
Re: [PATCH 7/8] thunderbolt: Add support for Intel Ice Lake
On Fri, Jul 5, 2019 at 12:58 PM Mika Westerberg
wrote:
>
> +static void icm_icl_rtd3_veto(struct tb *tb, const struct icm_pkg_header
> *hdr)
> +{
> + const struct icm_icl_event_rtd3_veto *pkg =
> + (const struct icm_icl_event_rtd3_veto *)hdr;
> + struct icm *icm = tb_priv(tb);
> +
> + tb_dbg(tb, "ICM rtd3 veto=0x%08x\n", pkg->veto_reason);
> +
> + if (pkg->veto_reason) {
> + if (!icm->veto) {
> + icm->veto = true;
> + /* Keep the domain powered while veto is in effect */
> + pm_runtime_get(&tb->dev);
> + }
> + } else {
> + if (icm->veto) {
> + icm->veto = false;
> + /* Allow the domain suspend now */
> + pm_runtime_mark_last_busy(&tb->dev);
> + pm_runtime_put_autosuspend(&tb->dev);
Handling the removal of the veto is duplicated below. Worth introducing as a
helper function?
> + }
> + }
> +}
> +
...
> @@ -1853,6 +1943,18 @@ static void icm_complete(struct tb *tb)
> if (tb->nhi->going_away)
> return;
>
> + /*
> +* If RTD3 was vetoed before we entered system suspend allow it
> +* again now before driver ready is sent. Firmware sends a new RTD3
> +* veto if it is still the case after we have sent it driver ready
> +* command.
> +*/
> + if (icm->veto) {
> + icm->veto = false;
> + pm_runtime_mark_last_busy(&tb->dev);
> + pm_runtime_put_autosuspend(&tb->dev);
> + }
> +
Here is the duplication.
> +static int nhi_suspend_power_down(struct tb *tb)
> +{
> + int ret;
> +
> + /*
> +* If there is no device connected we need to perform an additional
> +* handshake through LC mailbox and force power down before
> +* entering D3.
> +*/
> + ret = device_for_each_child(&tb->root_switch->dev, NULL,
> + nhi_device_connected);
> + if (!ret) {
> + lc_mailbox_cmd(tb->nhi, LC_PREPARE_FOR_RESET);
> + ret = lc_mailbox_cmd_complete(tb->nhi,
> + LC_MAILBOX_TIMEOUT);
> + if (ret)
> + return ret;
> +
> + return nhi_power_down(tb->nhi);
Just to be sure: unforce power is done only if no device is connected?
My understanding of the comment above was that unforce power should be done
anyway (so it should be outside of this if block), and the difference between
the cases is only about the additional LC mailbox message. I guess I misread it.
> + }
> +
> + return 0;
> +}
> +
Re: [PATCH 0/8] thunderbolt: Intel Ice Lake support
On Fri, Jul 5, 2019 at 1:33 PM Mika Westerberg wrote: > > On Fri, Jul 05, 2019 at 12:57:52PM +0300, Mika Westerberg wrote: > > Hi all, > > > > With the exception of the first patch which is fix, this series enables > > Thunderbolt on Intel Ice Lake. Biggest difference from the previous > > controllers is that the Thunderbolt controller is now integrated as part of > > the SoC. The firmware messages pretty much follow Titan Ridge but there are > > some differences as well (such as the new RTD3 veto notification). Also Ice > > Lake does not implement security levels so DMA protection is handled by > > IOMMU. > > > > This is v5.4 material but I'm sending it out now because I will be on > > vacation next 4 weeks mostly without internet access. When I get back I'll > > gather all the comments and update the series accordingly. > > > > Thanks! > > > > Mika Westerberg (8): > > thunderbolt: Correct path indices for PCIe tunnel > > thunderbolt: Move NVM upgrade support flag to struct icm > > thunderbolt: Use 32-bit writes when writing ring producer/consumer > > thunderbolt: Do not fail adding switch if some port is not implemented > > thunderbolt: Hide switch attributes that are not set > > thunderbolt: Expose active parts of NVM even if upgrade is not supported > > thunderbolt: Add support for Intel Ice Lake > > ACPI / property: Add two new Thunderbolt property GUIDs to the list > > Forgot to Cc Raanan and Raj, now added. Sorry about that. The patch > series can also be viewed here: > > > https://lore.kernel.org/lkml/20190705095800.43534-1-mika.westerb...@linux.intel.com/T/#m9cb5a393dfc79f1c2212d0787b6bad5b689db6bd Besides a few comments, LGTM. For Thunderbolt patches, Reviewed-by: Yehezkel Bernat
Re: [PATCH 7/8] thunderbolt: Add support for Intel Ice Lake
On Fri, Jul 5, 2019 at 5:51 PM Mika Westerberg
wrote:
>
> > > +static int nhi_suspend_power_down(struct tb *tb)
> > > +{
> > > + int ret;
> > > +
> > > + /*
> > > +* If there is no device connected we need to perform an
> > > additional
> > > +* handshake through LC mailbox and force power down before
> > > +* entering D3.
> > > +*/
> > > + ret = device_for_each_child(&tb->root_switch->dev, NULL,
> > > + nhi_device_connected);
> > > + if (!ret) {
> > > + lc_mailbox_cmd(tb->nhi, LC_PREPARE_FOR_RESET);
> > > + ret = lc_mailbox_cmd_complete(tb->nhi,
> > > + LC_MAILBOX_TIMEOUT);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + return nhi_power_down(tb->nhi);
> >
> > Just to be sure: unforce power is done only if no device is connected?
> > My understanding of the comment above was that unforce power should be done
> > anyway (so it should be outside of this if block), and the difference
> > between
> > the cases is only about the additional LC mailbox message. I guess I
> > misread it.
>
> nhi_power_down() should be only called if no device was connected so it
> should be in correct place. I can try to clarify the comment a bit,
> though.
Maybe adding the word "both" ("to perform both an additional") will make it
clearer. Maybe removing the "additional" (which to my ears sounds like "an
additional operation besides the normal one, to unforce power") is enough.
Again, your call. I'm not sure it's strictly needed, maybe it's just me.
Thanks!
Re: [PATCH 4/5] thunderbolt: Correlate PCI devices with Thunderbolt ports
On Mon, Sep 10, 2018 at 12:45 PM Mika Westerberg wrote: > > Hi Lukas, > > On Sun, Sep 09, 2018 at 11:42:01PM +0200, Lukas Wunner wrote: > > Ideas what we can do with correlation: > > > > * Represent the relationship between PCI devices and Thunderbolt ports > > with symlinks in sysfs. > > I wonder is that really useful? I don't think we should be adding sysfs > entries without any real reason why it would be needed and who would be > using them. I think Lukas mentioned where it can be useful, even if it isn't used right now. We also know this can be useful for some QoS configurations (even if we didn't found it useful enough for now). Lukas, What I'd like to ask is: if your point is mainly for OS-controlled tunneling management, I'd hope this future Connection Manager will have better ways to determine the assigned PCI bus number of the PCI devices it tunnels than the walk over the PCI tree the current patch does. Isn't it?
Re: [PATCH v2 0/4] thunderbolt: Miscellaneous cleanups
On Mon, Sep 17, 2018 at 5:43 PM Mika Westerberg wrote: > > Hi all, > > Here are a couple of changes which have been in my TODO list some time > already but have not had time to make a proper patch out of them until now. > > The only change to the previous version is that I realized it could be > useful for users to see the actual connected devices (including their name > which is read from DROM) printed to the system log. Hence the patch 2/4. > > Mika Westerberg (4): > thunderbolt: Make the driver less verbose > thunderbolt: Print connected devices > thunderbolt: Convert rest of the driver files to use SPDX identifier > thunderbolt: Add Intel as copyright holder > > drivers/thunderbolt/cap.c | 3 +- > drivers/thunderbolt/ctl.c | 9 +++-- > drivers/thunderbolt/ctl.h | 3 +- > drivers/thunderbolt/dma_port.c | 5 +-- > drivers/thunderbolt/dma_port.h | 5 +-- > drivers/thunderbolt/domain.c | 7 +--- > drivers/thunderbolt/eeprom.c | 5 ++- > drivers/thunderbolt/icm.c | 5 +-- > drivers/thunderbolt/nhi.c | 33 > drivers/thunderbolt/nhi.h | 3 +- > drivers/thunderbolt/nhi_regs.h | 1 + > drivers/thunderbolt/path.c | 26 ++--- > drivers/thunderbolt/property.c | 5 +-- > drivers/thunderbolt/switch.c | 71 +++--- > drivers/thunderbolt/tb.c | 10 ++--- > drivers/thunderbolt/tb.h | 9 +++-- > drivers/thunderbolt/tb_msgs.h | 5 +-- > drivers/thunderbolt/tb_regs.h | 3 +- > drivers/thunderbolt/xdomain.c | 5 +-- > include/linux/thunderbolt.h| 5 +-- > 20 files changed, 107 insertions(+), 111 deletions(-) > > -- > 2.18.0 > Acked-by: Yehezkel Bernat
Re: [PATCH 3/3] thunderbolt: Add Intel as copyright holder
> * Copyright (c) 2014 Andreas Noever > + * Copyright (C) 2018, Intel Corporation Nitpicking: (c) or (C)? I can't find anything in the documentation and both are found in various files.
Re: [PATCH v3] Add driver to force WMI Thunderbolt controller power status
(Now in plain text. Sorry about that.) On Sat, Sep 9, 2017 at 10:09 AM, Mika Westerberg wrote: > On Fri, Sep 08, 2017 at 10:23:11AM -0500, Mario Limonciello wrote: >> Current implementations of Intel Thunderbolt controllers will go >> into a low power mode when not in use. >> >> Many machines containing these controllers also have a GPIO wired up >> that can force the controller awake. This is offered via a ACPI-WMI >> interface intended to be manipulated by a userspace utility. >> >> This mechanism is provided by Intel to OEMs to include in BIOS. >> It uses an industry wide GUID that is populated in a separate _WDG >> entry with no binary MOF. >> >> This interface allows software such as fwupd to wake up thunderbolt >> controllers to query the firmware version or flash new firmware. >> >> Signed-off-by: Mario Limonciello > > Reviewed-by: Mika Westerberg FWIW, Reviewed-by: Yehezkel Bernat
