All of lore.kernel.org
 help / color / mirror / Atom feed
From: Heiko Stuebner <heiko@sntech.de>
To: Jeffy Chen <jeffy.chen@rock-chips.com>
Cc: linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org,
	Brian Norris <briannorris@chromium.org>,
	Douglas Anderson <dianders@chromium.org>,
	dri-devel@lists.freedesktop.org, Tomasz Figa <tfiga@chromium.org>,
	Guenter Roeck <groeck@chromium.org>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3] drm/rockchip: Refactor the component match logic.
Date: Wed, 15 Mar 2017 18:23:27 +0100	[thread overview]
Message-ID: <2110531.OeyX13mlk5@phil> (raw)
In-Reply-To: <19101003.9xjQ92OS5C@phil>

Am Mittwoch, 15. März 2017, 18:00:04 CET schrieb Heiko Stuebner:
> Am Mittwoch, 15. März 2017, 18:20:47 CET schrieb Jeffy Chen:
> > Currently we are adding all components from the dts, if one of their
> > drivers been disabled, we would not be able to bring up others.
> > 
> > Refactor component match logic, follow exynos drm.
> > 
> > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> 
> This reliably produces null pointer dereference errors in
> 	__platform_driver_register called from rockchip_drm_init
> on at least rk3036 and rk3288 (probably more) when applied on top of
> Linus' tree from today. Log attached and Rockchip drm compiled as module.
> 
> I'm currently dug into other areas, so hadn't have time to investigate
> further yet.

> +#define DRV_PTR(drv, cond) (IS_ENABLED(cond) ? &drv : NULL)
> +
> +static struct platform_driver *rockchip_drm_comp_drvs[] = {
> +	&vop_platform_driver,
> +	DRV_PTR(rockchip_dp_driver, CONFIG_ROCKCHIP_ANALOGIX_DP),
> +	DRV_PTR(cdn_dp_driver, CONFIG_ROCKCHIP_CDN_DP),
> +	DRV_PTR(dw_hdmi_rockchip_pltfm_driver, CONFIG_ROCKCHIP_DW_HDMI),
> +	DRV_PTR(dw_mipi_dsi_driver, CONFIG_ROCKCHIP_DW_MIPI_DSI),
> +	DRV_PTR(inno_hdmi_driver, CONFIG_ROCKCHIP_INNO_HDMI),
> +};

[...]

> +static int rockchip_drm_register_drivers(void)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(rockchip_drm_comp_drvs); i++) {
> +		ret = platform_driver_register(rockchip_drm_comp_drvs[i]);
> +		if (ret)
> +			goto err_unreg;
> +	}

This of course won't work in the NULL case, as platform_driver_register always 
dereferences its parameter [0], so you should only call it for the actual non-
null array elements - of course same for unregister.


Heiko

[0] http://lxr.free-electrons.com/source/drivers/base/platform.c#L617

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

WARNING: multiple messages have this Message-ID (diff)
From: heiko@sntech.de (Heiko Stuebner)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v3] drm/rockchip: Refactor the component match logic.
Date: Wed, 15 Mar 2017 18:23:27 +0100	[thread overview]
Message-ID: <2110531.OeyX13mlk5@phil> (raw)
In-Reply-To: <19101003.9xjQ92OS5C@phil>

Am Mittwoch, 15. M?rz 2017, 18:00:04 CET schrieb Heiko Stuebner:
> Am Mittwoch, 15. M?rz 2017, 18:20:47 CET schrieb Jeffy Chen:
> > Currently we are adding all components from the dts, if one of their
> > drivers been disabled, we would not be able to bring up others.
> > 
> > Refactor component match logic, follow exynos drm.
> > 
> > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> 
> This reliably produces null pointer dereference errors in
> 	__platform_driver_register called from rockchip_drm_init
> on at least rk3036 and rk3288 (probably more) when applied on top of
> Linus' tree from today. Log attached and Rockchip drm compiled as module.
> 
> I'm currently dug into other areas, so hadn't have time to investigate
> further yet.

> +#define DRV_PTR(drv, cond) (IS_ENABLED(cond) ? &drv : NULL)
> +
> +static struct platform_driver *rockchip_drm_comp_drvs[] = {
> +	&vop_platform_driver,
> +	DRV_PTR(rockchip_dp_driver, CONFIG_ROCKCHIP_ANALOGIX_DP),
> +	DRV_PTR(cdn_dp_driver, CONFIG_ROCKCHIP_CDN_DP),
> +	DRV_PTR(dw_hdmi_rockchip_pltfm_driver, CONFIG_ROCKCHIP_DW_HDMI),
> +	DRV_PTR(dw_mipi_dsi_driver, CONFIG_ROCKCHIP_DW_MIPI_DSI),
> +	DRV_PTR(inno_hdmi_driver, CONFIG_ROCKCHIP_INNO_HDMI),
> +};

[...]

> +static int rockchip_drm_register_drivers(void)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(rockchip_drm_comp_drvs); i++) {
> +		ret = platform_driver_register(rockchip_drm_comp_drvs[i]);
> +		if (ret)
> +			goto err_unreg;
> +	}

This of course won't work in the NULL case, as platform_driver_register always 
dereferences its parameter [0], so you should only call it for the actual non-
null array elements - of course same for unregister.


Heiko

[0] http://lxr.free-electrons.com/source/drivers/base/platform.c#L617

WARNING: multiple messages have this Message-ID (diff)
From: Heiko Stuebner <heiko@sntech.de>
To: Jeffy Chen <jeffy.chen@rock-chips.com>
Cc: Mark Yao <mark.yao@rock-chips.com>,
	Andrzej Hajda <a.hajda@samsung.com>,
	Guenter Roeck <groeck@chromium.org>,
	Brian Norris <briannorris@chromium.org>,
	Tomasz Figa <tfiga@chromium.org>,
	Douglas Anderson <dianders@chromium.org>,
	Sean Paul <seanpaul@chromium.org>,
	linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
	linux-rockchip@lists.infradead.org,
	David Airlie <airlied@linux.ie>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3] drm/rockchip: Refactor the component match logic.
Date: Wed, 15 Mar 2017 18:23:27 +0100	[thread overview]
Message-ID: <2110531.OeyX13mlk5@phil> (raw)
In-Reply-To: <19101003.9xjQ92OS5C@phil>

Am Mittwoch, 15. März 2017, 18:00:04 CET schrieb Heiko Stuebner:
> Am Mittwoch, 15. März 2017, 18:20:47 CET schrieb Jeffy Chen:
> > Currently we are adding all components from the dts, if one of their
> > drivers been disabled, we would not be able to bring up others.
> > 
> > Refactor component match logic, follow exynos drm.
> > 
> > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> 
> This reliably produces null pointer dereference errors in
> 	__platform_driver_register called from rockchip_drm_init
> on at least rk3036 and rk3288 (probably more) when applied on top of
> Linus' tree from today. Log attached and Rockchip drm compiled as module.
> 
> I'm currently dug into other areas, so hadn't have time to investigate
> further yet.

> +#define DRV_PTR(drv, cond) (IS_ENABLED(cond) ? &drv : NULL)
> +
> +static struct platform_driver *rockchip_drm_comp_drvs[] = {
> +	&vop_platform_driver,
> +	DRV_PTR(rockchip_dp_driver, CONFIG_ROCKCHIP_ANALOGIX_DP),
> +	DRV_PTR(cdn_dp_driver, CONFIG_ROCKCHIP_CDN_DP),
> +	DRV_PTR(dw_hdmi_rockchip_pltfm_driver, CONFIG_ROCKCHIP_DW_HDMI),
> +	DRV_PTR(dw_mipi_dsi_driver, CONFIG_ROCKCHIP_DW_MIPI_DSI),
> +	DRV_PTR(inno_hdmi_driver, CONFIG_ROCKCHIP_INNO_HDMI),
> +};

[...]

> +static int rockchip_drm_register_drivers(void)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(rockchip_drm_comp_drvs); i++) {
> +		ret = platform_driver_register(rockchip_drm_comp_drvs[i]);
> +		if (ret)
> +			goto err_unreg;
> +	}

This of course won't work in the NULL case, as platform_driver_register always 
dereferences its parameter [0], so you should only call it for the actual non-
null array elements - of course same for unregister.


Heiko

[0] http://lxr.free-electrons.com/source/drivers/base/platform.c#L617

  reply	other threads:[~2017-03-15 17:23 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-15 10:20 [PATCH v3] drm/rockchip: Refactor the component match logic Jeffy Chen
2017-03-15 10:20 ` Jeffy Chen
2017-03-15 13:52 ` Sean Paul
2017-03-15 13:52   ` Sean Paul
2017-03-15 13:52   ` Sean Paul
2017-03-15 17:00 ` Heiko Stuebner
2017-03-15 17:00   ` Heiko Stuebner
2017-03-15 17:00   ` Heiko Stuebner
2017-03-15 17:23   ` Heiko Stuebner [this message]
2017-03-15 17:23     ` Heiko Stuebner
2017-03-15 17:23     ` Heiko Stuebner
2017-03-16  2:07   ` jeffy
2017-03-16  2:07     ` jeffy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2110531.OeyX13mlk5@phil \
    --to=heiko@sntech.de \
    --cc=briannorris@chromium.org \
    --cc=dianders@chromium.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=groeck@chromium.org \
    --cc=jeffy.chen@rock-chips.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=tfiga@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
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.