Messages in this thread | | | From | Arnd Bergmann <> | Date | Tue, 22 Mar 2022 14:38:39 +0100 | Subject | Re: [PATCH 6/9] nvme-apple: Add initial Apple SoC NVMe driver |
| |
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?
> +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.
> +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?
> + 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.
> +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.
Arnd
| |