linux-kernel - Re: [PATCH v2] usb: typec: tcpm: Use configured PD revision for negotiation

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: <CAPTae5+GFX7HpB6Vp68LQtFgy+iMCj8cCOuHUPyn8nZPBt7+zQ@mail.gmail.com>
Date: Thu, 15 May 2025 22:10:05 -0700
From: Badhri Jagan Sridharan <badhri@...gle.com>
To: Amit Sunil Dhamne <amitsd@...gle.com>
Cc: Heikki Krogerus <heikki.krogerus@...ux.intel.com>, Cosmo Chou <chou.cosmo@...il.com>, 
	gregkh@...uxfoundation.org, linux-usb@...r.kernel.org, 
	linux-kernel@...r.kernel.org, cosmo.chou@...ntatw.com
Subject: Re: [PATCH v2] usb: typec: tcpm: Use configured PD revision for negotiation

On Thu, May 15, 2025 at 12:15 AM Amit Sunil Dhamne <amitsd@...gle.com> wrote:
>
> Hi Heikki,
>
> On 5/14/25 1:12 AM, Heikki Krogerus wrote:
> > On Tue, May 13, 2025 at 10:14:32PM +0800, Cosmo Chou wrote:
> >> On Tue, May 13, 2025 at 04:50:49PM +0300, Heikki Krogerus wrote:
> >>> On Tue, May 13, 2025 at 04:39:09PM +0300, Heikki Krogerus wrote:
> >>>> On Tue, May 13, 2025 at 09:08:34PM +0800, Cosmo Chou wrote:
> >>>>> Initialize negotiated_rev and negotiated_rev_prime based on the port's
> >>>>> configured PD revision (rev_major) rather than always defaulting to
> >>>>> PD_MAX_REV. This ensures ports start PD communication using their
> >>>>> appropriate revision level.
> >>>>>
> >>>>> This allows proper communication with devices that require specific
> >>>>> PD revision levels, especially for the hardware designed for PD 1.0
> >>>>> or 2.0 specifications.
> >>>>>
> >>>>> Signed-off-by: Cosmo Chou <chou.cosmo@...il.com>

Reviewed-by: Badhri Jagan Sridharan <badhri@...gle.com>

> >>>>> ---
> >>>>> Change log:
> >>>>>
> >>>>> v2:
> >>>>>   - Add PD_CAP_REVXX macros and use switch-case for better readability.
> >>>>>
> >>>>> ---
> >>>>>  drivers/usb/typec/tcpm/tcpm.c | 29 +++++++++++++++++++++++++----
> >>>>>  1 file changed, 25 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> >>>>> index 8adf6f954633..48e9cfc2b49a 100644
> >>>>> --- a/drivers/usb/typec/tcpm/tcpm.c
> >>>>> +++ b/drivers/usb/typec/tcpm/tcpm.c
> >>>>> @@ -313,6 +313,10 @@ struct pd_data {
> >>>>>     unsigned int operating_snk_mw;
> >>>>>  };
> >>>>>
> >>>>> +#define PD_CAP_REV10       0x1
> >>>>> +#define PD_CAP_REV20       0x2
> >>>>> +#define PD_CAP_REV30       0x3
> >>>>> +
> >>>>>  struct pd_revision_info {
> >>>>>     u8 rev_major;
> >>>>>     u8 rev_minor;
> >>>>> @@ -4665,6 +4669,25 @@ static void tcpm_set_initial_svdm_version(struct tcpm_port *port)
> >>>>>     }
> >>>>>  }
> >>>>>
> >>>>> +static void tcpm_set_initial_negotiated_rev(struct tcpm_port *port)
> >>>>> +{
> >>>>> +   switch (port->pd_rev.rev_major) {
> >>>>> +   case PD_CAP_REV10:
> >>>>> +           port->negotiated_rev = PD_REV10;
> >>>>> +           break;
> >>>>> +   case PD_CAP_REV20:
> >>>>> +           port->negotiated_rev = PD_REV20;
> >>>>> +           break;
> >>>>> +   case PD_CAP_REV30:
> >>>>> +           port->negotiated_rev = PD_REV30;
> >>>>> +           break;
> >>>>> +   default:
> >>>>> +           port->negotiated_rev = PD_MAX_REV;
> >>>>> +           break;
> >>>>> +   }
> >>>>> +   port->negotiated_rev_prime = port->negotiated_rev;
> >>>>> +}

IMHO this looks better in terms of readability although V1 is more concise.

Thanks,
Badhri

> >>>> Do we need this? Couldn't you just add one to rev_major?
> >>>>
> >>>>         port->negotiated_rev = port->pd_rev.rev_major + 1;
> >>>>         port->negotiated_rev_prime = port->pd_rev.rev_major + 1;
> >>>>
> >>>> Or am I missing something?
> >>> Sorry, I mean minus one :-)
> >>>
> >>>         port->negotiated_rev = port->pd_rev.rev_major - 1;
> >>>         port->negotiated_rev_prime = port->pd_rev.rev_major - 1;
>
> The only reason I asked for macros is that in the case of Spec Revision
> for header, the value for PD 3.0 is 0x2, PD 2.0 is 0x1 & so on. While
> for PD max revisions, it's the exact values. Having a clear distinction
> may be easier to follow. If you want to go with the +/- approach you can
> add a comment stating the above.
>
> I don't have a hard opinion on either approach :).
>
> Thanks,
>
> Amit
>
> >>>
> >>> --
> >>> heikki
> >> It seems to be the PATCH v1:
> >> https://lore.kernel.org/all/20250508174756.1300942-1-chou.cosmo@gmail.com/
> >>
> >> if (port->pd_rev.rev_major > 0 && port->pd_rev.rev_major <= PD_MAX_REV + 1) {
> >>         port->negotiated_rev = port->pd_rev.rev_major - 1;
> >>         port->negotiated_rev_prime = port->pd_rev.rev_major - 1
> >> } else {
> >>         port->negotiated_rev = PD_MAX_REV;
> >>         port->negotiated_rev_prime = PD_MAX_REV;
> >> }
> > Okay, sorry I missed that. I still don't like the extra definitions,
> > but I don't have any better idea (I guess macro is not an option?).
> > Reviewed-by: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> >
> > thanks,
> >