Re: [PATCH 1/2] cxl: Fix bug where AFU disable operation had no effect

[Frederic Barrat <fbarrat@linux.vnet.ibm.com>]

Hi Ian,

> -static int afu_control(struct cxl_afu *afu, u64 command,
> +static int afu_control(struct cxl_afu *afu, u64 command, u64 clear,
>   		       u64 result, u64 mask, bool enabled)

I'm not a big fan of the new "clear" argument, which forces us to pass 
an extra 0 most of the time. Why not always clearing the "action" bits 
of the register before applying the command? They are mutually 
exclusive, so it should work. I.e. :

+	AFU_Cntl = cxl_p2n_read(afu, CXL_AFU_Cntl_An);
+	AFU_Cntl &= ~(CXL_AFU_Cntl_An_E | CXL_AFU_Cntl_An_RA);
+	AFU_Cntl |= command;




> @@ -595,7 +597,16 @@ static int deactivate_afu_directed(struct cxl_afu *afu)
>   	cxl_sysfs_afu_m_remove(afu);
>   	cxl_chardev_afu_remove(afu);
>
> -	cxl_ops->afu_reset(afu);
> +	if (afu->adapter->native->sl_ops->needs_reset_before_disable) {
> +		/*
> +		 * XXX: We may be able to do away with this entirely - it is
> +		 * possible that this was only ever needed due to a bug where
> +		 * the disable operation did not clear the enable bit, however
> +		 * I will only consider dropping this after more regression
> +		 * testing on earlier PSL images.
> +		 */
> +		cxl_ops->afu_reset(afu);
> +	}
>   	cxl_afu_disable(afu);
>   	cxl_psl_purge(afu);
>
> @@ -735,7 +746,16 @@ static int native_attach_process(struct cxl_context *ctx, bool kernel,
>
>   static inline int detach_process_native_dedicated(struct cxl_context *ctx)
>   {
> -	cxl_ops->afu_reset(ctx->afu);
> +	if (ctx->afu->adapter->native->sl_ops->needs_reset_before_disable) {
> +		/*
> +		 * XXX: We may be able to do away with this entirely - it is
> +		 * possible that this was only ever needed due to a bug where
> +		 * the disable operation did not clear the enable bit, however
> +		 * I will only consider dropping this after more regression
> +		 * testing on earlier PSL images.
> +		 */
> +		cxl_ops->afu_reset(ctx->afu);
> +	}

For dedicated mode, the CAIA recommends an explicit reset of the AFU 
(section 2.1.1).

For directed mode, CAIA says it's AFU-specific. So for xsl, we only 
disable the afu and purge the xsl. Are we getting rid of the reset 
because it's useless in that environment, or because it times out? If 
just because of timeout, would it make sense to switch the order and 
disable first, then reset? I don't see any afu-reset on the next activation.

   Fred