| Message ID | 20240815-const_dfc_prepare-v2-1-8316b87b8ff9@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | driver core: Prevent device_find_child() from modifying caller's match data | expand |
Zijun Hu wrote: > From: Zijun Hu <quic_zijuhu@quicinc.com> > > The following API cluster takes the same type parameter list, but do not > have consistent parameter check as shown below. > > device_for_each_child(struct device *parent, ...) // check (!parent->p) > device_for_each_child_reverse(struct device *parent, ...) // same as above > device_find_child(struct device *parent, ...) // check (!parent) > Seems reasonable. What about device_find_child_by_name()? > Fixed by using consistent check (!parent || !parent->p) for the cluster. > > Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> > --- > drivers/base/core.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 1688e76cb64b..b1dd8c5590dc 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -4004,7 +4004,7 @@ int device_for_each_child(struct device *parent, void *data, > struct device *child; > int error = 0; > > - if (!parent->p) > + if (!parent || !parent->p) > return 0; > > klist_iter_init(&parent->p->klist_children, &i); > @@ -4034,7 +4034,7 @@ int device_for_each_child_reverse(struct device *parent, void *data, > struct device *child; > int error = 0; > > - if (!parent->p) > + if (!parent || !parent->p) > return 0; > > klist_iter_init(&parent->p->klist_children, &i); > @@ -4068,7 +4068,7 @@ struct device *device_find_child(struct device *parent, void *data, > struct klist_iter i; > struct device *child; > > - if (!parent) > + if (!parent || !parent->p) Perhaps this was just a typo which should have been. if (!parent->p) ? I think there is an expectation that none of these are called with a NULL parent. Ira > return NULL; > > klist_iter_init(&parent->p->klist_children, &i); > > -- > 2.34.1 >
On 2024/8/20 20:53, Ira Weiny wrote: > Zijun Hu wrote: >> From: Zijun Hu <quic_zijuhu@quicinc.com> >> >> The following API cluster takes the same type parameter list, but do not >> have consistent parameter check as shown below. >> >> device_for_each_child(struct device *parent, ...) // check (!parent->p) >> device_for_each_child_reverse(struct device *parent, ...) // same as above >> device_find_child(struct device *parent, ...) // check (!parent) >> > > Seems reasonable. > > What about device_find_child_by_name()? > Plan to simplify this API implementation by * atomic * API device_find_child() as following: https://lore.kernel.org/all/20240811-simply_api_dfcbn-v2-1-d0398acdc366@quicinc.com struct device *device_find_child_by_name(struct device *parent, const char *name) { return device_find_child(parent, name, device_match_name); } >> Fixed by using consistent check (!parent || !parent->p) for the cluster. >> >> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> >> --- >> drivers/base/core.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/base/core.c b/drivers/base/core.c >> index 1688e76cb64b..b1dd8c5590dc 100644 >> --- a/drivers/base/core.c >> +++ b/drivers/base/core.c >> @@ -4004,7 +4004,7 @@ int device_for_each_child(struct device *parent, void *data, >> struct device *child; >> int error = 0; >> >> - if (!parent->p) >> + if (!parent || !parent->p) >> return 0; >> >> klist_iter_init(&parent->p->klist_children, &i); >> @@ -4034,7 +4034,7 @@ int device_for_each_child_reverse(struct device *parent, void *data, >> struct device *child; >> int error = 0; >> >> - if (!parent->p) >> + if (!parent || !parent->p) >> return 0; >> >> klist_iter_init(&parent->p->klist_children, &i); >> @@ -4068,7 +4068,7 @@ struct device *device_find_child(struct device *parent, void *data, >> struct klist_iter i; >> struct device *child; >> >> - if (!parent) >> + if (!parent || !parent->p) > > Perhaps this was just a typo which should have been. > > if (!parent->p) > ? > maybe, but the following device_find_child_by_name() also use (!parent). > I think there is an expectation that none of these are called with a NULL > parent. > this patch aim is to make these atomic APIs have consistent checks as far as possible, that will make other patches within this series more acceptable. i combine two checks to (!parent || !parent->p) since i did not know which is better. > Ira > >> return NULL; >> >> klist_iter_init(&parent->p->klist_children, &i); >> >> -- >> 2.34.1 >> > >
Zijun Hu wrote: > On 2024/8/20 20:53, Ira Weiny wrote: > > Zijun Hu wrote: > >> From: Zijun Hu <quic_zijuhu@quicinc.com> > >> > >> The following API cluster takes the same type parameter list, but do not > >> have consistent parameter check as shown below. > >> > >> device_for_each_child(struct device *parent, ...) // check (!parent->p) > >> device_for_each_child_reverse(struct device *parent, ...) // same as above > >> device_find_child(struct device *parent, ...) // check (!parent) > >> > > > > Seems reasonable. > > > > What about device_find_child_by_name()? > > > > Plan to simplify this API implementation by * atomic * API > device_find_child() as following: > > https://lore.kernel.org/all/20240811-simply_api_dfcbn-v2-1-d0398acdc366@quicinc.com > struct device *device_find_child_by_name(struct device *parent, > const char *name) > { > return device_find_child(parent, name, device_match_name); > } Ok. Thanks. > > >> Fixed by using consistent check (!parent || !parent->p) for the cluster. > >> > >> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> > >> --- > >> drivers/base/core.c | 6 +++--- > >> 1 file changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/base/core.c b/drivers/base/core.c > >> index 1688e76cb64b..b1dd8c5590dc 100644 > >> --- a/drivers/base/core.c > >> +++ b/drivers/base/core.c > >> @@ -4004,7 +4004,7 @@ int device_for_each_child(struct device *parent, void *data, > >> struct device *child; > >> int error = 0; > >> > >> - if (!parent->p) > >> + if (!parent || !parent->p) > >> return 0; > >> > >> klist_iter_init(&parent->p->klist_children, &i); > >> @@ -4034,7 +4034,7 @@ int device_for_each_child_reverse(struct device *parent, void *data, > >> struct device *child; > >> int error = 0; > >> > >> - if (!parent->p) > >> + if (!parent || !parent->p) > >> return 0; > >> > >> klist_iter_init(&parent->p->klist_children, &i); > >> @@ -4068,7 +4068,7 @@ struct device *device_find_child(struct device *parent, void *data, > >> struct klist_iter i; > >> struct device *child; > >> > >> - if (!parent) > >> + if (!parent || !parent->p) > > > > Perhaps this was just a typo which should have been. > > > > if (!parent->p) > > ? > > > maybe, but the following device_find_child_by_name() also use (!parent). > > > I think there is an expectation that none of these are called with a NULL > > parent. > > > > this patch aim is to make these atomic APIs have consistent checks as > far as possible, that will make other patches within this series more > acceptable. > > i combine two checks to (!parent || !parent->p) since i did not know > which is better. I'm not entirely clear either. But checking the member p makes more sense to me than the parent parameter. I would expect that iterating the children of a device must be done only when the parent device is not NULL. parent->p is more subtle. I'm unclear why the API would need to allow that to run without error. Ira
On 2024/8/20 22:14, Ira Weiny wrote: > Zijun Hu wrote: >> On 2024/8/20 20:53, Ira Weiny wrote: >>> Zijun Hu wrote: >>>> From: Zijun Hu <quic_zijuhu@quicinc.com> >>>> >>>> The following API cluster takes the same type parameter list, but do not >>>> have consistent parameter check as shown below. >>>> >>>> device_for_each_child(struct device *parent, ...) // check (!parent->p) >>>> device_for_each_child_reverse(struct device *parent, ...) // same as above >>>> device_find_child(struct device *parent, ...) // check (!parent) >>>> >>> >>> Seems reasonable. >>> >>> What about device_find_child_by_name()? >>> >> >> Plan to simplify this API implementation by * atomic * API >> device_find_child() as following: >> >> https://lore.kernel.org/all/20240811-simply_api_dfcbn-v2-1-d0398acdc366@quicinc.com >> struct device *device_find_child_by_name(struct device *parent, >> const char *name) >> { >> return device_find_child(parent, name, device_match_name); >> } > > Ok. Thanks. > >> >>>> Fixed by using consistent check (!parent || !parent->p) for the cluster. >>>> >>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> >>>> --- >>>> drivers/base/core.c | 6 +++--- >>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/base/core.c b/drivers/base/core.c >>>> index 1688e76cb64b..b1dd8c5590dc 100644 >>>> --- a/drivers/base/core.c >>>> +++ b/drivers/base/core.c >>>> @@ -4004,7 +4004,7 @@ int device_for_each_child(struct device *parent, void *data, >>>> struct device *child; >>>> int error = 0; >>>> >>>> - if (!parent->p) >>>> + if (!parent || !parent->p) >>>> return 0; >>>> >>>> klist_iter_init(&parent->p->klist_children, &i); >>>> @@ -4034,7 +4034,7 @@ int device_for_each_child_reverse(struct device *parent, void *data, >>>> struct device *child; >>>> int error = 0; >>>> >>>> - if (!parent->p) >>>> + if (!parent || !parent->p) >>>> return 0; >>>> >>>> klist_iter_init(&parent->p->klist_children, &i); >>>> @@ -4068,7 +4068,7 @@ struct device *device_find_child(struct device *parent, void *data, >>>> struct klist_iter i; >>>> struct device *child; >>>> >>>> - if (!parent) >>>> + if (!parent || !parent->p) >>> >>> Perhaps this was just a typo which should have been. >>> >>> if (!parent->p) >>> ? >>> >> maybe, but the following device_find_child_by_name() also use (!parent). >> >>> I think there is an expectation that none of these are called with a NULL >>> parent. >>> >> >> this patch aim is to make these atomic APIs have consistent checks as >> far as possible, that will make other patches within this series more >> acceptable. >> >> i combine two checks to (!parent || !parent->p) since i did not know >> which is better. > > I'm not entirely clear either. But checking the member p makes more sense > to me than the parent parameter. I would expect that iterating the > children of a device must be done only when the parent device is not NULL. > > parent->p is more subtle. I'm unclear why the API would need to allow > that to run without error. > i prefer (!parent || !parent->p) with below reasons: 1) original API authors have such concern that either (!parent) or (!parent->p) maybe happen since they are checked, all their concerns can be covered by (!parent || !parent->p). 2) It is the more robust than either (!parent) or (!parent->p) 3) it also does not have any negative effect. > Ira
Zijun Hu wrote: > On 2024/8/20 22:14, Ira Weiny wrote: > > Zijun Hu wrote: > >> On 2024/8/20 20:53, Ira Weiny wrote: > >>> Zijun Hu wrote: > >>>> From: Zijun Hu <quic_zijuhu@quicinc.com> > >>>> > >>>> The following API cluster takes the same type parameter list, but do not > >>>> have consistent parameter check as shown below. > >>>> > >>>> device_for_each_child(struct device *parent, ...) // check (!parent->p) > >>>> device_for_each_child_reverse(struct device *parent, ...) // same as above > >>>> device_find_child(struct device *parent, ...) // check (!parent) > >>>> > >>> > >>> Seems reasonable. > >>> > >>> What about device_find_child_by_name()? > >>> > >> > >> Plan to simplify this API implementation by * atomic * API > >> device_find_child() as following: > >> > >> https://lore.kernel.org/all/20240811-simply_api_dfcbn-v2-1-d0398acdc366@quicinc.com > >> struct device *device_find_child_by_name(struct device *parent, > >> const char *name) > >> { > >> return device_find_child(parent, name, device_match_name); > >> } > > > > Ok. Thanks. > > > >> > >>>> Fixed by using consistent check (!parent || !parent->p) for the cluster. > >>>> > >>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> > >>>> --- > >>>> drivers/base/core.c | 6 +++--- > >>>> 1 file changed, 3 insertions(+), 3 deletions(-) > >>>> > >>>> diff --git a/drivers/base/core.c b/drivers/base/core.c > >>>> index 1688e76cb64b..b1dd8c5590dc 100644 > >>>> --- a/drivers/base/core.c > >>>> +++ b/drivers/base/core.c > >>>> @@ -4004,7 +4004,7 @@ int device_for_each_child(struct device *parent, void *data, > >>>> struct device *child; > >>>> int error = 0; > >>>> > >>>> - if (!parent->p) > >>>> + if (!parent || !parent->p) > >>>> return 0; > >>>> > >>>> klist_iter_init(&parent->p->klist_children, &i); > >>>> @@ -4034,7 +4034,7 @@ int device_for_each_child_reverse(struct device *parent, void *data, > >>>> struct device *child; > >>>> int error = 0; > >>>> > >>>> - if (!parent->p) > >>>> + if (!parent || !parent->p) > >>>> return 0; > >>>> > >>>> klist_iter_init(&parent->p->klist_children, &i); > >>>> @@ -4068,7 +4068,7 @@ struct device *device_find_child(struct device *parent, void *data, > >>>> struct klist_iter i; > >>>> struct device *child; > >>>> > >>>> - if (!parent) > >>>> + if (!parent || !parent->p) > >>> > >>> Perhaps this was just a typo which should have been. > >>> > >>> if (!parent->p) > >>> ? > >>> > >> maybe, but the following device_find_child_by_name() also use (!parent). > >> > >>> I think there is an expectation that none of these are called with a NULL > >>> parent. > >>> > >> > >> this patch aim is to make these atomic APIs have consistent checks as > >> far as possible, that will make other patches within this series more > >> acceptable. > >> > >> i combine two checks to (!parent || !parent->p) since i did not know > >> which is better. > > > > I'm not entirely clear either. But checking the member p makes more sense > > to me than the parent parameter. I would expect that iterating the > > children of a device must be done only when the parent device is not NULL. > > > > parent->p is more subtle. I'm unclear why the API would need to allow > > that to run without error. > > > i prefer (!parent || !parent->p) with below reasons: > > 1) > original API authors have such concern that either (!parent) or > (!parent->p) maybe happen since they are checked, all their concerns > can be covered by (!parent || !parent->p). > > 2) > It is the more robust than either (!parent) or (!parent->p) > > 3) > it also does not have any negative effect. It adds code and instructions to all paths calling these functions. What is the reason to allow? void foo() { ... device_for_each_child(NULL, ...); ... } What are we finding the child of in that case? Ira > > > Ira >
On 2024/8/24 01:19, Ira Weiny wrote: > Zijun Hu wrote: >> On 2024/8/20 22:14, Ira Weiny wrote: >>> Zijun Hu wrote: >>>> On 2024/8/20 20:53, Ira Weiny wrote: >>>>> Zijun Hu wrote: >>>>>> From: Zijun Hu <quic_zijuhu@quicinc.com> >>>>>> >>>>>> The following API cluster takes the same type parameter list, but do not >>>>>> have consistent parameter check as shown below. >>>>>> >>>>>> device_for_each_child(struct device *parent, ...) // check (!parent->p) >>>>>> device_for_each_child_reverse(struct device *parent, ...) // same as above >>>>>> device_find_child(struct device *parent, ...) // check (!parent) >>>>>> >>>>> >>>>> Seems reasonable. >>>>> >>>>> What about device_find_child_by_name()? >>>>> >>>> >>>> Plan to simplify this API implementation by * atomic * API >>>> device_find_child() as following: >>>> >>>> https://lore.kernel.org/all/20240811-simply_api_dfcbn-v2-1-d0398acdc366@quicinc.com >>>> struct device *device_find_child_by_name(struct device *parent, >>>> const char *name) >>>> { >>>> return device_find_child(parent, name, device_match_name); >>>> } >>> >>> Ok. Thanks. >>> >>>> >>>>>> Fixed by using consistent check (!parent || !parent->p) for the cluster. >>>>>> >>>>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> >>>>>> --- >>>>>> drivers/base/core.c | 6 +++--- >>>>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/drivers/base/core.c b/drivers/base/core.c >>>>>> index 1688e76cb64b..b1dd8c5590dc 100644 >>>>>> --- a/drivers/base/core.c >>>>>> +++ b/drivers/base/core.c >>>>>> @@ -4004,7 +4004,7 @@ int device_for_each_child(struct device *parent, void *data, >>>>>> struct device *child; >>>>>> int error = 0; >>>>>> >>>>>> - if (!parent->p) >>>>>> + if (!parent || !parent->p) >>>>>> return 0; >>>>>> >>>>>> klist_iter_init(&parent->p->klist_children, &i); >>>>>> @@ -4034,7 +4034,7 @@ int device_for_each_child_reverse(struct device *parent, void *data, >>>>>> struct device *child; >>>>>> int error = 0; >>>>>> >>>>>> - if (!parent->p) >>>>>> + if (!parent || !parent->p) >>>>>> return 0; >>>>>> >>>>>> klist_iter_init(&parent->p->klist_children, &i); >>>>>> @@ -4068,7 +4068,7 @@ struct device *device_find_child(struct device *parent, void *data, >>>>>> struct klist_iter i; >>>>>> struct device *child; >>>>>> >>>>>> - if (!parent) >>>>>> + if (!parent || !parent->p) >>>>> >>>>> Perhaps this was just a typo which should have been. >>>>> >>>>> if (!parent->p) >>>>> ? >>>>> >>>> maybe, but the following device_find_child_by_name() also use (!parent). >>>> >>>>> I think there is an expectation that none of these are called with a NULL >>>>> parent. >>>>> >>>> >>>> this patch aim is to make these atomic APIs have consistent checks as >>>> far as possible, that will make other patches within this series more >>>> acceptable. >>>> >>>> i combine two checks to (!parent || !parent->p) since i did not know >>>> which is better. >>> >>> I'm not entirely clear either. But checking the member p makes more sense >>> to me than the parent parameter. I would expect that iterating the >>> children of a device must be done only when the parent device is not NULL. >>> >>> parent->p is more subtle. I'm unclear why the API would need to allow >>> that to run without error. >>> >> i prefer (!parent || !parent->p) with below reasons: >> >> 1) >> original API authors have such concern that either (!parent) or >> (!parent->p) maybe happen since they are checked, all their concerns >> can be covered by (!parent || !parent->p). >> >> 2) >> It is the more robust than either (!parent) or (!parent->p) >> >> 3) >> it also does not have any negative effect. > > It adds code and instructions to all paths calling these functions. > such slight impacts can be ignored if a machine run linux OS. right? > What is the reason to allow? > 1) it allow to use device_for_each_child() without misgiving. 2) there are many many existing APIs which have similar checks such as get_device(), kfree()... > void foo() { > ... > device_for_each_child(NULL, ...); > ... > } > > What are we finding the child of in that case? > similar usage as device_find_child(NULL, ...) which have check (!parent). both device_for_each_child() and device_find_child() iterates over its child. original author's concern (!parent->p) for device_for_each_child() is applicable for the other. original author's concern (!parent) for device_find_child() is applicable for the other as well. so i use (!parent || !parent->p). > Ira > >> >>> Ira >> > >
On Wed, Aug 21, 2024 at 10:44:27PM +0800, Zijun Hu wrote: > On 2024/8/20 22:14, Ira Weiny wrote: > > Zijun Hu wrote: > >> On 2024/8/20 20:53, Ira Weiny wrote: > >>> Zijun Hu wrote: > >>>> From: Zijun Hu <quic_zijuhu@quicinc.com> > >>>> > >>>> The following API cluster takes the same type parameter list, but do not > >>>> have consistent parameter check as shown below. > >>>> > >>>> device_for_each_child(struct device *parent, ...) // check (!parent->p) > >>>> device_for_each_child_reverse(struct device *parent, ...) // same as above > >>>> device_find_child(struct device *parent, ...) // check (!parent) > >>>> > >>> > >>> Seems reasonable. > >>> > >>> What about device_find_child_by_name()? > >>> > >> > >> Plan to simplify this API implementation by * atomic * API > >> device_find_child() as following: > >> > >> https://lore.kernel.org/all/20240811-simply_api_dfcbn-v2-1-d0398acdc366@quicinc.com > >> struct device *device_find_child_by_name(struct device *parent, > >> const char *name) > >> { > >> return device_find_child(parent, name, device_match_name); > >> } > > > > Ok. Thanks. > > > >> > >>>> Fixed by using consistent check (!parent || !parent->p) for the cluster. > >>>> > >>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> > >>>> --- > >>>> drivers/base/core.c | 6 +++--- > >>>> 1 file changed, 3 insertions(+), 3 deletions(-) > >>>> > >>>> diff --git a/drivers/base/core.c b/drivers/base/core.c > >>>> index 1688e76cb64b..b1dd8c5590dc 100644 > >>>> --- a/drivers/base/core.c > >>>> +++ b/drivers/base/core.c > >>>> @@ -4004,7 +4004,7 @@ int device_for_each_child(struct device *parent, void *data, > >>>> struct device *child; > >>>> int error = 0; > >>>> > >>>> - if (!parent->p) > >>>> + if (!parent || !parent->p) > >>>> return 0; > >>>> > >>>> klist_iter_init(&parent->p->klist_children, &i); > >>>> @@ -4034,7 +4034,7 @@ int device_for_each_child_reverse(struct device *parent, void *data, > >>>> struct device *child; > >>>> int error = 0; > >>>> > >>>> - if (!parent->p) > >>>> + if (!parent || !parent->p) > >>>> return 0; > >>>> > >>>> klist_iter_init(&parent->p->klist_children, &i); > >>>> @@ -4068,7 +4068,7 @@ struct device *device_find_child(struct device *parent, void *data, > >>>> struct klist_iter i; > >>>> struct device *child; > >>>> > >>>> - if (!parent) > >>>> + if (!parent || !parent->p) > >>> > >>> Perhaps this was just a typo which should have been. > >>> > >>> if (!parent->p) > >>> ? > >>> > >> maybe, but the following device_find_child_by_name() also use (!parent). > >> > >>> I think there is an expectation that none of these are called with a NULL > >>> parent. > >>> > >> > >> this patch aim is to make these atomic APIs have consistent checks as > >> far as possible, that will make other patches within this series more > >> acceptable. > >> > >> i combine two checks to (!parent || !parent->p) since i did not know > >> which is better. > > > > I'm not entirely clear either. But checking the member p makes more sense > > to me than the parent parameter. I would expect that iterating the > > children of a device must be done only when the parent device is not NULL. > > > > parent->p is more subtle. I'm unclear why the API would need to allow > > that to run without error. > > > i prefer (!parent || !parent->p) with below reasons: > > 1) > original API authors have such concern that either (!parent) or > (!parent->p) maybe happen since they are checked, all their concerns > can be covered by (!parent || !parent->p). Wait, a device's parent can NOT be NULL except for some special cases when it is being created. And the ->p check is for internal stuff, meaning it has been initialized and registered properly, again, these functions should never be called for a device that this has not happened on. So if they are, crashing is fine as this should never have gotten through a development cycle. The ->p checks were added way after the initial driver core was created, as evolution of moving things out of struct device to prevent drivers from touching those fields. They were added add-hoc where needed and probably not everywhere as you are finding out. By adding these "robust" checks, we are making it harder for new code to be written at the expense of doing checks that we "know" are never going to happen in normal operation. It's a trade off. Only add them when you KNOW they will be needed please. thanks, greg k-h
On Thu, Aug 15, 2024 at 10:58:02PM +0800, Zijun Hu wrote: > From: Zijun Hu <quic_zijuhu@quicinc.com> > > The following API cluster takes the same type parameter list, but do not > have consistent parameter check as shown below. > > device_for_each_child(struct device *parent, ...) // check (!parent->p) > device_for_each_child_reverse(struct device *parent, ...) // same as above > device_find_child(struct device *parent, ...) // check (!parent) > > Fixed by using consistent check (!parent || !parent->p) for the cluster. > > Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> > --- > drivers/base/core.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 1688e76cb64b..b1dd8c5590dc 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -4004,7 +4004,7 @@ int device_for_each_child(struct device *parent, void *data, > struct device *child; > int error = 0; > > - if (!parent->p) > + if (!parent || !parent->p) > return 0; > > klist_iter_init(&parent->p->klist_children, &i); > @@ -4034,7 +4034,7 @@ int device_for_each_child_reverse(struct device *parent, void *data, > struct device *child; > int error = 0; > > - if (!parent->p) > + if (!parent || !parent->p) > return 0; > > klist_iter_init(&parent->p->klist_children, &i); > @@ -4068,7 +4068,7 @@ struct device *device_find_child(struct device *parent, void *data, > struct klist_iter i; > struct device *child; > > - if (!parent) > + if (!parent || !parent->p) > return NULL; My review comments that I just made previously were more "generic", sorry. This change looks fine, there's no additional runtime overhead for doing this check as we already have to dereference the pointer, so might as well be consistant. I'll go queue this up next week when I get back from conference travel. thanks, greg k-h
diff --git a/drivers/base/core.c b/drivers/base/core.c index 1688e76cb64b..b1dd8c5590dc 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -4004,7 +4004,7 @@ int device_for_each_child(struct device *parent, void *data, struct device *child; int error = 0; - if (!parent->p) + if (!parent || !parent->p) return 0; klist_iter_init(&parent->p->klist_children, &i); @@ -4034,7 +4034,7 @@ int device_for_each_child_reverse(struct device *parent, void *data, struct device *child; int error = 0; - if (!parent->p) + if (!parent || !parent->p) return 0; klist_iter_init(&parent->p->klist_children, &i); @@ -4068,7 +4068,7 @@ struct device *device_find_child(struct device *parent, void *data, struct klist_iter i; struct device *child; - if (!parent) + if (!parent || !parent->p) return NULL; klist_iter_init(&parent->p->klist_children, &i);