Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <CGME20221103195201eucas1p2a6ec2df41ebac3d9ccbb0b252c2cad34@eucas1p2.samsung.com>
 <20221103195154.21495-1-semen.protsenko@linaro.org> <a7d9cd18-a328-209c-c89f-afdcb7db3eb0@samsung.com>
 <b7ad6444-e7d2-1150-6134-3dae8129dcdb@samsung.com>
In-Reply-To: <b7ad6444-e7d2-1150-6134-3dae8129dcdb@samsung.com>
From:   Sam Protsenko <semen.protsenko@linaro.org>
Date:   Fri, 11 Nov 2022 14:29:49 +0100
Message-ID: <CAPLW+4=Y6qZG2XjJR_BkX-ar4GWdETKO1tteJjfbxVc664e4Kg@mail.gmail.com>
Subject: Re: [PATCH v2 0/6] iommu/exynos: Convert to a module
To:     Marek Szyprowski <m.szyprowski@samsung.com>
Cc:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>,
        Robin Murphy <robin.murphy@arm.com>,
        Sumit Semwal <sumit.semwal@linaro.org>,
        Alim Akhtar <alim.akhtar@samsung.com>,
        Janghyuck Kim <janghyuck.kim@samsung.com>,
        Cho KyongHo <pullip.cho@samsung.com>,
        Daniel Mentz <danielmentz@google.com>,
        David Virag <virag.david003@gmail.com>, iommu@lists.linux.dev,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-samsung-soc@vger.kernel.org,
        Saravana Kannan <saravanak@google.com>,
        Rob Herring <robh@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, 10 Nov 2022 at 15:36, Marek Szyprowski <m.szyprowski@samsung.com> wrote:

[snip]

> I've finally made Exynos IOMMU working as a module on Exynos5433 based
> TM2e board. It looks that this will be a bit longer journey that I've
> initially thought. I've posted a simple update of the fix for the driver
> initialization sequence, but the real problem is in the platform driver
> framework and OF helpers.
>
> Basically to get it working as a module I had to apply the following
> changes:
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 3dda62503102..f6921f5fcab6 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -257,7 +257,7 @@ static int deferred_devs_show(struct seq_file *s,
> void *data)
>   DEFINE_SHOW_ATTRIBUTE(deferred_devs);
>
>   #ifdef CONFIG_MODULES
> -int driver_deferred_probe_timeout = 10;
> +int driver_deferred_probe_timeout = 30;
>   #else
>   int driver_deferred_probe_timeout;
>   #endif
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 967f79b59016..e5df6672fee6 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1384,7 +1384,7 @@ static struct device_node *parse_interrupts(struct
> device_node *np,
>   static const struct supplier_bindings of_supplier_bindings[] = {
>          { .parse_prop = parse_clocks, },
>          { .parse_prop = parse_interconnects, },
> -       { .parse_prop = parse_iommus, .optional = true, },
> +       { .parse_prop = parse_iommus, },
>          { .parse_prop = parse_iommu_maps, .optional = true, },
>          { .parse_prop = parse_mboxes, },
>          { .parse_prop = parse_io_channels, },
>
> Without that a really nasty things happened.
>
> Initialization of the built-in drivers and loading modules takes time,
> so the default 10s deferred probe timeout is not enough to ensure that
> the built-in driver won't be probed before the Exynos IOMMU driver is
> loaded.
>

Yeah, the whole time-based sync looks nasty... I remember coming
across the slides by Andrzej Hajda called "Deferred Problem" [1], but
I guess the proposed solution was never applied. Just hope that
increasing the timeout is upstreamable solution.

[1] https://events19.linuxfoundation.org/wp-content/uploads/2017/12/Deferred-Problem-Issues-With-Complex-Dependencies-Between-Devices-in-Linux-Kernel-Andrzej-Hajda-Samsung.pdf

> The second change fixes the problem that driver core probes Exynos IOMMU
> controllers in parallel to probing the master devices, what results in
> calling exynos_iommu_of_xlate() and exynos_iommu_probe_device() even on
> the partially initialized IOMMU controllers or initializing the dma_ops
> under the already probed and working master device. This was easy to
> observe especially on the master devices with multiple IOMMU
> controllers. I wasn't able to solve this concurrency/race issues inside
> the Exynos IOMMU driver.
>
> Frankly speaking I don't know what is the rationale for making the
> 'iommus' property optional, but this simply doesn't work well with IOMMU
> driver being a module. CCed Saravana and Rob for this.
>

The patch which makes 'iommus' optional doesn't provide much of
insight on reasons in commit message either.

> Without fixing the above issues, I would add a warning that compiling
> the driver as a module leads to serious issues.
>

Nice catch! It doesn't reproduce on my platform, alas. Can I expect
you to submit those patches? If so, I'll probably just wait for those
to be applied, and then re-send my modularization series on top of it.
Does that sounds reasonable?

[snip]

>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>