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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3593 bytes --]

On Mon, 2 Jun 2025, Ruhl, Michael J wrote:

> >-----Original Message-----
> >From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> >Sent: Saturday, May 31, 2025 1:24 AM
> >To: Ruhl, Michael J <michael.j.ruhl@intel.com>
> >Cc: platform-driver-x86@vger.kernel.org; intel-xe@lists.freedesktop.org; Hans
> >de Goede <hdegoede@redhat.com>; De Marchi, Lucas
> ><lucas.demarchi@intel.com>; Vivi, Rodrigo <rodrigo.vivi@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@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),

TBH, I wouldn't worry over changing the precedence of returned error 
codes so just go ahead with that change (but please make it in a 
separate patch so it would be easy to revert in the very unlikely case 
that somebody depends on the old behavior).

> >Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 
> Thanks!
> 
> M
> 
> >
> >
> >--
> > i.
> 

-- 
 i.