linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] soc: qcom: update config dependencies for QCOM_RPMPD
@ 2019-01-18  4:48 Rajendra Nayak
  2019-01-18 17:39 ` Stephen Boyd
  0 siblings, 1 reply; 11+ messages in thread
From: Rajendra Nayak @ 2019-01-18  4:48 UTC (permalink / raw)
  To: andy.gross, david.brown
  Cc: linux-arm-msm, linux-kernel, linux, Rajendra Nayak

Since QCOM_RPMPD is bool and it depends on QCOM_SMD_RPM
which is tristate, configurations such as arm64:allmodconfig
result in

CONFIG_QCOM_RPMPD=y
CONFIG_QCOM_SMD_RPM=m

This in turn results in

drivers/soc/qcom/rpmpd.o: In function `rpmpd_send_corner':
rpmpd.c:(.text+0x10c): undefined reference to `qcom_rpm_smd_write'
drivers/soc/qcom/rpmpd.o: In function `rpmpd_power_on':
rpmpd.c:(.text+0x3b4): undefined reference to `qcom_rpm_smd_write'
drivers/soc/qcom/rpmpd.o: In function `rpmpd_power_off':
rpmpd.c:(.text+0x520): undefined reference to `qcom_rpm_smd_write'
make: *** [vmlinux] Error 1

Fix it by making QCOM_RPMPD depend on QCOM_SMD_RPM=y

Reported-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
Andy, this one should be applied on top of my v11 to add
rpmpd/rpmhpd drivers [1] and the fix from Bjorn to drop
A family RPM dependency [2]

[1] https://lkml.org/lkml/2019/1/9/1257
[2] https://lkml.org/lkml/2019/1/17/5

 drivers/soc/qcom/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index a5d5167c3f16..1ee298f6bf17 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -109,7 +109,7 @@ config QCOM_RPMHPD
 
 config QCOM_RPMPD
 	bool "Qualcomm RPM Power domain driver"
-	depends on QCOM_SMD_RPM
+	depends on QCOM_SMD_RPM=y
 	help
 	  QCOM RPM Power domain driver to support power-domains with
 	  performance states. The driver communicates a performance state
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] soc: qcom: update config dependencies for QCOM_RPMPD
  2019-01-18  4:48 [PATCH] soc: qcom: update config dependencies for QCOM_RPMPD Rajendra Nayak
@ 2019-01-18 17:39 ` Stephen Boyd
  2019-01-22  2:30   ` Rajendra Nayak
  2019-01-31  1:32   ` Bjorn Andersson
  0 siblings, 2 replies; 11+ messages in thread
From: Stephen Boyd @ 2019-01-18 17:39 UTC (permalink / raw)
  To: Rajendra Nayak, andy.gross, david.brown
  Cc: linux-arm-msm, linux-kernel, linux, Rajendra Nayak

Quoting Rajendra Nayak (2019-01-17 20:48:01)
>  drivers/soc/qcom/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index a5d5167c3f16..1ee298f6bf17 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -109,7 +109,7 @@ config QCOM_RPMHPD
>  
>  config QCOM_RPMPD
>         bool "Qualcomm RPM Power domain driver"

Just curious, does it need to be bool for some reason?


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] soc: qcom: update config dependencies for QCOM_RPMPD
  2019-01-18 17:39 ` Stephen Boyd
@ 2019-01-22  2:30   ` Rajendra Nayak
  2019-01-22  5:08     ` Guenter Roeck
  2019-01-31  1:32   ` Bjorn Andersson
  1 sibling, 1 reply; 11+ messages in thread
From: Rajendra Nayak @ 2019-01-22  2:30 UTC (permalink / raw)
  To: Stephen Boyd, andy.gross, david.brown; +Cc: linux-arm-msm, linux-kernel, linux



On 1/18/2019 11:09 PM, Stephen Boyd wrote:
> Quoting Rajendra Nayak (2019-01-17 20:48:01)
>>   drivers/soc/qcom/Kconfig | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index a5d5167c3f16..1ee298f6bf17 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -109,7 +109,7 @@ config QCOM_RPMHPD
>>   
>>   config QCOM_RPMPD
>>          bool "Qualcomm RPM Power domain driver"
> 
> Just curious, does it need to be bool for some reason?

Here's the link to the discussion around it on the v3 patchset of this series
https://lkml.org/lkml/2018/6/12/111

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] soc: qcom: update config dependencies for QCOM_RPMPD
  2019-01-22  2:30   ` Rajendra Nayak
@ 2019-01-22  5:08     ` Guenter Roeck
  2019-01-22  9:54       ` Rajendra Nayak
  0 siblings, 1 reply; 11+ messages in thread
From: Guenter Roeck @ 2019-01-22  5:08 UTC (permalink / raw)
  To: Rajendra Nayak, Stephen Boyd, andy.gross, david.brown
  Cc: linux-arm-msm, linux-kernel

On 1/21/19 6:30 PM, Rajendra Nayak wrote:
> 
> 
> On 1/18/2019 11:09 PM, Stephen Boyd wrote:
>> Quoting Rajendra Nayak (2019-01-17 20:48:01)
>>>   drivers/soc/qcom/Kconfig | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>>> index a5d5167c3f16..1ee298f6bf17 100644
>>> --- a/drivers/soc/qcom/Kconfig
>>> +++ b/drivers/soc/qcom/Kconfig
>>> @@ -109,7 +109,7 @@ config QCOM_RPMHPD
>>>   config QCOM_RPMPD
>>>          bool "Qualcomm RPM Power domain driver"
>>
>> Just curious, does it need to be bool for some reason?
> 
> Here's the link to the discussion around it on the v3 patchset of this series
> https://lkml.org/lkml/2018/6/12/111
> 

I think you were missing the implication of "if possible", the implication
being that the driver can now not be used in a multi-platform image, and that
having a bool driver depend on tristate drivers doesn't make much if any sense.

The argument in the exchange is odd - one can not remove any driver as long
there are client drivers / devices attached to it. This is true for all
drivers, not just for this one, and handled quite nicely by the driver core.
Just run "lsmod" and try to remove a driver with any attached users.

Guenter

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] soc: qcom: update config dependencies for QCOM_RPMPD
  2019-01-22  5:08     ` Guenter Roeck
@ 2019-01-22  9:54       ` Rajendra Nayak
  2019-01-22 10:08         ` Viresh Kumar
  0 siblings, 1 reply; 11+ messages in thread
From: Rajendra Nayak @ 2019-01-22  9:54 UTC (permalink / raw)
  To: Guenter Roeck, Stephen Boyd, andy.gross, david.brown
  Cc: linux-arm-msm, linux-kernel, Viresh Kumar, Ulf Hansson

Adding Viresh and Ulf to this thread,

On 1/22/2019 10:38 AM, Guenter Roeck wrote:
> On 1/21/19 6:30 PM, Rajendra Nayak wrote:
>>
>>
>> On 1/18/2019 11:09 PM, Stephen Boyd wrote:
>>> Quoting Rajendra Nayak (2019-01-17 20:48:01)
>>>>   drivers/soc/qcom/Kconfig | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>>>> index a5d5167c3f16..1ee298f6bf17 100644
>>>> --- a/drivers/soc/qcom/Kconfig
>>>> +++ b/drivers/soc/qcom/Kconfig
>>>> @@ -109,7 +109,7 @@ config QCOM_RPMHPD
>>>>   config QCOM_RPMPD
>>>>          bool "Qualcomm RPM Power domain driver"
>>>
>>> Just curious, does it need to be bool for some reason?
>>
>> Here's the link to the discussion around it on the v3 patchset of this series
>> https://lkml.org/lkml/2018/6/12/111
>>
> 
> I think you were missing the implication of "if possible", the implication
> being that the driver can now not be used in a multi-platform image, and that
> having a bool driver depend on tristate drivers doesn't make much if any sense.

Fair enough, I hadn't thought about the implications with multi platform builds,
I guess it makes sense to have these drivers as tristate then.

I was doing some quick testing by adding the calls to of_genpd_remove_last() as
suggested by Ulf for cleaning up the genpd registrations, and I run into this
backtrace when the driver re-probes followed by a remove

[   59.204525] kobject ((____ptrval____)): tried to init an initialized object, something is seriously wr.
[   59.214262] CPU: 3 PID: 1600 Comm: sh Not tainted 5.0.0-rc1-00012-g0513915837c5-dirty #32
[   59.222500] Hardware name: Qualcomm Technologies, Inc. SDM845 MTP (DT)
[   59.229081] Call trace:
[   59.231574]  dump_backtrace+0x0/0x148
[   59.235276]  show_stack+0x14/0x20
[   59.238631]  dump_stack+0x8c/0xac
[   59.241980]  kobject_init+0x8c/0xa0
[   59.245506]  device_initialize+0x34/0xc8
[   59.249474]  pm_genpd_init+0x170/0x260
[   59.253261]  rpmhpd_probe+0x194/0x2b0
[   59.256966]  platform_drv_probe+0x4c/0xa8
[   59.261011]  really_probe+0x1e4/0x2c8
[   59.264711]  driver_probe_device+0x58/0x10
[   59.268927]  bind_store+0xdc/0x178
[   59.272356]  drv_attr_store+0x20/0x30
[   59.276061]  sysfs_kf_write+0x48/0x58
[   59.279760]  kernfs_fop_write+0xcc/0x1c8
[   59.283728]  __vfs_write+0x34/0x170
[   59.287245]  vfs_write+0xa8/0x1b8
[   59.290592]  ksys_write+0x5c/0xc8
[   59.293941]  __arm64_sys_write+0x14/0x20
[   59.297897]  el0_svc_common+0xb4/0x118
[   59.301676]  el0_svc_handler+0x2c/0x80
[   59.305455]  el0_svc+0x8/0xc

Viresh/Ulf, looks like we need some cleanup of whats done by device_initialize()
in pm_genpd_init() to happen as part of the pm_genpd_remove()?

> 
> The argument in the exchange is odd - one can not remove any driver as long
> there are client drivers / devices attached to it. This is true for all
> drivers, not just for this one, and handled quite nicely by the driver core.
> Just run "lsmod" and try to remove a driver with any attached users.
> 
> Guenter

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] soc: qcom: update config dependencies for QCOM_RPMPD
  2019-01-22  9:54       ` Rajendra Nayak
@ 2019-01-22 10:08         ` Viresh Kumar
  2019-01-22 10:35           ` Rajendra Nayak
  0 siblings, 1 reply; 11+ messages in thread
From: Viresh Kumar @ 2019-01-22 10:08 UTC (permalink / raw)
  To: Rajendra Nayak
  Cc: Guenter Roeck, Stephen Boyd, andy.gross, david.brown,
	linux-arm-msm, linux-kernel, Ulf Hansson

On 22-01-19, 15:24, Rajendra Nayak wrote:
> I was doing some quick testing by adding the calls to of_genpd_remove_last() as
> suggested by Ulf for cleaning up the genpd registrations, and I run into this
> backtrace when the driver re-probes followed by a remove
> 
> [   59.204525] kobject ((____ptrval____)): tried to init an initialized object, something is seriously wr.
> [   59.214262] CPU: 3 PID: 1600 Comm: sh Not tainted 5.0.0-rc1-00012-g0513915837c5-dirty #32
> [   59.222500] Hardware name: Qualcomm Technologies, Inc. SDM845 MTP (DT)
> [   59.229081] Call trace:
> [   59.231574]  dump_backtrace+0x0/0x148
> [   59.235276]  show_stack+0x14/0x20
> [   59.238631]  dump_stack+0x8c/0xac
> [   59.241980]  kobject_init+0x8c/0xa0
> [   59.245506]  device_initialize+0x34/0xc8
> [   59.249474]  pm_genpd_init+0x170/0x260
> [   59.253261]  rpmhpd_probe+0x194/0x2b0
> [   59.256966]  platform_drv_probe+0x4c/0xa8
> [   59.261011]  really_probe+0x1e4/0x2c8
> [   59.264711]  driver_probe_device+0x58/0x10
> [   59.268927]  bind_store+0xdc/0x178
> [   59.272356]  drv_attr_store+0x20/0x30
> [   59.276061]  sysfs_kf_write+0x48/0x58
> [   59.279760]  kernfs_fop_write+0xcc/0x1c8
> [   59.283728]  __vfs_write+0x34/0x170
> [   59.287245]  vfs_write+0xa8/0x1b8
> [   59.290592]  ksys_write+0x5c/0xc8
> [   59.293941]  __arm64_sys_write+0x14/0x20
> [   59.297897]  el0_svc_common+0xb4/0x118
> [   59.301676]  el0_svc_handler+0x2c/0x80
> [   59.305455]  el0_svc+0x8/0xc
> 
> Viresh/Ulf, looks like we need some cleanup of whats done by device_initialize()
> in pm_genpd_init() to happen as part of the pm_genpd_remove()?

Yeah, we normally need to call put_device() for that. Maybe something like this:

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 500de1dee967..d5b984f042ec 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1812,6 +1812,7 @@ static int genpd_remove(struct generic_pm_domain *genpd)
        genpd_unlock(genpd);
        cancel_work_sync(&genpd->power_off_work);
        kfree(genpd->free);
+       put_device(&genpd->dev);
        pr_debug("%s: removed %s\n", __func__, genpd->name);
 
        return 0;

-- 
viresh

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] soc: qcom: update config dependencies for QCOM_RPMPD
  2019-01-22 10:08         ` Viresh Kumar
@ 2019-01-22 10:35           ` Rajendra Nayak
  2019-01-23 11:16             ` Viresh Kumar
  0 siblings, 1 reply; 11+ messages in thread
From: Rajendra Nayak @ 2019-01-22 10:35 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Guenter Roeck, Stephen Boyd, andy.gross, david.brown,
	linux-arm-msm, linux-kernel, Ulf Hansson


On 1/22/2019 3:38 PM, Viresh Kumar wrote:
> On 22-01-19, 15:24, Rajendra Nayak wrote:
>> I was doing some quick testing by adding the calls to of_genpd_remove_last() as
>> suggested by Ulf for cleaning up the genpd registrations, and I run into this
>> backtrace when the driver re-probes followed by a remove
>>
>> [   59.204525] kobject ((____ptrval____)): tried to init an initialized object, something is seriously wr.
>> [   59.214262] CPU: 3 PID: 1600 Comm: sh Not tainted 5.0.0-rc1-00012-g0513915837c5-dirty #32
>> [   59.222500] Hardware name: Qualcomm Technologies, Inc. SDM845 MTP (DT)
>> [   59.229081] Call trace:
>> [   59.231574]  dump_backtrace+0x0/0x148
>> [   59.235276]  show_stack+0x14/0x20
>> [   59.238631]  dump_stack+0x8c/0xac
>> [   59.241980]  kobject_init+0x8c/0xa0
>> [   59.245506]  device_initialize+0x34/0xc8
>> [   59.249474]  pm_genpd_init+0x170/0x260
>> [   59.253261]  rpmhpd_probe+0x194/0x2b0
>> [   59.256966]  platform_drv_probe+0x4c/0xa8
>> [   59.261011]  really_probe+0x1e4/0x2c8
>> [   59.264711]  driver_probe_device+0x58/0x10
>> [   59.268927]  bind_store+0xdc/0x178
>> [   59.272356]  drv_attr_store+0x20/0x30
>> [   59.276061]  sysfs_kf_write+0x48/0x58
>> [   59.279760]  kernfs_fop_write+0xcc/0x1c8
>> [   59.283728]  __vfs_write+0x34/0x170
>> [   59.287245]  vfs_write+0xa8/0x1b8
>> [   59.290592]  ksys_write+0x5c/0xc8
>> [   59.293941]  __arm64_sys_write+0x14/0x20
>> [   59.297897]  el0_svc_common+0xb4/0x118
>> [   59.301676]  el0_svc_handler+0x2c/0x80
>> [   59.305455]  el0_svc+0x8/0xc
>>
>> Viresh/Ulf, looks like we need some cleanup of whats done by device_initialize()
>> in pm_genpd_init() to happen as part of the pm_genpd_remove()?
> 
> Yeah, we normally need to call put_device() for that. Maybe something like this:
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 500de1dee967..d5b984f042ec 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1812,6 +1812,7 @@ static int genpd_remove(struct generic_pm_domain *genpd)
>          genpd_unlock(genpd);
>          cancel_work_sync(&genpd->power_off_work);
>          kfree(genpd->free);
> +       put_device(&genpd->dev);
>          pr_debug("%s: removed %s\n", __func__, genpd->name);
>   
>          return 0;

Seeing this now during remove

[   34.008216] ------------[ cut here ]------------
[   34.012894] Device 'mss' does not have a release() function, it is broken and must be fixed. See Docum.
[   34.024143] WARNING: CPU: 6 PID: 1600 at drivers/base/core.c:922 device_release+0x84/0x98
[   34.032379] Modules linked in:
[   34.035477] CPU: 6 PID: 1600 Comm: sh Not tainted 5.0.0-rc1-00012-g0513915837c5-dirty #34
[   34.043724] Hardware name: Qualcomm Technologies, Inc. SDM845 MTP (DT)
[   34.050305] pstate: 60400005 (nZCv daif +PAN -UAO)
[   34.055141] pc : device_release+0x84/0x98
[   34.059186] lr : device_release+0x84/0x98
[   34.063229] sp : ffff0000183c3b40
[   34.066574] x29: ffff0000183c3b40 x28: ffff800174b55100
[   34.071930] x27: 0000000000000000 x26: 0000000000000000
[   34.077285] x25: 0000000056000000 x24: 0000000000000015
[   34.082640] x23: dead000000000100 x22: dead000000000200
[   34.087997] x21: 0000000000000000 x20: ffff0000112f9780
[   34.093352] x19: ffff0000112f9770 x18: ffff0000111dd6c8
[   34.098719] x17: 0000000000000000 x16: 0000000000000000
[   34.104072] x15: ffff0000983c3867 x14: 6966206562207473
[   34.109438] x13: 756d20646e61206e x12: 656b6f7262207369
[   34.114791] x11: 207469202c6e6f69 x10: 74636e7566202928
[   34.120147] x9 : 657361656c657220 x8 : 742e7463656a626f
[   34.125514] x7 : 6b2f6e6f69746174 x6 : 0000000000000149
[   34.130867] x5 : 0000000000000020 x4 : 000000000001c200
[   34.136223] x3 : 00000000ffffffff x2 : ffff0000111f5828
[   34.141579] x1 : bc1d8ed5415ab200 x0 : 0000000000000000
[   34.146936] Call trace:
[   34.149413]  device_release+0x84/0x98
[   34.153116]  kobject_put+0x98/0xe8
[   34.156551]  put_device+0x14/0x28
[   34.159899]  genpd_remove+0x108/0x190
[   34.163598]  of_genpd_remove_last+0xbc/0xf0
[   34.167828]  rpmhpd_remove+0x2c/0x48
[   34.171436]  platform_drv_remove+0x24/0x68
[   34.175574]  device_release_driver_internal+0x184/0x218
[   34.180845]  device_release_driver+0x14/0x20
[   34.185152]  unbind_store+0xec/0x138
[   34.188760]  drv_attr_store+0x20/0x30
[   34.192465]  sysfs_kf_write+0x48/0x58
[   34.196163]  kernfs_fop_write+0xcc/0x1c8
[   34.200129]  __vfs_write+0x34/0x170
[   34.203654]  vfs_write+0xa8/0x1b8
[   34.206998]  ksys_write+0x5c/0xc8
[   34.210345]  __arm64_sys_write+0x14/0x20
[   34.214318]  el0_svc_common+0xb4/0x118
[   34.218107]  el0_svc_handler+0x2c/0x80
[   34.221899]  el0_svc+0x8/0xc
[   34.224804] ---[ end trace 9cee1458d0b6c175 ]---
[   34.229490] ------------[ cut here ]------------

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] soc: qcom: update config dependencies for QCOM_RPMPD
  2019-01-22 10:35           ` Rajendra Nayak
@ 2019-01-23 11:16             ` Viresh Kumar
  2019-01-23 14:22               ` Guenter Roeck
  0 siblings, 1 reply; 11+ messages in thread
From: Viresh Kumar @ 2019-01-23 11:16 UTC (permalink / raw)
  To: Rajendra Nayak
  Cc: Guenter Roeck, Stephen Boyd, andy.gross, david.brown,
	linux-arm-msm, linux-kernel, Ulf Hansson

On 22-01-19, 16:05, Rajendra Nayak wrote:
> Seeing this now during remove
> 
> [   34.008216] ------------[ cut here ]------------
> [   34.012894] Device 'mss' does not have a release() function, it is broken and must be fixed. See Docum.
> [   34.024143] WARNING: CPU: 6 PID: 1600 at drivers/base/core.c:922 device_release+0x84/0x98
> [   34.032379] Modules linked in:
> [   34.035477] CPU: 6 PID: 1600 Comm: sh Not tainted 5.0.0-rc1-00012-g0513915837c5-dirty #34
> [   34.043724] Hardware name: Qualcomm Technologies, Inc. SDM845 MTP (DT)
> [   34.050305] pstate: 60400005 (nZCv daif +PAN -UAO)
> [   34.055141] pc : device_release+0x84/0x98
> [   34.059186] lr : device_release+0x84/0x98
> [   34.063229] sp : ffff0000183c3b40
> [   34.066574] x29: ffff0000183c3b40 x28: ffff800174b55100
> [   34.071930] x27: 0000000000000000 x26: 0000000000000000
> [   34.077285] x25: 0000000056000000 x24: 0000000000000015
> [   34.082640] x23: dead000000000100 x22: dead000000000200
> [   34.087997] x21: 0000000000000000 x20: ffff0000112f9780
> [   34.093352] x19: ffff0000112f9770 x18: ffff0000111dd6c8
> [   34.098719] x17: 0000000000000000 x16: 0000000000000000
> [   34.104072] x15: ffff0000983c3867 x14: 6966206562207473
> [   34.109438] x13: 756d20646e61206e x12: 656b6f7262207369
> [   34.114791] x11: 207469202c6e6f69 x10: 74636e7566202928
> [   34.120147] x9 : 657361656c657220 x8 : 742e7463656a626f
> [   34.125514] x7 : 6b2f6e6f69746174 x6 : 0000000000000149
> [   34.130867] x5 : 0000000000000020 x4 : 000000000001c200
> [   34.136223] x3 : 00000000ffffffff x2 : ffff0000111f5828
> [   34.141579] x1 : bc1d8ed5415ab200 x0 : 0000000000000000
> [   34.146936] Call trace:
> [   34.149413]  device_release+0x84/0x98
> [   34.153116]  kobject_put+0x98/0xe8
> [   34.156551]  put_device+0x14/0x28
> [   34.159899]  genpd_remove+0x108/0x190
> [   34.163598]  of_genpd_remove_last+0xbc/0xf0
> [   34.167828]  rpmhpd_remove+0x2c/0x48
> [   34.171436]  platform_drv_remove+0x24/0x68
> [   34.175574]  device_release_driver_internal+0x184/0x218
> [   34.180845]  device_release_driver+0x14/0x20
> [   34.185152]  unbind_store+0xec/0x138
> [   34.188760]  drv_attr_store+0x20/0x30
> [   34.192465]  sysfs_kf_write+0x48/0x58
> [   34.196163]  kernfs_fop_write+0xcc/0x1c8
> [   34.200129]  __vfs_write+0x34/0x170
> [   34.203654]  vfs_write+0xa8/0x1b8
> [   34.206998]  ksys_write+0x5c/0xc8
> [   34.210345]  __arm64_sys_write+0x14/0x20
> [   34.214318]  el0_svc_common+0xb4/0x118
> [   34.218107]  el0_svc_handler+0x2c/0x80
> [   34.221899]  el0_svc+0x8/0xc
> [   34.224804] ---[ end trace 9cee1458d0b6c175 ]---
> [   34.229490] ------------[ cut here ]------------

This shall fix it:

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 500de1dee967..8a44b1f356d0 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1711,6 +1711,15 @@ static void genpd_lock_init(struct generic_pm_domain *genpd)
        }
 }
 
+static void genpd_device_release(struct device *dev)
+{
+}
+
+struct device_type genpd_device_type = {
+       .name =         "genpd_device",
+       .release =      genpd_device_release,
+};
+
 /**
  * pm_genpd_init - Initialize a generic I/O PM domain object.
  * @genpd: PM domain object to initialize.
@@ -1770,6 +1779,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
                pr_warn("%s : no governor for states\n", genpd->name);
        }
 
+       genpd->dev.type = &genpd_device_type;
        device_initialize(&genpd->dev);
        dev_set_name(&genpd->dev, "%s", genpd->name);
 

@Ulf: You fine with this ?

-- 
viresh

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] soc: qcom: update config dependencies for QCOM_RPMPD
  2019-01-23 11:16             ` Viresh Kumar
@ 2019-01-23 14:22               ` Guenter Roeck
  2019-01-24  6:45                 ` Viresh Kumar
  0 siblings, 1 reply; 11+ messages in thread
From: Guenter Roeck @ 2019-01-23 14:22 UTC (permalink / raw)
  To: Viresh Kumar, Rajendra Nayak
  Cc: Stephen Boyd, andy.gross, david.brown, linux-arm-msm,
	linux-kernel, Ulf Hansson

On 1/23/19 3:16 AM, Viresh Kumar wrote:
> On 22-01-19, 16:05, Rajendra Nayak wrote:
>> Seeing this now during remove
>>
>> [   34.008216] ------------[ cut here ]------------
>> [   34.012894] Device 'mss' does not have a release() function, it is broken and must be fixed. See Docum.
>> [   34.024143] WARNING: CPU: 6 PID: 1600 at drivers/base/core.c:922 device_release+0x84/0x98
>> [   34.032379] Modules linked in:
>> [   34.035477] CPU: 6 PID: 1600 Comm: sh Not tainted 5.0.0-rc1-00012-g0513915837c5-dirty #34
>> [   34.043724] Hardware name: Qualcomm Technologies, Inc. SDM845 MTP (DT)
>> [   34.050305] pstate: 60400005 (nZCv daif +PAN -UAO)
>> [   34.055141] pc : device_release+0x84/0x98
>> [   34.059186] lr : device_release+0x84/0x98
>> [   34.063229] sp : ffff0000183c3b40
>> [   34.066574] x29: ffff0000183c3b40 x28: ffff800174b55100
>> [   34.071930] x27: 0000000000000000 x26: 0000000000000000
>> [   34.077285] x25: 0000000056000000 x24: 0000000000000015
>> [   34.082640] x23: dead000000000100 x22: dead000000000200
>> [   34.087997] x21: 0000000000000000 x20: ffff0000112f9780
>> [   34.093352] x19: ffff0000112f9770 x18: ffff0000111dd6c8
>> [   34.098719] x17: 0000000000000000 x16: 0000000000000000
>> [   34.104072] x15: ffff0000983c3867 x14: 6966206562207473
>> [   34.109438] x13: 756d20646e61206e x12: 656b6f7262207369
>> [   34.114791] x11: 207469202c6e6f69 x10: 74636e7566202928
>> [   34.120147] x9 : 657361656c657220 x8 : 742e7463656a626f
>> [   34.125514] x7 : 6b2f6e6f69746174 x6 : 0000000000000149
>> [   34.130867] x5 : 0000000000000020 x4 : 000000000001c200
>> [   34.136223] x3 : 00000000ffffffff x2 : ffff0000111f5828
>> [   34.141579] x1 : bc1d8ed5415ab200 x0 : 0000000000000000
>> [   34.146936] Call trace:
>> [   34.149413]  device_release+0x84/0x98
>> [   34.153116]  kobject_put+0x98/0xe8
>> [   34.156551]  put_device+0x14/0x28
>> [   34.159899]  genpd_remove+0x108/0x190
>> [   34.163598]  of_genpd_remove_last+0xbc/0xf0
>> [   34.167828]  rpmhpd_remove+0x2c/0x48
>> [   34.171436]  platform_drv_remove+0x24/0x68
>> [   34.175574]  device_release_driver_internal+0x184/0x218
>> [   34.180845]  device_release_driver+0x14/0x20
>> [   34.185152]  unbind_store+0xec/0x138
>> [   34.188760]  drv_attr_store+0x20/0x30
>> [   34.192465]  sysfs_kf_write+0x48/0x58
>> [   34.196163]  kernfs_fop_write+0xcc/0x1c8
>> [   34.200129]  __vfs_write+0x34/0x170
>> [   34.203654]  vfs_write+0xa8/0x1b8
>> [   34.206998]  ksys_write+0x5c/0xc8
>> [   34.210345]  __arm64_sys_write+0x14/0x20
>> [   34.214318]  el0_svc_common+0xb4/0x118
>> [   34.218107]  el0_svc_handler+0x2c/0x80
>> [   34.221899]  el0_svc+0x8/0xc
>> [   34.224804] ---[ end trace 9cee1458d0b6c175 ]---
>> [   34.229490] ------------[ cut here ]------------
> 
> This shall fix it:
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 500de1dee967..8a44b1f356d0 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1711,6 +1711,15 @@ static void genpd_lock_init(struct generic_pm_domain *genpd)
>          }
>   }
>   
> +static void genpd_device_release(struct device *dev)
> +{
> +}
> +

See Documentation/kobject.txt.

"One important point cannot be overstated: every kobject must have a
release() method, and the kobject must persist (in a consistent state)
until that method is called. If these constraints are not met, the code is
flawed. Note that the kernel will warn you if you forget to provide a
release() method.  Do not try to get rid of this warning by providing an
"empty" release function.

If all your cleanup function needs to do is call kfree(), then you must
create a wrapper function which uses container_of() to upcast to the correct
type (as shown in the example above) and then calls kfree() on the overall
structure."

Guenter

> +struct device_type genpd_device_type = {
> +       .name =         "genpd_device",
> +       .release =      genpd_device_release,
> +};
> +
>   /**
>    * pm_genpd_init - Initialize a generic I/O PM domain object.
>    * @genpd: PM domain object to initialize.
> @@ -1770,6 +1779,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>                  pr_warn("%s : no governor for states\n", genpd->name);
>          }
>   
> +       genpd->dev.type = &genpd_device_type;
>          device_initialize(&genpd->dev);
>          dev_set_name(&genpd->dev, "%s", genpd->name);
>   
> 
> @Ulf: You fine with this ?
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] soc: qcom: update config dependencies for QCOM_RPMPD
  2019-01-23 14:22               ` Guenter Roeck
@ 2019-01-24  6:45                 ` Viresh Kumar
  0 siblings, 0 replies; 11+ messages in thread
From: Viresh Kumar @ 2019-01-24  6:45 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Rajendra Nayak, Stephen Boyd, andy.gross, david.brown,
	linux-arm-msm, linux-kernel, Ulf Hansson

On 23-01-19, 06:22, Guenter Roeck wrote:
> See Documentation/kobject.txt.
> 
> "One important point cannot be overstated: every kobject must have a
> release() method, and the kobject must persist (in a consistent state)
> until that method is called. If these constraints are not met, the code is
> flawed. Note that the kernel will warn you if you forget to provide a
> release() method.  Do not try to get rid of this warning by providing an
> "empty" release function.
> 
> If all your cleanup function needs to do is call kfree(), then you must
> create a wrapper function which uses container_of() to upcast to the correct
> type (as shown in the example above) and then calls kfree() on the overall
> structure."

Guenter, you are of course correct but I am not sure what else we can
do here. In the most common case device & kobject are allocated along
with the real entity, like struct genpd here, and we free those from
the release routine (i.e. kfree(genpd)). But in our case most of the
times the genpd is created statically by platform drivers and not by
the genpd core, and so we can't free it from release().

Over that the genpd->dev is only used by the OPP core currently and
the device isn't even registered with the device core.

-- 
viresh

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] soc: qcom: update config dependencies for QCOM_RPMPD
  2019-01-18 17:39 ` Stephen Boyd
  2019-01-22  2:30   ` Rajendra Nayak
@ 2019-01-31  1:32   ` Bjorn Andersson
  1 sibling, 0 replies; 11+ messages in thread
From: Bjorn Andersson @ 2019-01-31  1:32 UTC (permalink / raw)
  To: Stephen Boyd, Rob Herring, Alexander Graf
  Cc: Rajendra Nayak, andy.gross, david.brown, linux-arm-msm,
	linux-kernel, linux

On Fri 18 Jan 09:39 PST 2019, Stephen Boyd wrote:

> Quoting Rajendra Nayak (2019-01-17 20:48:01)
> >  drivers/soc/qcom/Kconfig | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> > index a5d5167c3f16..1ee298f6bf17 100644
> > --- a/drivers/soc/qcom/Kconfig
> > +++ b/drivers/soc/qcom/Kconfig
> > @@ -109,7 +109,7 @@ config QCOM_RPMHPD
> >  
> >  config QCOM_RPMPD
> >         bool "Qualcomm RPM Power domain driver"
> 
> Just curious, does it need to be bool for some reason?
> 

It's unfortunately not possible to have any genpd, iommu or pinctrl
drivers compiled as modules, because once you pass lateinit probe
deferral is purposefully broken. See
driver_deferred_probe_check_state().

This also means that if you're unlucky and your kernel reached lateinit
before the SMD communication with RPM is established and has brought up
the rpmpd, you are left with a completely broken system. Unfortunately
this isn't that hard to reproduce with a minimal defconfig.

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2019-01-31  1:33 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-18  4:48 [PATCH] soc: qcom: update config dependencies for QCOM_RPMPD Rajendra Nayak
2019-01-18 17:39 ` Stephen Boyd
2019-01-22  2:30   ` Rajendra Nayak
2019-01-22  5:08     ` Guenter Roeck
2019-01-22  9:54       ` Rajendra Nayak
2019-01-22 10:08         ` Viresh Kumar
2019-01-22 10:35           ` Rajendra Nayak
2019-01-23 11:16             ` Viresh Kumar
2019-01-23 14:22               ` Guenter Roeck
2019-01-24  6:45                 ` Viresh Kumar
2019-01-31  1:32   ` Bjorn Andersson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).