[PATCH v6] drm/xe: Add a null pointer check

[PATCH v6] drm/xe: Add a null pointer check

[Karthik Poosa]

Add a missing null pointer check.

v2: Release resources before returning error. (Riana)

v3: Update commit message. (Badal)

v4: Change drm_err to XE_WARN_ON. (Anshuman)

v5: Fix XE_WARN_ON check. (Riana)

v6: Update commit message. (Jani)

Fixes: 09d88e3beb64 ("drm/xe/pm: Init pcode and restore vram on power lost")
Signed-off-by: Karthik Poosa <karthik.poosa@intel.com>
Reviewed-by: Riana Tauro <riana.tauro@intel.com>
---
 drivers/gpu/drm/xe/xe_pm.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
index cc650a92c2fc..f6837bd6f9a3 100644
--- a/drivers/gpu/drm/xe/xe_pm.c
+++ b/drivers/gpu/drm/xe/xe_pm.c
@@ -383,6 +383,11 @@ int xe_pm_runtime_resume(struct xe_device *xe)
 	 * really lost power. Detecting primary Gt power is sufficient.
 	 */
 	gt = xe_device_get_gt(xe, 0);
+	if (XE_WARN_ON(!gt)) {
+		err = -ENXIO;
+		goto out;
+	}
+
 	xe->d3cold.power_lost = xe_guc_in_reset(&gt->uc.guc);
 
 	if (xe->d3cold.allowed && xe->d3cold.power_lost) {
-- 
2.25.1

Re: [PATCH v6] drm/xe: Add a null pointer check

[Michal Wajdeczko]

On 25.03.2024 15:12, Karthik Poosa wrote:
> Add a missing null pointer check.
> 
> v2: Release resources before returning error. (Riana)
> 
> v3: Update commit message. (Badal)
> 
> v4: Change drm_err to XE_WARN_ON. (Anshuman)
> 
> v5: Fix XE_WARN_ON check. (Riana)
> 
> v6: Update commit message. (Jani)
> 
> Fixes: 09d88e3beb64 ("drm/xe/pm: Init pcode and restore vram on power lost")
> Signed-off-by: Karthik Poosa <karthik.poosa@intel.com>
> Reviewed-by: Riana Tauro <riana.tauro@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_pm.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> index cc650a92c2fc..f6837bd6f9a3 100644
> --- a/drivers/gpu/drm/xe/xe_pm.c
> +++ b/drivers/gpu/drm/xe/xe_pm.c
> @@ -383,6 +383,11 @@ int xe_pm_runtime_resume(struct xe_device *xe)
>  	 * really lost power. Detecting primary Gt power is sufficient.
>  	 */
>  	gt = xe_device_get_gt(xe, 0);

maybe easier way to silence the tool is to use:

	gt = xe_root_mmio_gt(xe);

that don't have a path with NULL as return

> +	if (XE_WARN_ON(!gt)) {

but if you still want to add the WARN for the impossible case then as we
have xe it is better to use drm_WARN_ON()

> +		err = -ENXIO;
> +		goto out;
> +	}
> +
>  	xe->d3cold.power_lost = xe_guc_in_reset(&gt->uc.guc);
>  
>  	if (xe->d3cold.allowed && xe->d3cold.power_lost) {

Re: [PATCH v6] drm/xe: Add a null pointer check

[Matt Roper]

On Mon, Mar 25, 2024 at 07:42:23PM +0530, Karthik Poosa wrote:
> Add a missing null pointer check.

You have so many versions of this patch that I've lost track of which is
which, but as I commented on an earlier version, this whole patch seems
unnecessary.  All Intel devices will have at least one GT, and we can
never "resume from runtime PM" before those GT(s) get initialized.  So a
NULL return here is impossible.  If you want to be paranoid, you can add
an assert to make that invariant clear and it will get compiled out of
production releases.


Matt

> 
> v2: Release resources before returning error. (Riana)
> 
> v3: Update commit message. (Badal)
> 
> v4: Change drm_err to XE_WARN_ON. (Anshuman)
> 
> v5: Fix XE_WARN_ON check. (Riana)
> 
> v6: Update commit message. (Jani)
> 
> Fixes: 09d88e3beb64 ("drm/xe/pm: Init pcode and restore vram on power lost")
> Signed-off-by: Karthik Poosa <karthik.poosa@intel.com>
> Reviewed-by: Riana Tauro <riana.tauro@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_pm.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> index cc650a92c2fc..f6837bd6f9a3 100644
> --- a/drivers/gpu/drm/xe/xe_pm.c
> +++ b/drivers/gpu/drm/xe/xe_pm.c
> @@ -383,6 +383,11 @@ int xe_pm_runtime_resume(struct xe_device *xe)
>  	 * really lost power. Detecting primary Gt power is sufficient.
>  	 */
>  	gt = xe_device_get_gt(xe, 0);
> +	if (XE_WARN_ON(!gt)) {
> +		err = -ENXIO;
> +		goto out;
> +	}
> +
>  	xe->d3cold.power_lost = xe_guc_in_reset(&gt->uc.guc);
>  
>  	if (xe->d3cold.allowed && xe->d3cold.power_lost) {
> -- 
> 2.25.1
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation

Re: [PATCH v6] drm/xe: Add a null pointer check

[Poosa, Karthik]

On 25-03-2024 23:28, Matt Roper wrote:
> On Mon, Mar 25, 2024 at 07:42:23PM +0530, Karthik Poosa wrote:
>> Add a missing null pointer check.
> You have so many versions of this patch that I've lost track of which is
> which, but as I commented on an earlier version, this whole patch seems
> unnecessary.  All Intel devices will have at least one GT, and we can
> never "resume from runtime PM" before those GT(s) get initialized.  So a
> NULL return here is impossible.  If you want to be paranoid, you can add
> an assert to make that invariant clear and it will get compiled out of
> production releases.
>
>
> Matt
Unfortunately, I missed your comment on earlier version (v2). Will drop 
this patch.
>
>> v2: Release resources before returning error. (Riana)
>>
>> v3: Update commit message. (Badal)
>>
>> v4: Change drm_err to XE_WARN_ON. (Anshuman)
>>
>> v5: Fix XE_WARN_ON check. (Riana)
>>
>> v6: Update commit message. (Jani)
>>
>> Fixes: 09d88e3beb64 ("drm/xe/pm: Init pcode and restore vram on power lost")
>> Signed-off-by: Karthik Poosa <karthik.poosa@intel.com>
>> Reviewed-by: Riana Tauro <riana.tauro@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_pm.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
>> index cc650a92c2fc..f6837bd6f9a3 100644
>> --- a/drivers/gpu/drm/xe/xe_pm.c
>> +++ b/drivers/gpu/drm/xe/xe_pm.c
>> @@ -383,6 +383,11 @@ int xe_pm_runtime_resume(struct xe_device *xe)
>>   	 * really lost power. Detecting primary Gt power is sufficient.
>>   	 */
>>   	gt = xe_device_get_gt(xe, 0);
>> +	if (XE_WARN_ON(!gt)) {
>> +		err = -ENXIO;
>> +		goto out;
>> +	}
>> +
>>   	xe->d3cold.power_lost = xe_guc_in_reset(&gt->uc.guc);
>>   
>>   	if (xe->d3cold.allowed && xe->d3cold.power_lost) {
>> -- 
>> 2.25.1
>>