Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1736986 > unrolled thread

[PATCH v2 -next] spi: fix spi-sprd-adi build errors when SPI_SPRD_ADI=y and HWSPINLOCK=m

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 -next] spi: fix spi-sprd-adi build errors when  SPI_SPRD_ADI=y and HWSPINLOCK=m Randy Dunlap <rdunlap@infradead.org> - 2017-09-21 20:10 +0200
    Re: [PATCH v2 -next] spi: fix spi-sprd-adi build errors when  SPI_SPRD_ADI=y and HWSPINLOCK=m Mark Brown <broonie@kernel.org> - 2017-09-22 11:30 +0200
      Re: [PATCH v2 -next] spi: fix spi-sprd-adi build errors when  SPI_SPRD_ADI=y and HWSPINLOCK=m Randy Dunlap <rdunlap@infradead.org> - 2017-09-23 03:50 +0200
        Re: [PATCH v2 -next] spi: fix spi-sprd-adi build errors when  SPI_SPRD_ADI=y and HWSPINLOCK=m Mark Brown <broonie@kernel.org> - 2017-09-25 18:20 +0200
          Re: [PATCH v2 -next] spi: fix spi-sprd-adi build errors when  SPI_SPRD_ADI=y and HWSPINLOCK=m Randy Dunlap <rdunlap@infradead.org> - 2017-09-25 19:30 +0200
            Re: [PATCH v2 -next] spi: fix spi-sprd-adi build errors when  SPI_SPRD_ADI=y and HWSPINLOCK=m Mark Brown <broonie@kernel.org> - 2017-09-25 20:50 +0200
            Re: [PATCH v2 -next] spi: fix spi-sprd-adi build errors when  SPI_SPRD_ADI=y and HWSPINLOCK=m Geert Uytterhoeven <geert@linux-m68k.org> - 2017-09-25 21:30 +0200
              Re: [PATCH v2 -next] spi: fix spi-sprd-adi build errors when  SPI_SPRD_ADI=y and HWSPINLOCK=m Mark Brown <broonie@kernel.org> - 2017-09-25 21:30 +0200
                Re: [PATCH v2 -next] spi: fix spi-sprd-adi build errors when  SPI_SPRD_ADI=y and HWSPINLOCK=m Randy Dunlap <rdunlap@infradead.org> - 2017-09-25 21:40 +0200
                  Re: [PATCH v2 -next] spi: fix spi-sprd-adi build errors when  SPI_SPRD_ADI=y and HWSPINLOCK=m Mark Brown <broonie@kernel.org> - 2017-09-25 21:50 +0200
                    Re: [PATCH v2 -next] spi: fix spi-sprd-adi build errors when  SPI_SPRD_ADI=y and HWSPINLOCK=m Randy Dunlap <rdunlap@infradead.org> - 2017-09-25 22:00 +0200
                      Re: [PATCH v2 -next] spi: fix spi-sprd-adi build errors when  SPI_SPRD_ADI=y and HWSPINLOCK=m Mark Brown <broonie@kernel.org> - 2017-09-25 23:20 +0200
                        Re: [PATCH v2 -next] spi: fix spi-sprd-adi build errors when  SPI_SPRD_ADI=y and HWSPINLOCK=m Randy Dunlap <rdunlap@infradead.org> - 2017-09-26 00:40 +0200

#1736986 — [PATCH v2 -next] spi: fix spi-sprd-adi build errors when SPI_SPRD_ADI=y and HWSPINLOCK=m

From: Randy Dunlap <rdunlap@infradead.org>

Fix build errors when CONFIG_HWSPINLOCK=m and SPI_SPRD_ADI=y.
That combination is not allowed.

drivers/spi/spi-sprd-adi.o: In function `sprd_adi_remove':
spi-sprd-adi.c:(.text+0x13): undefined reference to `hwspin_lock_free'
drivers/spi/spi-sprd-adi.o: In function `sprd_adi_probe':
spi-sprd-adi.c:(.text+0xf5): undefined reference to `of_hwspin_lock_get_id'
spi-sprd-adi.c:(.text+0x107): undefined reference to `hwspin_lock_request_specific'
spi-sprd-adi.c:(.text+0x22e): undefined reference to `hwspin_lock_free'
drivers/spi/spi-sprd-adi.o: In function `sprd_adi_transfer_one':
spi-sprd-adi.c:(.text+0x2eb): undefined reference to `__hwspin_lock_timeout'
spi-sprd-adi.c:(.text+0x349): undefined reference to `__hwspin_unlock'
spi-sprd-adi.c:(.text+0x389): undefined reference to `__hwspin_lock_timeout'
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>
Cc: Baolin Wang <Baolin.Wang@spreadtrum.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: linux-spi@vger.kernel.org
---
 drivers/spi/Kconfig |    1 +
 1 file changed, 1 insertion(+)

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

[toc] | [next] | [standalone]

#1737329

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

#1737953

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

-- 
~Randy

[toc] | [prev] | [next] | [standalone]

#1739149

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

#1739187

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.


-- 
~Randy

[toc] | [prev] | [next] | [standalone]

#1739220

[Multipart message — attachments visible in raw view] — view raw

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?

[toc] | [prev] | [next] | [standalone]

#1739232

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

[toc] | [prev] | [next] | [standalone]

#1739233

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

#1739240

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?

-- 
~Randy

[toc] | [prev] | [next] | [standalone]

#1739248

[Multipart message — attachments visible in raw view] — view raw

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.  

[toc] | [prev] | [next] | [standalone]

#1739253

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.

-- 
~Randy

[toc] | [prev] | [next] | [standalone]

#1739285

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

#1739320

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.


-- 
~Randy

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web