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@xxxxxxx> > > > > > > > > > > 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@xxxxxxxxx> > > > > > Closes: https://github.com/fwupd/fwupd/issues/8200 > > > > > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx> > > > > > --- > > > > > 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
