On Wed, Dec 11, 2024 at 1:58 PM Stephen Boyd <sboyd@xxxxxxxxxx> wrote:
>
> Quoting Abhishek Pandit-Subedi (2024-12-06 15:38:16)
> > diff --git a/drivers/platform/chrome/cros_typec_altmode.c b/drivers/platform/chrome/cros_typec_altmode.c
> > new file mode 100644
> > index 000000000000..bb7c7ad2ff6e
> > --- /dev/null
> > +++ b/drivers/platform/chrome/cros_typec_altmode.c
> > @@ -0,0 +1,281 @@
> [...]
> > +
> > +static const struct typec_altmode_ops cros_typec_altmode_ops = {
> > + .enter = cros_typec_altmode_enter,
> > + .exit = cros_typec_altmode_exit,
> > + .vdm = cros_typec_altmode_vdm,
> > +};
> > +
> > +#if IS_ENABLED(CONFIG_TYPEC_DP_ALTMODE)
> > +int cros_typec_displayport_status_update(struct typec_altmode *altmode,
> > + struct typec_displayport_data *data)
> > +{
> > + struct cros_typec_dp_data *dp_data =
> > + typec_altmode_get_drvdata(altmode);
>
> How does this work? I see that the type of the drvdata is struct
> cros_typec_altmode_data per the allocation in
> cros_typec_register_displayport(), but here we're treating it as the
> type struct cros_typec_dp_data, which has a struct
> cros_typec_altmode_data as the first member. The allocation is too small
> from what I can tell. The same problem looks to be there in
> cros_typec_displayport_vdm().
>
> > + struct cros_typec_altmode_data *adata = &dp_data->adata;
> > +
> > + if (!dp_data->pending_status_update) {
> > + dev_dbg(&altmode->dev,
> > + "Got DPStatus without a pending request");
> > + return 0;
> > + }
> > +
> > + if (dp_data->configured && dp_data->data.conf != data->conf)
> > + dev_dbg(&altmode->dev,
> > + "DP Conf doesn't match. Requested 0x%04x, Actual 0x%04x",
> > + dp_data->data.conf, data->conf);
> > +
> > + mutex_lock(&adata->lock);
> > +
> > + dp_data->data = *data;
> > + dp_data->pending_status_update = false;
> > + adata->header |= VDO_CMDT(CMDT_RSP_ACK);
> > + adata->vdo_data = &dp_data->data.status;
> > + adata->vdo_size = 2;
> > + schedule_work(&adata->work);
> > +
> > + mutex_unlock(&adata->lock);
> > +
> > + return 0;
> > +}
> > +
> > +struct typec_altmode *
> > +cros_typec_register_displayport(struct cros_typec_port *port,
> > + struct typec_altmode_desc *desc,
> > + bool ap_mode_entry)
> > +{
> > + struct typec_altmode *alt;
> > + struct cros_typec_altmode_data *data;
> > +
> > + alt = typec_port_register_altmode(port->port, desc);
> > + if (IS_ERR(alt))
> > + return alt;
> > +
> > + data = devm_kzalloc(&alt->dev, sizeof(*data), GFP_KERNEL);
> > + if (!data) {
> > + typec_unregister_altmode(alt);
> > + return ERR_PTR(-ENOMEM);
> > + }
> > +
> > + INIT_WORK(&data->work, cros_typec_altmode_work);
> > + mutex_init(&data->lock);
> > + data->alt = alt;
> > + data->port = port;
> > + data->ap_mode_entry = ap_mode_entry;
> > + data->sid = desc->svid;
> > + data->mode = desc->mode;
> > +
> > + typec_altmode_set_ops(alt, &cros_typec_altmode_ops);
> > + typec_altmode_set_drvdata(alt, data);
>
> 'data' is of type struct cros_typec_altmode_data here
This should have been allocated as cros_typec_dp_data. Missed during a
previous refactor that changed the type from a union to this format.
>
> > +
> > + return alt;
> > +}
