Hi RD,
On Mon, Oct 16, 2023 at 3:56 PM RD Babiera <rdbabiera@xxxxxxxxxx> wrote:
>
> 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 configuration
> that resolves into a source and sink combination. The CTS test is designed
> 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 sink
> combination before checking for a compatible pin configuration.
>
> Fixes: 0e3bb7d6894d ("usb: typec: Add driver for DisplayPort alternate mode")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: RD Babiera <rdbabiera@xxxxxxxxxx>
> ---
> 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 and sink */
> + port_cap = DP_CAP_CAPABILITY(port->vdo);
> + partner_cap = 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
