[PATCHv2 1/2] pci: spear: Use platform_driver_probe instead of platform_driver_register

[PATCHv2 1/2] pci: spear: Use platform_driver_probe instead of platform_driver_register

[Matwey V. Kornilov]

Use platform_driver_probe instead of platform_driver_register
because the former allows us to use probe function placed into __init section
and the driver itself is not support hotplugging (yet?).

Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
---
Changes from v1:
 - Use platform_driver_probe instead of platform_driver_register to make linker happy.

 drivers/pci/host/pcie-spear13xx.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/pci/host/pcie-spear13xx.c b/drivers/pci/host/pcie-spear13xx.c
index 866465f..51e1344 100644
--- a/drivers/pci/host/pcie-spear13xx.c
+++ b/drivers/pci/host/pcie-spear13xx.c
@@ -371,7 +371,6 @@ static const struct of_device_id spear13xx_pcie_of_match[] = {
 MODULE_DEVICE_TABLE(of, spear13xx_pcie_of_match);
 
 static struct platform_driver spear13xx_pcie_driver __initdata = {
-	.probe		= spear13xx_pcie_probe,
 	.driver = {
 		.name	= "spear-pcie",
 		.of_match_table = of_match_ptr(spear13xx_pcie_of_match),
@@ -382,7 +381,7 @@ static struct platform_driver spear13xx_pcie_driver __initdata = {
 
 static int __init spear13xx_pcie_init(void)
 {
-	return platform_driver_register(&spear13xx_pcie_driver);
+	return platform_driver_probe(&spear13xx_pcie_driver, spear13xx_pcie_probe);
 }
 module_init(spear13xx_pcie_init);
 
-- 
2.1.4

[PATCHv2 2/2] pci: spear: Drop __initdata from struct platform_driver spear13xx_pcie_driver

[Matwey V. Kornilov]

spear13xx_pcie_driver.driver is allocated in text.init section
and then the pointer to it is passed futher. This patch is to avoid
crashes like the following, when freed memory is used:

 #0  __device_attach (drv=0xc0ed5608 <spear13xx_pcie_driver+20>, data=0xdb622610) at ../drivers/base/dd.c:409
 #1  0xc07a4798 in bus_for_each_drv (bus=<optimized out>, start=<optimized out>, data=0xda0, fn=0xc07a6740 <__device_attach>)
    at ../drivers/base/bus.c:463
 #2  0xc07a6324 in device_attach (dev=0xdb622610) at ../drivers/base/dd.c:447
 #3  0xc07a5814 in bus_probe_device (dev=0xdb622610) at ../drivers/base/bus.c:558
 #4  0xc07a38d8 in device_add (dev=<optimized out>) at ../drivers/base/core.c:1058
 #5  0xc08b6a5c in of_device_add (ofdev=<optimized out>) at ../drivers/of/device.c:66
 #6  0xc08b742c in of_platform_device_create_pdata (np=<optimized out>, bus_id=0x0 <__vectors_start>, platform_data=0x0 <__vectors_start>,
    parent=<optimized out>) at ../drivers/of/platform.c:241
 #7  0xc08b7568 in of_platform_bus_create (bus=0xdfa46780, matches=0x0 <__vectors_start>, lookup=0x0 <__vectors_start>, parent=0xdb183410,
    strict=true) at ../drivers/of/platform.c:414
 #8  0xc08b79bc in of_platform_populate (root=0xc0ed5608 <spear13xx_pcie_driver+20>, matches=0xdb622610, lookup=0xda0,
    parent=0xc07a6740 <__device_attach>) at ../drivers/of/platform.c:501
 #9  0xbf000030 in am335x_child_probe (pdev=0xdb183400) at ../drivers/usb/musb/musb_am335x.c:12
 #10 0xc07a83f0 in platform_drv_probe (_dev=0xdb183410) at ../drivers/base/platform.c:512
 #11 0xc07a64e8 in really_probe (drv=<optimized out>, dev=<optimized out>) at ../drivers/base/dd.c:302
 #12 driver_probe_device (drv=0xbf000234, dev=0xdb183410) at ../drivers/base/dd.c:399
 #13 0xc07a6820 in __driver_attach (dev=0xdb183410, data=0xbf000234) at ../drivers/base/dd.c:477
 #14 0xc07a46e8 in bus_for_each_dev (bus=<optimized out>, start=<optimized out>, data=0xda0, fn=0xc07a83a4 <platform_drv_probe>)
    at ../drivers/base/bus.c:313
 #15 0xc07a5ebc in driver_attach (drv=<optimized out>) at ../drivers/base/dd.c:496
 #16 0xc07a5af0 in bus_add_driver (drv=0xbf000234) at ../drivers/base/bus.c:694
 #17 0xc07a6fec in driver_register (drv=0xbf000234) at ../drivers/base/driver.c:167
 #18 0xc0209c34 in do_one_initcall (fn=0xbf002000) at ../init/main.c:801
 #19 0xc02e0494 in do_init_module (mod=<optimized out>) at ../kernel/module.c:3142
 #20 load_module (info=0xdb6b1f54, uargs=<optimized out>, flags=<optimized out>) at ../kernel/module.c:3461
 #21 0xc02e0a44 in SYSC_finit_module (flags=<optimized out>, uargs=<optimized out>, fd=<optimized out>) at ../kernel/module.c:3537
 #22 SyS_finit_module (fd=7, uargs=-1225602132, flags=0) at ../kernel/module.c:3518
 #23 0xc021a680 in ?? ()

Fixes: 6675ef212da (PCI: spear: Fix Section mismatch compilation warning for probe())
Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
---
 drivers/pci/host/pcie-spear13xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/host/pcie-spear13xx.c b/drivers/pci/host/pcie-spear13xx.c
index 51e1344..b94d4fb 100644
--- a/drivers/pci/host/pcie-spear13xx.c
+++ b/drivers/pci/host/pcie-spear13xx.c
@@ -370,7 +370,7 @@ static const struct of_device_id spear13xx_pcie_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, spear13xx_pcie_of_match);
 
-static struct platform_driver spear13xx_pcie_driver __initdata = {
+static struct platform_driver spear13xx_pcie_driver = {
 	.driver = {
 		.name	= "spear-pcie",
 		.of_match_table = of_match_ptr(spear13xx_pcie_of_match),
-- 
2.1.4

Re: [PATCHv2 1/2] pci: spear: Use platform_driver_probe instead of platform_driver_register

[Viresh Kumar]

On 11 February 2015 at 00:31, Matwey V. Kornilov <matwey@sai.msu.ru> wrote:
> Use platform_driver_probe instead of platform_driver_register
> because the former allows us to use probe function placed into __init section
> and the driver itself is not support hotplugging (yet?).
>
> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
> ---
> Changes from v1:
>  - Use platform_driver_probe instead of platform_driver_register to make linker happy.
>
>  drivers/pci/host/pcie-spear13xx.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-spear13xx.c b/drivers/pci/host/pcie-spear13xx.c
> index 866465f..51e1344 100644
> --- a/drivers/pci/host/pcie-spear13xx.c
> +++ b/drivers/pci/host/pcie-spear13xx.c
> @@ -371,7 +371,6 @@ static const struct of_device_id spear13xx_pcie_of_match[] = {
>  MODULE_DEVICE_TABLE(of, spear13xx_pcie_of_match);
>
>  static struct platform_driver spear13xx_pcie_driver __initdata = {
> -       .probe          = spear13xx_pcie_probe,
>         .driver = {
>                 .name   = "spear-pcie",
>                 .of_match_table = of_match_ptr(spear13xx_pcie_of_match),
> @@ -382,7 +381,7 @@ static struct platform_driver spear13xx_pcie_driver __initdata = {
>
>  static int __init spear13xx_pcie_init(void)
>  {
> -       return platform_driver_register(&spear13xx_pcie_driver);
> +       return platform_driver_probe(&spear13xx_pcie_driver, spear13xx_pcie_probe);
>  }
>  module_init(spear13xx_pcie_init);

The problem here is that the driver is never registered. And we might never
see it in sysfs. Probably a better solution would be to just remove all __init
parts from this driver. Let it take space even after it is used. And don't care
about it.

Sorry was busy in a conference and couldn't reply to your earlier mail on this.

Re: [PATCHv2 1/2] pci: spear: Use platform_driver_probe instead of platform_driver_register

[Stanimir Varbanov]

Hi,

On 02/11/2015 05:35 AM, Viresh Kumar wrote:
> On 11 February 2015 at 00:31, Matwey V. Kornilov <matwey@sai.msu.ru> wrote:
>> Use platform_driver_probe instead of platform_driver_register
>> because the former allows us to use probe function placed into __init section
>> and the driver itself is not support hotplugging (yet?).
>>
>> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
>> ---
>> Changes from v1:
>>   - Use platform_driver_probe instead of platform_driver_register to make linker happy.
>>
>>   drivers/pci/host/pcie-spear13xx.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/host/pcie-spear13xx.c b/drivers/pci/host/pcie-spear13xx.c
>> index 866465f..51e1344 100644
>> --- a/drivers/pci/host/pcie-spear13xx.c
>> +++ b/drivers/pci/host/pcie-spear13xx.c
>> @@ -371,7 +371,6 @@ static const struct of_device_id spear13xx_pcie_of_match[] = {
>>   MODULE_DEVICE_TABLE(of, spear13xx_pcie_of_match);
>>
>>   static struct platform_driver spear13xx_pcie_driver __initdata = {
>> -       .probe          = spear13xx_pcie_probe,
>>          .driver = {
>>                  .name   = "spear-pcie",
>>                  .of_match_table = of_match_ptr(spear13xx_pcie_of_match),
>> @@ -382,7 +381,7 @@ static struct platform_driver spear13xx_pcie_driver __initdata = {
>>
>>   static int __init spear13xx_pcie_init(void)
>>   {
>> -       return platform_driver_register(&spear13xx_pcie_driver);
>> +       return platform_driver_probe(&spear13xx_pcie_driver, spear13xx_pcie_probe);
>>   }
>>   module_init(spear13xx_pcie_init);
>
> The problem here is that the driver is never registered. And we might never
> see it in sysfs. Probably a better solution would be to just remove all __init
> parts from this driver. Let it take space even after it is used. And don't care
> about it.
>
> Sorry was busy in a conference and couldn't reply to your earlier mail on this.

Using platform_driver_probe() prevents deferred probe which is problem 
because you get phy, clocks from probe, and these calls could return 
EPROBE_DEFFER.

Also dw_pcie_host_init() called from spear13xx_add_pcie_port() is 
annotated as __init. I wanted send a patch which removes __init from 
dw_pcie_host_init() but I haven't time, yet.

regards,
Stan

Re: [PATCHv2 1/2] pci: spear: Use platform_driver_probe instead of platform_driver_register

[Stanimir Varbanov]

please don't top posting.

On 02/11/2015 10:38 AM, Matwey V. Kornilov wrote:
> It sounds reasonable, but does current implementation support deferring?

yes, it uses deferred probe see devm_phy_get() and devm_clk_get() calls 
in .probe and how the returned values are checked.

> Struct  spear13xx_pcie_driver just dissapears from memory after init.

IMO this is expected because spear13xx_pcie_driver is annotated as 
__initdata.

The proper solution was pointed by Kumar below, but we also need to 
remove the __init from dw_pcie_host_init() which is called from driver 
.probe.

> 11.02.2015 11:18 пользователь "Stanimir Varbanov" <svarbanov@mm-sol.com>
> написал:
>
>> Hi,
>>
>> On 02/11/2015 05:35 AM, Viresh Kumar wrote:
>>
>>> On 11 February 2015 at 00:31, Matwey V. Kornilov <matwey@sai.msu.ru>
>>> wrote:
>>>
>>>> Use platform_driver_probe instead of platform_driver_register
>>>> because the former allows us to use probe function placed into __init
>>>> section
>>>> and the driver itself is not support hotplugging (yet?).
>>>>
>>>> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
>>>> ---
>>>> Changes from v1:
>>>>    - Use platform_driver_probe instead of platform_driver_register to
>>>> make linker happy.ven after it is used
>>>>
>>>>    drivers/pci/host/pcie-spear13xx.c | 3 +--
>>>>    1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/host/pcie-spear13xx.c b/drivers/pci/host/pcie-
>>>> spear13xx.c
>>>> index 866465f..51e1344 100644
>>>> --- a/drivers/pci/host/pcie-spear13xx.c
>>>> +++ b/drivers/pci/host/pcie-spear13xx.c
>>>> @@ -371,7 +371,6 @@ static const struct of_device_id
>>>> spear13xx_pcie_of_match[] = {
>>>>    MODULE_DEVICE_TABLE(of, spear13xx_pcie_of_match);
>>>>
>>>>    static struct platform_driver spear13xx_pcie_driver __initdata = {
>>>> -       .probe          = spear13xx_pcie_probe,
>>>>           .driver = {
>>>>                   .name   = "spear-pcie",
>>>>                   .of_match_table = of_match_ptr(spear13xx_pcie_
>>>> of_match),
>>>> @@ -382,7 +381,7 @@ static struct platform_driver spear13xx_pcie_driver
>>>> __initdata = {
>>>>
>>>>    static int __init spear13xx_pcie_init(void)
>>>>    {
>>>> -       return platform_driver_register(&spear13xx_pcie_driver);
>>>> +       return platform_driver_probe(&spear13xx_pcie_driver,
>>>> spear13xx_pcie_probe);
>>>>    }
>>>>    module_init(spear13xx_pcie_init);
>>>>
>>>
>>> The problem here is that the driver is never registered. And we might
>>> never
>>> see it in sysfs. Probably a better solution would be to just remove all
>>> __init
>>> parts from this driver. Let it take space even after it is used. And
>>> don't care
>>> about it.
>>>
>>> Sorry was busy in a conference and couldn't reply to your earlier mail on
>>> this.
>>>
>>
>> Using platform_driver_probe() prevents deferred probe which is problem
>> because you get phy, clocks from probe, and these calls could return
>> EPROBE_DEFFER.
>>
>> Also dw_pcie_host_init() called from spear13xx_add_pcie_port() is
>> annotated as __init. I wanted send a patch which removes __init from
>> dw_pcie_host_init() but I haven't time, yet.
>>
>> regards,
>> Stan
>>
>>
>

Re: [PATCHv2 1/2] pci: spear: Use platform_driver_probe instead of platform_driver_register

[Matwey V. Kornilov]

2015-02-11 12:19 GMT+03:00 Stanimir Varbanov <svarbanov@mm-sol.com>:
> please don't top posting.
>
> On 02/11/2015 10:38 AM, Matwey V. Kornilov wrote:
>>
>> It sounds reasonable, but does current implementation support deferring?
>
>
> yes, it uses deferred probe see devm_phy_get() and devm_clk_get() calls in
> .probe and how the returned values are checked.
>
>> Struct  spear13xx_pcie_driver just dissapears from memory after init.
>
>
> IMO this is expected because spear13xx_pcie_driver is annotated as
> __initdata.

The question was how deferred probe had been expected to work? When
probe has been deferred at first attempt, then second attempt
performed after init section, so proble() function must be available,
but it is not. Isn't it?

>
> The proper solution was pointed by Kumar below, but we also need to remove
> the __init from dw_pcie_host_init() which is called from driver .probe.
>
>> 11.02.2015 11:18 пользователь "Stanimir Varbanov" <svarbanov@mm-sol.com>
>> написал:
>>
>>> Hi,
>>>
>>> On 02/11/2015 05:35 AM, Viresh Kumar wrote:
>>>
>>>> On 11 February 2015 at 00:31, Matwey V. Kornilov <matwey@sai.msu.ru>
>>>> wrote:
>>>>
>>>>> Use platform_driver_probe instead of platform_driver_register
>>>>> because the former allows us to use probe function placed into __init
>>>>> section
>>>>> and the driver itself is not support hotplugging (yet?).
>>>>>
>>>>> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
>>>>> ---
>>>>> Changes from v1:
>>>>>    - Use platform_driver_probe instead of platform_driver_register to
>>>>> make linker happy.ven after it is used
>>>>>
>>>>>
>>>>>    drivers/pci/host/pcie-spear13xx.c | 3 +--
>>>>>    1 file changed, 1 insertion(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pci/host/pcie-spear13xx.c b/drivers/pci/host/pcie-
>>>>> spear13xx.c
>>>>> index 866465f..51e1344 100644
>>>>> --- a/drivers/pci/host/pcie-spear13xx.c
>>>>> +++ b/drivers/pci/host/pcie-spear13xx.c
>>>>> @@ -371,7 +371,6 @@ static const struct of_device_id
>>>>> spear13xx_pcie_of_match[] = {
>>>>>    MODULE_DEVICE_TABLE(of, spear13xx_pcie_of_match);
>>>>>
>>>>>    static struct platform_driver spear13xx_pcie_driver __initdata = {
>>>>> -       .probe          = spear13xx_pcie_probe,
>>>>>           .driver = {
>>>>>                   .name   = "spear-pcie",
>>>>>                   .of_match_table = of_match_ptr(spear13xx_pcie_
>>>>> of_match),
>>>>> @@ -382,7 +381,7 @@ static struct platform_driver spear13xx_pcie_driver
>>>>> __initdata = {
>>>>>
>>>>>    static int __init spear13xx_pcie_init(void)
>>>>>    {
>>>>> -       return platform_driver_register(&spear13xx_pcie_driver);
>>>>> +       return platform_driver_probe(&spear13xx_pcie_driver,
>>>>> spear13xx_pcie_probe);
>>>>>    }
>>>>>    module_init(spear13xx_pcie_init);
>>>>>
>>>>
>>>> The problem here is that the driver is never registered. And we might
>>>> never
>>>> see it in sysfs. Probably a better solution would be to just remove all
>>>> __init
>>>> parts from this driver. Let it take space even after it is used. And
>>>> don't care
>>>> about it.
>>>>
>>>> Sorry was busy in a conference and couldn't reply to your earlier mail
>>>> on
>>>> this.
>>>>
>>>
>>> Using platform_driver_probe() prevents deferred probe which is problem
>>> because you get phy, clocks from probe, and these calls could return
>>> EPROBE_DEFFER.
>>>
>>> Also dw_pcie_host_init() called from spear13xx_add_pcie_port() is
>>> annotated as __init. I wanted send a patch which removes __init from
>>> dw_pcie_host_init() but I haven't time, yet.
>>>
>>> regards,
>>> Stan
>>>
>>>
>>
>



-- 
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia

Re: [PATCHv2 1/2] pci: spear: Use platform_driver_probe instead of platform_driver_register

[Stanimir Varbanov]

Hi,

On 02/12/2015 11:07 AM, Matwey V. Kornilov wrote:
> 2015-02-11 12:19 GMT+03:00 Stanimir Varbanov <svarbanov@mm-sol.com>:
>> please don't top posting.
>>
>> On 02/11/2015 10:38 AM, Matwey V. Kornilov wrote:HI
>>
>>> It sounds reasonable, but does current implementation support deferring?
>>
>>thor
>> yes, it uses deferred probe see devm_phy_get() and devm_clk_get() calls in
>> .probe and how the returned values are checked.
>>
>>> Struct  spear13xx_pcie_driver just dissapears from memory after init.
>>
>>
>> IMO this is expected because spear13xx_pcie_driver is annotated as
>> __initdata.
> 
> The question was how deferred probe had been expected to work? When
> probe has been deferred at first attempt, then second attempt
> performed after init section, so proble() function must be available,at
> but it is not. Isn't it?
> 

That is questionable, maybe it works by chance?.


regards,
Stan

Re: [PATCHv2 1/2] pci: spear: Use platform_driver_probe instead of platform_driver_register

[Matwey V. Kornilov]

2015-02-13 5:08 GMT+03:00 Stanimir Varbanov <svarbanov@mm-sol.com>:
> Hi,
>
> On 02/12/2015 11:07 AM, Matwey V. Kornilov wrote:
>> 2015-02-11 12:19 GMT+03:00 Stanimir Varbanov <svarbanov@mm-sol.com>:
>>> please don't top posting.
>>>
>>> On 02/11/2015 10:38 AM, Matwey V. Kornilov wrote:HI
>>>
>>>> It sounds reasonable, but does current implementation support deferring?
>>>
>>>thor
>>> yes, it uses deferred probe see devm_phy_get() and devm_clk_get() calls in
>>> .probe and how the returned values are checked.
>>>
>>>> Struct  spear13xx_pcie_driver just dissapears from memory after init.
>>>
>>>
>>> IMO this is expected because spear13xx_pcie_driver is annotated as
>>> __initdata.
>>
>> The question was how deferred probe had been expected to work? When
>> probe has been deferred at first attempt, then second attempt
>> performed after init section, so proble() function must be available,at
>> but it is not. Isn't it?
>>
>
> That is questionable, maybe it works by chance?.

Or maybe it doesn't work at all.


-- 
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia

Re: [PATCHv2 1/2] pci: spear: Use platform_driver_probe instead of platform_driver_register

[Viresh Kumar]

On 15 February 2015 at 21:16, Matwey V. Kornilov <matwey@sai.msu.ru> wrote:
> Or maybe it doesn't work at all.

It wouldn't have as that wasn't required earlier. But when we are fixing it, we
fix it proper.