From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752186AbcDSCF1 (ORCPT ); Mon, 18 Apr 2016 22:05:27 -0400 Received: from m199-177.yeah.net ([123.58.177.199]:6656 "EHLO m199-177.yeah.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751650AbcDSCF0 (ORCPT ); Mon, 18 Apr 2016 22:05:26 -0400 Subject: Re: [RFT v2] iommu/amd: use subsys_initcall() on amdv2 iommu To: "Luis R. Rodriguez" , Oded Gabbay References: <20160316171719.GE2195@8bytes.org> <1459273313-5139-1-git-send-email-mcgrof@kernel.org> <570BA694.8040900@amd.com> <570BAC2B.4090508@amd.com> <20160412220715.GL1990@wotan.suse.de> <20160418120350.GE1990@wotan.suse.de> Cc: =?UTF-8?Q?Christian_K=c3=b6nig?= , iommu@lists.linux-foundation.org, "Linux-Kernel@Vger. Kernel. Org" , Wan Zongshun From: Wan Zongshun Message-ID: <571591CC.40700@iommu.org> Date: Tue, 19 Apr 2016 10:02:52 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.7.2 MIME-Version: 1.0 In-Reply-To: <20160418120350.GE1990@wotan.suse.de> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-HM-Spam-Status: e1koWUFPN1dZCBgUCR5ZQVZIVU1JS0tLS0pJSEJMTE1NSldZCQ4XHghZQV koKz0kKzooKCQyNSQzPjo*PilBS1VLQDYjJCI#KCQyNSQzPjo*PilBS1VLQCsvKSQiPigkMjUkMz 46Pz4pQUtVS0A4NC41LykiJDg1QUtVS0ApPjwyNDUkOigyOkFLVUtAKyk0LTI1OD4kKDM6NTwzOj JBS1VLQD8iNTo2MjgkMiskNTQkMjUkMz46Pz4pQUtVS0A2LjcvMiQpOCsvJD8yPT0#KT41LyQyNS QzPjo*PilBSVVLQDIrJEokNjI1Li8#JDg1LyRLJEpLQUtVS0AyKyRISyQ2MjUuLz4kODUvJEskTk tBS1VLQDIrJC80PzoiJDg1LyRLJEpLS0FLVUtAMiskTiQ2MjUuLz4kODUvJEskSktBS1VLQDIrJE okMzQuKSQ4NS8kSyRKS0tBS1VLQCguOTE#OC8kTiQ2MjUuLz4kODUvJEskSktBS1VLQCguOTE#OC 8kLzQ*OiIkODUvJEskSktLQUtVS0AoLjkxPjgvJEokMzQuKSQ4NS8kSyRKS0tBS1VLQD01JC4pNy QzLy8rKCQzNzEkS0NLSktDQUtVS0A9NSQ2OiIkT0pCJDM3MSRKJEtDS0hLT0FLVUhIQD0rJCk#JD 0sJDM3MSRLQ0tIS01BVkxVTkAoLjkkPkFKVU5OQD01JDY6IiRPSkIkMzcxJEkkS0NLSEtPQUtVS1 kG X-HM-Sender-Digest: e1kSHx4VD1lBWUc6OTo6Cio*SzoyK0sRLg9KFREsMigaFAhVSlVKT01K S0hKTktDT0tLVTMWGhIXVQ0MOxIUFhYOVRQJHEVZV1kMHhlZQR0aFwgeV1kIAVlBQ0pNSDdXWRIL WUFZSUpLVUpIVUJMVUpNQ1kG X-HM-Tid: 0a542c43972464276a41e11061ef Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org -------- 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 wrote: >>> >>> On Apr 18, 2016 7:48 AM, "Oded Gabbay" wrote: >>>> >>>> On Wed, Apr 13, 2016 at 1:07 AM, Luis R. Rodriguez >>>> 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 >>>>>>> wrote: >>>>>>>> Am 09.04.2016 um 02:25 schrieb Luis R. Rodriguez: >>>>>>>>> On Tue, Mar 29, 2016 at 10:41 AM, Luis R. Rodriguez >>>>>>>>> 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 >>>>>>>>>> Cc: Christian König >>>>>>>>>> Signed-off-by: Luis R. Rodriguez >>>>>>>> >>>>>>>> Sorry for not responding earlier. Just coming back to all the stuff >>>>>>>> on my TODO list. >>>>>>>> >>>>>>>> Patch is Acked-by: Christian König >>>>>>> >>>>>>> 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 >