Re: [Intel-wired-lan] [PATCH 1/6 iwl-next] idpf: implement virtchnl transaction manager

[Simon Horman <horms@kernel.org>]

On Mon, Jan 22, 2024 at 01:11:20PM -0800, Alan Brady wrote:
> This starts refactoring how virtchnl messages are handled by adding a
> transaction manager (idpf_vc_xn_manager).
> 
> There are two primary motivations here which are to enable handling of
> multiple messages at once and to make it more robust in general. As it
> is right now, the driver may only have one pending message at a time and
> there's no guarantee that the response we receive was actually intended
> for the message we sent prior.
> 
> This works by utilizing a "cookie" field of the message descriptor. It
> is arbitrary what data we put in the cookie and the response is required
> to have the same cookie the original message was sent with. Then using a
> "transaction" abstraction that uses the completion API to pair responses
> to the message it belongs to.
> 
> The cookie works such that the first half is the index to the
> transaction in our array, and the second half is a "salt" that gets
> incremented every message. This enables quick lookups into the array and
> also ensuring we have the correct message. The salt is necessary because
> after, for example, a message times out and we deem the response was
> lost for some reason, we could theoretically reuse the same index but
> using a different salt ensures that when we do actually get a response
> it's not the old message that timed out previously finally coming in.
> Since the number of transactions allocated is U8_MAX and the salt is 8
> bits, we can never have a conflict because we can't roll over the salt
> without using more transactions than we have available.
> 
> This starts by only converting the VIRTCHNL2_OP_VERSION message to use
> this new transaction API. Follow up patches will convert all virtchnl
> messages to use the API.
> 
> Reviewed-by: Igor Bagnucki <igor.bagnucki@intel.com>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Alan Brady <alan.brady@intel.com>
> Co-developed-by: Joshua Hay <joshua.a.hay@intel.com>
> Signed-off-by: Joshua Hay <joshua.a.hay@intel.com>

...

> +/**
> + * idpf_vc_xn_init - Initialize virtchnl transaction object
> + * @vcxn_mngr: pointer to vc transaction manager struct
> + */
> +static void idpf_vc_xn_init(struct idpf_vc_xn_manager *vcxn_mngr)
> +{
> +	int i;
> +
> +	spin_lock_init(&vcxn_mngr->xn_bm_lock);
> +
> +	for (i = 0; i < ARRAY_SIZE(vcxn_mngr->ring); i++) {
> +		struct idpf_vc_xn *xn = &vcxn_mngr->ring[i];
> +
> +		xn->state = IDPF_VC_XN_IDLE;
> +		xn->idx = i;
> +		idpf_vc_xn_release_bufs(xn);
> +		init_completion(&xn->completed);
> +	}

Hi Alan and Joshua,

I'm slightly surprised to see that
it is safe to initialise xn_bm_lock above,
but it needs to be taken below.

> +
> +	spin_lock_bh(&vcxn_mngr->xn_bm_lock);
> +	bitmap_set(vcxn_mngr->free_xn_bm, 0, IDPF_VC_XN_RING_LEN);
> +	spin_unlock_bh(&vcxn_mngr->xn_bm_lock);
> +}