thunderbolt: Don't display retimers unless nvm was initialized
Subsystems (2)
PATCH 1/1 thunderbolt: Don't display retimers unless nvm was initialized
From: Mario Limonciello <[email protected]>
The read will never succeed if nvm wasn't initialized.
--- drivers/thunderbolt/retimer.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)
drivers/thunderbolt/retimer.c +14 -3
@@ -321,9 +321,7 @@ static ssize_t nvm_version_show(struct device *dev,if (!mutex_trylock (&rt->tb->lock))return restart_syscall (); -if (!rt->nvm) - ret = -EAGAIN ; -else if (rt->no_nvm_upgrade) +if (rt->no_nvm_upgrade) ret = -EOPNOTSUPP ;else ret =sysfs_emit (buf,"%x.%x\n" , rt->nvm->major, rt->nvm->minor);
@@ -342,6 +340,18 @@ static ssize_t vendor_show(struct device *dev, struct device_attribute *attr, }static DEVICE_ATTR_RO (vendor); +static umode_t retimer_is_visible (struct kobject *kobj, +struct attribute *attr,int n) +{ +struct device *dev =kobj_to_dev (kobj); +struct tb_retimer *rt =tb_to_retimer (dev); + +if (!rt->nvm) +return 0 ; +return attr->mode; + +} +static struct attribute *retimer_attrs[] = { &dev_attr_device.attr, &dev_attr_nvm_authenticate.attr,
@@ -351,6 +361,7 @@ static struct attribute *retimer_attrs[] = { };static const struct attribute_group retimer_group = { + .is_visible = retimer_is_visible, .attrs = retimer_attrs, };
Signature · 1 line
2.43.0
Hi Mario,
Quoted 3 lines · Mario Limonciello
From: Mario Limonciello <[email protected]>
The read will never succeed if nvm wasn't initialized.
Okay but we would need to understand why it was not initialized in the first place?
I see this is ThinkPad Thunderbolt 4 Dock so probably Intel hardware? You say you can reproduce this too so can you send me full dmesg with thunderbolt dynamic debugging enabled? I would like to understand this bit more deeper before we add any workarounds.
Quoted 51 lines
Reported-by Richard Hughes <[email protected]>Signed-off-by Mario Limonciello <[email protected]>--- drivers/thunderbolt/retimer.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)drivers/thunderbolt/retimer.c +14
-3@@ -321,9 +321,7 @@ static ssize_t nvm_version_show(struct device *dev,if (!mutex_trylock (&rt->tb->lock))return restart_syscall (); -if (!rt->nvm) - ret = -EAGAIN ; -else if (rt->no_nvm_upgrade) +if (rt->no_nvm_upgrade) ret = -EOPNOTSUPP ;else ret =sysfs_emit (buf,"%x.%x\n" , rt->nvm->major, rt->nvm->minor);@@ -342,6 +340,18 @@ static ssize_t vendor_show(struct device *dev, struct device_attribute *attr, }static DEVICE_ATTR_RO (vendor); +static umode_t retimer_is_visible (struct kobject *kobj, +struct attribute *attr,int n) +{ +struct device *dev =kobj_to_dev (kobj); +struct tb_retimer *rt =tb_to_retimer (dev); + +if (!rt->nvm) +return 0 ; +return attr->mode; + +} +static struct attribute *retimer_attrs[] = { &dev_attr_device.attr, &dev_attr_nvm_authenticate.attr,@@ -351,6 +361,7 @@ static struct attribute *retimer_attrs[] = { };static const struct attribute_group retimer_group = { + .is_visible = retimer_is_visible, .attrs = retimer_attrs, };Signature · 1 line
2.43.0
Quoted 9 lines · Mika Westerberg
Hi Mario,
Quoted 3 lines · Mario Limonciello
From: Mario Limonciello <[email protected]>
The read will never succeed if nvm wasn't initialized.
Okay but we would need to understand why it was not initialized in the first place?
Oh sorry I should have included that/
https://gist.github.com/superm1/c3763840fefa54298258a6fbec399007
As you can see it's an unknown retimer NVM format. So this ends up down the path of "NVM upgrade disabled". So that's why I'm thinking the visibility is the right move to adjust here (IE this patch).
Quoted 32 lines
I see this is ThinkPad Thunderbolt 4 Dock so probably Intel hardware? You say you can reproduce this too so can you send me full dmesg with thunderbolt dynamic debugging enabled? I would like to understand this bit more deeper before we add any workarounds.
Quoted 26 lines
Reported-by Richard Hughes <[email protected]>Signed-off-by Mario Limonciello <[email protected]>--- drivers/thunderbolt/retimer.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)drivers/thunderbolt/retimer.c +1
-3@@ -321,9 +321,7 @@ static ssize_t nvm_version_show(struct device *dev,if (!mutex_trylock (&rt->tb->lock))return restart_syscall (); -if (!rt->nvm) - ret = -EAGAIN ; -else if (rt->no_nvm_upgrade) +if (rt->no_nvm_upgrade) ret = -EOPNOTSUPP ;else ret =sysfs_emit (buf,"%x.%x\n" , rt->nvm->major, rt->nvm->minor);@@ -342,6 +340,18 @@ static ssize_t vendor_show(struct device *dev, struct device_attribute *attr, }static DEVICE_ATTR_RO (vendor);
Quoted 11 lines
drivers/thunderbolt/retimer.c static ssize_t vendor_show(struct device *dev, struct device_attribute *attr, · line 353
Quoted 11 lines
drivers/thunderbolt/retimer.c +11
-0+static umode_t retimer_is_visible (struct kobject *kobj, +struct attribute *attr,int n) +{ +struct device *dev =kobj_to_dev (kobj); +struct tb_retimer *rt =tb_to_retimer (dev); + +if (!rt->nvm) +return 0 ; +return attr->mode; + +}
I just noticed I had a spurious newline here. If we end up taking this patch would you mind just fixing it up? If there is other feedback I'll fix it on a v2.
Quoted 14 lines
Quoted 14 lines
drivers/thunderbolt/retimer.c +2
-0+static struct attribute *retimer_attrs[] = { &dev_attr_device.attr, &dev_attr_nvm_authenticate.attr,@@ -351,6 +361,7 @@ static struct attribute *retimer_attrs[] = { };static const struct attribute_group retimer_group = { + .is_visible = retimer_is_visible, .attrs = retimer_attrs, };Signature · 1 line
2.43.0
Quoted 11 lines · Mario Limonciello
Quoted 9 lines · Mika Westerberg
Hi Mario,
Quoted 3 lines · Mario Limonciello
From: Mario Limonciello <[email protected]>
The read will never succeed if nvm wasn't initialized.
Okay but we would need to understand why it was not initialized in the first place?
Quoted 7 lines
Oh sorry I should have included that/
https://gist.github.com/superm1/c3763840fefa54298258a6fbec399007
As you can see it's an unknown retimer NVM format. So this ends up down the path of "NVM upgrade disabled". So that's why I'm thinking the visibility is the right move to adjust here (IE this patch).
This is actually on-board retimer of the AMD platform:
Dec 09 07:29:11 fedora kernel: thunderbolt 0-0:2.1: retimer NVM format of vendor 0x7fea unknown Dec 09 07:29:11 fedora kernel: thunderbolt 0-0:2.1: NVM upgrade disabled Dec 09 07:29:11 fedora kernel: thunderbolt 0-0:2.1: new retimer found, vendor=0x7fea device=0x1032
I would think you guys want to make it upgradeable as well, no?
Quoted 12 lines
Quoted 12 lines
I see this is ThinkPad Thunderbolt 4 Dock so probably Intel hardware? You say you can reproduce this too so can you send me full dmesg with thunderbolt dynamic debugging enabled? I would like to understand this bit more deeper before we add any workarounds.
Quoted 7 lines
Reported-by Richard Hughes <[email protected]>Signed-off-by Mario Limonciello <[email protected]>--- drivers/thunderbolt/retimer.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)
Quoted 9 lines
Quoted 9 lines
drivers/thunderbolt/retimer.c static ssize_t nvm_version_show(struct device *dev, · line 324
Quoted 9 lines
drivers/thunderbolt/retimer.c +0
-2@@ -321,9 +321,7 @@ static ssize_t nvm_version_show(struct device *dev,if (!mutex_trylock (&rt->tb->lock))return restart_syscall (); -if (!rt->nvm) - ret = -EAGAIN ;
This is actually here because it might take some time for the NVM to be available after the upgrade so changing this may cause issues on its own.
Instead we should check first the
rt->no_nvm_upgrade
and return -EOPNOTSUPP which I believe fwupd handles?
Quoted 36 lines
Quoted 11 lines
Quoted 11 lines
drivers/thunderbolt/retimer.c +4
-1-else if (rt->no_nvm_upgrade) +if (rt->no_nvm_upgrade) ret = -EOPNOTSUPP ;else ret =sysfs_emit (buf,"%x.%x\n" , rt->nvm->major, rt->nvm->minor);@@ -342,6 +340,18 @@ static ssize_t vendor_show(struct device *dev, struct device_attribute *attr, }static DEVICE_ATTR_RO (vendor); +static umode_t retimer_is_visible (struct kobject *kobj, +struct attribute *attr,int n) +{Quoted 8 lines
drivers/thunderbolt/retimer.c static ssize_t vendor_show(struct device *dev, struct device_attribute *attr, · line 353
Quoted 8 lines
drivers/thunderbolt/retimer.c +8
-0+struct device *dev =kobj_to_dev (kobj); +struct tb_retimer *rt =tb_to_retimer (dev); + +if (!rt->nvm) +return 0 ; +return attr->mode; + +}I just noticed I had a spurious newline here. If we end up taking this patch would you mind just fixing it up? If there is other feedback I'll fix it on a v2.
Quoted 12 lines
Quoted 12 lines
drivers/thunderbolt/retimer.c +2
-0+static struct attribute *retimer_attrs[] = { &dev_attr_device.attr, &dev_attr_nvm_authenticate.attr,@@ -351,6 +361,7 @@ static struct attribute *retimer_attrs[] = { };static const struct attribute_group retimer_group = { + .is_visible = retimer_is_visible, .attrs = retimer_attrs, };Signature · 1 line
2.43.0
Quoted 14 lines · Mika Westerberg
Quoted 13 lines · Mario Limonciello
Quoted 9 lines · Mika Westerberg
Hi Mario,
Quoted 3 lines · Mario Limonciello
From: Mario Limonciello <[email protected]>
The read will never succeed if nvm wasn't initialized.
Okay but we would need to understand why it was not initialized in the first place?
Oh sorry I should have included that/
Quoted 7 lines
Quoted 5 lines
https://gist.github.com/superm1/c3763840fefa54298258a6fbec399007
As you can see it's an unknown retimer NVM format. So this ends up down the path of "NVM upgrade disabled". So that's why I'm thinking the visibility is the right move to adjust here (IE this patch).
This is actually on-board retimer of the AMD platform:
Oh, good point.
Quoted 6 lines
Dec 09 07:29:11 fedora kernel: thunderbolt 0-0:2.1: retimer NVM format of vendor 0x7fea unknown Dec 09 07:29:11 fedora kernel: thunderbolt 0-0:2.1: NVM upgrade disabled Dec 09 07:29:11 fedora kernel: thunderbolt 0-0:2.1: new retimer found, vendor=0x7fea device=0x1032I would think you guys want to make it upgradeable as well, no?
For AMD platforms retimers are nominally upgraded by the platform's BIOS upgrade, there haven't been asks from anyone to upgrade in AFAIK OS (Windows or Linux).
Quoted 26 lines
Quoted 13 lines
Quoted 13 lines
I see this is ThinkPad Thunderbolt 4 Dock so probably Intel hardware? You say you can reproduce this too so can you send me full dmesg with thunderbolt dynamic debugging enabled? I would like to understand this bit more deeper before we add any workarounds.
Quoted 7 lines
Reported-by Richard Hughes <[email protected]>Signed-off-by Mario Limonciello <[email protected]>--- drivers/thunderbolt/retimer.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)Quoted 9 lines
Quoted 9 lines
drivers/thunderbolt/retimer.c static ssize_t nvm_version_show(struct device *dev, · line 324
Quoted 9 lines
drivers/thunderbolt/retimer.c +0
-2@@ -321,9 +321,7 @@ static ssize_t nvm_version_show(struct device *dev,if (!mutex_trylock (&rt->tb->lock))return restart_syscall (); -if (!rt->nvm) - ret = -EAGAIN ;This is actually here because it might take some time for the NVM to be available after the upgrade so changing this may cause issues on its own.
Quoted 6 lines
Instead we should check first the
rt->no_nvm_upgradeand return
-EOPNOTSUPPwhich I believe fwupd handles?
Well I don't think it's right to export the sysfs file in the first place if we "know" it's not going to work. That's disingenuous to software.
How about looking for rt->no_nvm_upgrade in the new retimer_is_visible?
I think it should get the same intent and not break this retry logic.
Quoted 36 lines
Quoted 36 lines
Quoted 11 lines
Quoted 11 lines
drivers/thunderbolt/retimer.c +4
-1-else if (rt->no_nvm_upgrade) +if (rt->no_nvm_upgrade) ret = -EOPNOTSUPP ;else ret =sysfs_emit (buf,"%x.%x\n" , rt->nvm->major, rt->nvm->minor);@@ -342,6 +340,18 @@ static ssize_t vendor_show(struct device *dev, struct device_attribute *attr, }static DEVICE_ATTR_RO (vendor); +static umode_t retimer_is_visible (struct kobject *kobj, +struct attribute *attr,int n) +{Quoted 8 lines
drivers/thunderbolt/retimer.c static ssize_t vendor_show(struct device *dev, struct device_attribute *attr, · line 353
Quoted 8 lines
drivers/thunderbolt/retimer.c +8
-0+struct device *dev =kobj_to_dev (kobj); +struct tb_retimer *rt =tb_to_retimer (dev); + +if (!rt->nvm) +return 0 ; +return attr->mode; + +}I just noticed I had a spurious newline here. If we end up taking this patch would you mind just fixing it up? If there is other feedback I'll fix it on a v2.
Quoted 12 lines
Quoted 12 lines
drivers/thunderbolt/retimer.c +2
-0+static struct attribute *retimer_attrs[] = { &dev_attr_device.attr, &dev_attr_nvm_authenticate.attr,@@ -351,6 +361,7 @@ static struct attribute *retimer_attrs[] = { };static const struct attribute_group retimer_group = { + .is_visible = retimer_is_visible, .attrs = retimer_attrs, };Signature · 1 line
2.43.0
Quoted 30 lines · Mario Limonciello
Quoted 14 lines · Mika Westerberg
Quoted 13 lines · Mario Limonciello
Quoted 9 lines · Mika Westerberg
Hi Mario,
Quoted 3 lines · Mario Limonciello
From: Mario Limonciello <[email protected]>
The read will never succeed if nvm wasn't initialized.
Okay but we would need to understand why it was not initialized in the first place?
Oh sorry I should have included that/
Quoted 7 lines
Quoted 5 lines
https://gist.github.com/superm1/c3763840fefa54298258a6fbec399007
As you can see it's an unknown retimer NVM format. So this ends up down the path of "NVM upgrade disabled". So that's why I'm thinking the visibility is the right move to adjust here (IE this patch).
This is actually on-board retimer of the AMD platform:
Oh, good point.
Quoted 5 lines
Dec 09 07:29:11 fedora kernel: thunderbolt 0-0:2.1: retimer NVM format of vendor 0x7fea unknown Dec 09 07:29:11 fedora kernel: thunderbolt 0-0:2.1: NVM upgrade disabled Dec 09 07:29:11 fedora kernel: thunderbolt 0-0:2.1: new retimer found, vendor=0x7fea device=0x1032
Quoted 5 lines
Quoted 1 line
I would think you guys want to make it upgradeable as well, no?
For AMD platforms retimers are nominally upgraded by the platform's BIOS upgrade, there haven't been asks from anyone to upgrade in AFAIK OS (Windows or Linux).
Right, until Chrome wants it and also a way to do that with no active connection ;-)
Quoted 32 lines
Quoted 25 lines
Quoted 12 lines
Quoted 12 lines
I see this is ThinkPad Thunderbolt 4 Dock so probably Intel hardware? You say you can reproduce this too so can you send me full dmesg with thunderbolt dynamic debugging enabled? I would like to understand this bit more deeper before we add any workarounds.
Quoted 7 lines
Reported-by Richard Hughes <[email protected]>Signed-off-by Mario Limonciello <[email protected]>--- drivers/thunderbolt/retimer.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)Quoted 9 lines
Quoted 9 lines
drivers/thunderbolt/retimer.c static ssize_t nvm_version_show(struct device *dev, · line 324
Quoted 9 lines
drivers/thunderbolt/retimer.c +0
-2@@ -321,9 +321,7 @@ static ssize_t nvm_version_show(struct device *dev,if (!mutex_trylock (&rt->tb->lock))return restart_syscall (); -if (!rt->nvm) - ret = -EAGAIN ;This is actually here because it might take some time for the NVM to be available after the upgrade so changing this may cause issues on its own.
Quoted 6 lines
Instead we should check first the
rt->no_nvm_upgradeand return
-EOPNOTSUPPwhich I believe fwupd handles?
Quoted 6 lines
Well I don't think it's right to export the sysfs file in the first place if we "know" it's not going to work. That's disingenuous to software.
How about looking for
rt->no_nvm_upgradein the newretimer_is_visible?I think it should get the same intent and not break this retry logic.
Yeah, that sounds good to me.
Quoted 37 lines
Quoted 37 lines
Quoted 37 lines
Quoted 12 lines
Quoted 12 lines
drivers/thunderbolt/retimer.c +4
-1-else if (rt->no_nvm_upgrade) +if (rt->no_nvm_upgrade) ret = -EOPNOTSUPP ;else ret =sysfs_emit (buf,"%x.%x\n" , rt->nvm->major, rt->nvm->minor);@@ -342,6 +340,18 @@ static ssize_t vendor_show(struct device *dev, struct device_attribute *attr, }static DEVICE_ATTR_RO (vendor); +static umode_t retimer_is_visible (struct kobject *kobj, +struct attribute *attr,int n) +{Quoted 8 lines
drivers/thunderbolt/retimer.c static ssize_t vendor_show(struct device *dev, struct device_attribute *attr, · line 353
Quoted 8 lines
drivers/thunderbolt/retimer.c +8
-0+struct device *dev =kobj_to_dev (kobj); +struct tb_retimer *rt =tb_to_retimer (dev); + +if (!rt->nvm) +return 0 ; +return attr->mode; + +}I just noticed I had a spurious newline here. If we end up taking this patch would you mind just fixing it up? If there is other feedback I'll fix it on a v2.
Quoted 12 lines
Quoted 12 lines
drivers/thunderbolt/retimer.c +2
-0+static struct attribute *retimer_attrs[] = { &dev_attr_device.attr, &dev_attr_nvm_authenticate.attr,@@ -351,6 +361,7 @@ static struct attribute *retimer_attrs[] = { };static const struct attribute_group retimer_group = { + .is_visible = retimer_is_visible, .attrs = retimer_attrs, };Signature · 1 line
2.43.0