Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
MIME-Version: 1.0
References: <20231016225617.3182108-2-rdbabiera@google.com>
In-Reply-To: <20231016225617.3182108-2-rdbabiera@google.com>
From:   Prashant Malani <pmalani@chromium.org>
Date:   Tue, 17 Oct 2023 14:33:18 -0700
Message-ID: <CACeCKac2kknw2s7orH1tu4RsiCr5+WYy1VQ483ok_zuzC2N9zQ@mail.gmail.com>
Subject: Re: [PATCH v1] usb: typec: altmodes/displayport: verify compatible
 source/sink role combination
To:     RD Babiera <rdbabiera@google.com>
Cc:     heikki.krogerus@linux.intel.com, gregkh@linuxfoundation.org,
        badhri@google.com, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org, stable@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi RD,

On Mon, Oct 16, 2023 at 3:56=E2=80=AFPM RD Babiera <rdbabiera@google.com> w=
rote:
>
> DisplayPort Alt Mode CTS test 10.3.8 states that both sides of the
> connection shall be compatible with one another such that the connection
> is not Source to Source or Sink to Sink.
>
> The DisplayPort driver currently checks for a compatible pin configuratio=
n
> that resolves into a source and sink combination. The CTS test is designe=
d
> to send a Discover Modes message that has a compatible pin configuration
> but advertises the same port capability as the device; the current check
> fails this.
>
> Verify that the port and port partner resolve into a valid source and sin=
k
> combination before checking for a compatible pin configuration.
>
> Fixes: 0e3bb7d6894d ("usb: typec: Add driver for DisplayPort alternate mo=
de")
> Cc: stable@vger.kernel.org
> Signed-off-by: RD Babiera <rdbabiera@google.com>
> ---
>  drivers/usb/typec/altmodes/displayport.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec=
/altmodes/displayport.c
> index 718da02036d8..3b35a6b8cb72 100644
> --- a/drivers/usb/typec/altmodes/displayport.c
> +++ b/drivers/usb/typec/altmodes/displayport.c
> @@ -575,9 +575,18 @@ int dp_altmode_probe(struct typec_altmode *alt)
>         struct fwnode_handle *fwnode;
>         struct dp_altmode *dp;
>         int ret;
> +       int port_cap, partner_cap;

VDOs are 32-bit, so u32 is probably better here.

>
>         /* FIXME: Port can only be DFP_U. */
>
> +       /* Make sure that the port and partner can resolve into source an=
d sink */
> +       port_cap =3D DP_CAP_CAPABILITY(port->vdo);
> +       partner_cap =3D DP_CAP_CAPABILITY(alt->vdo);
> +       if (!((port_cap & DP_CAP_DFP_D) && (partner_cap & DP_CAP_UFP_D)) =
&&

nit: bitwise '&' has a higher precedence than logical '&&', so the
innermost parentheses shouldn't be necessary:

           if (!(port_cap & DP_CAP_DFP_D && partner_cap & DP_CAP_UFP_D) &&
               !(port_cap & DP_CAP_UFP_D && partner_cap & DP_CAP_DFP_D))
                   return -ENODEV;
                ...

OTOH, perhaps you should just introduce a macro that performs this
bitwise operation for even better
readability. Something like

#define DP_CAP_IS_DFP_D(_cap_)        (!!(DP_CAP_CAPABILITY(_cap_) &
DP_CAP_DFP_D))

(not sure if "!!" is tolerated in kernel style, but you get the gist...)


> +           !((port_cap & DP_CAP_UFP_D) && (partner_cap & DP_CAP_DFP_D)))=
 {
> +               return -ENODEV;

Single line if statements can drop curly braces [1]

Best regards,

-Prashant

[1] https://www.kernel.org/doc/html/v4.10/process/coding-style.html#placing=
-braces-and-spaces