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

["Chang S. Bae" <chang.seok.bae@intel.com>]

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.

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?

Also, reading the status inside the loop is just a way to determine 
whether to break — not to interpret or act on it. That should come after 
the wait completes.