| Message ID | 20210120123535.40226-3-paul@crapouillou.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixes to bridge/panel and ingenic-drm | expand |
Le mer. 20 janv. 2021 à 14:01, Daniel Vetter <daniel@ffwll.ch> a écrit : > On Wed, Jan 20, 2021 at 1:36 PM Paul Cercueil <paul@crapouillou.net> > 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@vger.kernel.org> # 5.8+ >> Signed-off-by: Paul Cercueil <paul@crapouillou.net> >> --- >> >> 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. -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
Le mer. 20 janv. 2021 à 15:04, Daniel Vetter <daniel@ffwll.ch> a écrit : > On Wed, Jan 20, 2021 at 2:21 PM Paul Cercueil <paul@crapouillou.net> > wrote: >> >> >> >> Le mer. 20 janv. 2021 à 14:01, Daniel Vetter <daniel@ffwll.ch> a >> écrit : >> > On Wed, Jan 20, 2021 at 1:36 PM Paul Cercueil >> <paul@crapouillou.net> >> > 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@vger.kernel.org> # 5.8+ >> >> Signed-off-by: Paul Cercueil <paul@crapouillou.net> >> >> --- >> >> >> >> 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 Alright, I will add a wrapper. Cheers, -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
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, + 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");
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@vger.kernel.org> # 5.8+ Signed-off-by: Paul Cercueil <paul@crapouillou.net> --- 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(-)