Re: [PATCH] thunderbolt: Don't display retimers unless nvm was initialized

[Mika Westerberg <mika.westerberg@linux.intel.com>]

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