* [bug report] drm/amd/dc: Add dc display driver (v2)
@ 2019-01-09 19:29 Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2019-01-09 19:29 UTC (permalink / raw)
To: harry.wentland-5C7GfCeVMHo; +Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Hello Harry Wentland,
This is a semi-automatic email about new static checker warnings.
The patch 4562236b3bc0: "drm/amd/dc: Add dc display driver (v2)" from
Sep 12, 2017, leads to the following Smatch complaint:
drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc.c:754 construct()
error: we previously assumed 'dc->current_state' could be null (see line 677)
drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc.c
676
677 if (!dc->current_state) {
^^^^^^^^^^^^^^^^^^
678 dm_error("%s: failed to create validate ctx\n", __func__);
679 goto fail;
^^^^^^^^^
680 }
681
682 /* Create logger */
683
684 dc_ctx->dce_environment = init_params->dce_environment;
685
686 dc_version = resource_parse_asic_id(init_params->asic_id);
687 dc_ctx->dce_version = dc_version;
688
689 /* Resource should construct all asic specific resources.
690 * This should be the only place where we need to parse the asic id
691 */
692 if (init_params->vbios_override)
693 dc_ctx->dc_bios = init_params->vbios_override;
694 else {
695 /* Create BIOS parser */
696 struct bp_init_data bp_init_data;
697
698 bp_init_data.ctx = dc_ctx;
699 bp_init_data.bios = init_params->asic_id.atombios_base_address;
700
701 dc_ctx->dc_bios = dal_bios_parser_create(
702 &bp_init_data, dc_version);
703
704 if (!dc_ctx->dc_bios) {
705 ASSERT_CRITICAL(false);
706 goto fail;
707 }
708
709 dc_ctx->created_bios = true;
710 }
711
712 /* Create I2C AUX */
713 dc_ctx->i2caux = dal_i2caux_create(dc_ctx);
714
715 if (!dc_ctx->i2caux) {
716 ASSERT_CRITICAL(false);
717 goto fail;
718 }
719
720 dc_ctx->perf_trace = dc_perf_trace_create();
721 if (!dc_ctx->perf_trace) {
722 ASSERT_CRITICAL(false);
723 goto fail;
724 }
725
726 /* Create GPIO service */
727 dc_ctx->gpio_service = dal_gpio_service_create(
728 dc_version,
729 dc_ctx->dce_environment,
730 dc_ctx);
731
732 if (!dc_ctx->gpio_service) {
733 ASSERT_CRITICAL(false);
734 goto fail;
735 }
736
737 dc->res_pool = dc_create_resource_pool(
738 dc,
739 init_params->num_virtual_links,
740 dc_version,
741 init_params->asic_id);
742 if (!dc->res_pool)
743 goto fail;
744
745 dc_resource_state_construct(dc, dc->current_state);
746
747 if (!create_links(dc, init_params->num_virtual_links))
748 goto fail;
749
750 return true;
751
752 fail:
753
754 destruct(dc);
^^
"dc->current_state" gets dereferenced inside the function. This style
of one function cleans up everything error handling is always buggy...
:/
https://plus.google.com/u/0/106378716002406849458/posts/1Ud9JbaYnPr
755 return false;
756 }
regards,
dan carpenter
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 3+ messages in thread
* [bug report] drm/amd/dc: Add dc display driver (v2)
@ 2018-11-01 9:48 Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2018-11-01 9:48 UTC (permalink / raw)
To: harry.wentland-5C7GfCeVMHo; +Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Hello Harry Wentland,
The patch 4562236b3bc0: "drm/amd/dc: Add dc display driver (v2)" from
Sep 12, 2017, leads to the following static checker warning:
drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc.c:688 construct()
error: we previously assumed 'dc->current_state' could be null (see line 617)
drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc.c
519 static void destruct(struct dc *dc)
520 {
521 dc_release_state(dc->current_state);
^^^^^^^^^^^^^^^^^
destruct assumes that dc->current_state is non-NULL
522 dc->current_state = NULL;
523
524 destroy_links(dc);
525
526 dc_destroy_resource_pool(dc);
527
528 if (dc->ctx->gpio_service)
^^^^^^^
This is also a NULL dereference bug but Smatch does not catch it.
529 dal_gpio_service_destroy(&dc->ctx->gpio_service);
530
531 if (dc->ctx->i2caux)
532 dal_i2caux_destroy(&dc->ctx->i2caux);
533
534 if (dc->ctx->created_bios)
535 dal_bios_parser_destroy(&dc->ctx->dc_bios);
536
537 kfree(dc->ctx);
538 dc->ctx = NULL;
539
540 kfree(dc->bw_vbios);
541 dc->bw_vbios = NULL;
542
543 kfree(dc->bw_dceip);
544 dc->bw_dceip = NULL;
545
546 #ifdef CONFIG_DRM_AMD_DC_DCN1_0
547 kfree(dc->dcn_soc);
548 dc->dcn_soc = NULL;
549
550 kfree(dc->dcn_ip);
551 dc->dcn_ip = NULL;
552
553 #endif
554 }
555
556 static bool construct(struct dc *dc,
557 const struct dc_init_data *init_params)
558 {
559 struct dc_context *dc_ctx;
560 struct bw_calcs_dceip *dc_dceip;
561 struct bw_calcs_vbios *dc_vbios;
562 #ifdef CONFIG_DRM_AMD_DC_DCN1_0
563 struct dcn_soc_bounding_box *dcn_soc;
564 struct dcn_ip_params *dcn_ip;
565 #endif
566
567 enum dce_version dc_version = DCE_VERSION_UNKNOWN;
568
569 dc_dceip = kzalloc(sizeof(*dc_dceip), GFP_KERNEL);
570 if (!dc_dceip) {
571 dm_error("%s: failed to create dceip\n", __func__);
572 goto fail;
573 }
574
575 dc->bw_dceip = dc_dceip;
576
577 dc_vbios = kzalloc(sizeof(*dc_vbios), GFP_KERNEL);
578 if (!dc_vbios) {
579 dm_error("%s: failed to create vbios\n", __func__);
580 goto fail;
581 }
582
583 dc->bw_vbios = dc_vbios;
584 #ifdef CONFIG_DRM_AMD_DC_DCN1_0
585 dcn_soc = kzalloc(sizeof(*dcn_soc), GFP_KERNEL);
586 if (!dcn_soc) {
587 dm_error("%s: failed to create dcn_soc\n", __func__);
588 goto fail;
589 }
590
591 dc->dcn_soc = dcn_soc;
592
593 dcn_ip = kzalloc(sizeof(*dcn_ip), GFP_KERNEL);
594 if (!dcn_ip) {
595 dm_error("%s: failed to create dcn_ip\n", __func__);
596 goto fail;
597 }
598
599 dc->dcn_ip = dcn_ip;
600 #endif
601
602 dc_ctx = kzalloc(sizeof(*dc_ctx), GFP_KERNEL);
603 if (!dc_ctx) {
604 dm_error("%s: failed to create ctx\n", __func__);
605 goto fail;
606 }
607
608 dc_ctx->cgs_device = init_params->cgs_device;
609 dc_ctx->driver_context = init_params->driver;
610 dc_ctx->dc = dc;
611 dc_ctx->asic_id = init_params->asic_id;
612 dc_ctx->dc_sink_id_count = 0;
613 dc->ctx = dc_ctx;
614
615 dc->current_state = dc_create_state();
616
617 if (!dc->current_state) {
^^^^^^^^^^^^^^^^^^
Check for NULL.
618 dm_error("%s: failed to create validate ctx\n", __func__);
619 goto fail;
620 }
621
622 /* Create logger */
623
624 dc_ctx->dce_environment = init_params->dce_environment;
625
626 dc_version = resource_parse_asic_id(init_params->asic_id);
627 dc_ctx->dce_version = dc_version;
628
629 /* Resource should construct all asic specific resources.
630 * This should be the only place where we need to parse the asic id
631 */
632 if (init_params->vbios_override)
633 dc_ctx->dc_bios = init_params->vbios_override;
634 else {
635 /* Create BIOS parser */
636 struct bp_init_data bp_init_data;
637
638 bp_init_data.ctx = dc_ctx;
639 bp_init_data.bios = init_params->asic_id.atombios_base_address;
640
641 dc_ctx->dc_bios = dal_bios_parser_create(
642 &bp_init_data, dc_version);
643
644 if (!dc_ctx->dc_bios) {
645 ASSERT_CRITICAL(false);
646 goto fail;
647 }
648
649 dc_ctx->created_bios = true;
650 }
651
652 /* Create I2C AUX */
653 dc_ctx->i2caux = dal_i2caux_create(dc_ctx);
654
655 if (!dc_ctx->i2caux) {
656 ASSERT_CRITICAL(false);
657 goto fail;
658 }
659
660 /* Create GPIO service */
661 dc_ctx->gpio_service = dal_gpio_service_create(
662 dc_version,
663 dc_ctx->dce_environment,
664 dc_ctx);
665
666 if (!dc_ctx->gpio_service) {
667 ASSERT_CRITICAL(false);
668 goto fail;
669 }
670
671 dc->res_pool = dc_create_resource_pool(
672 dc,
673 init_params->num_virtual_links,
674 dc_version,
675 init_params->asic_id);
676 if (!dc->res_pool)
677 goto fail;
678
679 dc_resource_state_construct(dc, dc->current_state);
680
681 if (!create_links(dc, init_params->num_virtual_links))
682 goto fail;
683
684 return true;
685
686 fail:
687
688 destruct(dc);
689 return false;
690 }
regards,
dan carpenter
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 3+ messages in thread
* [bug report] drm/amd/dc: Add dc display driver (v2)
@ 2017-11-06 8:20 Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2017-11-06 8:20 UTC (permalink / raw)
To: harry.wentland-5C7GfCeVMHo; +Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Hello Harry Wentland,
The patch 4562236b3bc0: "drm/amd/dc: Add dc display driver (v2)" from
Sep 12, 2017, leads to the following static checker warning:
drivers/gpu/drm/amd/amdgpu/../display/dc/dm_services.h:117 get_reg_field_value_ex()
warn: mask and shift to zero
drivers/gpu/drm/amd/display/dc/dce/dce_link_encoder.c
1357 void dce110_link_encoder_enable_hpd(struct link_encoder *enc)
1358 {
1359 struct dce110_link_encoder *enc110 = TO_DCE110_LINK_ENC(enc);
1360 struct dc_context *ctx = enc110->base.ctx;
1361 uint32_t addr = HPD_REG(DC_HPD_CONTROL);
1362 uint32_t hpd_enable = 0;
^^^^^^^^^^^^^^
This is always just zero so the static checker complains:
1363 uint32_t value = dm_read_reg(ctx, addr);
1364
1365 get_reg_field_value(hpd_enable, DC_HPD_CONTROL, DC_HPD_EN);
^^^^^^^^^^
When we pass zero to here, we know that the answer is always going to
be zero. But then we ignore the result from get_reg_field_value()
and I'm going to change Smatch so that that also generates a static
checker warning. What's going on???
1366
1367 if (hpd_enable == 0)
^^^^^^^^^^^^^^^
This is true always.
1368 set_reg_field_value(value, 1, DC_HPD_CONTROL, DC_HPD_EN);
^^^^^^^^^^^^^^^^^^^^
And we don't use the return for this either? ARgh!! This whole
function is a no-op? My Brian is melting???
1369 }
regards,
dan carpenter
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-01-09 19:29 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-09 19:29 [bug report] drm/amd/dc: Add dc display driver (v2) Dan Carpenter
-- strict thread matches above, loose matches on Subject: below --
2018-11-01 9:48 Dan Carpenter
2017-11-06 8:20 Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.