linux-kernel - Re: [PATCH v2 0/4] usb: typec: tipd: add patch update support for tps6598x

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <nza4s2kjmcptz6epbyegwy6wh32buyxm5evnk2jultqblgzs4b@6mzuklpqhby7>
Date: Fri, 5 Jan 2024 13:42:16 +0530
From: Jai Luthra <j-luthra@...com>
To: Javier Carrasco <javier.carrasco@...fvision.net>, <rogerq@...nel.org>
CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Heikki Krogerus
	<heikki.krogerus@...ux.intel.com>,
        <linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <vigneshr@...com>, <d-gole@...com>, <nm@...com>
Subject: Re: [PATCH v2 0/4] usb: typec: tipd: add patch update support for
 tps6598x

On Jan 04, 2024 at 17:25:27 +0100, Javier Carrasco wrote:
> Hi Jai Luthra,
> On 04.01.24 16:47, Jai Luthra wrote>> FYI, this series breaks boot for
> TI SK-AM62A and SK-AM62 which use
> >> TPS6598x as the USB-C PD chip.
> >>
> >> The platforms stopped booting since next-20240103 [1], and reverting 
> >> this series [4] seems to fix the issue [2]
> >>
> >> Is there any change needed in the board device-tree [3] and bindings?
> >>
> >> We don't specify any firmware in the device-tree node, but seems like 
> >> that is an assumption in this series. I tried reverting it (below 
> >> change) but that did *not* help with the stuck boot.
> >>Thanks a lot for your high-quality feedback. I am glad to see that you
> even found a solution to the issue.
> 
> The firmware update only happens if the device is in patch mode ('PTCH'
> in the Mode register - 0x03), which is the expected behavior because the
> device waits in that mode until a patch arrives. That is probably the
> reason why your first attempt did not work (no update was triggered).

Understood. Btw your mail client seems to mess up quotes/spacing above.

> 
> The problem seems to be related to the reset function, as you already
> noticed. That function is only called in suspend/resume, if the probe
> fails and in the remove function.
> 
> Did the probe function fail and if so, could you see why? The reset
> function is identical for the tps25750 and tps6598x ('GAID' with no
> parameters), so I wonder why that should be the source of the problem.

I added some prints and can see that the probe fails once at 
fwnode_usb_role_switch_get() because the other endpoint (of USB device) 
is not yet probed. It then re-probes later where it passes.

The GAID reset being done unconditionally in your series seems to cause 
the board to get stuck in the boot process when it hits the above error 
due to probe-order between USB subsystem and this IC. My guess would be 
SoC stops getting power because we reset the PD chip?

Anyway, I will send below change as a separate "Fixes:" patch for now, 
to keep how things as they were before your series.

If you have a better architecture in mind that can reset only when PTCH 
has been applied and not for other probe defers, feel free to send it on 
top of it.

> > The following change seems to fix boot on SK-AM62A without reverting 
> > this whole series:
> > 
> > ------------------
> > 
> > diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> > index a956eb976906a5..8ba2aa05db519b 100644
> > --- a/drivers/usb/typec/tipd/core.c
> > +++ b/drivers/usb/typec/tipd/core.c
> > @@ -1223,11 +1223,16 @@ static int cd321x_reset(struct tps6598x *tps)
> >  	return 0;
> >  }
> >  
> > -static int tps6598x_reset(struct tps6598x *tps)
> > +static int tps25750_reset(struct tps6598x *tps)
> >  {
> >  	return tps6598x_exec_cmd_tmo(tps, "GAID", 0, NULL, 0, NULL, 2000, 0);
> >  }
> >  
> > +static int tps6598x_reset(struct tps6598x *tps)
> > +{
> > +	return 0;
> > +}
> > +
> >  static int
> >  tps25750_register_port(struct tps6598x *tps, struct fwnode_handle *fwnode)
> >  {
> > @@ -1545,7 +1550,7 @@ static const struct tipd_data tps25750_data = {
> >  	.trace_status = trace_tps25750_status,
> >  	.apply_patch = tps25750_apply_patch,
> >  	.init = tps25750_init,
> > -	.reset = tps6598x_reset,
> > +	.reset = tps25750_reset,
> >  };
> >  
> >  static const struct of_device_id tps6598x_of_match[] = {
> > 
> > ------------------
> > 
> > I am not an expert on this, will let you/others decide on what should be 
> > the correct way to reset TPS6598x for patching without breaking this SK.
> > 
> > 
> 
> The driver did not support cold resets before, but that does not mean
> that they should not happen like it does for the tps25750. Your fix just
> removes the reset function for the tps6598x, and I would like to know
> why the reset happened in the fist place.
> 
> Thanks and best regards,
> Javier Carrasco

-- 
Thanks,
Jai

GPG Fingerprint: 4DE0 D818 E5D5 75E8 D45A AFC5 43DE 91F9 249A 7145

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)