[bug report] drm/sun4i: Handle DRM_BUS_FLAG_PIXDATA_*EDGE checking if panel is used.

[bug report] drm/sun4i: Handle DRM_BUS_FLAG_PIXDATA_*EDGE checking if panel is used.

[Dan Carpenter]

Hello Giulio Benetti,

The patch 490cda5a3c82: "drm/sun4i: Handle DRM_BUS_FLAG_PIXDATA_*EDGE
checking if panel is used." from Jul 18, 2018, leads to the following
static checker warning:

	drivers/gpu/drm/sun4i/sun4i_tcon.c:558 sun4i_tcon0_mode_set_rgb()
	warn: 'tcon->panel' isn't an ERR_PTR

drivers/gpu/drm/sun4i/sun4i_tcon.c
   481                                       const struct drm_display_mode *mode)
   482  {
   483          unsigned int bp, hsync, vsync;
   484          u8 clk_delay;
   485          u32 val = 0;
   486  
   487          WARN_ON(!tcon->quirks->has_channel_0);
   488  
   489          tcon->dclk_min_div = 6;
   490          tcon->dclk_max_div = 127;
   491          sun4i_tcon0_mode_set_common(tcon, mode);
   492  
   493          /* Set dithering if needed */
   494          sun4i_tcon0_mode_set_dithering(tcon, tcon->panel->connector);
                                                     ^^^^^^^^^^^^^
Dereference.

   495  
   496          /* Adjust clock delay */
   497          clk_delay = sun4i_tcon_get_clk_delay(mode, 0);
   498          regmap_update_bits(tcon->regs, SUN4I_TCON0_CTL_REG,
   499                             SUN4I_TCON0_CTL_CLK_DELAY_MASK,
   500                             SUN4I_TCON0_CTL_CLK_DELAY(clk_delay));
   501  
   502          /*
   503           * This is called a backporch in the register documentation,
   504           * but it really is the back porch + hsync
   505           */
   506          bp = mode->crtc_htotal - mode->crtc_hsync_start;
   507          DRM_DEBUG_DRIVER("Setting horizontal total %d, backporch %d\n",
   508                           mode->crtc_htotal, bp);
   509  
   510          /* Set horizontal display timings */
   511          regmap_write(tcon->regs, SUN4I_TCON0_BASIC1_REG,
   512                       SUN4I_TCON0_BASIC1_H_TOTAL(mode->crtc_htotal) |
   513                       SUN4I_TCON0_BASIC1_H_BACKPORCH(bp));
   514  
   515          /*
   516           * This is called a backporch in the register documentation,
   517           * but it really is the back porch + hsync
   518           */
   519          bp = mode->crtc_vtotal - mode->crtc_vsync_start;
   520          DRM_DEBUG_DRIVER("Setting vertical total %d, backporch %d\n",
   521                           mode->crtc_vtotal, bp);
   522  
   523          /* Set vertical display timings */
   524          regmap_write(tcon->regs, SUN4I_TCON0_BASIC2_REG,
   525                       SUN4I_TCON0_BASIC2_V_TOTAL(mode->crtc_vtotal * 2) |
   526                       SUN4I_TCON0_BASIC2_V_BACKPORCH(bp));
   527  
   528          /* Set Hsync and Vsync length */
   529          hsync = mode->crtc_hsync_end - mode->crtc_hsync_start;
   530          vsync = mode->crtc_vsync_end - mode->crtc_vsync_start;
   531          DRM_DEBUG_DRIVER("Setting HSYNC %d, VSYNC %d\n", hsync, vsync);
   532          regmap_write(tcon->regs, SUN4I_TCON0_BASIC3_REG,
   533                       SUN4I_TCON0_BASIC3_V_SYNC(vsync) |
   534                       SUN4I_TCON0_BASIC3_H_SYNC(hsync));
   535  
   536          /* Setup the polarity of the various signals */
   537          if (mode->flags & DRM_MODE_FLAG_PHSYNC)
   538                  val |= SUN4I_TCON0_IO_POL_HSYNC_POSITIVE;
   539  
   540          if (mode->flags & DRM_MODE_FLAG_PVSYNC)
   541                  val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
   542  
   543          /*
   544           * On A20 and similar SoCs, the only way to achieve Positive Edge
   545           * (Rising Edge), is setting dclk clock phase to 2/3(240°).
   546           * By default TCON works in Negative Edge(Falling Edge),
   547           * this is why phase is set to 0 in that case.
   548           * Unfortunately there's no way to logically invert dclk through
   549           * IO_POL register.
   550           * The only acceptable way to work, triple checked with scope,
   551           * is using clock phase set to 0° for Negative Edge and set to 240°
   552           * for Positive Edge.
   553           * On A33 and similar SoCs there would be a 90° phase option,
   554           * but it divides also dclk by 2.
   555           * Following code is a way to avoid quirks all around TCON
   556           * and DOTCLOCK drivers.
   557           */
   558          if (!IS_ERR(tcon->panel)) {
                     ^^^^^^^^^^^^^^^^^^
Unpossible to be an error pointer!

   559                  struct drm_panel *panel = tcon->panel;
   560                  struct drm_connector *connector = panel->connector;
   561                  struct drm_display_info display_info = connector->display_info;
   562  
   563                  if (display_info.bus_flags & DRM_BUS_FLAG_PIXDATA_POSEDGE)

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

Re: [bug report] drm/sun4i: Handle DRM_BUS_FLAG_PIXDATA_*EDGE checking if panel is used.

[Giulio Benetti]

Il 01/10/2018 11:36, Dan Carpenter ha scritto:
> Hello Giulio Benetti,
> 
> The patch 490cda5a3c82: "drm/sun4i: Handle DRM_BUS_FLAG_PIXDATA_*EDGE
> checking if panel is used." from Jul 18, 2018, leads to the following
> static checker warning:
> 	drivers/gpu/drm/sun4i/sun4i_tcon.c:558 sun4i_tcon0_mode_set_rgb()
> 	warn: 'tcon->panel' isn't an ERR_PTR
> 
> drivers/gpu/drm/sun4i/sun4i_tcon.c
>     481                                       const struct drm_display_mode *mode)
>     482  {
>     483          unsigned int bp, hsync, vsync;
>     484          u8 clk_delay;
>     485          u32 val = 0;
>     486
>     487          WARN_ON(!tcon->quirks->has_channel_0);
>     488
>     489          tcon->dclk_min_div = 6;
>     490          tcon->dclk_max_div = 127;
>     491          sun4i_tcon0_mode_set_common(tcon, mode);
>     492
>     493          /* Set dithering if needed */
>     494          sun4i_tcon0_mode_set_dithering(tcon, tcon->panel->connector);
>                                                       ^^^^^^^^^^^^^

Here there should be something like:
if (!tcon->panel) {
	sun4i_tcon0_mode_set_dithering(tcon, tcon->panel->connector);
}

>     495
>     496          /* Adjust clock delay */
>     497          clk_delay = sun4i_tcon_get_clk_delay(mode, 0);
>     498          regmap_update_bits(tcon->regs, SUN4I_TCON0_CTL_REG,
>     499                             SUN4I_TCON0_CTL_CLK_DELAY_MASK,
>     500                             SUN4I_TCON0_CTL_CLK_DELAY(clk_delay));
>     501
>     502          /*
>     503           * This is called a backporch in the register documentation,
>     504           * but it really is the back porch + hsync
>     505           */
>     506          bp = mode->crtc_htotal - mode->crtc_hsync_start;
>     507          DRM_DEBUG_DRIVER("Setting horizontal total %d, backporch %d\n",
>     508                           mode->crtc_htotal, bp);
>     509
>     510          /* Set horizontal display timings */
>     511          regmap_write(tcon->regs, SUN4I_TCON0_BASIC1_REG,
>     512                       SUN4I_TCON0_BASIC1_H_TOTAL(mode->crtc_htotal) |
>     513                       SUN4I_TCON0_BASIC1_H_BACKPORCH(bp));
>     514
>     515          /*
>     516           * This is called a backporch in the register documentation,
>     517           * but it really is the back porch + hsync
>     518           */
>     519          bp = mode->crtc_vtotal - mode->crtc_vsync_start;
>     520          DRM_DEBUG_DRIVER("Setting vertical total %d, backporch %d\n",
>     521                           mode->crtc_vtotal, bp);
>     522
>     523          /* Set vertical display timings */
>     524          regmap_write(tcon->regs, SUN4I_TCON0_BASIC2_REG,
>     525                       SUN4I_TCON0_BASIC2_V_TOTAL(mode->crtc_vtotal * 2) |
>     526                       SUN4I_TCON0_BASIC2_V_BACKPORCH(bp));
>     527
>     528          /* Set Hsync and Vsync length */
>     529          hsync = mode->crtc_hsync_end - mode->crtc_hsync_start;
>     530          vsync = mode->crtc_vsync_end - mode->crtc_vsync_start;
>     531          DRM_DEBUG_DRIVER("Setting HSYNC %d, VSYNC %d\n", hsync, vsync);
>     532          regmap_write(tcon->regs, SUN4I_TCON0_BASIC3_REG,
>     533                       SUN4I_TCON0_BASIC3_V_SYNC(vsync) |
>     534                       SUN4I_TCON0_BASIC3_H_SYNC(hsync));
>     535
>     536          /* Setup the polarity of the various signals */
>     537          if (mode->flags & DRM_MODE_FLAG_PHSYNC)
>     538                  val |= SUN4I_TCON0_IO_POL_HSYNC_POSITIVE;
>     539
>     540          if (mode->flags & DRM_MODE_FLAG_PVSYNC)
>     541                  val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
>     542
>     543          /*
>     544           * On A20 and similar SoCs, the only way to achieve Positive Edge
>     545           * (Rising Edge), is setting dclk clock phase to 2/3(240°).
>     546           * By default TCON works in Negative Edge(Falling Edge),
>     547           * this is why phase is set to 0 in that case.
>     548           * Unfortunately there's no way to logically invert dclk through
>     549           * IO_POL register.
>     550           * The only acceptable way to work, triple checked with scope,
>     551           * is using clock phase set to 0° for Negative Edge and set to 240°
>     552           * for Positive Edge.
>     553           * On A33 and similar SoCs there would be a 90° phase option,
>     554           * but it divides also dclk by 2.
>     555           * Following code is a way to avoid quirks all around TCON
>     556           * and DOTCLOCK drivers.
>     557           */
>     558          if (!IS_ERR(tcon->panel)) {
>                       ^^^^^^^^^^^^^^^^^^
> Unpossible to be an error pointer!

So maybe I should use IS_ERR_OR_NULL() instead of IS_ERR(), or maybe simply:
if (!tcon->panel) {

Right?

Anyway I'm going to test it with "sparse".
Is it the same tool you've used for this?

Thanks

Kind regards
-- 
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale € 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [bug report] drm/sun4i: Handle DRM_BUS_FLAG_PIXDATA_*EDGE checking if panel is used.

[Giulio Benetti]

Il 02/10/2018 15:26, Giulio Benetti ha scritto:
> Il 01/10/2018 11:36, Dan Carpenter ha scritto:
>> Hello Giulio Benetti,
>>
>> The patch 490cda5a3c82: "drm/sun4i: Handle DRM_BUS_FLAG_PIXDATA_*EDGE
>> checking if panel is used." from Jul 18, 2018, leads to the following
>> static checker warning:
>> 	drivers/gpu/drm/sun4i/sun4i_tcon.c:558 sun4i_tcon0_mode_set_rgb()
>> 	warn: 'tcon->panel' isn't an ERR_PTR
>>
>> drivers/gpu/drm/sun4i/sun4i_tcon.c
>>      481                                       const struct drm_display_mode *mode)
>>      482  {
>>      483          unsigned int bp, hsync, vsync;
>>      484          u8 clk_delay;
>>      485          u32 val = 0;
>>      486
>>      487          WARN_ON(!tcon->quirks->has_channel_0);
>>      488
>>      489          tcon->dclk_min_div = 6;
>>      490          tcon->dclk_max_div = 127;
>>      491          sun4i_tcon0_mode_set_common(tcon, mode);
>>      492
>>      493          /* Set dithering if needed */
>>      494          sun4i_tcon0_mode_set_dithering(tcon, tcon->panel->connector);
>>                                                        ^^^^^^^^^^^^^
> 
> Here there should be something like:
> if (!tcon->panel) {
> 	sun4i_tcon0_mode_set_dithering(tcon, tcon->panel->connector);
> }
> 
>>      495
>>      496          /* Adjust clock delay */
>>      497          clk_delay = sun4i_tcon_get_clk_delay(mode, 0);
>>      498          regmap_update_bits(tcon->regs, SUN4I_TCON0_CTL_REG,
>>      499                             SUN4I_TCON0_CTL_CLK_DELAY_MASK,
>>      500                             SUN4I_TCON0_CTL_CLK_DELAY(clk_delay));
>>      501
>>      502          /*
>>      503           * This is called a backporch in the register documentation,
>>      504           * but it really is the back porch + hsync
>>      505           */
>>      506          bp = mode->crtc_htotal - mode->crtc_hsync_start;
>>      507          DRM_DEBUG_DRIVER("Setting horizontal total %d, backporch %d\n",
>>      508                           mode->crtc_htotal, bp);
>>      509
>>      510          /* Set horizontal display timings */
>>      511          regmap_write(tcon->regs, SUN4I_TCON0_BASIC1_REG,
>>      512                       SUN4I_TCON0_BASIC1_H_TOTAL(mode->crtc_htotal) |
>>      513                       SUN4I_TCON0_BASIC1_H_BACKPORCH(bp));
>>      514
>>      515          /*
>>      516           * This is called a backporch in the register documentation,
>>      517           * but it really is the back porch + hsync
>>      518           */
>>      519          bp = mode->crtc_vtotal - mode->crtc_vsync_start;
>>      520          DRM_DEBUG_DRIVER("Setting vertical total %d, backporch %d\n",
>>      521                           mode->crtc_vtotal, bp);
>>      522
>>      523          /* Set vertical display timings */
>>      524          regmap_write(tcon->regs, SUN4I_TCON0_BASIC2_REG,
>>      525                       SUN4I_TCON0_BASIC2_V_TOTAL(mode->crtc_vtotal * 2) |
>>      526                       SUN4I_TCON0_BASIC2_V_BACKPORCH(bp));
>>      527
>>      528          /* Set Hsync and Vsync length */
>>      529          hsync = mode->crtc_hsync_end - mode->crtc_hsync_start;
>>      530          vsync = mode->crtc_vsync_end - mode->crtc_vsync_start;
>>      531          DRM_DEBUG_DRIVER("Setting HSYNC %d, VSYNC %d\n", hsync, vsync);
>>      532          regmap_write(tcon->regs, SUN4I_TCON0_BASIC3_REG,
>>      533                       SUN4I_TCON0_BASIC3_V_SYNC(vsync) |
>>      534                       SUN4I_TCON0_BASIC3_H_SYNC(hsync));
>>      535
>>      536          /* Setup the polarity of the various signals */
>>      537          if (mode->flags & DRM_MODE_FLAG_PHSYNC)
>>      538                  val |= SUN4I_TCON0_IO_POL_HSYNC_POSITIVE;
>>      539
>>      540          if (mode->flags & DRM_MODE_FLAG_PVSYNC)
>>      541                  val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
>>      542
>>      543          /*
>>      544           * On A20 and similar SoCs, the only way to achieve Positive Edge
>>      545           * (Rising Edge), is setting dclk clock phase to 2/3(240°).
>>      546           * By default TCON works in Negative Edge(Falling Edge),
>>      547           * this is why phase is set to 0 in that case.
>>      548           * Unfortunately there's no way to logically invert dclk through
>>      549           * IO_POL register.
>>      550           * The only acceptable way to work, triple checked with scope,
>>      551           * is using clock phase set to 0° for Negative Edge and set to 240°
>>      552           * for Positive Edge.
>>      553           * On A33 and similar SoCs there would be a 90° phase option,
>>      554           * but it divides also dclk by 2.
>>      555           * Following code is a way to avoid quirks all around TCON
>>      556           * and DOTCLOCK drivers.
>>      557           */
>>      558          if (!IS_ERR(tcon->panel)) {
>>                        ^^^^^^^^^^^^^^^^^^
>> Unpossible to be an error pointer!
> 
> So maybe I should use IS_ERR_OR_NULL() instead of IS_ERR(), or maybe simply:
> if (!tcon->panel) {
> 
> Right?
> 
> Anyway I'm going to test it with "sparse".
> Is it the same tool you've used for this?

You use "smatch", now I'm reproducing the same warnings and going to fix 
them.

Best regards
-- 
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale € 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [bug report] drm/sun4i: Handle DRM_BUS_FLAG_PIXDATA_*EDGE checking if panel is used.

[Giulio Benetti]

Il 02/10/2018 16:12, Giulio Benetti ha scritto:
> Il 02/10/2018 15:26, Giulio Benetti ha scritto:
>> Il 01/10/2018 11:36, Dan Carpenter ha scritto:
>>> Hello Giulio Benetti,
>>>
>>> The patch 490cda5a3c82: "drm/sun4i: Handle DRM_BUS_FLAG_PIXDATA_*EDGE
>>> checking if panel is used." from Jul 18, 2018, leads to the following
>>> static checker warning:
>>> 	drivers/gpu/drm/sun4i/sun4i_tcon.c:558 sun4i_tcon0_mode_set_rgb()
>>> 	warn: 'tcon->panel' isn't an ERR_PTR
>>>
>>> drivers/gpu/drm/sun4i/sun4i_tcon.c
>>>       481                                       const struct drm_display_mode *mode)
>>>       482  {
>>>       483          unsigned int bp, hsync, vsync;
>>>       484          u8 clk_delay;
>>>       485          u32 val = 0;
>>>       486
>>>       487          WARN_ON(!tcon->quirks->has_channel_0);
>>>       488
>>>       489          tcon->dclk_min_div = 6;
>>>       490          tcon->dclk_max_div = 127;
>>>       491          sun4i_tcon0_mode_set_common(tcon, mode);
>>>       492
>>>       493          /* Set dithering if needed */
>>>       494          sun4i_tcon0_mode_set_dithering(tcon, tcon->panel->connector);
>>>                                                         ^^^^^^^^^^^^^
>>
>> Here there should be something like:
>> if (!tcon->panel) {
>> 	sun4i_tcon0_mode_set_dithering(tcon, tcon->panel->connector);
>> }
>>
>>>       495
>>>       496          /* Adjust clock delay */
>>>       497          clk_delay = sun4i_tcon_get_clk_delay(mode, 0);
>>>       498          regmap_update_bits(tcon->regs, SUN4I_TCON0_CTL_REG,
>>>       499                             SUN4I_TCON0_CTL_CLK_DELAY_MASK,
>>>       500                             SUN4I_TCON0_CTL_CLK_DELAY(clk_delay));
>>>       501
>>>       502          /*
>>>       503           * This is called a backporch in the register documentation,
>>>       504           * but it really is the back porch + hsync
>>>       505           */
>>>       506          bp = mode->crtc_htotal - mode->crtc_hsync_start;
>>>       507          DRM_DEBUG_DRIVER("Setting horizontal total %d, backporch %d\n",
>>>       508                           mode->crtc_htotal, bp);
>>>       509
>>>       510          /* Set horizontal display timings */
>>>       511          regmap_write(tcon->regs, SUN4I_TCON0_BASIC1_REG,
>>>       512                       SUN4I_TCON0_BASIC1_H_TOTAL(mode->crtc_htotal) |
>>>       513                       SUN4I_TCON0_BASIC1_H_BACKPORCH(bp));
>>>       514
>>>       515          /*
>>>       516           * This is called a backporch in the register documentation,
>>>       517           * but it really is the back porch + hsync
>>>       518           */
>>>       519          bp = mode->crtc_vtotal - mode->crtc_vsync_start;
>>>       520          DRM_DEBUG_DRIVER("Setting vertical total %d, backporch %d\n",
>>>       521                           mode->crtc_vtotal, bp);
>>>       522
>>>       523          /* Set vertical display timings */
>>>       524          regmap_write(tcon->regs, SUN4I_TCON0_BASIC2_REG,
>>>       525                       SUN4I_TCON0_BASIC2_V_TOTAL(mode->crtc_vtotal * 2) |
>>>       526                       SUN4I_TCON0_BASIC2_V_BACKPORCH(bp));
>>>       527
>>>       528          /* Set Hsync and Vsync length */
>>>       529          hsync = mode->crtc_hsync_end - mode->crtc_hsync_start;
>>>       530          vsync = mode->crtc_vsync_end - mode->crtc_vsync_start;
>>>       531          DRM_DEBUG_DRIVER("Setting HSYNC %d, VSYNC %d\n", hsync, vsync);
>>>       532          regmap_write(tcon->regs, SUN4I_TCON0_BASIC3_REG,
>>>       533                       SUN4I_TCON0_BASIC3_V_SYNC(vsync) |
>>>       534                       SUN4I_TCON0_BASIC3_H_SYNC(hsync));
>>>       535
>>>       536          /* Setup the polarity of the various signals */
>>>       537          if (mode->flags & DRM_MODE_FLAG_PHSYNC)
>>>       538                  val |= SUN4I_TCON0_IO_POL_HSYNC_POSITIVE;
>>>       539
>>>       540          if (mode->flags & DRM_MODE_FLAG_PVSYNC)
>>>       541                  val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
>>>       542
>>>       543          /*
>>>       544           * On A20 and similar SoCs, the only way to achieve Positive Edge
>>>       545           * (Rising Edge), is setting dclk clock phase to 2/3(240°).
>>>       546           * By default TCON works in Negative Edge(Falling Edge),
>>>       547           * this is why phase is set to 0 in that case.
>>>       548           * Unfortunately there's no way to logically invert dclk through
>>>       549           * IO_POL register.
>>>       550           * The only acceptable way to work, triple checked with scope,
>>>       551           * is using clock phase set to 0° for Negative Edge and set to 240°
>>>       552           * for Positive Edge.
>>>       553           * On A33 and similar SoCs there would be a 90° phase option,
>>>       554           * but it divides also dclk by 2.
>>>       555           * Following code is a way to avoid quirks all around TCON
>>>       556           * and DOTCLOCK drivers.
>>>       557           */
>>>       558          if (!IS_ERR(tcon->panel)) {
>>>                         ^^^^^^^^^^^^^^^^^^
>>> Unpossible to be an error pointer!
>>
>> So maybe I should use IS_ERR_OR_NULL() instead of IS_ERR(), or maybe simply:
>> if (!tcon->panel) {
>>
>> Right?
>>
>> Anyway I'm going to test it with "sparse".
>> Is it the same tool you've used for this?
> 
> You use "smatch", now I'm reproducing the same warnings and going to fix
> them.

Sorry but I can't reproduce your log.
Which util did you use for that?

Thanks in advance
-- 
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale € 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] drm/sun4i: tcon: fix check of tcon->panel null pointer

[Giulio Benetti]

At the moment, the check of tcon->panel to be valid is wrong. IS_ERR()
has been used, but that macro doesn't check if tcon->panel pointer is
null or not, but check if tcon->panel is between -1 and -4095(MAX_ERRNO).

Remove IS_ERR() from tcon->panel checking and let "if (tcon->panel)" as
condition to check if it's a pointer not null.

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index c78cd35a1294..e4b3bd0307ef 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -555,7 +555,7 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
 	 * Following code is a way to avoid quirks all around TCON
 	 * and DOTCLOCK drivers.
 	 */
-	if (!IS_ERR(tcon->panel)) {
+	if (tcon->panel) {
 		struct drm_panel *panel = tcon->panel;
 		struct drm_connector *connector = panel->connector;
 		struct drm_display_info display_info = connector->display_info;
-- 
2.17.1

[PATCH 1/2] drm/sun4i: tcon: fix check of tcon->panel null pointer

[Giulio Benetti]

At the moment, the check of tcon->panel to be valid is wrong. IS_ERR()
has been used, but that macro doesn't check if tcon->panel pointer is
null or not, but check if tcon->panel is between -1 and -4095(MAX_ERRNO).

Remove IS_ERR() from tcon->panel checking and let "if (tcon->panel)" as
condition to check if it's a pointer not null.

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index c78cd35a1294..e4b3bd0307ef 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -555,7 +555,7 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
 	 * Following code is a way to avoid quirks all around TCON
 	 * and DOTCLOCK drivers.
 	 */
-	if (!IS_ERR(tcon->panel)) {
+	if (tcon->panel) {
 		struct drm_panel *panel = tcon->panel;
 		struct drm_connector *connector = panel->connector;
 		struct drm_display_info display_info = connector->display_info;
-- 
2.17.1

[PATCH 2/2] drm/sun4i: tcon: prevent tcon->panel dereference if null

[Giulio Benetti]

If using tcon with VGA, tcon->panel will be null(0), this will cause
segmentation fault when trying to dereference tcon->panel->connector.

Add tcon->panel null check before calling
sun4i_tcon0_mode_set_dithering().

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index e4b3bd0307ef..f949287d926c 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -491,7 +491,8 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
 	sun4i_tcon0_mode_set_common(tcon, mode);
 
 	/* Set dithering if needed */
-	sun4i_tcon0_mode_set_dithering(tcon, tcon->panel->connector);
+	if (tcon->panel)
+		sun4i_tcon0_mode_set_dithering(tcon, tcon->panel->connector);
 
 	/* Adjust clock delay */
 	clk_delay = sun4i_tcon_get_clk_delay(mode, 0);
-- 
2.17.1

[PATCH 2/2] drm/sun4i: tcon: prevent tcon->panel dereference if null

[Giulio Benetti]

If using tcon with VGA, tcon->panel will be null(0), this will cause
segmentation fault when trying to dereference tcon->panel->connector.

Add tcon->panel null check before calling
sun4i_tcon0_mode_set_dithering().

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index e4b3bd0307ef..f949287d926c 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -491,7 +491,8 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
 	sun4i_tcon0_mode_set_common(tcon, mode);
 
 	/* Set dithering if needed */
-	sun4i_tcon0_mode_set_dithering(tcon, tcon->panel->connector);
+	if (tcon->panel)
+		sun4i_tcon0_mode_set_dithering(tcon, tcon->panel->connector);
 
 	/* Adjust clock delay */
 	clk_delay = sun4i_tcon_get_clk_delay(mode, 0);
-- 
2.17.1

[PATCH 1/2] drm/sun4i: tcon: fix check of tcon->panel null pointer

[Giulio Benetti]

At the moment, the check of tcon->panel to be valid is wrong. IS_ERR()
has been used, but that macro doesn't check if tcon->panel pointer is
null or not, but check if tcon->panel is between -1 and -4095(MAX_ERRNO).

Remove IS_ERR() from tcon->panel checking and let "if (tcon->panel)" as
condition to check if it's a pointer not null.

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index c78cd35a1294..e4b3bd0307ef 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -555,7 +555,7 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
 	 * Following code is a way to avoid quirks all around TCON
 	 * and DOTCLOCK drivers.
 	 */
-	if (!IS_ERR(tcon->panel)) {
+	if (tcon->panel) {
 		struct drm_panel *panel = tcon->panel;
 		struct drm_connector *connector = panel->connector;
 		struct drm_display_info display_info = connector->display_info;
-- 
2.17.1

[PATCH 1/2] drm/sun4i: tcon: fix check of tcon->panel null pointer

[Giulio Benetti]

At the moment, the check of tcon->panel to be valid is wrong. IS_ERR()
has been used, but that macro doesn't check if tcon->panel pointer is
null or not, but check if tcon->panel is between -1 and -4095(MAX_ERRNO).

Remove IS_ERR() from tcon->panel checking and let "if (tcon->panel)" as
condition to check if it's a pointer not null.

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index c78cd35a1294..e4b3bd0307ef 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -555,7 +555,7 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
 	 * Following code is a way to avoid quirks all around TCON
 	 * and DOTCLOCK drivers.
 	 */
-	if (!IS_ERR(tcon->panel)) {
+	if (tcon->panel) {
 		struct drm_panel *panel = tcon->panel;
 		struct drm_connector *connector = panel->connector;
 		struct drm_display_info display_info = connector->display_info;
-- 
2.17.1

[PATCH 1/2] drm/sun4i: tcon: fix check of tcon->panel null pointer

[Giulio Benetti]

At the moment, the check of tcon->panel to be valid is wrong. IS_ERR()
has been used, but that macro doesn't check if tcon->panel pointer is
null or not, but check if tcon->panel is between -1 and -4095(MAX_ERRNO).

Remove IS_ERR() from tcon->panel checking and let "if (tcon->panel)" as
condition to check if it's a pointer not null.

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index c78cd35a1294..e4b3bd0307ef 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -555,7 +555,7 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
 	 * Following code is a way to avoid quirks all around TCON
 	 * and DOTCLOCK drivers.
 	 */
-	if (!IS_ERR(tcon->panel)) {
+	if (tcon->panel) {
 		struct drm_panel *panel = tcon->panel;
 		struct drm_connector *connector = panel->connector;
 		struct drm_display_info display_info = connector->display_info;
-- 
2.17.1

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

[PATCH 2/2] drm/sun4i: tcon: prevent tcon->panel dereference if null

[Giulio Benetti]

If using tcon with VGA, tcon->panel will be null(0), this will cause
segmentation fault when trying to dereference tcon->panel->connector.

Add tcon->panel null check before calling
sun4i_tcon0_mode_set_dithering().

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index e4b3bd0307ef..f949287d926c 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -491,7 +491,8 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
 	sun4i_tcon0_mode_set_common(tcon, mode);
 
 	/* Set dithering if needed */
-	sun4i_tcon0_mode_set_dithering(tcon, tcon->panel->connector);
+	if (tcon->panel)
+		sun4i_tcon0_mode_set_dithering(tcon, tcon->panel->connector);
 
 	/* Adjust clock delay */
 	clk_delay = sun4i_tcon_get_clk_delay(mode, 0);
-- 
2.17.1

[PATCH 2/2] drm/sun4i: tcon: prevent tcon->panel dereference if null

[Giulio Benetti]

If using tcon with VGA, tcon->panel will be null(0), this will cause
segmentation fault when trying to dereference tcon->panel->connector.

Add tcon->panel null check before calling
sun4i_tcon0_mode_set_dithering().

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index e4b3bd0307ef..f949287d926c 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -491,7 +491,8 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
 	sun4i_tcon0_mode_set_common(tcon, mode);
 
 	/* Set dithering if needed */
-	sun4i_tcon0_mode_set_dithering(tcon, tcon->panel->connector);
+	if (tcon->panel)
+		sun4i_tcon0_mode_set_dithering(tcon, tcon->panel->connector);
 
 	/* Adjust clock delay */
 	clk_delay = sun4i_tcon_get_clk_delay(mode, 0);
-- 
2.17.1

Re: [PATCH 1/2] drm/sun4i: tcon: fix check of tcon->panel null pointer

[Giulio Benetti]

Sorry for sending twice(and top posting), but I was not subscribed to 
dri-devel ML, so patchwork was not aware of these 2 patches.

Best regards
-- 
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale € 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

Il 02/10/2018 23:59, Giulio Benetti ha scritto:
> At the moment, the check of tcon->panel to be valid is wrong. IS_ERR()
> has been used, but that macro doesn't check if tcon->panel pointer is
> null or not, but check if tcon->panel is between -1 and -4095(MAX_ERRNO).
> 
> Remove IS_ERR() from tcon->panel checking and let "if (tcon->panel)" as
> condition to check if it's a pointer not null.
> 
> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> ---
>   drivers/gpu/drm/sun4i/sun4i_tcon.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index c78cd35a1294..e4b3bd0307ef 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -555,7 +555,7 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
>   	 * Following code is a way to avoid quirks all around TCON
>   	 * and DOTCLOCK drivers.
>   	 */
> -	if (!IS_ERR(tcon->panel)) {
> +	if (tcon->panel) {
>   		struct drm_panel *panel = tcon->panel;
>   		struct drm_connector *connector = panel->connector;
>   		struct drm_display_info display_info = connector->display_info;
> 

[PATCH 1/2] drm/sun4i: tcon: fix check of tcon->panel null pointer

[Giulio Benetti]

Sorry for sending twice(and top posting), but I was not subscribed to 
dri-devel ML, so patchwork was not aware of these 2 patches.

Best regards
-- 
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale ? 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

Il 02/10/2018 23:59, Giulio Benetti ha scritto:
> At the moment, the check of tcon->panel to be valid is wrong. IS_ERR()
> has been used, but that macro doesn't check if tcon->panel pointer is
> null or not, but check if tcon->panel is between -1 and -4095(MAX_ERRNO).
> 
> Remove IS_ERR() from tcon->panel checking and let "if (tcon->panel)" as
> condition to check if it's a pointer not null.
> 
> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> ---
>   drivers/gpu/drm/sun4i/sun4i_tcon.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index c78cd35a1294..e4b3bd0307ef 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -555,7 +555,7 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
>   	 * Following code is a way to avoid quirks all around TCON
>   	 * and DOTCLOCK drivers.
>   	 */
> -	if (!IS_ERR(tcon->panel)) {
> +	if (tcon->panel) {
>   		struct drm_panel *panel = tcon->panel;
>   		struct drm_connector *connector = panel->connector;
>   		struct drm_display_info display_info = connector->display_info;
> 

Re: [PATCH 1/2] drm/sun4i: tcon: fix check of tcon->panel null pointer

[Giulio Benetti]

Sorry for sending twice(and top posting), but I was not subscribed to 
dri-devel ML, so patchwork was not aware of these 2 patches.

Best regards
-- 
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale € 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

Il 02/10/2018 23:59, Giulio Benetti ha scritto:
> At the moment, the check of tcon->panel to be valid is wrong. IS_ERR()
> has been used, but that macro doesn't check if tcon->panel pointer is
> null or not, but check if tcon->panel is between -1 and -4095(MAX_ERRNO).
> 
> Remove IS_ERR() from tcon->panel checking and let "if (tcon->panel)" as
> condition to check if it's a pointer not null.
> 
> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> ---
>   drivers/gpu/drm/sun4i/sun4i_tcon.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index c78cd35a1294..e4b3bd0307ef 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -555,7 +555,7 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
>   	 * Following code is a way to avoid quirks all around TCON
>   	 * and DOTCLOCK drivers.
>   	 */
> -	if (!IS_ERR(tcon->panel)) {
> +	if (tcon->panel) {
>   		struct drm_panel *panel = tcon->panel;
>   		struct drm_connector *connector = panel->connector;
>   		struct drm_display_info display_info = connector->display_info;
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/sun4i: tcon: prevent tcon->panel dereference if null

[Chen-Yu Tsai]

On Wed, Oct 3, 2018 at 5:59 AM Giulio Benetti
<giulio.benetti@micronovasrl.com> wrote:
>
> If using tcon with VGA, tcon->panel will be null(0), this will cause
> segmentation fault when trying to dereference tcon->panel->connector.
>
> Add tcon->panel null check before calling
> sun4i_tcon0_mode_set_dithering().
>
> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>

Fixes: f11adcecbd5f ("drm/sun4i: tcon: Add dithering support for
                      RGB565/RGB666 LCD panels")

Reviewed-by: Chen-Yu Tsai <wens@csie.org>

[PATCH 2/2] drm/sun4i: tcon: prevent tcon->panel dereference if null

[Chen-Yu Tsai]

On Wed, Oct 3, 2018 at 5:59 AM Giulio Benetti
<giulio.benetti@micronovasrl.com> wrote:
>
> If using tcon with VGA, tcon->panel will be null(0), this will cause
> segmentation fault when trying to dereference tcon->panel->connector.
>
> Add tcon->panel null check before calling
> sun4i_tcon0_mode_set_dithering().
>
> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>

Fixes: f11adcecbd5f ("drm/sun4i: tcon: Add dithering support for
                      RGB565/RGB666 LCD panels")

Reviewed-by: Chen-Yu Tsai <wens@csie.org>

Re: [PATCH 1/2] drm/sun4i: tcon: fix check of tcon->panel null pointer

[Chen-Yu Tsai]

On Wed, Oct 3, 2018 at 5:59 AM Giulio Benetti
<giulio.benetti@micronovasrl.com> wrote:
>
> At the moment, the check of tcon->panel to be valid is wrong. IS_ERR()
> has been used, but that macro doesn't check if tcon->panel pointer is
> null or not, but check if tcon->panel is between -1 and -4095(MAX_ERRNO).
>
> Remove IS_ERR() from tcon->panel checking and let "if (tcon->panel)" as
> condition to check if it's a pointer not null.

There's actually more than one occurance of this error:

drivers/gpu/drm/sun4i/sun4i_lvds.c:     if (!IS_ERR(tcon->panel)) {
drivers/gpu/drm/sun4i/sun4i_lvds.c:     if (!IS_ERR(tcon->panel)) {
drivers/gpu/drm/sun4i/sun4i_rgb.c:      if (!IS_ERR(tcon->panel)) {
drivers/gpu/drm/sun4i/sun4i_rgb.c:      if (!IS_ERR(tcon->panel)) {

These four are responsible for enabling and disabling the panel.

drivers/gpu/drm/sun4i/sun4i_tcon.c:     if (!IS_ERR(tcon->panel)) {

All this looks like it was left over from commit ebc9446135671 ("drm: convert
drivers to use drm_of_find_panel_or_bridge"). We have checks against tcon->panel
in several places and not all of them were converted.

ChenYu

> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> ---
>  drivers/gpu/drm/sun4i/sun4i_tcon.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index c78cd35a1294..e4b3bd0307ef 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -555,7 +555,7 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
>          * Following code is a way to avoid quirks all around TCON
>          * and DOTCLOCK drivers.
>          */
> -       if (!IS_ERR(tcon->panel)) {
> +       if (tcon->panel) {
>                 struct drm_panel *panel = tcon->panel;
>                 struct drm_connector *connector = panel->connector;
>                 struct drm_display_info display_info = connector->display_info;
> --
> 2.17.1
>

[PATCH 1/2] drm/sun4i: tcon: fix check of tcon->panel null pointer

[Chen-Yu Tsai]

On Wed, Oct 3, 2018 at 5:59 AM Giulio Benetti
<giulio.benetti@micronovasrl.com> wrote:
>
> At the moment, the check of tcon->panel to be valid is wrong. IS_ERR()
> has been used, but that macro doesn't check if tcon->panel pointer is
> null or not, but check if tcon->panel is between -1 and -4095(MAX_ERRNO).
>
> Remove IS_ERR() from tcon->panel checking and let "if (tcon->panel)" as
> condition to check if it's a pointer not null.

There's actually more than one occurance of this error:

drivers/gpu/drm/sun4i/sun4i_lvds.c:     if (!IS_ERR(tcon->panel)) {
drivers/gpu/drm/sun4i/sun4i_lvds.c:     if (!IS_ERR(tcon->panel)) {
drivers/gpu/drm/sun4i/sun4i_rgb.c:      if (!IS_ERR(tcon->panel)) {
drivers/gpu/drm/sun4i/sun4i_rgb.c:      if (!IS_ERR(tcon->panel)) {

These four are responsible for enabling and disabling the panel.

drivers/gpu/drm/sun4i/sun4i_tcon.c:     if (!IS_ERR(tcon->panel)) {

All this looks like it was left over from commit ebc9446135671 ("drm: convert
drivers to use drm_of_find_panel_or_bridge"). We have checks against tcon->panel
in several places and not all of them were converted.

ChenYu

> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> ---
>  drivers/gpu/drm/sun4i/sun4i_tcon.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index c78cd35a1294..e4b3bd0307ef 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -555,7 +555,7 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
>          * Following code is a way to avoid quirks all around TCON
>          * and DOTCLOCK drivers.
>          */
> -       if (!IS_ERR(tcon->panel)) {
> +       if (tcon->panel) {
>                 struct drm_panel *panel = tcon->panel;
>                 struct drm_connector *connector = panel->connector;
>                 struct drm_display_info display_info = connector->display_info;
> --
> 2.17.1
>

Re: [PATCH 1/2] drm/sun4i: tcon: fix check of tcon->panel null pointer

[Giulio Benetti]

Il 03/10/2018 11:43, Chen-Yu Tsai ha scritto:
> On Wed, Oct 3, 2018 at 5:59 AM Giulio Benetti
> <giulio.benetti@micronovasrl.com> wrote:
>>
>> At the moment, the check of tcon->panel to be valid is wrong. IS_ERR()
>> has been used, but that macro doesn't check if tcon->panel pointer is
>> null or not, but check if tcon->panel is between -1 and -4095(MAX_ERRNO).
>>
>> Remove IS_ERR() from tcon->panel checking and let "if (tcon->panel)" as
>> condition to check if it's a pointer not null.
> 
> There's actually more than one occurance of this error:
> 
> drivers/gpu/drm/sun4i/sun4i_lvds.c:     if (!IS_ERR(tcon->panel)) {
> drivers/gpu/drm/sun4i/sun4i_lvds.c:     if (!IS_ERR(tcon->panel)) {
> drivers/gpu/drm/sun4i/sun4i_rgb.c:      if (!IS_ERR(tcon->panel)) {
> drivers/gpu/drm/sun4i/sun4i_rgb.c:      if (!IS_ERR(tcon->panel)) {
> 
> These four are responsible for enabling and disabling the panel.
> 
> drivers/gpu/drm/sun4i/sun4i_tcon.c:     if (!IS_ERR(tcon->panel)) {
> 
> All this looks like it was left over from commit ebc9446135671 ("drm: convert
> drivers to use drm_of_find_panel_or_bridge"). We have checks against tcon->panel
> in several places and not all of them were converted.
> 

Then, I'm going to send a patchset to correct them.

Best regards
-- 
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale € 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

[PATCH 1/2] drm/sun4i: tcon: fix check of tcon->panel null pointer

[Giulio Benetti]

Il 03/10/2018 11:43, Chen-Yu Tsai ha scritto:
> On Wed, Oct 3, 2018 at 5:59 AM Giulio Benetti
> <giulio.benetti@micronovasrl.com> wrote:
>>
>> At the moment, the check of tcon->panel to be valid is wrong. IS_ERR()
>> has been used, but that macro doesn't check if tcon->panel pointer is
>> null or not, but check if tcon->panel is between -1 and -4095(MAX_ERRNO).
>>
>> Remove IS_ERR() from tcon->panel checking and let "if (tcon->panel)" as
>> condition to check if it's a pointer not null.
> 
> There's actually more than one occurance of this error:
> 
> drivers/gpu/drm/sun4i/sun4i_lvds.c:     if (!IS_ERR(tcon->panel)) {
> drivers/gpu/drm/sun4i/sun4i_lvds.c:     if (!IS_ERR(tcon->panel)) {
> drivers/gpu/drm/sun4i/sun4i_rgb.c:      if (!IS_ERR(tcon->panel)) {
> drivers/gpu/drm/sun4i/sun4i_rgb.c:      if (!IS_ERR(tcon->panel)) {
> 
> These four are responsible for enabling and disabling the panel.
> 
> drivers/gpu/drm/sun4i/sun4i_tcon.c:     if (!IS_ERR(tcon->panel)) {
> 
> All this looks like it was left over from commit ebc9446135671 ("drm: convert
> drivers to use drm_of_find_panel_or_bridge"). We have checks against tcon->panel
> in several places and not all of them were converted.
> 

Then, I'm going to send a patchset to correct them.

Best regards
-- 
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale ? 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

[PATCH v2 1/2] drm/sun4i: tcon: fix check of tcon->panel null pointer

[Giulio Benetti]

At the moment, the check of tcon->panel to be valid is wrong. IS_ERR()
has been used, but that macro doesn't check if tcon->panel pointer is
null or not, but check if tcon->panel is between -1 and -4095(MAX_ERRNO).

Remove IS_ERR() from tcon->panel checking and let "if (tcon->panel)" as
condition to check if it's a pointer not null.

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
Changes V1->V2:
* correct same bug for all same occurences in drm/sun4i folder

 drivers/gpu/drm/sun4i/sun4i_lvds.c | 4 ++--
 drivers/gpu/drm/sun4i/sun4i_rgb.c  | 4 ++--
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_lvds.c b/drivers/gpu/drm/sun4i/sun4i_lvds.c
index af7dcb6da351..e7eb0d1e17be 100644
--- a/drivers/gpu/drm/sun4i/sun4i_lvds.c
+++ b/drivers/gpu/drm/sun4i/sun4i_lvds.c
@@ -75,7 +75,7 @@ static void sun4i_lvds_encoder_enable(struct drm_encoder *encoder)
 
 	DRM_DEBUG_DRIVER("Enabling LVDS output\n");
 
-	if (!IS_ERR(tcon->panel)) {
+	if (tcon->panel) {
 		drm_panel_prepare(tcon->panel);
 		drm_panel_enable(tcon->panel);
 	}
@@ -88,7 +88,7 @@ static void sun4i_lvds_encoder_disable(struct drm_encoder *encoder)
 
 	DRM_DEBUG_DRIVER("Disabling LVDS output\n");
 
-	if (!IS_ERR(tcon->panel)) {
+	if (tcon->panel) {
 		drm_panel_disable(tcon->panel);
 		drm_panel_unprepare(tcon->panel);
 	}
diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c
index bf068da6b12e..f4a22689eb54 100644
--- a/drivers/gpu/drm/sun4i/sun4i_rgb.c
+++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c
@@ -135,7 +135,7 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder)
 
 	DRM_DEBUG_DRIVER("Enabling RGB output\n");
 
-	if (!IS_ERR(tcon->panel)) {
+	if (tcon->panel) {
 		drm_panel_prepare(tcon->panel);
 		drm_panel_enable(tcon->panel);
 	}
@@ -148,7 +148,7 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder)
 
 	DRM_DEBUG_DRIVER("Disabling RGB output\n");
 
-	if (!IS_ERR(tcon->panel)) {
+	if (tcon->panel) {
 		drm_panel_disable(tcon->panel);
 		drm_panel_unprepare(tcon->panel);
 	}
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index c78cd35a1294..e4b3bd0307ef 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -555,7 +555,7 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
 	 * Following code is a way to avoid quirks all around TCON
 	 * and DOTCLOCK drivers.
 	 */
-	if (!IS_ERR(tcon->panel)) {
+	if (tcon->panel) {
 		struct drm_panel *panel = tcon->panel;
 		struct drm_connector *connector = panel->connector;
 		struct drm_display_info display_info = connector->display_info;
-- 
2.17.1

[PATCH v2 1/2] drm/sun4i: tcon: fix check of tcon->panel null pointer

[Giulio Benetti]

At the moment, the check of tcon->panel to be valid is wrong. IS_ERR()
has been used, but that macro doesn't check if tcon->panel pointer is
null or not, but check if tcon->panel is between -1 and -4095(MAX_ERRNO).

Remove IS_ERR() from tcon->panel checking and let "if (tcon->panel)" as
condition to check if it's a pointer not null.

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
Changes V1->V2:
* correct same bug for all same occurences in drm/sun4i folder

 drivers/gpu/drm/sun4i/sun4i_lvds.c | 4 ++--
 drivers/gpu/drm/sun4i/sun4i_rgb.c  | 4 ++--
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_lvds.c b/drivers/gpu/drm/sun4i/sun4i_lvds.c
index af7dcb6da351..e7eb0d1e17be 100644
--- a/drivers/gpu/drm/sun4i/sun4i_lvds.c
+++ b/drivers/gpu/drm/sun4i/sun4i_lvds.c
@@ -75,7 +75,7 @@ static void sun4i_lvds_encoder_enable(struct drm_encoder *encoder)
 
 	DRM_DEBUG_DRIVER("Enabling LVDS output\n");
 
-	if (!IS_ERR(tcon->panel)) {
+	if (tcon->panel) {
 		drm_panel_prepare(tcon->panel);
 		drm_panel_enable(tcon->panel);
 	}
@@ -88,7 +88,7 @@ static void sun4i_lvds_encoder_disable(struct drm_encoder *encoder)
 
 	DRM_DEBUG_DRIVER("Disabling LVDS output\n");
 
-	if (!IS_ERR(tcon->panel)) {
+	if (tcon->panel) {
 		drm_panel_disable(tcon->panel);
 		drm_panel_unprepare(tcon->panel);
 	}
diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c
index bf068da6b12e..f4a22689eb54 100644
--- a/drivers/gpu/drm/sun4i/sun4i_rgb.c
+++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c
@@ -135,7 +135,7 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder)
 
 	DRM_DEBUG_DRIVER("Enabling RGB output\n");
 
-	if (!IS_ERR(tcon->panel)) {
+	if (tcon->panel) {
 		drm_panel_prepare(tcon->panel);
 		drm_panel_enable(tcon->panel);
 	}
@@ -148,7 +148,7 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder)
 
 	DRM_DEBUG_DRIVER("Disabling RGB output\n");
 
-	if (!IS_ERR(tcon->panel)) {
+	if (tcon->panel) {
 		drm_panel_disable(tcon->panel);
 		drm_panel_unprepare(tcon->panel);
 	}
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index c78cd35a1294..e4b3bd0307ef 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -555,7 +555,7 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
 	 * Following code is a way to avoid quirks all around TCON
 	 * and DOTCLOCK drivers.
 	 */
-	if (!IS_ERR(tcon->panel)) {
+	if (tcon->panel) {
 		struct drm_panel *panel = tcon->panel;
 		struct drm_connector *connector = panel->connector;
 		struct drm_display_info display_info = connector->display_info;
-- 
2.17.1

[PATCH v2 2/2] drm/sun4i: tcon: prevent tcon->panel dereference if null

[Giulio Benetti]

If using tcon with VGA, tcon->panel will be null(0), this will cause
segmentation fault when trying to dereference tcon->panel->connector.

Add tcon->panel null check before calling
sun4i_tcon0_mode_set_dithering().

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
Fixes: f11adcecbd5f ("drm/sun4i: tcon: Add dithering support for
                      RGB565/RGB666 LCD panels")
Reviewed-by: Chen-Yu Tsai <wens@csie.org>
---
Changes V1->V2:
* none

 drivers/gpu/drm/sun4i/sun4i_tcon.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index e4b3bd0307ef..f949287d926c 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -491,7 +491,8 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
 	sun4i_tcon0_mode_set_common(tcon, mode);
 
 	/* Set dithering if needed */
-	sun4i_tcon0_mode_set_dithering(tcon, tcon->panel->connector);
+	if (tcon->panel)
+		sun4i_tcon0_mode_set_dithering(tcon, tcon->panel->connector);
 
 	/* Adjust clock delay */
 	clk_delay = sun4i_tcon_get_clk_delay(mode, 0);
-- 
2.17.1

[PATCH v2 2/2] drm/sun4i: tcon: prevent tcon->panel dereference if null

[Giulio Benetti]

If using tcon with VGA, tcon->panel will be null(0), this will cause
segmentation fault when trying to dereference tcon->panel->connector.

Add tcon->panel null check before calling
sun4i_tcon0_mode_set_dithering().

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
Fixes: f11adcecbd5f ("drm/sun4i: tcon: Add dithering support for
                      RGB565/RGB666 LCD panels")
Reviewed-by: Chen-Yu Tsai <wens@csie.org>
---
Changes V1->V2:
* none

 drivers/gpu/drm/sun4i/sun4i_tcon.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index e4b3bd0307ef..f949287d926c 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -491,7 +491,8 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
 	sun4i_tcon0_mode_set_common(tcon, mode);
 
 	/* Set dithering if needed */
-	sun4i_tcon0_mode_set_dithering(tcon, tcon->panel->connector);
+	if (tcon->panel)
+		sun4i_tcon0_mode_set_dithering(tcon, tcon->panel->connector);
 
 	/* Adjust clock delay */
 	clk_delay = sun4i_tcon_get_clk_delay(mode, 0);
-- 
2.17.1

[PATCH v2 2/2] drm/sun4i: tcon: prevent tcon->panel dereference if null

[Giulio Benetti]

If using tcon with VGA, tcon->panel will be null(0), this will cause
segmentation fault when trying to dereference tcon->panel->connector.

Add tcon->panel null check before calling
sun4i_tcon0_mode_set_dithering().

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
Fixes: f11adcecbd5f ("drm/sun4i: tcon: Add dithering support for
                      RGB565/RGB666 LCD panels")
Reviewed-by: Chen-Yu Tsai <wens@csie.org>
---
Changes V1->V2:
* none

 drivers/gpu/drm/sun4i/sun4i_tcon.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index e4b3bd0307ef..f949287d926c 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -491,7 +491,8 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
 	sun4i_tcon0_mode_set_common(tcon, mode);
 
 	/* Set dithering if needed */
-	sun4i_tcon0_mode_set_dithering(tcon, tcon->panel->connector);
+	if (tcon->panel)
+		sun4i_tcon0_mode_set_dithering(tcon, tcon->panel->connector);
 
 	/* Adjust clock delay */
 	clk_delay = sun4i_tcon_get_clk_delay(mode, 0);
-- 
2.17.1

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

Re: [PATCH v2 1/2] drm/sun4i: tcon: fix check of tcon->panel null pointer

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 901 bytes --]

Hi,

On Wed, Oct 03, 2018 at 04:24:57PM +0200, Giulio Benetti wrote:
> At the moment, the check of tcon->panel to be valid is wrong. IS_ERR()
> has been used, but that macro doesn't check if tcon->panel pointer is
> null or not, but check if tcon->panel is between -1 and -4095(MAX_ERRNO).
> 
> Remove IS_ERR() from tcon->panel checking and let "if (tcon->panel)" as
> condition to check if it's a pointer not null.
> 
> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>

The commit log should be improved. The issue isn't really with the
IS_ERR macro as you suggest, but that what is returned by
of_drm_find_panel, and thus stored in tcon->panel. is not an error
pointer in the first place but a NULL pointer on error. So the check
doesn't check for the proper thing.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 1/2] drm/sun4i: tcon: fix check of tcon->panel null pointer

[Maxime Ripard]

Hi,

On Wed, Oct 03, 2018 at 04:24:57PM +0200, Giulio Benetti wrote:
> At the moment, the check of tcon->panel to be valid is wrong. IS_ERR()
> has been used, but that macro doesn't check if tcon->panel pointer is
> null or not, but check if tcon->panel is between -1 and -4095(MAX_ERRNO).
> 
> Remove IS_ERR() from tcon->panel checking and let "if (tcon->panel)" as
> condition to check if it's a pointer not null.
> 
> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>

The commit log should be improved. The issue isn't really with the
IS_ERR macro as you suggest, but that what is returned by
of_drm_find_panel, and thus stored in tcon->panel. is not an error
pointer in the first place but a NULL pointer on error. So the check
doesn't check for the proper thing.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20181004/5728b4a0/attachment.sig>

Re: [PATCH v2 1/2] drm/sun4i: tcon: fix check of tcon->panel null pointer

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 901 bytes --]

Hi,

On Wed, Oct 03, 2018 at 04:24:57PM +0200, Giulio Benetti wrote:
> At the moment, the check of tcon->panel to be valid is wrong. IS_ERR()
> has been used, but that macro doesn't check if tcon->panel pointer is
> null or not, but check if tcon->panel is between -1 and -4095(MAX_ERRNO).
> 
> Remove IS_ERR() from tcon->panel checking and let "if (tcon->panel)" as
> condition to check if it's a pointer not null.
> 
> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>

The commit log should be improved. The issue isn't really with the
IS_ERR macro as you suggest, but that what is returned by
of_drm_find_panel, and thus stored in tcon->panel. is not an error
pointer in the first place but a NULL pointer on error. So the check
doesn't check for the proper thing.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 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

Re: [PATCH v2 2/2] drm/sun4i: tcon: prevent tcon->panel dereference if null

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 522 bytes --]

On Wed, Oct 03, 2018 at 04:24:58PM +0200, Giulio Benetti wrote:
> If using tcon with VGA,

We don't have support for VGA at the moment. Or are you talking about
using a VGA bridge?

> tcon->panel will be null(0), this will cause segmentation fault when
> trying to dereference tcon->panel->connector.

It's not a segmentation fault, but a null pointer dereference. And
that case will also happen with bridges.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 2/2] drm/sun4i: tcon: prevent tcon->panel dereference if null

[Maxime Ripard]

On Wed, Oct 03, 2018 at 04:24:58PM +0200, Giulio Benetti wrote:
> If using tcon with VGA,

We don't have support for VGA at the moment. Or are you talking about
using a VGA bridge?

> tcon->panel will be null(0), this will cause segmentation fault when
> trying to dereference tcon->panel->connector.

It's not a segmentation fault, but a null pointer dereference. And
that case will also happen with bridges.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20181004/fc276c6c/attachment.sig>

Re: [PATCH v2 2/2] drm/sun4i: tcon: prevent tcon->panel dereference if null

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 522 bytes --]

On Wed, Oct 03, 2018 at 04:24:58PM +0200, Giulio Benetti wrote:
> If using tcon with VGA,

We don't have support for VGA at the moment. Or are you talking about
using a VGA bridge?

> tcon->panel will be null(0), this will cause segmentation fault when
> trying to dereference tcon->panel->connector.

It's not a segmentation fault, but a null pointer dereference. And
that case will also happen with bridges.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 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

Re: [PATCH v2 2/2] drm/sun4i: tcon: prevent tcon->panel dereference if null

[Giulio Benetti]

Hi,

Il 04/10/2018 21:56, Maxime Ripard ha scritto:
> On Wed, Oct 03, 2018 at 04:24:58PM +0200, Giulio Benetti wrote:
>> If using tcon with VGA,
> 
> We don't have support for VGA at the moment. Or are you talking about
> using a VGA bridge?

You're right, in general VGA is not the point.
tcon->panel is retrieved by drm_of_find_panel_or_bridge() and panel can 
be present or not.

>> tcon->panel will be null(0), this will cause segmentation fault when
>> trying to dereference tcon->panel->connector.
> 
> It's not a segmentation fault, but a null pointer dereference. And
> that case will also happen with bridges.

Right.

Going to improve/rewrite commit logs and submit v2 patchset.

Thanks for reviewing.

Best regards
-- 
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale € 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

[PATCH v2 2/2] drm/sun4i: tcon: prevent tcon->panel dereference if null

[Giulio Benetti]

Hi,

Il 04/10/2018 21:56, Maxime Ripard ha scritto:
> On Wed, Oct 03, 2018 at 04:24:58PM +0200, Giulio Benetti wrote:
>> If using tcon with VGA,
> 
> We don't have support for VGA at the moment. Or are you talking about
> using a VGA bridge?

You're right, in general VGA is not the point.
tcon->panel is retrieved by drm_of_find_panel_or_bridge() and panel can 
be present or not.

>> tcon->panel will be null(0), this will cause segmentation fault when
>> trying to dereference tcon->panel->connector.
> 
> It's not a segmentation fault, but a null pointer dereference. And
> that case will also happen with bridges.

Right.

Going to improve/rewrite commit logs and submit v2 patchset.

Thanks for reviewing.

Best regards
-- 
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale ? 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

[PATCH v3 1/2] drm/sun4i: tcon: fix check of tcon->panel null pointer

[Giulio Benetti]

Since tcon->panel is a pointer returned by of_drm_find_panel() need to
check if it is not NULL, hence a valid pointer.
IS_ERR() instead checks return error values, not NULL pointers.

Substitute "if (!IS_ERR(tcon->panel))" with "if (tcon->panel)".

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
Changes V1->V2:
* correct same bug for all same occurences in drm/sun4i folder

Changes V2->V3:
* Improve commit log

 drivers/gpu/drm/sun4i/sun4i_lvds.c | 4 ++--
 drivers/gpu/drm/sun4i/sun4i_rgb.c  | 4 ++--
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_lvds.c b/drivers/gpu/drm/sun4i/sun4i_lvds.c
index af7dcb6da351..e7eb0d1e17be 100644
--- a/drivers/gpu/drm/sun4i/sun4i_lvds.c
+++ b/drivers/gpu/drm/sun4i/sun4i_lvds.c
@@ -75,7 +75,7 @@ static void sun4i_lvds_encoder_enable(struct drm_encoder *encoder)
 
 	DRM_DEBUG_DRIVER("Enabling LVDS output\n");
 
-	if (!IS_ERR(tcon->panel)) {
+	if (tcon->panel) {
 		drm_panel_prepare(tcon->panel);
 		drm_panel_enable(tcon->panel);
 	}
@@ -88,7 +88,7 @@ static void sun4i_lvds_encoder_disable(struct drm_encoder *encoder)
 
 	DRM_DEBUG_DRIVER("Disabling LVDS output\n");
 
-	if (!IS_ERR(tcon->panel)) {
+	if (tcon->panel) {
 		drm_panel_disable(tcon->panel);
 		drm_panel_unprepare(tcon->panel);
 	}
diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c
index bf068da6b12e..f4a22689eb54 100644
--- a/drivers/gpu/drm/sun4i/sun4i_rgb.c
+++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c
@@ -135,7 +135,7 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder)
 
 	DRM_DEBUG_DRIVER("Enabling RGB output\n");
 
-	if (!IS_ERR(tcon->panel)) {
+	if (tcon->panel) {
 		drm_panel_prepare(tcon->panel);
 		drm_panel_enable(tcon->panel);
 	}
@@ -148,7 +148,7 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder)
 
 	DRM_DEBUG_DRIVER("Disabling RGB output\n");
 
-	if (!IS_ERR(tcon->panel)) {
+	if (tcon->panel) {
 		drm_panel_disable(tcon->panel);
 		drm_panel_unprepare(tcon->panel);
 	}
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index c78cd35a1294..e4b3bd0307ef 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -555,7 +555,7 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
 	 * Following code is a way to avoid quirks all around TCON
 	 * and DOTCLOCK drivers.
 	 */
-	if (!IS_ERR(tcon->panel)) {
+	if (tcon->panel) {
 		struct drm_panel *panel = tcon->panel;
 		struct drm_connector *connector = panel->connector;
 		struct drm_display_info display_info = connector->display_info;
-- 
2.17.1

[PATCH v3 1/2] drm/sun4i: tcon: fix check of tcon->panel null pointer

[Giulio Benetti]

Since tcon->panel is a pointer returned by of_drm_find_panel() need to
check if it is not NULL, hence a valid pointer.
IS_ERR() instead checks return error values, not NULL pointers.

Substitute "if (!IS_ERR(tcon->panel))" with "if (tcon->panel)".

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
Changes V1->V2:
* correct same bug for all same occurences in drm/sun4i folder

Changes V2->V3:
* Improve commit log

 drivers/gpu/drm/sun4i/sun4i_lvds.c | 4 ++--
 drivers/gpu/drm/sun4i/sun4i_rgb.c  | 4 ++--
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_lvds.c b/drivers/gpu/drm/sun4i/sun4i_lvds.c
index af7dcb6da351..e7eb0d1e17be 100644
--- a/drivers/gpu/drm/sun4i/sun4i_lvds.c
+++ b/drivers/gpu/drm/sun4i/sun4i_lvds.c
@@ -75,7 +75,7 @@ static void sun4i_lvds_encoder_enable(struct drm_encoder *encoder)
 
 	DRM_DEBUG_DRIVER("Enabling LVDS output\n");
 
-	if (!IS_ERR(tcon->panel)) {
+	if (tcon->panel) {
 		drm_panel_prepare(tcon->panel);
 		drm_panel_enable(tcon->panel);
 	}
@@ -88,7 +88,7 @@ static void sun4i_lvds_encoder_disable(struct drm_encoder *encoder)
 
 	DRM_DEBUG_DRIVER("Disabling LVDS output\n");
 
-	if (!IS_ERR(tcon->panel)) {
+	if (tcon->panel) {
 		drm_panel_disable(tcon->panel);
 		drm_panel_unprepare(tcon->panel);
 	}
diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c
index bf068da6b12e..f4a22689eb54 100644
--- a/drivers/gpu/drm/sun4i/sun4i_rgb.c
+++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c
@@ -135,7 +135,7 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder)
 
 	DRM_DEBUG_DRIVER("Enabling RGB output\n");
 
-	if (!IS_ERR(tcon->panel)) {
+	if (tcon->panel) {
 		drm_panel_prepare(tcon->panel);
 		drm_panel_enable(tcon->panel);
 	}
@@ -148,7 +148,7 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder)
 
 	DRM_DEBUG_DRIVER("Disabling RGB output\n");
 
-	if (!IS_ERR(tcon->panel)) {
+	if (tcon->panel) {
 		drm_panel_disable(tcon->panel);
 		drm_panel_unprepare(tcon->panel);
 	}
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index c78cd35a1294..e4b3bd0307ef 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -555,7 +555,7 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
 	 * Following code is a way to avoid quirks all around TCON
 	 * and DOTCLOCK drivers.
 	 */
-	if (!IS_ERR(tcon->panel)) {
+	if (tcon->panel) {
 		struct drm_panel *panel = tcon->panel;
 		struct drm_connector *connector = panel->connector;
 		struct drm_display_info display_info = connector->display_info;
-- 
2.17.1

[PATCH v3 1/2] drm/sun4i: tcon: fix check of tcon->panel null pointer

[Giulio Benetti]

Since tcon->panel is a pointer returned by of_drm_find_panel() need to
check if it is not NULL, hence a valid pointer.
IS_ERR() instead checks return error values, not NULL pointers.

Substitute "if (!IS_ERR(tcon->panel))" with "if (tcon->panel)".

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
---
Changes V1->V2:
* correct same bug for all same occurences in drm/sun4i folder

Changes V2->V3:
* Improve commit log

 drivers/gpu/drm/sun4i/sun4i_lvds.c | 4 ++--
 drivers/gpu/drm/sun4i/sun4i_rgb.c  | 4 ++--
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_lvds.c b/drivers/gpu/drm/sun4i/sun4i_lvds.c
index af7dcb6da351..e7eb0d1e17be 100644
--- a/drivers/gpu/drm/sun4i/sun4i_lvds.c
+++ b/drivers/gpu/drm/sun4i/sun4i_lvds.c
@@ -75,7 +75,7 @@ static void sun4i_lvds_encoder_enable(struct drm_encoder *encoder)
 
 	DRM_DEBUG_DRIVER("Enabling LVDS output\n");
 
-	if (!IS_ERR(tcon->panel)) {
+	if (tcon->panel) {
 		drm_panel_prepare(tcon->panel);
 		drm_panel_enable(tcon->panel);
 	}
@@ -88,7 +88,7 @@ static void sun4i_lvds_encoder_disable(struct drm_encoder *encoder)
 
 	DRM_DEBUG_DRIVER("Disabling LVDS output\n");
 
-	if (!IS_ERR(tcon->panel)) {
+	if (tcon->panel) {
 		drm_panel_disable(tcon->panel);
 		drm_panel_unprepare(tcon->panel);
 	}
diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c
index bf068da6b12e..f4a22689eb54 100644
--- a/drivers/gpu/drm/sun4i/sun4i_rgb.c
+++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c
@@ -135,7 +135,7 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder)
 
 	DRM_DEBUG_DRIVER("Enabling RGB output\n");
 
-	if (!IS_ERR(tcon->panel)) {
+	if (tcon->panel) {
 		drm_panel_prepare(tcon->panel);
 		drm_panel_enable(tcon->panel);
 	}
@@ -148,7 +148,7 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder)
 
 	DRM_DEBUG_DRIVER("Disabling RGB output\n");
 
-	if (!IS_ERR(tcon->panel)) {
+	if (tcon->panel) {
 		drm_panel_disable(tcon->panel);
 		drm_panel_unprepare(tcon->panel);
 	}
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index c78cd35a1294..e4b3bd0307ef 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -555,7 +555,7 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
 	 * Following code is a way to avoid quirks all around TCON
 	 * and DOTCLOCK drivers.
 	 */
-	if (!IS_ERR(tcon->panel)) {
+	if (tcon->panel) {
 		struct drm_panel *panel = tcon->panel;
 		struct drm_connector *connector = panel->connector;
 		struct drm_display_info display_info = connector->display_info;
-- 
2.17.1

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

[PATCH v3 2/2] drm/sun4i: tcon: prevent tcon->panel dereference if NULL

[Giulio Benetti]

If tcon->panel pointer is NULL, trying to dereference from it
(i.e. tcon->panel->connector) will cause a null pointer dereference.

Add tcon->panel null pointer check before calling
sun4i_tcon0_mode_set_dithering().

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
Fixes: f11adcecbd5f ("drm/sun4i: tcon: Add dithering support for
                      RGB565/RGB666 LCD panels")
Reviewed-by: Chen-Yu Tsai <wens@csie.org>
---
Changes V1->V2:
* None

Changes V2->V3:
* Rewrite commit log

 drivers/gpu/drm/sun4i/sun4i_tcon.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index e4b3bd0307ef..f949287d926c 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -491,7 +491,8 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
 	sun4i_tcon0_mode_set_common(tcon, mode);
 
 	/* Set dithering if needed */
-	sun4i_tcon0_mode_set_dithering(tcon, tcon->panel->connector);
+	if (tcon->panel)
+		sun4i_tcon0_mode_set_dithering(tcon, tcon->panel->connector);
 
 	/* Adjust clock delay */
 	clk_delay = sun4i_tcon_get_clk_delay(mode, 0);
-- 
2.17.1

[PATCH v3 2/2] drm/sun4i: tcon: prevent tcon->panel dereference if NULL

[Giulio Benetti]

If tcon->panel pointer is NULL, trying to dereference from it
(i.e. tcon->panel->connector) will cause a null pointer dereference.

Add tcon->panel null pointer check before calling
sun4i_tcon0_mode_set_dithering().

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
Fixes: f11adcecbd5f ("drm/sun4i: tcon: Add dithering support for
                      RGB565/RGB666 LCD panels")
Reviewed-by: Chen-Yu Tsai <wens@csie.org>
---
Changes V1->V2:
* None

Changes V2->V3:
* Rewrite commit log

 drivers/gpu/drm/sun4i/sun4i_tcon.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index e4b3bd0307ef..f949287d926c 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -491,7 +491,8 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon,
 	sun4i_tcon0_mode_set_common(tcon, mode);
 
 	/* Set dithering if needed */
-	sun4i_tcon0_mode_set_dithering(tcon, tcon->panel->connector);
+	if (tcon->panel)
+		sun4i_tcon0_mode_set_dithering(tcon, tcon->panel->connector);
 
 	/* Adjust clock delay */
 	clk_delay = sun4i_tcon_get_clk_delay(mode, 0);
-- 
2.17.1

Re: [PATCH v3 2/2] drm/sun4i: tcon: prevent tcon->panel dereference if NULL

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 656 bytes --]

On Fri, Oct 05, 2018 at 11:59:51PM +0200, Giulio Benetti wrote:
> If tcon->panel pointer is NULL, trying to dereference from it
> (i.e. tcon->panel->connector) will cause a null pointer dereference.
> 
> Add tcon->panel null pointer check before calling
> sun4i_tcon0_mode_set_dithering().
> 
> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> Fixes: f11adcecbd5f ("drm/sun4i: tcon: Add dithering support for
>                       RGB565/RGB666 LCD panels")
> Reviewed-by: Chen-Yu Tsai <wens@csie.org>

Applied both, thanks!
Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 2/2] drm/sun4i: tcon: prevent tcon->panel dereference if NULL

[Maxime Ripard]

On Fri, Oct 05, 2018 at 11:59:51PM +0200, Giulio Benetti wrote:
> If tcon->panel pointer is NULL, trying to dereference from it
> (i.e. tcon->panel->connector) will cause a null pointer dereference.
> 
> Add tcon->panel null pointer check before calling
> sun4i_tcon0_mode_set_dithering().
> 
> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> Fixes: f11adcecbd5f ("drm/sun4i: tcon: Add dithering support for
>                       RGB565/RGB666 LCD panels")
> Reviewed-by: Chen-Yu Tsai <wens@csie.org>

Applied both, thanks!
Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20181008/46145777/attachment.sig>

Re: [bug report] drm/sun4i: Handle DRM_BUS_FLAG_PIXDATA_*EDGE checking if panel is used.

[Dan Carpenter]

On Tue, Oct 02, 2018 at 03:26:22PM +0200, Giulio Benetti wrote:
> Il 01/10/2018 11:36, Dan Carpenter ha scritto:
> > Hello Giulio Benetti,
> > 
> > The patch 490cda5a3c82: "drm/sun4i: Handle DRM_BUS_FLAG_PIXDATA_*EDGE
> > checking if panel is used." from Jul 18, 2018, leads to the following
> > static checker warning:
> > 	drivers/gpu/drm/sun4i/sun4i_tcon.c:558 sun4i_tcon0_mode_set_rgb()
> > 	warn: 'tcon->panel' isn't an ERR_PTR
> > 
> > drivers/gpu/drm/sun4i/sun4i_tcon.c
> >     481                                       const struct drm_display_mode *mode)
> >     482  {
> >     483          unsigned int bp, hsync, vsync;
> >     484          u8 clk_delay;
> >     485          u32 val = 0;
> >     486
> >     487          WARN_ON(!tcon->quirks->has_channel_0);
> >     488
> >     489          tcon->dclk_min_div = 6;
> >     490          tcon->dclk_max_div = 127;
> >     491          sun4i_tcon0_mode_set_common(tcon, mode);
> >     492
> >     493          /* Set dithering if needed */
> >     494          sun4i_tcon0_mode_set_dithering(tcon, tcon->panel->connector);
> >                                                       ^^^^^^^^^^^^^
> 
> Here there should be something like:
> if (!tcon->panel) {
> 	sun4i_tcon0_mode_set_dithering(tcon, tcon->panel->connector);
> }
> 
> >     495
> >     496          /* Adjust clock delay */
> >     497          clk_delay = sun4i_tcon_get_clk_delay(mode, 0);
> >     498          regmap_update_bits(tcon->regs, SUN4I_TCON0_CTL_REG,
> >     499                             SUN4I_TCON0_CTL_CLK_DELAY_MASK,
> >     500                             SUN4I_TCON0_CTL_CLK_DELAY(clk_delay));
> >     501
> >     502          /*
> >     503           * This is called a backporch in the register documentation,
> >     504           * but it really is the back porch + hsync
> >     505           */
> >     506          bp = mode->crtc_htotal - mode->crtc_hsync_start;
> >     507          DRM_DEBUG_DRIVER("Setting horizontal total %d, backporch %d\n",
> >     508                           mode->crtc_htotal, bp);
> >     509
> >     510          /* Set horizontal display timings */
> >     511          regmap_write(tcon->regs, SUN4I_TCON0_BASIC1_REG,
> >     512                       SUN4I_TCON0_BASIC1_H_TOTAL(mode->crtc_htotal) |
> >     513                       SUN4I_TCON0_BASIC1_H_BACKPORCH(bp));
> >     514
> >     515          /*
> >     516           * This is called a backporch in the register documentation,
> >     517           * but it really is the back porch + hsync
> >     518           */
> >     519          bp = mode->crtc_vtotal - mode->crtc_vsync_start;
> >     520          DRM_DEBUG_DRIVER("Setting vertical total %d, backporch %d\n",
> >     521                           mode->crtc_vtotal, bp);
> >     522
> >     523          /* Set vertical display timings */
> >     524          regmap_write(tcon->regs, SUN4I_TCON0_BASIC2_REG,
> >     525                       SUN4I_TCON0_BASIC2_V_TOTAL(mode->crtc_vtotal * 2) |
> >     526                       SUN4I_TCON0_BASIC2_V_BACKPORCH(bp));
> >     527
> >     528          /* Set Hsync and Vsync length */
> >     529          hsync = mode->crtc_hsync_end - mode->crtc_hsync_start;
> >     530          vsync = mode->crtc_vsync_end - mode->crtc_vsync_start;
> >     531          DRM_DEBUG_DRIVER("Setting HSYNC %d, VSYNC %d\n", hsync, vsync);
> >     532          regmap_write(tcon->regs, SUN4I_TCON0_BASIC3_REG,
> >     533                       SUN4I_TCON0_BASIC3_V_SYNC(vsync) |
> >     534                       SUN4I_TCON0_BASIC3_H_SYNC(hsync));
> >     535
> >     536          /* Setup the polarity of the various signals */
> >     537          if (mode->flags & DRM_MODE_FLAG_PHSYNC)
> >     538                  val |= SUN4I_TCON0_IO_POL_HSYNC_POSITIVE;
> >     539
> >     540          if (mode->flags & DRM_MODE_FLAG_PVSYNC)
> >     541                  val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
> >     542
> >     543          /*
> >     544           * On A20 and similar SoCs, the only way to achieve Positive Edge
> >     545           * (Rising Edge), is setting dclk clock phase to 2/3(240°).
> >     546           * By default TCON works in Negative Edge(Falling Edge),
> >     547           * this is why phase is set to 0 in that case.
> >     548           * Unfortunately there's no way to logically invert dclk through
> >     549           * IO_POL register.
> >     550           * The only acceptable way to work, triple checked with scope,
> >     551           * is using clock phase set to 0° for Negative Edge and set to 240°
> >     552           * for Positive Edge.
> >     553           * On A33 and similar SoCs there would be a 90° phase option,
> >     554           * but it divides also dclk by 2.
> >     555           * Following code is a way to avoid quirks all around TCON
> >     556           * and DOTCLOCK drivers.
> >     557           */
> >     558          if (!IS_ERR(tcon->panel)) {
> >                       ^^^^^^^^^^^^^^^^^^
> > Unpossible to be an error pointer!
> 
> So maybe I should use IS_ERR_OR_NULL() instead of IS_ERR(), or maybe simply:
> if (!tcon->panel) {
> 
> Right?
> 
> Anyway I'm going to test it with "sparse".
> Is it the same tool you've used for this?
> 

This is an unreleased Smatch test because:
1) Some things return error pointers depending on the config.
2) Pointless IS_ERR() checks are quite common.

I haven't looked at the context to see how to fix this.  My guess is
that ->panel can't be NULL nor error pointer, so the check should be
removed.  But again, I haven't looked at the code.

Btw, when you're mixing error pointers and NULL then normally NULL is a
special kind of success...  Like maybe you want to enable some hardware
if it's there and the function returns error pointers if memory
allocations fail but NULL if the hardware isn't there and isn't
required.

regards,
dan carpenter


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

Re: [bug report] drm/sun4i: Handle DRM_BUS_FLAG_PIXDATA_*EDGE checking if panel is used.

[Giulio Benetti]

Hello Dan,

Il 11/10/2018 09:16, Dan Carpenter ha scritto:
> This is an unreleased Smatch test because:
> 1) Some things return error pointers depending on the config.
> 2) Pointless IS_ERR() checks are quite common.

Look forward for new Smatch, very useful.

> I haven't looked at the context to see how to fix this.  My guess is
> that ->panel can't be NULL nor error pointer, so the check should be
> removed.  But again, I haven't looked at the code.

Thanks for pointing as those 2 bugs are already fixed:
https://cgit.freedesktop.org/drm/drm-misc/commit/?id=49c5c0769a919549d814de52f7aceaa1c86a3fc7
https://cgit.freedesktop.org/drm/drm-misc/commit/?id=548ae867efb1741fa55cedb5e73d7d0e75acd1f2

> Btw, when you're mixing error pointers and NULL then normally NULL is a
> special kind of success...  Like maybe you want to enable some hardware
> if it's there and the function returns error pointers if memory
> allocations fail but NULL if the hardware isn't there and isn't
> required.

Yes, what you've pointed me helped me a lot to understand it.

Thank you again.

Best regards
-- 
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale € 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 2/2] drm/sun4i: tcon: prevent tcon->panel dereference if NULL

[Chen-Yu Tsai]

On Mon, Oct 8, 2018 at 5:21 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Fri, Oct 05, 2018 at 11:59:51PM +0200, Giulio Benetti wrote:
> > If tcon->panel pointer is NULL, trying to dereference from it
> > (i.e. tcon->panel->connector) will cause a null pointer dereference.
> >
> > Add tcon->panel null pointer check before calling
> > sun4i_tcon0_mode_set_dithering().
> >
> > Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> > Fixes: f11adcecbd5f ("drm/sun4i: tcon: Add dithering support for
> >                       RGB565/RGB666 LCD panels")
> > Reviewed-by: Chen-Yu Tsai <wens@csie.org>
>
> Applied both, thanks!

I think this one could have been applied to drm-misc-next-fixes,
as the bug was only recently introduced.

We'll have to make sure it gets backported somehow.

ChenYu

[PATCH v3 2/2] drm/sun4i: tcon: prevent tcon->panel dereference if NULL

[Chen-Yu Tsai]

On Mon, Oct 8, 2018 at 5:21 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Fri, Oct 05, 2018 at 11:59:51PM +0200, Giulio Benetti wrote:
> > If tcon->panel pointer is NULL, trying to dereference from it
> > (i.e. tcon->panel->connector) will cause a null pointer dereference.
> >
> > Add tcon->panel null pointer check before calling
> > sun4i_tcon0_mode_set_dithering().
> >
> > Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> > Fixes: f11adcecbd5f ("drm/sun4i: tcon: Add dithering support for
> >                       RGB565/RGB666 LCD panels")
> > Reviewed-by: Chen-Yu Tsai <wens@csie.org>
>
> Applied both, thanks!

I think this one could have been applied to drm-misc-next-fixes,
as the bug was only recently introduced.

We'll have to make sure it gets backported somehow.

ChenYu

Re: [PATCH v3 2/2] drm/sun4i: tcon: prevent tcon->panel dereference if NULL

[Icenowy Zheng]

在 2018-10-08一的 11:21 +0200，Maxime Ripard写道：
> On Fri, Oct 05, 2018 at 11:59:51PM +0200, Giulio Benetti wrote:
> > If tcon->panel pointer is NULL, trying to dereference from it
> > (i.e. tcon->panel->connector) will cause a null pointer
> > dereference.
> > 
> > Add tcon->panel null pointer check before calling
> > sun4i_tcon0_mode_set_dithering().
> > 
> > Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> > Fixes: f11adcecbd5f ("drm/sun4i: tcon: Add dithering support for
> >                       RGB565/RGB666 LCD panels")
> > Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> 
> Applied both, thanks!

Please bring them to 4.20.

Currently in 4.20-rc1, bridge support of sun4i-drm is broken because of
this problem.

> Maxime
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 2/2] drm/sun4i: tcon: prevent tcon->panel dereference if NULL

[Icenowy Zheng]

? 2018-10-08?? 11:21 +0200?Maxime Ripard???
> On Fri, Oct 05, 2018 at 11:59:51PM +0200, Giulio Benetti wrote:
> > If tcon->panel pointer is NULL, trying to dereference from it
> > (i.e. tcon->panel->connector) will cause a null pointer
> > dereference.
> > 
> > Add tcon->panel null pointer check before calling
> > sun4i_tcon0_mode_set_dithering().
> > 
> > Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> > Fixes: f11adcecbd5f ("drm/sun4i: tcon: Add dithering support for
> >                       RGB565/RGB666 LCD panels")
> > Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> 
> Applied both, thanks!

Please bring them to 4.20.

Currently in 4.20-rc1, bridge support of sun4i-drm is broken because of
this problem.

> Maxime
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/2] drm/sun4i: tcon: prevent tcon->panel dereference if NULL

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1019 bytes --]

On Mon, Nov 05, 2018 at 09:23:08PM +0800, Icenowy Zheng wrote:
> 在 2018-10-08一的 11:21 +0200，Maxime Ripard写道：
> > On Fri, Oct 05, 2018 at 11:59:51PM +0200, Giulio Benetti wrote:
> > > If tcon->panel pointer is NULL, trying to dereference from it
> > > (i.e. tcon->panel->connector) will cause a null pointer
> > > dereference.
> > > 
> > > Add tcon->panel null pointer check before calling
> > > sun4i_tcon0_mode_set_dithering().
> > > 
> > > Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> > > Fixes: f11adcecbd5f ("drm/sun4i: tcon: Add dithering support for
> > >                       RGB565/RGB666 LCD panels")
> > > Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> > 
> > Applied both, thanks!
> 
> Please bring them to 4.20.
> 
> Currently in 4.20-rc1, bridge support of sun4i-drm is broken because of
> this problem.

I've merged them in drm-misc-fixes, thanks!

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v3 2/2] drm/sun4i: tcon: prevent tcon->panel dereference if NULL

[Maxime Ripard]

On Mon, Nov 05, 2018 at 09:23:08PM +0800, Icenowy Zheng wrote:
> ? 2018-10-08?? 11:21 +0200?Maxime Ripard???
> > On Fri, Oct 05, 2018 at 11:59:51PM +0200, Giulio Benetti wrote:
> > > If tcon->panel pointer is NULL, trying to dereference from it
> > > (i.e. tcon->panel->connector) will cause a null pointer
> > > dereference.
> > > 
> > > Add tcon->panel null pointer check before calling
> > > sun4i_tcon0_mode_set_dithering().
> > > 
> > > Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> > > Fixes: f11adcecbd5f ("drm/sun4i: tcon: Add dithering support for
> > >                       RGB565/RGB666 LCD panels")
> > > Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> > 
> > Applied both, thanks!
> 
> Please bring them to 4.20.
> 
> Currently in 4.20-rc1, bridge support of sun4i-drm is broken because of
> this problem.

I've merged them in drm-misc-fixes, thanks!

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20181106/c369b5f1/attachment.sig>