| Message ID | ed5b8ca2-b853-755d-2801-57cc0b626508@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 21, 2017 at 11:02:31AM -0700, Randy Dunlap wrote: > spi-sprd-adi.c:(.text+0x3ee): undefined reference to `__hwspin_unlock' > > v2: allow build with or without HWSPINLOCK, but restrict to =m > if HWSPINLOCK=m. > > Signed-off-by: Randy Dunlap <rdunlap@infradead.org> Please put inter version changelogs after the --- as covered in SubmittingPatches. This still seems like the stubs aren't doing the right thing - if we can use hwspinlocks in a module when they're enabled I'd expect to be able to build the stubs that way too.
On 09/22/17 02:26, Mark Brown wrote: > On Thu, Sep 21, 2017 at 11:02:31AM -0700, Randy Dunlap wrote: > >> spi-sprd-adi.c:(.text+0x3ee): undefined reference to `__hwspin_unlock' >> >> v2: allow build with or without HWSPINLOCK, but restrict to =m >> if HWSPINLOCK=m. >> >> Signed-off-by: Randy Dunlap <rdunlap@infradead.org> > > Please put inter version changelogs after the --- as covered in > SubmittingPatches. This still seems like the stubs aren't doing the Yep. > right thing - if we can use hwspinlocks in a module when they're enabled > I'd expect to be able to build the stubs that way too. Sorry, I'm not understanding what you are trying to say on that one. The hwspinlock stubs are present if HWSPINLOCK is not enabled (when <linux/hwspinlock.h> is #included). The following kconfig combinations build: HWSPINLOCK SPI_SPRD_ADI n y n m y y y m m m but this combo is not allowed (with the patch) or causes build errors (without the patch): m y
On Fri, Sep 22, 2017 at 06:46:46PM -0700, Randy Dunlap wrote: > On 09/22/17 02:26, Mark Brown wrote: > > right thing - if we can use hwspinlocks in a module when they're enabled > > I'd expect to be able to build the stubs that way too. > Sorry, I'm not understanding what you are trying to say on that one. > HWSPINLOCK SPI_SPRD_ADI > but this combo is not allowed (with the patch) or causes build errors > (without the patch): > m y Why is that not just an || COMPILE_TEST dependency then? The dependency you're trying to introduce is weird and confusing, we shouldn't be having to do things like that.
On 09/25/17 09:12, Mark Brown wrote: > On Fri, Sep 22, 2017 at 06:46:46PM -0700, Randy Dunlap wrote: >> On 09/22/17 02:26, Mark Brown wrote: > >>> right thing - if we can use hwspinlocks in a module when they're enabled >>> I'd expect to be able to build the stubs that way too. > >> Sorry, I'm not understanding what you are trying to say on that one. > >> HWSPINLOCK SPI_SPRD_ADI > >> but this combo is not allowed (with the patch) or causes build errors >> (without the patch): >> m y > > Why is that not just an || COMPILE_TEST dependency then? The dependency > you're trying to introduce is weird and confusing, we shouldn't be > having to do things like that. It already uses COMPILE_TEST: config SPI_SPRD_ADI tristate "Spreadtrum ADI controller" depends on ARCH_SPRD || COMPILE_TEST help ADI driver based on SPI for Spreadtrum SoCs. but that allows build errors when SPI_SPRD_ADI=y and HWSPINLOCK=y. As for the dependency that I am adding: + depends on HWSPINLOCK || HWSPINLOCK=n that idiom is used over and over again in Kconfig files to prevent this kind of build problem in loadable modules.
On Mon, Sep 25, 2017 at 10:19:56AM -0700, Randy Dunlap wrote: > On 09/25/17 09:12, Mark Brown wrote: > > Why is that not just an || COMPILE_TEST dependency then? The dependency > > you're trying to introduce is weird and confusing, we shouldn't be > > having to do things like that. > It already uses COMPILE_TEST: > config SPI_SPRD_ADI > tristate "Spreadtrum ADI controller" > depends on ARCH_SPRD || COMPILE_TEST > help > ADI driver based on SPI for Spreadtrum SoCs. > but that allows build errors when SPI_SPRD_ADI=y and HWSPINLOCK=y. > As for the dependency that I am adding: > + depends on HWSPINLOCK || HWSPINLOCK=n > that idiom is used over and over again in Kconfig files to prevent this kind of > build problem in loadable modules. And what is that equivalent to in relation to COMPILE_TEST, especially considering the situations where you'd build this without hwspinlock support?
Hi Randy, On Mon, Sep 25, 2017 at 7:19 PM, Randy Dunlap <rdunlap@infradead.org> wrote: > On 09/25/17 09:12, Mark Brown wrote: >> On Fri, Sep 22, 2017 at 06:46:46PM -0700, Randy Dunlap wrote: >>> On 09/22/17 02:26, Mark Brown wrote: >> >>>> right thing - if we can use hwspinlocks in a module when they're enabled >>>> I'd expect to be able to build the stubs that way too. >> >>> Sorry, I'm not understanding what you are trying to say on that one. >> >>> HWSPINLOCK SPI_SPRD_ADI >> >>> but this combo is not allowed (with the patch) or causes build errors >>> (without the patch): >>> m y >> >> Why is that not just an || COMPILE_TEST dependency then? The dependency >> you're trying to introduce is weird and confusing, we shouldn't be >> having to do things like that. > > It already uses COMPILE_TEST: > > config SPI_SPRD_ADI > tristate "Spreadtrum ADI controller" > depends on ARCH_SPRD || COMPILE_TEST > help > ADI driver based on SPI for Spreadtrum SoCs. > > but that allows build errors when SPI_SPRD_ADI=y and HWSPINLOCK=y. You mean "when SPI_SPRD_ADI=y and HWSPINLOCK=m"? > As for the dependency that I am adding: > + depends on HWSPINLOCK || HWSPINLOCK=n > > that idiom is used over and over again in Kconfig files to prevent this kind of > build problem in loadable modules. Indeed, it's confusing, but used all over the place. The issue is builtin drivers that depend on a modular API. The clean way is to separate API and implementation, so the API can be builtin, and the implementation can be modular. Hence the API should provide stubs that call into function pointers, to be registered by the module providing the implementation. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 25, 2017 at 09:20:31PM +0200, Geert Uytterhoeven wrote: > The issue is builtin drivers that depend on a modular API. The clean way > is to separate API and implementation, so the API can be builtin, and the > implementation can be modular. > Hence the API should provide stubs that call into function pointers, to be > registered by the module providing the implementation. In this case the problem is even more basic in that the driver does actually depend on having hwspinlocks for any production use.
On 09/25/17 12:22, Mark Brown wrote: > On Mon, Sep 25, 2017 at 09:20:31PM +0200, Geert Uytterhoeven wrote: > >> The issue is builtin drivers that depend on a modular API. The clean way >> is to separate API and implementation, so the API can be builtin, and the >> implementation can be modular. >> Hence the API should provide stubs that call into function pointers, to be >> registered by the module providing the implementation. > > In this case the problem is even more basic in that the driver does > actually depend on having hwspinlocks for any production use. > so just add: depends on HWSPINLOCK Is that satisfactory to you?
On Mon, Sep 25, 2017 at 12:31:03PM -0700, Randy Dunlap wrote: > On 09/25/17 12:22, Mark Brown wrote: > > In this case the problem is even more basic in that the driver does > > actually depend on having hwspinlocks for any production use. > so just add: > depends on HWSPINLOCK > Is that satisfactory to you? That would then get in the way of build test coverage.
On 09/25/17 12:44, Mark Brown wrote: > On Mon, Sep 25, 2017 at 12:31:03PM -0700, Randy Dunlap wrote: >> On 09/25/17 12:22, Mark Brown wrote: > >>> In this case the problem is even more basic in that the driver does >>> actually depend on having hwspinlocks for any production use. > >> so just add: >> depends on HWSPINLOCK > >> Is that satisfactory to you? > > That would then get in the way of build test coverage. I don't agree, but whatever. I give up.
On Mon, Sep 25, 2017 at 12:53:20PM -0700, Randy Dunlap wrote: > On 09/25/17 12:44, Mark Brown wrote: > > That would then get in the way of build test coverage. > I don't agree, but whatever. I give up. OK. Just as a general thing it's really important that dependency changes be thought through and express what the constraints are in ways that reflect what's actually going on and what's useful for users, it seems to be a frequent problem and it does get in the way.
On 09/25/17 14:11, Mark Brown wrote: > On Mon, Sep 25, 2017 at 12:53:20PM -0700, Randy Dunlap wrote: >> On 09/25/17 12:44, Mark Brown wrote: > >>> That would then get in the way of build test coverage. > >> I don't agree, but whatever. I give up. > > OK. Just as a general thing it's really important that dependency > changes be thought through and express what the constraints are in ways > that reflect what's actually going on and what's useful for users, it > seems to be a frequent problem and it does get in the way. Yes, it's a frequent problem that drivers do not build due to some kconfig dependency issue(s). I would prefer to see the driver maintainer(s) fix build errors themselves, but they are not always aware of the idiosyncrasies of kconfig. I think that people often submit drivers (or driver changes) after building it in one (working) configuration only, without regard for the other possible combinations.
--- linux-next-20170921.orig/drivers/spi/Kconfig +++ linux-next-20170921/drivers/spi/Kconfig @@ -625,6 +625,7 @@ config SPI_SIRF config SPI_SPRD_ADI tristate "Spreadtrum ADI controller" depends on ARCH_SPRD || COMPILE_TEST + depends on HWSPINLOCK || HWSPINLOCK=n help ADI driver based on SPI for Spreadtrum SoCs.