Message-ID: <b7ad6444-e7d2-1150-6134-3dae8129dcdb@samsung.com>
Date:   Thu, 10 Nov 2022 15:36:39 +0100
From:   Marek Szyprowski <m.szyprowski@...sung.com>
To:     Sam Protsenko <semen.protsenko@...aro.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc:     Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>,
        Robin Murphy <robin.murphy@....com>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Alim Akhtar <alim.akhtar@...sung.com>,
        Janghyuck Kim <janghyuck.kim@...sung.com>,
        Cho KyongHo <pullip.cho@...sung.com>,
        Daniel Mentz <danielmentz@...gle.com>,
        David Virag <virag.david003@...il.com>, iommu@...ts.linux.dev,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-samsung-soc@...r.kernel.org,
        Saravana Kannan <saravanak@...gle.com>,
        Rob Herring <robh@...nel.org>
Subject: Re: [PATCH v2 0/6] iommu/exynos: Convert to a module

On 04.11.2022 13:10, Marek Szyprowski wrote:
> On 03.11.2022 20:51, Sam Protsenko wrote:
>> As exynos-iommu driver is not a critical platform driver, it can be
>> converted to a loadable module to avoid loading it on non-Exynos
>> platforms in order to improve the RAM footprint. This patch series
>> converts it to a module and does some related cleanups. IOMMU/DMA
>> specifics were taken into the account, so remove/exit methods weren't
>> added.
>>
>> There are two drivers using CONFIG_EXYNOS_IOMMU in their code:
>> DRM_EXYNOS and S5P_MFC. Both were checked, and only a slight change was
>> needed for S5P_MFC driver (patch #6).
>
> Funny, compiling this driver as a module revealed an issue in the 
> driver initialization sequence, here is a fix that need to be applied 
> before this patchset:
>
> https://lore.kernel.org/all/20221104115511.28256-1-m.szyprowski@samsung.com/ 
>
>
> Besides that, the driver nukes with NULL pointer dereference in 
> exynos_iommu_of_xlate() when compiled as a module on ARM 64bit 
> Exynos5433 based TM2e board. ARM 32bit based board works fine. I'm 
> checking this issue now.
>
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.

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.

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


>> Changes in v2:
>>    - Extracted the "shutdown" method addition into a separate patch
>>    - Added MODULE_DEVICE_TABLE(of, ...) to support hot-plug loading
>>    - Added MODULE_ALIAS("platform:exynos-sysmmu")
>>    - Added fix for S5P_MFC driver to work correctly with EXYNOS_IOMMU=m
>>    - Fixed checkpatch coding style suggestion with "--strict" flag
>>    - Rebased on top of most recent joro/iommu.git:next
>>
>> Sam Protsenko (6):
>>    iommu: Export iommu_group_default_domain() API
>>    iommu/exynos: Fix retval on getting clocks in probe
>>    iommu/exynos: Modularize the driver
>>    iommu/exynos: Implement shutdown driver method
>>    iommu/exynos: Rearrange the platform driver code
>>    media: platform: Use IS_ENABLED() to check EXYNOS_IOMMU in s5p_mfc
>>
>>   drivers/iommu/Kconfig                         |   2 +-
>>   drivers/iommu/exynos-iommu.c                  | 355 +++++++++---------
>>   drivers/iommu/iommu.c                         |   1 +
>>   .../platform/samsung/s5p-mfc/s5p_mfc_iommu.h  |   4 +-
>>   4 files changed, 191 insertions(+), 171 deletions(-)
>>
> Best regards

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

Powered by blists - more mailing lists