Re: [PATCH v2 0/6] iommu/exynos: Convert to a module

From: Sam Protsenko <semen.protsenko@linaro.org>
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
Subject: Re: [PATCH v2 0/6] iommu/exynos: Convert to a module
Date: Fri, 4 Nov 2022 14:22:57 +0100	[thread overview]
Message-ID: <CAPLW+4nrehDSNbjNzh5jWVWH3dMoN+YmXPQfpaTrqB1jjzHT-w@mail.gmail.com> (raw)
In-Reply-To: <a7d9cd18-a328-209c-c89f-afdcb7db3eb0@samsung.com>

On Fri, 4 Nov 2022 at 13:10, Marek Szyprowski <m.szyprowski@samsung.com> 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/
>

I wonder why I didn't stumble upon that issue. Anyway, the fix looks
valid. I'll wait until you finish looking into that NULL pointer
dereference, and then we can figure out how to organize our patches.
Maybe I could send v3, including your patches in the beginning of the
series, etc.

> 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.
>

Thanks for looking into that, Marek! I've tested the modularization
patch series using my emulated translation driver [1,2]. It works
fine, but exynos_iommu_of_xlate() is only executed once in my case,
when the test driver module is loaded. It's finished successfully
though. Not sure what is the cause of NULL pointer dereferences you're
seeing, alas I still don't have real users of SysMMU driver on my
platform, and my test driver is the only way for me to test it. Please
let me know if I can help in any way (e.g. I can trace some calls on
my board for you).

[1] https://github.com/joe-skb7/linux/blob/e850-96-mainline-iommu/drivers/iommu/samsung-iommu-test.c#L263
[2] https://github.com/joe-skb7/linux/blob/e850-96-mainline-iommu/arch/arm64/boot/dts/exynos/exynos850.dtsi#L477

> > 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
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>