drm/sun4i: Handle DRM_BUS_FLAG_PIXDATA_*EDGE checking if panel is used.
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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Il 01/10/2018 11:36, Dan Carpenter ha scritto:
Quoted 9 lines
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
Quoted 15 lines
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);
}Quoted 65 lines
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)) {
^^^^^^^^^^^^^^^^^^Quoted 1 line
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
Signature · 14 lines
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 [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Il 02/10/2018 15:26, Giulio Benetti ha scritto:
Quoted 98 lines
Il 01/10/2018 11:36, Dan Carpenter ha scritto:
Quoted 9 lines
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.cQuoted 15 lines
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); }Quoted 65 lines
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)) { ^^^^^^^^^^^^^^^^^^Quoted 1 line
Unpossible to be an error pointer!
Quoted 7 lines
So maybe I should use
IS_ERR_OR_NULL()instead ofIS_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
Signature · 14 lines
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 [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Il 02/10/2018 16:12, Giulio Benetti ha scritto:
Quoted 102 lines
Il 02/10/2018 15:26, Giulio Benetti ha scritto:
Quoted 101 lines
Il 01/10/2018 11:36, Dan Carpenter ha scritto:
Quoted 9 lines
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.cQuoted 15 lines
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); }Quoted 65 lines
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)) { ^^^^^^^^^^^^^^^^^^Quoted 1 line
Unpossible to be an error pointer!
So maybe I should use
IS_ERR_OR_NULL()instead ofIS_ERR(), or maybe simply:
if (!tcon->panel) {
Quoted 7 lines
Quoted 4 lines
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
Signature · 14 lines
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 [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH 1/2 drm/sun4i: tcon: fix check of tcon->panel null pointer
In reply to Dan CarpenterAt 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.
--- drivers/gpu/drm/sun4i/sun4i_tcon.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/sun4i/sun4i_tcon.c +1 -1
@@ -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;
Signature · 1 line
2.17.1
PATCH 2/2 drm/sun4i: tcon: prevent tcon->panel dereference if null
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 callingsun4i_tcon0_mode_set_dithering().
--- drivers/gpu/drm/sun4i/sun4i_tcon.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/gpu/drm/sun4i/sun4i_tcon.c +2 -1
@@ -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 );
Signature · 1 line
2.17.1
PATCH 1/2 drm/sun4i: tcon: fix check of tcon->panel null pointer
In reply to Dan CarpenterAt 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.
--- drivers/gpu/drm/sun4i/sun4i_tcon.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/sun4i/sun4i_tcon.c +1 -1
@@ -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;
Signature · 1 line
2.17.1
PATCH 2/2 drm/sun4i: tcon: prevent tcon->panel dereference if null
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 callingsun4i_tcon0_mode_set_dithering().
--- drivers/gpu/drm/sun4i/sun4i_tcon.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/gpu/drm/sun4i/sun4i_tcon.c +2 -1
@@ -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 );
Signature · 1 line
2.17.1
Re: [PATCH 1/2] drm/sun4i: tcon: fix check of tcon->panel null pointer
In reply to Giulio BenettiSorry 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
Signature · 12 lines
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:
Quoted 26 lines
At the moment, the check of
tcon->panelto be valid is wrong.IS_ERR()has been used, but that macro doesn't check iftcon->panelpointer is null or not, but check iftcon->panelis between -1 and -4095(MAX_ERRNO).Remove
IS_ERR()fromtcon->panelchecking and let "if (tcon->panel)" as condition to check if it's a pointer not null.Signed-off-by Giulio Benetti <[email protected]>--- drivers/gpu/drm/sun4i/sun4i_tcon.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpu/drm/sun4i/sun4i_tcon.c +1
-1@@ -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 2/2] drm/sun4i: tcon: prevent tcon->panel dereference if null
In reply to Giulio BenettiQuoted 8 lines · Giulio Benetti
If using tcon with VGA,
tcon->panelwill be null(0), this will cause segmentation fault when trying to dereferencetcon->panel->connector.Add
tcon->panelnull check before callingsun4i_tcon0_mode_set_dithering().Signed-off-by Giulio Benetti <[email protected]>
RGB565/RGB666 LCD panels")
Re: [PATCH 1/2] drm/sun4i: tcon: fix check of tcon->panel null pointer
In reply to Giulio BenettiQuoted 7 lines · Giulio Benetti
At the moment, the check of
tcon->panelto be valid is wrong.IS_ERR()has been used, but that macro doesn't check iftcon->panelpointer is null or not, but check iftcon->panelis between -1 and -4095(MAX_ERRNO).Remove
IS_ERR()fromtcon->panelchecking 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
Quoted 21 lines
Signed-off-by Giulio Benetti <[email protected]>--- drivers/gpu/drm/sun4i/sun4i_tcon.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpu/drm/sun4i/sun4i_tcon.c +1
-2@@ -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
Il 03/10/2018 11:43, Chen-Yu Tsai ha scritto:
Quoted 17 lines
Quoted 7 lines · Giulio Benetti
At the moment, the check of
tcon->panelto be valid is wrong.IS_ERR()has been used, but that macro doesn't check iftcon->panelpointer is null or not, but check iftcon->panelis between -1 and -4095(MAX_ERRNO).Remove
IS_ERR()fromtcon->panelchecking 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)) {
Quoted 8 lines
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 usedrm_of_find_panel_or_bridge"). We have checks againsttcon->panelin several places and not all of them were converted.
Then, I'm going to send a patchset to correct them.
Best regards
Signature · 10 lines
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
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.
---
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(-)
drivers/gpu/drm/sun4i/sun4i_lvds.c +2 -2
@@ -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); }
drivers/gpu/drm/sun4i/sun4i_rgb.c +2 -2
@@ -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); }
drivers/gpu/drm/sun4i/sun4i_tcon.c +1 -1
@@ -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;
Signature · 1 line
2.17.1
PATCH v2 2/2 drm/sun4i: tcon: prevent tcon->panel dereference if null
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 callingsun4i_tcon0_mode_set_dithering().
RGB565/RGB666 LCD panels")
---
Changes V1->V2:
* none
drivers/gpu/drm/sun4i/sun4i_tcon.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/gpu/drm/sun4i/sun4i_tcon.c +2 -1
@@ -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 );
Signature · 1 line
2.17.1
Re: [PATCH v2 1/2] drm/sun4i: tcon: fix check of tcon->panel null pointer
In reply to Giulio BenettiHi,
Quoted 8 lines · Giulio Benetti
At the moment, the check of
tcon->panelto be valid is wrong.IS_ERR()has been used, but that macro doesn't check iftcon->panelpointer is null or not, but check iftcon->panelis between -1 and -4095(MAX_ERRNO).Remove
IS_ERR()fromtcon->panelchecking and let "if (tcon->panel)" as condition to check if it's a pointer not null.Signed-off-by Giulio Benetti <[email protected]>
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
Signature · 10 lines
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 2/2] drm/sun4i: tcon: prevent tcon->panel dereference if null
In reply to Giulio BenettiQuoted 1 line · Giulio Benetti
If using tcon with VGA,
We don't have support for VGA at the moment. Or are you talking about using a VGA bridge?
Quoted 2 lines
tcon->panelwill be null(0), this will cause segmentation fault when trying to dereferencetcon->panel->connector.
It's not a segmentation fault, but a null pointer dereference. And that case will also happen with bridges.
Maxime
Signature · 10 lines
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
Hi,
Il 04/10/2018 21:56, Maxime Ripard ha scritto:
Quoted 5 lines · Giulio Benetti
Quoted 1 line
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.
Quoted 5 lines
Quoted 2 lines
tcon->panelwill be null(0), this will cause segmentation fault when trying to dereferencetcon->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
Signature · 10 lines
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
In reply to Maxime RipardSince 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)".
---
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(-)
drivers/gpu/drm/sun4i/sun4i_lvds.c +2 -2
@@ -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); }
drivers/gpu/drm/sun4i/sun4i_rgb.c +2 -2
@@ -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); }
drivers/gpu/drm/sun4i/sun4i_tcon.c +1 -1
@@ -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;
Signature · 1 line
2.17.1
PATCH v3 2/2 drm/sun4i: tcon: prevent tcon->panel dereference if NULL
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 callingsun4i_tcon0_mode_set_dithering().
RGB565/RGB666 LCD panels")
---
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(-)
drivers/gpu/drm/sun4i/sun4i_tcon.c +2 -1
@@ -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 );
Signature · 1 line
2.17.1
Maxime Ripard
AppliedRe: [PATCH v3 2/2] drm/sun4i: tcon: prevent tcon->panel dereference if NULL
Quoted 10 lines · Giulio Benetti
If
tcon->panelpointer is NULL, trying to dereference from it (i.e.tcon->panel->connector) will cause a null pointer dereference.Add
tcon->panelnull pointer check before callingsun4i_tcon0_mode_set_dithering().Signed-off-by Giulio Benetti <[email protected]>Fixes f11adcecbd5f ("drm/sun4i: tcon: Add dithering support forRGB565/RGB666 LCD panels")Reviewed-by Chen-Yu Tsai <[email protected]>
Applied both, thanks!
Maxime
Signature · 3 lines
Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Quoted 98 lines · Giulio Benetti
Il 01/10/2018 11:36, Dan Carpenter ha scritto:
Quoted 9 lines
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.cQuoted 15 lines
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); }Quoted 65 lines
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)) { ^^^^^^^^^^^^^^^^^^Quoted 1 line
Unpossible to be an error pointer!
Quoted 8 lines
So maybe I should use
IS_ERR_OR_NULL()instead ofIS_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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hello Dan,
Il 11/10/2018 09:16, Dan Carpenter ha scritto:
Quoted 3 lines
This is an unreleased Smatch test because:
1) Some things return error pointers depending on the config.
2) PointlessIS_ERR()checks are quite common.
Look forward for new Smatch, very useful.
Quoted 3 lines
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
Quoted 5 lines
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
Signature · 14 lines
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 [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 2/2] drm/sun4i: tcon: prevent tcon->panel dereference if NULL
In reply to Maxime RipardQuoted 8 lines · Maxime Ripard
Quoted 6 lines · Giulio Benetti
If
tcon->panelpointer is NULL, trying to dereference from it (i.e.tcon->panel->connector) will cause a null pointer dereference.Add
tcon->panelnull pointer check before callingsun4i_tcon0_mode_set_dithering().
Quoted 6 lines
Quoted 4 lines
Signed-off-by Giulio Benetti <[email protected]>Fixes f11adcecbd5f ("drm/sun4i: tcon: Add dithering support forRGB565/RGB666 LCD panels")Reviewed-by Chen-Yu Tsai <[email protected]>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
In reply to Maxime RipardQuoted 8 lines · 在 2018-10-08一的 11:21 +0200，Maxime Ripard写道：
Quoted 7 lines · Giulio Benetti
If
tcon->panelpointer is NULL, trying to dereference from it (i.e.tcon->panel->connector) will cause a null pointer dereference.Add
tcon->panelnull pointer check before callingsun4i_tcon0_mode_set_dithering().
Quoted 6 lines
Quoted 4 lines
Signed-off-by Giulio Benetti <[email protected]>Fixes f11adcecbd5f ("drm/sun4i: tcon: Add dithering support forRGB565/RGB666 LCD panels")Reviewed-by Chen-Yu Tsai <[email protected]>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.
Quoted 6 lines
Maxime
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Maxime Ripard
AppliedRe: [PATCH v3 2/2] drm/sun4i: tcon: prevent tcon->panel dereference if NULL
Quoted 14 lines · Icenowy Zheng
Quoted 13 lines · 在 2018-10-08一的 11:21 +0200，Maxime Ripard写道：
Quoted 11 lines · Giulio Benetti
If
tcon->panelpointer is NULL, trying to dereference from it (i.e.tcon->panel->connector) will cause a null pointer dereference.Add
tcon->panelnull pointer check before callingsun4i_tcon0_mode_set_dithering().Signed-off-by Giulio Benetti <[email protected]>Fixes f11adcecbd5f ("drm/sun4i: tcon: Add dithering support forRGB565/RGB666 LCD panels")Reviewed-by Chen-Yu Tsai <[email protected]>
Quoted 6 lines
Quoted 1 line
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
Signature · 3 lines
Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com