Quoting Douglas Anderson (2023-06-12 16:53:03) > Memory for the "struct device" for any given device isn't supposed to > be released until the device's release() is called. This is important > because someone might be holding a kobject reference to the "struct > device" and might try to access one of its members even after any > other cleanup/uninitialization has happened. > > Code analysis of ti-sn65dsi86 shows that this isn't quite right. When > the code was written, it was believed that we could rely on the fact > that the child devices would all be freed before the parent devices > and thus we didn't need to worry about a release() function. While I > still believe that the parent's "struct device" is guaranteed to > outlive the child's "struct device" (because the child holds a kobject > reference to the parent), the parent's "devm" allocated memory is a > different story. That appears to be freed much earlier. > > Let's make this better for ti-sn65dsi86 by allocating each auxiliary > with kzalloc and then free that memory in the release(). > > Fixes: bf73537f411b ("drm/bridge: ti-sn65dsi86: Break GPIO and MIPI-to-eDP bridge into sub-drivers") > Suggested-by: Stephen Boyd <swboyd@xxxxxxxxxxxx> > Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx> > --- Thanks! > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index 597ceb7024e0..db1461cc3170 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -464,27 +464,32 @@ static void ti_sn65dsi86_delete_aux(void *data) > auxiliary_device_delete(data); > } > > -/* > - * AUX bus docs say that a non-NULL release is mandatory, but it makes no > - * sense for the model used here where all of the aux devices are allocated > - * in the single shared structure. We'll use this noop as a workaround. > - */ > -static void ti_sn65dsi86_noop(struct device *dev) {} > +static void ti_sn65dsi86_aux_device_release(struct device *dev) > +{ > + struct auxiliary_device *aux = container_of(dev, struct auxiliary_device, dev); > + > + kfree(aux); > +} > > static int ti_sn65dsi86_add_aux_device(struct ti_sn65dsi86 *pdata, > - struct auxiliary_device *aux, > + struct auxiliary_device **aux_out, > const char *name) > { > struct device *dev = pdata->dev; > + struct auxiliary_device *aux; > int ret; > > + aux = kzalloc(sizeof(*aux), GFP_KERNEL); Check for allocation failure? > + > aux->name = name; > aux->dev.parent = dev; > - aux->dev.release = ti_sn65dsi86_noop; > + aux->dev.release = ti_sn65dsi86_aux_device_release; > device_set_of_node_from_dev(&aux->dev, dev); > ret = auxiliary_device_init(aux); > - if (ret) > + if (ret) { > + kfree(aux); > return ret; > + } > ret = devm_add_action_or_reset(dev, ti_sn65dsi86_uninit_aux, aux); > if (ret) > return ret; > @@ -494,6 +499,9 @@ static int ti_sn65dsi86_add_aux_device(struct ti_sn65dsi86 *pdata, > return ret; > ret = devm_add_action_or_reset(dev, ti_sn65dsi86_delete_aux, aux); > Nitpick: Stick this if line to the line above > + if (!ret) > + *aux_out = aux; > + > return ret;