On Wed, Jan 20, 2021 at 2:21 PM Paul Cercueil <paul@xxxxxxxxxxxxxxx> wrote:
>
>
>
> Le mer. 20 janv. 2021 à 14:01, Daniel Vetter <daniel@xxxxxxxx> a
> écrit :
> > On Wed, Jan 20, 2021 at 1:36 PM Paul Cercueil <paul@xxxxxxxxxxxxxxx>
> > wrote:
> >>
> >> Since the encoders have been devm-allocated, they will be freed way
> >> before drm_mode_config_cleanup() is called. To avoid use-after-free
> >> conditions, we then must ensure that drm_encoder_cleanup() is called
> >> before the encoders are freed.
> >>
> >> v2: Use the new __drmm_simple_encoder_alloc() function
> >>
> >> Fixes: c369cb27c267 ("drm/ingenic: Support multiple panels/bridges")
> >> Cc: <stable@xxxxxxxxxxxxxxx> # 5.8+
> >> Signed-off-by: Paul Cercueil <paul@xxxxxxxxxxxxxxx>
> >> ---
> >>
> >> Notes:
> >> Use the V1 of this patch to fix v5.11 and older kernels. This
> >> V2 only
> >> applies on the current drm-misc-next branch.
> >>
> >> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 16 +++++++---------
> >> 1 file changed, 7 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> >> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> >> index 7bb31fbee29d..158433b4c084 100644
> >> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> >> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> >> @@ -1014,20 +1014,18 @@ static int ingenic_drm_bind(struct device
> >> *dev, bool has_components)
> >> bridge =
> >> devm_drm_panel_bridge_add_typed(dev, panel,
> >>
> >> DRM_MODE_CONNECTOR_DPI);
> >>
> >> - encoder = devm_kzalloc(dev, sizeof(*encoder),
> >> GFP_KERNEL);
> >> - if (!encoder)
> >> - return -ENOMEM;
> >> + encoder = __drmm_simple_encoder_alloc(drm,
> >> sizeof(*encoder), 0,
> >
> > Please don't use the __ prefixed functions, those are the internal
> > ones. The official one comes with type checking and all that included.
> > Otherwise lgtm.
> > -Daniel
>
> The non-prefixed one assumes that I want to allocate a struct that
> contains the encoder, not just the drm_encoder itself.
Hm, but using the internal one is also a bit too ugly. A
drm_plain_simple_enocder_alloc(drm, type) wrapper would be the right
thing here I think? Setting the offsets and struct sizes directly in
these in drivers really doesn't feel like a good idea. I think simple
encoder is the only case where we really have a need for a
non-embeddable struct.
-Daniel
>
> -Paul
>
> >> +
> >> DRM_MODE_ENCODER_DPI);
> >> + if (IS_ERR(encoder)) {
> >> + ret = PTR_ERR(encoder);
> >> + dev_err(dev, "Failed to init encoder:
> >> %d\n", ret);
> >> + return ret;
> >> + }
> >>
> >> encoder->possible_crtcs = 1;
> >>
> >> drm_encoder_helper_add(encoder,
> >> &ingenic_drm_encoder_helper_funcs);
> >>
> >> - ret = drm_simple_encoder_init(drm, encoder,
> >> DRM_MODE_ENCODER_DPI);
> >> - if (ret) {
> >> - dev_err(dev, "Failed to init encoder:
> >> %d\n", ret);
> >> - return ret;
> >> - }
> >> -
> >> ret = drm_bridge_attach(encoder, bridge, NULL, 0);
> >> if (ret) {
> >> dev_err(dev, "Unable to attach bridge\n");
> >> --
> >> 2.29.2
> >>
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
>
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
