Messages in this thread | | | Subject | Re: [RFT v2] iommu/amd: use subsys_initcall() on amdv2 iommu | From | Wan Zongshun <> | Date | Tue, 19 Apr 2016 10:02:52 +0800 |
| |
-------- Original Message -------- > On Mon, Apr 18, 2016 at 10:02:24AM +0300, Oded Gabbay wrote: >> On Mon, Apr 18, 2016 at 9:55 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: >>> >>> On Apr 18, 2016 7:48 AM, "Oded Gabbay" <oded.gabbay@gmail.com> wrote: >>>> >>>> On Wed, Apr 13, 2016 at 1:07 AM, Luis R. Rodriguez <mcgrof@kernel.org> >>>> wrote: >>>>> On Mon, Apr 11, 2016 at 03:52:43PM +0200, Christian König wrote: >>>>>> Am 11.04.2016 um 15:39 schrieb Oded Gabbay: >>>>>>> On Mon, Apr 11, 2016 at 4:28 PM, Christian König >>>>>>> <christian.koenig@amd.com> wrote: >>>>>>>> Am 09.04.2016 um 02:25 schrieb Luis R. Rodriguez: >>>>>>>>> On Tue, Mar 29, 2016 at 10:41 AM, Luis R. Rodriguez >>>>>>>>> <mcgrof@kernel.org> wrote: >>>>>>>>>> We need to ensure amd iommu v2 initializes before >>>>>>>>>> driver uses such as drivers/gpu/drm/amd/amdkfd/kfd_module.c, >>>>>>>>>> to do this make its init routine a subsys_initcall() which >>>>>>>>>> ensures its load init is called first than modules when >>>>>>>>>> built-in. >>>>>>>>>> >>>>>>>>>> This reverts the old work around implemented through commit >>>>>>>>>> 1bacc894c227fad8a7 ("drivers: Move iommu/ before gpu/ in >>>>>>>>>> Makefile"), >>>>>>>>>> instead of making the dependency implicit by linker order this >>>>>>>>>> makes the ordering requirement explicit through proper kernel >>>>>>>>>> APIs. >>>>>>>>>> >>>>>>>>>> Cc: Oded Gabbay <oded.gabbay@amd.com> >>>>>>>>>> Cc: Christian König <christian.koenig@amd.com> >>>>>>>>>> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> >>>>>>>> >>>>>>>> Sorry for not responding earlier. Just coming back to all the stuff >>>>>>>> on my TODO list. >>>>>>>> >>>>>>>> Patch is Acked-by: Christian König <christian.koenig@amd.com> >>>>>>> >>>>>>> Christian, >>>>>>> Just wanted to be sure if you tested this patch-set or not. >>>>>> >>>>>> I did NOT tested it. If AMD IOMMU requires something which will now >>>>>> initialize after the IOMMU module we will obviously run into trouble >>>>>> again. >>>>>> >>>>>> I assumed that the creator of the patch did some testing. >>>>> >>>>> Nope, hence [RTF] Request For Testing. >>>>> >>>>>>> I don't think it should be merged without testing. If you already >>>>>>> tested it than fine. If not, I think I can do it in the next week or >>>>>>> so (just came back from PTO). >>>>>> >>>>>> Yeah, agree totally. >>>>> >>>>> Agreed, please let me know if someone is able to test and confirm >>>>> this works. It should work. >>>>> >>>>> Luis >>>> >>>> Hi, >>>> So I finally got to test this patch and it's not working. >>>> The reason is that AMD IOMMUv2 gets initialized *before* AMD IOMMUv1 >>>> driver ! >>> >>> Thanks can you try using late_initcall() instead then? >>> >>> Luis >> >> That will make it initialize *after* drm subsystem, which will cause >> another bug. > > Hold up, I thought that we needed AMD IOMMUv2 to get initialized > before AMD IOMMUv1 ? That's what the patch did. Can someone clarify > the requirements then?
We must keep AMD IOMMUv2 to get initialized after AMD IOMMUv1, So your patch make the sequence reverse.
> > I'll provide some review of the current state of affairs first, without the > patch. AMD IOMMUv1 uses x86_init.iommu.iommu_init and that has its own init > semantics. Specifically that gets called via pci_iommu_init() which is pegged > on the init order via rootfs_initcall(pci_iommu_init); > > Then AMD IOMMUv2 uses module_init() and that when is built-in falls on to > __initcall() which is device_initcall().
Exactly, it is amd iommu v1 v2 call sequence. > > The order is: > > #define pure_initcall(fn) __define_initcall(fn, 0) > > #define core_initcall(fn) __define_initcall(fn, 1) > #define core_initcall_sync(fn) __define_initcall(fn, 1s) > #define postcore_initcall(fn) __define_initcall(fn, 2) > #define postcore_initcall_sync(fn) __define_initcall(fn, 2s) > #define arch_initcall(fn) __define_initcall(fn, 3) > #define arch_initcall_sync(fn) __define_initcall(fn, 3s) > #define subsys_initcall(fn) __define_initcall(fn, 4) > #define subsys_initcall_sync(fn) __define_initcall(fn, 4s) > #define fs_initcall(fn) __define_initcall(fn, 5) > #define fs_initcall_sync(fn) __define_initcall(fn, 5s) > #define rootfs_initcall(fn) __define_initcall(fn, rootfs) > #define device_initcall(fn) __define_initcall(fn, 6) > #define device_initcall_sync(fn) __define_initcall(fn, 6s) > #define late_initcall(fn) __define_initcall(fn, 7) > #define late_initcall_sync(fn) __define_initcall(fn, 7s) > > So technically rootfs_initcall() (v1 amd) should be being called > first already, and after that AMD IOMMUv2 gets called next. > > You said that with my patch you saw AMD IOMMUv2 kick off first, > that was intentional as I thought that's what you needed. Can > someone please describe the requirements? > > Also what does drm use that you say has a conflict already? What > drm code are we talking about exactly ?
You have to take carefully to arrange the calling sequence for iommuv1, iommuv2, kfd module, and drm like the following sequence : v1 ->v2->kfd, drm.
iommuv1 -- rootfs_initcall(fn) IOMMUV2 -- device_initcall(fn) kfd module -- late_initcall(fn) drm -- late_initcall(fn)
Thanks! Wan Zongshun.
> > Luis > _______________________________________________ > iommu mailing list > iommu@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu >
| |