Hi,
On Thu, Feb 18, 2016 at 10:39:05PM +0100, Daniel Vetter wrote:
On Thu, Feb 18, 2016 at 9:34 PM, Lukas Wunner lukas@wunner.de wrote:
Ok, makes sense. I still think adding the check to the client_register function would be good, just as a safety measure.
Hm, the idea of calling vga_switcheroo_client_probe_defer() twice causes me pain in the stomach. It's surprising for drivers which just don't need it at the moment (amdgpu and snd_hda_intel) and it feels like overengineering and pampering driver developers beyond reasonable measure. Also while the single existing check is cheap, we might later on add checks that take more time and slow things down.
It's motivated by Rusty's API Manifesto:
http://sweng.the-davies.net/Home/rustys-api-design-manifesto
Interesting, thank you.
With the mandatory check in _register we reach level 5 - it'll blow up at runtime when we try to register it.
The manifesto says "5. Do it right or it will always break at runtime".
However even if we add a WARN_ON(vga_switcheroo_client_probe_defer(pdev)) to register_client(), it will not *always* spew a stacktrace but only on the machines this concerns (MacBook Pros). Since failure to defer breaks GPU switching, level 5 is already reached. Chances are this won't go unnoticed by the user.
For more context: We have tons of fun with EPROBE_DEFER handling between i915 and snd-hda
I don't understand, there is currently not a single occurrence of EPROBE_DEFER in i915, apart from the one I added.
In sound/ there are 88 occurrences of EPROBE_DEFER in soc/, plus 1 in ppc/ and that's it. So not a single one in pci/hda/ where hda_intel.c resides.
Is the fun with EPROBE_DEFER handling caused by the lack thereof?
Best regards,
Lukas