drivers/base/bus.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
From: Zijun Hu <quic_zijuhu@quicinc.com>
For the following function call chain:
API driver_register() -> bus_add_driver() -> driver_attach()
There is an error handling path for driver_attach() returning non-zero
or failure in bus_add_driver(), remove it with below reasons:
- It is impossible for driver_attach() to have failure in bus_add_driver()
For int driver_attach(const struct device_driver *drv), the only factor
which makes it failed is that bus_to_subsys(@drv->bus) is NULL, but
the factor has been excluded by bus_add_driver() before calling it.
- driver_attach() is irrelevant with driver_register(), so the former's
result should not also have an impact on the later.
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
Changes in v2:
- Remove the error handling path instead of WARN_ON() it.
- Correct title and commit message
- Link to v1: https://lore.kernel.org/r/20240915-bus_add_driver_fix-v1-1-ce5cf1f66601@quicinc.com
---
drivers/base/bus.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 657c93c38b0d..54ff92aece92 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -674,7 +674,8 @@ int bus_add_driver(struct device_driver *drv)
if (sp->drivers_autoprobe) {
error = driver_attach(drv);
if (error)
- goto out_del_list;
+ pr_warn("%s: failed to attach driver '%s' to bus '%s'\n",
+ __func__, drv->name, sp->bus->name);
}
error = module_add_driver(drv->owner, drv);
if (error) {
@@ -708,7 +709,6 @@ int bus_add_driver(struct device_driver *drv)
out_detach:
driver_detach(drv);
-out_del_list:
klist_del(&priv->knode_bus);
out_unregister:
kobject_put(&priv->kobj);
---
base-commit: 6a36d828bdef0e02b1e6c12e2160f5b83be6aab5
change-id: 20240915-bus_add_driver_fix-f54841e6a69a
Best regards,
--
Zijun Hu <quic_zijuhu@quicinc.com>On 2024/9/17 14:49, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
>
> For the following function call chain:
> API driver_register() -> bus_add_driver() -> driver_attach()
>
> There is an error handling path for driver_attach() returning non-zero
> or failure in bus_add_driver(), remove it with below reasons:
>
> - It is impossible for driver_attach() to have failure in bus_add_driver()
> For int driver_attach(const struct device_driver *drv), the only factor
> which makes it failed is that bus_to_subsys(@drv->bus) is NULL, but
> the factor has been excluded by bus_add_driver() before calling it.
>
> - driver_attach() is irrelevant with driver_register(), so the former's
> result should not also have an impact on the later.
>
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
> Changes in v2:
> - Remove the error handling path instead of WARN_ON() it.
> - Correct title and commit message
> - Link to v1: https://lore.kernel.org/r/20240915-bus_add_driver_fix-v1-1-ce5cf1f66601@quicinc.com
> ---
> drivers/base/bus.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index 657c93c38b0d..54ff92aece92 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -674,7 +674,8 @@ int bus_add_driver(struct device_driver *drv)
> if (sp->drivers_autoprobe) {
> error = driver_attach(drv);
> if (error)
> - goto out_del_list;
> + pr_warn("%s: failed to attach driver '%s' to bus '%s'\n",
> + __func__, drv->name, sp->bus->name);
driver_attach() has __must_check attribute and this error may be
inconsequential for driver_register(), so give pr_warn() here
> }
> error = module_add_driver(drv->owner, drv);
> if (error) {
> @@ -708,7 +709,6 @@ int bus_add_driver(struct device_driver *drv)
>
> out_detach:
> driver_detach(drv);
> -out_del_list:
> klist_del(&priv->knode_bus);
> out_unregister:
> kobject_put(&priv->kobj);
>
> ---
> base-commit: 6a36d828bdef0e02b1e6c12e2160f5b83be6aab5
> change-id: 20240915-bus_add_driver_fix-f54841e6a69a
>
> Best regards,
On Tue, Sep 17, 2024 at 02:53:32PM +0800, Zijun Hu wrote:
> On 2024/9/17 14:49, Zijun Hu wrote:
> > From: Zijun Hu <quic_zijuhu@quicinc.com>
> >
> > For the following function call chain:
> > API driver_register() -> bus_add_driver() -> driver_attach()
> >
> > There is an error handling path for driver_attach() returning non-zero
> > or failure in bus_add_driver(), remove it with below reasons:
> >
> > - It is impossible for driver_attach() to have failure in bus_add_driver()
> > For int driver_attach(const struct device_driver *drv), the only factor
> > which makes it failed is that bus_to_subsys(@drv->bus) is NULL, but
> > the factor has been excluded by bus_add_driver() before calling it.
> >
> > - driver_attach() is irrelevant with driver_register(), so the former's
> > result should not also have an impact on the later.
> >
> > Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> > ---
> > Changes in v2:
> > - Remove the error handling path instead of WARN_ON() it.
> > - Correct title and commit message
> > - Link to v1: https://lore.kernel.org/r/20240915-bus_add_driver_fix-v1-1-ce5cf1f66601@quicinc.com
> > ---
> > drivers/base/bus.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> > index 657c93c38b0d..54ff92aece92 100644
> > --- a/drivers/base/bus.c
> > +++ b/drivers/base/bus.c
> > @@ -674,7 +674,8 @@ int bus_add_driver(struct device_driver *drv)
> > if (sp->drivers_autoprobe) {
> > error = driver_attach(drv);
> > if (error)
> > - goto out_del_list;
> > + pr_warn("%s: failed to attach driver '%s' to bus '%s'\n",
> > + __func__, drv->name, sp->bus->name);
>
> driver_attach() has __must_check attribute and this error may be
> inconsequential for driver_register(), so give pr_warn() here
Yes, but you now ignore the error, so someone will come back and add
that error handling in. I'd just leave it as-is.
thanks,
greg k-h
On 2024/10/13 23:02, Greg Kroah-Hartman wrote:
> On Tue, Sep 17, 2024 at 02:53:32PM +0800, Zijun Hu wrote:
>> On 2024/9/17 14:49, Zijun Hu wrote:
>>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>>
>>> For the following function call chain:
>>> API driver_register() -> bus_add_driver() -> driver_attach()
>>>
>>> There is an error handling path for driver_attach() returning non-zero
>>> or failure in bus_add_driver(), remove it with below reasons:
>>>
>>> - It is impossible for driver_attach() to have failure in bus_add_driver()
>>> For int driver_attach(const struct device_driver *drv), the only factor
>>> which makes it failed is that bus_to_subsys(@drv->bus) is NULL, but
>>> the factor has been excluded by bus_add_driver() before calling it.
>>>
>>> - driver_attach() is irrelevant with driver_register(), so the former's
>>> result should not also have an impact on the later.
>>>
>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>>> ---
>>> Changes in v2:
>>> - Remove the error handling path instead of WARN_ON() it.
>>> - Correct title and commit message
>>> - Link to v1: https://lore.kernel.org/r/20240915-bus_add_driver_fix-v1-1-ce5cf1f66601@quicinc.com
>>> ---
>>> drivers/base/bus.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
>>> index 657c93c38b0d..54ff92aece92 100644
>>> --- a/drivers/base/bus.c
>>> +++ b/drivers/base/bus.c
>>> @@ -674,7 +674,8 @@ int bus_add_driver(struct device_driver *drv)
>>> if (sp->drivers_autoprobe) {
>>> error = driver_attach(drv);
>>> if (error)
>>> - goto out_del_list;
>>> + pr_warn("%s: failed to attach driver '%s' to bus '%s'\n",
>>> + __func__, drv->name, sp->bus->name);
>>
>> driver_attach() has __must_check attribute and this error may be
>> inconsequential for driver_register(), so give pr_warn() here
>
> Yes, but you now ignore the error, so someone will come back and add
> that error handling in. I'd just leave it as-is.
>
driver API driver_attach() may ONLY have below error -EINVAL.
is it worthy of a __must_check attribute ?
i agree with you to leave it as-is if your answer is "YES".
otherwise, i would like to also simply remove __must_check attribute.
int driver_attach(const struct device_driver *drv)
{
return bus_for_each_dev(drv->bus, NULL, (void *)drv, __driver_attach);
}
int bus_for_each_dev(const struct bus_type *bus, struct device *start,
void *data, int (*fn)(struct device *, void *))
{
struct subsys_private *sp = bus_to_subsys(bus);
...
int error = 0;
if (!sp)
return -EINVAL; // this is the only error for the API.
...
}
> thanks,
>
> greg k-h
On Sun, Oct 13, 2024 at 11:46:46PM +0800, Zijun Hu wrote:
> On 2024/10/13 23:02, Greg Kroah-Hartman wrote:
> > On Tue, Sep 17, 2024 at 02:53:32PM +0800, Zijun Hu wrote:
> >> On 2024/9/17 14:49, Zijun Hu wrote:
> >>> From: Zijun Hu <quic_zijuhu@quicinc.com>
> >>>
> >>> For the following function call chain:
> >>> API driver_register() -> bus_add_driver() -> driver_attach()
> >>>
> >>> There is an error handling path for driver_attach() returning non-zero
> >>> or failure in bus_add_driver(), remove it with below reasons:
> >>>
> >>> - It is impossible for driver_attach() to have failure in bus_add_driver()
> >>> For int driver_attach(const struct device_driver *drv), the only factor
> >>> which makes it failed is that bus_to_subsys(@drv->bus) is NULL, but
> >>> the factor has been excluded by bus_add_driver() before calling it.
> >>>
> >>> - driver_attach() is irrelevant with driver_register(), so the former's
> >>> result should not also have an impact on the later.
> >>>
> >>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> >>> ---
> >>> Changes in v2:
> >>> - Remove the error handling path instead of WARN_ON() it.
> >>> - Correct title and commit message
> >>> - Link to v1: https://lore.kernel.org/r/20240915-bus_add_driver_fix-v1-1-ce5cf1f66601@quicinc.com
> >>> ---
> >>> drivers/base/bus.c | 4 ++--
> >>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> >>> index 657c93c38b0d..54ff92aece92 100644
> >>> --- a/drivers/base/bus.c
> >>> +++ b/drivers/base/bus.c
> >>> @@ -674,7 +674,8 @@ int bus_add_driver(struct device_driver *drv)
> >>> if (sp->drivers_autoprobe) {
> >>> error = driver_attach(drv);
> >>> if (error)
> >>> - goto out_del_list;
> >>> + pr_warn("%s: failed to attach driver '%s' to bus '%s'\n",
> >>> + __func__, drv->name, sp->bus->name);
> >>
> >> driver_attach() has __must_check attribute and this error may be
> >> inconsequential for driver_register(), so give pr_warn() here
> >
> > Yes, but you now ignore the error, so someone will come back and add
> > that error handling in. I'd just leave it as-is.
> >
>
> driver API driver_attach() may ONLY have below error -EINVAL.
> is it worthy of a __must_check attribute ?
Yes.
> i agree with you to leave it as-is if your answer is "YES".
> otherwise, i would like to also simply remove __must_check attribute.
Please don't. If you do that, then callers will end up not checking the
results, and we do not want that.
thanks,
greg k-h
On 2024/10/14 00:00, Greg Kroah-Hartman wrote:
> On Sun, Oct 13, 2024 at 11:46:46PM +0800, Zijun Hu wrote:
>> On 2024/10/13 23:02, Greg Kroah-Hartman wrote:
>>> On Tue, Sep 17, 2024 at 02:53:32PM +0800, Zijun Hu wrote:
>>>> On 2024/9/17 14:49, Zijun Hu wrote:
>>>>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>>>>
>>>>> For the following function call chain:
>>>>> API driver_register() -> bus_add_driver() -> driver_attach()
>>>>>
>>>>> There is an error handling path for driver_attach() returning non-zero
>>>>> or failure in bus_add_driver(), remove it with below reasons:
>>>>>
>>>>> - It is impossible for driver_attach() to have failure in bus_add_driver()
>>>>> For int driver_attach(const struct device_driver *drv), the only factor
>>>>> which makes it failed is that bus_to_subsys(@drv->bus) is NULL, but
>>>>> the factor has been excluded by bus_add_driver() before calling it.
>>>>>
>>>>> - driver_attach() is irrelevant with driver_register(), so the former's
>>>>> result should not also have an impact on the later.
>>>>>
>>>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>>>>> ---
>>>>> Changes in v2:
>>>>> - Remove the error handling path instead of WARN_ON() it.
>>>>> - Correct title and commit message
>>>>> - Link to v1: https://lore.kernel.org/r/20240915-bus_add_driver_fix-v1-1-ce5cf1f66601@quicinc.com
>>>>> ---
>>>>> drivers/base/bus.c | 4 ++--
>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
>>>>> index 657c93c38b0d..54ff92aece92 100644
>>>>> --- a/drivers/base/bus.c
>>>>> +++ b/drivers/base/bus.c
>>>>> @@ -674,7 +674,8 @@ int bus_add_driver(struct device_driver *drv)
>>>>> if (sp->drivers_autoprobe) {
>>>>> error = driver_attach(drv);
>>>>> if (error)
>>>>> - goto out_del_list;
>>>>> + pr_warn("%s: failed to attach driver '%s' to bus '%s'\n",
>>>>> + __func__, drv->name, sp->bus->name);
>>>>
>>>> driver_attach() has __must_check attribute and this error may be
>>>> inconsequential for driver_register(), so give pr_warn() here
>>>
>>> Yes, but you now ignore the error, so someone will come back and add
>>> that error handling in. I'd just leave it as-is.
>>>
>>
>> driver API driver_attach() may ONLY have below error -EINVAL.
>> is it worthy of a __must_check attribute ?
>
> Yes.
>
>> i agree with you to leave it as-is if your answer is "YES".
>> otherwise, i would like to also simply remove __must_check attribute.
>
> Please don't. If you do that, then callers will end up not checking the
> results, and we do not want that.
>
okay.
but as 2nd reason of commit message explained:
driver_attach() failure should NOT cause driver_register() failure, so
should ignore driver_attach() failure here. but driver_attach() has
__must_check attribute, it will has build error if ignore the failure,
so this solution is worked out: if (error) pr_warn().
does this solution meet __must_check attribute ?
> thanks,
>
> greg k-h
On Mon, Oct 14, 2024 at 12:13:30AM +0800, Zijun Hu wrote:
> On 2024/10/14 00:00, Greg Kroah-Hartman wrote:
> > On Sun, Oct 13, 2024 at 11:46:46PM +0800, Zijun Hu wrote:
> >> On 2024/10/13 23:02, Greg Kroah-Hartman wrote:
> >>> On Tue, Sep 17, 2024 at 02:53:32PM +0800, Zijun Hu wrote:
> >>>> On 2024/9/17 14:49, Zijun Hu wrote:
> >>>>> From: Zijun Hu <quic_zijuhu@quicinc.com>
> >>>>>
> >>>>> For the following function call chain:
> >>>>> API driver_register() -> bus_add_driver() -> driver_attach()
> >>>>>
> >>>>> There is an error handling path for driver_attach() returning non-zero
> >>>>> or failure in bus_add_driver(), remove it with below reasons:
> >>>>>
> >>>>> - It is impossible for driver_attach() to have failure in bus_add_driver()
> >>>>> For int driver_attach(const struct device_driver *drv), the only factor
> >>>>> which makes it failed is that bus_to_subsys(@drv->bus) is NULL, but
> >>>>> the factor has been excluded by bus_add_driver() before calling it.
> >>>>>
> >>>>> - driver_attach() is irrelevant with driver_register(), so the former's
> >>>>> result should not also have an impact on the later.
> >>>>>
> >>>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> >>>>> ---
> >>>>> Changes in v2:
> >>>>> - Remove the error handling path instead of WARN_ON() it.
> >>>>> - Correct title and commit message
> >>>>> - Link to v1: https://lore.kernel.org/r/20240915-bus_add_driver_fix-v1-1-ce5cf1f66601@quicinc.com
> >>>>> ---
> >>>>> drivers/base/bus.c | 4 ++--
> >>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> >>>>> index 657c93c38b0d..54ff92aece92 100644
> >>>>> --- a/drivers/base/bus.c
> >>>>> +++ b/drivers/base/bus.c
> >>>>> @@ -674,7 +674,8 @@ int bus_add_driver(struct device_driver *drv)
> >>>>> if (sp->drivers_autoprobe) {
> >>>>> error = driver_attach(drv);
> >>>>> if (error)
> >>>>> - goto out_del_list;
> >>>>> + pr_warn("%s: failed to attach driver '%s' to bus '%s'\n",
> >>>>> + __func__, drv->name, sp->bus->name);
> >>>>
> >>>> driver_attach() has __must_check attribute and this error may be
> >>>> inconsequential for driver_register(), so give pr_warn() here
> >>>
> >>> Yes, but you now ignore the error, so someone will come back and add
> >>> that error handling in. I'd just leave it as-is.
> >>>
> >>
> >> driver API driver_attach() may ONLY have below error -EINVAL.
> >> is it worthy of a __must_check attribute ?
> >
> > Yes.
> >
> >> i agree with you to leave it as-is if your answer is "YES".
> >> otherwise, i would like to also simply remove __must_check attribute.
> >
> > Please don't. If you do that, then callers will end up not checking the
> > results, and we do not want that.
> >
> okay.
>
> but as 2nd reason of commit message explained:
> driver_attach() failure should NOT cause driver_register() failure, so
> should ignore driver_attach() failure here.
How could driver_attach() fail and it still be ok to continue?
thanks,
greg k-h
On 2024/10/14 00:24, Greg Kroah-Hartman wrote:
> On Mon, Oct 14, 2024 at 12:13:30AM +0800, Zijun Hu wrote:
>> On 2024/10/14 00:00, Greg Kroah-Hartman wrote:
>>> On Sun, Oct 13, 2024 at 11:46:46PM +0800, Zijun Hu wrote:
>>>> On 2024/10/13 23:02, Greg Kroah-Hartman wrote:
>>>>> On Tue, Sep 17, 2024 at 02:53:32PM +0800, Zijun Hu wrote:
>>>>>> On 2024/9/17 14:49, Zijun Hu wrote:
>>>>>>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>>>>>>
>>>>>>> For the following function call chain:
>>>>>>> API driver_register() -> bus_add_driver() -> driver_attach()
>>>>>>>
>>>>>>> There is an error handling path for driver_attach() returning non-zero
>>>>>>> or failure in bus_add_driver(), remove it with below reasons:
>>>>>>>
>>>>>>> - It is impossible for driver_attach() to have failure in bus_add_driver()
>>>>>>> For int driver_attach(const struct device_driver *drv), the only factor
>>>>>>> which makes it failed is that bus_to_subsys(@drv->bus) is NULL, but
>>>>>>> the factor has been excluded by bus_add_driver() before calling it.
>>>>>>>
>>>>>>> - driver_attach() is irrelevant with driver_register(), so the former's
>>>>>>> result should not also have an impact on the later.
>>>>>>>
>>>>>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>>>>>>> ---
>>>>>>> Changes in v2:
>>>>>>> - Remove the error handling path instead of WARN_ON() it.
>>>>>>> - Correct title and commit message
>>>>>>> - Link to v1: https://lore.kernel.org/r/20240915-bus_add_driver_fix-v1-1-ce5cf1f66601@quicinc.com
>>>>>>> ---
>>>>>>> drivers/base/bus.c | 4 ++--
>>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
>>>>>>> index 657c93c38b0d..54ff92aece92 100644
>>>>>>> --- a/drivers/base/bus.c
>>>>>>> +++ b/drivers/base/bus.c
>>>>>>> @@ -674,7 +674,8 @@ int bus_add_driver(struct device_driver *drv)
>>>>>>> if (sp->drivers_autoprobe) {
>>>>>>> error = driver_attach(drv);
>>>>>>> if (error)
>>>>>>> - goto out_del_list;
>>>>>>> + pr_warn("%s: failed to attach driver '%s' to bus '%s'\n",
>>>>>>> + __func__, drv->name, sp->bus->name);
>>>>>>
>>>>>> driver_attach() has __must_check attribute and this error may be
>>>>>> inconsequential for driver_register(), so give pr_warn() here
>>>>>
>>>>> Yes, but you now ignore the error, so someone will come back and add
>>>>> that error handling in. I'd just leave it as-is.
>>>>>
>>>>
>>>> driver API driver_attach() may ONLY have below error -EINVAL.
>>>> is it worthy of a __must_check attribute ?
>>>
>>> Yes.
>>>
>>>> i agree with you to leave it as-is if your answer is "YES".
>>>> otherwise, i would like to also simply remove __must_check attribute.
>>>
>>> Please don't. If you do that, then callers will end up not checking the
>>> results, and we do not want that.
>>>
>> okay.
>>
>> but as 2nd reason of commit message explained:
>> driver_attach() failure should NOT cause driver_register() failure, so
>> should ignore driver_attach() failure here.
>
> How could driver_attach() fail and it still be ok to continue?
>
sorry, i am wrong, agree with you to leave it as-is.
driver_attach() failure means driver itself error not attaching it with
devices error. so should cause driver_register() failure even if the
error is impossible here.
thanks.
> thanks,
>
> greg k-h
© 2016 - 2024 Red Hat, Inc.