Re: [PATCH 1/5] thunderbolt: Silently ignore CLx enabling in case CLx is not supported

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

Hi,

On Wed, May 04, 2022 at 01:51:27PM +0300, Gil Fine wrote:
> Hi Mika,
> 
> On Mon, May 02, 2022 at 12:50:57PM +0300, Mika Westerberg wrote:
> > Hi Gil,
> > 
> > On Sun, May 01, 2022 at 11:33:17PM +0300, Gil Fine wrote:
> > > We can't enable CLx if it is not supported either by the host or device,
> > > or by the USB4/TBT link (e.g. when an optical cable is used).
> > > We silently ignore CLx enabling in this case.
> > > 
> > > Signed-off-by: Gil Fine <gil.fine@intel.com>
> > > ---
> > >  drivers/thunderbolt/tb.c | 10 ++++++++--
> > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> > > index 44d04b651a8b..7419cd1aefba 100644
> > > --- a/drivers/thunderbolt/tb.c
> > > +++ b/drivers/thunderbolt/tb.c
> > > @@ -581,6 +581,7 @@ static void tb_scan_port(struct tb_port *port)
> > >  	struct tb_cm *tcm = tb_priv(port->sw->tb);
> > >  	struct tb_port *upstream_port;
> > >  	struct tb_switch *sw;
> > > +	int ret;
> > >  
> > >  	if (tb_is_upstream_port(port))
> > >  		return;
> > > @@ -669,7 +670,9 @@ static void tb_scan_port(struct tb_port *port)
> > >  	tb_switch_lane_bonding_enable(sw);
> > >  	/* Set the link configured */
> > >  	tb_switch_configure_link(sw);
> > > -	if (tb_switch_enable_clx(sw, TB_CL0S))
> > > +	/* Silently ignore CLx enabling in case CLx is not supported */
> > > +	ret = tb_switch_enable_clx(sw, TB_CL0S);
> > > +	if (ret && ret != -EOPNOTSUPP)
> > 
> > I think we can debug log this and also:
> > 
> > >  		tb_sw_warn(sw, "failed to enable CLx on upstream port\n");
> > 
> > can we use something like "failed to enable CL%d on upstream port\n" and
> > pass the CL state there too? I think it is useful to see what what CL
> > state failed.
> Not sure is necessary because:
> for CL0s/CL1 from SW (Driver) perspective, they are actually enabled and supported together.
> I mean from HW/FW perspective, entry to CL1 e.g. can be objected and only
> allow entry to CL0s. But SW behaviour is similar for both.
> I mean, that if SW CM fails to enable CL1, it will also fail enabling CL0s and
> vice-versa.

OK

> So, it may be relevant if some-day we add CL2 support (which probably will not
> happen, because the introduce of the adaptive mode in USB4 v2)
> I am thinking to merge both of them to be one ENUM and to name it someting like
> "TB_CL0s_CL1":
> enum tb_clx {
> »·······TB_CLX_DISABLE,
> »·······TB_CL0s_CL1,
> »·······TB_CL2,
> }
> And then do this:
> ret = tb_switch_enable_clx(sw, TB_CL0s_CL1);
> if (ret && ret != -EOPNOTSUPP)
> 	tb_sw_warn(sw, "failed to enable %s on upstream port\n", tb_switch_clx_name(TB_CL0S_CL1));
> What do you think?

If they are always enabled together then sure but I suggest we call the
state then TB_CL1 and that includes both states (we can add comment if
needed, probably not if that's clear from the USB4 spec).