diff mbox

[GIT,PULL] qcom SoC changes for 4.4 *RESEND*

Message ID 6622700.vqj78zoFs4@wuerfel (mailing list archive)
State Not Applicable, archived
Delegated to: Andy Gross
Headers show

Commit Message

Arnd Bergmann Oct. 16, 2015, 7:19 p.m. UTC 
On Friday 16 October 2015 09:56:30 Stephen Boyd wrote:
> On 10/15/2015 02:05 PM, Arnd Bergmann wrote:
> >
> >  This also seemed like the right place
> > to add a bugfix that I had in my queue:
> >
> > commit 73ebb85444b0472d90bb70a1a9e6b5df3f92c14c
> > Author: Arnd Bergmann <arnd@arndb.de>
> > Date:   Tue Oct 13 17:05:39 2015 +0200
> >
> >     soc: qcom/smem: add HWSPINLOCK dependency
> >     
> >     This fixes a build error when smem is enabled without hwspinlock:
> >     
> >     drivers/built-in.o: In function `qcom_smem_alloc':
> >     rockchip-efuse.c:(.text+0x7a3e4): undefined reference to `__hwspin_lock_timeout'
> >     rockchip-efuse.c:(.text+0x7a568): undefined reference to `__hwspin_unlock'
> >     drivers/built-in.o: In function `qcom_smem_remove':
> >     rockchip-efuse.c:(.text+0x7a5cc): undefined reference to `hwspin_lock_free'
> >     drivers/built-in.o: In function `qcom_smem_probe':
> >     rockchip-efuse.c:(.text+0x7a960): undefined reference to `hwspin_lock_request_specific'
> >     rockchip-efuse.c:(.text+0x7a988): undefined reference to `of_hwspin_lock_get_id'
> >     drivers/built-in.o: In function `qcom_smem_get':
> >     rockchip-efuse.c:(.text+0x7aa24): undefined reference to `__hwspin_lock_timeout'
> >     rockchip-efuse.c:(.text+0x7aafc): undefined reference to `__hwspin_unlock'
> >     
> 
> Can you share your .config? It looks like there are stubs for these, so
> I'm lost how we got undefined references.
> 
http://pastebin.com/HtrC510p

The problem is CONFIG_HWSPINLOCK=m && CONFIG_QCOM_SMEM=y. Sorry for sending
an incomplete patch description, I had not noticed those wrapper functions.

My patch fixes the issue, but does not allow you to build QCOM_SMEM without
HWSPINLOCK. If we want that configuration to be valid, we need one of
the two changes below:




	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Stephen Boyd Oct. 16, 2015, 8:04 p.m. UTC | #1 
On 10/16, Arnd Bergmann wrote:
> On Friday 16 October 2015 09:56:30 Stephen Boyd wrote:
> > 
> > Can you share your .config? It looks like there are stubs for these, so
> > I'm lost how we got undefined references.
> > 
> http://pastebin.com/HtrC510p
> 
> The problem is CONFIG_HWSPINLOCK=m && CONFIG_QCOM_SMEM=y. Sorry for sending
> an incomplete patch description, I had not noticed those wrapper functions.
> 
> My patch fixes the issue, but does not allow you to build QCOM_SMEM without
> HWSPINLOCK. If we want that configuration to be valid, we need one of
> the two changes below:
> 
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index eec76141d9b9..cae0ffa19bca 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -22,7 +22,7 @@ config QCOM_PM
>  config QCOM_SMEM
>  	tristate "Qualcomm Shared Memory Manager (SMEM)"
>  	depends on ARCH_QCOM
> -	depends on HWSPINLOCK
> +	depends on HWSPINLOCK && !HWSPINLOCK

What does this mean? Is it missing an =m?

>  	help
>  	  Say y here to enable support for the Qualcomm Shared Memory Manager.
>  	  The driver provides an interface to items in a heap shared among all
> diff --git a/include/linux/hwspinlock.h b/include/linux/hwspinlock.h
> index 859d673d98c8..cdfd9fd2ba11 100644
> --- a/include/linux/hwspinlock.h
> +++ b/include/linux/hwspinlock.h
> @@ -59,7 +59,7 @@ struct hwspinlock_pdata {
>  	int base_id;
>  };
>  
> -#if defined(CONFIG_HWSPINLOCK) || defined(CONFIG_HWSPINLOCK_MODULE)
> +#if IS_REACHABLE(CONFIG_HWSPINLOCK)
>  
>  int hwspin_lock_register(struct hwspinlock_device *bank, struct device *dev,
>  		const struct hwspinlock_ops *ops, int base_id, int num_locks);
> 

I'd prefer this patch. Is there some way to capture that
QCOM_SMEM must be a module if HWSPINLOCK is a module unless we're
doing a compile test, in which case we don't care?

I guess that would just be:

	depends on HWSPINLOCK || COMPILE_TEST
Arnd Bergmann Oct. 16, 2015, 8:17 p.m. UTC | #2 
On Friday 16 October 2015 13:04:17 Stephen Boyd wrote:
> On 10/16, Arnd Bergmann wrote:
> > On Friday 16 October 2015 09:56:30 Stephen Boyd wrote:
> > > 
> > > Can you share your .config? It looks like there are stubs for these, so
> > > I'm lost how we got undefined references.
> > > 
> > http://pastebin.com/HtrC510p
> > 
> > The problem is CONFIG_HWSPINLOCK=m && CONFIG_QCOM_SMEM=y. Sorry for sending
> > an incomplete patch description, I had not noticed those wrapper functions.
> > 
> > My patch fixes the issue, but does not allow you to build QCOM_SMEM without
> > HWSPINLOCK. If we want that configuration to be valid, we need one of
> > the two changes below:
> > 
> > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> > index eec76141d9b9..cae0ffa19bca 100644
> > --- a/drivers/soc/qcom/Kconfig
> > +++ b/drivers/soc/qcom/Kconfig
> > @@ -22,7 +22,7 @@ config QCOM_PM
> >  config QCOM_SMEM
> >  	tristate "Qualcomm Shared Memory Manager (SMEM)"
> >  	depends on ARCH_QCOM
> > -	depends on HWSPINLOCK
> > +	depends on HWSPINLOCK && !HWSPINLOCK
> 
> What does this mean?

This is the magic way in Kconfig to express what we want ;-)

It says it depends on either HWSPINLOCK being disabled (!HWSPINLOCK),
or it depends on HWSPINLOCK. The latter implies that QCOM_SMEM cannot
be built-in when HWSPINLOCK=m, but they are allowed to both be built-in.

> Is it missing an =m?

No

> >  	help
> >  	  Say y here to enable support for the Qualcomm Shared Memory Manager.
> >  	  The driver provides an interface to items in a heap shared among all
> > diff --git a/include/linux/hwspinlock.h b/include/linux/hwspinlock.h
> > index 859d673d98c8..cdfd9fd2ba11 100644
> > --- a/include/linux/hwspinlock.h
> > +++ b/include/linux/hwspinlock.h
> > @@ -59,7 +59,7 @@ struct hwspinlock_pdata {
> >  	int base_id;
> >  };
> >  
> > -#if defined(CONFIG_HWSPINLOCK) || defined(CONFIG_HWSPINLOCK_MODULE)
> > +#if IS_REACHABLE(CONFIG_HWSPINLOCK)
> >  
> >  int hwspin_lock_register(struct hwspinlock_device *bank, struct device *dev,
> >  		const struct hwspinlock_ops *ops, int base_id, int num_locks);
> > 
> 
> I'd prefer this patch. Is there some way to capture that
> QCOM_SMEM must be a module if HWSPINLOCK is a module unless we're
> doing a compile test, in which case we don't care?

The problem here is that while we avoid the build error, we get the
counterintuitive behavior that you can have the HWSPINLOCK module
loaded but the built-in QCOM_SMEM module does not use it because
it wasn't reachable at compile time.

> I guess that would just be:
> 
> 	depends on HWSPINLOCK || COMPILE_TEST

This would not solve the randconfig build problem.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Boyd Oct. 16, 2015, 9:26 p.m. UTC | #3 
On 10/16, Arnd Bergmann wrote:
> On Friday 16 October 2015 13:04:17 Stephen Boyd wrote:
> > On 10/16, Arnd Bergmann wrote:
> > > On Friday 16 October 2015 09:56:30 Stephen Boyd wrote:
> > > > 
> > > > Can you share your .config? It looks like there are stubs for these, so
> > > > I'm lost how we got undefined references.
> > > > 
> > > http://pastebin.com/HtrC510p
> > > 
> > > The problem is CONFIG_HWSPINLOCK=m && CONFIG_QCOM_SMEM=y. Sorry for sending
> > > an incomplete patch description, I had not noticed those wrapper functions.
> > > 
> > > My patch fixes the issue, but does not allow you to build QCOM_SMEM without
> > > HWSPINLOCK. If we want that configuration to be valid, we need one of
> > > the two changes below:
> > > 
> > > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> > > index eec76141d9b9..cae0ffa19bca 100644
> > > --- a/drivers/soc/qcom/Kconfig
> > > +++ b/drivers/soc/qcom/Kconfig
> > > @@ -22,7 +22,7 @@ config QCOM_PM
> > >  config QCOM_SMEM
> > >  	tristate "Qualcomm Shared Memory Manager (SMEM)"
> > >  	depends on ARCH_QCOM
> > > -	depends on HWSPINLOCK
> > > +	depends on HWSPINLOCK && !HWSPINLOCK
> > 
> > What does this mean?
> 
> This is the magic way in Kconfig to express what we want ;-)
> 
> It says it depends on either HWSPINLOCK being disabled (!HWSPINLOCK),
> or it depends on HWSPINLOCK. The latter implies that QCOM_SMEM cannot
> be built-in when HWSPINLOCK=m, but they are allowed to both be built-in.
> 

I think you mean OR then?

	depends on HWSPINLOCK || !HWSPINLOCK

If I have HWSPINLOCK=y and try menuconfig with your patch, the
smem config isn't selectable. But if HWSPINLOCK=m then the smem
config is visible and smem can only be a module.

> 
> > >  	help
> > >  	  Say y here to enable support for the Qualcomm Shared Memory Manager.
> > >  	  The driver provides an interface to items in a heap shared among all
> > > diff --git a/include/linux/hwspinlock.h b/include/linux/hwspinlock.h
> > > index 859d673d98c8..cdfd9fd2ba11 100644
> > > --- a/include/linux/hwspinlock.h
> > > +++ b/include/linux/hwspinlock.h
> > > @@ -59,7 +59,7 @@ struct hwspinlock_pdata {
> > >  	int base_id;
> > >  };
> > >  
> > > -#if defined(CONFIG_HWSPINLOCK) || defined(CONFIG_HWSPINLOCK_MODULE)
> > > +#if IS_REACHABLE(CONFIG_HWSPINLOCK)
> > >  
> > >  int hwspin_lock_register(struct hwspinlock_device *bank, struct device *dev,
> > >  		const struct hwspinlock_ops *ops, int base_id, int num_locks);
> > > 
> > 
> > I'd prefer this patch. Is there some way to capture that
> > QCOM_SMEM must be a module if HWSPINLOCK is a module unless we're
> > doing a compile test, in which case we don't care?
> 
> The problem here is that while we avoid the build error, we get the
> counterintuitive behavior that you can have the HWSPINLOCK module
> loaded but the built-in QCOM_SMEM module does not use it because
> it wasn't reachable at compile time.
> 

Agreed, which is why I suggested depends on HWSPINLOCK ||
COMPILE_TEST + the IS_REACHABLE patch. That way if hwspinlock=y,
smem can be y or m, if hwspinlock=m, smem can only be m, and if
hwspinlock=n then smem can only be y or m if it has
COMPILE_TEST=y. In the last case the IS_REACHABLE ifdef will "do
the right thing" and make it so that HWSPINLOCK symbols are
stubbed out if hwspinlock=m and smem=y or hwspinlock=n and
smem=y/m.

I don't think we ever care about a kernel that actually runs when
COMPILE_TEST=y, so this seems to be ok.
Arnd Bergmann Oct. 16, 2015, 9:37 p.m. UTC | #4 
On Friday 16 October 2015 14:26:27 Stephen Boyd wrote:
> On 10/16, Arnd Bergmann wrote:
> > On Friday 16 October 2015 13:04:17 Stephen Boyd wrote:
> > > On 10/16, Arnd Bergmann wrote:
> > > > On Friday 16 October 2015 09:56:30 Stephen Boyd wrote:
> > > > > 
> > > > > Can you share your .config? It looks like there are stubs for these, so
> > > > > I'm lost how we got undefined references.
> > > > > 
> > > > http://pastebin.com/HtrC510p
> > > > 
> > > > The problem is CONFIG_HWSPINLOCK=m && CONFIG_QCOM_SMEM=y. Sorry for sending
> > > > an incomplete patch description, I had not noticed those wrapper functions.
> > > > 
> > > > My patch fixes the issue, but does not allow you to build QCOM_SMEM without
> > > > HWSPINLOCK. If we want that configuration to be valid, we need one of
> > > > the two changes below:
> > > > 
> > > > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> > > > index eec76141d9b9..cae0ffa19bca 100644
> > > > --- a/drivers/soc/qcom/Kconfig
> > > > +++ b/drivers/soc/qcom/Kconfig
> > > > @@ -22,7 +22,7 @@ config QCOM_PM
> > > >  config QCOM_SMEM
> > > >  	tristate "Qualcomm Shared Memory Manager (SMEM)"
> > > >  	depends on ARCH_QCOM
> > > > -	depends on HWSPINLOCK
> > > > +	depends on HWSPINLOCK && !HWSPINLOCK
> > > 
> > > What does this mean?
> > 
> > This is the magic way in Kconfig to express what we want ;-)
> > 
> > It says it depends on either HWSPINLOCK being disabled (!HWSPINLOCK),
> > or it depends on HWSPINLOCK. The latter implies that QCOM_SMEM cannot
> > be built-in when HWSPINLOCK=m, but they are allowed to both be built-in.
> > 
> 
> I think you mean OR then?
> 
> 	depends on HWSPINLOCK || !HWSPINLOCK

Yes, sorry about that.

> If I have HWSPINLOCK=y and try menuconfig with your patch, the
> smem config isn't selectable. But if HWSPINLOCK=m then the smem
> config is visible and smem can only be a module.

Ok, I have to admit that I don't even know how Kconfig interprets
(FOO && !FOO), so that's probably the result of my typo.

> > 
> > > >  	help
> > > >  	  Say y here to enable support for the Qualcomm Shared Memory Manager.
> > > >  	  The driver provides an interface to items in a heap shared among all
> > > > diff --git a/include/linux/hwspinlock.h b/include/linux/hwspinlock.h
> > > > index 859d673d98c8..cdfd9fd2ba11 100644
> > > > --- a/include/linux/hwspinlock.h
> > > > +++ b/include/linux/hwspinlock.h
> > > > @@ -59,7 +59,7 @@ struct hwspinlock_pdata {
> > > >  	int base_id;
> > > >  };
> > > >  
> > > > -#if defined(CONFIG_HWSPINLOCK) || defined(CONFIG_HWSPINLOCK_MODULE)
> > > > +#if IS_REACHABLE(CONFIG_HWSPINLOCK)
> > > >  
> > > >  int hwspin_lock_register(struct hwspinlock_device *bank, struct device *dev,
> > > >  		const struct hwspinlock_ops *ops, int base_id, int num_locks);
> > > > 
> > > 
> > > I'd prefer this patch. Is there some way to capture that
> > > QCOM_SMEM must be a module if HWSPINLOCK is a module unless we're
> > > doing a compile test, in which case we don't care?
> > 
> > The problem here is that while we avoid the build error, we get the
> > counterintuitive behavior that you can have the HWSPINLOCK module
> > loaded but the built-in QCOM_SMEM module does not use it because
> > it wasn't reachable at compile time.
> > 
> 
> Agreed, which is why I suggested depends on HWSPINLOCK ||
> COMPILE_TEST + the IS_REACHABLE patch. That way if hwspinlock=y,
> smem can be y or m, if hwspinlock=m, smem can only be m, and if
> hwspinlock=n then smem can only be y or m if it has
> COMPILE_TEST=y. In the last case the IS_REACHABLE ifdef will "do
> the right thing" and make it so that HWSPINLOCK symbols are
> stubbed out if hwspinlock=m and smem=y or hwspinlock=n and
> smem=y/m.

Ah, I see. The downside is still that other users of HWSPINLOCK
might now accidentally build but not work if HWSPINLOCK=m.
 
> I don't think we ever care about a kernel that actually runs when
> COMPILE_TEST=y, so this seems to be ok.

Right, for the QCOM_SMEM support that is clearly ok as long as you
have the Kconfig logic there.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index eec76141d9b9..cae0ffa19bca 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -22,7 +22,7 @@  config QCOM_PM
 config QCOM_SMEM
 	tristate "Qualcomm Shared Memory Manager (SMEM)"
 	depends on ARCH_QCOM
-	depends on HWSPINLOCK
+	depends on HWSPINLOCK && !HWSPINLOCK
 	help
 	  Say y here to enable support for the Qualcomm Shared Memory Manager.
 	  The driver provides an interface to items in a heap shared among all
diff --git a/include/linux/hwspinlock.h b/include/linux/hwspinlock.h
index 859d673d98c8..cdfd9fd2ba11 100644
--- a/include/linux/hwspinlock.h
+++ b/include/linux/hwspinlock.h
@@ -59,7 +59,7 @@  struct hwspinlock_pdata {
 	int base_id;
 };
 
-#if defined(CONFIG_HWSPINLOCK) || defined(CONFIG_HWSPINLOCK_MODULE)
+#if IS_REACHABLE(CONFIG_HWSPINLOCK)
 
 int hwspin_lock_register(struct hwspinlock_device *bank, struct device *dev,
 		const struct hwspinlock_ops *ops, int base_id, int num_locks);