Re: [PATCH 6/9] nvme-apple: Add initial Apple SoC NVMe driver

["Sven Peter" <sven@svenpeter.dev>]

On Tue, Mar 22, 2022, at 14:38, Arnd Bergmann wrote:
> On Mon, Mar 21, 2022 at 5:50 PM Sven Peter <sven@svenpeter.dev> wrote:
>
>> +static int apple_nvme_sart_dma_setup(void *cookie, struct apple_rtkit_shmem *bfr,
>> +                                    dma_addr_t iova, size_t size)
>> +{
>> +       struct apple_nvme *anv = cookie;
>> +       int ret;
>> +
>> +       if (iova)
>> +               return -EINVAL;
>> +
>> +       bfr->buffer = dma_alloc_coherent(anv->dev, size, &iova, GFP_KERNEL);
>> +       if (!bfr->buffer)
>> +               return -ENOMEM;
>
> You pass 'iova' as an argument, but then replace it with the address
> returned by dma_alloc_coherent(). Can you remove the function
> argument?

Yup, will remove it.

>
>> +static void apple_nvmmu_inval(struct apple_nvme_queue *q, unsigned int tag)
>> +{
>> +       struct apple_nvme *anv = queue_to_apple_nvme(q);
>> +
>> +       writel(tag, anv->mmio_nvme + APPLE_NVMMU_TCB_INVAL);
>> +       if (readl_relaxed(anv->mmio_nvme + APPLE_NVMMU_TCB_STAT))
>> +               dev_warn(anv->dev, "NVMMU TCB invalidation failed\n");
>> +}
>
> I don't like to see the _relaxed() accessors used without an explanation
> about why that helps. Please use the non-relaxed version, or make sure
> it's obvious here why you use it.

Ok, I'll either use the non-relaxed ones or add a comment whenever I use
the relaxed version. In this case here there's no write to any DMA buffers
that needs to be visible to the device. That writel there could actually
be a writel_relaxed as well. There just used to be a write to a buffer
above (which is another good reason to always comment when using the
non-relaxed ones, maybe then I would've noticed then and updated it).


>
>> +bad_sgl:
>> +       WARN(DO_ONCE(apple_nvme_print_sgl, iod->sg, iod->nents),
>> +            "Invalid SGL for payload:%d nents:%d\n", blk_rq_payload_bytes(req),
>> +            iod->nents);
>
> I think you mean WARN_ONCE() here?

This is taken from pci.c which used to use WARN_ONCE but was replaced in
d08774738446e77734777adcf5d1045237b4475a with this construction here.
The commit message mentions

    The WARN_ONCE macro returns true if the condition is true, not if the
    warn was raised, so we're printing the scatter list every time it's
    invalid. This is excessive and makes debugging harder, so this patch
    prints it just once.


>
>> +       writel_relaxed(0, anv->mmio_coproc + APPLE_ANS_COPROC_CPU_CONTROL);
>> +       (void)readl_relaxed(anv->mmio_coproc + APPLE_ANS_COPROC_CPU_CONTROL);
>
> What is the purpose of the readl_relaxed() here? It looks like you are
> trying to flush
> the write to the hardware, but then again
>
>   a) on Apple hardware, the registers are mapped using PROT_DEVICE_nGnRnE,
>       so MMIO writes are never posted
>
>   b) the read is "_relaxed", so there is no barrier, and the result is
> unused, so
>       it would appear that the CPU can just keep executing code anyway.
>
> Since this is all the initialization path, I can't imagine what the
> relaxation of
> the barriers helps with.

Agreed, I've actually tried replacing all non-relaxed ones with the normal
accessors (even those inside the hot path) and didn't see any performance
difference.
I can use the normal ones here and I'll consider using the non-relaxed ones
in the hot path together with a comment why they are safe in those places.

>
>> +static int apple_nvme_reg_read32(struct nvme_ctrl *ctrl, u32 off, u32 *val)
>> +{
>> +       *val = readl_relaxed(ctrl_to_apple_nvme(ctrl)->mmio_nvme + off);
>> +       return 0;
>> +}
>> +
>> +static int apple_nvme_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val)
>> +{
>> +       writel_relaxed(val, ctrl_to_apple_nvme(ctrl)->mmio_nvme + off);
>> +       return 0;
>> +}
>
> If you have generic register access functions, don't make them use
> _relaxed internally. If there are instances that need to be _relaxed,
> add another version of the accessor that spells this out in the caller.

Ok, there are used internally by the nvme core and can't do any kind of DMA
right now but I agree that it's better to use the non-relaxed ones here
to prevent surprises in the future.


Sven