Re: [PATCH v3] drm/rockchip: Refactor the component match logic.

[jeffy]

Hi Heiko,

On 03/16/2017 01:00 AM, Heiko Stuebner wrote:

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 
Reviewed-by: Andrzej Hajda 


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.


Heiko


oops, sorry, i'll upload a new version to fix that, thanx.

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

Re: [PATCH v3] drm/rockchip: Refactor the component match logic.

[Heiko Stuebner]

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 
> > Reviewed-by: Andrzej Hajda 
> 
> 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) ?  : NULL)
> +
> +static struct platform_driver *rockchip_drm_comp_drvs[] = {
> + _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

Re: [PATCH v3] drm/rockchip: Refactor the component match logic.

[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 
> Reviewed-by: Andrzej Hajda 

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.


Heiko^C
U-Boot SPL 2016.11-01442-g5699f71-dirty (Nov 29 2016 - 23:23:39)


U-Boot 2016.11-01442-g5699f71-dirty (Nov 29 2016 - 23:23:39 +0100)

Model: Firefly-RK3288
DRAM:  2 GiB
MMC:   dwmmc@ff0c: 0, dwmmc@ff0f: 1
Card did not respond to voltage select!
*** Warning - MMC init failed, using default environment

In:serial
Out:   serial
Err:   serial
Net:   
Warning: ethernet@ff29 (eth0) using random MAC address - ca:ff:bf:e6:3b:7d
eth0: ethernet@ff29
Hit any key to stop autoboot:  2  1  0 
ethernet@ff29 Waiting for PHY auto negotiation to complete. done
Speed: 100, full duplex
BOOTP broadcast 1
DHCP client bound to address 192.168.140.51 (15 ms)
Using ethernet@ff29 device
TFTP from server 192.168.140.1; our IP address is 192.168.140.51
Filename 'hstuebner/firefly.vmlinuz'.
Load address: 0x400
Loading: *#
	 #
	 #
	 #
	 #
	 #
	 #
	 ##
	 2.6 MiB/s
done
Bytes transferred = 7350967 (702ab7 hex)
## Loading kernel from FIT Image at 0400 ...
   Using 'conf@1' configuration
   Trying 'kernel@1' kernel subimage
 Description:  Mainline kernel
 Type: Kernel Image
 Compression:  uncompressed
 Data Start:   0x04e4
 Data Size:7311440 Bytes = 7 MiB
 Architecture: ARM
 OS:   Linux
 Load Address: 0x0028
 Entry Point:  0x0028
   Verifying Hash Integrity ... OK
## Loading fdt from FIT Image at 0400 ...
   Using 'conf@1' configuration
   Trying 'fdt@1' fdt subimage
 Description:  RK3288-Firefly
 Type: Flat Device Tree
 Compression:  uncompressed
 Data Start:   0x046f91d8
 Data Size:37834 Bytes = 36.9 KiB
 Architecture: ARM
   Verifying Hash Integrity ... OK
   Booting using the fdt blob at 0x46f91d8
   Loading Kernel Image ... OK
   Loading Device Tree to 0fff3000, end 03c9 ... OK

Starting kernel ...

[0.00] Booting Linux on physical CPU 0x500
[0.00] Linux version 4.11.0-rc2-01650-gaa6c2d83ac0f-dirty (hstuebner@phil) (gcc version 6.2.1 20161124 (Debian 6.2.1-5) ) #234 SMP Wed Mar 15 17:42:52 CET 2017
[0.00] CPU: ARMv7 Processor [410fc0d1] revision 1 (ARMv7), cr=10c5387d
[0.00] CPU: div instructions available: patching division code
[0.00] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing instruction cache
[0.00] OF: fdt: Machine model: Firefly-RK3288
[0.00] efi: Getting EFI parameters from FDT:
[0.00] efi: UEFI not found.
[0.00] cma: Reserved 64 MiB at 0x7c00
[0.00] Memory policy: Data cache writealloc
[0.00] percpu: Embedded 16 pages/cpu @eef98000 s34380 r8192 d22964 u65536
[0.00] Built 1 zonelists in Zone order, mobility grouping on.  Total pages: 522752
[0.00] Kernel command line: earlyprintk console=ttyS2,115200n8 init=/sbin/init ip=dhcp nfsroot=192.168.140.1:/home/devel/nfs/rootfs-firefly root=/dev/nfs rw noinitrd
[0.00] PID hash table entries: 4096 (order: 2, 16384 bytes)
[0.00] Dentry cache hash table entries: 131072 (order: 7, 524288 bytes)
[0.00] Inode-cache hash table entries: 65536 (order: 6, 262144 bytes)
[0.00] Memory: 1994840K/2097152K available (10240K kernel code, 1192K rwdata, 4012K rodata, 2048K init, 535K bss, 36776K reserved, 65536K cma-reserved, 1245184K highmem)
[0.00] Virtual kernel memory layout:
[0.00] vector  : 0x - 0x1000   (   4 kB)
[0.00] fixmap  : 0xffc0 - 0xfff0   (3072 kB)
[0.00] vmalloc : 0xf080 - 0xff80   ( 240 MB)
[0.00] lowmem  : 0xc000 - 0xf000   ( 768 MB)
[0.00] pkmap   : 0xbfe0 - 0xc000   (   2 MB)
[0.00] modules : 0xbf00 - 0xbfe0   (  14 MB)
[0.00]   .text : 

[PATCH v3] drm/rockchip: Refactor the component match logic.

[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 
Reviewed-by: Andrzej Hajda 

---

Changes in v3:
Address Andrzej Hajda 's comments.

Changes in v2:
Address Sean Paul 's comments.

 drivers/gpu/drm/rockchip/Kconfig|  10 +-
 drivers/gpu/drm/rockchip/Makefile   |  16 +--
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c |   9 +-
 drivers/gpu/drm/rockchip/cdn-dp-core.c  |   8 +-
 drivers/gpu/drm/rockchip/dw-mipi-dsi.c  |   8 +-
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c |  10 +-
 drivers/gpu/drm/rockchip/inno_hdmi.c|  10 +-
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 154 
 drivers/gpu/drm/rockchip/rockchip_drm_drv.h |   6 +
 drivers/gpu/drm/rockchip/rockchip_vop_reg.c |   8 +-
 10 files changed, 131 insertions(+), 108 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/Kconfig b/drivers/gpu/drm/rockchip/Kconfig
index 0e4eb84..50c41c0 100644
--- a/drivers/gpu/drm/rockchip/Kconfig
+++ b/drivers/gpu/drm/rockchip/Kconfig
@@ -13,7 +13,7 @@ config DRM_ROCKCHIP
  IP found on the SoC.
 
 config ROCKCHIP_ANALOGIX_DP
-   tristate "Rockchip specific extensions for Analogix DP driver"
+   bool "Rockchip specific extensions for Analogix DP driver"
depends on DRM_ROCKCHIP
select DRM_ANALOGIX_DP
help
@@ -22,7 +22,7 @@ config ROCKCHIP_ANALOGIX_DP
  on RK3288 based SoC, you should selet this option.
 
 config ROCKCHIP_CDN_DP
-tristate "Rockchip cdn DP"
+bool "Rockchip cdn DP"
 depends on DRM_ROCKCHIP
depends on EXTCON
select SND_SOC_HDMI_CODEC if SND_SOC
@@ -33,7 +33,7 @@ config ROCKCHIP_CDN_DP
  option.
 
 config ROCKCHIP_DW_HDMI
-tristate "Rockchip specific extensions for Synopsys DW HDMI"
+bool "Rockchip specific extensions for Synopsys DW HDMI"
 depends on DRM_ROCKCHIP
 select DRM_DW_HDMI
 help
@@ -43,7 +43,7 @@ config ROCKCHIP_DW_HDMI
  option.
 
 config ROCKCHIP_DW_MIPI_DSI
-   tristate "Rockchip specific extensions for Synopsys DW MIPI DSI"
+   bool "Rockchip specific extensions for Synopsys DW MIPI DSI"
depends on DRM_ROCKCHIP
select DRM_MIPI_DSI
help
@@ -53,7 +53,7 @@ config ROCKCHIP_DW_MIPI_DSI
 option.
 
 config ROCKCHIP_INNO_HDMI
-   tristate "Rockchip specific extensions for Innosilicon HDMI"
+   bool "Rockchip specific extensions for Innosilicon HDMI"
depends on DRM_ROCKCHIP
help
  This selects support for Rockchip SoC specific extensions
diff --git a/drivers/gpu/drm/rockchip/Makefile 
b/drivers/gpu/drm/rockchip/Makefile
index c931e2a..fa8dc9d 100644
--- a/drivers/gpu/drm/rockchip/Makefile
+++ b/drivers/gpu/drm/rockchip/Makefile
@@ -3,14 +3,14 @@
 # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
 
 rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \
-   rockchip_drm_gem.o rockchip_drm_psr.o rockchip_drm_vop.o
+   rockchip_drm_gem.o rockchip_drm_psr.o \
+   rockchip_drm_vop.o rockchip_vop_reg.o
 rockchipdrm-$(CONFIG_DRM_FBDEV_EMULATION) += rockchip_drm_fbdev.o
 
-obj-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o
-obj-$(CONFIG_ROCKCHIP_CDN_DP) += cdn-dp.o
-cdn-dp-objs := cdn-dp-core.o cdn-dp-reg.o
-obj-$(CONFIG_ROCKCHIP_DW_HDMI) += dw_hdmi-rockchip.o
-obj-$(CONFIG_ROCKCHIP_DW_MIPI_DSI) += dw-mipi-dsi.o
-obj-$(CONFIG_ROCKCHIP_INNO_HDMI) += inno_hdmi.o
+rockchipdrm-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o
+rockchipdrm-$(CONFIG_ROCKCHIP_CDN_DP) += cdn-dp-core.o cdn-dp-reg.o
+rockchipdrm-$(CONFIG_ROCKCHIP_DW_HDMI) += dw_hdmi-rockchip.o
+rockchipdrm-$(CONFIG_ROCKCHIP_DW_MIPI_DSI) += dw-mipi-dsi.o
+rockchipdrm-$(CONFIG_ROCKCHIP_INNO_HDMI) += inno_hdmi.o
 
-obj-$(CONFIG_DRM_ROCKCHIP) += rockchipdrm.o rockchip_vop_reg.o
+obj-$(CONFIG_DRM_ROCKCHIP) += rockchipdrm.o
diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c 
b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index 8548e82..91ebe5c 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -507,7 +507,7 @@ static const struct of_device_id rockchip_dp_dt_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, rockchip_dp_dt_ids);
 
-static struct platform_driver rockchip_dp_driver = {
+struct platform_driver rockchip_dp_driver = {
.probe = rockchip_dp_probe,
.remove = rockchip_dp_remove,
.driver = {
@@ -516,10 +516,3 @@ static struct platform_driver rockchip_dp_driver = {
   .of_match_table = of_match_ptr(rockchip_dp_dt_ids),
},
 };
-
-module_platform_driver(rockchip_dp_driver);
-
-MODULE_AUTHOR("Yakir Yang 

Re: [PATCH v3] drm/rockchip: Refactor the component match logic.

[Sean Paul]

On Wed, Mar 15, 2017 at 06:20:47PM +0800, Jeffy Chen wrote:
> 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 
> Reviewed-by: Andrzej Hajda 

Still looks good to me. If Mark doesn't object in the next 24 hours, I'll apply.

Sean

> 
> ---
> 
> Changes in v3:
> Address Andrzej Hajda 's comments.
> 
> Changes in v2:
> Address Sean Paul 's comments.
> 
>  drivers/gpu/drm/rockchip/Kconfig|  10 +-
>  drivers/gpu/drm/rockchip/Makefile   |  16 +--
>  drivers/gpu/drm/rockchip/analogix_dp-rockchip.c |   9 +-
>  drivers/gpu/drm/rockchip/cdn-dp-core.c  |   8 +-
>  drivers/gpu/drm/rockchip/dw-mipi-dsi.c  |   8 +-
>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c |  10 +-
>  drivers/gpu/drm/rockchip/inno_hdmi.c|  10 +-
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 154 
> 
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.h |   6 +
>  drivers/gpu/drm/rockchip/rockchip_vop_reg.c |   8 +-
>  10 files changed, 131 insertions(+), 108 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/Kconfig 
> b/drivers/gpu/drm/rockchip/Kconfig
> index 0e4eb84..50c41c0 100644
> --- a/drivers/gpu/drm/rockchip/Kconfig
> +++ b/drivers/gpu/drm/rockchip/Kconfig
> @@ -13,7 +13,7 @@ config DRM_ROCKCHIP
> IP found on the SoC.
>  
>  config ROCKCHIP_ANALOGIX_DP
> - tristate "Rockchip specific extensions for Analogix DP driver"
> + bool "Rockchip specific extensions for Analogix DP driver"
>   depends on DRM_ROCKCHIP
>   select DRM_ANALOGIX_DP
>   help
> @@ -22,7 +22,7 @@ config ROCKCHIP_ANALOGIX_DP
> on RK3288 based SoC, you should selet this option.
>  
>  config ROCKCHIP_CDN_DP
> -tristate "Rockchip cdn DP"
> +bool "Rockchip cdn DP"
>  depends on DRM_ROCKCHIP
>   depends on EXTCON
>   select SND_SOC_HDMI_CODEC if SND_SOC
> @@ -33,7 +33,7 @@ config ROCKCHIP_CDN_DP
> option.
>  
>  config ROCKCHIP_DW_HDMI
> -tristate "Rockchip specific extensions for Synopsys DW HDMI"
> +bool "Rockchip specific extensions for Synopsys DW HDMI"
>  depends on DRM_ROCKCHIP
>  select DRM_DW_HDMI
>  help
> @@ -43,7 +43,7 @@ config ROCKCHIP_DW_HDMI
> option.
>  
>  config ROCKCHIP_DW_MIPI_DSI
> - tristate "Rockchip specific extensions for Synopsys DW MIPI DSI"
> + bool "Rockchip specific extensions for Synopsys DW MIPI DSI"
>   depends on DRM_ROCKCHIP
>   select DRM_MIPI_DSI
>   help
> @@ -53,7 +53,7 @@ config ROCKCHIP_DW_MIPI_DSI
>option.
>  
>  config ROCKCHIP_INNO_HDMI
> - tristate "Rockchip specific extensions for Innosilicon HDMI"
> + bool "Rockchip specific extensions for Innosilicon HDMI"
>   depends on DRM_ROCKCHIP
>   help
> This selects support for Rockchip SoC specific extensions
> diff --git a/drivers/gpu/drm/rockchip/Makefile 
> b/drivers/gpu/drm/rockchip/Makefile
> index c931e2a..fa8dc9d 100644
> --- a/drivers/gpu/drm/rockchip/Makefile
> +++ b/drivers/gpu/drm/rockchip/Makefile
> @@ -3,14 +3,14 @@
>  # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
>  
>  rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \
> - rockchip_drm_gem.o rockchip_drm_psr.o rockchip_drm_vop.o
> + rockchip_drm_gem.o rockchip_drm_psr.o \
> + rockchip_drm_vop.o rockchip_vop_reg.o
>  rockchipdrm-$(CONFIG_DRM_FBDEV_EMULATION) += rockchip_drm_fbdev.o
>  
> -obj-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o
> -obj-$(CONFIG_ROCKCHIP_CDN_DP) += cdn-dp.o
> -cdn-dp-objs := cdn-dp-core.o cdn-dp-reg.o
> -obj-$(CONFIG_ROCKCHIP_DW_HDMI) += dw_hdmi-rockchip.o
> -obj-$(CONFIG_ROCKCHIP_DW_MIPI_DSI) += dw-mipi-dsi.o
> -obj-$(CONFIG_ROCKCHIP_INNO_HDMI) += inno_hdmi.o
> +rockchipdrm-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o
> +rockchipdrm-$(CONFIG_ROCKCHIP_CDN_DP) += cdn-dp-core.o cdn-dp-reg.o
> +rockchipdrm-$(CONFIG_ROCKCHIP_DW_HDMI) += dw_hdmi-rockchip.o
> +rockchipdrm-$(CONFIG_ROCKCHIP_DW_MIPI_DSI) += dw-mipi-dsi.o
> +rockchipdrm-$(CONFIG_ROCKCHIP_INNO_HDMI) += inno_hdmi.o
>  
> -obj-$(CONFIG_DRM_ROCKCHIP) += rockchipdrm.o rockchip_vop_reg.o
> +obj-$(CONFIG_DRM_ROCKCHIP) += rockchipdrm.o
> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c 
> b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> index 8548e82..91ebe5c 100644
> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> @@ -507,7 +507,7 @@ static const struct of_device_id rockchip_dp_dt_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(of, rockchip_dp_dt_ids);
>  
> -static struct platform_driver rockchip_dp_driver = {
> +struct platform_driver rockchip_dp_driver = {
>