Patchwork [10/10] drivers: misc: use module_platform_driver_probe()

login
register
mail settings
Submitter Fabio Porcedda
Date March 14, 2013, 1:11 p.m.
Message ID <1363266691-15757-12-git-send-email-fabio.porcedda@gmail.com>
Download mbox | patch
Permalink /patch/227655/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Fabio Porcedda - March 14, 2013, 1:11 p.m.
This patch converts the drivers to use the
module_platform_driver_probe() macro which makes the code smaller and
a bit simpler.

Signed-off-by: Fabio Porcedda <fabio.porcedda@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Arnd Bergmann <arnd@arndb.de>
---
 drivers/misc/atmel_pwm.c  | 12 +-----------
 drivers/misc/ep93xx_pwm.c | 13 +------------
 2 files changed, 2 insertions(+), 23 deletions(-)
Arnd Bergmann - March 14, 2013, 1:58 p.m.
On Thursday 14 March 2013, Fabio Porcedda wrote:
> This patch converts the drivers to use the
> module_platform_driver_probe() macro which makes the code smaller and
> a bit simpler.
> 
> Signed-off-by: Fabio Porcedda <fabio.porcedda@gmail.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/misc/atmel_pwm.c  | 12 +-----------
>  drivers/misc/ep93xx_pwm.c | 13 +------------
>  2 files changed, 2 insertions(+), 23 deletions(-)

The patch itself seems fine, but there are two issues around it:

* The PWM drivers should really get moved to drivers/pwm and converted to the new
  PWM subsystem. I don't know if Hartley or Hans-Christian have plans to do
  that already.

* Regarding the use of module_platform_driver_probe, I'm a little worried about
  the interactions with deferred probing. I don't think there are any regressions,
  but we should probably make people aware that one cannot return -EPROBE_DEFER
  from a platform_driver_probe function.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sascha Hauer - March 14, 2013, 2:06 p.m.
On Thu, Mar 14, 2013 at 01:58:05PM +0000, Arnd Bergmann wrote:
> On Thursday 14 March 2013, Fabio Porcedda wrote:
> > This patch converts the drivers to use the
> > module_platform_driver_probe() macro which makes the code smaller and
> > a bit simpler.
> > 
> > Signed-off-by: Fabio Porcedda <fabio.porcedda@gmail.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > ---
> >  drivers/misc/atmel_pwm.c  | 12 +-----------
> >  drivers/misc/ep93xx_pwm.c | 13 +------------
> >  2 files changed, 2 insertions(+), 23 deletions(-)
> 
> The patch itself seems fine, but there are two issues around it:
> 
> * The PWM drivers should really get moved to drivers/pwm and converted to the new
>   PWM subsystem. I don't know if Hartley or Hans-Christian have plans to do
>   that already.
> 
> * Regarding the use of module_platform_driver_probe, I'm a little worried about
>   the interactions with deferred probing. I don't think there are any regressions,
>   but we should probably make people aware that one cannot return -EPROBE_DEFER
>   from a platform_driver_probe function.

I'm worried about this aswell. I think platform_driver_probe shouldn't
be used anymore. Even if a driver does not explicitly make use of
-EPROBE_DEFER, it leaks in very quickly if a driver for example uses a
regulator and just returns the error value from regulator_get.

Sascha
Fabio Porcedda - March 15, 2013, 11:18 a.m.
On Thu, Mar 14, 2013 at 3:06 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Thu, Mar 14, 2013 at 01:58:05PM +0000, Arnd Bergmann wrote:
>> On Thursday 14 March 2013, Fabio Porcedda wrote:
>> > This patch converts the drivers to use the
>> > module_platform_driver_probe() macro which makes the code smaller and
>> > a bit simpler.
>> >
>> > Signed-off-by: Fabio Porcedda <fabio.porcedda@gmail.com>
>> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> > Cc: Arnd Bergmann <arnd@arndb.de>
>> > ---
>> >  drivers/misc/atmel_pwm.c  | 12 +-----------
>> >  drivers/misc/ep93xx_pwm.c | 13 +------------
>> >  2 files changed, 2 insertions(+), 23 deletions(-)
>>
>> The patch itself seems fine, but there are two issues around it:
>>
>> * The PWM drivers should really get moved to drivers/pwm and converted to the new
>>   PWM subsystem. I don't know if Hartley or Hans-Christian have plans to do
>>   that already.
>>
>> * Regarding the use of module_platform_driver_probe, I'm a little worried about
>>   the interactions with deferred probing. I don't think there are any regressions,
>>   but we should probably make people aware that one cannot return -EPROBE_DEFER
>>   from a platform_driver_probe function.

The use of module_platform_driver_probe() doesn't change anything about that,
it's exactly the same thing as using "return platform_driver_probe()".
I'm right or I'm missing something? Maybe are you just speaking about
the misuse of "platform_driver_probe"?

Best regards
Fabio Porcedda

>
> I'm worried about this aswell. I think platform_driver_probe shouldn't
> be used anymore. Even if a driver does not explicitly make use of
> -EPROBE_DEFER, it leaks in very quickly if a driver for example uses a
> regulator and just returns the error value from regulator_get.

> Sascha
>
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann - March 15, 2013, 11:28 a.m.
On Friday 15 March 2013, Fabio Porcedda wrote:
> >> * Regarding the use of module_platform_driver_probe, I'm a little worried about
> >>   the interactions with deferred probing. I don't think there are any regressions,
> >>   but we should probably make people aware that one cannot return -EPROBE_DEFER
> >>   from a platform_driver_probe function.
> 
> The use of module_platform_driver_probe() doesn't change anything about that,
> it's exactly the same thing as using "return platform_driver_probe()".
> I'm right or I'm missing something? Maybe are you just speaking about
> the misuse of "platform_driver_probe"?

Yes, that was what I meant. The point is that if we need to review or remove
all uses of platform_driver_probe, it would be better not to introduce a
module_platform_driver_probe() interface to make it easier to use.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
hartleys - March 15, 2013, 5:43 p.m.
On Thursday, March 14, 2013 6:58 AM, Arnd Bergmann wrote:
> On Thursday 14 March 2013, Fabio Porcedda wrote:
>> This patch converts the drivers to use the
>> module_platform_driver_probe() macro which makes the code smaller and
>> a bit simpler.
>> 
>> Signed-off-by: Fabio Porcedda <fabio.porcedda@gmail.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> ---
>>  drivers/misc/atmel_pwm.c  | 12 +-----------
>>  drivers/misc/ep93xx_pwm.c | 13 +------------
>>  2 files changed, 2 insertions(+), 23 deletions(-)
>
> The patch itself seems fine, but there are two issues around it:
>
> * The PWM drivers should really get moved to drivers/pwm and converted to the new
>   PWM subsystem. I don't know if Hartley or Hans-Christian have plans to do
>   that already.

Arnd,

Ill look at converting the ep93xx pwm driver to the PWM subsystem. The only issue is
the current driver exposes a sysfs interface that I think is not available in that subsystem.

>* Regarding the use of module_platform_driver_probe, I'm a little worried about
>  the interactions with deferred probing. I don't think there are any regressions,
>  but we should probably make people aware that one cannot return -EPROBE_DEFER
>  from a platform_driver_probe function.

The ep93xx pwm driver does not need to use platform_driver_probe(). It can be changed
to use module_platform_driver() by just moving the .probe to the platform_driver. This
driver was added before module_platform_driver() was available and I used the
platform_driver_probe() thinking it would save a couple lines of code.

I'll change this in a bit. Right now I'm trying to work out why kernel 3.8 is not booting
on the ep93xx. I had 3.6.6 on my development board and 3.7 works fine but 3.8 hangs
without uncompressing the kernel.

Regards,
Hartley



--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Fabio Porcedda - March 15, 2013, 6:09 p.m.
On Fri, Mar 15, 2013 at 12:28 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 15 March 2013, Fabio Porcedda wrote:
>> >> * Regarding the use of module_platform_driver_probe, I'm a little worried about
>> >>   the interactions with deferred probing. I don't think there are any regressions,
>> >>   but we should probably make people aware that one cannot return -EPROBE_DEFER
>> >>   from a platform_driver_probe function.
>>
>> The use of module_platform_driver_probe() doesn't change anything about that,
>> it's exactly the same thing as using "return platform_driver_probe()".
>> I'm right or I'm missing something? Maybe are you just speaking about
>> the misuse of "platform_driver_probe"?
>
> Yes, that was what I meant. The point is that if we need to review or remove
> all uses of platform_driver_probe, it would be better not to introduce a
> module_platform_driver_probe() interface to make it easier to use.

Just to let you know, the module_platform_driver_probe() macro is
already in v3.9-rc1 and is already used by some drivers.
In linux-next there are already many patches that use that macro.

Best regards
--
Fabio Porcedda
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann - March 15, 2013, 8:18 p.m.
On Friday 15 March 2013, H Hartley Sweeten wrote:
> Arnd,
> 
> Ill look at converting the ep93xx pwm driver to the PWM subsystem. The only issue is
> the current driver exposes a sysfs interface that I think is not available in that subsystem.

You can probably keep providing that interface if you have active users.

> >* Regarding the use of module_platform_driver_probe, I'm a little worried about
> >  the interactions with deferred probing. I don't think there are any regressions,
> >  but we should probably make people aware that one cannot return -EPROBE_DEFER
> >  from a platform_driver_probe function.
> 
> The ep93xx pwm driver does not need to use platform_driver_probe(). It can be changed
> to use module_platform_driver() by just moving the .probe to the platform_driver. This
> driver was added before module_platform_driver() was available and I used the
> platform_driver_probe() thinking it would save a couple lines of code.
> 
> I'll change this in a bit. Right now I'm trying to work out why kernel 3.8 is not booting
> on the ep93xx. I had 3.6.6 on my development board and 3.7 works fine but 3.8 hangs
> without uncompressing the kernel.

Ok, thanks!

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Fabio Porcedda - March 18, 2013, 10:03 a.m.
On Fri, Mar 15, 2013 at 9:18 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 15 March 2013, H Hartley Sweeten wrote:
>> Arnd,
>>
>> Ill look at converting the ep93xx pwm driver to the PWM subsystem. The only issue is
>> the current driver exposes a sysfs interface that I think is not available in that subsystem.
>
> You can probably keep providing that interface if you have active users.
>
>> >* Regarding the use of module_platform_driver_probe, I'm a little worried about
>> >  the interactions with deferred probing. I don't think there are any regressions,
>> >  but we should probably make people aware that one cannot return -EPROBE_DEFER
>> >  from a platform_driver_probe function.
>>
>> The ep93xx pwm driver does not need to use platform_driver_probe(). It can be changed
>> to use module_platform_driver() by just moving the .probe to the platform_driver. This
>> driver was added before module_platform_driver() was available and I used the
>> platform_driver_probe() thinking it would save a couple lines of code.

Since by using platform_driver_probe() the  function
ep93xx_pwm_probe() is freed after initialization,
is better to use module_platform_drive_probe().
IMHO i don't see any good reason to use module_platform_driver() for
this driver.

Best regards
Fabio Porcedda

>> I'll change this in a bit. Right now I'm trying to work out why kernel 3.8 is not booting
>> on the ep93xx. I had 3.6.6 on my development board and 3.7 works fine but 3.8 hangs
>> without uncompressing the kernel.
>
> Ok, thanks!
>
>         Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann - March 18, 2013, 10:58 a.m.
On Monday 18 March 2013, Fabio Porcedda wrote:
> Since by using platform_driver_probe() the  function
> ep93xx_pwm_probe() is freed after initialization,
> is better to use module_platform_drive_probe().
> IMHO i don't see any good reason to use module_platform_driver() for
> this driver.

As I commented earlier, the platform_driver_probe() and
module_platform_drive_probe() interfaces are rather dangerous in combination
with deferred probing, I would much prefer Harley's patch.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Fabio Porcedda - March 18, 2013, 11:20 a.m.
On Mon, Mar 18, 2013 at 11:58 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 18 March 2013, Fabio Porcedda wrote:
>> Since by using platform_driver_probe() the  function
>> ep93xx_pwm_probe() is freed after initialization,
>> is better to use module_platform_drive_probe().
>> IMHO i don't see any good reason to use module_platform_driver() for
>> this driver.
>
> As I commented earlier, the platform_driver_probe() and
> module_platform_drive_probe() interfaces are rather dangerous in combination
> with deferred probing, I would much prefer Harley's patch.

Since those drivers don't use -EPROBE_DEFER i was thinking that they don't use
deferred probing.
I'm missing something?

Best regards
Fabio Porcedda

>         Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann - March 18, 2013, 11:28 a.m.
On Monday 18 March 2013, Fabio Porcedda wrote:
> 
> On Mon, Mar 18, 2013 at 11:58 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Monday 18 March 2013, Fabio Porcedda wrote:
> >> Since by using platform_driver_probe() the  function
> >> ep93xx_pwm_probe() is freed after initialization,
> >> is better to use module_platform_drive_probe().
> >> IMHO i don't see any good reason to use module_platform_driver() for
> >> this driver.
> >
> > As I commented earlier, the platform_driver_probe() and
> > module_platform_drive_probe() interfaces are rather dangerous in combination
> > with deferred probing, I would much prefer Harley's patch.
> 
> Since those drivers don't use -EPROBE_DEFER i was thinking that they don't use
> deferred probing.
> I'm missing something?

clk_get() may return -EPROBE_DEFER after ep93xx is converted to use the
common clk API. We currently return the value of clk_get from the probe()
function, which will automatically do the right thing as long as the probe
function remains reachable.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Fabio Porcedda - March 19, 2013, 8:55 a.m.
On Mon, Mar 18, 2013 at 12:28 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 18 March 2013, Fabio Porcedda wrote:
>>
>> On Mon, Mar 18, 2013 at 11:58 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Monday 18 March 2013, Fabio Porcedda wrote:
>> >> Since by using platform_driver_probe() the  function
>> >> ep93xx_pwm_probe() is freed after initialization,
>> >> is better to use module_platform_drive_probe().
>> >> IMHO i don't see any good reason to use module_platform_driver() for
>> >> this driver.
>> >
>> > As I commented earlier, the platform_driver_probe() and
>> > module_platform_drive_probe() interfaces are rather dangerous in combination
>> > with deferred probing, I would much prefer Harley's patch.
>>
>> Since those drivers don't use -EPROBE_DEFER i was thinking that they don't use
>> deferred probing.
>> I'm missing something?
>
> clk_get() may return -EPROBE_DEFER after ep93xx is converted to use the
> common clk API. We currently return the value of clk_get from the probe()
> function, which will automatically do the right thing as long as the probe
> function remains reachable.

Thanks for the explanation.

Regards
Fabio Porcedda

>         Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Geert Uytterhoeven - March 19, 2013, 9:04 a.m.
On Tue, Mar 19, 2013 at 9:55 AM, Fabio Porcedda
<fabio.porcedda@gmail.com> wrote:
> On Mon, Mar 18, 2013 at 12:28 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Monday 18 March 2013, Fabio Porcedda wrote:
>>> On Mon, Mar 18, 2013 at 11:58 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> > On Monday 18 March 2013, Fabio Porcedda wrote:
>>> >> Since by using platform_driver_probe() the  function
>>> >> ep93xx_pwm_probe() is freed after initialization,
>>> >> is better to use module_platform_drive_probe().
>>> >> IMHO i don't see any good reason to use module_platform_driver() for
>>> >> this driver.
>>> >
>>> > As I commented earlier, the platform_driver_probe() and
>>> > module_platform_drive_probe() interfaces are rather dangerous in combination
>>> > with deferred probing, I would much prefer Harley's patch.
>>>
>>> Since those drivers don't use -EPROBE_DEFER i was thinking that they don't use
>>> deferred probing.
>>> I'm missing something?
>>
>> clk_get() may return -EPROBE_DEFER after ep93xx is converted to use the
>> common clk API. We currently return the value of clk_get from the probe()
>> function, which will automatically do the right thing as long as the probe
>> function remains reachable.
>
> Thanks for the explanation.

Hmm, so we may have drivers that (now) work perfectly fine with
module_platform_driver_probe()/platform_driver_probe(), but will start
failing suddenly in the future?

I guess we need a big fat WARN_ON(-EPROBE_DEFER) in
platform_driver_probe() to catch these?

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-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann - March 19, 2013, 4:48 p.m.
On Tuesday 19 March 2013, Geert Uytterhoeven wrote:
> Hmm, so we may have drivers that (now) work perfectly fine with
> module_platform_driver_probe()/platform_driver_probe(), but will start
> failing suddenly in the future?

They will fail if someone changes the initialization order. That would
already break drivers before deferred probing support (and was the reason
we added feature in the first place), but now we can be much more liberal
with the order in which drivers are initialized, except when they are
using platform_driver_probe()

> I guess we need a big fat WARN_ON(-EPROBE_DEFER) in
> platform_driver_probe() to catch these?

Yes, very good idea.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Fabio Porcedda - March 19, 2013, 5:11 p.m.
On Tue, Mar 19, 2013 at 5:48 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 19 March 2013, Geert Uytterhoeven wrote:
>> Hmm, so we may have drivers that (now) work perfectly fine with
>> module_platform_driver_probe()/platform_driver_probe(), but will start
>> failing suddenly in the future?
>
> They will fail if someone changes the initialization order. That would
> already break drivers before deferred probing support (and was the reason
> we added feature in the first place), but now we can be much more liberal
> with the order in which drivers are initialized, except when they are
> using platform_driver_probe()
>
>> I guess we need a big fat WARN_ON(-EPROBE_DEFER) in
>> platform_driver_probe() to catch these?
>
> Yes, very good idea.
>
>         Arnd

If it's fine, I'll send a patch for that.

Regards
--
Fabio Porcedda
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann - March 19, 2013, 5:59 p.m.
On Tuesday 19 March 2013, Fabio Porcedda wrote:
> On Tue, Mar 19, 2013 at 5:48 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Tuesday 19 March 2013, Geert Uytterhoeven wrote:
> >> Hmm, so we may have drivers that (now) work perfectly fine with
> >> module_platform_driver_probe()/platform_driver_probe(), but will start
> >> failing suddenly in the future?
> >
> > They will fail if someone changes the initialization order. That would
> > already break drivers before deferred probing support (and was the reason
> > we added feature in the first place), but now we can be much more liberal
> > with the order in which drivers are initialized, except when they are
> > using platform_driver_probe()
> >
> >> I guess we need a big fat WARN_ON(-EPROBE_DEFER) in
> >> platform_driver_probe() to catch these?
> >
> > Yes, very good idea.
> 
> If it's fine, I'll send a patch for that.

That would be cool, yes. I looked at it earlier (after sending my email above)
and couldn't find an easy way to do it though, because platform_drv_probe
does not know whether it is called from platform_driver_probe or not.

Maybe using something other than platform_driver_register would work here.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Fabio Porcedda - March 20, 2013, 9:02 a.m.
On Tue, Mar 19, 2013 at 6:59 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 19 March 2013, Fabio Porcedda wrote:
>> On Tue, Mar 19, 2013 at 5:48 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Tuesday 19 March 2013, Geert Uytterhoeven wrote:
>> >> Hmm, so we may have drivers that (now) work perfectly fine with
>> >> module_platform_driver_probe()/platform_driver_probe(), but will start
>> >> failing suddenly in the future?
>> >
>> > They will fail if someone changes the initialization order. That would
>> > already break drivers before deferred probing support (and was the reason
>> > we added feature in the first place), but now we can be much more liberal
>> > with the order in which drivers are initialized, except when they are
>> > using platform_driver_probe()
>> >
>> >> I guess we need a big fat WARN_ON(-EPROBE_DEFER) in
>> >> platform_driver_probe() to catch these?
>> >
>> > Yes, very good idea.
>>
>> If it's fine, I'll send a patch for that.
>
> That would be cool, yes. I looked at it earlier (after sending my email above)
> and couldn't find an easy way to do it though, because platform_drv_probe
> does not know whether it is called from platform_driver_probe or not.
>
> Maybe using something other than platform_driver_register would work here.
>
>         Arnd

I think we can check inside the  deferred_probe_work_func()
if the dev->probe function pointer is equal to platform_drv_probe_fail().

Regards
--
Fabio Porcedda
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann - March 20, 2013, 10:20 a.m.
On Wednesday 20 March 2013, Fabio Porcedda wrote:
> I think we can check inside the  deferred_probe_work_func()
> if the dev->probe function pointer is equal to platform_drv_probe_fail().

I think it's too late by then, because that would only warn if we try to probe
it again, but when platform_driver_probe() does not succeed immediately, it
unregisters the driver again, so we never get there.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Fabio Porcedda - March 20, 2013, 10:39 a.m.
On Wed, Mar 20, 2013 at 11:20 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 20 March 2013, Fabio Porcedda wrote:
>> I think we can check inside the  deferred_probe_work_func()
>> if the dev->probe function pointer is equal to platform_drv_probe_fail().
>
> I think it's too late by then, because that would only warn if we try to probe
> it again, but when platform_driver_probe() does not succeed immediately, it

Maybe you mean "does succeed immediately" ?

> unregisters the driver again, so we never get there.

--
Fabio Porcedda
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann - March 20, 2013, 11:46 a.m.
On Wednesday 20 March 2013, Fabio Porcedda wrote:
> 
> On Wed, Mar 20, 2013 at 11:20 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Wednesday 20 March 2013, Fabio Porcedda wrote:
> >> I think we can check inside the  deferred_probe_work_func()
> >> if the dev->probe function pointer is equal to platform_drv_probe_fail().
> >
> > I think it's too late by then, because that would only warn if we try to probe
> > it again, but when platform_driver_probe() does not succeed immediately, it
> 
> Maybe you mean "does succeed immediately" ?

I mean in this code (simplified for the sake of discussion)

int __init_or_module platform_driver_probe(struct platform_driver *drv,
                int (*probe)(struct platform_device *))
{
        int retval, code;

        drv->probe = probe;
        retval = code = platform_driver_register(drv);

        drv->probe = NULL;
        if (code == 0 && list_empty(&drv->driver.p->klist_devices.k_list))
                retval = -ENODEV;
        drv->driver.probe = platform_drv_probe_fail;

        if (code != retval)
                platform_driver_unregister(drv);
        return retval;
}

we assume that all devices are bound to drivers during the call to
platform_driver_register, and if the device list is empty afterwards,
we unregister the driver and will never get to the deferred probing
stage.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Fabio Porcedda - March 21, 2013, 1:10 p.m.
On Wed, Mar 20, 2013 at 12:46 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 20 March 2013, Fabio Porcedda wrote:
>>
>> On Wed, Mar 20, 2013 at 11:20 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Wednesday 20 March 2013, Fabio Porcedda wrote:
>> >> I think we can check inside the  deferred_probe_work_func()
>> >> if the dev->probe function pointer is equal to platform_drv_probe_fail().
>> >
>> > I think it's too late by then, because that would only warn if we try to probe
>> > it again, but when platform_driver_probe() does not succeed immediately, it
>>
>> Maybe you mean "does succeed immediately" ?
>
> I mean in this code (simplified for the sake of discussion)
>
> int __init_or_module platform_driver_probe(struct platform_driver *drv,
>                 int (*probe)(struct platform_device *))
> {
>         int retval, code;
>
>         drv->probe = probe;
>         retval = code = platform_driver_register(drv);
>
>         drv->probe = NULL;
>         if (code == 0 && list_empty(&drv->driver.p->klist_devices.k_list))
>                 retval = -ENODEV;
>         drv->driver.probe = platform_drv_probe_fail;
>
>         if (code != retval)
>                 platform_driver_unregister(drv);
>         return retval;
> }
>
> we assume that all devices are bound to drivers during the call to
> platform_driver_register, and if the device list is empty afterwards,
> we unregister the driver and will never get to the deferred probing
> stage.

Thanks for the explanation, I understand now that is not that simple.

I was hoping it was easier.

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

Patch

diff --git a/drivers/misc/atmel_pwm.c b/drivers/misc/atmel_pwm.c
index 28f5aaa..494d050 100644
--- a/drivers/misc/atmel_pwm.c
+++ b/drivers/misc/atmel_pwm.c
@@ -393,17 +393,7 @@  static struct platform_driver atmel_pwm_driver = {
 	 */
 };
 
-static int __init pwm_init(void)
-{
-	return platform_driver_probe(&atmel_pwm_driver, pwm_probe);
-}
-module_init(pwm_init);
-
-static void __exit pwm_exit(void)
-{
-	platform_driver_unregister(&atmel_pwm_driver);
-}
-module_exit(pwm_exit);
+module_platform_driver_probe(atmel_pwm_driver, pwm_probe);
 
 MODULE_DESCRIPTION("Driver for AT32/AT91 PWM module");
 MODULE_LICENSE("GPL");
diff --git a/drivers/misc/ep93xx_pwm.c b/drivers/misc/ep93xx_pwm.c
index 16d7179..96787ec 100644
--- a/drivers/misc/ep93xx_pwm.c
+++ b/drivers/misc/ep93xx_pwm.c
@@ -365,18 +365,7 @@  static struct platform_driver ep93xx_pwm_driver = {
 	.remove		= __exit_p(ep93xx_pwm_remove),
 };
 
-static int __init ep93xx_pwm_init(void)
-{
-	return platform_driver_probe(&ep93xx_pwm_driver, ep93xx_pwm_probe);
-}
-
-static void __exit ep93xx_pwm_exit(void)
-{
-	platform_driver_unregister(&ep93xx_pwm_driver);
-}
-
-module_init(ep93xx_pwm_init);
-module_exit(ep93xx_pwm_exit);
+module_platform_driver_probe(ep93xx_pwm_driver, ep93xx_pwm_probe);
 
 MODULE_AUTHOR("Matthieu Crapet <mcrapet@gmail.com>, "
 	      "H Hartley Sweeten <hsweeten@visionengravers.com>");