driver_find_device() can be called with an unregistered driver. Need
to check driver_private to see if it's populated or not, especially
under deferrable probe.
Signed-off-by: Hiroshi DOYU <[email protected]>
---
In [PATCHv5 2/3] ARM: tegra: Add SMMU enabler in AHB:
http://article.gmane.org/gmane.linux.ports.tegra/4658
"tegra_ahb_driver" may not be populated when it's called.
---
drivers/base/driver.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index 3ec3896..207c27d 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -80,7 +80,7 @@ struct device *driver_find_device(struct device_driver *drv,
struct klist_iter i;
struct device *dev;
- if (!drv)
+ if (!drv || !drv->p)
return NULL;
klist_iter_init_node(&drv->p->klist_devices, &i,
--
1.7.5.4
On Thu, May 10, 2012 at 10:35:02AM +0300, Hiroshi DOYU wrote:
> driver_find_device() can be called with an unregistered driver.
Who does that? Where in the kernel? Why would you try to do that?
> Need to check driver_private to see if it's populated or not,
> especially under deferrable probe.
Hm, I don't know if this will really catch a driver that was registered
and then was unregistered, right? It seems like moving the real problem
somewhere else, why not fix the original issue instead?
> Signed-off-by: Hiroshi DOYU <[email protected]>
> ---
> In [PATCHv5 2/3] ARM: tegra: Add SMMU enabler in AHB:
> http://article.gmane.org/gmane.linux.ports.tegra/4658
>
> "tegra_ahb_driver" may not be populated when it's called.
It can? I don't see that in that patch.
confused,
greg k-h
On 05/10/2012 08:11 AM, Greg Kroah-Hartman wrote:
> On Thu, May 10, 2012 at 10:35:02AM +0300, Hiroshi DOYU wrote:
>> driver_find_device() can be called with an unregistered driver.
>
> Who does that? Where in the kernel? Why would you try to do that?
>
>> Need to check driver_private to see if it's populated or not,
>> especially under deferrable probe.
>
> Hm, I don't know if this will really catch a driver that was registered
> and then was unregistered, right? It seems like moving the real problem
> somewhere else, why not fix the original issue instead?
>
>> Signed-off-by: Hiroshi DOYU <[email protected]>
>> ---
>> In [PATCHv5 2/3] ARM: tegra: Add SMMU enabler in AHB:
>> http://article.gmane.org/gmane.linux.ports.tegra/4658
>>
>> "tegra_ahb_driver" may not be populated when it's called.
>
> It can? I don't see that in that patch.
I think this is what's happening:
The Tegra SMMU driver is registered using subsys_initcall().
The Tegra AHB driver is registered using module_platform_driver, so
module_init().
The device objects for both are registered at basically the same time
during the machine's initialization call, which I think happens before
both or some of the above two calls.
So, SMMU ends up getting probed before the AHB driver has even been
registered.
That's why in patch Hiroshi linked, in tegra_ahb_enable_smmu(), the AHB
driver hasn't (or may not have) been registered, so driver_find_device()
can experience the issue this patch attempts to solve.
On Thu, May 10, 2012 at 09:59:15AM -0600, Stephen Warren wrote:
> On 05/10/2012 08:11 AM, Greg Kroah-Hartman wrote:
> > On Thu, May 10, 2012 at 10:35:02AM +0300, Hiroshi DOYU wrote:
> >> driver_find_device() can be called with an unregistered driver.
> >
> > Who does that? Where in the kernel? Why would you try to do that?
> >
> >> Need to check driver_private to see if it's populated or not,
> >> especially under deferrable probe.
> >
> > Hm, I don't know if this will really catch a driver that was registered
> > and then was unregistered, right? It seems like moving the real problem
> > somewhere else, why not fix the original issue instead?
> >
> >> Signed-off-by: Hiroshi DOYU <[email protected]>
> >> ---
> >> In [PATCHv5 2/3] ARM: tegra: Add SMMU enabler in AHB:
> >> http://article.gmane.org/gmane.linux.ports.tegra/4658
> >>
> >> "tegra_ahb_driver" may not be populated when it's called.
> >
> > It can? I don't see that in that patch.
>
> I think this is what's happening:
>
> The Tegra SMMU driver is registered using subsys_initcall().
>
> The Tegra AHB driver is registered using module_platform_driver, so
> module_init().
>
> The device objects for both are registered at basically the same time
> during the machine's initialization call, which I think happens before
> both or some of the above two calls.
>
> So, SMMU ends up getting probed before the AHB driver has even been
> registered.
What do those two drivers have to do with each other?
> That's why in patch Hiroshi linked, in tegra_ahb_enable_smmu(), the AHB
> driver hasn't (or may not have) been registered, so driver_find_device()
> can experience the issue this patch attempts to solve.
It sounds like you have a locking or ordering problem somewhere in this
platform, and it needs to be resolved there. Odds are, this tiny check
is the least of your worries, right?
Who is doing the driver_find_device() call in the first place? The
driver core? Or something else?
greg k-h
On 05/10/2012 12:16 PM, Greg Kroah-Hartman wrote:
> On Thu, May 10, 2012 at 09:59:15AM -0600, Stephen Warren wrote:
>> On 05/10/2012 08:11 AM, Greg Kroah-Hartman wrote:
>>> On Thu, May 10, 2012 at 10:35:02AM +0300, Hiroshi DOYU wrote:
>>>> driver_find_device() can be called with an unregistered driver.
>>>
>>> Who does that? Where in the kernel? Why would you try to do that?
>>>
>>>> Need to check driver_private to see if it's populated or not,
>>>> especially under deferrable probe.
>>>
>>> Hm, I don't know if this will really catch a driver that was registered
>>> and then was unregistered, right? It seems like moving the real problem
>>> somewhere else, why not fix the original issue instead?
>>>
>>>> Signed-off-by: Hiroshi DOYU <[email protected]>
>>>> ---
>>>> In [PATCHv5 2/3] ARM: tegra: Add SMMU enabler in AHB:
>>>> http://article.gmane.org/gmane.linux.ports.tegra/4658
>>>>
>>>> "tegra_ahb_driver" may not be populated when it's called.
>>>
>>> It can? I don't see that in that patch.
>>
>> I think this is what's happening:
>>
>> The Tegra SMMU driver is registered using subsys_initcall().
>>
>> The Tegra AHB driver is registered using module_platform_driver, so
>> module_init().
>>
>> The device objects for both are registered at basically the same time
>> during the machine's initialization call, which I think happens before
>> both or some of the above two calls.
>>
>> So, SMMU ends up getting probed before the AHB driver has even been
>> registered.
>
> What do those two drivers have to do with each other?
The AHB HW module contains a bit to say "enable the SMMU". This bit
cannot be turned on until certain initialization has been performed by
the SMMU driver.
So, SMMU's probe performs the initialization, then attempts to contact
the AHB driver to ask it to enable the SMMU.
>> That's why in patch Hiroshi linked, in tegra_ahb_enable_smmu(), the AHB
>> driver hasn't (or may not have) been registered, so driver_find_device()
>> can experience the issue this patch attempts to solve.
>
> It sounds like you have a locking or ordering problem somewhere in this
> platform, and it needs to be resolved there. Odds are, this tiny check
> is the least of your worries, right?
>
> Who is doing the driver_find_device() call in the first place? The
> driver core? Or something else?
SMMU's probe calls tegra_ahb_enable_smmu().
tegra_ahb_enable_smmu() needs to find the AHB's struct device so that it
can find get the driver data associated with it, which contains the
ioremap'd registers amongst other things.
In order to find the correct device, tegra_ahb_enable_smmu() is passed
the device tree node of the appropriate AHB that controls the SMMU.
The two drivers use deferred probe to ensure that their relative probe
order doesn't matter. If AHB is probed first, then
tegra_ahb_enable_smmu()'s driver_find_device() succeeds, and the AHB
register is programmed to enable the SMMU. If the SMMU is probed first,
then tegra_ahb_enable_smmu() returns -EPROBE_DEFER to hold off the SMMU
from completing its probe. I believe this is exactly the kind of thing
-EPROBE_DEFER was intended for.
On Thu, May 10, 2012 at 02:00:48PM -0600, Stephen Warren wrote:
> On 05/10/2012 12:16 PM, Greg Kroah-Hartman wrote:
> > On Thu, May 10, 2012 at 09:59:15AM -0600, Stephen Warren wrote:
> >> On 05/10/2012 08:11 AM, Greg Kroah-Hartman wrote:
> >>> On Thu, May 10, 2012 at 10:35:02AM +0300, Hiroshi DOYU wrote:
> >>>> driver_find_device() can be called with an unregistered driver.
> >>>
> >>> Who does that? Where in the kernel? Why would you try to do that?
> >>>
> >>>> Need to check driver_private to see if it's populated or not,
> >>>> especially under deferrable probe.
> >>>
> >>> Hm, I don't know if this will really catch a driver that was registered
> >>> and then was unregistered, right? It seems like moving the real problem
> >>> somewhere else, why not fix the original issue instead?
> >>>
> >>>> Signed-off-by: Hiroshi DOYU <[email protected]>
> >>>> ---
> >>>> In [PATCHv5 2/3] ARM: tegra: Add SMMU enabler in AHB:
> >>>> http://article.gmane.org/gmane.linux.ports.tegra/4658
> >>>>
> >>>> "tegra_ahb_driver" may not be populated when it's called.
> >>>
> >>> It can? I don't see that in that patch.
> >>
> >> I think this is what's happening:
> >>
> >> The Tegra SMMU driver is registered using subsys_initcall().
> >>
> >> The Tegra AHB driver is registered using module_platform_driver, so
> >> module_init().
> >>
> >> The device objects for both are registered at basically the same time
> >> during the machine's initialization call, which I think happens before
> >> both or some of the above two calls.
> >>
> >> So, SMMU ends up getting probed before the AHB driver has even been
> >> registered.
> >
> > What do those two drivers have to do with each other?
>
> The AHB HW module contains a bit to say "enable the SMMU". This bit
> cannot be turned on until certain initialization has been performed by
> the SMMU driver.
>
> So, SMMU's probe performs the initialization, then attempts to contact
> the AHB driver to ask it to enable the SMMU.
>
> >> That's why in patch Hiroshi linked, in tegra_ahb_enable_smmu(), the AHB
> >> driver hasn't (or may not have) been registered, so driver_find_device()
> >> can experience the issue this patch attempts to solve.
> >
> > It sounds like you have a locking or ordering problem somewhere in this
> > platform, and it needs to be resolved there. Odds are, this tiny check
> > is the least of your worries, right?
> >
> > Who is doing the driver_find_device() call in the first place? The
> > driver core? Or something else?
>
> SMMU's probe calls tegra_ahb_enable_smmu().
>
> tegra_ahb_enable_smmu() needs to find the AHB's struct device so that it
> can find get the driver data associated with it, which contains the
> ioremap'd registers amongst other things.
>
> In order to find the correct device, tegra_ahb_enable_smmu() is passed
> the device tree node of the appropriate AHB that controls the SMMU.
>
> The two drivers use deferred probe to ensure that their relative probe
> order doesn't matter. If AHB is probed first, then
> tegra_ahb_enable_smmu()'s driver_find_device() succeeds, and the AHB
> register is programmed to enable the SMMU. If the SMMU is probed first,
> then tegra_ahb_enable_smmu() returns -EPROBE_DEFER to hold off the SMMU
> from completing its probe. I believe this is exactly the kind of thing
> -EPROBE_DEFER was intended for.
Ok, thanks for explaining it better, that would have been nice to have
in the original patch submission :)
This fix, is it needed for 3.4, or just 3.5?
I kind of don't like relying on ->p as a check to see if things are set
up properly, I wonder if we could use the kobject state_initalized field
instead somehow, or maybe state_in_sysfs? Oh wait, that's in ->p as
well. Ok, I guess this is as good as it can get.
Can someone resend this to me, with the information above in the
changelog comment, and I'll be glad to apply it? Again, I need to know
if this is needed for 3.4 or it can wait for 3.5?
thanks,
greg k-h
On 05/10/2012 04:28 PM, Greg Kroah-Hartman wrote:
> On Thu, May 10, 2012 at 02:00:48PM -0600, Stephen Warren wrote:
>> On 05/10/2012 12:16 PM, Greg Kroah-Hartman wrote:
>>> On Thu, May 10, 2012 at 09:59:15AM -0600, Stephen Warren wrote:
>>>> On 05/10/2012 08:11 AM, Greg Kroah-Hartman wrote:
>>>>> On Thu, May 10, 2012 at 10:35:02AM +0300, Hiroshi DOYU wrote:
>>>>>> driver_find_device() can be called with an unregistered driver.
>>>>>
>>>>> Who does that? Where in the kernel? Why would you try to do that?
>>>>>
>>>>>> Need to check driver_private to see if it's populated or not,
>>>>>> especially under deferrable probe.
>>>>>
>>>>> Hm, I don't know if this will really catch a driver that was registered
>>>>> and then was unregistered, right? It seems like moving the real problem
>>>>> somewhere else, why not fix the original issue instead?
>>>>>
>>>>>> Signed-off-by: Hiroshi DOYU <[email protected]>
>>>>>> ---
>>>>>> In [PATCHv5 2/3] ARM: tegra: Add SMMU enabler in AHB:
>>>>>> http://article.gmane.org/gmane.linux.ports.tegra/4658
>>>>>>
>>>>>> "tegra_ahb_driver" may not be populated when it's called.
>>>>>
>>>>> It can? I don't see that in that patch.
>>>>
>>>> I think this is what's happening:
>>>>
>>>> The Tegra SMMU driver is registered using subsys_initcall().
>>>>
>>>> The Tegra AHB driver is registered using module_platform_driver, so
>>>> module_init().
>>>>
>>>> The device objects for both are registered at basically the same time
>>>> during the machine's initialization call, which I think happens before
>>>> both or some of the above two calls.
>>>>
>>>> So, SMMU ends up getting probed before the AHB driver has even been
>>>> registered.
>>>
>>> What do those two drivers have to do with each other?
>>
>> The AHB HW module contains a bit to say "enable the SMMU". This bit
>> cannot be turned on until certain initialization has been performed by
>> the SMMU driver.
>>
>> So, SMMU's probe performs the initialization, then attempts to contact
>> the AHB driver to ask it to enable the SMMU.
>>
>>>> That's why in patch Hiroshi linked, in tegra_ahb_enable_smmu(), the AHB
>>>> driver hasn't (or may not have) been registered, so driver_find_device()
>>>> can experience the issue this patch attempts to solve.
>>>
>>> It sounds like you have a locking or ordering problem somewhere in this
>>> platform, and it needs to be resolved there. Odds are, this tiny check
>>> is the least of your worries, right?
>>>
>>> Who is doing the driver_find_device() call in the first place? The
>>> driver core? Or something else?
>>
>> SMMU's probe calls tegra_ahb_enable_smmu().
>>
>> tegra_ahb_enable_smmu() needs to find the AHB's struct device so that it
>> can find get the driver data associated with it, which contains the
>> ioremap'd registers amongst other things.
>>
>> In order to find the correct device, tegra_ahb_enable_smmu() is passed
>> the device tree node of the appropriate AHB that controls the SMMU.
>>
>> The two drivers use deferred probe to ensure that their relative probe
>> order doesn't matter. If AHB is probed first, then
>> tegra_ahb_enable_smmu()'s driver_find_device() succeeds, and the AHB
>> register is programmed to enable the SMMU. If the SMMU is probed first,
>> then tegra_ahb_enable_smmu() returns -EPROBE_DEFER to hold off the SMMU
>> from completing its probe. I believe this is exactly the kind of thing
>> -EPROBE_DEFER was intended for.
>
> Ok, thanks for explaining it better, that would have been nice to have
> in the original patch submission :)
>
> This fix, is it needed for 3.4, or just 3.5?
The SMMU driver that needs this will be added in 3.6 (so putting this
patch in 3.5 would be good to simplify dependencies). It's not needed in
3.4.
> I kind of don't like relying on ->p as a check to see if things are set
> up properly, I wonder if we could use the kobject state_initalized field
> instead somehow, or maybe state_in_sysfs? Oh wait, that's in ->p as
> well. Ok, I guess this is as good as it can get.
In the case you mentioned of the driver getting unregistered, can the
unregister code clear ->p?
> Can someone resend this to me, with the information above in the
> changelog comment, and I'll be glad to apply it? Again, I need to know
> if this is needed for 3.4 or it can wait for 3.5?
I'll let Hiroshi do that.
On Thu, May 10, 2012 at 04:34:40PM -0600, Stephen Warren wrote:
> On 05/10/2012 04:28 PM, Greg Kroah-Hartman wrote:
> > On Thu, May 10, 2012 at 02:00:48PM -0600, Stephen Warren wrote:
> >> On 05/10/2012 12:16 PM, Greg Kroah-Hartman wrote:
> >>> On Thu, May 10, 2012 at 09:59:15AM -0600, Stephen Warren wrote:
> >>>> On 05/10/2012 08:11 AM, Greg Kroah-Hartman wrote:
> >>>>> On Thu, May 10, 2012 at 10:35:02AM +0300, Hiroshi DOYU wrote:
> >>>>>> driver_find_device() can be called with an unregistered driver.
> >>>>>
> >>>>> Who does that? Where in the kernel? Why would you try to do that?
> >>>>>
> >>>>>> Need to check driver_private to see if it's populated or not,
> >>>>>> especially under deferrable probe.
> >>>>>
> >>>>> Hm, I don't know if this will really catch a driver that was registered
> >>>>> and then was unregistered, right? It seems like moving the real problem
> >>>>> somewhere else, why not fix the original issue instead?
> >>>>>
> >>>>>> Signed-off-by: Hiroshi DOYU <[email protected]>
> >>>>>> ---
> >>>>>> In [PATCHv5 2/3] ARM: tegra: Add SMMU enabler in AHB:
> >>>>>> http://article.gmane.org/gmane.linux.ports.tegra/4658
> >>>>>>
> >>>>>> "tegra_ahb_driver" may not be populated when it's called.
> >>>>>
> >>>>> It can? I don't see that in that patch.
> >>>>
> >>>> I think this is what's happening:
> >>>>
> >>>> The Tegra SMMU driver is registered using subsys_initcall().
> >>>>
> >>>> The Tegra AHB driver is registered using module_platform_driver, so
> >>>> module_init().
> >>>>
> >>>> The device objects for both are registered at basically the same time
> >>>> during the machine's initialization call, which I think happens before
> >>>> both or some of the above two calls.
> >>>>
> >>>> So, SMMU ends up getting probed before the AHB driver has even been
> >>>> registered.
> >>>
> >>> What do those two drivers have to do with each other?
> >>
> >> The AHB HW module contains a bit to say "enable the SMMU". This bit
> >> cannot be turned on until certain initialization has been performed by
> >> the SMMU driver.
> >>
> >> So, SMMU's probe performs the initialization, then attempts to contact
> >> the AHB driver to ask it to enable the SMMU.
> >>
> >>>> That's why in patch Hiroshi linked, in tegra_ahb_enable_smmu(), the AHB
> >>>> driver hasn't (or may not have) been registered, so driver_find_device()
> >>>> can experience the issue this patch attempts to solve.
> >>>
> >>> It sounds like you have a locking or ordering problem somewhere in this
> >>> platform, and it needs to be resolved there. Odds are, this tiny check
> >>> is the least of your worries, right?
> >>>
> >>> Who is doing the driver_find_device() call in the first place? The
> >>> driver core? Or something else?
> >>
> >> SMMU's probe calls tegra_ahb_enable_smmu().
> >>
> >> tegra_ahb_enable_smmu() needs to find the AHB's struct device so that it
> >> can find get the driver data associated with it, which contains the
> >> ioremap'd registers amongst other things.
> >>
> >> In order to find the correct device, tegra_ahb_enable_smmu() is passed
> >> the device tree node of the appropriate AHB that controls the SMMU.
> >>
> >> The two drivers use deferred probe to ensure that their relative probe
> >> order doesn't matter. If AHB is probed first, then
> >> tegra_ahb_enable_smmu()'s driver_find_device() succeeds, and the AHB
> >> register is programmed to enable the SMMU. If the SMMU is probed first,
> >> then tegra_ahb_enable_smmu() returns -EPROBE_DEFER to hold off the SMMU
> >> from completing its probe. I believe this is exactly the kind of thing
> >> -EPROBE_DEFER was intended for.
> >
> > Ok, thanks for explaining it better, that would have been nice to have
> > in the original patch submission :)
> >
> > This fix, is it needed for 3.4, or just 3.5?
>
> The SMMU driver that needs this will be added in 3.6 (so putting this
> patch in 3.5 would be good to simplify dependencies). It's not needed in
> 3.4.
Ok, good to know.
> > I kind of don't like relying on ->p as a check to see if things are set
> > up properly, I wonder if we could use the kobject state_initalized field
> > instead somehow, or maybe state_in_sysfs? Oh wait, that's in ->p as
> > well. Ok, I guess this is as good as it can get.
>
> In the case you mentioned of the driver getting unregistered, can the
> unregister code clear ->p?
I do not know. Try it in the bus_remove_driver() call after the
kobject_put() line, that might do it, but it might cause problems, I
can't remember at the moment. It would need a lot of testing
thanks,
greg k-h
Thanks Stephen for explaining the background of this patch.
On Fri, 11 May 2012 00:34:40 +0200
Stephen Warren <[email protected]> wrote:
> On 05/10/2012 04:28 PM, Greg Kroah-Hartman wrote:
> > On Thu, May 10, 2012 at 02:00:48PM -0600, Stephen Warren wrote:
> >> On 05/10/2012 12:16 PM, Greg Kroah-Hartman wrote:
> >>> On Thu, May 10, 2012 at 09:59:15AM -0600, Stephen Warren wrote:
> >>>> On 05/10/2012 08:11 AM, Greg Kroah-Hartman wrote:
> >>>>> On Thu, May 10, 2012 at 10:35:02AM +0300, Hiroshi DOYU wrote:
> >>>>>> driver_find_device() can be called with an unregistered driver.
...
> > Can someone resend this to me, with the information above in the
> > changelog comment, and I'll be glad to apply it? Again, I need to know
> > if this is needed for 3.4 or it can wait for 3.5?
>
> I'll let Hiroshi do that.
Greg, here's the update patch with the above explanation. Please take
this one if the following explanation makes sense.
>From d7cf9b53d3bbcce75ae55a9eed03c7e091529e3d Mon Sep 17 00:00:00 2001
From: Hiroshi DOYU <[email protected]>
Date: Wed, 9 May 2012 14:21:03 +0300
Subject: [PATCH 1/1] Driver Core: don't oops with unregistered driver in
driver_find_device()
driver_find_device() can be called with an unregistered driver. Need
to check driver_private to see if it's populated or not, especially
under deferrable probe.
In the case that there are 2 drivers, one depends on the other. With
-EPROBE_DEFER, two drivers can use deferred probe to ensure that their
relative probe order doesn't matter. If dependee driver is probed
first, then the dependant's driver_find_device('dependee')
succeeds. If the dependant is probed first, then the dependant's
driver_find_device('dependee') should return NULL, and the dependant
should get -EPROBE_DEFER. driver_find_device() needs to return NULL if
it's not populated.
Signed-off-by: Hiroshi DOYU <[email protected]>
---
In [PATCHv5 2/3] ARM: tegra: Add SMMU enabler in AHB:
http://article.gmane.org/gmane.linux.ports.tegra/4658
"tegra_ahb_driver" may not be populated when it's called.
For more SMMU/AHB specific discussion, refer to the following thread:
https://lkml.org/lkml/2012/5/10/21
---
drivers/base/driver.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index 3ec3896..207c27d 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -80,7 +80,7 @@ struct device *driver_find_device(struct device_driver *drv,
struct klist_iter i;
struct device *dev;
- if (!drv)
+ if (!drv || !drv->p)
return NULL;
klist_iter_init_node(&drv->p->klist_devices, &i,
--
1.7.5.4