Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell

[Rob Herring]

On Thu, Aug 27, 2015 at 7:28 AM,  <ygardi@xxxxxxxxxxxxxx> wrote:
>>> On Aug 21, 2015 3:10 PM, "Yaniv Gardi" <ygardi@xxxxxxxxxxxxxx> wrote:
>>>>
>>>> Add a write memory barrier to make sure descriptors prepared are
>>>> actually
>>>> written to memory before ringing the doorbell. We have also added the
>>>> write memory barrier after ringing the doorbell register so that
>>>> controller sees the new request immediately.

[...]

>>>> +       /* Make sure descriptors are ready before ringing the doorbell
>>>> */
>>>> +       wmb();
>>>
>>> The writel for the doorbell will do a barrier first. (I didn't check
>>> what exactly ufshcd_writel does, but that is why I don't like these
>>> private access wrappers.)
>
> the barrier here is important and a must.
> before it we prepare descriptors.
> after it we write to DOOR-BELL.
> (and after the DOOR-BELL we have another one.)
> if we remove it, we might get the DOOR-BELL written, before the
> descriptors written.

If you dig into what writel does, you will see that what the code here
ends up being is:

descriptor setup
wmb()
__iomwb() -> wmb on arm64
doorbell register write

So explain why you need 2 barriers.

Barriers in a driver are a red flag. Usually they are not needed, but
sometimes they are. You have to be able to explain why if they are.
Pretty much every descriptor based DMA device works as you describe
and has the ordering problem. Because of that, the core code takes
care of this.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/