| Message ID | 1434993894-5911-2-git-send-email-jarkko.sakkinen@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, Jun 22, 2015 at 08:24:50PM +0300, Jarkko Sakkinen wrote: > Added a new function sysfs_link_group_to_kobj() that adds a symlink > from attribute or group to a kobject. Exported kernfs_remove_by_name_ns > in order to provide a way to remove such symlinks. > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> Hmmm... is this *really* necessary? If linking from the parent kobj doesn't make a fundamental functional difference, I don't think this is a good idea. If linking to the parent doesn't work, why doesn't it? Shouldn't that already be a different kobj then? I'd really like to keep groups as a dumb container of simple attrs. Thanks.
On Mon, Jun 22, 2015 at 01:30:39PM -0400, Tejun Heo wrote: > On Mon, Jun 22, 2015 at 08:24:50PM +0300, Jarkko Sakkinen wrote: > > Added a new function sysfs_link_group_to_kobj() that adds a symlink > > from attribute or group to a kobject. Exported kernfs_remove_by_name_ns > > in order to provide a way to remove such symlinks. > > > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > Hmmm... is this *really* necessary? If linking from the parent kobj > doesn't make a fundamental functional difference, I don't think this > is a good idea. If linking to the parent doesn't work, why doesn't > it? Shouldn't that already be a different kobj then? I'd really like > to keep groups as a dumb container of simple attrs. TPM is undergoing a migration of core attributes from the platform_device to the core's struct device. The only purpose of the symlink was to provide userspace compatability with the old location. Jason ------------------------------------------------------------------------------ Monitor 25 network devices or servers for free with OpManager! OpManager is web-based network management software that monitors network devices and physical & virtual servers, alerts via email & sms for fault. Monitor 25 devices for free with no restriction. Download now http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
On Mon, Jun 22, 2015 at 11:52:53AM -0600, Jason Gunthorpe wrote: > On Mon, Jun 22, 2015 at 01:30:39PM -0400, Tejun Heo wrote: > > On Mon, Jun 22, 2015 at 08:24:50PM +0300, Jarkko Sakkinen wrote: > > > Added a new function sysfs_link_group_to_kobj() that adds a symlink > > > from attribute or group to a kobject. Exported kernfs_remove_by_name_ns > > > in order to provide a way to remove such symlinks. > > > > > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > > > Hmmm... is this *really* necessary? If linking from the parent kobj > > doesn't make a fundamental functional difference, I don't think this > > is a good idea. If linking to the parent doesn't work, why doesn't > > it? Shouldn't that already be a different kobj then? I'd really like > > to keep groups as a dumb container of simple attrs. > > TPM is undergoing a migration of core attributes from the > platform_device to the core's struct device. > > The only purpose of the symlink was to provide userspace > compatability with the old location. Ah, yeah, that's painful. Can you please briefly explain why it wasn't necessary before? Are you merging multiple devices into one? Thanks.
On Mon, Jun 22, 2015 at 02:01:54PM -0400, Tejun Heo wrote: > On Mon, Jun 22, 2015 at 11:52:53AM -0600, Jason Gunthorpe wrote: > > On Mon, Jun 22, 2015 at 01:30:39PM -0400, Tejun Heo wrote: > > > On Mon, Jun 22, 2015 at 08:24:50PM +0300, Jarkko Sakkinen wrote: > > > > Added a new function sysfs_link_group_to_kobj() that adds a symlink > > > > from attribute or group to a kobject. Exported kernfs_remove_by_name_ns > > > > in order to provide a way to remove such symlinks. > > > > > > > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > > > > > Hmmm... is this *really* necessary? If linking from the parent kobj > > > doesn't make a fundamental functional difference, I don't think this > > > is a good idea. If linking to the parent doesn't work, why doesn't > > > it? Shouldn't that already be a different kobj then? I'd really like > > > to keep groups as a dumb container of simple attrs. > > > > TPM is undergoing a migration of core attributes from the > > platform_device to the core's struct device. > > > > The only purpose of the symlink was to provide userspace > > compatability with the old location. > > Ah, yeah, that's painful. Can you please briefly explain why it > wasn't necessary before? Are you merging multiple devices into one? I've been working on for a while on TPM 2.0 protocol support and wanted to do things right from the beginning for TPM 2.0 level devices. The first thing was to introduce a device class for TPM devices (both 1.x and 2.0), which I did when the baseline support for TPM 2.0 landed in 4.0. The second is to introduce *necessary* sysfs attributes in the correct place so that they are created and destroyed race free. All sysfs attributes for TPM 1.x devices have been placed in the pdev directory. For major part of the attributes I decided not to add them at all for TPM 2.0 devices because they break all the conventions suggested in sysfs-rules.txt and more importantly you can acquire the data that they contain by using /dev/tpm0 and TPM protocol (either 1.x or 2.0). The PPI interface is necessary because it is used to take the ownership of the device. The solution that I'm presenting in this patch set is something that I just considered "least harm" when considering backwards compatibility and raciness. > Thanks. > > -- > tejun /Jarkko ------------------------------------------------------------------------------ Monitor 25 network devices or servers for free with OpManager! OpManager is web-based network management software that monitors network devices and physical & virtual servers, alerts via email & sms for fault. Monitor 25 devices for free with no restriction. Download now http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
On Mon, Jun 22, 2015 at 02:01:54PM -0400, Tejun Heo wrote: > > TPM is undergoing a migration of core attributes from the > > platform_device to the core's struct device. > > > > The only purpose of the symlink was to provide userspace > > compatability with the old location. > > Ah, yeah, that's painful. Can you please briefly explain why it > wasn't necessary before? Are you merging multiple devices into one? Prior to the clean up work starting each and every driver was adding sysfs files on its own to the platform_device and we had no core struct tpm.struct device all. The first cleanup moved all the sysfs creation into core code, but kept the use of the platform device. A second cleanup added the struct device and removed the misc_device abuse. The next round was hoped to bring the core code into alignment with everything else in the kernel: - Create sysfs files at the right time so userspace isn't racey - Don't stomp the drvdata of the platform_device in core code - Have the understandable lifetime model that most of the rest of the kernel has The concept was to move the sysfs files to the natural location on the core's struct tpm.struct device and leave a symlink behind on the platform_device for compat. We could also just move the sysfs file and ignore the compat symlink, but it is not clear to me if that is an OK UAPI break for sysfs? Jason ------------------------------------------------------------------------------ Monitor 25 network devices or servers for free with OpManager! OpManager is web-based network management software that monitors network devices and physical & virtual servers, alerts via email & sms for fault. Monitor 25 devices for free with no restriction. Download now http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
Hello, Jason. On Tue, Jun 23, 2015 at 10:19:32AM -0600, Jason Gunthorpe wrote: > The concept was to move the sysfs files to the natural location on the > core's struct tpm.struct device and leave a symlink behind on the > platform_device for compat. I see. The only problem I see is that this might get abused. Can you clearly mark the function that it's not intended to be used in anything new, e.g. by renaming it to sth like __compat_only_sysfs_link_entry_to_kobj(). > We could also just move the sysfs file and ignore the compat symlink, > but it is not clear to me if that is an OK UAPI break for sysfs? We shouldn't if it has actual chance of disturbing userland. Thanks.
On 07/02/2015 08:04 PM, Tejun Heo wrote: > Hello, Jason. > > On Tue, Jun 23, 2015 at 10:19:32AM -0600, Jason Gunthorpe wrote: >> The concept was to move the sysfs files to the natural location on the >> core's struct tpm.struct device and leave a symlink behind on the >> platform_device for compat. > > I see. The only problem I see is that this might get abused. Can you > clearly mark the function that it's not intended to be used in > anything new, e.g. by renaming it to sth like > __compat_only_sysfs_link_entry_to_kobj(). Great! We will do that. Thanks for taking time and understanding what we are dealing with. We are trying to fix all the bad behavior from TPM subsystem and bring up to the todays standards so that next gen TPM 2.0 devices will have a solid platform. >> We could also just move the sysfs file and ignore the compat symlink, >> but it is not clear to me if that is an OK UAPI break for sysfs? > > We shouldn't if it has actual chance of disturbing userland. > > Thanks. /Jarkko ------------------------------------------------------------------------------ Don't Limit Your Business. Reach for the Cloud. GigeNET's Cloud Solutions provide you with the tools and support that you need to offload your IT needs and focus on growing your business. Configured For All Businesses. Start Your Cloud Today. https://www.gigenetcloud.com/
diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c index b400c04..5c72996 100644 --- a/fs/sysfs/group.c +++ b/fs/sysfs/group.c @@ -352,3 +352,45 @@ void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name, } } EXPORT_SYMBOL_GPL(sysfs_remove_link_from_group); + +/** + * sysfs_link_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. + */ +int sysfs_link_entry_to_kobj(struct kobject *kobj, struct kobject *target_kobj, + const char *target_name) +{ + struct kernfs_node *target; + struct kernfs_node *entry; + struct kernfs_node *link; + + /* + * We don't own @target_kobj and it may be removed at any time. + * Synchronize using sysfs_symlink_target_lock. See sysfs_remove_dir() + * for details. + */ + spin_lock(&sysfs_symlink_target_lock); + target = target_kobj->sd; + if (target) + kernfs_get(target); + spin_unlock(&sysfs_symlink_target_lock); + if (!target) + return -ENOENT; + + entry = kernfs_find_and_get(target_kobj->sd, target_name); + if (!entry) { + kernfs_put(target); + return -ENOENT; + } + + link = kernfs_create_link(kobj->sd, target_name, entry); + if (IS_ERR(link) && PTR_ERR(link) == -EEXIST) + sysfs_warn_dup(kobj->sd, target_name); + + kernfs_put(entry); + kernfs_put(target); + return IS_ERR(link) ? PTR_ERR(link) : 0; +} +EXPORT_SYMBOL_GPL(sysfs_link_entry_to_kobj); diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h index 99382c0..bfdf5c7 100644 --- a/include/linux/sysfs.h +++ b/include/linux/sysfs.h @@ -264,6 +264,8 @@ int sysfs_add_link_to_group(struct kobject *kobj, const char *group_name, struct kobject *target, const char *link_name); void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name, const char *link_name); +int sysfs_link_entry_to_kobj(struct kobject *kobj, struct kobject *target_kobj, + const char *target_name); void sysfs_notify(struct kobject *kobj, const char *dir, const char *attr); @@ -436,6 +438,13 @@ static inline void sysfs_remove_link_from_group(struct kobject *kobj, { } +static inline int sysfs_link_entry_to_kobj(struct kobject *kobj, + struct kobject *target_kobj, + const char *target_name) +{ + return 0; +} + static inline void sysfs_notify(struct kobject *kobj, const char *dir, const char *attr) {
Added a new function sysfs_link_group_to_kobj() that adds a symlink from attribute or group to a kobject. Exported kernfs_remove_by_name_ns in order to provide a way to remove such symlinks. Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> --- fs/sysfs/group.c | 42 ++++++++++++++++++++++++++++++++++++++++++ include/linux/sysfs.h | 9 +++++++++ 2 files changed, 51 insertions(+)