linux-kernel - RE: [PATCH 3/4] platform/chrome: cros_ec_typec: Add Displayport Alternatemode 2.1 Support

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] [day] [month] [year] [list]

Message-ID: <MWHPR11MB0048EA29C4D2654BF176FAF3A9E3A@MWHPR11MB0048.namprd11.prod.outlook.com>
Date:   Fri, 25 Aug 2023 00:02:04 +0000
From:   "Patel, Utkarsh H" <utkarsh.h.patel@...el.com>
To:     Prashant Malani <pmalani@...omium.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "heikki.krogerus@...ux.intel.com" <heikki.krogerus@...ux.intel.com>,
        "andriy.shevchenko@...ux.intel.com" 
        <andriy.shevchenko@...ux.intel.com>,
        "bleung@...omium.org" <bleung@...omium.org>
Subject: RE: [PATCH 3/4] platform/chrome: cros_ec_typec: Add Displayport
 Alternatemode 2.1 Support

Hi Prashant,

> > > I don't understand why the conf VDO is being recreated here.
> > > cable_vdo should already contain the necessary bits. Just use the
> > > cable_vdo that you get from cros_typec_get_cable_vdo(); it will have
> > > all the bits set correctly already (the EC should be doing that).
> > >
> > > The "if" condition should also be unnecessary.
> > >
> > > You are already doing something similar in the other patch for
> > > "active retimer cable support" [1]
> >
> > "if" condition is necessary because there are cables (LRD Gen3) with DPSID
> but does not support DPAM 2.1 and in that case we need to check TBTSID of
> the cable and use the cable properties e.g. SPEED and Type.
> 
> Ok, then grab the two VDOs distinctly (cable_dp_vdo and cable_tbt_vdo).
> Also, the explanation you gave above seems like a good candidate for a
> comment above the "if" block.
> 
> > Since that information is already available in pd_ctrl, we are leveraging it.  I
> will remove the part where it's checking retimer cable as DP2.1 is anyway not
> supported.
> 
> As mentioned in earlier patches related to this, we want to avoid using EC-
> specific data structures as much as possible moving forward, especially if the
> information can be gleaned from the available DiscSVID VDOs. So, please use
> the required cable VDO (whether it is DP or TBT SID).

Ack.

> 
> >
> > >
> > > > +
> > > > +   port->state.data = &dp21_data;
> > > > +
> > > > +   return typec_mux_set(port->mux, &port->state);
> > >
> > > Note that now you have reversed the order in which the muxes are set
> > > (which leads to subtle timing issues with Burnside Bridge and other similar
> retimers).
> > > So please don't do this.
> >
> > Are you suggesting to add same order here?
> 
> Specifically, breaking out a separate function for DP 2.1 and then embedding
> that in the overall enable_dp() function makes things unnecessarily complex.
> 
> Just craft the VDOs onew time, within enable_dp(), and then use the existing
> locations where cros_retimer_set and typec_mux_set() are called.
> 
> This will become more clear once you rebase this commit on top of the
> changes in [1]
> 
> Effectively cros_typec_enable_dp() should handle all DP cases, without
> needing a special function broken out for DP 2.1

Ack.

> 
> > > This entire feature isn't necessary. Regardless of whether dp2.1 is
> > > supported or not, the port driver just needs to forward the
> > > cable_vdo it receives faithfully to the mux driver, which can deal
> > > with internal details (based on whether *it* supports DP 2.1 or not).
> >
> > I am Okay with that.
> > We thought we can avoid unnecessary check for the cable_vdo for DP
> alternate mode on platform where DP2.1 is not supported.
> The optimization is miniscule enough to not be worth it the added code
> verbosity.

Ack. I will remove it.

Sincerely,
Utkarsh Patel.