| Message ID | 20241206183318.1701180-1-superm1@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | thunderbolt: Don't display retimers unless nvm was initialized | expand |
Hi Mario, On Fri, Dec 06, 2024 at 12:33:18PM -0600, Mario Limonciello wrote: > From: Mario Limonciello <mario.limonciello@amd.com> > > 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. > Reported-by: Richard Hughes <hughsient@gmail.com> > Closes: https://github.com/fwupd/fwupd/issues/8200 > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > drivers/thunderbolt/retimer.c | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/drivers/thunderbolt/retimer.c b/drivers/thunderbolt/retimer.c > index 89d2919d0193e..7be435aee7217 100644 > --- a/drivers/thunderbolt/retimer.c > +++ b/drivers/thunderbolt/retimer.c > @@ -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, > }; > > -- > 2.43.0
On 12/9/2024 00:24, Mika Westerberg wrote: > Hi Mario, > > On Fri, Dec 06, 2024 at 12:33:18PM -0600, Mario Limonciello wrote: >> From: Mario Limonciello <mario.limonciello@amd.com> >> >> 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). > > 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. > >> Reported-by: Richard Hughes <hughsient@gmail.com> >> Closes: https://github.com/fwupd/fwupd/issues/8200 >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >> --- >> drivers/thunderbolt/retimer.c | 17 ++++++++++++++--- >> 1 file changed, 14 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/thunderbolt/retimer.c b/drivers/thunderbolt/retimer.c >> index 89d2919d0193e..7be435aee7217 100644 >> --- a/drivers/thunderbolt/retimer.c >> +++ b/drivers/thunderbolt/retimer.c >> @@ -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; >> + >> +} 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. >> + >> 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, >> }; >> >> -- >> 2.43.0
On Mon, Dec 09, 2024 at 08:15:16AM -0600, Mario Limonciello wrote: > On 12/9/2024 00:24, Mika Westerberg wrote: > > Hi Mario, > > > > On Fri, Dec 06, 2024 at 12:33:18PM -0600, Mario Limonciello wrote: > > > From: Mario Limonciello <mario.limonciello@amd.com> > > > > > > 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). 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? > > 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. > > > > > Reported-by: Richard Hughes <hughsient@gmail.com> > > > Closes: https://github.com/fwupd/fwupd/issues/8200 > > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > > > --- > > > drivers/thunderbolt/retimer.c | 17 ++++++++++++++--- > > > 1 file changed, 14 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/thunderbolt/retimer.c b/drivers/thunderbolt/retimer.c > > > index 89d2919d0193e..7be435aee7217 100644 > > > --- a/drivers/thunderbolt/retimer.c > > > +++ b/drivers/thunderbolt/retimer.c > > > @@ -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? > > > - 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; > > > + > > > +} > > 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. > > > > + > > > 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, > > > }; > > > -- > > > 2.43.0
On 12/9/2024 09:40, Mika Westerberg wrote: > On Mon, Dec 09, 2024 at 08:15:16AM -0600, Mario Limonciello wrote: >> On 12/9/2024 00:24, Mika Westerberg wrote: >>> Hi Mario, >>> >>> On Fri, Dec 06, 2024 at 12:33:18PM -0600, Mario Limonciello wrote: >>>> From: Mario Limonciello <mario.limonciello@amd.com> >>>> >>>> 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). > > This is actually on-board retimer of the AMD platform: Oh, good point. > > 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? 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). > >>> 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. >>> >>>> Reported-by: Richard Hughes <hughsient@gmail.com> >>>> Closes: https://github.com/fwupd/fwupd/issues/8200 >>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >>>> --- >>>> drivers/thunderbolt/retimer.c | 17 ++++++++++++++--- >>>> 1 file changed, 14 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/thunderbolt/retimer.c b/drivers/thunderbolt/retimer.c >>>> index 89d2919d0193e..7be435aee7217 100644 >>>> --- a/drivers/thunderbolt/retimer.c >>>> +++ b/drivers/thunderbolt/retimer.c >>>> @@ -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? > 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. >>>> - 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; >>>> + >>>> +} >> >> 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. >> >>>> + >>>> 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, >>>> }; >>>> -- >>>> 2.43.0
On Mon, Dec 09, 2024 at 09:44:43AM -0600, Mario Limonciello wrote: > On 12/9/2024 09:40, Mika Westerberg wrote: > > On Mon, Dec 09, 2024 at 08:15:16AM -0600, Mario Limonciello wrote: > > > On 12/9/2024 00:24, Mika Westerberg wrote: > > > > Hi Mario, > > > > > > > > On Fri, Dec 06, 2024 at 12:33:18PM -0600, Mario Limonciello wrote: > > > > > From: Mario Limonciello <mario.limonciello@amd.com> > > > > > > > > > > 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). > > > > This is actually on-board retimer of the AMD platform: > > Oh, good point. > > > > > 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? > > 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 ;-) > > > > 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. > > > > > > > > > Reported-by: Richard Hughes <hughsient@gmail.com> > > > > > Closes: https://github.com/fwupd/fwupd/issues/8200 > > > > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > > > > > --- > > > > > drivers/thunderbolt/retimer.c | 17 ++++++++++++++--- > > > > > 1 file changed, 14 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/thunderbolt/retimer.c b/drivers/thunderbolt/retimer.c > > > > > index 89d2919d0193e..7be435aee7217 100644 > > > > > --- a/drivers/thunderbolt/retimer.c > > > > > +++ b/drivers/thunderbolt/retimer.c > > > > > @@ -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? > > > > 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. Yeah, that sounds good to me. > > > > > > - 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; > > > > > + > > > > > +} > > > > > > 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. > > > > > > > > + > > > > > 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, > > > > > }; > > > > > -- > > > > > 2.43.0
diff --git a/drivers/thunderbolt/retimer.c b/drivers/thunderbolt/retimer.c index 89d2919d0193e..7be435aee7217 100644 --- a/drivers/thunderbolt/retimer.c +++ b/drivers/thunderbolt/retimer.c @@ -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, };