Re: + spi-imx-use-platform_driver_probe-as-probe-lives-in-inittext.patch added to -mm tree

Re: + spi-imx-use-platform_driver_probe-as-probe-lives-in-inittext.patch added to -mm tree

[Grant Likely]

2009/12/9 Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>:
> On Tue, Dec 08, 2009 at 05:34:21PM -0700, Grant Likely wrote:
>> Hi Uwe.
>>
>> On Tue, Dec 8, 2009 at 4:30 PM,  <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> wrote:
>> > Subject: spi-imx: use platform_driver_probe as probe lives in .init.text
>> > From: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> >
>> > Using platform_driver_register with a probe function defined using __init
>> > is wrong.
>> >
>> > This fixes an oops after:
>> >
>> >        cd /sys/bus/platform/drivers/spi_imx
>> >        echo -n spi_imx.0 > unbind
>> >        echo -n spi_imx.0 > bind
>> >
>> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> > Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
>> > Cc: Daniel Mack <daniel-rDUAYElUppE@public.gmane.org>
>> > Cc: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> > Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
>> > Signed-off-by: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
>> > ---
>> >
>> >  drivers/spi/spi_imx.c |    3 +--
>> >  1 file changed, 1 insertion(+), 2 deletions(-)
>> >
>> > diff -puN drivers/spi/spi_imx.c~spi-imx-use-platform_driver_probe-as-probe-lives-in-inittext drivers/spi/spi_imx.c
>> > --- a/drivers/spi/spi_imx.c~spi-imx-use-platform_driver_probe-as-probe-lives-in-inittext
>> > +++ a/drivers/spi/spi_imx.c
>> > @@ -666,13 +666,12 @@ static struct platform_driver spi_imx_dr
>> >                   .name = DRIVER_NAME,
>> >                   .owner = THIS_MODULE,
>> >                   },
>> > -       .probe = spi_imx_probe,
>> >        .remove = __exit_p(spi_imx_remove),
>>
>> This looks wrong.  Why can't spi_imx_probe and spi_imx_remove be
>> changed to be __devinit and __devexit?
> I don't care much, but I thought this to be in accordance to
>
>        d1e44d9 (SPI driver runtime footprint shrinkage)
>
> (http://git.kernel.org/linus/d1e44d9).  I have a whole bunch of such
> patches and already sent it three times (I think).  There is no
> accordance in the community that one of the two possibilities is better
> than the other.

hmmm.  I don't like the pattern, but I'm not going to reject it out of
hand.  Let me think about it for a bit.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

------------------------------------------------------------------------------
Return on Information:
Google Enterprise Search pays you back
Get the facts.
http://p.sf.net/sfu/google-dev2dev

Re: + spi-imx-use-platform_driver_probe-as-probe-lives-in-inittext.patch added to -mm tree

[Grant Likely]

2009/12/9 Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>:
> 2009/12/9 Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>:
>> On Tue, Dec 08, 2009 at 05:34:21PM -0700, Grant Likely wrote:
>>> Hi Uwe.
>>>
>>> On Tue, Dec 8, 2009 at 4:30 PM,  <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> wrote:
>>> > Subject: spi-imx: use platform_driver_probe as probe lives in .init.text
>>> > From: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>>> >
>>> > Using platform_driver_register with a probe function defined using __init
>>> > is wrong.
>>> >
>>> > This fixes an oops after:
>>> >
>>> >        cd /sys/bus/platform/drivers/spi_imx
>>> >        echo -n spi_imx.0 > unbind
>>> >        echo -n spi_imx.0 > bind
>>> >
>>> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>>> > Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
>>> > Cc: Daniel Mack <daniel-rDUAYElUppE@public.gmane.org>
>>> > Cc: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>>> > Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
>>> > Signed-off-by: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
>>> > ---
>>> >
>>> >  drivers/spi/spi_imx.c |    3 +--
>>> >  1 file changed, 1 insertion(+), 2 deletions(-)
>>> >
>>> > diff -puN drivers/spi/spi_imx.c~spi-imx-use-platform_driver_probe-as-probe-lives-in-inittext drivers/spi/spi_imx.c
>>> > --- a/drivers/spi/spi_imx.c~spi-imx-use-platform_driver_probe-as-probe-lives-in-inittext
>>> > +++ a/drivers/spi/spi_imx.c
>>> > @@ -666,13 +666,12 @@ static struct platform_driver spi_imx_dr
>>> >                   .name = DRIVER_NAME,
>>> >                   .owner = THIS_MODULE,
>>> >                   },
>>> > -       .probe = spi_imx_probe,
>>> >        .remove = __exit_p(spi_imx_remove),
>>>
>>> This looks wrong.  Why can't spi_imx_probe and spi_imx_remove be
>>> changed to be __devinit and __devexit?
>> I don't care much, but I thought this to be in accordance to
>>
>>        d1e44d9 (SPI driver runtime footprint shrinkage)
>>
>> (http://git.kernel.org/linus/d1e44d9).  I have a whole bunch of such
>> patches and already sent it three times (I think).  There is no
>> accordance in the community that one of the two possibilities is better
>> than the other.
>
> hmmm.  I don't like the pattern, but I'm not going to reject it out of
> hand.  Let me think about it for a bit.

But regardless, I'll make a decision before the end of the merge
window because I know it fixes an oops.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

------------------------------------------------------------------------------
Return on Information:
Google Enterprise Search pays you back
Get the facts.
http://p.sf.net/sfu/google-dev2dev

Re: + spi-imx-use-platform_driver_probe-as-probe-lives-in-inittext.patch added to -mm tree

[Grant Likely]

2009/12/9 Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>:
> 2009/12/9 Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>:
>> 2009/12/9 Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>:
>>> On Tue, Dec 08, 2009 at 05:34:21PM -0700, Grant Likely wrote:
>>>> Hi Uwe.
>>>>
>>>> On Tue, Dec 8, 2009 at 4:30 PM,  <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> wrote:
>>>> > Subject: spi-imx: use platform_driver_probe as probe lives in .init.text
>>>> > From: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>>>> >
>>>> > Using platform_driver_register with a probe function defined using __init
>>>> > is wrong.
>>>> >
>>>> > This fixes an oops after:
>>>> >
>>>> >        cd /sys/bus/platform/drivers/spi_imx
>>>> >        echo -n spi_imx.0 > unbind
>>>> >        echo -n spi_imx.0 > bind
>>>> >
>>>> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>>>> > Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
>>>> > Cc: Daniel Mack <daniel-rDUAYElUppE@public.gmane.org>
>>>> > Cc: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>>>> > Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
>>>> > Signed-off-by: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
>>>> > ---
>>>> >
>>>> >  drivers/spi/spi_imx.c |    3 +--
>>>> >  1 file changed, 1 insertion(+), 2 deletions(-)
>>>> >
>>>> > diff -puN drivers/spi/spi_imx.c~spi-imx-use-platform_driver_probe-as-probe-lives-in-inittext drivers/spi/spi_imx.c
>>>> > --- a/drivers/spi/spi_imx.c~spi-imx-use-platform_driver_probe-as-probe-lives-in-inittext
>>>> > +++ a/drivers/spi/spi_imx.c
>>>> > @@ -666,13 +666,12 @@ static struct platform_driver spi_imx_dr
>>>> >                   .name = DRIVER_NAME,
>>>> >                   .owner = THIS_MODULE,
>>>> >                   },
>>>> > -       .probe = spi_imx_probe,
>>>> >        .remove = __exit_p(spi_imx_remove),
>>>>
>>>> This looks wrong.  Why can't spi_imx_probe and spi_imx_remove be
>>>> changed to be __devinit and __devexit?
>>> I don't care much, but I thought this to be in accordance to
>>>
>>>        d1e44d9 (SPI driver runtime footprint shrinkage)
>>>
>>> (http://git.kernel.org/linus/d1e44d9).  I have a whole bunch of such
>>> patches and already sent it three times (I think).  There is no
>>> accordance in the community that one of the two possibilities is better
>>> than the other.
>>
>> hmmm.  I don't like the pattern, but I'm not going to reject it out of
>> hand.  Let me think about it for a bit.
>
> But regardless, I'll make a decision before the end of the merge
> window because I know it fixes an oops.

I've committed a patch to change probe/remove to __devinit/__devexit.
>From a patterns point of view, I don't think it is the right thing to
do.  I understand the pragmatic reason for calling
platform_driver_probe() in the module init, but I think there has to
be a better way to go about it.

The way I see it, right now CONFIG_HOTPLUG is an all or nothing thing.
 Either all drivers discard __devinit sections, or none of them do.
It should be possible to make that decision at build time (or even run
time in the case of modules) without changing the coding pattern on a
driver-by-driver basis.  I think it is worth investigating, but in the
mean time I've just done the __devinit thing to fix the oops.

This isn't a final decision, but I wanted to merge something before
the window closed.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

------------------------------------------------------------------------------
Return on Information:
Google Enterprise Search pays you back
Get the facts.
http://p.sf.net/sfu/google-dev2dev