[PATCH 03/10] platform/x86/intel/pmt: use guard(mutex)
Ilpo Järvinen
ilpo.jarvinen at linux.intel.com
Mon Jun 2 15:37:33 UTC 2025
On Mon, 2 Jun 2025, Ruhl, Michael J wrote:
> >-----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),
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 at linux.intel.com>
>
> Thanks!
>
> M
>
> >
> >
> >--
> > i.
>
--
i.
More information about the Intel-xe
mailing list