| Message ID | 20191206122434.29587-3-sourabhjain@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | reorganize and add FADump sysfs files | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (567dea0e848944848650d7fd27699e2de5d49353) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 67 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
On Fri, Dec 06, 2019 at 05:54:30PM +0530, Sourabh Jain wrote: > The __compat_only_sysfs_link_entry_to_kobj function creates a symlink to a > kobject but doesn't provide an option to change the symlink file name. > > This patch adds a wrapper function create_sysfs_symlink_entry_to_kobj that > extends the __compat_only_sysfs_link_entry_to_kobj functionality which > allows function caller to customize the symlink name. > > Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com> > --- > fs/sysfs/group.c | 28 +++++++++++++++++++++++++--- > include/linux/sysfs.h | 12 ++++++++++++ > 2 files changed, 37 insertions(+), 3 deletions(-) > > diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c > index d41c21fef138..5eb38145b957 100644 > --- a/fs/sysfs/group.c > +++ b/fs/sysfs/group.c > @@ -424,6 +424,25 @@ EXPORT_SYMBOL_GPL(sysfs_remove_link_from_group); > int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj, > struct kobject *target_kobj, > const char *target_name) > +{ > + return create_sysfs_symlink_entry_to_kobj(kobj, target_kobj, > + target_name, NULL); > +} > +EXPORT_SYMBOL_GPL(__compat_only_sysfs_link_entry_to_kobj); > + > +/** > + * create_sysfs_symlink_entry_to_kobj - add a symlink to a kobject pointing > + * to a group or an attribute > + * @kobj: The kobject containing the group. > + * @target_kobj: The target kobject. > + * @target_name: The name of the target group or attribute. > + * @symlink_name: The name of the symlink file (target_name will be > + * considered if symlink_name is NULL). > + */ > +int create_sysfs_symlink_entry_to_kobj(struct kobject *kobj, > + struct kobject *target_kobj, > + const char *target_name, > + const char *symlink_name) > { > struct kernfs_node *target; > struct kernfs_node *entry; > @@ -448,12 +467,15 @@ int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj, > return -ENOENT; > } > > - link = kernfs_create_link(kobj->sd, target_name, entry); > + if (!symlink_name) > + symlink_name = target_name; > + > + link = kernfs_create_link(kobj->sd, symlink_name, entry); > if (IS_ERR(link) && PTR_ERR(link) == -EEXIST) > - sysfs_warn_dup(kobj->sd, target_name); > + sysfs_warn_dup(kobj->sd, symlink_name); > > kernfs_put(entry); > kernfs_put(target); > return PTR_ERR_OR_ZERO(link); > } > -EXPORT_SYMBOL_GPL(__compat_only_sysfs_link_entry_to_kobj); > +EXPORT_SYMBOL_GPL(create_sysfs_symlink_entry_to_kobj); > diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h > index 5420817ed317..123c6f10333a 100644 > --- a/include/linux/sysfs.h > +++ b/include/linux/sysfs.h > @@ -300,6 +300,10 @@ void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name, > int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj, > struct kobject *target_kobj, > const char *target_name); > +int create_sysfs_symlink_entry_to_kobj(struct kobject *kobj, > + struct kobject *target_kobj, > + const char *target_name, > + const char *symlink_name); sysfs_create_symlink_entry_to_kobj()? I can't remember why we put __compat_only there, perhaps because we do not want people to really use this unless you really really have to? So then keep compat_only here as well? What breaks if you remove those undocumented sysfs files? What userspace tool do you have that will even notice? thanks, greg k-h
On 12/6/19 6:16 PM, Greg KH wrote: > On Fri, Dec 06, 2019 at 05:54:30PM +0530, Sourabh Jain wrote: >> The __compat_only_sysfs_link_entry_to_kobj function creates a symlink to a >> kobject but doesn't provide an option to change the symlink file name. >> >> This patch adds a wrapper function create_sysfs_symlink_entry_to_kobj that >> extends the __compat_only_sysfs_link_entry_to_kobj functionality which >> allows function caller to customize the symlink name. >> >> Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com> >> --- >> fs/sysfs/group.c | 28 +++++++++++++++++++++++++--- >> include/linux/sysfs.h | 12 ++++++++++++ >> 2 files changed, 37 insertions(+), 3 deletions(-) >> >> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c >> index d41c21fef138..5eb38145b957 100644 >> --- a/fs/sysfs/group.c >> +++ b/fs/sysfs/group.c >> @@ -424,6 +424,25 @@ EXPORT_SYMBOL_GPL(sysfs_remove_link_from_group); >> int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj, >> struct kobject *target_kobj, >> const char *target_name) >> +{ >> + return create_sysfs_symlink_entry_to_kobj(kobj, target_kobj, >> + target_name, NULL); >> +} >> +EXPORT_SYMBOL_GPL(__compat_only_sysfs_link_entry_to_kobj); >> + >> +/** >> + * create_sysfs_symlink_entry_to_kobj - add a symlink to a kobject pointing >> + * to a group or an attribute >> + * @kobj: The kobject containing the group. >> + * @target_kobj: The target kobject. >> + * @target_name: The name of the target group or attribute. >> + * @symlink_name: The name of the symlink file (target_name will be >> + * considered if symlink_name is NULL). >> + */ >> +int create_sysfs_symlink_entry_to_kobj(struct kobject *kobj, >> + struct kobject *target_kobj, >> + const char *target_name, >> + const char *symlink_name) >> { >> struct kernfs_node *target; >> struct kernfs_node *entry; >> @@ -448,12 +467,15 @@ int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj, >> return -ENOENT; >> } >> >> - link = kernfs_create_link(kobj->sd, target_name, entry); >> + if (!symlink_name) >> + symlink_name = target_name; >> + >> + link = kernfs_create_link(kobj->sd, symlink_name, entry); >> if (IS_ERR(link) && PTR_ERR(link) == -EEXIST) >> - sysfs_warn_dup(kobj->sd, target_name); >> + sysfs_warn_dup(kobj->sd, symlink_name); >> >> kernfs_put(entry); >> kernfs_put(target); >> return PTR_ERR_OR_ZERO(link); >> } >> -EXPORT_SYMBOL_GPL(__compat_only_sysfs_link_entry_to_kobj); >> +EXPORT_SYMBOL_GPL(create_sysfs_symlink_entry_to_kobj); >> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h >> index 5420817ed317..123c6f10333a 100644 >> --- a/include/linux/sysfs.h >> +++ b/include/linux/sysfs.h >> @@ -300,6 +300,10 @@ void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name, >> int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj, >> struct kobject *target_kobj, >> const char *target_name); >> +int create_sysfs_symlink_entry_to_kobj(struct kobject *kobj, >> + struct kobject *target_kobj, >> + const char *target_name, >> + const char *symlink_name); > > sysfs_create_symlink_entry_to_kobj()? > > I can't remember why we put __compat_only there, perhaps because we do > not want people to really use this unless you really really have to? We don't have much option here. I tried replicating the sysfs files in older patch series but creating symlink at old location is much better approach. The __compat_only_sysfs_link_entry_to_kobj function is pretty generic, unable to understand the reason behind restricting its usage. > > So then keep compat_only here as well? Sure, I will rename the wrapper function. But how about changing the function signature instead of creating a wrapper function? Considering the fact that there are only two places this function has called. > > What breaks if you remove those undocumented sysfs files? What > userspace tool do you have that will even notice? The scripts used in kdump service need those sysfs files to control the dump collection. So we can't just move the sysfs files to the new location. Thanks, Sourabh Jain
On Fri, Dec 06, 2019 at 11:57:53PM +0530, Sourabh Jain wrote: > > > On 12/6/19 6:16 PM, Greg KH wrote: > > On Fri, Dec 06, 2019 at 05:54:30PM +0530, Sourabh Jain wrote: > >> The __compat_only_sysfs_link_entry_to_kobj function creates a symlink to a > >> kobject but doesn't provide an option to change the symlink file name. > >> > >> This patch adds a wrapper function create_sysfs_symlink_entry_to_kobj that > >> extends the __compat_only_sysfs_link_entry_to_kobj functionality which > >> allows function caller to customize the symlink name. > >> > >> Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com> > >> --- > >> fs/sysfs/group.c | 28 +++++++++++++++++++++++++--- > >> include/linux/sysfs.h | 12 ++++++++++++ > >> 2 files changed, 37 insertions(+), 3 deletions(-) > >> > >> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c > >> index d41c21fef138..5eb38145b957 100644 > >> --- a/fs/sysfs/group.c > >> +++ b/fs/sysfs/group.c > >> @@ -424,6 +424,25 @@ EXPORT_SYMBOL_GPL(sysfs_remove_link_from_group); > >> int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj, > >> struct kobject *target_kobj, > >> const char *target_name) > >> +{ > >> + return create_sysfs_symlink_entry_to_kobj(kobj, target_kobj, > >> + target_name, NULL); > >> +} > >> +EXPORT_SYMBOL_GPL(__compat_only_sysfs_link_entry_to_kobj); > >> + > >> +/** > >> + * create_sysfs_symlink_entry_to_kobj - add a symlink to a kobject pointing > >> + * to a group or an attribute > >> + * @kobj: The kobject containing the group. > >> + * @target_kobj: The target kobject. > >> + * @target_name: The name of the target group or attribute. > >> + * @symlink_name: The name of the symlink file (target_name will be > >> + * considered if symlink_name is NULL). > >> + */ > >> +int create_sysfs_symlink_entry_to_kobj(struct kobject *kobj, > >> + struct kobject *target_kobj, > >> + const char *target_name, > >> + const char *symlink_name) > >> { > >> struct kernfs_node *target; > >> struct kernfs_node *entry; > >> @@ -448,12 +467,15 @@ int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj, > >> return -ENOENT; > >> } > >> > >> - link = kernfs_create_link(kobj->sd, target_name, entry); > >> + if (!symlink_name) > >> + symlink_name = target_name; > >> + > >> + link = kernfs_create_link(kobj->sd, symlink_name, entry); > >> if (IS_ERR(link) && PTR_ERR(link) == -EEXIST) > >> - sysfs_warn_dup(kobj->sd, target_name); > >> + sysfs_warn_dup(kobj->sd, symlink_name); > >> > >> kernfs_put(entry); > >> kernfs_put(target); > >> return PTR_ERR_OR_ZERO(link); > >> } > >> -EXPORT_SYMBOL_GPL(__compat_only_sysfs_link_entry_to_kobj); > >> +EXPORT_SYMBOL_GPL(create_sysfs_symlink_entry_to_kobj); > >> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h > >> index 5420817ed317..123c6f10333a 100644 > >> --- a/include/linux/sysfs.h > >> +++ b/include/linux/sysfs.h > >> @@ -300,6 +300,10 @@ void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name, > >> int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj, > >> struct kobject *target_kobj, > >> const char *target_name); > >> +int create_sysfs_symlink_entry_to_kobj(struct kobject *kobj, > >> + struct kobject *target_kobj, > >> + const char *target_name, > >> + const char *symlink_name); > > > > sysfs_create_symlink_entry_to_kobj()? > > > > I can't remember why we put __compat_only there, perhaps because we do > > not want people to really use this unless you really really have to? > > We don't have much option here. I tried replicating the sysfs files > in older patch series but creating symlink at old location is much > better approach. > > The __compat_only_sysfs_link_entry_to_kobj function is pretty generic, > unable to understand the reason behind restricting its usage. > > > > > So then keep compat_only here as well? > > Sure, I will rename the wrapper function. > > But how about changing the function signature instead of creating > a wrapper function? > > Considering the fact that there are only two places this function > has called. > > > > > What breaks if you remove those undocumented sysfs files? What > > userspace tool do you have that will even notice? > > The scripts used in kdump service need those sysfs files to control > the dump collection. So we can't just move the sysfs files to the > new location. If you can not change them, then just document them and live with it. Why do this extra work to create a symlink for something you will never use? greg k-h
On 12/7/19 12:44 AM, Greg KH wrote: > On Fri, Dec 06, 2019 at 11:57:53PM +0530, Sourabh Jain wrote: >> >> >> On 12/6/19 6:16 PM, Greg KH wrote: >>> On Fri, Dec 06, 2019 at 05:54:30PM +0530, Sourabh Jain wrote: >>>> The __compat_only_sysfs_link_entry_to_kobj function creates a symlink to a >>>> kobject but doesn't provide an option to change the symlink file name. >>>> >>>> This patch adds a wrapper function create_sysfs_symlink_entry_to_kobj that >>>> extends the __compat_only_sysfs_link_entry_to_kobj functionality which >>>> allows function caller to customize the symlink name. >>>> >>>> Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com> >>>> --- >>>> fs/sysfs/group.c | 28 +++++++++++++++++++++++++--- >>>> include/linux/sysfs.h | 12 ++++++++++++ >>>> 2 files changed, 37 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c >>>> index d41c21fef138..5eb38145b957 100644 >>>> --- a/fs/sysfs/group.c >>>> +++ b/fs/sysfs/group.c >>>> @@ -424,6 +424,25 @@ EXPORT_SYMBOL_GPL(sysfs_remove_link_from_group); >>>> int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj, >>>> struct kobject *target_kobj, >>>> const char *target_name) >>>> +{ >>>> + return create_sysfs_symlink_entry_to_kobj(kobj, target_kobj, >>>> + target_name, NULL); >>>> +} >>>> +EXPORT_SYMBOL_GPL(__compat_only_sysfs_link_entry_to_kobj); >>>> + >>>> +/** >>>> + * create_sysfs_symlink_entry_to_kobj - add a symlink to a kobject pointing >>>> + * to a group or an attribute >>>> + * @kobj: The kobject containing the group. >>>> + * @target_kobj: The target kobject. >>>> + * @target_name: The name of the target group or attribute. >>>> + * @symlink_name: The name of the symlink file (target_name will be >>>> + * considered if symlink_name is NULL). >>>> + */ >>>> +int create_sysfs_symlink_entry_to_kobj(struct kobject *kobj, >>>> + struct kobject *target_kobj, >>>> + const char *target_name, >>>> + const char *symlink_name) >>>> { >>>> struct kernfs_node *target; >>>> struct kernfs_node *entry; >>>> @@ -448,12 +467,15 @@ int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj, >>>> return -ENOENT; >>>> } >>>> >>>> - link = kernfs_create_link(kobj->sd, target_name, entry); >>>> + if (!symlink_name) >>>> + symlink_name = target_name; >>>> + >>>> + link = kernfs_create_link(kobj->sd, symlink_name, entry); >>>> if (IS_ERR(link) && PTR_ERR(link) == -EEXIST) >>>> - sysfs_warn_dup(kobj->sd, target_name); >>>> + sysfs_warn_dup(kobj->sd, symlink_name); >>>> >>>> kernfs_put(entry); >>>> kernfs_put(target); >>>> return PTR_ERR_OR_ZERO(link); >>>> } >>>> -EXPORT_SYMBOL_GPL(__compat_only_sysfs_link_entry_to_kobj); >>>> +EXPORT_SYMBOL_GPL(create_sysfs_symlink_entry_to_kobj); >>>> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h >>>> index 5420817ed317..123c6f10333a 100644 >>>> --- a/include/linux/sysfs.h >>>> +++ b/include/linux/sysfs.h >>>> @@ -300,6 +300,10 @@ void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name, >>>> int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj, >>>> struct kobject *target_kobj, >>>> const char *target_name); >>>> +int create_sysfs_symlink_entry_to_kobj(struct kobject *kobj, >>>> + struct kobject *target_kobj, >>>> + const char *target_name, >>>> + const char *symlink_name); >>> >>> sysfs_create_symlink_entry_to_kobj()? >>> >>> I can't remember why we put __compat_only there, perhaps because we do >>> not want people to really use this unless you really really have to? >> >> We don't have much option here. I tried replicating the sysfs files >> in older patch series but creating symlink at old location is much >> better approach. >> >> The __compat_only_sysfs_link_entry_to_kobj function is pretty generic, >> unable to understand the reason behind restricting its usage. >> >>> >>> So then keep compat_only here as well? >> >> Sure, I will rename the wrapper function. >> >> But how about changing the function signature instead of creating >> a wrapper function? >> >> Considering the fact that there are only two places this function >> has called. >> >>> >>> What breaks if you remove those undocumented sysfs files? What >>> userspace tool do you have that will even notice? >> >> The scripts used in kdump service need those sysfs files to control >> the dump collection. So we can't just move the sysfs files to the >> new location. > > If you can not change them, then just document them and live with it. > Why do this extra work to create a symlink for something you will never > use? Eventually the scripts will change but I think it is better to have some overlap time to avoid breaking those scripts. Thanks, Sourabh Jain
diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c index d41c21fef138..5eb38145b957 100644 --- a/fs/sysfs/group.c +++ b/fs/sysfs/group.c @@ -424,6 +424,25 @@ EXPORT_SYMBOL_GPL(sysfs_remove_link_from_group); int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj, struct kobject *target_kobj, const char *target_name) +{ + return create_sysfs_symlink_entry_to_kobj(kobj, target_kobj, + target_name, NULL); +} +EXPORT_SYMBOL_GPL(__compat_only_sysfs_link_entry_to_kobj); + +/** + * create_sysfs_symlink_entry_to_kobj - add a symlink to a kobject pointing + * to a group or an attribute + * @kobj: The kobject containing the group. + * @target_kobj: The target kobject. + * @target_name: The name of the target group or attribute. + * @symlink_name: The name of the symlink file (target_name will be + * considered if symlink_name is NULL). + */ +int create_sysfs_symlink_entry_to_kobj(struct kobject *kobj, + struct kobject *target_kobj, + const char *target_name, + const char *symlink_name) { struct kernfs_node *target; struct kernfs_node *entry; @@ -448,12 +467,15 @@ int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj, return -ENOENT; } - link = kernfs_create_link(kobj->sd, target_name, entry); + if (!symlink_name) + symlink_name = target_name; + + link = kernfs_create_link(kobj->sd, symlink_name, entry); if (IS_ERR(link) && PTR_ERR(link) == -EEXIST) - sysfs_warn_dup(kobj->sd, target_name); + sysfs_warn_dup(kobj->sd, symlink_name); kernfs_put(entry); kernfs_put(target); return PTR_ERR_OR_ZERO(link); } -EXPORT_SYMBOL_GPL(__compat_only_sysfs_link_entry_to_kobj); +EXPORT_SYMBOL_GPL(create_sysfs_symlink_entry_to_kobj); diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h index 5420817ed317..123c6f10333a 100644 --- a/include/linux/sysfs.h +++ b/include/linux/sysfs.h @@ -300,6 +300,10 @@ void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name, int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj, struct kobject *target_kobj, const char *target_name); +int create_sysfs_symlink_entry_to_kobj(struct kobject *kobj, + struct kobject *target_kobj, + const char *target_name, + const char *symlink_name); void sysfs_notify(struct kobject *kobj, const char *dir, const char *attr); @@ -508,6 +512,14 @@ static inline int __compat_only_sysfs_link_entry_to_kobj( return 0; } +static int create_sysfs_symlink_entry_to_kobj(struct kobject *kobj, + struct kobject *target_kobj, + const char *target_name, + const char *symlink_name) +{ + return 0; +} + static inline void sysfs_notify(struct kobject *kobj, const char *dir, const char *attr) {
The __compat_only_sysfs_link_entry_to_kobj function creates a symlink to a kobject but doesn't provide an option to change the symlink file name. This patch adds a wrapper function create_sysfs_symlink_entry_to_kobj that extends the __compat_only_sysfs_link_entry_to_kobj functionality which allows function caller to customize the symlink name. Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com> --- fs/sysfs/group.c | 28 +++++++++++++++++++++++++--- include/linux/sysfs.h | 12 ++++++++++++ 2 files changed, 37 insertions(+), 3 deletions(-)