Re: [PATCH v3 5/6] x86/microcode/intel: Support mailbox transfer

[Dave Hansen <dave.hansen@intel.com>]

On 4/16/25 10:22, Chang S. Bae wrote:
> On 4/16/2025 7:14 AM, Chao Gao wrote:
>>> +/*
>>> + * Wait for the hardware to complete a transaction.
>>> + * Return true on success, false on failure.
>>> + */
>>> +static bool wait_for_transaction(struct staging_state *ss)
>>> +{
>>> +    u32 timeout, status;
>>> +
>>> +    /* Allow time for hardware to complete the operation: */
>>> +    for (timeout = 0; timeout < MBOX_XACTION_TIMEOUT_MS; timeout++) {
>>> +        msleep(1);
>>> +
>>> +        status = readl(ss->mmio_base + MBOX_STATUS_OFFSET);
>>> +        /* Break out early if the hardware is ready: */
>>> +        if (status & MASK_MBOX_STATUS_READY)
>>> +            break;
>>
>> Shouldn't we exit the loop if the MASK_MBOX_STATUS_ERROR is set, or is
>> the
>> ERROR bit always set in conjunction with the READY bit?
>>
>>> +    }
>>> +
>>> +    status = readl(ss->mmio_base + MBOX_STATUS_OFFSET);
>>
>> I still think this read is not needed.
> 
> No, if it times out, it could exit without having read the status.

Can it? There are only two ways out of the loop: the explicit break and
the implicit break from the 'timeout' check. Both happen at the "end" of
the loop, unless the timeout< check failed up front:

	for (timeout = 0; timeout < MBOX_XACTION_TIMEOUT_MS; timeout++){
		msleep(1);
		status = readl(ss->mmio_base + MBOX_STATUS_OFFSET);
		if (status & MASK_MBOX_STATUS_READY)
			break;
	}

> But this is a slow path — instead of trying to save a few cycles, I
> wanted to present the logic more clearly:
> 
>   * Give the hardware enough time, but not forever.
>     * Oh, we don't need to wait anymore if it's done
> 
>   * After waiting, check the status and handle it properly.
> 
> Isn’t it clearer to read the status after the wait, right before the
> error handling?

It's simpler to read the code the way you have it. You don't have to
consider if 'status' could be uninitialized. It makes the "timeout loop"
*COMPLETELY* independent from the rest of the function. It could be:

foo()
{
	spin_until_ready();

	status = readl(ss->mmio_base + MBOX_STATUS_OFFSET);

	...
}

for example.

It's less likely to break, too. Let's say someone added a new break:

	for (timeout = 0; timeout < MBOX_XACTION_TIMEOUT_MS; timeout++){
+		if (some_other_condition());
+			break;
		msleep(1);
		status = readl(ss->mmio_base + MBOX_STATUS_OFFSET);
		if (status & MASK_MBOX_STATUS_READY)
			break;
	}

you'd have a bug on your hands.

I'm fine with the way you have it. I probably would have written it that
way too.