On Tue, Dec 17, 2019 at 01:54:56PM +0200, Stefan Mavrodiev wrote: > Hi, > > On 12/17/19 1:49 PM, Maxime Ripard wrote: > > On Tue, Dec 17, 2019 at 12:46:03PM +0100, Maxime Ripard wrote: > > > On Tue, Dec 17, 2019 at 08:45:07AM +0200, Stefan Mavrodiev wrote: > > > > Hi, > > > > > > > > On 12/16/19 6:12 PM, Maxime Ripard wrote: > > > > > Hi, > > > > > > > > > > On Mon, Dec 16, 2019 at 04:43:48PM +0200, Stefan Mavrodiev wrote: > > > > > > It's possible hdmi->connector and hdmi->encoder divices to be NULL. > > > > > > This can happen when building as kernel module and you try to remove > > > > > > the module. > > > > > > > > > > > > This patch make simple null check, before calling the cleanup functions. > > > > > > > > > > > > Signed-off-by: Stefan Mavrodiev <stefan@xxxxxxxxxx> > > > > > > --- > > > > > > drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 6 ++++-- > > > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c > > > > > > index a7c4654445c7..b61e00f2ecb8 100644 > > > > > > --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c > > > > > > +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c > > > > > > @@ -685,8 +685,10 @@ static void sun4i_hdmi_unbind(struct device *dev, struct device *master, > > > > > > struct sun4i_hdmi *hdmi = dev_get_drvdata(dev); > > > > > > > > > > > > cec_unregister_adapter(hdmi->cec_adap); > > > > > > - drm_connector_cleanup(&hdmi->connector); > > > > > > - drm_encoder_cleanup(&hdmi->encoder); > > > > > > + if (hdmi->connector.dev) > > > > > > + drm_connector_cleanup(&hdmi->connector); > > > > > > + if (hdmi->encoder.dev) > > > > > > + drm_encoder_cleanup(&hdmi->encoder); > > > > > Hmmm, this doesn't look right. Do you have more information on how you > > > > > can reproduce it? > > > > Just build sun4i_drm_hdmi as module (CONFIG_DRM_SUN4I_HDMI=m). Then try to > > > > unload the module: > > > > > > > > # rmmod sun4i_drm_hdmi > > > > > > > > And you get this: > > > > > > > > Unable to handle kernel NULL pointer dereference at virtual address 00000000 > > > > pgd = 6b032436 > > > > [00000000] *pgd=00000000 > > > > Internal error: Oops: 5 [#1] SMP ARM > > > > Modules linked in: sun4i_drm_hdmi(-) > > > > CPU: 0 PID: 1081 Comm: rmmod Not tainted 5.5.0-rc1-00030-g6ec417030d93 #33 > > > > Hardware name: Allwinner sun7i (A20) Family > > > > PC is at drm_connector_cleanup+0x40/0x208 > > > > LR is at sun4i_hdmi_unbind+0x10/0x54 [sun4i_drm_hdmi] > > > > ... > > > > > > > > > > > > I've tested that with sunxi/for-next branch on A20-OLinuXino board. > > > Yeah, you detailed the symptoms nicely in the commit log, but my point > > > was that we shouldn't end up in that situation in the first place. > > > > > > Your patch works around it, but it doesn't fix the underlying > > > issue. Is drm_connector_cleanup (or the encoder variant) called twice? > > Answering myself, yes it is. It's both the destroy call back and > > called in unbind. We should just remove the one in the unbind then. > > Should I do this or leave it to you? You started that discussion, so it's only fair that you do the patch :) Maxime
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
