Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 6 Sep 2018 10:13:37 +0200
From:   Lukas Wunner <lukas@wunner.de>
To:     Mika Westerberg <mika.westerberg@linux.intel.com>
Cc:     linux-kernel@vger.kernel.org,
        Andreas Noever <andreas.noever@gmail.com>,
        Michael Jamet <michael.jamet@intel.com>,
        Yehezkel Bernat <YehezkelShB@gmail.com>,
        Lu Baolu <baolu.lu@linux.intel.com>
Subject: Re: [PATCH 2/2] thunderbolt: Initialize after IOMMUs
Message-ID: <20180906081337.3mosfpfggfmoj633@wunner.de>
References: <20180903132012.23531-1-mika.westerberg@linux.intel.com>
 <20180903132012.23531-2-mika.westerberg@linux.intel.com>
 <20180905084746.xmckzskxzsdliz3e@wunner.de>
 <20180905094602.GH2283@lahna.fi.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180905094602.GH2283@lahna.fi.intel.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Sep 05, 2018 at 12:46:02PM +0300, Mika Westerberg wrote:
> On Wed, Sep 05, 2018 at 10:47:46AM +0200, Lukas Wunner wrote:
> > On Mon, Sep 03, 2018 at 04:20:12PM +0300, Mika Westerberg wrote:
> > > If IOMMU is enabled and Thunderbolt driver is built into the kernel
> > > image, it will be probed before IOMMUs are attached to the PCI bus.
> > > Because of this DMA mappings the driver does will not go through IOMMU
> > > and start failing right after IOMMUs are enabled.
> > > 
> > > For this reason move the Thunderbolt driver initialization happen at
> > > rootfs level.
> > > 
> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > ---
> > >  drivers/thunderbolt/nhi.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> > > index 88cff05a1808..5cd6bdfa068f 100644
> > > --- a/drivers/thunderbolt/nhi.c
> > > +++ b/drivers/thunderbolt/nhi.c
> > > @@ -1191,5 +1191,5 @@ static void __exit nhi_unload(void)
> > >  	tb_domain_exit();
> > >  }
> > >  
> > > -fs_initcall(nhi_init);
> > > +rootfs_initcall(nhi_init);
> > >  module_exit(nhi_unload);
> > 
> > I think the dependency on the IOMMU should be open coded by returning
> > -EPROBE_DEFER from the ->probe hook if it's not yet attached.
> > Shuffling around initcall order is just applying duct tape.
> 
> It is not a dependency. The same thing can happen with any other driver
> if they happen to initialize any DMA with the device before IOMMUs are
> initialized.
> 
> > Commit acb40d841257 already changed module_init() to fs_initcall()
> > and now it has to be changed again.  Shows how fragile this is.
> 
> It is a bit fragile but I don't see any other way to handle this than
> trusting on the link ordering. Both -EPROBE_DEFER and device_links are
> out of the question AFAICT.

So with this patch, you rely on the linker ordering nhi_init() after
ir_dev_scope_init(), however to the best of my knowledge the link
order is not guaranteed.

In that sense, commit acb40d841257 was already flawed because it
executed nhi_init() at "fs" initcall level, the *same* level used by
map_properties() in drivers/firmware/efi/apple-properties.c, which
retrieves the DROM device property needed by tb_drom_copy_efi().

That was arguably a regression which the above patch cures because
"rootfs" is guaranteed to run after "fs".  Still, the fragility
remains that ir_dev_scope_init() isn't guaranteed to run before
nhi_init().

Looking at commit acb40d841257, which started this, I'm wondering
why you did not simply export tbnet_init() and call it from the
thunderbolt driver after the property stuff has been fully set up?
After all, thunderbolt-net is useless without thunderbolt or am I
missing something?  Then you could revert back to module_init().

Thanks,

Lukas