Re: [PATCH v4 5/7] platform/chrome: cros_ec_typec: Displayport support

[Stephen Boyd <sboyd@kernel.org>]

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

> +
> +       return alt;
> +}