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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAK8P3a27-eWP=krGQOp29krcc7bVME9=MbN6B3Rs7Q4Ran_VFw@mail.gmail.com>
Date:   Tue, 22 Mar 2022 14:38:39 +0100
From:   Arnd Bergmann <arnd@...db.de>
To:     Sven Peter <sven@...npeter.dev>
Cc:     Keith Busch <kbusch@...nel.org>, Jens Axboe <axboe@...com>,
        Christoph Hellwig <hch@....de>,
        Sagi Grimberg <sagi@...mberg.me>,
        Hector Martin <marcan@...can.st>,
        Alyssa Rosenzweig <alyssa@...enzweig.io>,
        Rob Herring <robh+dt@...nel.org>,
        Arnd Bergmann <arnd@...db.de>, Marc Zyngier <maz@...nel.org>,
        DTML <devicetree@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-nvme@...ts.infradead.org
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@...npeter.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