[PATCH 03/10] platform/x86/intel/pmt: use guard(mutex)

[Ruhl, Michael J]

>-----Original Message-----
>From: Ilpo Järvinen <ilpo.jarvinen at linux.intel.com>
>Sent: Saturday, May 31, 2025 1:24 AM
>To: Ruhl, Michael J <michael.j.ruhl at intel.com>
>Cc: platform-driver-x86 at vger.kernel.org; intel-xe at lists.freedesktop.org; Hans
>de Goede <hdegoede at redhat.com>; De Marchi, Lucas
><lucas.demarchi at intel.com>; Vivi, Rodrigo <rodrigo.vivi at intel.com>
>Subject: Re: [PATCH 03/10] platform/x86/intel/pmt: use guard(mutex)
>
>On Fri, 30 May 2025, Michael J. Ruhl wrote:
>
>> Update the mutex paths to use the new guard() mechanism.
>>
>> With the removal of goto, do some minor cleanup of the current
>> logic path.
>>
>> Signed-off-by: Michael J. Ruhl <michael.j.ruhl at intel.com>
>> ---
>>  drivers/platform/x86/intel/pmt/crashlog.c | 32 +++++++++++------------
>>  1 file changed, 15 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c
>b/drivers/platform/x86/intel/pmt/crashlog.c
>> index d40c8e212733..c6d8a7a61d39 100644
>> --- a/drivers/platform/x86/intel/pmt/crashlog.c
>> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
>> @@ -155,9 +155,9 @@ enable_store(struct device *dev, struct
>device_attribute *attr,
>>  	if (result)
>>  		return result;
>>
>> -	mutex_lock(&entry->control_mutex);
>> +	guard(mutex)(&entry->control_mutex);
>> +
>>  	pmt_crashlog_set_disable(&entry->entry, !enabled);
>> -	mutex_unlock(&entry->control_mutex);
>>
>>  	return count;
>>  }
>> @@ -189,26 +189,24 @@ trigger_store(struct device *dev, struct
>device_attribute *attr,
>>  	if (result)
>>  		return result;
>>
>> -	mutex_lock(&entry->control_mutex);
>> +	guard(mutex)(&entry->control_mutex);
>>
>>  	if (!trigger) {
>>  		pmt_crashlog_set_clear(&entry->entry);
>> -	} else if (pmt_crashlog_complete(&entry->entry)) {
>> -		/* we cannot trigger a new crash if one is still pending */
>> -		result = -EEXIST;
>> -		goto err;
>> -	} else if (pmt_crashlog_disabled(&entry->entry)) {
>> -		/* if device is currently disabled, return busy */
>> -		result = -EBUSY;
>> -		goto err;
>> -	} else {
>> -		pmt_crashlog_set_execute(&entry->entry);
>> +		return count;
>>  	}
>>
>> -	result = count;
>> -err:
>> -	mutex_unlock(&entry->control_mutex);
>> -	return result;
>> +	/* we cannot trigger a new crash if one is still pending */
>> +	if (pmt_crashlog_complete(&entry->entry))
>> +		return -EEXIST;
>> +
>> +	/* if device is currently disabled, return busy */
>> +	if (pmt_crashlog_disabled(&entry->entry))
>> +		return -EBUSY;
>> +
>> +	pmt_crashlog_set_execute(&entry->entry);
>> +
>> +	return count;
>>  }
>>  static DEVICE_ATTR_RW(trigger);
>
>Thanks, the control flow is very straightforward after this change.

Checking for the disable bit after checking for the complete doesn't really make sense to me, 
but I was concerned with "changing" the user experience...

Is this something that can be "updated"? (i.e. swapping the complete and disabled checks),

>Reviewed-by: Ilpo Järvinen <ilpo.jarvinen at linux.intel.com>

Thanks!

M

>
>
>--
> i.