[PATCH RESEND v3 0/4] DRM: small improvements

[PATCH RESEND v3 0/4] DRM: small improvements

[Luca Ceresoli]

This series brings small improvements to the DRM documentation, logging and
a warning on an incorrect code path.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
Changes in v3:
- patch 3: various fixes suggested by Jani Nikula and kernel test robot
- Updated reviewed-by tags
- Link to v2: https://lore.kernel.org/r/20241106-drm-small-improvements-v2-0-f6e2aef86719@bootlin.com

Changes in v2:
- Added patches 3 and 4
- Updated reviewed-by tags
- Link to v1: https://lore.kernel.org/r/20241018-drm-small-improvements-v1-0-cc316e1a98c9@bootlin.com

---
Luca Ceresoli (4):
      drm/drm_mode_object: fix typo in kerneldoc
      drm/atomic-helper: improve CRTC enabled/connectors mismatch logging message
      drm/mode_object: add drm_mode_object_read_refcount()
      drm/connector: warn when cleaning up a refcounted connector

 drivers/gpu/drm/drm_atomic_helper.c |  5 +++--
 drivers/gpu/drm/drm_connector.c     |  6 ++++++
 drivers/gpu/drm/drm_mode_object.c   | 17 +++++++++++++++++
 include/drm/drm_mode_object.h       |  3 ++-
 4 files changed, 28 insertions(+), 3 deletions(-)
---
base-commit: 42f7652d3eb527d03665b09edac47f85fb600924
change-id: 20241018-drm-small-improvements-1d104cc10280

Best regards,
-- 
Luca Ceresoli <luca.ceresoli@bootlin.com>

[PATCH RESEND v3 1/4] drm/drm_mode_object: fix typo in kerneldoc

[Luca Ceresoli]

Remove unintended extra word.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 include/drm/drm_mode_object.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/drm/drm_mode_object.h b/include/drm/drm_mode_object.h
index 08d7a7f0188fea79e2d8ad5ee6cc5044300f1a26..c68edbd126d04d51221f50aa2b4166475543b59f 100644
--- a/include/drm/drm_mode_object.h
+++ b/include/drm/drm_mode_object.h
@@ -35,7 +35,7 @@ struct drm_file;
  * @id: userspace visible identifier
  * @type: type of the object, one of DRM_MODE_OBJECT\_\*
  * @properties: properties attached to this object, including values
- * @refcount: reference count for objects which with dynamic lifetime
+ * @refcount: reference count for objects with dynamic lifetime
  * @free_cb: free function callback, only set for objects with dynamic lifetime
  *
  * Base structure for modeset objects visible to userspace. Objects can be

-- 
2.34.1

Re: [PATCH RESEND v3 1/4] drm/drm_mode_object: fix typo in kerneldoc

[Louis Chauvet]

On 16/12/24 - 17:40, Luca Ceresoli wrote:
> Remove unintended extra word.
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

Acked-by: Louis Chauvet <louis.chauvet@bootlin.com>

> ---
>  include/drm/drm_mode_object.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/drm/drm_mode_object.h b/include/drm/drm_mode_object.h
> index 08d7a7f0188fea79e2d8ad5ee6cc5044300f1a26..c68edbd126d04d51221f50aa2b4166475543b59f 100644
> --- a/include/drm/drm_mode_object.h
> +++ b/include/drm/drm_mode_object.h
> @@ -35,7 +35,7 @@ struct drm_file;
>   * @id: userspace visible identifier
>   * @type: type of the object, one of DRM_MODE_OBJECT\_\*
>   * @properties: properties attached to this object, including values
> - * @refcount: reference count for objects which with dynamic lifetime
> + * @refcount: reference count for objects with dynamic lifetime
>   * @free_cb: free function callback, only set for objects with dynamic lifetime
>   *
>   * Base structure for modeset objects visible to userspace. Objects can be
> 
> -- 
> 2.34.1
> 

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH RESEND v3 2/4] drm/atomic-helper: improve CRTC enabled/connectors mismatch logging message

[Luca Ceresoli]

This message reports a mismatch between new_crtc_state->enable and
has_connectors, which should be either both true or both false. However it
does not mention which one is true and which is false, which can be useful
for debugging. Add the value of both avriables to the log message.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 43cdf39019a44537794cc5a519d139b0cb77073c..3c3bdef9bcf3c4ffcd861744f6607f317ab0c041 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -666,8 +666,9 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 		}
 
 		if (new_crtc_state->enable != has_connectors) {
-			drm_dbg_atomic(dev, "[CRTC:%d:%s] enabled/connectors mismatch\n",
-				       crtc->base.id, crtc->name);
+			drm_dbg_atomic(dev, "[CRTC:%d:%s] enabled/connectors mismatch (%d/%d)\n",
+				       crtc->base.id, crtc->name,
+				       new_crtc_state->enable, has_connectors);
 
 			return -EINVAL;
 		}

-- 
2.34.1

Re: [PATCH RESEND v3 2/4] drm/atomic-helper: improve CRTC enabled/connectors mismatch logging message

[Louis Chauvet]

On 16/12/24 - 17:40, Luca Ceresoli wrote:
> This message reports a mismatch between new_crtc_state->enable and
> has_connectors, which should be either both true or both false. However it
> does not mention which one is true and which is false, which can be useful
> for debugging. Add the value of both avriables to the log message.
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

Acked-by: Louis	Chauvet	<louis.chauvet@bootlin.com>

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 43cdf39019a44537794cc5a519d139b0cb77073c..3c3bdef9bcf3c4ffcd861744f6607f317ab0c041 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -666,8 +666,9 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>  		}
>  
>  		if (new_crtc_state->enable != has_connectors) {
> -			drm_dbg_atomic(dev, "[CRTC:%d:%s] enabled/connectors mismatch\n",
> -				       crtc->base.id, crtc->name);
> +			drm_dbg_atomic(dev, "[CRTC:%d:%s] enabled/connectors mismatch (%d/%d)\n",
> +				       crtc->base.id, crtc->name,
> +				       new_crtc_state->enable, has_connectors);
>  
>  			return -EINVAL;
>  		}
> 
> -- 
> 2.34.1
> 

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH RESEND v3 3/4] drm/mode_object: add drm_mode_object_read_refcount()

[Luca Ceresoli]

Add a wrapper to kref_read() just like the ones already in place for
kref_get() and kref_put(). This will be used for sanity checks on object
lifetime.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

Changed in v3:

 * use conventions for 'Returns' doc syntax
 * ditch DRM_DEBUG() and as a consequence rework and simplify the entire
   function
 * fix function name in kerneldoc
---
 drivers/gpu/drm/drm_mode_object.c | 17 +++++++++++++++++
 include/drm/drm_mode_object.h     |  1 +
 2 files changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
index df4cc0e8e263d5887a799cf1a61d998234be7158..b9a16aceb926782eb033434eb6967ce9fd2e94f7 100644
--- a/drivers/gpu/drm/drm_mode_object.c
+++ b/drivers/gpu/drm/drm_mode_object.c
@@ -217,6 +217,23 @@ void drm_mode_object_get(struct drm_mode_object *obj)
 }
 EXPORT_SYMBOL(drm_mode_object_get);
 
+/**
+ * drm_mode_object_read_refcount - read the refcount for a mode object
+ * @obj: DRM mode object
+ *
+ * Returns:
+ * The current object refcount if it is a refcounted modeset object, or 0
+ * for any other object.
+ */
+unsigned int drm_mode_object_read_refcount(struct drm_mode_object *obj)
+{
+	if (obj->free_cb)
+		return kref_read(&obj->refcount);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_mode_object_read_refcount);
+
 /**
  * drm_object_attach_property - attach a property to a modeset object
  * @obj: drm modeset object
diff --git a/include/drm/drm_mode_object.h b/include/drm/drm_mode_object.h
index c68edbd126d04d51221f50aa2b4166475543b59f..3d2c739e703888bf4520c61594d480f128d50e56 100644
--- a/include/drm/drm_mode_object.h
+++ b/include/drm/drm_mode_object.h
@@ -123,6 +123,7 @@ struct drm_mode_object *drm_mode_object_find(struct drm_device *dev,
 					     uint32_t id, uint32_t type);
 void drm_mode_object_get(struct drm_mode_object *obj);
 void drm_mode_object_put(struct drm_mode_object *obj);
+unsigned int drm_mode_object_read_refcount(struct drm_mode_object *obj);
 
 int drm_object_property_set_value(struct drm_mode_object *obj,
 				  struct drm_property *property,

-- 
2.34.1

Re: [PATCH RESEND v3 3/4] drm/mode_object: add drm_mode_object_read_refcount()

[Louis Chauvet]

On 16/12/24 - 17:40, Luca Ceresoli wrote:
> Add a wrapper to kref_read() just like the ones already in place for
> kref_get() and kref_put(). This will be used for sanity checks on object
> lifetime.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

Acked-by: Louis Chauvet <louis.chauvet@bootlin.com>

Thanks,
Louis Chauvet
 
> ---
> 
> Changed in v3:
> 
>  * use conventions for 'Returns' doc syntax
>  * ditch DRM_DEBUG() and as a consequence rework and simplify the entire
>    function
>  * fix function name in kerneldoc
> ---
>  drivers/gpu/drm/drm_mode_object.c | 17 +++++++++++++++++
>  include/drm/drm_mode_object.h     |  1 +
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> index df4cc0e8e263d5887a799cf1a61d998234be7158..b9a16aceb926782eb033434eb6967ce9fd2e94f7 100644
> --- a/drivers/gpu/drm/drm_mode_object.c
> +++ b/drivers/gpu/drm/drm_mode_object.c
> @@ -217,6 +217,23 @@ void drm_mode_object_get(struct drm_mode_object *obj)
>  }
>  EXPORT_SYMBOL(drm_mode_object_get);
>  
> +/**
> + * drm_mode_object_read_refcount - read the refcount for a mode object
> + * @obj: DRM mode object
> + *
> + * Returns:
> + * The current object refcount if it is a refcounted modeset object, or 0
> + * for any other object.
> + */
> +unsigned int drm_mode_object_read_refcount(struct drm_mode_object *obj)
> +{
> +	if (obj->free_cb)
> +		return kref_read(&obj->refcount);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_mode_object_read_refcount);
> +
>  /**
>   * drm_object_attach_property - attach a property to a modeset object
>   * @obj: drm modeset object
> diff --git a/include/drm/drm_mode_object.h b/include/drm/drm_mode_object.h
> index c68edbd126d04d51221f50aa2b4166475543b59f..3d2c739e703888bf4520c61594d480f128d50e56 100644
> --- a/include/drm/drm_mode_object.h
> +++ b/include/drm/drm_mode_object.h
> @@ -123,6 +123,7 @@ struct drm_mode_object *drm_mode_object_find(struct drm_device *dev,
>  					     uint32_t id, uint32_t type);
>  void drm_mode_object_get(struct drm_mode_object *obj);
>  void drm_mode_object_put(struct drm_mode_object *obj);
> +unsigned int drm_mode_object_read_refcount(struct drm_mode_object *obj);
>  
>  int drm_object_property_set_value(struct drm_mode_object *obj,
>  				  struct drm_property *property,
> 
> -- 
> 2.34.1
> 

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH RESEND v3 4/4] drm/connector: warn when cleaning up a refcounted connector

[Luca Ceresoli]

Calling drm_connector_cleanup() should only be done via the free_cb =>
.destroy path, which cleans up the struct drm_connector only when the
refcount drops to zero.

A cleanup done with a refcount higher than 0 can result from buggy code,
e.g. by doing cleanup directly in the drivers teardown code. Serious
trouble can happen if this happens, so warn about it.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 drivers/gpu/drm/drm_connector.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index fc35f47e2849ed6786d6223ac9c69e1c359fc648..e0bf9c490af43055de4caaee1580a4befbd608c5 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -624,6 +624,12 @@ void drm_connector_cleanup(struct drm_connector *connector)
 	struct drm_device *dev = connector->dev;
 	struct drm_display_mode *mode, *t;
 
+	/*
+	 * Cleanup must happen when the last ref is put, via the
+	 * drm_connector_free() callback.
+	 */
+	WARN_ON(drm_mode_object_read_refcount(&connector->base) != 0);
+
 	/* The connector should have been removed from userspace long before
 	 * it is finally destroyed.
 	 */

-- 
2.34.1

Re: [PATCH RESEND v3 4/4] drm/connector: warn when cleaning up a refcounted connector

[Louis Chauvet]

On 16/12/24 - 17:40, Luca Ceresoli wrote:
> Calling drm_connector_cleanup() should only be done via the free_cb =>
> .destroy path, which cleans up the struct drm_connector only when the
> refcount drops to zero.
> 
> A cleanup done with a refcount higher than 0 can result from buggy code,
> e.g. by doing cleanup directly in the drivers teardown code. Serious
> trouble can happen if this happens, so warn about it.
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

Acked-by: Louis Chauvet <louis.chauvet@bootlin.com>

> ---
>  drivers/gpu/drm/drm_connector.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index fc35f47e2849ed6786d6223ac9c69e1c359fc648..e0bf9c490af43055de4caaee1580a4befbd608c5 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -624,6 +624,12 @@ void drm_connector_cleanup(struct drm_connector *connector)
>  	struct drm_device *dev = connector->dev;
>  	struct drm_display_mode *mode, *t;
>  
> +	/*
> +	 * Cleanup must happen when the last ref is put, via the
> +	 * drm_connector_free() callback.
> +	 */
> +	WARN_ON(drm_mode_object_read_refcount(&connector->base) != 0);
> +
>  	/* The connector should have been removed from userspace long before
>  	 * it is finally destroyed.
>  	 */
> 
> -- 
> 2.34.1
> 

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH RESEND v3 4/4] drm/connector: warn when cleaning up a refcounted connector

[kerne test robot]

Hello,


the WARN added in this commit is hit in our tests, below just FYI.


kernel test robot noticed "WARNING:at_drivers/gpu/drm/drm_connector.c:#drm_connector_cleanup[drm]" on:

commit: b7e98abf2a95872b85ec5e94c98c98a7c8d10519 ("[PATCH RESEND v3 4/4] drm/connector: warn when cleaning up a refcounted connector")
url: https://github.com/intel-lab-lkp/linux/commits/Luca-Ceresoli/drm-drm_mode_object-fix-typo-in-kerneldoc/20241217-004320
patch link: https://lore.kernel.org/all/20241216-drm-small-improvements-v3-4-78bbc95ac776@bootlin.com/
patch subject: [PATCH RESEND v3 4/4] drm/connector: warn when cleaning up a refcounted connector

in testcase: perf-fuzzer
version: perf-fuzzer-x86_64-a052241-1_20241102
with following parameters:

	runtime: 1h



config: x86_64-rhel-9.4-bpf
compiler: gcc-12
test machine: 20 threads 1 sockets (Commet Lake) with 16G memory

(please refer to attached dmesg/kmsg for entire log/backtrace)



If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202412252329.a12c3be7-lkp@intel.com


The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20241225/202412252329.a12c3be7-lkp@intel.com


[   75.546607][  T377] i915 0000:00:02.0: [drm] [ENCODER:98:DDI A/PHY A] failed to retrieve link info, disabling eDP
[   75.557310][  T377] ------------[ cut here ]------------
[   75.562737][  T377] WARNING: CPU: 9 PID: 377 at drivers/gpu/drm/drm_connector.c:631 drm_connector_cleanup+0x591/0x5c0 [drm]
[   75.574145][  T377] Modules linked in: intel_rapl_msr intel_rapl_common intel_uncore_frequency intel_uncore_frequency_common snd_sof_pci_intel_cnl snd_sof_intel_hda_generic soundwire_intel soundwire_cadence snd_sof_intel_hda_common snd_soc_hdac_hda snd_sof_intel_hda_mlink snd_sof_intel_hda snd_sof_pci snd_sof_xtensa_dsp snd_sof snd_sof_utils snd_soc_acpi_intel_match soundwire_generic_allocation x86_pkg_temp_thermal snd_soc_acpi intel_powerclamp btrfs soundwire_bus snd_soc_avs snd_soc_hda_codec snd_hda_ext_core coretemp snd_soc_core blake2b_generic snd_compress xor zstd_compress snd_hda_intel kvm_intel tps6598x snd_intel_dspcfg raid6_pq netconsole libcrc32c regmap_i2c snd_intel_sdw_acpi i915(+) kvm snd_hda_codec sd_mod crct10dif_pclmul crc32_pclmul snd_hda_core sg cec crc32c_intel snd_hwdep drm_buddy ghash_clmulni_intel ipmi_devintf ipmi_msghandler drm_display_helper snd_pcm sdhci_pci ahci rapl i2c_designware_platform ttm i2c_designware_core libahci snd_timer cqhci intel_cstate drm_kms_helper intel_lpss_pci sdhci i2c_i801
[   75.574336][  T377]  intel_uncore snd mei_me intel_lpss intel_gtt wmi_bmof pcspkr libata mmc_core i2c_smbus ppdev soundcore mei agpgart idma64 intel_pch_thermal intel_wmi_thunderbolt serial_multi_instantiate parport_pc intel_pmc_core video parport intel_vsec pmt_telemetry wmi pinctrl_cannonlake pmt_class acpi_tad acpi_pad serio_raw binfmt_misc drm dm_mod ip_tables x_tables sch_fq_codel
[   75.699513][  T377] CPU: 9 UID: 0 PID: 377 Comm: (udev-worker) Not tainted 6.12.0-rc4-00004-gb7e98abf2a95 #1
[   75.709478][  T377] RIP: 0010:drm_connector_cleanup+0x591/0x5c0 [drm]
[   75.716148][  T377] Code: 00 e8 73 60 6a c2 4c 89 f7 e8 9b 8b 6f c2 48 89 ef e8 73 f0 34 c1 e9 49 fb ff ff 0f 0b 48 89 df e8 64 e3 ff ff e9 c4 fa ff ff <0f> 0b e9 a4 fa ff ff be 03 00 00 00 48 89 ef e8 1b 93 95 c1 e9 21
[   75.735742][  T377] RSP: 0018:ffff88813ef1f2a8 EFLAGS: 00010202
[   75.741757][  T377] RAX: 0000000000000001 RBX: ffff88811ee48000 RCX: dffffc0000000000
[   75.749672][  T377] RDX: 0000000000000003 RSI: ffffffffc03362c5 RDI: ffff88811ee48050
[   75.757588][  T377] RBP: ffff88811ee48000 R08: 0000000000000000 R09: ffffed1023dc900a
[   75.765506][  T377] R10: ffff88811ee48053 R11: 0000000000000000 R12: ffff8881400c8000
[   75.773423][  T377] R13: 0000000000000000 R14: ffffffffc19f9f80 R15: ffffffffc1a011a0
[   75.781342][  T377] FS:  00007f0aece108c0(0000) GS:ffff8883a1e80000(0000) knlGS:0000000000000000
[   75.790216][  T377] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   75.796737][  T377] CR2: 00005654bfbb9378 CR3: 0000000105976006 CR4: 00000000003726f0
[   75.804663][  T377] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   75.812577][  T377] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[   75.820489][  T377] Call Trace:
[   75.823699][  T377]  <TASK>
[   75.826566][  T377]  ? drm_connector_cleanup+0x591/0x5c0 [drm]
[   75.832581][  T377]  ? __warn+0x9d/0x140
[   75.836581][  T377]  ? drm_connector_cleanup+0x591/0x5c0 [drm]
[   75.842597][  T377]  ? report_bug+0x1a6/0x1d0
[   75.847028][  T377]  ? handle_bug+0x53/0xa0
[   75.851288][  T377]  ? exc_invalid_op+0x13/0x40
[   75.855903][  T377]  ? asm_exc_invalid_op+0x16/0x20
[   75.860873][  T377]  ? drm_mode_object_read_refcount+0x35/0x40 [drm]
[   75.867428][  T377]  ? drm_connector_cleanup+0x591/0x5c0 [drm]
[   75.873436][  T377]  ? drm_connector_cleanup+0x34/0x5c0 [drm]
[   75.879362][  T377]  intel_dp_init_connector+0x773/0x920 [i915]
[   75.885913][  T377]  ? __pfx_intel_ddi_prepare_link_retrain+0x10/0x10 [i915]
[   75.893478][  T377]  intel_ddi_init+0x109c/0x1990 [i915]
[   75.899335][  T377]  ? __pfx_intel_ddi_init+0x10/0x10 [i915]
[   75.905503][  T377]  intel_bios_for_each_encoder+0x39/0x60 [i915]
[   75.912129][  T377]  intel_setup_outputs+0x754/0x1080 [i915]
[   75.918307][  T377]  ? intel_vga_disable+0xb4/0x1b0 [i915]
[   75.924306][  T377]  intel_display_driver_probe_nogem+0x1f2/0x2f0 [i915]
[   75.931534][  T377]  i915_driver_probe+0x301/0x7f0 [i915]
[   75.937450][  T377]  ? __pfx_i915_driver_probe+0x10/0x10 [i915]
[   75.943840][  T377]  ? drm_privacy_screen_get+0x1b4/0x1f0 [drm]
[   75.949954][  T377]  ? intel_display_driver_probe_defer+0x4b/0x60 [i915]
[   75.957193][  T377]  ? i915_pci_probe+0x149/0x210 [i915]
[   75.963002][  T377]  ? __pfx_i915_pci_probe+0x10/0x10 [i915]
[   75.969128][  T377]  local_pci_probe+0x72/0xd0
[   75.973650][  T377]  pci_call_probe+0xfc/0x2a0
[   75.978165][  T377]  ? __pfx_pci_call_probe+0x10/0x10
[   75.983305][  T377]  ? pci_match_device+0x1c0/0x230
[   75.988272][  T377]  ? pci_match_device+0x1d8/0x230
[   75.993233][  T377]  pci_device_probe+0x9d/0x150
[   75.997936][  T377]  ? driver_sysfs_add+0xb0/0x130
[   76.002830][  T377]  really_probe+0x13b/0x4e0
[   76.007275][  T377]  __driver_probe_device+0xc8/0x1e0
[   76.012414][  T377]  driver_probe_device+0x4a/0xf0
[   76.017294][  T377]  __driver_attach+0x136/0x290
[   76.021995][  T377]  ? __pfx___driver_attach+0x10/0x10
[   76.027210][  T377]  bus_for_each_dev+0xd0/0x140
[   76.031917][  T377]  ? __pfx_bus_for_each_dev+0x10/0x10
[   76.037233][  T377]  ? bus_add_driver+0x17a/0x320
[   76.042028][  T377]  bus_add_driver+0x19a/0x320
[   76.046636][  T377]  driver_register+0x9c/0x1c0
[   76.051245][  T377]  i915_init+0x21/0xe0 [i915]
[   76.056364][  T377]  ? __pfx_i915_init+0x10/0x10 [i915]
[   76.062066][  T377]  do_one_initcall+0x8e/0x250
[   76.066678][  T377]  ? __pfx_do_one_initcall+0x10/0x10
[   76.071896][  T377]  ? kasan_unpoison+0x23/0x50
[   76.076499][  T377]  ? __kasan_slab_alloc+0x2f/0x70
[   76.081447][  T377]  ? rcu_is_watching+0x1c/0x50
[   76.086149][  T377]  ? __kmalloc_cache_noprof+0x2c4/0x380
[   76.091631][  T377]  ? rcu_is_watching+0x1c/0x50
[   76.096329][  T377]  ? kasan_unpoison+0x23/0x50
[   76.100954][  T377]  do_init_module+0x13a/0x3e0
[   76.105561][  T377]  init_module_from_file+0xcf/0x130
[   76.110688][  T377]  ? __pfx_init_module_from_file+0x10/0x10
[   76.116435][  T377]  ? __lock_release+0x130/0x260
[   76.121821][  T377]  ? idempotent_init_module+0x456/0x470
[   76.127317][  T377]  ? idempotent_init_module+0x456/0x470
[   76.132795][  T377]  ? do_raw_spin_unlock+0x83/0xf0
[   76.137760][  T377]  idempotent_init_module+0x1b1/0x470
[   76.143067][  T377]  ? __pfx_idempotent_init_module+0x10/0x10
[   76.148885][  T377]  ? __seccomp_filter+0x101/0x730
[   76.153855][  T377]  ? cap_capable+0x9f/0xd0
[   76.158197][  T377]  ? security_capable+0x49/0xf0
[   76.162992][  T377]  __x64_sys_finit_module+0x78/0xd0
[   76.168126][  T377]  do_syscall_64+0x8c/0x170
[   76.172562][  T377]  ? lockdep_hardirqs_on_prepare+0x131/0x200
[   76.178480][  T377]  ? syscall_exit_to_user_mode+0xa2/0x2a0
[   76.184143][  T377]  ? do_syscall_64+0x98/0x170
[   76.188758][  T377]  ? ksys_read+0xdc/0x170
[   76.193029][  T377]  ? __pfx_ksys_read+0x10/0x10
[   76.197721][  T377]  ? __pfx___seccomp_filter+0x10/0x10
[   76.203026][  T377]  ? mark_held_locks+0x24/0x90
[   76.207724][  T377]  ? lockdep_hardirqs_on_prepare+0x131/0x200
[   76.213636][  T377]  ? syscall_exit_to_user_mode+0xa2/0x2a0
[   76.219287][  T377]  ? do_syscall_64+0x98/0x170
[   76.223898][  T377]  ? mark_held_locks+0x24/0x90
[   76.228599][  T377]  ? lockdep_hardirqs_on_prepare+0x131/0x200
[   76.234514][  T377]  ? syscall_exit_to_user_mode+0xa2/0x2a0
[   76.240174][  T377]  ? do_syscall_64+0x98/0x170
[   76.244781][  T377]  ? lockdep_hardirqs_on_prepare+0x131/0x200
[   76.250698][  T377]  ? syscall_exit_to_user_mode+0xa2/0x2a0
[   76.256359][  T377]  ? do_syscall_64+0x98/0x170
[   76.260965][  T377]  ? __pfx_ksys_read+0x10/0x10
[   76.265660][  T377]  ? mark_held_locks+0x24/0x90
[   76.270371][  T377]  ? lockdep_hardirqs_on_prepare+0x131/0x200
[   76.276280][  T377]  ? syscall_exit_to_user_mode+0xa2/0x2a0
[   76.281940][  T377]  ? do_syscall_64+0x98/0x170
[   76.286542][  T377]  ? clear_bhb_loop+0x25/0x80
[   76.291148][  T377]  ? clear_bhb_loop+0x25/0x80
[   76.295752][  T377]  ? clear_bhb_loop+0x25/0x80
[   76.300364][  T377]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[   76.306191][  T377] RIP: 0033:0x7f0aed521719
[   76.310543][  T377] Code: 08 89 e8 5b 5d c3 66 2e 0f 1f 84 00 00 00 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d b7 06 0d 00 f7 d8 64 89 01 48
[   76.330135][  T377] RSP: 002b:00007ffee75cdb88 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
[   76.338496][  T377] RAX: ffffffffffffffda RBX: 0000563966406a10 RCX: 00007f0aed521719
[   76.346406][  T377] RDX: 0000000000000000 RSI: 00007f0aed6b4efd RDI: 0000000000000014
[   76.354322][  T377] RBP: 00007f0aed6b4efd R08: 0000000000000000 R09: 00005639663cee60
[   76.362242][  T377] R10: 0000000000000014 R11: 0000000000000246 R12: 0000000000020000
[   76.370158][  T377] R13: 0000000000000000 R14: 0000563966417500 R15: 00007ffee75cddc0
[   76.378097][  T377]  </TASK>
[   76.381044][  T377] irq event stamp: 118993
[   76.385295][  T377] hardirqs last  enabled at (119007): [<ffffffff8125e472>] __up_console_sem+0x52/0x60
[   76.394797][  T377] hardirqs last disabled at (119026): [<ffffffff8125e457>] __up_console_sem+0x37/0x60
[   76.404286][  T377] softirqs last  enabled at (119020): [<ffffffff81153515>] handle_softirqs+0x255/0x370
[   76.413859][  T377] softirqs last disabled at (119015): [<ffffffff81153732>] __irq_exit_rcu+0xf2/0x110
[   76.423251][  T377] ---[ end trace 0000000000000000 ]---
[   76.457822][  T377] [drm] Initialized i915 1.6.0 for 0000:00:02.0 on minor 0
[   76.512348][  T377] ACPI: video: Video Device [GFX0] (multi-head: yes  rom: no  post: no)
[   76.531042][  T377] input: Video Bus as /devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/LNXVIDEO:00/input/input5
[   76.602276][  T377] i915 0000:00:02.0: [drm] Cannot find any crtc or sizes


-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH RESEND v3 4/4] drm/connector: warn when cleaning up a refcounted connector

[Luca Ceresoli]

Hello,

On Wed, 25 Dec 2024 23:15:53 +0800
kerne test robot <oliver.sang@intel.com> wrote:

> the WARN added in this commit is hit in our tests, below just FYI.
> 
> kernel test robot noticed "WARNING:at_drivers/gpu/drm/drm_connector.c:#drm_connector_cleanup[drm]" on:
... 
> [   75.546607][  T377] i915 0000:00:02.0: [drm] [ENCODER:98:DDI A/PHY A] failed to retrieve link info, disabling eDP
> [   75.557310][  T377] ------------[ cut here ]------------
> [   75.562737][  T377] WARNING: CPU: 9 PID: 377 at drivers/gpu/drm/drm_connector.c:631 drm_connector_cleanup+0x591/0x5c0 [drm]
...
> [   75.820489][  T377] Call Trace:
> [   75.823699][  T377]  <TASK>
> [   75.826566][  T377]  ? drm_connector_cleanup+0x591/0x5c0 [drm]
> [   75.832581][  T377]  ? __warn+0x9d/0x140
> [   75.836581][  T377]  ? drm_connector_cleanup+0x591/0x5c0 [drm]
> [   75.842597][  T377]  ? report_bug+0x1a6/0x1d0
> [   75.847028][  T377]  ? handle_bug+0x53/0xa0
> [   75.851288][  T377]  ? exc_invalid_op+0x13/0x40
> [   75.855903][  T377]  ? asm_exc_invalid_op+0x16/0x20
> [   75.860873][  T377]  ? drm_mode_object_read_refcount+0x35/0x40 [drm]
> [   75.867428][  T377]  ? drm_connector_cleanup+0x591/0x5c0 [drm]
> [   75.873436][  T377]  ? drm_connector_cleanup+0x34/0x5c0 [drm]
> [   75.879362][  T377]  intel_dp_init_connector+0x773/0x920 [i915]

OK, so we have this warning because intel_dp_init_connector() does in
the error path (stripped code):

intel_dp_init_connector()
{
   ...
   drm_connector_init_with_ddc(); // sets refcount to 1
   ...
   if (<some error condition>) {
      goto fail;
   }
   ...

fail:
   drm_connector_cleanup(connector); // refcount == 1, warning triggers here
}

My patch is based on the assumption that a connector is always freed by
drm_connector_put() when refcount goes to 0.

The code here (and in other drivers) is an exception that makes sense
specifically for error handling during probe or initialization, and
only when the connector pointer has not been taken by other parts of
the code. However it makes the warning check unavoidably generate false
positives.

At first sight, false positives can be removed by replacing:

-drm_connector_cleanup(connector);
+drm_connector_put(connector);

and letting drm_connector_put() end up in calling the .destroy func.
However that imposes to ensure .destroy is idempotent and does not
"destroy too much", by code inspection at least. By a quick search I
counted about 30 instances.

So, I think there are three options:

 1. this patch is useful (and it helped Luca find an actual bug in his
    code) so we want it but first we need to remove the false positives
 2. we want to support _cleanup instead of a _put() in the error path,
    so let's remove this patch
 3. let's keep the patch and ignore the warning: future patches adding
    _cleanup() in the error path could get a kernel test robot notice
    like this, so it would be good to use _put() in future drivers

Thoughts about this?

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com