* [PATCH 1/1] drm/sun4i: hdmi: Check for null pointer before cleanup @ 2019-12-16 14:43 ` Stefan Mavrodiev 0 siblings, 0 replies; 24+ messages in thread From: Stefan Mavrodiev @ 2019-12-16 14:43 UTC (permalink / raw) To: Maxime Ripard, Chen-Yu Tsai, David Airlie, Daniel Vetter, open list:DRM DRIVERS FOR ALLWINNER A10, moderated list:ARM/Allwinner sunXi SoC support, open list Cc: linux-sunxi, Stefan Mavrodiev 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@olimex.com> --- 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); i2c_del_adapter(hdmi->i2c); i2c_put_adapter(hdmi->ddc_i2c); clk_disable_unprepare(hdmi->mod_clk); -- 2.17.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 1/1] drm/sun4i: hdmi: Check for null pointer before cleanup @ 2019-12-16 14:43 ` Stefan Mavrodiev 0 siblings, 0 replies; 24+ messages in thread From: Stefan Mavrodiev @ 2019-12-16 14:43 UTC (permalink / raw) To: Maxime Ripard, Chen-Yu Tsai, David Airlie, Daniel Vetter, open list:DRM DRIVERS FOR ALLWINNER A10, moderated list:ARM/Allwinner sunXi SoC support, open list Cc: linux-sunxi, Stefan Mavrodiev 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@olimex.com> --- 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); i2c_del_adapter(hdmi->i2c); i2c_put_adapter(hdmi->ddc_i2c); clk_disable_unprepare(hdmi->mod_clk); -- 2.17.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 1/1] drm/sun4i: hdmi: Check for null pointer before cleanup @ 2019-12-16 14:43 ` Stefan Mavrodiev 0 siblings, 0 replies; 24+ messages in thread From: Stefan Mavrodiev @ 2019-12-16 14:43 UTC (permalink / raw) To: Maxime Ripard, Chen-Yu Tsai, David Airlie, Daniel Vetter, open list:DRM DRIVERS FOR ALLWINNER A10, moderated list:ARM/Allwinner sunXi SoC support, open list Cc: linux-sunxi, Stefan Mavrodiev 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@olimex.com> --- 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); i2c_del_adapter(hdmi->i2c); i2c_put_adapter(hdmi->ddc_i2c); clk_disable_unprepare(hdmi->mod_clk); -- 2.17.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 1/1] drm/sun4i: hdmi: Check for null pointer before cleanup 2019-12-16 14:43 ` Stefan Mavrodiev (?) @ 2019-12-16 16:12 ` Maxime Ripard -1 siblings, 0 replies; 24+ messages in thread From: Maxime Ripard @ 2019-12-16 16:12 UTC (permalink / raw) To: Stefan Mavrodiev Cc: Chen-Yu Tsai, David Airlie, Daniel Vetter, open list:DRM DRIVERS FOR ALLWINNER A10, moderated list:ARM/Allwinner sunXi SoC support, open list, linux-sunxi [-- Attachment #1: Type: text/plain, Size: 1224 bytes --] 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@olimex.com> > --- > 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? Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/1] drm/sun4i: hdmi: Check for null pointer before cleanup @ 2019-12-16 16:12 ` Maxime Ripard 0 siblings, 0 replies; 24+ messages in thread From: Maxime Ripard @ 2019-12-16 16:12 UTC (permalink / raw) To: Stefan Mavrodiev Cc: David Airlie, linux-sunxi, open list, open list:DRM DRIVERS FOR ALLWINNER A10, Chen-Yu Tsai, moderated list:ARM/Allwinner sunXi SoC support [-- Attachment #1.1: Type: text/plain, Size: 1224 bytes --] 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@olimex.com> > --- > 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? Maxime [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/1] drm/sun4i: hdmi: Check for null pointer before cleanup @ 2019-12-16 16:12 ` Maxime Ripard 0 siblings, 0 replies; 24+ messages in thread From: Maxime Ripard @ 2019-12-16 16:12 UTC (permalink / raw) To: Stefan Mavrodiev Cc: David Airlie, linux-sunxi, open list, open list:DRM DRIVERS FOR ALLWINNER A10, Chen-Yu Tsai, Daniel Vetter, moderated list:ARM/Allwinner sunXi SoC support [-- Attachment #1.1: Type: text/plain, Size: 1224 bytes --] 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@olimex.com> > --- > 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? Maxime [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/1] drm/sun4i: hdmi: Check for null pointer before cleanup 2019-12-16 16:12 ` Maxime Ripard (?) @ 2019-12-17 6:45 ` Stefan Mavrodiev -1 siblings, 0 replies; 24+ messages in thread From: Stefan Mavrodiev @ 2019-12-17 6:45 UTC (permalink / raw) To: Maxime Ripard Cc: Stefan Mavrodiev, Chen-Yu Tsai, David Airlie, Daniel Vetter, open list:DRM DRIVERS FOR ALLWINNER A10, moderated list:ARM/Allwinner sunXi SoC support, open list, linux-sunxi 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@olimex.com> >> --- >> 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. Best regards, Stefan > > Maxime ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/1] drm/sun4i: hdmi: Check for null pointer before cleanup @ 2019-12-17 6:45 ` Stefan Mavrodiev 0 siblings, 0 replies; 24+ messages in thread From: Stefan Mavrodiev @ 2019-12-17 6:45 UTC (permalink / raw) To: Maxime Ripard Cc: Stefan Mavrodiev, David Airlie, linux-sunxi, open list, open list:DRM DRIVERS FOR ALLWINNER A10, Chen-Yu Tsai, moderated list:ARM/Allwinner sunXi SoC support 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@olimex.com> >> --- >> 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. Best regards, Stefan > > Maxime _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/1] drm/sun4i: hdmi: Check for null pointer before cleanup @ 2019-12-17 6:45 ` Stefan Mavrodiev 0 siblings, 0 replies; 24+ messages in thread From: Stefan Mavrodiev @ 2019-12-17 6:45 UTC (permalink / raw) To: Maxime Ripard Cc: Stefan Mavrodiev, David Airlie, linux-sunxi, open list, open list:DRM DRIVERS FOR ALLWINNER A10, Chen-Yu Tsai, Daniel Vetter, moderated list:ARM/Allwinner sunXi SoC support 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@olimex.com> >> --- >> 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. Best regards, Stefan > > Maxime _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/1] drm/sun4i: hdmi: Check for null pointer before cleanup 2019-12-17 6:45 ` Stefan Mavrodiev (?) @ 2019-12-17 11:46 ` Maxime Ripard -1 siblings, 0 replies; 24+ messages in thread From: Maxime Ripard @ 2019-12-17 11:46 UTC (permalink / raw) To: Stefan Mavrodiev Cc: Chen-Yu Tsai, David Airlie, Daniel Vetter, open list:DRM DRIVERS FOR ALLWINNER A10, moderated list:ARM/Allwinner sunXi SoC support, open list, linux-sunxi [-- Attachment #1: Type: text/plain, Size: 2392 bytes --] 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@olimex.com> > > > --- > > > 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? Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/1] drm/sun4i: hdmi: Check for null pointer before cleanup @ 2019-12-17 11:46 ` Maxime Ripard 0 siblings, 0 replies; 24+ messages in thread From: Maxime Ripard @ 2019-12-17 11:46 UTC (permalink / raw) To: Stefan Mavrodiev Cc: David Airlie, linux-sunxi, open list, open list:DRM DRIVERS FOR ALLWINNER A10, Chen-Yu Tsai, moderated list:ARM/Allwinner sunXi SoC support [-- Attachment #1.1: Type: text/plain, Size: 2392 bytes --] 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@olimex.com> > > > --- > > > 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? Maxime [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/1] drm/sun4i: hdmi: Check for null pointer before cleanup @ 2019-12-17 11:46 ` Maxime Ripard 0 siblings, 0 replies; 24+ messages in thread From: Maxime Ripard @ 2019-12-17 11:46 UTC (permalink / raw) To: Stefan Mavrodiev Cc: David Airlie, linux-sunxi, open list, open list:DRM DRIVERS FOR ALLWINNER A10, Chen-Yu Tsai, Daniel Vetter, moderated list:ARM/Allwinner sunXi SoC support [-- Attachment #1.1: Type: text/plain, Size: 2392 bytes --] 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@olimex.com> > > > --- > > > 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? Maxime [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/1] drm/sun4i: hdmi: Check for null pointer before cleanup 2019-12-17 11:46 ` Maxime Ripard (?) @ 2019-12-17 11:49 ` Maxime Ripard -1 siblings, 0 replies; 24+ messages in thread From: Maxime Ripard @ 2019-12-17 11:49 UTC (permalink / raw) To: Stefan Mavrodiev Cc: Chen-Yu Tsai, David Airlie, Daniel Vetter, open list:DRM DRIVERS FOR ALLWINNER A10, moderated list:ARM/Allwinner sunXi SoC support, open list, linux-sunxi [-- Attachment #1: Type: text/plain, Size: 2709 bytes --] 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@olimex.com> > > > > --- > > > > 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. Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/1] drm/sun4i: hdmi: Check for null pointer before cleanup @ 2019-12-17 11:49 ` Maxime Ripard 0 siblings, 0 replies; 24+ messages in thread From: Maxime Ripard @ 2019-12-17 11:49 UTC (permalink / raw) To: Stefan Mavrodiev Cc: David Airlie, linux-sunxi, open list, open list:DRM DRIVERS FOR ALLWINNER A10, Chen-Yu Tsai, moderated list:ARM/Allwinner sunXi SoC support [-- Attachment #1.1: Type: text/plain, Size: 2709 bytes --] 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@olimex.com> > > > > --- > > > > 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. Maxime [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/1] drm/sun4i: hdmi: Check for null pointer before cleanup @ 2019-12-17 11:49 ` Maxime Ripard 0 siblings, 0 replies; 24+ messages in thread From: Maxime Ripard @ 2019-12-17 11:49 UTC (permalink / raw) To: Stefan Mavrodiev Cc: David Airlie, linux-sunxi, open list, open list:DRM DRIVERS FOR ALLWINNER A10, Chen-Yu Tsai, Daniel Vetter, moderated list:ARM/Allwinner sunXi SoC support [-- Attachment #1.1: Type: text/plain, Size: 2709 bytes --] 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@olimex.com> > > > > --- > > > > 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. Maxime [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/1] drm/sun4i: hdmi: Check for null pointer before cleanup 2019-12-17 11:49 ` Maxime Ripard (?) @ 2019-12-17 11:54 ` Stefan Mavrodiev -1 siblings, 0 replies; 24+ messages in thread From: Stefan Mavrodiev @ 2019-12-17 11:54 UTC (permalink / raw) To: Maxime Ripard Cc: Stefan Mavrodiev, Chen-Yu Tsai, David Airlie, Daniel Vetter, open list:DRM DRIVERS FOR ALLWINNER A10, moderated list:ARM/Allwinner sunXi SoC support, open list, linux-sunxi 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@olimex.com> >>>>> --- >>>>> 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? Stefan > > Maxime ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/1] drm/sun4i: hdmi: Check for null pointer before cleanup @ 2019-12-17 11:54 ` Stefan Mavrodiev 0 siblings, 0 replies; 24+ messages in thread From: Stefan Mavrodiev @ 2019-12-17 11:54 UTC (permalink / raw) To: Maxime Ripard Cc: Stefan Mavrodiev, David Airlie, linux-sunxi, open list, open list:DRM DRIVERS FOR ALLWINNER A10, Chen-Yu Tsai, moderated list:ARM/Allwinner sunXi SoC support 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@olimex.com> >>>>> --- >>>>> 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? Stefan > > Maxime _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/1] drm/sun4i: hdmi: Check for null pointer before cleanup @ 2019-12-17 11:54 ` Stefan Mavrodiev 0 siblings, 0 replies; 24+ messages in thread From: Stefan Mavrodiev @ 2019-12-17 11:54 UTC (permalink / raw) To: Maxime Ripard Cc: Stefan Mavrodiev, David Airlie, linux-sunxi, open list, open list:DRM DRIVERS FOR ALLWINNER A10, Chen-Yu Tsai, Daniel Vetter, moderated list:ARM/Allwinner sunXi SoC support 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@olimex.com> >>>>> --- >>>>> 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? Stefan > > Maxime _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/1] drm/sun4i: hdmi: Check for null pointer before cleanup 2019-12-17 11:54 ` Stefan Mavrodiev (?) @ 2019-12-17 11:57 ` Maxime Ripard -1 siblings, 0 replies; 24+ messages in thread From: Maxime Ripard @ 2019-12-17 11:57 UTC (permalink / raw) To: Stefan Mavrodiev Cc: Chen-Yu Tsai, David Airlie, Daniel Vetter, open list:DRM DRIVERS FOR ALLWINNER A10, moderated list:ARM/Allwinner sunXi SoC support, open list, linux-sunxi [-- Attachment #1: Type: text/plain, Size: 3186 bytes --] 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@olimex.com> > > > > > > --- > > > > > > 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 #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/1] drm/sun4i: hdmi: Check for null pointer before cleanup @ 2019-12-17 11:57 ` Maxime Ripard 0 siblings, 0 replies; 24+ messages in thread From: Maxime Ripard @ 2019-12-17 11:57 UTC (permalink / raw) To: Stefan Mavrodiev Cc: David Airlie, linux-sunxi, open list, open list:DRM DRIVERS FOR ALLWINNER A10, Chen-Yu Tsai, moderated list:ARM/Allwinner sunXi SoC support [-- Attachment #1.1: Type: text/plain, Size: 3186 bytes --] 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@olimex.com> > > > > > > --- > > > > > > 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 #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/1] drm/sun4i: hdmi: Check for null pointer before cleanup @ 2019-12-17 11:57 ` Maxime Ripard 0 siblings, 0 replies; 24+ messages in thread From: Maxime Ripard @ 2019-12-17 11:57 UTC (permalink / raw) To: Stefan Mavrodiev Cc: David Airlie, linux-sunxi, open list, open list:DRM DRIVERS FOR ALLWINNER A10, Chen-Yu Tsai, Daniel Vetter, moderated list:ARM/Allwinner sunXi SoC support [-- Attachment #1.1: Type: text/plain, Size: 3186 bytes --] 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@olimex.com> > > > > > > --- > > > > > > 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 #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/1] drm/sun4i: hdmi: Check for null pointer before cleanup 2019-12-16 14:43 ` Stefan Mavrodiev (?) @ 2019-12-17 7:26 ` Uwe Kleine-König -1 siblings, 0 replies; 24+ messages in thread From: Uwe Kleine-König @ 2019-12-17 7:26 UTC (permalink / raw) To: Stefan Mavrodiev Cc: Maxime Ripard, Chen-Yu Tsai, David Airlie, Daniel Vetter, open list:DRM DRIVERS FOR ALLWINNER A10, moderated list:ARM/Allwinner sunXi SoC support, open list, linux-sunxi 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. The wording is broken and s/divices/devices/. I'd write: It's possible that the parent devices of the connector and/or encoder are NULL. This makes drm_connector_cleanup and drm_encoder_cleanup respectively trigger a NULL pointer exeception: <...log here...> This is reproducible by <...receipt here...> So add a check for NULL before calling these functions. Of course this doesn't address the reservations by Maxime. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/1] drm/sun4i: hdmi: Check for null pointer before cleanup @ 2019-12-17 7:26 ` Uwe Kleine-König 0 siblings, 0 replies; 24+ messages in thread From: Uwe Kleine-König @ 2019-12-17 7:26 UTC (permalink / raw) To: Stefan Mavrodiev Cc: David Airlie, linux-sunxi, open list, open list:DRM DRIVERS FOR ALLWINNER A10, Chen-Yu Tsai, moderated list:ARM/Allwinner sunXi SoC support 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. The wording is broken and s/divices/devices/. I'd write: It's possible that the parent devices of the connector and/or encoder are NULL. This makes drm_connector_cleanup and drm_encoder_cleanup respectively trigger a NULL pointer exeception: <...log here...> This is reproducible by <...receipt here...> So add a check for NULL before calling these functions. Of course this doesn't address the reservations by Maxime. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/1] drm/sun4i: hdmi: Check for null pointer before cleanup @ 2019-12-17 7:26 ` Uwe Kleine-König 0 siblings, 0 replies; 24+ messages in thread From: Uwe Kleine-König @ 2019-12-17 7:26 UTC (permalink / raw) To: Stefan Mavrodiev Cc: David Airlie, linux-sunxi, open list, open list:DRM DRIVERS FOR ALLWINNER A10, Chen-Yu Tsai, Maxime Ripard, Daniel Vetter, moderated list:ARM/Allwinner sunXi SoC support 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. The wording is broken and s/divices/devices/. I'd write: It's possible that the parent devices of the connector and/or encoder are NULL. This makes drm_connector_cleanup and drm_encoder_cleanup respectively trigger a NULL pointer exeception: <...log here...> This is reproducible by <...receipt here...> So add a check for NULL before calling these functions. Of course this doesn't address the reservations by Maxime. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2019-12-18 8:13 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-16 14:43 [PATCH 1/1] drm/sun4i: hdmi: Check for null pointer before cleanup Stefan Mavrodiev 2019-12-16 14:43 ` Stefan Mavrodiev 2019-12-16 14:43 ` Stefan Mavrodiev 2019-12-16 16:12 ` Maxime Ripard 2019-12-16 16:12 ` Maxime Ripard 2019-12-16 16:12 ` Maxime Ripard 2019-12-17 6:45 ` Stefan Mavrodiev 2019-12-17 6:45 ` Stefan Mavrodiev 2019-12-17 6:45 ` Stefan Mavrodiev 2019-12-17 11:46 ` Maxime Ripard 2019-12-17 11:46 ` Maxime Ripard 2019-12-17 11:46 ` Maxime Ripard 2019-12-17 11:49 ` Maxime Ripard 2019-12-17 11:49 ` Maxime Ripard 2019-12-17 11:49 ` Maxime Ripard 2019-12-17 11:54 ` Stefan Mavrodiev 2019-12-17 11:54 ` Stefan Mavrodiev 2019-12-17 11:54 ` Stefan Mavrodiev 2019-12-17 11:57 ` Maxime Ripard 2019-12-17 11:57 ` Maxime Ripard 2019-12-17 11:57 ` Maxime Ripard 2019-12-17 7:26 ` Uwe Kleine-König 2019-12-17 7:26 ` Uwe Kleine-König 2019-12-17 7:26 ` Uwe Kleine-König
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.