Hi Andy, I took a quick look at arch_setup_pdev_archdata(), overridden it in dcdbas.c and it works well (despite it's being called twice, since it's called from platform_device_alloc and platform_device_register). However, as I am not super familiar with ELF weak method references, especially with its scope resolution / versioning part, so as I see this weak method was introduced mainly for arch/** specific hooks. Is it safe to override this method from driver code, when let's say there's another implementation in the x86 arch code (currently there isn't)? So while I agree that archdata should be ideally written from within this hook to make sure the value is there when the bus notifiers call the iommu code (currently not registered for platform_bus), I am just a bit worried that this might mask a future generic arch implementation and could cause issues. What do you think? Best Regards, Szabolcs On Thu, Jan 24, 2019 at 1:44 PM Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote: > > On Thu, Jan 24, 2019 at 10:31 PM Szabolcs Fruhwald <sfruhwald@xxxxxxxxxx> wrote: > > > > (+iommu list for visibility and confirmation of the intended constant > > externalization, see 2nd point below) > > > Absolutely, I thought so too. But, since the actual need to force > > id-mapping comes from the lack of support for non-pci buses > > particularly by the intel iommu implementation, it seemed a bit odd to > > move this constant into iommu.h. Other platforms' implementations are > > usually handling and hooking into other buses, eg platform bus. > > > > However, even these other hw platform iommu implementations are using > > ACPI or other (bios related) tables to generate source ids, which > > might still be an issue with drivers like dcdbas, with no real device > > info in these tables. Not to mention that forcing id-mapping might be > > a useful tool in driver developers' hands by other reasons too. > > Therefore, I just came to the conclusion that it is indeed useful to > > have this constant present in the common iommu header file (but with a > > more expressing name). > > > > Especially, as I just found that dcdbas is *not* the first one to > > implement this workaround: > > https://github.com/torvalds/linux/blob/71f3a82fab1b631ae9cb1feb677f498d4ca5007d/drivers/gpu/drm/i915/selftests/mock_gem_device.c#L154 > > > Let me come up with a v2 patch-set, containing a separate patch > > externalizing this constant from intel_iommu.c to iommu.h and making > > the above code using it too first, followed by this change in dcdbas. > > Wait... It sounds to me like a part of arch code where we define > arch_setup_pdev_archdata() and use this dummy domain. > Though dummy domain definition should come from IOMMU framework. > > -- > With Best Regards, > Andy Shevchenko
