When sysfs_create_file() fails, recommend to print the related failure
information. And it is useless to still 'KOBJ_ADD' to user space.
Signed-off-by: Chen Gang <[email protected]>
---
kernel/params.c | 8 +++++++-
1 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/kernel/params.c b/kernel/params.c
index 440e65d..f5299c1 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -845,7 +845,13 @@ static void __init version_sysfs_builtin(void)
mk = locate_module_kobject(vattr->module_name);
if (mk) {
err = sysfs_create_file(&mk->kobj, &vattr->mattr.attr);
- kobject_uevent(&mk->kobj, KOBJ_ADD);
+ if (err)
+ printk(KERN_WARNING
+ "%s (%d): sysfs_create_file fail for %s, err: %d\n",
+ __FILE__, __LINE__,
+ vattr->module_name, err);
+ else
+ kobject_uevent(&mk->kobj, KOBJ_ADD);
kobject_put(&mk->kobj);
}
}
--
1.7.7.6
Chen Gang <[email protected]> writes:
> When sysfs_create_file() fails, recommend to print the related failure
> information. And it is useless to still 'KOBJ_ADD' to user space.
>
> Signed-off-by: Chen Gang <[email protected]>
sysfs_create_file() should not fail during boot, should it?
Cheers,
Rusty.
> ---
> kernel/params.c | 8 +++++++-
> 1 files changed, 7 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/params.c b/kernel/params.c
> index 440e65d..f5299c1 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -845,7 +845,13 @@ static void __init version_sysfs_builtin(void)
> mk = locate_module_kobject(vattr->module_name);
> if (mk) {
> err = sysfs_create_file(&mk->kobj, &vattr->mattr.attr);
> - kobject_uevent(&mk->kobj, KOBJ_ADD);
> + if (err)
> + printk(KERN_WARNING
> + "%s (%d): sysfs_create_file fail for %s, err: %d\n",
> + __FILE__, __LINE__,
> + vattr->module_name, err);
> + else
> + kobject_uevent(&mk->kobj, KOBJ_ADD);
> kobject_put(&mk->kobj);
> }
> }
> --
> 1.7.7.6
On 07/09/2013 04:07 PM, Rusty Russell wrote:
> Chen Gang <[email protected]> writes:
>> When sysfs_create_file() fails, recommend to print the related failure
>> information. And it is useless to still 'KOBJ_ADD' to user space.
>>
>> Signed-off-by: Chen Gang <[email protected]>
>
> sysfs_create_file() should not fail during boot, should it?
>
Hmm..., please reference locate_module_kobject() in "kernel/params.c",
which is an '__init' function, and also call sysfs_create_file(), it
processes the related error.
So I recommend to get the check too in version_sysfs_builtin().
Thanks.
> Cheers,
> Rusty.
>
>> ---
>> kernel/params.c | 8 +++++++-
>> 1 files changed, 7 insertions(+), 1 deletions(-)
>>
>> diff --git a/kernel/params.c b/kernel/params.c
>> index 440e65d..f5299c1 100644
>> --- a/kernel/params.c
>> +++ b/kernel/params.c
>> @@ -845,7 +845,13 @@ static void __init version_sysfs_builtin(void)
>> mk = locate_module_kobject(vattr->module_name);
>> if (mk) {
>> err = sysfs_create_file(&mk->kobj, &vattr->mattr.attr);
>> - kobject_uevent(&mk->kobj, KOBJ_ADD);
>> + if (err)
>> + printk(KERN_WARNING
>> + "%s (%d): sysfs_create_file fail for %s, err: %d\n",
>> + __FILE__, __LINE__,
>> + vattr->module_name, err);
>> + else
>> + kobject_uevent(&mk->kobj, KOBJ_ADD);
>> kobject_put(&mk->kobj);
>> }
>> }
>> --
>> 1.7.7.6
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
--
Chen Gang
On 07/10/2013 10:17 AM, Chen Gang F T wrote:
> On 07/09/2013 04:07 PM, Rusty Russell wrote:
>> Chen Gang <[email protected]> writes:
>>> When sysfs_create_file() fails, recommend to print the related failure
>>> information. And it is useless to still 'KOBJ_ADD' to user space.
>>>
>>> Signed-off-by: Chen Gang <[email protected]>
>>
>> sysfs_create_file() should not fail during boot, should it?
>>
>
> Hmm..., please reference locate_module_kobject() in "kernel/params.c",
> which is an '__init' function, and also call sysfs_create_file(), it
> processes the related error.
>
> So I recommend to get the check too in version_sysfs_builtin().
>
Oh, also for locate_module_kobject(), if !CONFIG_MODULES, when error
occurs, it still print the information about "Adding module".
Hmm..., do we need call kobject_get() before kobject_put() in failure
processing block ?
740 mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL);
741 BUG_ON(!mk);
742
743 mk->mod = THIS_MODULE;
744 mk->kobj.kset = module_kset;
745 err = kobject_init_and_add(&mk->kobj, &module_ktype, NULL,
746 "%s", name);
747 #ifdef CONFIG_MODULES
748 if (!err)
749 err = sysfs_create_file(&mk->kobj, &module_uevent.attr);
750 #endif
751 if (err) {
752 kobject_put(&mk->kobj);
753 pr_crit("Adding module '%s' to sysfs failed (%d), the system may be unstable.\n",
754 name, err);
755 return NULL;
756 }
757
758 /* So that we hold reference in both cases. */
759 kobject_get(&mk->kobj);
760 }
761
762 return mk;
763 }
> Thanks.
>
>> Cheers,
>> Rusty.
>>
>>> ---
>>> kernel/params.c | 8 +++++++-
>>> 1 files changed, 7 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/kernel/params.c b/kernel/params.c
>>> index 440e65d..f5299c1 100644
>>> --- a/kernel/params.c
>>> +++ b/kernel/params.c
>>> @@ -845,7 +845,13 @@ static void __init version_sysfs_builtin(void)
>>> mk = locate_module_kobject(vattr->module_name);
>>> if (mk) {
>>> err = sysfs_create_file(&mk->kobj, &vattr->mattr.attr);
>>> - kobject_uevent(&mk->kobj, KOBJ_ADD);
>>> + if (err)
>>> + printk(KERN_WARNING
>>> + "%s (%d): sysfs_create_file fail for %s, err: %d\n",
>>> + __FILE__, __LINE__,
>>> + vattr->module_name, err);
>>> + else
>>> + kobject_uevent(&mk->kobj, KOBJ_ADD);
>>> kobject_put(&mk->kobj);
>>> }
>>> }
>>> --
>>> 1.7.7.6
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
>>
>
>
--
Chen Gang
On 07/10/2013 10:35 AM, Chen Gang wrote:
> On 07/10/2013 10:17 AM, Chen Gang F T wrote:
>> On 07/09/2013 04:07 PM, Rusty Russell wrote:
>>> Chen Gang <[email protected]> writes:
>>>> When sysfs_create_file() fails, recommend to print the related failure
>>>> information. And it is useless to still 'KOBJ_ADD' to user space.
>>>>
>>>> Signed-off-by: Chen Gang <[email protected]>
>>>
>>> sysfs_create_file() should not fail during boot, should it?
>>>
>>
>> Hmm..., please reference locate_module_kobject() in "kernel/params.c",
>> which is an '__init' function, and also call sysfs_create_file(), it
>> processes the related error.
>>
>> So I recommend to get the check too in version_sysfs_builtin().
>>
>
> Oh, also for locate_module_kobject(), if !CONFIG_MODULES, when error
> occurs, it still print the information about "Adding module".
>
> Hmm..., do we need call kobject_get() before kobject_put() in failure
> processing block ?
>
Oh, sorry for what I said for kobject_get/put() items above, it is
incorrect.
What about the diff below for kobject_get() ?
-------------------------------diff begin-------------------------------
diff --git a/kernel/params.c b/kernel/params.c
index 440e65d..ef8d720 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -754,11 +754,11 @@ static struct module_kobject * __init locate_module_kobject(const char *name)
name, err);
return NULL;
}
-
- /* So that we hold reference in both cases. */
- kobject_get(&mk->kobj);
}
+ /* So that we hold reference in both cases. */
+ kobject_get(&mk->kobj);
+
return mk;
}
-------------------------------diff end---------------------------------
And it also need add additional kobject_put(), if we really need
process the failure in version_sysfs_builtin().
Thanks.
>
> 740 mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL);
> 741 BUG_ON(!mk);
> 742
> 743 mk->mod = THIS_MODULE;
> 744 mk->kobj.kset = module_kset;
> 745 err = kobject_init_and_add(&mk->kobj, &module_ktype, NULL,
> 746 "%s", name);
> 747 #ifdef CONFIG_MODULES
> 748 if (!err)
> 749 err = sysfs_create_file(&mk->kobj, &module_uevent.attr);
> 750 #endif
> 751 if (err) {
> 752 kobject_put(&mk->kobj);
> 753 pr_crit("Adding module '%s' to sysfs failed (%d), the system may be unstable.\n",
> 754 name, err);
> 755 return NULL;
> 756 }
> 757
> 758 /* So that we hold reference in both cases. */
> 759 kobject_get(&mk->kobj);
> 760 }
> 761
> 762 return mk;
> 763 }
>
>
>> Thanks.
>>
>>> Cheers,
>>> Rusty.
>>>
>>>> ---
>>>> kernel/params.c | 8 +++++++-
>>>> 1 files changed, 7 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/kernel/params.c b/kernel/params.c
>>>> index 440e65d..f5299c1 100644
>>>> --- a/kernel/params.c
>>>> +++ b/kernel/params.c
>>>> @@ -845,7 +845,13 @@ static void __init version_sysfs_builtin(void)
>>>> mk = locate_module_kobject(vattr->module_name);
>>>> if (mk) {
>>>> err = sysfs_create_file(&mk->kobj, &vattr->mattr.attr);
>>>> - kobject_uevent(&mk->kobj, KOBJ_ADD);
>>>> + if (err)
>>>> + printk(KERN_WARNING
>>>> + "%s (%d): sysfs_create_file fail for %s, err: %d\n",
>>>> + __FILE__, __LINE__,
>>>> + vattr->module_name, err);
>>>> + else
>>>> + kobject_uevent(&mk->kobj, KOBJ_ADD);
>>>> kobject_put(&mk->kobj);
>>>> }
>>>> }
>>>> --
>>>> 1.7.7.6
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at http://www.tux.org/lkml/
>>>
>>
>>
>
>
--
Chen Gang
Chen Gang F T <[email protected]> writes:
> On 07/09/2013 04:07 PM, Rusty Russell wrote:
>> Chen Gang <[email protected]> writes:
>>> When sysfs_create_file() fails, recommend to print the related failure
>>> information. And it is useless to still 'KOBJ_ADD' to user space.
>>>
>>> Signed-off-by: Chen Gang <[email protected]>
>>
>> sysfs_create_file() should not fail during boot, should it?
>>
>
> Hmm..., please reference locate_module_kobject() in "kernel/params.c",
> which is an '__init' function, and also call sysfs_create_file(), it
> processes the related error.
>
> So I recommend to get the check too in version_sysfs_builtin().
It still can't fail. sysfs_create_file() can fail due to OOM (not at
boot) or name duplication (not here).
You can BUG_ON() if you want.
And feel free to fix locate_module_kobject() in a separate patch.
Cheers,
Rusty.
On 07/11/2013 09:53 AM, Rusty Russell wrote:
> Chen Gang F T <[email protected]> writes:
>> > On 07/09/2013 04:07 PM, Rusty Russell wrote:
>>> >> Chen Gang <[email protected]> writes:
>>>> >>> When sysfs_create_file() fails, recommend to print the related failure
>>>> >>> information. And it is useless to still 'KOBJ_ADD' to user space.
>>>> >>>
>>>> >>> Signed-off-by: Chen Gang <[email protected]>
>>> >>
>>> >> sysfs_create_file() should not fail during boot, should it?
>>> >>
>> >
>> > Hmm..., please reference locate_module_kobject() in "kernel/params.c",
>> > which is an '__init' function, and also call sysfs_create_file(), it
>> > processes the related error.
>> >
>> > So I recommend to get the check too in version_sysfs_builtin().
> It still can't fail. sysfs_create_file() can fail due to OOM (not at
> boot) or name duplication (not here).
>
> You can BUG_ON() if you want.
>
OK, thanks, I will send patch v2 for it.
> And feel free to fix locate_module_kobject() in a separate patch.
OK, thanks, I will send related patch for it.
:-)
Thanks.
--
Chen Gang
When sysfs_create_file() fails, need consider about it. And process it
with BUG_ON(), because sysfs_create_file() can fail due to OOM (not at
boot) or name duplication (not here).
Also correct the error printing information when failure occurs.
Signed-off-by: Chen Gang <[email protected]>
---
kernel/params.c | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/kernel/params.c b/kernel/params.c
index 440e65d..3d70f90 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -745,12 +745,14 @@ static struct module_kobject * __init locate_module_kobject(const char *name)
err = kobject_init_and_add(&mk->kobj, &module_ktype, NULL,
"%s", name);
#ifdef CONFIG_MODULES
- if (!err)
+ if (!err) {
err = sysfs_create_file(&mk->kobj, &module_uevent.attr);
+ BUG_ON(err);
+ }
#endif
if (err) {
kobject_put(&mk->kobj);
- pr_crit("Adding module '%s' to sysfs failed (%d), the system may be unstable.\n",
+ pr_crit("Initializing and adding module '%s' failed (%d), the system may be unstable.\n",
name, err);
return NULL;
}
@@ -845,6 +847,7 @@ static void __init version_sysfs_builtin(void)
mk = locate_module_kobject(vattr->module_name);
if (mk) {
err = sysfs_create_file(&mk->kobj, &vattr->mattr.attr);
+ BUG_ON(err);
kobject_uevent(&mk->kobj, KOBJ_ADD);
kobject_put(&mk->kobj);
}
--
1.7.7.6
On 07/10/2013 11:01 AM, Chen Gang F T wrote:
>
>> > Hmm..., do we need call kobject_get() before kobject_put() in failure
>> > processing block ?
>> >
> Oh, sorry for what I said for kobject_get/put() items above, it is
> incorrect.
>
> What about the diff below for kobject_get() ?
>
> -------------------------------diff begin-------------------------------
>
> diff --git a/kernel/params.c b/kernel/params.c
> index 440e65d..ef8d720 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -754,11 +754,11 @@ static struct module_kobject * __init locate_module_kobject(const char *name)
> name, err);
> return NULL;
> }
> -
> - /* So that we hold reference in both cases. */
> - kobject_get(&mk->kobj);
> }
>
> + /* So that we hold reference in both cases. */
> + kobject_get(&mk->kobj);
> +
> return mk;
> }
>
> -------------------------------diff end---------------------------------
>
Sorry again, this diff is incorrect, the original implementation has no
issues.
> And it also need add additional kobject_put(), if we really need
> process the failure in version_sysfs_builtin().
If need process failure, we really need it, but now, we only use
BUG_ON() is enough.
Thanks.
--
Chen Gang
Hello Maintainers:
Please help check this patch, when you have time.
Thanks.
On 07/11/2013 12:06 PM, Chen Gang wrote:
> When sysfs_create_file() fails, need consider about it. And process it
> with BUG_ON(), because sysfs_create_file() can fail due to OOM (not at
> boot) or name duplication (not here).
>
> Also correct the error printing information when failure occurs.
>
> Signed-off-by: Chen Gang <[email protected]>
> ---
> kernel/params.c | 7 +++++--
> 1 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/params.c b/kernel/params.c
> index 440e65d..3d70f90 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -745,12 +745,14 @@ static struct module_kobject * __init locate_module_kobject(const char *name)
> err = kobject_init_and_add(&mk->kobj, &module_ktype, NULL,
> "%s", name);
> #ifdef CONFIG_MODULES
> - if (!err)
> + if (!err) {
> err = sysfs_create_file(&mk->kobj, &module_uevent.attr);
> + BUG_ON(err);
> + }
> #endif
> if (err) {
> kobject_put(&mk->kobj);
> - pr_crit("Adding module '%s' to sysfs failed (%d), the system may be unstable.\n",
> + pr_crit("Initializing and adding module '%s' failed (%d), the system may be unstable.\n",
> name, err);
> return NULL;
> }
> @@ -845,6 +847,7 @@ static void __init version_sysfs_builtin(void)
> mk = locate_module_kobject(vattr->module_name);
> if (mk) {
> err = sysfs_create_file(&mk->kobj, &vattr->mattr.attr);
> + BUG_ON(err);
> kobject_uevent(&mk->kobj, KOBJ_ADD);
> kobject_put(&mk->kobj);
> }
>
--
Chen Gang