Re: [PATCH] cxl: Set the valid bit in PE for dedicated mode

[Michael Ellerman <mpe@ellerman.id.au>]

Vaibhav Jain <vaibhav@linux.vnet.ibm.com> writes:

> Hi Mpe,
>
> Thanks for reviewing the patch
>
> Michael Ellerman <mpe@ellerman.id.au> writes:
>
>>> +	ctx->elem->software_state = cpu_to_be32(CXL_PE_SOFTWARE_STATE_V);
>>> +	/* Make sure the changes to the PE are visible to the card */
>>
>> A barrier orders something vs something else. So what's the something
>> else in this case? Is it the afu_reset() below, what does that actually do?
>>
>
> The issue is with call to afu_enable() after the call to afu_reset that
> would start the AFU. If this load gets reordered and PSL doesnt see the
> valid bit set for this structure then it will result in PSL entering a
> freeze-state.

OK, so it's ordering the store above to ctx->elem->software_state vs the
store to the AFU in afu_enable().

> Though on second thoughts afu_enable() is grabbing a spin-lock before
> doing an mmio to start the AFU that would be forcing a barrier
> anyways. But since that spans the function boundary hence to be safe
> have added a write barrier after populating the process element.

The spin lock doesn't help you, stores are allowed to leak into the
locked region.

But the MMIO is preceeded by a sync.

> Lastly function is not performance critical as it will be usually called
> in the life time of a process only once. So the impact smp_wmb() is
> having would be minimal.

Sure. Performance is not the issue, barriers are subtle so it's
important that they're well documented.


So I don't think you need the barrier, because the out_be64() will do it
for you. But if you really want to add one, I don't mind.

But, you should use wmb(), not smp_wmb(), because the ordering is still
required on non-SMP systems. And please update the comment to capture
all of the above discussion.

cheers