On Fri, 2016-12-16 at 14:28 +0100, Jean Delvare wrote: > Hi Andy, > > Thanks for the review. > > On Fri, 16 Dec 2016 13:31:19 +0200, Andy Shevchenko wrote: > > On Fri, 2016-12-16 at 11:50 +0100, Jean Delvare wrote: > > > Hiding tristate options with "if EXPERT" is usually not a good > > > idea. > > > You can decide that the driver should be included by default, but > > > you > > > don't know if the user wants it built-in or as a module. Hiding > > > the > > > option prevents the user from making that decision. > > > > Then please start from 8250_PCI. > > 8250_PCI is indeed a candidate for a similar change. However it is > less > specific and also doesn't select other drivers, so fixing it had lower > priority for me. I can look into it later. Okay, just to make sure you know that drivers we are talking about used to be a part of 8250_pci > > > This is even more problematic when said option selects other > > > options. > > > You end up with several device drivers forcibly built into the > > > kernel. > > > > > > In this specific case, drivers 8250_lpss, dw_dmac_core and > > > dw_dmac_pci end up being built-in as soon as SERIAL_8250=y. It is > > > very common for distribution kernels to build the subsystem core > > > code > > > into the kernel, because almost everybody will need it, but build > > > all > > > the drivers as modules. This should be made possible. > > > > ...and make new problems occur. We have no driver dependency between > > them, so, depending of load ordering the user will or will not have > > DMA > > resource available. > > OK, I see. How is the problem solved when all drivers are built-in? > When modular, what is the order in which 8250_lpss, dw_dmac_core and > dw_dmac_pci (and maybe others?) should be loaded? Luckily it works, but any asynchronous driver probe or similar non- linear probing will break this easily. Or just link ordering... (Btw, for dw_dmac in ACPI/platform case we put it on subsys initcall because of that) We are starving for something like what Rafael has been doing for PM (device dependencies). Kernel has a lot of home brewed custom solutions for such cases: regulators, DMA, etc, where we have no explicit parent<>child relationship. > Users setting CONFIG_EXPERT=y can already build these drivers as > modules. It's even the default if CONFIG_SERIAL_8250=m, even when > CONFIG_EXPERT isn't enabled. So the problem you mention already exists > today, it has little to do with my proposed patch. That's true, though like I mentioned one who enables this has to have clear understanding what might be the consequences. > I think there are ways to solve that kind inter-module "soft" > dependencies. Either in kernel-space, using try_then_request_module(), > or in user-space, with modprobe install statements. This should be > done. See above. > > > That's why I would like to keep this for EXPERTs. > > > > CONFIG_EXPERT=y > > CONFIG_SERIAL_8250_LPSS=m > > > > will not work? > > It would "work" in that it would allow us to modularize the drivers. > However enabling CONFIG_EXPERT would make more than 100 extra kernel > options visible, making our kernel configuration work even more > tedious > and error-prone. It worked for now, no one complained about having 8250_pci compiled in until now. > If even general-purpose distribution kernels need to enable > CONFIG_EXPERT to get a sane configuration, it pretty much voids the > point of this option. So I do not consider this possibility as > satisfactory, sorry. > > > > > > > So drop the "if EXPERT" and make SERIAL_8250_LPSS visible. The > > > right > > > way to avoid annoying people who couldn't care less about this > > > option > > > is to make it depend on X86_INTEL_LPSS. There may be no technical > > > dependency between these two options, but in practice, either > > > people > > > want support for a platform, or they don't. > > > > No. You forget Intel Quark. It has nothing to do with > > X86_INTEL_LPSS. > > OK, it wasn't obvious to me. No big deal, we can omit this part of the > patch, it was only an "optimization" which I can easily live without. Yeah, please, remove those parts from your patches and re-submit. For "if EXPERT" part I'm not the one who is supporting it (I remember shouting Linus when we had enabled two HSU_DMA options instead of one). I leave this to more experienced developers / maintainers. -- Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> Intel Finland Oy -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
