Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
MIME-Version: 1.0
References: <20220426201539.12829-1-sven@svenpeter.dev> <20220426201539.12829-5-sven@svenpeter.dev>
In-Reply-To: <20220426201539.12829-5-sven@svenpeter.dev>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Tue, 26 Apr 2022 22:55:37 +0200
Message-ID: <CAK8P3a39t-yyhtVKiEvD9_FvLiVxySnJRXaOv4E4q77t6mDesw@mail.gmail.com>
Subject: Re: [PATCH v3 4/6] soc: apple: Add SART driver
To:     Sven Peter <sven@svenpeter.dev>
Cc:     Arnd Bergmann <arnd@arndb.de>, Hector Martin <marcan@marcan.st>,
        Alyssa Rosenzweig <alyssa@rosenzweig.io>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzk+dt@kernel.org>,
        Keith Busch <kbusch@kernel.org>, Jens Axboe <axboe@fb.com>,
        Christoph Hellwig <hch@lst.de>,
        Sagi Grimberg <sagi@grimberg.me>,
        Marc Zyngier <maz@kernel.org>, Janne Grunau <j@jannau.net>,
        DTML <devicetree@vger.kernel.org>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-nvme@lists.infradead.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Apr 26, 2022 at 10:15 PM Sven Peter <sven@svenpeter.dev> wrote:
>
> The NVMe co-processor on the Apple M1 uses a DMA address filter called
> SART for some DMA transactions. This adds a simple driver used to
> configure the memory regions from which DMA transactions are allowed.
>
> Unlike a real IOMMU, SART does not support any pagetables and can't be
> implemented inside the IOMMU subsystem using iommu_ops.
>
> It also can't be implemented using dma_map_ops since not all DMA
> transactions of the NVMe controller are filtered by SART.
> Instead, most buffers have to be registered using the integrated NVMe
> IOMMU and we can't have two separate dma_map_ops implementations for a
> single device.
>
> Co-developed-by: Hector Martin <marcan@marcan.st>
> Signed-off-by: Hector Martin <marcan@marcan.st>
> Signed-off-by: Sven Peter <sven@svenpeter.dev>

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

One more detail I noticed:

> +#if IS_ENABLED(CONFIG_APPLE_SART)
> +
> +struct apple_sart;
> +
> +/*
> + * Get a reference to the SART attached to dev.
> + *
> + * Looks for the phandle reference in apple,sart and returns a pointer
> + * to the corresponding apple_sart struct to be used with
> + * apple_sart_add_allowed_region and apple_sart_remove_allowed_region.
> + */
> +struct apple_sart *devm_apple_sart_get(struct device *dev);
> +

In general, I don't like to hide declarations in a header behind an #if. Unless
there is a good reason for doing this, just make the declaration unconditional,
which avoids recompiling when the symbol changes.

The only other effect is that you get a link-time error instead of a
compile-time
error if you messed up the Kconfig dependencies, but as long as the dependencies
are correct it will be fine either way.

      Arnd