Hi Guenter,

On Fri, Mar 16, 2018 at 02:32:06PM -0700, Guenter Roeck wrote:
> On Fri, Mar 09, 2018 at 06:19:18PM +0300, Heikki Krogerus wrote:
> > This adds more complete handling of VDMs and registration of
> > partner alternate modes, and introduces callbacks for
> > alternate mode operations.
> > 
> > Only DFP role is supported for now.
> > 
> > Signed-off-by: Heikki Krogerus <heikki.kroge...@linux.intel.com>
> > ---
> >  drivers/usb/typec/tcpm.c | 133 
> > +++++++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 111 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> > index bfb843ebffa6..34bf5c1f4d81 100644
> > --- a/drivers/usb/typec/tcpm.c
> > +++ b/drivers/usb/typec/tcpm.c
> > @@ -158,13 +158,14 @@ enum pd_msg_request {
> >  /* Alternate mode support */
> >  
> >  #define SVID_DISCOVERY_MAX 16
> > +#define ALTMODE_DISCOVERY_MAX      (SVID_DISCOVERY_MAX * 
> > MODE_DISCOVERY_MAX)
> >  
> >  struct pd_mode_data {
> >     int svid_index;         /* current SVID index           */
> >     int nsvids;
> >     u16 svids[SVID_DISCOVERY_MAX];
> >     int altmodes;           /* number of alternate modes    */
> > -   struct typec_altmode_desc altmode_desc[SVID_DISCOVERY_MAX];
> > +   struct typec_altmode_desc altmode_desc[ALTMODE_DISCOVERY_MAX];
> >  };
> >  
> >  struct tcpm_port {
> > @@ -280,8 +281,8 @@ struct tcpm_port {
> >     /* Alternate mode data */
> >  
> >     struct pd_mode_data mode_data;
> > -   struct typec_altmode *partner_altmode[SVID_DISCOVERY_MAX * 6];
> > -   struct typec_altmode *port_altmode[SVID_DISCOVERY_MAX * 6];
> > +   struct typec_altmode *partner_altmode[ALTMODE_DISCOVERY_MAX];
> > +   struct typec_altmode *port_altmode[ALTMODE_DISCOVERY_MAX];
> >  
> >     /* Deadline in jiffies to exit src_try_wait state */
> >     unsigned long max_wait;
> > @@ -985,36 +986,53 @@ static void svdm_consume_modes(struct tcpm_port 
> > *port, const __le32 *payload,
> >                      pmdata->altmodes, paltmode->svid,
> >                      paltmode->mode, paltmode->vdo);
> >  
> > -           port->partner_altmode[pmdata->altmodes] =
> > -                   typec_partner_register_altmode(port->partner, paltmode);
> > -           if (!port->partner_altmode[pmdata->altmodes]) {
> > -                   tcpm_log(port,
> > -                            "Failed to register modes for SVID 0x%04x",
> > -                            paltmode->svid);
> > -                   return;
> > -           }
> >             pmdata->altmodes++;
> >     }
> >  }
> >  
> > +static void tcpm_register_partner_altmodes(struct tcpm_port *port)
> > +{
> > +   struct pd_mode_data *modep = &port->mode_data;
> > +   struct typec_altmode *altmode;
> > +   int i;
> > +
> > +   for (i = 0; i < modep->altmodes; i++) {
> > +           altmode = typec_partner_register_altmode(port->partner,
> > +                                           &modep->altmode_desc[i]);
> > +           if (!altmode)
> > +                   tcpm_log(port, "Failed to register partner SVID 0x%04x",
> > +                            modep->altmode_desc[i].svid);
> > +           port->partner_altmode[i] = altmode;
> > +   }
> > +}
> > +
> >  #define supports_modal(port)       
> > PD_IDH_MODAL_SUPP((port)->partner_ident.id_header)
> >  
> >  static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int 
> > cnt,
> >                     u32 *response)
> >  {
> > -   u32 p0 = le32_to_cpu(payload[0]);
> > -   int cmd_type = PD_VDO_CMDT(p0);
> > -   int cmd = PD_VDO_CMD(p0);
> > +   struct typec_altmode *altmode;
> >     struct pd_mode_data *modep;
> > +   u32 p[PD_MAX_PAYLOAD];
> >     int rlen = 0;
> > -   u16 svid;
> > +   int cmd_type;
> > +   int cmd;
> >     int i;
> >  
> > +   for (i = 0; i < cnt; i++)
> > +           p[i] = le32_to_cpu(payload[i]);
> > +
> > +   cmd_type = PD_VDO_CMDT(p[0]);
> > +   cmd = PD_VDO_CMD(p[0]);
> > +
> >     tcpm_log(port, "Rx VDM cmd 0x%x type %d cmd %d len %d",
> > -            p0, cmd_type, cmd, cnt);
> > +            p[0], cmd_type, cmd, cnt);
> >  
> >     modep = &port->mode_data;
> >  
> > +   altmode = typec_match_altmode(port->port_altmode, ALTMODE_DISCOVERY_MAX,
> > +                                 PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
> 
> This can return NULL ...
> 
> > +
> >     switch (cmd_type) {
> >     case CMDT_INIT:
> >             switch (cmd) {
> > @@ -1036,17 +1054,21 @@ static int tcpm_pd_svdm(struct tcpm_port *port, 
> > const __le32 *payload, int cnt,
> >             case CMD_EXIT_MODE:
> >                     break;
> >             case CMD_ATTENTION:
> > -                   break;
> > +                   typec_altmode_attention(altmode, p[1]);
> 
> ... and NULL is not handled by typec_altmode_attention().

True.

> > +                   return 0;
> >             default:
> > +                   if (typec_altmode_vdm(altmode, p[0], &p[1], cnt) ==
> > +                       VDM_OK)
> 
> typec_altmode_vdm() returns old fashioned error messages.
> Not sure what it is supposed to return, though.

The idea was that the alternate mode driver can inform the port driver
that it not support a specific VDM. In that case we need to NAK.

I'll think about this a bit more.

> > +                           return 0;
> >                     break;
> >             }
> >             if (rlen >= 1) {
> > -                   response[0] = p0 | VDO_CMDT(CMDT_RSP_ACK);
> > +                   response[0] = p[0] | VDO_CMDT(CMDT_RSP_ACK);
> >             } else if (rlen == 0) {
> > -                   response[0] = p0 | VDO_CMDT(CMDT_RSP_NAK);
> > +                   response[0] = p[0] | VDO_CMDT(CMDT_RSP_NAK);
> >                     rlen = 1;
> >             } else {
> > -                   response[0] = p0 | VDO_CMDT(CMDT_RSP_BUSY);
> > +                   response[0] = p[0] | VDO_CMDT(CMDT_RSP_BUSY);
> >                     rlen = 1;
> >             }
> >             break;
> > @@ -1079,20 +1101,26 @@ static int tcpm_pd_svdm(struct tcpm_port *port, 
> > const __le32 *payload, int cnt,
> >                     svdm_consume_modes(port, payload, cnt);
> >                     modep->svid_index++;
> >                     if (modep->svid_index < modep->nsvids) {
> > -                           svid = modep->svids[modep->svid_index];
> > +                           u16 svid = modep->svids[modep->svid_index];
> >                             response[0] = VDO(svid, 1, CMD_DISCOVER_MODES);
> >                             rlen = 1;
> >                     } else {
> > -                           /* enter alternate mode if/when implemented */
> > +                           tcpm_register_partner_altmodes(port);
> >                     }
> >                     break;
> >             case CMD_ENTER_MODE:
> > +                   typec_altmode_enter(altmode);
> 
> typec_altmode_enter() don't handle altmode == NULL.

True. I'll fix that as well.

> No error handling ?
> 
> > +                   break;
> > +           case CMD_EXIT_MODE:
> > +                   typec_altmode_exit(altmode);
> 
> Doesn't handle NULL. Error handling ?

True, and I'll add error handling for there.

> >                     break;
> >             default:
> > +                   typec_altmode_vdm(altmode, p[0], &p[1], cnt);
> >                     break;
> >             }
> >             break;
> >     default:
> > +           typec_altmode_vdm(altmode, p[0], &p[1], cnt);
> >             break;
> >     }
> >  
> > @@ -1352,6 +1380,47 @@ static int tcpm_validate_caps(struct tcpm_port 
> > *port, const u32 *pdo,
> >     return 0;
> >  }
> >  
> > +static void tcpm_altmode_enter(struct typec_altmode *altmode)
> > +{
> > +   struct tcpm_port *port = typec_altmode_get_drvdata(altmode);
> > +   u32 header;
> > +
> > +   header = VDO(altmode->svid, 1, CMD_ENTER_MODE);
> > +   header |= VDO_OPOS(altmode->mode);
> > +
> > +   tcpm_queue_vdm(port, header, NULL, 0);
> > +   mod_delayed_work(port->wq, &port->vdm_state_machine, 0);
> > +}
> > +
> > +static void tcpm_altmode_exit(struct typec_altmode *altmode)
> > +{
> > +   struct tcpm_port *port = typec_altmode_get_drvdata(altmode);
> > +   u32 header;
> > +
> > +   header = VDO(altmode->svid, 1, CMD_EXIT_MODE);
> > +   header |= VDO_OPOS(altmode->mode);
> > +
> > +   tcpm_queue_vdm(port, header, NULL, 0);
> > +   mod_delayed_work(port->wq, &port->vdm_state_machine, 0);
> > +}
> > +
> > +static int tcpm_altmode_vdm(struct typec_altmode *altmode,
> > +                       u32 header, const u32 *data, int count)
> > +{
> > +   struct tcpm_port *port = typec_altmode_get_drvdata(altmode);
> > +
> > +   tcpm_queue_vdm(port, header, data, count - 1);
> > +   mod_delayed_work(port->wq, &port->vdm_state_machine, 0);
> > +
> > +   return 0;
> > +}
> > +
> > +static const struct typec_altmode_ops tcpm_altmode_ops = {
> > +   .enter = tcpm_altmode_enter,
> > +   .exit = tcpm_altmode_exit,
> > +   .vdm = tcpm_altmode_vdm,
> > +};
> > +
> >  /*
> >   * PD (data, control) command handling functions
> >   */
> > @@ -3479,6 +3548,23 @@ static void tcpm_init(struct tcpm_port *port)
> >     tcpm_set_state(port, PORT_RESET, 0);
> >  }
> >  
> > +static int tcpm_activate_mode(struct typec_altmode *alt, int activate)
> > +{
> > +   struct tcpm_port *port = typec_altmode_get_drvdata(alt);
> > +   u32 header;
> > +
> > +   header = VDO(cpu_to_le16(alt->svid), 1,
> > +                activate ? CMD_ENTER_MODE : CMD_EXIT_MODE);
> > +   header |= VDO_OPOS(alt->mode);
> > +
> > +   mutex_lock(&port->lock);
> > +   tcpm_queue_vdm(port, header, NULL, 0);
> > +   mod_delayed_work(port->wq, &port->vdm_state_machine, 0);
> > +   mutex_unlock(&port->lock);
> 
> Some of the above functions are port mutex protected, some are not.
> Is this on purpose ?

No. The plan is to always use the mutex.

I can't call typec_altmode_enter/exit() directly from tcpm_pd_svdm()
because the alternate mode drivers may call typec_altmode_enter() from
their probe drivers. I guess the solution is that instead of directly
calling typec_altmode_enter/exit() from tcpm_pm_svdm(), we schedule a
work where we call them. Or would you have some better ideas for this?

For this RFC I just hacked it and didn't use the mutex in
tcpm_altmode_enter/exit().


Thanks for reviewing these,

-- 
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to