Re: [PATCH v1 2/3] firmware: coreboot: loader for Linux-owned SMI handler

[Brian Norris <briannorris@chromium.org>]

On Sat, Jun 14, 2025 at 02:59:33PM +0200, Michal Gorlas wrote:
> On Thu, Jun 12, 2025 at 03:38:21PM -0700, Brian Norris wrote:
> > > +	mdelay(100);
> > 
> > Why the delay? At least use a comment to tell us. And if it's really
> > needed, use msleep(), not mdelay(). scripts/checkpatch.pl should have
> > warned you. And, please use scripts/checkpatch.pl if you aren't already
> > ;)
> > 
> 
> Long story short, SMIs on real hardware like to take longer from time to
> time, and the delay was a "safeguard". It is probably not the proper way
> to handle it, but locking here was not helpful at all, lock was released
> regardless of CPU being still in SMM context (I assume due to SMIs being
> invisible to whatever runs in ring-0). Have to admit though, that 100ms
> is a consequence of trial and error. I would actually use some on advice
> how to handle this properly.

Sorry, I don't have any advice here at the moment.

> scripts/checkpatch.pl was not complaining
> about it. It only gave me:
> 
> WARNING: quoted string split across lines
> #57: FILE: drivers/firmware/google/mm_loader.c:57:
> +		     ".return_not_changed:"
> +		     "movq	%%rcx, %[status]\n\t"
> 
> total: 0 errors, 1 warnings, 0 checks, 186 lines checked

I must have either misread or misremembered checkpatch's behavior.
Possibly both. It has various other delay-realted warnings that point
you at the kerneldoc comments for mdelay() and msleep() though, and the
mdelay() comments say:

 * Please double check, whether mdelay() is the right way to go or whether a
 * refactoring of the code is the better variant to be able to use msleep()
 * instead.

Brian