Re: [PATCH 2/2] thunderbolt: Initialize after IOMMUs

[Mika Westerberg]

On Thu, Sep 06, 2018 at 10:13:37AM +0200, Lukas Wunner wrote:
> 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@xxxxxxxxxxxxxxx>
> > > > ---
> > > >  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.

What says that?

As far as I can tell it has been used with initcalls forever to make
sure certain block of code gets executed at certain time.

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

The same reason you don't call PCI driver functions from PCI core. It
makes absolutely zero sense.

Thunderbolt is bus and provides driver API to drivers. We hopefully are
getting other service drivers (say SCSI over TBT) that are going to be
use the same interfaces.