thunderbolt: Add support for Intel Titan Ridge
cover thunderbolt: Add support for Intel Titan Ridge
Hi,
This series adds support for Intel Titan Ridge Thunderbolt controller. Titan Ridge is the next generation Thunderbolt 3 controller and successor of Alpine Ridge.
In addition to fixes and Titan Ridge support this series adds following:
- USB only security level (SL4).
- A new attribute for devices telling whether they were connected automatically during boot.
- Preboot ACL allows userspace to specify a list of devices (based on device unique_id) that the firmware automatically connects during boot.
Bjorn, I've Cc'd you the series as well because I would like to get your blessing for patch [2/18] as it uses functions from PCI subsystem.
Thanks!
Mika Westerberg (13):
thunderbolt: Resume control channel after hibernation image is created
thunderbolt: Serialize PCIe tunnel creation with PCI rescan
thunderbolt: Handle connecting device in place of host properly
thunderbolt: Do not overwrite error code when domain adding fails
thunderbolt: Wait a bit longer for root switch config space
thunderbolt: Wait a bit longer for ICM to authenticate the active NVM
thunderbolt: Handle rejected Thunderbolt devices
thunderbolt: Factor common ICM add and update operations out
thunderbolt: Add tb_switch_get()
thunderbolt: Add constant for approval timeout
thunderbolt: Move driver ready handling to struct icm
thunderbolt: Add support for preboot ACL
thunderbolt: Introduce USB only (SL4) security level
Radion Mirchevsky (4):
thunderbolt: Correct function name in kernel-doc comment
thunderbolt: Add tb_switch_find_by_route()
thunderbolt: Add tb_xdomain_find_by_route()
thunderbolt: Add support for Intel Titan Ridge
Yehezkel Bernat (1):
thunderbolt: Add 'boot' attribute for devices
Documentation/ABI/testing/sysfs-bus-thunderbolt | 33 ++ drivers/thunderbolt/dma_port.c | 28 +- drivers/thunderbolt/domain.c | 129 +++- drivers/thunderbolt/icm.c | 757 +++++++++++++++++++++--- drivers/thunderbolt/nhi.c | 5 +- drivers/thunderbolt/nhi.h | 5 + drivers/thunderbolt/switch.c | 61 +- drivers/thunderbolt/tb.h | 14 + drivers/thunderbolt/tb_msgs.h | 180 +++++- drivers/thunderbolt/xdomain.c | 40 +- include/linux/thunderbolt.h | 19 + 11 files changed, 1172 insertions(+), 99 deletions(-)
Signature · 1 line
2.15.1
PATCH 1/18 thunderbolt: Resume control channel after hibernation image is created
The driver misses implementation of PM hook that undoes what ->freeze_noirq() does after the hibernation image is created. This means the control channel is not resumed properly and the Thunderbolt bus becomes useless in later stages of hibernation (when the image is stored or if the operation fails).
Fix this by pointing ->thaw_noirq to driver nhi_resume_noirq(). This makes sure the control channel is resumed properly.
--- drivers/thunderbolt/nhi.c | 1 + 1 file changed, 1 insertion(+)
drivers/thunderbolt/nhi.c +1 -0
@@ -1064,6 +1064,7 @@ static const struct dev_pm_ops nhi_pm_ops = {* we just disable hotplug, the * pci-tunnels stay alive. */ + .thaw_noirq = nhi_resume_noirq, .restore_noirq = nhi_resume_noirq, .suspend = nhi_suspend, .freeze = nhi_suspend,
Signature · 1 line
2.15.1
PATCH 2/18 thunderbolt: Serialize PCIe tunnel creation with PCI rescan
In reply to Mika WesterbergWe need to make sure a new PCIe tunnel is not created in a middle of previous PCI rescan because otherwise the rescan code might find too much and fail to reconfigure devices properly. This is important when native PCIe hotplug is used. In BIOS assisted hotplug there should be no such issue.
--- drivers/thunderbolt/switch.c | 9 +++++++++ 1 file changed, 9 insertions(+)
drivers/thunderbolt/switch.c +9 -0
@@ -716,6 +716,13 @@ static int tb_switch_set_authorized(struct tb_switch *sw, unsigned int val)if (sw->authorized) goto unlock; +/* +* Make sure there is no PCIe rescan ongoing when a new PCIe +* tunnel is created. Otherwise the PCIe rescan code might find +* the new tunnel too early. +*/ +pci_lock_rescan_remove (); +switch (val) {/* Approve switch */ case 1 :
@@ -735,6 +742,8 @@ static int tb_switch_set_authorized(struct tb_switch *sw, unsigned int val)break ; } +pci_unlock_rescan_remove (); +if (!ret) { sw->authorized = val;/* Notify status change to the userspace */
Signature · 1 line
2.15.1
PATCH 3/18 thunderbolt: Handle connecting device in place of host properly
In reply to Mika WesterbergIf the system is suspended and user disconnects cable to another host and connects it to a Thunderbolt device instead we get a warning from driver core about adding duplicate sysfs attribute and adding the new device fails.
Handle this properly so that we first remove the existing XDomain connection before adding new devices.
--- drivers/thunderbolt/icm.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-)
drivers/thunderbolt/icm.c +17 -9
@@ -383,6 +383,15 @@ static void remove_switch(struct tb_switch *sw)tb_switch_remove (sw); } +static void remove_xdomain (struct tb_xdomain *xd) +{ +struct tb_switch *sw; + + sw =tb_to_switch (xd->dev.parent); +tb_port_at (xd->route, sw)->xdomain =NULL ; +tb_xdomain_remove (xd); +} +static void icm_fr_device_connected (struct tb *tb,const struct icm_pkg_header *hdr) {
@@ -391,6 +400,7 @@ icm_fr_device_connected(struct tb *tb, const struct icm_pkg_header *hdr)struct tb_switch *sw, *parent_sw;struct icm *icm =tb_priv (tb);bool authorized = false; +struct tb_xdomain *xd;u8 link, depth;u64 route;int ret;
@@ -467,6 +477,13 @@ icm_fr_device_connected(struct tb *tb, const struct icm_pkg_header *hdr)tb_switch_put (sw); } +/* Remove existing XDomain connection if found */ + xd =tb_xdomain_find_by_link_depth (tb, link, depth); +if (xd) { +remove_xdomain (xd); +tb_xdomain_put (xd); + } + parent_sw =tb_switch_find_by_link_depth (tb, link, depth -1 );if (!parent_sw) { tb_err(tb, "failed to find parent switch for %u.%u\n",
@@ -529,15 +546,6 @@ icm_fr_device_disconnected(struct tb *tb, const struct icm_pkg_header *hdr)tb_switch_put (sw); } -static void remove_xdomain (struct tb_xdomain *xd) -{ -struct tb_switch *sw; - - sw =tb_to_switch (xd->dev.parent); -tb_port_at (xd->route, sw)->xdomain =NULL ; -tb_xdomain_remove (xd); -} -static void icm_fr_xdomain_connected (struct tb *tb,const struct icm_pkg_header *hdr) {
Signature · 1 line
2.15.1
PATCH 4/18 thunderbolt: Do not overwrite error code when domain adding fails
In reply to Mika WesterbergIf the Thunderbolt domain adding fails for some reason we currently always return -EIO instead of the real error code. To make debugging easier return the actual error code instead.
--- drivers/thunderbolt/nhi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/thunderbolt/nhi.c +1 -1
@@ -1036,7 +1036,7 @@ static int nhi_probe(struct pci_dev *pdev, const struct pci_device_id *id)*/ tb_domain_put (tb);nhi_shutdown (nhi); -return -EIO ; +return res; }pci_set_drvdata (pdev, tb);
Signature · 1 line
2.15.1
PATCH 5/18 thunderbolt: Wait a bit longer for root switch config space
In reply to Mika WesterbergIn some case reading root switch config space takes longer than what we are currently waiting in the driver resulting timeout and failure. Increase number of retries to allow some more time for the root switch config space to become accesssible.
Also log an error if the timeout is exceeded so we know why the driver probe failed.
--- drivers/thunderbolt/icm.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/thunderbolt/icm.c +2 -1
@@ -828,7 +828,7 @@ __icm_driver_ready(struct tb *tb, enum tb_security_level *security_level)struct icm_pkg_driver_ready request = { .hdr.code =ICM_DRIVER_READY , }; -unsigned int retries =10 ; +unsigned int retries =50 ;int ret;memset (&reply,0 ,sizeof (reply));
@@ -856,6 +856,7 @@ __icm_driver_ready(struct tb *tb, enum tb_security_level *security_level)msleep (50 ); }while (--retries); +tb_err (tb,"failed to read root switch config space, giving up\n" );return -ETIMEDOUT ; }
Signature · 1 line
2.15.1
PATCH 6/18 thunderbolt: Wait a bit longer for ICM to authenticate the active NVM
In reply to Mika WesterbergSometimes during cold boot ICM has not yet authenticated the active NVM image leading to timeout and failing the driver probe. Allow ICM to take some more time and increase the timeout to 3 seconds before we give up.
While there fix icm_firmware_init() to return the real error code without overwriting it with -ENODEV.
--- drivers/thunderbolt/icm.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/thunderbolt/icm.c +5 -2
@@ -736,14 +736,14 @@ static bool icm_ar_is_supported(struct tb *tb)static int icm_ar_get_mode (struct tb *tb) {struct tb_nhi *nhi = tb->nhi; -int retries =5 ; +int retries =60 ;u32 val;do { val =ioread32 (nhi->iobase +REG_FW_STS );if (val ®_FW_STS_NVM_AUTH_DONE )break ; -msleep (30 ); +msleep (50 ); }while (--retries);if (!retries) {
@@ -1063,6 +1063,9 @@ static int icm_firmware_init(struct tb *tb)break ;default : +if (ret <0 ) +return ret; +tb_err (tb,"ICM firmware is in wrong mode: %u\n" , ret);return -ENODEV ; }
Signature · 1 line
2.15.1
RE: [PATCH 06/18] thunderbolt: Wait a bit longer for ICM to authenticate the active NVM
Quoted 57 lines
-----Original Message-----
From: Mika Westerberg [mailto:[email protected]]
Sent: Tuesday, February 13, 2018 11:00 AM
To: [email protected]
Cc: Andreas Noever <[email protected]>; Michael Jamet <[email protected]>; Yehezkel Bernat <[email protected]>; Bjorn Helgaas <[email protected]>; Limonciello, Mario <[email protected]>; Radion Mirchevsky <[email protected]>; Mika Westerberg <[email protected]>
Subject: [PATCH 06/18] thunderbolt: Wait a bit longer for ICM to authenticate the active NVMSometimes during cold boot ICM has not yet authenticated the active NVM image leading to timeout and failing the driver probe. Allow ICM to take some more time and increase the timeout to 3 seconds before we give up.
While there fix
icm_firmware_init()to return the real error code without overwriting it with-ENODEV.Signed-off-by Mika Westerberg <[email protected]>--- drivers/thunderbolt/icm.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)drivers/thunderbolt/icm.c +5
-3@@ -736,14 +736,14 @@ static bool icm_ar_is_supported(struct tb *tb)static int icm_ar_get_mode (struct tb *tb) {struct tb_nhi *nhi = tb->nhi; -int retries =5 ; +int retries =60 ;u32 val;do { val =ioread32 (nhi->iobase +REG_FW_STS );if (val ®_FW_STS_NVM_AUTH_DONE )break ; -msleep (30 ); +msleep (50 ); }while (--retries);if (!retries) {@@ -1063,6 +1063,9 @@ static int icm_firmware_init(struct tb *tb)break ;default : +if (ret <0 ) +return ret; +tb_err (tb,"ICM firmware is in wrong mode: %u\n" , ret);return -ENODEV ; } --
Quoted 1 line
2.15.1
Mika,
Some of your patches in this series already have the stable tag, but I think especially this one is probably a good candidate to add to @stable.
Re: [PATCH 06/18] thunderbolt: Wait a bit longer for ICM to authenticate the active NVM
Quoted 61 lines · On Tue, Feb 13, 2018 at 05:21:33PM +0000, [email protected] wrote:
Quoted 57 lines
-----Original Message-----
From: Mika Westerberg [mailto:[email protected]]
Sent: Tuesday, February 13, 2018 11:00 AM
To: [email protected]
Cc: Andreas Noever <[email protected]>; Michael Jamet <[email protected]>; Yehezkel Bernat <[email protected]>; Bjorn Helgaas <[email protected]>; Limonciello, Mario <[email protected]>; Radion Mirchevsky <[email protected]>; Mika Westerberg <[email protected]>
Subject: [PATCH 06/18] thunderbolt: Wait a bit longer for ICM to authenticate the active NVMSometimes during cold boot ICM has not yet authenticated the active NVM image leading to timeout and failing the driver probe. Allow ICM to take some more time and increase the timeout to 3 seconds before we give up.
While there fix
icm_firmware_init()to return the real error code without overwriting it with-ENODEV.Signed-off-by Mika Westerberg <[email protected]>--- drivers/thunderbolt/icm.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)drivers/thunderbolt/icm.c +5
-3@@ -736,14 +736,14 @@ static bool icm_ar_is_supported(struct tb *tb)static int icm_ar_get_mode (struct tb *tb) {struct tb_nhi *nhi = tb->nhi; -int retries =5 ; +int retries =60 ;u32 val;do { val =ioread32 (nhi->iobase +REG_FW_STS );if (val ®_FW_STS_NVM_AUTH_DONE )break ; -msleep (30 ); +msleep (50 ); }while (--retries);if (!retries) {@@ -1063,6 +1063,9 @@ static int icm_firmware_init(struct tb *tb)break ;default : +if (ret <0 ) +return ret; +tb_err (tb,"ICM firmware is in wrong mode: %u\n" , ret);return -ENODEV ; } --Quoted 1 line
2.15.1
Mika,
Quoted 2 lines
Some of your patches in this series already have the stable tag, but I think especially this one is probably a good candidate to add to @stable.
You are right. I'll add stable tag for v2.
PATCH 7/18 thunderbolt: Handle rejected Thunderbolt devices
In reply to Mika WesterbergThe ICM firmware may reject devices for different reasons, even if we have asked it to accept anything. If we notice a device is rejected, we just log the event and bail out.
--- drivers/thunderbolt/icm.c | 6 ++++++ drivers/thunderbolt/tb_msgs.h | 1 + 2 files changed, 7 insertions(+)
drivers/thunderbolt/icm.c +6 -0
@@ -410,6 +410,12 @@ icm_fr_device_connected(struct tb *tb, const struct icm_pkg_header *hdr)ICM_LINK_INFO_DEPTH_SHIFT ; authorized = pkg->link_info &ICM_LINK_INFO_APPROVED ; +if (pkg->link_info &ICM_LINK_INFO_REJECTED ) { +tb_info (tb,"switch at %u.%u was rejected by ICM firmware\n" , + link, depth); +return ; + } + ret = icm->get_route (tb, link, depth, &route);if (ret) { tb_err(tb, "failed to find route string for switch at %u.%u\n",
drivers/thunderbolt/tb_msgs.h +1 -0
@@ -176,6 +176,7 @@ struct icm_fr_event_device_connected {#define ICM_LINK_INFO_DEPTH_SHIFT 4#define ICM_LINK_INFO_DEPTH_MASK GENMASK(7, 4)#define ICM_LINK_INFO_APPROVED BIT(8) +#define ICM_LINK_INFO_REJECTED BIT(9)struct icm_fr_pkg_approve_device {struct icm_pkg_header hdr;
Signature · 1 line
2.15.1
Re: [07/18] thunderbolt: Handle rejected Thunderbolt devices
Quoted 15 lines · Mika Westerberg
The ICM firmware may reject devices for different reasons, even if we have asked it to accept anything. If we notice a device is rejected, we just log the event and bail out.
Signed-off-by Mika Westerberg <[email protected]>--- drivers/thunderbolt/icm.c | 6 ++++++ drivers/thunderbolt/tb_msgs.h | 1 + 2 files changed, 7 insertions(+)drivers/thunderbolt/icm.c +0
-0
drivers/thunderbolt/icm.c icm_fr_device_connected(struct tb *tb, const struct icm_pkg_header *hdr) · line 415
Quoted 6 lines
drivers/thunderbolt/icm.c +3
-0@@ -410,6 +410,12 @@ icm_fr_device_connected(struct tb *tb, const struct icm_pkg_header *hdr)ICM_LINK_INFO_DEPTH_SHIFT ; authorized = pkg->link_info &ICM_LINK_INFO_APPROVED ; +if (pkg->link_info &ICM_LINK_INFO_REJECTED ) { +tb_info (tb,"switch at %u.%u was rejected by ICM firmware\n" , + link, depth);
This kind of condition sounds more like an error instead of info. Please bump this up to tb_WARN/tb_warn ideally tb_err().
BTW - why do we have tb_WARN and tb_warn in drivers/thunderbolt/tb.h ?
-jeremy
Quoted 18 lines
drivers/thunderbolt/icm.c +3
-0+return ; + } + ret = icm->get_route (tb, link, depth, &route);if (ret) { tb_err(tb, "failed to find route string for switch at %u.%u\n",drivers/thunderbolt/tb_msgs.h +1
-0@@ -176,6 +176,7 @@ struct icm_fr_event_device_connected {#define ICM_LINK_INFO_DEPTH_SHIFT 4#define ICM_LINK_INFO_DEPTH_MASK GENMASK(7, 4)#define ICM_LINK_INFO_APPROVED BIT(8) +#define ICM_LINK_INFO_REJECTED BIT(9)struct icm_fr_pkg_approve_device {struct icm_pkg_header hdr;
Re: [07/18] thunderbolt: Handle rejected Thunderbolt devices
Quoted 6 lines · Jeremy McNicoll
drivers/thunderbolt/icm.c icm_fr_device_connected(struct tb *tb, const struct icm_pkg_header *hdr) · line 415
Quoted 3 lines
drivers/thunderbolt/icm.c +3
-0+if (pkg->link_info &ICM_LINK_INFO_REJECTED ) { +tb_info (tb,"switch at %u.%u was rejected by ICM firmware\n" , + link, depth);This kind of condition sounds more like an error instead of info. Please bump this up to tb_WARN/tb_warn ideally
tb_err().
No, this is not an error.
Quoted 1 line
BTW - why do we have
tb_WARNandtb_warnindrivers/thunderbolt/tb.h?
_WARN dumps out backtrace as well.
Re: [07/18] thunderbolt: Handle rejected Thunderbolt devices
Quoted 9 lines · Mika Westerberg
Quoted 6 lines · Jeremy McNicoll
drivers/thunderbolt/icm.c icm_fr_device_connected(struct tb *tb, const struct icm_pkg_header *hdr) · line 415
Quoted 3 lines
drivers/thunderbolt/icm.c +3
-0+if (pkg->link_info &ICM_LINK_INFO_REJECTED ) { +tb_info (tb,"switch at %u.%u was rejected by ICM firmware\n" , + link, depth);This kind of condition sounds more like an error instead of info. Please bump this up to tb_WARN/tb_warn ideally
tb_err().No, this is not an error.
To be more clear, it is totally fine to have the firmware to reject some devices. For example in case of the new usbonly security level the firmware rejects other devices but the first.
Re: [07/18] thunderbolt: Handle rejected Thunderbolt devices
Quoted 9 lines · Mika Westerberg
Quoted 8 lines · Mika Westerberg
Quoted 6 lines · Jeremy McNicoll
drivers/thunderbolt/icm.c icm_fr_device_connected(struct tb *tb, const struct icm_pkg_header *hdr) · line 415
Quoted 3 lines
drivers/thunderbolt/icm.c +3
-0+if (pkg->link_info &ICM_LINK_INFO_REJECTED ) { +tb_info (tb,"switch at %u.%u was rejected by ICM firmware\n" , + link, depth);This kind of condition sounds more like an error instead of info. Please bump this up to tb_WARN/tb_warn ideally
tb_err().
Quoted 6 lines
Quoted 1 line
No, this is not an error.
To be more clear, it is totally fine to have the firmware to reject some devices. For example in case of the new usbonly security level the firmware rejects other devices but the first.
Ok. Is that kind of information available to the kernel? What security mode we are in?
ie) if (LINK_REJECTED && !USB_SECURITY)
print "Error switch %u was rejected since its not usbonly"
endif
I am sure something like that simplified pseudo code above would be somewhat useful to users when debugging.
-jeremy
Re: [07/18] thunderbolt: Handle rejected Thunderbolt devices
Quoted 20 lines · Jeremy McNicoll
Quoted 9 lines · Mika Westerberg
Quoted 8 lines · Mika Westerberg
Quoted 6 lines · Jeremy McNicoll
drivers/thunderbolt/icm.c icm_fr_device_connected(struct tb *tb, const struct icm_pkg_header *hdr) · line 415
Quoted 3 lines
drivers/thunderbolt/icm.c +3
-0+if (pkg->link_info &ICM_LINK_INFO_REJECTED ) { +tb_info (tb,"switch at %u.%u was rejected by ICM firmware\n" , + link, depth);This kind of condition sounds more like an error instead of info. Please bump this up to tb_WARN/tb_warn ideally
tb_err().Quoted 6 lines
Quoted 1 line
No, this is not an error.
To be more clear, it is totally fine to have the firmware to reject some devices. For example in case of the new usbonly security level the firmware rejects other devices but the first.
Ok. Is that kind of information available to the kernel? What security mode we are in?
Quoted 6 lines
ie) if (
LINK_REJECTED&& !USB_SECURITY)
print "Error switch %u was rejected since its not usbonly"
endifI am sure something like that simplified pseudo code above would be somewhat useful to users when debugging.
That's why it is on info level so it goes to dmesg but does not scare the user :-)
Re: [07/18] thunderbolt: Handle rejected Thunderbolt devices
Quoted 25 lines · Mika Westerberg
Quoted 24 lines · Jeremy McNicoll
Quoted 9 lines · Mika Westerberg
Quoted 8 lines · Mika Westerberg
Quoted 6 lines · Jeremy McNicoll
drivers/thunderbolt/icm.c icm_fr_device_connected(struct tb *tb, const struct icm_pkg_header *hdr) · line 415
Quoted 3 lines
drivers/thunderbolt/icm.c +3
-0+if (pkg->link_info &ICM_LINK_INFO_REJECTED ) { +tb_info (tb,"switch at %u.%u was rejected by ICM firmware\n" , + link, depth);This kind of condition sounds more like an error instead of info. Please bump this up to tb_WARN/tb_warn ideally
tb_err().Quoted 6 lines
Quoted 1 line
No, this is not an error.
To be more clear, it is totally fine to have the firmware to reject some devices. For example in case of the new usbonly security level the firmware rejects other devices but the first.
Ok. Is that kind of information available to the kernel? What security mode we are in?
ie) if (
LINK_REJECTED&& !USB_SECURITY)
print "Error switch %u was rejected since its not usbonly"
endif
Quoted 6 lines
Quoted 2 lines
I am sure something like that simplified pseudo code above would be somewhat useful to users when debugging.
That's why it is on info level so it goes to dmesg but does not scare the user :-)
The point I am trying to make is that it would be nice to be able to know WHY the link was rejected and not just that it was rejected.
Re: [07/18] thunderbolt: Handle rejected Thunderbolt devices
Quoted 29 lines · Jeremy McNicoll
Quoted 28 lines · Mika Westerberg
Quoted 26 lines · Jeremy McNicoll
Quoted 9 lines · Mika Westerberg
Quoted 8 lines · Mika Westerberg
Quoted 6 lines · Jeremy McNicoll
Review
Quoted 3 lines
(quoted diff) +3
-0+ if (pkg->link_info & ICM_LINK_INFO_REJECTED) { + tb_info(tb, "switch at %u.%u was rejected by ICM firmware\n", + link, depth);This kind of condition sounds more like an error instead of info. Please bump this up to tb_WARN/tb_warn ideally
tb_err().Quoted 6 lines
Quoted 1 line
No, this is not an error.
To be more clear, it is totally fine to have the firmware to reject some devices. For example in case of the new usbonly security level the firmware rejects other devices but the first.
Ok. Is that kind of information available to the kernel? What security mode we are in?
ie) if (
LINK_REJECTED&& !USB_SECURITY)
print "Error switch %u was rejected since its not usbonly"
endifI am sure something like that simplified pseudo code above would be somewhat useful to users when debugging.
Quoted 6 lines
Quoted 3 lines
That's why it is on info level so it goes to dmesg but does not scare the user :-)
The point I am trying to make is that it would be nice to be able to know WHY the link was rejected and not just that it was rejected.
Fair enough. In practice (since we ask the firmware to accept any device) the only reason for rejection is that the topology limit is exceeded (too many devices in the chain).
I'm thinking to change the message to something like:
tb_info(tb, "switch at %u.%u was rejected by ICM firmware because topology limit exceeded\n", link, depth);
And do the same for Titan Ridge in patch [18/18].
Security level can be read directly from "security" sysfs attribute of the domain so that information does not need to be duplicated IMHO.
Does that work for you?
Re: [07/18] thunderbolt: Handle rejected Thunderbolt devices
Quoted 46 lines · Mika Westerberg
Quoted 29 lines · Jeremy McNicoll
Quoted 28 lines · Mika Westerberg
Quoted 26 lines · Jeremy McNicoll
Quoted 9 lines · Mika Westerberg
Quoted 8 lines · Mika Westerberg
Quoted 6 lines · Jeremy McNicoll
Review
Quoted 3 lines
(quoted diff) +3
-0+ if (pkg->link_info & ICM_LINK_INFO_REJECTED) { + tb_info(tb, "switch at %u.%u was rejected by ICM firmware\n", + link, depth);This kind of condition sounds more like an error instead of info. Please bump this up to tb_WARN/tb_warn ideally
tb_err().Quoted 6 lines
Quoted 1 line
No, this is not an error.
To be more clear, it is totally fine to have the firmware to reject some devices. For example in case of the new usbonly security level the firmware rejects other devices but the first.
Ok. Is that kind of information available to the kernel? What security mode we are in?
ie) if (
LINK_REJECTED&& !USB_SECURITY)
print "Error switch %u was rejected since its not usbonly"
endifI am sure something like that simplified pseudo code above would be somewhat useful to users when debugging.
Quoted 6 lines
Quoted 3 lines
That's why it is on info level so it goes to dmesg but does not scare the user :-)
The point I am trying to make is that it would be nice to be able to know WHY the link was rejected and not just that it was rejected.
Fair enough. In practice (since we ask the firmware to accept any device) the only reason for rejection is that the topology limit is exceeded (too many devices in the chain).
I'm thinking to change the message to something like:
tb_info(tb, "switch at %u.%u was rejected by ICM firmware because topology limit exceeded\n", link, depth);
Quoted 7 lines
And do the same for Titan Ridge in patch [18/18].
Security level can be read directly from "security" sysfs attribute of the domain so that information does not need to be duplicated IMHO.
Does that work for you?
Sounds good to me.
-jeremy
PATCH 8/18 thunderbolt: Factor common ICM add and update operations out
In reply to Mika WesterbergThe newer ICM will not use link and depth to address devices. Instead it uses route strings. In order to take advantage of the existing code factor out common operations so that we can use the same functions with the new ICM as well.
No functional changes intended.
--- drivers/thunderbolt/icm.c | 136 +++++++++++++++++++++++++++++----------------- 1 file changed, 87 insertions(+), 49 deletions(-)
drivers/thunderbolt/icm.c +87 -49
@@ -374,6 +374,57 @@ static int icm_fr_disconnect_xdomain_paths(struct tb *tb, struct tb_xdomain *xd)return 0 ; } +static void add_switch (struct tb_switch *parent_sw,u64 route, +const uuid_t *uuid,u8 connection_id,u8 connection_key, +u8 link,u8 depth,enum tb_security_level security_level, +bool authorized) +{ +struct tb_switch *sw; + + sw =tb_switch_alloc (parent_sw->tb, &parent_sw->dev, route); +if (!sw) +return ; + + sw->uuid =kmemdup (uuid,sizeof (*uuid),GFP_KERNEL ); + sw->connection_id = connection_id; + sw->connection_key = connection_key; + sw->link = link; + sw->depth = depth; + sw->authorized = authorized; + sw->security_level = security_level; + +/* Link the two switches now */ +tb_port_at (route, parent_sw)->remote =tb_upstream_port (sw); +tb_upstream_port (sw)->remote =tb_port_at (route, parent_sw); + +if (tb_switch_add (sw)) { +tb_port_at (tb_route (sw), parent_sw)->remote =NULL ; +tb_switch_put (sw); +return ; + } +} + +static void update_switch (struct tb_switch *parent_sw,struct tb_switch *sw, +u64 route,u8 connection_id,u8 connection_key, +u8 link,u8 depth) +{ +/* Disconnect from parent */ +tb_port_at (tb_route (sw), parent_sw)->remote =NULL ; +/* Re-connect via updated port*/ +tb_port_at (route, parent_sw)->remote =tb_upstream_port (sw); + +/* Update with the new addressing information */ + sw->config.route_hi =upper_32_bits (route); + sw->config.route_lo =lower_32_bits (route); + sw->connection_id = connection_id; + sw->connection_key = connection_key; + sw->link = link; + sw->depth = depth; + +/* This switch still exists */ + sw->is_unplugged = false; +} +static void remove_switch (struct tb_switch *sw) {struct tb_switch *parent_sw;
@@ -383,6 +434,31 @@ static void remove_switch(struct tb_switch *sw)tb_switch_remove (sw); } +static void add_xdomain (struct tb_switch *sw,u64 route, +const uuid_t *local_uuid,const uuid_t *remote_uuid, +u8 link,u8 depth) +{ +struct tb_xdomain *xd; + + xd =tb_xdomain_alloc (sw->tb, &sw->dev, route, local_uuid, remote_uuid); +if (!xd) +return ; + + xd->link = link; + xd->depth = depth; + +tb_port_at (route, sw)->xdomain = xd; + +tb_xdomain_add (xd); +} + +static void update_xdomain (struct tb_xdomain *xd,u64 route,u8 link) +{ + xd->link = link; + xd->route = route; + xd->is_unplugged = false; +} +static void remove_xdomain (struct tb_xdomain *xd) {struct tb_switch *sw;
@@ -397,6 +473,7 @@ icm_fr_device_connected(struct tb *tb, const struct icm_pkg_header *hdr) {const struct icm_fr_event_device_connected *pkg = (const struct icm_fr_event_device_connected *)hdr; +enum tb_security_level security_level;struct tb_switch *sw, *parent_sw;struct icm *icm =tb_priv (tb);bool authorized = false;
@@ -409,6 +486,8 @@ icm_fr_device_connected(struct tb *tb, const struct icm_pkg_header *hdr) depth = (pkg->link_info &ICM_LINK_INFO_DEPTH_MASK ) >>ICM_LINK_INFO_DEPTH_SHIFT ; authorized = pkg->link_info &ICM_LINK_INFO_APPROVED ; + security_level = (pkg->hdr.flags &ICM_FLAGS_SLEVEL_MASK ) >> +ICM_FLAGS_SLEVEL_SHIFT ;if (pkg->link_info &ICM_LINK_INFO_REJECTED ) { tb_info(tb, "switch at %u.%u was rejected by ICM firmware\n",
@@ -441,16 +520,8 @@ icm_fr_device_connected(struct tb *tb, const struct icm_pkg_header *hdr)*/ if (sw->depth == depth && sw_phy_port == phy_port && !!sw->authorized == authorized) { -tb_port_at (tb_route (sw), parent_sw)->remote =NULL ; -tb_port_at (route, parent_sw)->remote = -tb_upstream_port (sw); - sw->config.route_hi =upper_32_bits (route); - sw->config.route_lo =lower_32_bits (route); - sw->connection_id = pkg->connection_id; - sw->connection_key = pkg->connection_key; - sw->link = link; - sw->depth = depth; - sw->is_unplugged = false; +update_switch (parent_sw, sw, route, pkg->connection_id, + pkg->connection_key, link, depth);tb_switch_put (sw);return ; }
@@ -497,30 +568,10 @@ icm_fr_device_connected(struct tb *tb, const struct icm_pkg_header *hdr)return ; } - sw =tb_switch_alloc (tb, &parent_sw->dev, route); -if (!sw) { -tb_switch_put (parent_sw); -return ; - } +add_switch (parent_sw, route, &pkg->ep_uuid, pkg->connection_id, + pkg->connection_key, link, depth, security_level, + authorized); - sw->uuid =kmemdup (&pkg->ep_uuid,sizeof (pkg->ep_uuid),GFP_KERNEL ); - sw->connection_id = pkg->connection_id; - sw->connection_key = pkg->connection_key; - sw->link = link; - sw->depth = depth; - sw->authorized = authorized; - sw->security_level = (pkg->hdr.flags &ICM_FLAGS_SLEVEL_MASK ) >> -ICM_FLAGS_SLEVEL_SHIFT ; - -/* Link the two switches now */ -tb_port_at (route, parent_sw)->remote =tb_upstream_port (sw); -tb_upstream_port (sw)->remote =tb_port_at (route, parent_sw); - - ret =tb_switch_add (sw); -if (ret) { -tb_port_at (tb_route (sw), parent_sw)->remote =NULL ; -tb_switch_put (sw); - }tb_switch_put (parent_sw); }
@@ -591,9 +642,7 @@ icm_fr_xdomain_connected(struct tb *tb, const struct icm_pkg_header *hdr) phy_port =phy_port_from_route (route, depth);if (xd->depth == depth && xd_phy_port == phy_port) { - xd->link = link; - xd->route = route; - xd->is_unplugged = false; +update_xdomain (xd, route, link);tb_xdomain_put (xd);return ; }
@@ -643,19 +692,8 @@ icm_fr_xdomain_connected(struct tb *tb, const struct icm_pkg_header *hdr)return ; } - xd =tb_xdomain_alloc (sw->tb, &sw->dev, route, - &pkg->local_uuid, &pkg->remote_uuid); -if (!xd) { -tb_switch_put (sw); -return ; - } - - xd->link = link; - xd->depth = depth; - -tb_port_at (route, sw)->xdomain = xd; - -tb_xdomain_add (xd); +add_xdomain (sw, route, &pkg->local_uuid, &pkg->remote_uuid, link, + depth);tb_switch_put (sw); }
Signature · 1 line
2.15.1
PATCH 9/18 thunderbolt: Correct function name in kernel-doc comment
In reply to Mika WesterbergFrom: Radion Mirchevsky <[email protected]>
Use correct name in kernel-doc of tb_switch_find_by_uuid().
--- drivers/thunderbolt/switch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/thunderbolt/switch.c +1 -1
@@ -1528,7 +1528,7 @@ struct tb_switch *tb_switch_find_by_link_depth(struct tb *tb, u8 link, u8 depth) }/** -* tb_switch_find_by_link_depth() - Find switch by UUID +* tb_switch_find_by_uuid() - Find switch by UUID * @tb: Domain the switch belongs * @uuid: UUID to look for *
Signature · 1 line
2.15.1
PATCH 10/18 thunderbolt: Add tb_switch_get()
In reply to Mika WesterbergSometimes there is need for increasing reference count of a switch as well. This also follows what we have for xdomains.
--- drivers/thunderbolt/tb.h | 7 +++++++ 1 file changed, 7 insertions(+)
drivers/thunderbolt/tb.h +7 -0
@@ -387,6 +387,13 @@ struct tb_switch *tb_switch_find_by_link_depth(struct tb *tb, u8 link, u8 depth);struct tb_switch *tb_switch_find_by_uuid (struct tb *tb,const uuid_t *uuid); +static inline struct tb_switch *tb_switch_get (struct tb_switch *sw) +{ +if (sw) +get_device (&sw->dev); +return sw; +} +static inline void tb_switch_put (struct tb_switch *sw) {put_device (&sw->dev);
Signature · 1 line
2.15.1
PATCH 11/18 thunderbolt: Add tb_switch_find_by_route()
In reply to Mika WesterbergFrom: Radion Mirchevsky <[email protected]>
With the new ICM messaging there is need for find switch by route string instead of link and depth. Add new function that makes it possible.
--- drivers/thunderbolt/switch.c | 33 +++++++++++++++++++++++++++++++++ drivers/thunderbolt/tb.h | 1 + 2 files changed, 34 insertions(+)
drivers/thunderbolt/switch.c +33 -0
@@ -1479,6 +1479,7 @@ struct tb_sw_lookup {u8 link;u8 depth;const uuid_t *uuid; +u64 route; };static int tb_switch_match (struct device *dev,void *data)
@@ -1494,6 +1495,11 @@ static int tb_switch_match(struct device *dev, void *data)if (lookup->uuid)return !memcmp (sw->uuid, lookup->uuid,sizeof (*lookup->uuid)); +if (lookup->route) { +return sw->config.route_lo ==lower_32_bits (lookup->route) && + sw->config.route_hi ==upper_32_bits (lookup->route); + } +/* Root switch is matched only by depth */ if (!lookup->depth)return !sw->depth;
@@ -1551,6 +1557,33 @@ struct tb_switch *tb_switch_find_by_uuid(struct tb *tb, const uuid_t *uuid)return NULL ; } +/** +* tb_switch_find_by_route() - Find switch by route string +* @tb: Domain the switch belongs +* @route: Route string to look for +* +* Returned switch has reference count increased so the caller needs to +* call tb_switch_put() when done with the switch. +*/ +struct tb_switch *tb_switch_find_by_route (struct tb *tb,u64 route) +{ +struct tb_sw_lookup lookup; +struct device *dev; + +if (!route) +return tb_switch_get (tb->root_switch); + +memset (&lookup,0 ,sizeof (lookup)); + lookup.tb = tb; + lookup.route = route; + + dev =bus_find_device (&tb_bus_type,NULL , &lookup, tb_switch_match); +if (dev) +return tb_to_switch (dev); + +return NULL ; +} +void tb_switch_exit (void ) {ida_destroy (&nvm_ida);
drivers/thunderbolt/tb.h +1 -0
@@ -386,6 +386,7 @@ struct tb_switch *get_switch_at_route(struct tb_switch *sw, u64 route);struct tb_switch *tb_switch_find_by_link_depth (struct tb *tb,u8 link,u8 depth);struct tb_switch *tb_switch_find_by_uuid (struct tb *tb,const uuid_t *uuid); +struct tb_switch *tb_switch_find_by_route (struct tb *tb,u64 route);static inline struct tb_switch *tb_switch_get (struct tb_switch *sw) {
Signature · 1 line
2.15.1
PATCH 12/18 thunderbolt: Add tb_xdomain_find_by_route()
In reply to Mika WesterbergFrom: Radion Mirchevsky <[email protected]>
This is needed by the new ICM interface to find xdomains by route string instead of link and depth.
--- drivers/thunderbolt/xdomain.c | 40 +++++++++++++++++++++++++++++++++++++++- include/linux/thunderbolt.h | 13 +++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-)
drivers/thunderbolt/xdomain.c +39 -1
@@ -1255,6 +1255,7 @@ struct tb_xdomain_lookup {const uuid_t *uuid;u8 link;u8 depth; +u64 route; }; static struct tb_xdomain *switch_find_xdomain(struct tb_switch *sw,
@@ -1275,9 +1276,13 @@ static struct tb_xdomain *switch_find_xdomain(struct tb_switch *sw,if (lookup->uuid) {if (uuid_equal (xd->remote_uuid, lookup->uuid))return xd; - }else if (lookup->link == xd->link && + }else if (lookup->link && + lookup->link == xd->link && lookup->depth == xd->depth) {return xd; + }else if (lookup->route && + lookup->route == xd->route) { +return xd; } }else if (port->remote) { xd =switch_find_xdomain (port->remote->sw, lookup);
@@ -1357,6 +1362,39 @@ struct tb_xdomain *tb_xdomain_find_by_link_depth(struct tb *tb, u8 link,return NULL ; } +/** +* tb_xdomain_find_by_route() - Find an XDomain by route string +* @tb: Domain where the XDomain belongs to +* @route: XDomain route string +* +* Finds XDomain by walking through the Thunderbolt topology below @tb. +* The returned XDomain will have its reference count increased so the +* caller needs to call tb_xdomain_put() when it is done with the +* object. +* +* This will find all XDomains including the ones that are not yet added +* to the bus (handshake is still in progress). +* +* The caller needs to hold @tb->lock. +*/ +struct tb_xdomain *tb_xdomain_find_by_route (struct tb *tb,u64 route) +{ +struct tb_xdomain_lookup lookup; +struct tb_xdomain *xd; + +memset (&lookup,0 ,sizeof (lookup)); + lookup.route = route; + + xd =switch_find_xdomain (tb->root_switch, &lookup); +if (xd) { +get_device (&xd->dev); +return xd; + } + +return NULL ; +} +EXPORT_SYMBOL_GPL (tb_xdomain_find_by_route); +bool tb_xdomain_handle_request (struct tb *tb,enum tb_cfg_pkg_type type,const void *buf,size_t size) {
include/linux/thunderbolt.h +13 -0
@@ -237,6 +237,7 @@ int tb_xdomain_enable_paths(struct tb_xdomain *xd, u16 transmit_path, u16 receive_ring);int tb_xdomain_disable_paths (struct tb_xdomain *xd);struct tb_xdomain *tb_xdomain_find_by_uuid (struct tb *tb,const uuid_t *uuid); +struct tb_xdomain *tb_xdomain_find_by_route (struct tb *tb,u64 route);static inline struct tb_xdomain *tb_xdomain_find_by_uuid_locked (struct tb *tb,const uuid_t *uuid)
@@ -250,6 +251,18 @@ tb_xdomain_find_by_uuid_locked(struct tb *tb, const uuid_t *uuid)return xd; } +static inline struct tb_xdomain * +tb_xdomain_find_by_route_locked (struct tb *tb,u64 route) +{ +struct tb_xdomain *xd; + +mutex_lock (&tb->lock); + xd =tb_xdomain_find_by_route (tb, route); +mutex_unlock (&tb->lock); + +return xd; +} +static inline struct tb_xdomain *tb_xdomain_get (struct tb_xdomain *xd) {if (xd)
Signature · 1 line
2.15.1
Re: [PATCH 12/18] thunderbolt: Add tb_xdomain_find_by_route()
drivers/thunderbolt/xdomain.c struct tb_xdomain *tb_xdomain_find_by_link_depth(struct tb *tb, u8 link, · line 1394
Quoted 13 lines · Mika Westerberg
From: Radion Mirchevsky <[email protected]>
This is needed by the new ICM interface to find xdomains by route string instead of link and depth.
drivers/thunderbolt/xdomain.c +7
-0+ xd =switch_find_xdomain (tb->root_switch, &lookup); +if (xd) { +get_device (&xd->dev); +return xd; + } + +return NULL ;
return tb_xdomain_get(xd);
?
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH 12/18] thunderbolt: Add tb_xdomain_find_by_route()
Quoted 9 lines · Andy Shevchenko
Quoted 6 lines · Mika Westerberg
From: Radion Mirchevsky <[email protected]>
This is needed by the new ICM interface to find xdomains by route string instead of link and depth.
+ xd =
switch_find_xdomain(tb->root_switch, &lookup);
Quoted 8 lines
drivers/thunderbolt/xdomain.c struct tb_xdomain *tb_xdomain_find_by_link_depth(struct tb *tb, u8 link, · line 1394
Quoted 6 lines
drivers/thunderbolt/xdomain.c +6
-0+if (xd) { +get_device (&xd->dev); +return xd; + } + +return NULL ;return
tb_xdomain_get(xd);
Indeed, that can be used here as well. I'll change it in v2.
PATCH 13/18 thunderbolt: Add constant for approval timeout
In reply to Mika WesterbergWe will be using this from Titan Ridge support code as well so make it constant.
--- drivers/thunderbolt/icm.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/thunderbolt/icm.c +3 -2
@@ -41,7 +41,8 @@#define PHY_PORT_CS1_LINK_STATE_MASK GENMASK(29, 26)#define PHY_PORT_CS1_LINK_STATE_SHIFT 26 -#define ICM_TIMEOUT 5000/* ms */ +#define ICM_TIMEOUT 5000/* ms */ +#define ICM_APPROVE_TIMEOUT 10000/* ms */ #define ICM_MAX_LINK 4#define ICM_MAX_DEPTH 6
@@ -260,7 +261,7 @@ static int icm_fr_approve_switch(struct tb *tb, struct tb_switch *sw)memset (&reply,0 ,sizeof (reply));/* Use larger timeout as establishing tunnels can take some time */ ret =icm_request (tb, &request,sizeof (request), &reply,sizeof (reply), -1 ,10000 ); +1 ,ICM_APPROVE_TIMEOUT );if (ret)return ret;
Signature · 1 line
2.15.1
PATCH 14/18 thunderbolt: Move driver ready handling to struct icm
In reply to Mika WesterbergIntel Titan Ridge uses slightly different format for ICM driver ready response, so add a new ->driver_ready() callback to struct icm and move the existing handling to a separate function which we then use in Falcon Ridge and Alpine Ridge.
No functional changes intended.
--- drivers/thunderbolt/icm.c | 42 +++++++++++++++++++++++++++++++----------- drivers/thunderbolt/tb_msgs.h | 6 ++++-- 2 files changed, 35 insertions(+), 13 deletions(-)
drivers/thunderbolt/icm.c +31 -11
@@ -59,6 +59,7 @@* @is_supported: Checks if we can support ICM on this controller * @get_mode: Read and return the ICM firmware mode (optional) * @get_route: Find a route string for given switch +* @driver_ready: Send driver ready message to ICM * @device_connected: Handle device connected ICM message * @device_disconnected: Handle device disconnected ICM message * @xdomain_connected - Handle XDomain connected ICM message
@@ -73,6 +74,8 @@ struct icm {bool (*is_supported)(struct tb *tb);int (*get_mode)(struct tb *tb);int (*get_route)(struct tb *tb,u8 link,u8 depth,u64 *route); +int (*driver_ready)(struct tb *tb, +enum tb_security_level *security_level);void (*device_connected)(struct tb *tb,const struct icm_pkg_header *hdr); void (*device_disconnected)(struct tb *tb,
@@ -246,6 +249,27 @@ static int icm_fr_get_route(struct tb *tb, u8 link, u8 depth, u64 *route)return ret; } +static int +icm_fr_driver_ready (struct tb *tb,enum tb_security_level *security_level) +{ +struct icm_fr_pkg_driver_ready_response reply; +struct icm_pkg_driver_ready request = { + .hdr.code =ICM_DRIVER_READY , + }; +int ret; + +memset (&reply,0 ,sizeof (reply)); + ret =icm_request (tb, &request,sizeof (request), &reply,sizeof (reply), +1 ,ICM_TIMEOUT ); +if (ret) +return ret; + +if (security_level) + *security_level = reply.security_level &ICM_FR_SLEVEL_MASK ; + +return 0 ; +} +static int icm_fr_approve_switch (struct tb *tb,struct tb_switch *sw) {struct icm_fr_pkg_approve_device request;
@@ -869,21 +893,15 @@ static void icm_handle_event(struct tb *tb, enum tb_cfg_pkg_type type,static int __icm_driver_ready (struct tb *tb,enum tb_security_level *security_level) { -struct icm_pkg_driver_ready_response reply; -struct icm_pkg_driver_ready request = { - .hdr.code =ICM_DRIVER_READY , - }; +struct icm *icm =tb_priv (tb);unsigned int retries =50 ;int ret; -memset (&reply,0 ,sizeof (reply)); - ret =icm_request (tb, &request,sizeof (request), &reply,sizeof (reply), -1 ,ICM_TIMEOUT ); -if (ret) + ret = icm->driver_ready (tb, security_level); +if (ret) { +tb_err (tb,"failed to send driver ready to ICM\n" );return ret; - -if (security_level) - *security_level = reply.security_level &0xf ; + }/* * Hold on here until the switch config space is accessible so
@@ -1329,6 +1347,7 @@ struct tb *icm_probe(struct tb_nhi *nhi)case PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_NHI : icm->is_supported = icm_fr_is_supported; icm->get_route = icm_fr_get_route; + icm->driver_ready = icm_fr_driver_ready; icm->device_connected = icm_fr_device_connected; icm->device_disconnected = icm_fr_device_disconnected; icm->xdomain_connected = icm_fr_xdomain_connected;
@@ -1344,6 +1363,7 @@ struct tb *icm_probe(struct tb_nhi *nhi) icm->is_supported = icm_ar_is_supported; icm->get_mode = icm_ar_get_mode; icm->get_route = icm_ar_get_route; + icm->driver_ready = icm_fr_driver_ready; icm->device_connected = icm_fr_device_connected; icm->device_disconnected = icm_fr_device_disconnected; icm->xdomain_connected = icm_fr_xdomain_connected;
drivers/thunderbolt/tb_msgs.h +4 -2
@@ -127,14 +127,16 @@ struct icm_pkg_driver_ready {struct icm_pkg_header hdr; }; -struct icm_pkg_driver_ready_response { +/* Falcon Ridge & Alpine Ridge common messages */ + +struct icm_fr_pkg_driver_ready_response {struct icm_pkg_header hdr;u8 romver;u8 ramver;u16 security_level; }; -/* Falcon Ridge & Alpine Ridge common messages */ +#define ICM_FR_SLEVEL_MASK 0xfstruct icm_fr_pkg_get_topology {struct icm_pkg_header hdr;
Signature · 1 line
2.15.1
PATCH 15/18 thunderbolt: Add 'boot' attribute for devices
In reply to Mika WesterbergFrom: Yehezkel Bernat <[email protected]>
In various cases, Thunderbolt device can be connected by ICM on boot without waiting for approval from user. Most cases are related to OEM-specific BIOS configurations. This information is interesting for user-space as if the device isn't in SW ACL, it may create a friction in the user experience where the device is automatically authorized if it's connected on boot but requires an explicit user action if connected after OS is up. User-space can use this information to suggest adding the device to SW ACL for auto-authorization on later connections.
--- Documentation/ABI/testing/sysfs-bus-thunderbolt | 7 +++++++ drivers/thunderbolt/icm.c | 12 ++++++++---- drivers/thunderbolt/switch.c | 14 ++++++++++++++ drivers/thunderbolt/tb.h | 2 ++ drivers/thunderbolt/tb_msgs.h | 1 + 5 files changed, 32 insertions(+), 4 deletions(-)
Documentation/ABI/testing/sysfs-bus-thunderbolt +7 -0
@@ -38,6 +38,13 @@ Description: This attribute is used to authorize Thunderbolt devices the device did not contain a key at all, and EKEYREJECTED if the challenge response did not match. +What: /sys/bus/thunderbolt/devices/.../boot +Date: Jun 2018 +KernelVersion: 4.17 +Contact: [email protected] +Description: This attribute contains 1 if Thunderbolt device was already + authorized on boot and 0 otherwise. + What: /sys/bus/thunderbolt/devices/.../key Date: Sep 2017 KernelVersion: 4.13
drivers/thunderbolt/icm.c +8 -4
@@ -402,7 +402,7 @@ static int icm_fr_disconnect_xdomain_paths(struct tb *tb, struct tb_xdomain *xd)static void add_switch (struct tb_switch *parent_sw,u64 route,const uuid_t *uuid,u8 connection_id,u8 connection_key,u8 link,u8 depth,enum tb_security_level security_level, -bool authorized) +bool authorized,bool boot) {struct tb_switch *sw;
@@ -417,6 +417,7 @@ static void add_switch(struct tb_switch *parent_sw, u64 route, sw->depth = depth; sw->authorized = authorized; sw->security_level = security_level; + sw->boot = boot;/* Link the two switches now */ tb_port_at (route, parent_sw)->remote =tb_upstream_port (sw);
@@ -431,7 +432,7 @@ static void add_switch(struct tb_switch *parent_sw, u64 route,static void update_switch (struct tb_switch *parent_sw,struct tb_switch *sw,u64 route,u8 connection_id,u8 connection_key, -u8 link,u8 depth) +u8 link,u8 depth,bool boot) {/* Disconnect from parent */ tb_port_at (tb_route (sw), parent_sw)->remote =NULL ;
@@ -445,6 +446,7 @@ static void update_switch(struct tb_switch *parent_sw, struct tb_switch *sw, sw->connection_key = connection_key; sw->link = link; sw->depth = depth; + sw->boot = boot;/* This switch still exists */ sw->is_unplugged = false;
@@ -504,6 +506,7 @@ icm_fr_device_connected(struct tb *tb, const struct icm_pkg_header *hdr)bool authorized = false;struct tb_xdomain *xd;u8 link, depth; +bool boot;u64 route;int ret;
@@ -513,6 +516,7 @@ icm_fr_device_connected(struct tb *tb, const struct icm_pkg_header *hdr) authorized = pkg->link_info &ICM_LINK_INFO_APPROVED ; security_level = (pkg->hdr.flags &ICM_FLAGS_SLEVEL_MASK ) >>ICM_FLAGS_SLEVEL_SHIFT ; + boot = pkg->link_info &ICM_LINK_INFO_BOOT ;if (pkg->link_info &ICM_LINK_INFO_REJECTED ) { tb_info(tb, "switch at %u.%u was rejected by ICM firmware\n",
@@ -546,7 +550,7 @@ icm_fr_device_connected(struct tb *tb, const struct icm_pkg_header *hdr)if (sw->depth == depth && sw_phy_port == phy_port && !!sw->authorized == authorized) {update_switch (parent_sw, sw, route, pkg->connection_id, - pkg->connection_key, link, depth); + pkg->connection_key, link, depth, boot);tb_switch_put (sw);return ; }
@@ -595,7 +599,7 @@ icm_fr_device_connected(struct tb *tb, const struct icm_pkg_header *hdr)add_switch (parent_sw, route, &pkg->ep_uuid, pkg->connection_id, pkg->connection_key, link, depth, security_level, - authorized); + authorized, boot);tb_switch_put (parent_sw); }
drivers/thunderbolt/switch.c +14 -0
@@ -775,6 +775,15 @@ static ssize_t authorized_store(struct device *dev, }static DEVICE_ATTR_RW (authorized); +static ssize_t boot_show (struct device *dev,struct device_attribute *attr, +char *buf) +{ +struct tb_switch *sw =tb_to_switch (dev); + +return sprintf (buf,"%u\n" , sw->boot); +} +static DEVICE_ATTR_RO (boot); +static ssize_t device_show (struct device *dev,struct device_attribute *attr,char *buf) {
@@ -951,6 +960,7 @@ static DEVICE_ATTR_RO(unique_id);static struct attribute *switch_attrs[] = { &dev_attr_authorized.attr, + &dev_attr_boot.attr, &dev_attr_device.attr, &dev_attr_device_name.attr, &dev_attr_key.attr,
@@ -979,6 +989,10 @@ static umode_t switch_attr_is_visible(struct kobject *kobj,if (sw->dma_port)return attr->mode;return 0 ; + }else if (attr == &dev_attr_boot.attr) { +if (tb_route (sw)) +return attr->mode; +return 0 ; }return sw->safe_mode ?0 : attr->mode;
drivers/thunderbolt/tb.h +2 -0
@@ -66,6 +66,7 @@ struct tb_switch_nvm {* @nvm: Pointer to the NVM if the switch has one (%NULL otherwise) * @no_nvm_upgrade: Prevent NVM upgrade of this switch * @safe_mode: The switch is in safe-mode +* @boot: Whether the switch was already authorized on boot or not * @authorized: Whether the switch is authorized by user or policy * @work: Work used to automatically authorize a switch * @security_level: Switch supported security level
@@ -99,6 +100,7 @@ struct tb_switch {struct tb_switch_nvm *nvm;bool no_nvm_upgrade;bool safe_mode; +bool boot;unsigned int authorized;struct work_struct work;enum tb_security_level security_level;
drivers/thunderbolt/tb_msgs.h +1 -0
@@ -179,6 +179,7 @@ struct icm_fr_event_device_connected {#define ICM_LINK_INFO_DEPTH_MASK GENMASK(7, 4)#define ICM_LINK_INFO_APPROVED BIT(8)#define ICM_LINK_INFO_REJECTED BIT(9) +#define ICM_LINK_INFO_BOOT BIT(10)struct icm_fr_pkg_approve_device {struct icm_pkg_header hdr;
Signature · 1 line
2.15.1
PATCH 16/18 thunderbolt: Add support for preboot ACL
In reply to Mika WesterbergPreboot ACL is a mechanism that allows connecting Thunderbolt devices boot time in more secure way than the legacy Thunderbolt boot support. As with the legacy boot option, this also needs to be enabled from the BIOS before booting is allowed. Difference to the legacy mode is that the userspace software explicitly adds device UUIDs by sending a special message to the ICM firmware. Only the devices listed in the boot ACL are connected automatically during the boot. This works in both "user" and "secure" security levels.
We implement this in Linux by exposing a new sysfs attribute (boot_acl) below each Thunderbolt domain. The userspace software can then update the full list as needed.
--- Documentation/ABI/testing/sysfs-bus-thunderbolt | 23 ++++ drivers/thunderbolt/domain.c | 122 ++++++++++++++++++ drivers/thunderbolt/icm.c | 157 ++++++++++++++++++++++-- drivers/thunderbolt/tb.h | 4 + drivers/thunderbolt/tb_msgs.h | 35 +++++- include/linux/thunderbolt.h | 2 + 6 files changed, 333 insertions(+), 10 deletions(-)
Documentation/ABI/testing/sysfs-bus-thunderbolt +23 -0
@@ -1,3 +1,26 @@ +What: /sys/bus/thunderbolt/devices/.../domainX/boot_acl +Date: Jun 2018 +KernelVersion: 4.17 +Contact: [email protected] +Description: Holds a comma separated list of device unique_ids that + are allowed to be connected automatically during system + startup (e.g boot devices). The list always contains + maximum supported number of unique_ids where unused + entries are empty. This allows the userspace software + to determine how many entries the controller supports. + If there are multiple controllers, each controller has + its own ACL list and size may be different between the + controllers. + + System BIOS may have an option "Preboot ACL" or similar + that needs to be selected before this list is taken into + consideration. + + Software always updates a full list in each write. + + If a device is authorized automatically during boot its + boot attribute is set to 1. + What: /sys/bus/thunderbolt/devices/.../domainX/security Date: Sep 2017 KernelVersion: 4.13
drivers/thunderbolt/domain.c +122 -0
139 lines not on this page · raw message
drivers/thunderbolt/icm.c +148 -9
241 lines not on this page · raw message
drivers/thunderbolt/tb.h +4 -0
@@ -200,6 +200,8 @@ struct tb_path {* @suspend: Connection manager specific suspend * @complete: Connection manager specific complete * @handle_event: Handle thunderbolt event +* @get_boot_acl: Get boot ACL list +* @set_boot_acl: Set boot ACL list * @approve_switch: Approve switch * @add_switch_key: Add key to switch * @challenge_switch_key: Challenge switch using key
@@ -217,6 +219,8 @@ struct tb_cm_ops {void (*complete)(struct tb *tb);void (*handle_event)(struct tb *tb,enum tb_cfg_pkg_type ,const void *buf,size_t size); +int (*get_boot_acl)(struct tb *tb,uuid_t *uuids,size_t nuuids); +int (*set_boot_acl)(struct tb *tb,const uuid_t *uuids,size_t nuuids);int (*approve_switch)(struct tb *tb,struct tb_switch *sw);int (*add_switch_key)(struct tb *tb,struct tb_switch *sw); int (*challenge_switch_key)(struct tb *tb, struct tb_switch *sw,
drivers/thunderbolt/tb_msgs.h +34 -1
@@ -102,6 +102,7 @@ enum icm_pkg_code {ICM_ADD_DEVICE_KEY =0x6 ,ICM_GET_ROUTE =0xa ,ICM_APPROVE_XDOMAIN =0x10 , + ICM_PREBOOT_ACL = 0x18, }; enum icm_event_code {
@@ -122,12 +123,13 @@ struct icm_pkg_header {#define ICM_FLAGS_NO_KEY BIT(1)#define ICM_FLAGS_SLEVEL_SHIFT 3#define ICM_FLAGS_SLEVEL_MASK GENMASK(4, 3) +#define ICM_FLAGS_WRITE BIT(7)struct icm_pkg_driver_ready {struct icm_pkg_header hdr; }; -/* Falcon Ridge & Alpine Ridge common messages */ +/* Falcon Ridge only messages */ struct icm_fr_pkg_driver_ready_response {struct icm_pkg_header hdr;
@@ -138,6 +140,8 @@ struct icm_fr_pkg_driver_ready_response {#define ICM_FR_SLEVEL_MASK 0xf +/* Falcon Ridge & Alpine Ridge common messages */ +struct icm_fr_pkg_get_topology {struct icm_pkg_header hdr; };
@@ -274,6 +278,18 @@ struct icm_fr_pkg_approve_xdomain_response {/* Alpine Ridge only messages */ +struct icm_ar_pkg_driver_ready_response { +struct icm_pkg_header hdr; +u8 romver; +u8 ramver; +u16 info; +}; + +#define ICM_AR_INFO_SLEVEL_MASK 0xf +#define ICM_AR_INFO_BOOT_ACL_SHIFT 7 +#define ICM_AR_INFO_BOOT_ACL_MASK GENMASK(11, 7) +#define ICM_AR_INFO_BOOT_ACL_SUPPORTED BIT(13) +struct icm_ar_pkg_get_route {struct icm_pkg_header hdr;u16 reserved;
@@ -288,6 +304,23 @@ struct icm_ar_pkg_get_route_response {u32 route_lo; }; +struct icm_ar_boot_acl_entry { +u32 uuid_lo; +u32 uuid_hi; +}; + +#define ICM_AR_PREBOOT_ACL_ENTRIES 16 + +struct icm_ar_pkg_preboot_acl { +struct icm_pkg_header hdr; +struct icm_ar_boot_acl_entry acl[ICM_AR_PREBOOT_ACL_ENTRIES ]; +}; + +struct icm_ar_pkg_preboot_acl_response { +struct icm_pkg_header hdr; +struct icm_ar_boot_acl_entry acl[ICM_AR_PREBOOT_ACL_ENTRIES ]; +}; +/* XDomain messages */ struct tb_xdomain_header {
include/linux/thunderbolt.h +2 -0
@@ -65,6 +65,7 @@ enum tb_security_level {* @cm_ops: Connection manager specific operations vector * @index: Linux assigned domain number * @security_level: Current security level +* @nboot_acl: Number of boot ACLs the domain supports * @privdata: Private connection manager specific data */ struct tb {
@@ -77,6 +78,7 @@ struct tb {const struct tb_cm_ops *cm_ops;int index;enum tb_security_level security_level; +size_t nboot_acl;unsigned long privdata[0 ]; };
Signature · 1 line
2.15.1
Re: [PATCH 16/18] thunderbolt: Add support for preboot ACL
Quoted 13 lines · Mika Westerberg
Preboot ACL is a mechanism that allows connecting Thunderbolt devices boot time in more secure way than the legacy Thunderbolt boot support. As with the legacy boot option, this also needs to be enabled from the BIOS before booting is allowed. Difference to the legacy mode is that the userspace software explicitly adds device UUIDs by sending a special message to the ICM firmware. Only the devices listed in the boot ACL are connected automatically during the boot. This works in both "user" and "secure" security levels.
We implement this in Linux by exposing a new sysfs attribute (
boot_acl) below each Thunderbolt domain. The userspace software can then update the full list as needed.
drivers/thunderbolt/domain.c static const char * const tb_security_names[] = { · line 142
Quoted 10 lines
drivers/thunderbolt/domain.c +9
-0+if (mutex_lock_interruptible (&tb->lock)) { + ret = -ERESTARTSYS ; + goto out; + } + ret = tb->cm_ops->get_boot_acl (tb, uuids, tb->nboot_acl); +mutex_unlock (&tb->lock); + +if (ret) + goto out;
Can we use more traditional pattern, i.e.mutex_lock();
ret = ...;
if (ret) {
...
mutex_unlock();
goto ...;
}mutex_unlock();
?
Quoted 1 line
+ for (ret = 0, i = 0; i <
tb->nboot_acl; i++) {
Wouldn't be slightly better to check for overflow beforehand
i < min(PAGE_SIZE / (UUID_STRING_LEN + 1), tb->nboot_acl)
and then simple
ret = sprintf(buf + ret, "%pUb", &uuids[i]);
?
Quoted 8 lines
drivers/thunderbolt/domain.c +7
-0+if (!uuid_is_null (&uuids[i])) + ret +=snprintf (buf + ret,PAGE_SIZE - ret,"%pUb" , + &uuids[i]); + + ret +=snprintf (buf + ret,PAGE_SIZE - ret,"%s" , + i < tb->nboot_acl -1 ?"," :"\n" ); + }
drivers/thunderbolt/domain.c static const char * const tb_security_names[] = { · line 188
Quoted 10 lines
drivers/thunderbolt/domain.c +6
-0+static ssize_t boot_acl_store (struct device *dev,struct device_attribute *attr, +const char *buf,size_t count) +{ +int i =0 ; + uuid_str =strim (str); +while ((s =strsep (&uuid_str,"," )) !=NULL && i < tb->nboot_acl) {
for (i = 0; (s = strsep(&uuid_str, ",")) != NULL && i < tb->nboot_acl; i++) {
size_t len = strlen(s);
if (!len)
continue;
...
}
?
Btw, nboot_acl can be 0, right? Perhaps check it first? (Or in other
words: which one is anticipated to be more frequent: nboot_acl = 0, or string w/o any ',' in it?)
drivers/thunderbolt/domain.c static const char * const tb_security_names[] = { · line 195
Quoted 9 lines
drivers/thunderbolt/domain.c +7
-0+size_t len =strlen (s); + +if (len) { +if (len !=UUID_STRING_LEN ) { + ret = -EINVAL ; + goto err_free_acl; + }
uuid_parse() does this check. No need to duplicate.
Quoted 7 lines
drivers/thunderbolt/domain.c +7
-0+ ret =uuid_parse (s, &acl[i]); +if (ret) + goto err_free_acl; + } + + i++; + }
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH 16/18] thunderbolt: Add support for preboot ACL
Quoted 36 lines · Andy Shevchenko
Quoted 13 lines · Mika Westerberg
Preboot ACL is a mechanism that allows connecting Thunderbolt devices boot time in more secure way than the legacy Thunderbolt boot support. As with the legacy boot option, this also needs to be enabled from the BIOS before booting is allowed. Difference to the legacy mode is that the userspace software explicitly adds device UUIDs by sending a special message to the ICM firmware. Only the devices listed in the boot ACL are connected automatically during the boot. This works in both "user" and "secure" security levels.
We implement this in Linux by exposing a new sysfs attribute (
boot_acl) below each Thunderbolt domain. The userspace software can then update the full list as needed.drivers/thunderbolt/domain.c static const char * const tb_security_names[] = { · line 142
Quoted 10 lines
drivers/thunderbolt/domain.c +9
-0+if (mutex_lock_interruptible (&tb->lock)) { + ret = -ERESTARTSYS ; + goto out; + } + ret = tb->cm_ops->get_boot_acl (tb, uuids, tb->nboot_acl); +mutex_unlock (&tb->lock); + +if (ret) + goto out;Can we use more traditional pattern, i.e.
mutex_lock();
ret = ...;
if (ret) {
...
mutex_unlock();
goto ...;
}mutex_unlock();
Quoted 1 line
?
OK.
Quoted 11 lines
Quoted 1 line
+ for (ret = 0, i = 0; i <
tb->nboot_acl; i++) {Wouldn't be slightly better to check for overflow beforehand
i <
min(PAGE_SIZE / (UUID_STRING_LEN + 1), tb->nboot_acl)and then simple
ret =
sprintf(buf + ret, "%pUb", &uuids[i]);?
Well, this follows the common pattern used with formatting sysfs attributes.
Quoted 22 lines
Quoted 8 lines
drivers/thunderbolt/domain.c +7
-0+if (!uuid_is_null (&uuids[i])) + ret +=snprintf (buf + ret,PAGE_SIZE - ret,"%pUb" , + &uuids[i]); + + ret +=snprintf (buf + ret,PAGE_SIZE - ret,"%s" , + i < tb->nboot_acl -1 ?"," :"\n" ); + }drivers/thunderbolt/domain.c static const char * const tb_security_names[] = { · line 188
Quoted 10 lines
drivers/thunderbolt/domain.c +6
-0+static ssize_t boot_acl_store (struct device *dev,struct device_attribute *attr, +const char *buf,size_t count) +{ +int i =0 ; + uuid_str =strim (str); +while ((s =strsep (&uuid_str,"," )) !=NULL && i < tb->nboot_acl) {for (i = 0; (s =
strsep(&uuid_str, ",")) != NULL && i <tb->nboot_acl; i++) {
size_tlen = strlen(s);
Quoted 6 lines
if (!len)
continue;...
}?
I think the way it is done in this patch is more readable than what you are proposing ;-)
Quoted 3 lines
Btw,
nboot_aclcan be 0, right? Perhaps check it first? (Or in other
words: which one is anticipated to be more frequent:nboot_acl= 0, or string w/o any ',' in it?)
If nboot_acl is 0 the sysfs attribute is not visible at all.
Quoted 11 lines
drivers/thunderbolt/domain.c static const char * const tb_security_names[] = { · line 195
Quoted 9 lines
drivers/thunderbolt/domain.c +7
-0+size_t len =strlen (s); + +if (len) { +if (len !=UUID_STRING_LEN ) { + ret = -EINVAL ; + goto err_free_acl; + }
uuid_parse()does this check. No need to duplicate.
It does not actually. Only thing it checks that the string is at least UUID_STRING_LEN. If the string is longer it just ignores the rest. We on the other side want to have strictly UUID_STRING_LEN strings.
Thanks for the comments :)
PATCH 17/18 thunderbolt: Introduce USB only (SL4) security level
In reply to Mika WesterbergThis new security level works so that it creates one PCIe tunnel to the connected Thunderbolt dock, removing PCIe links downstream of the dock. This leaves only the internal USB controller visible.
Display Port tunnels are created normally.
While there make sure security sysfs attribute returns "unknown" for any future security level.
--- Documentation/ABI/testing/sysfs-bus-thunderbolt | 3 +++ drivers/thunderbolt/domain.c | 7 ++++++- include/linux/thunderbolt.h | 4 ++++ 3 files changed, 13 insertions(+), 1 deletion(-)
Documentation/ABI/testing/sysfs-bus-thunderbolt +3 -0
@@ -35,6 +35,9 @@ Description: This attribute holds current Thunderbolt security level minimum. User needs to authorize each device. dponly: Automatically tunnel Display port (and USB). No PCIe tunnels are created. + usbonly: Automatically tunnel USB controller of the + connected Thunderbolt dock (and Display Port). All + PCIe links downstream of the dock are removed. What: /sys/bus/thunderbolt/devices/.../authorized Date: Sep 2017
drivers/thunderbolt/domain.c +6 -1
@@ -117,6 +117,7 @@ static const char * const tb_security_names[] = { [TB_SECURITY_USER] = "user", [TB_SECURITY_SECURE] = "secure", [TB_SECURITY_DPONLY] = "dponly", + [TB_SECURITY_USBONLY] = "usbonly", }; static ssize_t boot_acl_show(struct device *dev, struct device_attribute *attr,
@@ -226,8 +227,12 @@ static ssize_t security_show(struct device *dev, struct device_attribute *attr, char *buf) { struct tb *tb = container_of(dev, struct tb, dev); +const char *name ="unknown" ; -return sprintf (buf,"%s\n" , tb_security_names[tb->security_level]); +if (tb->security_level <ARRAY_SIZE (tb_security_names)) + name = tb_security_names[tb->security_level]; + +return sprintf (buf,"%s\n" , name); }static DEVICE_ATTR_RO (security);
include/linux/thunderbolt.h +4 -0
@@ -45,12 +45,16 @@ enum tb_cfg_pkg_type {* @TB_SECURITY_USER: User approval required at minimum * @TB_SECURITY_SECURE: One time saved key required at minimum * @TB_SECURITY_DPONLY: Only tunnel Display port (and USB) +* @TB_SECURITY_USBONLY: Only tunnel USB controller of the connected +* Thunderbolt dock (and Display Port). All PCIe +* links downstream of the dock are removed. */ enum tb_security_level {TB_SECURITY_NONE ,TB_SECURITY_USER ,TB_SECURITY_SECURE ,TB_SECURITY_DPONLY , +TB_SECURITY_USBONLY , };/**
Signature · 1 line
2.15.1
Re: [PATCH 17/18] thunderbolt: Introduce USB only (SL4) security level
Quoted 10 lines · Mika Westerberg
This new security level works so that it creates one PCIe tunnel to the connected Thunderbolt dock, removing PCIe links downstream of the dock. This leaves only the internal USB controller visible.
Display Port tunnels are created normally.
While there make sure security sysfs attribute returns "unknown" for any future security level.
Signed-off-by Mika Westerberg <[email protected]>
Hi,
Also update Documentation/admin-guide/thunderbolt.rst ??
Quoted 58 lines
--- Documentation/ABI/testing/sysfs-bus-thunderbolt | 3 +++ drivers/thunderbolt/domain.c | 7 ++++++- include/linux/thunderbolt.h | 4 ++++ 3 files changed, 13 insertions(+), 1 deletion(-)Documentation/ABI/testing/sysfs-bus-thunderbolt +3
-0@@ -35,6 +35,9 @@ Description: This attribute holds current Thunderbolt security level minimum. User needs to authorize each device. dponly: Automatically tunnel Display port (and USB). No PCIe tunnels are created. + usbonly: Automatically tunnel USB controller of the + connected Thunderbolt dock (and Display Port). All + PCIe links downstream of the dock are removed. What: /sys/bus/thunderbolt/devices/.../authorized Date: Sep 2017drivers/thunderbolt/domain.c +6
-1@@ -117,6 +117,7 @@ static const char * const tb_security_names[] = { [TB_SECURITY_USER] = "user", [TB_SECURITY_SECURE] = "secure", [TB_SECURITY_DPONLY] = "dponly", + [TB_SECURITY_USBONLY] = "usbonly", }; static ssize_t boot_acl_show(struct device *dev, struct device_attribute *attr,@@ -226,8 +227,12 @@ static ssize_t security_show(struct device *dev, struct device_attribute *attr, char *buf) { struct tb *tb = container_of(dev, struct tb, dev); +const char *name ="unknown" ; -return sprintf (buf,"%s\n" , tb_security_names[tb->security_level]); +if (tb->security_level <ARRAY_SIZE (tb_security_names)) + name = tb_security_names[tb->security_level]; + +return sprintf (buf,"%s\n" , name); }static DEVICE_ATTR_RO (security);include/linux/thunderbolt.h +3
-0@@ -45,12 +45,16 @@ enum tb_cfg_pkg_type {* @TB_SECURITY_USER: User approval required at minimum * @TB_SECURITY_SECURE: One time saved key required at minimum * @TB_SECURITY_DPONLY: Only tunnel Display port (and USB) +* @TB_SECURITY_USBONLY: Only tunnel USB controller of the connected +* Thunderbolt dock (and Display Port). All PCIe +* links downstream of the dock are removed.
include/linux/thunderbolt.h enum tb_cfg_pkg_type { · line 60
Quoted 10 lines
include/linux/thunderbolt.h +1
-0*/ enum tb_security_level {TB_SECURITY_NONE ,TB_SECURITY_USER ,TB_SECURITY_SECURE ,TB_SECURITY_DPONLY , +TB_SECURITY_USBONLY , };/**
thanks,
Signature · 1 line
~Randy
Re: [PATCH 17/18] thunderbolt: Introduce USB only (SL4) security level
Quoted 7 lines · Randy Dunlap
Quoted 6 lines · Mika Westerberg
This new security level works so that it creates one PCIe tunnel to the connected Thunderbolt dock, removing PCIe links downstream of the dock. This leaves only the internal USB controller visible.
Display Port tunnels are created normally.
Quoted 8 lines
Quoted 4 lines
While there make sure security sysfs attribute returns "unknown" for any future security level.
Signed-off-by Mika Westerberg <[email protected]>Hi,
Also update
Documentation/admin-guide/thunderbolt.rst??
Good point. I'll do that in v2.
PATCH 18/18 thunderbolt: Add support for Intel Titan Ridge
In reply to Mika WesterbergFrom: Radion Mirchevsky <[email protected]>
Intel Titan Ridge is the next Thunderbolt 3 controller. The ICM firmware message format in Titan Ridge differs from Falcon Ridge and Alpine Ridge somewhat because it is using route strings addressing devices. In addition to that the DMA port of 4-channel (two port) controller is in different port number than the previous controllers. There are some other minor differences as well.
This patch add support for Intel Titan Ridge and the new ICM firmware message format.
--- drivers/thunderbolt/dma_port.c | 28 ++- drivers/thunderbolt/icm.c | 379 +++++++++++++++++++++++++++++++++++++++++ drivers/thunderbolt/nhi.c | 2 + drivers/thunderbolt/nhi.h | 5 + drivers/thunderbolt/switch.c | 3 + drivers/thunderbolt/tb_msgs.h | 141 +++++++++++++++ 6 files changed, 543 insertions(+), 15 deletions(-)
drivers/thunderbolt/dma_port.c +13 -15
@@ -170,24 +170,22 @@ static int dma_port_write(struct tb_ctl *ctl, const void *buffer, u64 route,static int dma_find_port (struct tb_switch *sw) { -int port, ret; -u32 type; +static const int ports[] = {7 ,5 ,3 }; +int i;/* -* The DMA (NHI) port is either 3 or 5 depending on the -* controller. Try both starting from 5 which is more common. +* The DMA (NHI) port is either 3, 5 or 7 depending on the +* controller. Try all of them. */ - port =5 ; - ret =dma_port_read (sw->tb->ctl, &type,tb_route (sw), port,2 ,1 , -DMA_PORT_TIMEOUT ); -if (!ret && (type &0xffffff ) ==TB_TYPE_NHI ) -return port; - - port =3 ; - ret =dma_port_read (sw->tb->ctl, &type,tb_route (sw), port,2 ,1 , -DMA_PORT_TIMEOUT ); -if (!ret && (type &0xffffff ) ==TB_TYPE_NHI ) -return port; +for (i =0 ; i <ARRAY_SIZE (ports); i++) { +u32 type; +int ret; + + ret =dma_port_read (sw->tb->ctl, &type,tb_route (sw), ports[i], +2 ,1 ,DMA_PORT_TIMEOUT ); +if (!ret && (type &0xffffff ) ==TB_TYPE_NHI ) +return ports[i]; + }return -ENODEV ; }
drivers/thunderbolt/icm.c +379 -0
403 lines not on this page · raw message
drivers/thunderbolt/nhi.c +2 -0
@@ -1111,6 +1111,8 @@ static struct pci_device_id nhi_ids[] = { { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_2C_NHI) }, { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_NHI) }, { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_USBONLY_NHI) }, + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_NHI) }, + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_NHI) }, { 0,} };
drivers/thunderbolt/nhi.h +5 -0
@@ -45,5 +45,10 @@ enum nhi_fw_mode nhi_mailbox_mode(struct tb_nhi *nhi);#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_USBONLY_NHI 0x15dc#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_USBONLY_NHI 0x15dd#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_USBONLY_NHI 0x15de +#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_NHI 0x15e8 +#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_BRIDGE 0x15e7 +#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_NHI 0x15eb +#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_BRIDGE 0x15ea +#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_DD_BRIDGE 0x15ef#endif
drivers/thunderbolt/switch.c +3 -0
@@ -1051,6 +1051,9 @@ static int tb_switch_get_generation(struct tb_switch *sw)case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_4C_BRIDGE :case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_2C_BRIDGE :case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_BRIDGE : +case PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_BRIDGE : +case PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_BRIDGE : +case PCI_DEVICE_ID_INTEL_TITAN_RIDGE_DD_BRIDGE :return 3 ;default :
drivers/thunderbolt/tb_msgs.h +141 -0
153 lines not on this page · raw message
Signature · 1 line
2.15.1
Quoted 7 lines · Mika Westerberg
Intel Titan Ridge is the next Thunderbolt 3 controller. The ICM firmware message format in Titan Ridge differs from Falcon Ridge and Alpine Ridge somewhat because it is using route strings addressing devices. In addition to that the DMA port of 4-channel (two port) controller is in different port number than the previous controllers. There are some other minor differences as well.
Quoted 6 lines
This patch add support for Intel Titan Ridge and the new ICM firmware message format.
static int
dma_find_port(struct tb_switch *sw)
{
+ static const int ports[] = { 7, 5, 3 };
Is it anything special in ordering? Otherwise I would keep it the same as in comment below.
Quoted 10 lines
drivers/thunderbolt/dma_port.c +7
-0+int i;/* +* The DMA (NHI) port is either 3, 5 or 7 depending on the +* controller. Try all of them. */ +for (i =0 ; i <ARRAY_SIZE (ports); i++) { +u32 type; +int ret; +
drivers/thunderbolt/dma_port.c static int dma_port_write(struct tb_ctl *ctl, const void *buffer, u64 route,
Quoted 10 lines
drivers/thunderbolt/dma_port.c +9
-0+ ret =dma_port_read (sw->tb->ctl, &type,tb_route (sw), ports[i], +2 ,1 ,DMA_PORT_TIMEOUT ); +if (!ret && (type &0xffffff ) ==TB_TYPE_NHI ) +return ports[i]; + } +static inline u64 get_parent_route (u64 route) +{ +int depth =tb_route_length (route); +return depth ? route & ~((u64 )0xff << (depth -1 ) *TB_ROUTE_SHIFT ) :0 ;
0xffULL ?
Quoted 14 lines
(quoted diff) +9
-0+} + const struct icm_tr_event_device_connected *pkg = + (const struct icm_tr_event_device_connected *)hdr; + const struct icm_tr_event_device_disconnected *pkg = + (const struct icm_tr_event_device_disconnected *)hdr; + const struct icm_tr_event_xdomain_connected *pkg = + (const struct icm_tr_event_xdomain_connected *)hdr; + const struct icm_tr_event_xdomain_disconnected *pkg = + (const struct icm_tr_event_xdomain_disconnected *)hdr;
Review
Quoted 10 lines
(quoted diff) +5
-0#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_USBONLY_NHI 0x15dc #define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_USBONLY_NHI 0x15dd #define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_USBONLY_NHI 0x15de +#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_NHI 0x15e8 +#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_BRIDGE 0x15e7 +#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_NHI 0x15eb +#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_BRIDGE 0x15ea +#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_DD_BRIDGE 0x15ef
Can we keep it sorted?
Quoted 1 line
+#define
ICM_TR_INFO_SLEVEL_MASK0x7
GENMASK() ?
Quoted 2 lines
drivers/thunderbolt/tb_msgs.h +2
-0+#define ICM_TR_INFO_BOOT_ACL_SHIFT 7 +#define ICM_TR_INFO_BOOT_ACL_MASK GENMASK(12, 7)
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 13 lines · Andy Shevchenko
Quoted 9 lines · Mika Westerberg
Intel Titan Ridge is the next Thunderbolt 3 controller. The ICM firmware message format in Titan Ridge differs from Falcon Ridge and Alpine Ridge somewhat because it is using route strings addressing devices. In addition to that the DMA port of 4-channel (two port) controller is in different port number than the previous controllers. There are some other minor differences as well.
This patch add support for Intel Titan Ridge and the new ICM firmware message format.
Quoted 6 lines
Quoted 3 lines
static int
dma_find_port(struct tb_switch *sw)
{
+ static const int ports[] = { 7, 5, 3 };Is it anything special in ordering? Otherwise I would keep it the same as in comment below.
I don't think there is anything special in ordering so I can update it accordingly.
Quoted 42 lines
Quoted 10 lines
drivers/thunderbolt/dma_port.c +7
-0+int i;/* +* The DMA (NHI) port is either 3, 5 or 7 depending on the +* controller. Try all of them. */ +for (i =0 ; i <ARRAY_SIZE (ports); i++) { +u32 type; +int ret; +drivers/thunderbolt/dma_port.c static int dma_port_write(struct tb_ctl *ctl, const void *buffer, u64 route,
Quoted 10 lines
drivers/thunderbolt/dma_port.c +9
-0+ ret =dma_port_read (sw->tb->ctl, &type,tb_route (sw), ports[i], +2 ,1 ,DMA_PORT_TIMEOUT ); +if (!ret && (type &0xffffff ) ==TB_TYPE_NHI ) +return ports[i]; + } +static inline u64 get_parent_route (u64 route) +{ +int depth =tb_route_length (route); +return depth ? route & ~((u64 )0xff << (depth -1 ) *TB_ROUTE_SHIFT ) :0 ;
0xffULL?Quoted 18 lines
(quoted diff) +9
-0+} + const struct icm_tr_event_device_connected *pkg = + (const struct icm_tr_event_device_connected *)hdr; + const struct icm_tr_event_device_disconnected *pkg = + (const struct icm_tr_event_device_disconnected *)hdr; + const struct icm_tr_event_xdomain_connected *pkg = + (const struct icm_tr_event_xdomain_connected *)hdr; + const struct icm_tr_event_xdomain_disconnected *pkg = + (const struct icm_tr_event_xdomain_disconnected *)hdr; #define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_USBONLY_NHI 0x15dc #define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_USBONLY_NHI 0x15dd #define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_USBONLY_NHI 0x15de
Quoted 7 lines
drivers/thunderbolt/nhi.h enum nhi_fw_mode nhi_mailbox_mode(struct tb_nhi *nhi); · line 52
Quoted 5 lines
drivers/thunderbolt/nhi.h +5
-0+#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_NHI 0x15e8 +#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_BRIDGE 0x15e7 +#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_NHI 0x15eb +#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_BRIDGE 0x15ea +#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_DD_BRIDGE 0x15efCan we keep it sorted?
It is sorted by the controller type ;-)
Quoted 4 lines
Quoted 2 lines
+#define
ICM_TR_INFO_SLEVEL_MASK0x7
GENMASK()?
OK.
Quoted 6 lines
Quoted 2 lines
drivers/thunderbolt/tb_msgs.h +2
-0+#define ICM_TR_INFO_BOOT_ACL_SHIFT 7 +#define ICM_TR_INFO_BOOT_ACL_MASK GENMASK(12, 7)Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 10 lines · Mika Westerberg
Quoted 9 lines · Andy Shevchenko
drivers/thunderbolt/icm.c static inline u64 get_route(u32 route_hi, u32 route_lo) · line 124
Quoted 4 lines · Mika Westerberg
drivers/thunderbolt/icm.c +4
-0+static inline u64 get_parent_route (u64 route) +{ +int depth =tb_route_length (route); +return depth ? route & ~((u64 )0xff << (depth -1 ) *TB_ROUTE_SHIFT ) :0 ;
0xffULL?
Agreed or not?
Quoted 12 lines
Quoted 11 lines
drivers/thunderbolt/nhi.h enum nhi_fw_mode nhi_mailbox_mode(struct tb_nhi *nhi); · line 52
Quoted 9 lines
#define
PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_USBONLY_NHI0x15dc
#definePCI_DEVICE_ID_INTEL_ALPINE_RIDGE_USBONLY_NHI0x15dd
#definePCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_USBONLY_NHI0x15dedrivers/thunderbolt/nhi.h +5
-0+#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_NHI 0x15e8 +#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_BRIDGE 0x15e7 +#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_NHI 0x15eb +#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_BRIDGE 0x15ea +#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_DD_BRIDGE 0x15efCan we keep it sorted?
Quoted 1 line
It is sorted by the controller type ;-)
Yes, this is not what I'm talking about. Inside the group you can easily keep it sorted.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 6 lines · Andy Shevchenko
Quoted 3 lines · Mika Westerberg
Quoted 2 lines · Andy Shevchenko
On Tue, Feb 13, 2018 at 7:00 PM, Mika Westerberg
<[email protected]> wrote:
Quoted 8 lines
Quoted 6 lines
Quoted 6 lines
drivers/thunderbolt/icm.c static inline u64 get_route(u32 route_hi, u32 route_lo) · line 124
Quoted 4 lines
drivers/thunderbolt/icm.c +4
-0+static inline u64 get_parent_route (u64 route) +{ +int depth =tb_route_length (route); +return depth ? route & ~((u64 )0xff << (depth -1 ) *TB_ROUTE_SHIFT ) :0 ;
0xffULL?Agreed or not?
Agreed :)
Quoted 12 lines
Quoted 12 lines
Quoted 11 lines
drivers/thunderbolt/nhi.h enum nhi_fw_mode nhi_mailbox_mode(struct tb_nhi *nhi); · line 52
Quoted 9 lines
#define
PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_USBONLY_NHI0x15dc
#definePCI_DEVICE_ID_INTEL_ALPINE_RIDGE_USBONLY_NHI0x15dd
#definePCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_USBONLY_NHI0x15dedrivers/thunderbolt/nhi.h +5
-0+#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_NHI 0x15e8 +#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_BRIDGE 0x15e7 +#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_NHI 0x15eb +#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_BRIDGE 0x15ea +#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_DD_BRIDGE 0x15efCan we keep it sorted?
Quoted 4 lines
Quoted 1 line
It is sorted by the controller type ;-)
Yes, this is not what I'm talking about. Inside the group you can easily keep it sorted.
OK, that works for me.
Quoted 13 lines · Mika Westerberg
Hi,
This series adds support for Intel Titan Ridge Thunderbolt controller. Titan Ridge is the next generation Thunderbolt 3 controller and successor of Alpine Ridge.
In addition to fixes and Titan Ridge support this series adds following:
- USB only security level (SL4).
- A new attribute for devices telling whether they were connected automatically during boot.
Quoted 7 lines
- Preboot ACL allows userspace to specify a list of devices (based on device
unique_id) that the firmware automatically connects during boot.Bjorn, I've Cc'd you the series as well because I would like to get your blessing for patch [2/18] as it uses functions from PCI subsystem.
Thanks!
For patches 1-17,
The last one will take a bit more time to go through.
Quoted 41 lines
Mika Westerberg (13):
thunderbolt: Resume control channel after hibernation image is created
thunderbolt: Serialize PCIe tunnel creation with PCI rescan
thunderbolt: Handle connecting device in place of host properly
thunderbolt: Do not overwrite error code when domain adding fails
thunderbolt: Wait a bit longer for root switch config space
thunderbolt: Wait a bit longer for ICM to authenticate the active NVM
thunderbolt: Handle rejected Thunderbolt devices
thunderbolt: Factor common ICM add and update operations out
thunderbolt: Addtb_switch_get()
thunderbolt: Add constant for approval timeout
thunderbolt: Move driver ready handling to struct icm
thunderbolt: Add support for preboot ACL
thunderbolt: Introduce USB only (SL4) security levelRadion Mirchevsky (4):
thunderbolt: Correct function name in kernel-doc comment
thunderbolt: Addtb_switch_find_by_route()
thunderbolt: Addtb_xdomain_find_by_route()
thunderbolt: Add support for Intel Titan RidgeYehezkel Bernat (1):
thunderbolt: Add 'boot' attribute for devicesDocumentation/ABI/testing/sysfs-bus-thunderbolt | 33 ++ drivers/thunderbolt/dma_port.c | 28 +- drivers/thunderbolt/domain.c | 129 +++- drivers/thunderbolt/icm.c | 757 +++++++++++++++++++++--- drivers/thunderbolt/nhi.c | 5 +- drivers/thunderbolt/nhi.h | 5 + drivers/thunderbolt/switch.c | 61 +- drivers/thunderbolt/tb.h | 14 + drivers/thunderbolt/tb_msgs.h | 180 +++++- drivers/thunderbolt/xdomain.c | 40 +- include/linux/thunderbolt.h | 19 + 11 files changed, 1172 insertions(+), 99 deletions(-)--
2.15.1
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 18 lines · Andy Shevchenko
Quoted 16 lines · Mika Westerberg
Hi,
This series adds support for Intel Titan Ridge Thunderbolt controller. Titan Ridge is the next generation Thunderbolt 3 controller and successor of Alpine Ridge.
In addition to fixes and Titan Ridge support this series adds following:
- USB only security level (SL4).
- A new attribute for devices telling whether they were connected automatically during boot.
- Preboot ACL allows userspace to specify a list of devices (based on device
unique_id) that the firmware automatically connects during boot.
Quoted 8 lines
Quoted 4 lines
Bjorn, I've Cc'd you the series as well because I would like to get your blessing for patch [2/18] as it uses functions from PCI subsystem.
Thanks!
For patches 1-17,
Reviewed-by Andy Shevchenko <[email protected]>
Thanks!