linux-kernel - RE: [PATCH v3 1/2] platform/chrome: cros_ec

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: <MWHPR11MB0048D02003AC21B36F297EF6A931A@MWHPR11MB0048.namprd11.prod.outlook.com>
Date:   Tue, 11 Jul 2023 16:06:50 +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>,
        "bleung@...omium.org" <bleung@...omium.org>
Subject: RE: [PATCH v3 1/2] platform/chrome: cros_ec_typec: Configure Retimer
 cable type

Hi Prashant,

Thank you for the review. 

> > diff --git a/drivers/platform/chrome/cros_ec_typec.c
> > b/drivers/platform/chrome/cros_ec_typec.c
> > index 25f9767c28e8..0ea085fec55a 100644
> > --- a/drivers/platform/chrome/cros_ec_typec.c
> > +++ b/drivers/platform/chrome/cros_ec_typec.c
> > @@ -406,6 +406,20 @@ static int cros_typec_usb_safe_state(struct
> cros_typec_port *port)
> >         return ret;
> >  }
> >
> > +static int cros_typec_get_cable_vdo(struct cros_typec_port *port, u16
> > +svid)
> Return type should be u32.
> Also, since you're not using common return patterns (-ve return value),
> please add a kernel doc comment specifying what the return value holds ("0
> if VDO is not found" etc.)

Ack.

> 
> > +{
> > +       struct list_head *head = &port->plug_mode_list;
> > +       struct cros_typec_altmode_node *node;
> > +       int ret = 0;
> > +
> > +       list_for_each_entry(node, head, list) {
> > +               if (node->amode->svid == svid)
> > +                       return node->amode->vdo;
> > +       }
> > +
> > +       return ret;
> > +}
> > +
> >  /*
> >   * Spoof the VDOs that were likely communicated by the partner for TBT
> alt
> >   * mode.
> > @@ -416,6 +430,7 @@ static int cros_typec_enable_tbt(struct
> > cros_typec_data *typec,  {
> >         struct cros_typec_port *port = typec->ports[port_num];
> >         struct typec_thunderbolt_data data;
> > +       u32 cable_vdo;
> If we're using this in only 1 place, we can just inline it completely:
>           data.cable_mode |= cros_typec_get_cable_vdo(port,
> USB_TYPEC_TBT_SID);
> 

Ack.

> >         int ret;
> >
> >         if (typec->pd_ctrl_ver < 2) {
> > @@ -432,6 +447,10 @@ static int cros_typec_enable_tbt(struct
> > cros_typec_data *typec,
> >
> >         /* Cable Discover Mode VDO */
> >         data.cable_mode = TBT_MODE;
> > +
> > +       cable_vdo = cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID);
> > +       data.cable_mode |= cable_vdo;
> > +
> >         data.cable_mode |= TBT_SET_CABLE_SPEED(pd_ctrl->cable_speed);
> >
> >         if (pd_ctrl->control_flags & USB_PD_CTRL_OPTICAL_CABLE) @@
> > -513,17 +532,22 @@ static int cros_typec_enable_usb4(struct
> > cros_typec_data *typec,  {
> >         struct cros_typec_port *port = typec->ports[port_num];
> >         struct enter_usb_data data;
> > +       u32 cable_vdo;
> >
> >         data.eudo = EUDO_USB_MODE_USB4 << EUDO_USB_MODE_SHIFT;
> >
> > +       cable_vdo = cros_typec_get_cable_vdo(port, USB_TYPEC_TBT_SID);
> 
> Same deal here, if we're only using this variable in one place, just inline the
> call inside the else if() statement.
> 

Ack. 


Sincerely,
Utkarsh Patel.