Re: [PATCH v2a 4/6] x86/microcode/intel: Implement staging handler

[Chao Gao <chao.gao@intel.com>]

> /*
>- * Handle the staging process using the mailbox MMIO interface.
>- * Return the result state.
>+ * Prepare for a new microcode transfer by resetting hardware and
>+ * initializing software states.
>+ */
>+static void init_stage(struct staging_state *ss)
>+{
>+	ss->ucode_ptr = ucode_patch_late;
>+	ss->ucode_len = get_totalsize(&ucode_patch_late->hdr);
>+
>+	/* Reset tracking variables */
>+	ss->offset = 0;
>+	ss->bytes_sent = 0;

Nit: no need to reset them, as

>+	struct staging_state ss = {};

in do_stage() will zero the whole structure.

>+/*
>+ * Handle the staging process using the mailbox MMIO interface. The
>+ * microcode image is transferred in chunks until completion. Return the
>+ * result state.
>  */
> static enum ucode_state do_stage(u64 mmio_pa)
> {
>-	pr_debug_once("Staging implementation is pending.\n");
>-	return UCODE_ERROR;
>+	struct staging_state ss = {};
>+
>+	ss.mmio_base = ioremap(mmio_pa, MBOX_REG_NUM * MBOX_REG_SIZE);
>+	if (WARN_ON_ONCE(!ss.mmio_base))
>+		return UCODE_ERROR;
>+
>+	init_stage(&ss);
>+
>+	/* Perform the staging process while within the retry limit */
>+	while (!is_stage_complete(ss.offset) && can_send_next_chunk(&ss)) {
>+		/* Send a chunk of microcode each time: */
>+		if (!send_data_chunk(&ss))
>+			break;
>+		/*
>+		 * Then, ask the hardware which piece of the image it
>+		 * needs next. The same piece may be sent more than once.
>+		 */
>+		if (!fetch_next_offset(&ss))
>+			break;

why send_data_chunk() and fetch_next_offset() return a boolean instead of
an error or ucode_state?

Using the return value to indicate just success or failure, while relying
on another variable to report detailed error/state, seems a bit clumsy to
me.

>+	}
>+
>+	iounmap(ss.mmio_base);
>+	return ss.state;
> }
> 
> static void stage_microcode(void)
>-- 
>2.45.2
>