diff mbox

[03/10] sysfs: added __compat_only_sysfs_link_entry_to_kobj()

Message ID 1445020843-9382-4-git-send-email-jarkko.sakkinen@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jarkko Sakkinen Oct. 16, 2015, 6:40 p.m. UTC
Added a new function __compat_only_sysfs_link_group_to_kobj() that adds
a symlink from attribute or group to a kobject. This needed for
maintaining backwards compatibility with PPI attributes in the TPM
driver.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 fs/sysfs/group.c      | 44 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/sysfs.h | 11 +++++++++++
 2 files changed, 55 insertions(+)

Comments

Peter Huewe Oct. 18, 2015, 1:37 a.m. UTC | #1
Am Freitag, 16. Oktober 2015, 20:40:22 schrieb Jarkko Sakkinen:
> Added a new function __compat_only_sysfs_link_group_to_kobj() that adds
> a symlink from attribute or group to a kobject. This needed for
> maintaining backwards compatibility with PPI attributes in the TPM
> driver.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
@Greg: Can I get an acked-by or something from you as the sysfs maintainer?

Thanks,
Peter

------------------------------------------------------------------------------
Jarkko Sakkinen Oct. 18, 2015, 11:21 a.m. UTC | #2
On Sun, Oct 18, 2015 at 03:37:16AM +0200, Peter Hüwe wrote:
> Am Freitag, 16. Oktober 2015, 20:40:22 schrieb Jarkko Sakkinen:
> > Added a new function __compat_only_sysfs_link_group_to_kobj() that adds
> > a symlink from attribute or group to a kobject. This needed for
> > maintaining backwards compatibility with PPI attributes in the TPM
> > driver.
> > 
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> @Greg: Can I get an acked-by or something from you as the sysfs maintainer?

I think everyone was otherwise OK with this with only one contradiction
as I read previous dicussions about this patch:

- This name was propsed by Tejun Heo
- Greg doubted whether it is a good name.

We can fixup to another name if someone says what it would be.

> Thanks,
> Peter

/Jarkko

------------------------------------------------------------------------------
Jeremiah Mahler Nov. 7, 2015, 12:23 a.m. UTC | #3
Jarkko,

On Fri, Oct 16, 2015 at 09:40:22PM +0300, Jarkko Sakkinen wrote:
> Added a new function __compat_only_sysfs_link_group_to_kobj() that adds
> a symlink from attribute or group to a kobject. This needed for
> maintaining backwards compatibility with PPI attributes in the TPM
> driver.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  fs/sysfs/group.c      | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/sysfs.h | 11 +++++++++++
>  2 files changed, 55 insertions(+)
> 
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index 39a0199..e123659 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -352,3 +352,47 @@ void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name,
>  	}
>  }
>  EXPORT_SYMBOL_GPL(sysfs_remove_link_from_group);
> +
> +/**
> + * __compat_only_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 __compat_only_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(__compat_only_sysfs_link_entry_to_kobj);
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index 9f65758..ea090ea 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -268,6 +268,9 @@ 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 __compat_only_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);
>  
> @@ -451,6 +454,14 @@ static inline void sysfs_remove_link_from_group(struct kobject *kobj,
>  {
>  }
>  
> +static inline int __compat_only_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)
>  {
> -- 
> 2.5.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

One problem with this patch is that it adds a bunch of code but none of
it is used.  So if there is a bug, git bisect will point to a later
patch instead of this one.
Jeremiah Mahler Nov. 7, 2015, 2:55 a.m. UTC | #4
Jarkko,

On Fri, Oct 16, 2015 at 09:40:22PM +0300, Jarkko Sakkinen wrote:
> Added a new function __compat_only_sysfs_link_group_to_kobj() that adds
> a symlink from attribute or group to a kobject. This needed for
> maintaining backwards compatibility with PPI attributes in the TPM
> driver.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  fs/sysfs/group.c      | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/sysfs.h | 11 +++++++++++
>  2 files changed, 55 insertions(+)
> 
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index 39a0199..e123659 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -352,3 +352,47 @@ void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name,
>  	}
>  }
>  EXPORT_SYMBOL_GPL(sysfs_remove_link_from_group);
> +
> +/**
> + * __compat_only_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 __compat_only_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;
> +	}
> +

On an Acer C720 this call to kernfs_find_and_get fails resulting in
a failed resume after suspend.

Apparently it can't find an object for the name "ppi".

This bug does not appear until the next patch is applied which
calls __compat_only_sysfs_link_entry_to_kobj.

[...]
Jarkko Sakkinen Nov. 7, 2015, 10:55 a.m. UTC | #5
On Fri, Nov 06, 2015 at 06:55:18PM -0800, Jeremiah Mahler wrote:
> Jarkko,
> 
> On Fri, Oct 16, 2015 at 09:40:22PM +0300, Jarkko Sakkinen wrote:
> > Added a new function __compat_only_sysfs_link_group_to_kobj() that adds
> > a symlink from attribute or group to a kobject. This needed for
> > maintaining backwards compatibility with PPI attributes in the TPM
> > driver.
> > 
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> >  fs/sysfs/group.c      | 44 ++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/sysfs.h | 11 +++++++++++
> >  2 files changed, 55 insertions(+)
> > 
> > diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> > index 39a0199..e123659 100644
> > --- a/fs/sysfs/group.c
> > +++ b/fs/sysfs/group.c
> > @@ -352,3 +352,47 @@ void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name,
> >  	}
> >  }
> >  EXPORT_SYMBOL_GPL(sysfs_remove_link_from_group);
> > +
> > +/**
> > + * __compat_only_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 __compat_only_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;
> > +	}
> > +
> 
> On an Acer C720 this call to kernfs_find_and_get fails resulting in
> a failed resume after suspend.
> 
> Apparently it can't find an object for the name "ppi".
> 
> This bug does not appear until the next patch is applied which
> calls __compat_only_sysfs_link_entry_to_kobj.

I think I might have found something thanks to your help

First I found this old bug:

https://bugzilla.redhat.com/show_bug.cgi?id=1096511

What is happening is that DSM is not found and therefore tpm_add_ppi()
does not add ppi to sysfs groups array.

__compat_only_sysfs_link_entry_to_kobj() is called after tpm_add_ppi
unconditionally for TPM1.

I'll implement a fix for this ASAP.

Thank you for great effort on finding tis!

> -- 
> - Jeremiah Mahler

/Jarkko

------------------------------------------------------------------------------
Jarkko Sakkinen Nov. 7, 2015, 11:41 a.m. UTC | #6
On Sat, Nov 07, 2015 at 12:55:43PM +0200, Jarkko Sakkinen wrote:
> On Fri, Nov 06, 2015 at 06:55:18PM -0800, Jeremiah Mahler wrote:
> > Jarkko,
> > 
> > On Fri, Oct 16, 2015 at 09:40:22PM +0300, Jarkko Sakkinen wrote:
> > > Added a new function __compat_only_sysfs_link_group_to_kobj() that adds
> > > a symlink from attribute or group to a kobject. This needed for
> > > maintaining backwards compatibility with PPI attributes in the TPM
> > > driver.
> > > 
> > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > ---
> > >  fs/sysfs/group.c      | 44 ++++++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/sysfs.h | 11 +++++++++++
> > >  2 files changed, 55 insertions(+)
> > > 
> > > diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> > > index 39a0199..e123659 100644
> > > --- a/fs/sysfs/group.c
> > > +++ b/fs/sysfs/group.c
> > > @@ -352,3 +352,47 @@ void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name,
> > >  	}
> > >  }
> > >  EXPORT_SYMBOL_GPL(sysfs_remove_link_from_group);
> > > +
> > > +/**
> > > + * __compat_only_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 __compat_only_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;
> > > +	}
> > > +
> > 
> > On an Acer C720 this call to kernfs_find_and_get fails resulting in
> > a failed resume after suspend.
> > 
> > Apparently it can't find an object for the name "ppi".
> > 
> > This bug does not appear until the next patch is applied which
> > calls __compat_only_sysfs_link_entry_to_kobj.
> 
> I think I might have found something thanks to your help
> 
> First I found this old bug:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1096511
> 
> What is happening is that DSM is not found and therefore tpm_add_ppi()
> does not add ppi to sysfs groups array.
> 
> __compat_only_sysfs_link_entry_to_kobj() is called after tpm_add_ppi
> unconditionally for TPM1.
> 
> I'll implement a fix for this ASAP.
> 
> Thank you for great effort on finding tis!

I pushed a fix over here:

https://github.com/jsakkine/linux-tpmdd/tree/fixes

I'm at the moment in process testing it. Can you run 'acpidump' on your
laptop and send the output me in private? I'll just verify that my
deduction for the root cause is correct.

> > -- 
> > - Jeremiah Mahler

/Jarkko

------------------------------------------------------------------------------
Jeremiah Mahler Nov. 7, 2015, 6:08 p.m. UTC | #7
Jarkko,

On Sat, Nov 07, 2015 at 01:41:37PM +0200, Jarkko Sakkinen wrote:
> On Sat, Nov 07, 2015 at 12:55:43PM +0200, Jarkko Sakkinen wrote:
> > On Fri, Nov 06, 2015 at 06:55:18PM -0800, Jeremiah Mahler wrote:
> > > Jarkko,
> > > 
[...]
> > > > +	entry = kernfs_find_and_get(target_kobj->sd, target_name);
> > > > +	if (!entry) {
> > > > +		kernfs_put(target);
> > > > +		return -ENOENT;
> > > > +	}
> > > > +
> > > 
> > > On an Acer C720 this call to kernfs_find_and_get fails resulting in
> > > a failed resume after suspend.
> > > 
> > > Apparently it can't find an object for the name "ppi".
> > > 
> > > This bug does not appear until the next patch is applied which
> > > calls __compat_only_sysfs_link_entry_to_kobj.
> > 
> > I think I might have found something thanks to your help
> > 
> > First I found this old bug:
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1096511
> > 
> > What is happening is that DSM is not found and therefore tpm_add_ppi()
> > does not add ppi to sysfs groups array.
> > 
> > __compat_only_sysfs_link_entry_to_kobj() is called after tpm_add_ppi
> > unconditionally for TPM1.
> > 
> > I'll implement a fix for this ASAP.
> > 
> > Thank you for great effort on finding tis!
> 
> I pushed a fix over here:
> 
> https://github.com/jsakkine/linux-tpmdd/tree/fixes
> 
This fix does work since it effectively avoids the call to
__compat_only_sysfs_link_entry_to_kobj().

Have you tested cases where __compat_only_sysfs_link_entry_to_kobj()
is actually used?

[...]
Jarkko Sakkinen Nov. 7, 2015, 10:31 p.m. UTC | #8
On Sat, Nov 07, 2015 at 10:08:56AM -0800, Jeremiah Mahler wrote:
> Jarkko,
> 
> On Sat, Nov 07, 2015 at 01:41:37PM +0200, Jarkko Sakkinen wrote:
> > On Sat, Nov 07, 2015 at 12:55:43PM +0200, Jarkko Sakkinen wrote:
> > > On Fri, Nov 06, 2015 at 06:55:18PM -0800, Jeremiah Mahler wrote:
> > > > Jarkko,
> > > > 
> [...]
> > > > > +	entry = kernfs_find_and_get(target_kobj->sd, target_name);
> > > > > +	if (!entry) {
> > > > > +		kernfs_put(target);
> > > > > +		return -ENOENT;
> > > > > +	}
> > > > > +
> > > > 
> > > > On an Acer C720 this call to kernfs_find_and_get fails resulting in
> > > > a failed resume after suspend.
> > > > 
> > > > Apparently it can't find an object for the name "ppi".
> > > > 
> > > > This bug does not appear until the next patch is applied which
> > > > calls __compat_only_sysfs_link_entry_to_kobj.
> > > 
> > > I think I might have found something thanks to your help
> > > 
> > > First I found this old bug:
> > > 
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1096511
> > > 
> > > What is happening is that DSM is not found and therefore tpm_add_ppi()
> > > does not add ppi to sysfs groups array.
> > > 
> > > __compat_only_sysfs_link_entry_to_kobj() is called after tpm_add_ppi
> > > unconditionally for TPM1.
> > > 
> > > I'll implement a fix for this ASAP.
> > > 
> > > Thank you for great effort on finding tis!
> > 
> > I pushed a fix over here:
> > 
> > https://github.com/jsakkine/linux-tpmdd/tree/fixes
> > 
> This fix does work since it effectively avoids the call to
> __compat_only_sysfs_link_entry_to_kobj().
> 
> Have you tested cases where __compat_only_sysfs_link_entry_to_kobj()
> is actually used?

Yes, of course I have. And I checked your DSDT and my assumption was
correct. There was no DSM in the ACPI object.

However, there is probably another regression but it is caused by some
patch that was added earlier. I strongly believe it is not caused by any
of my 4.4 patches.

I think what was happening with you was that
__compat_only_sysfs_link_entry_to_kobj() was returning -ENOENT, which it
should do when target is not found. This was propagated to tpm_tis and
it probably messes up clean up somehow.

I have to test my hypothesis as soon as possible. The fix that I pushed
is still valid no matter which way the things are.

> [...]
> 
> -- 
> - Jeremiah Mahler

/Jarkko

------------------------------------------------------------------------------
Jeremiah Mahler Nov. 7, 2015, 11:11 p.m. UTC | #9
Jarkko,

On Sun, Nov 08, 2015 at 12:31:09AM +0200, Jarkko Sakkinen wrote:
> On Sat, Nov 07, 2015 at 10:08:56AM -0800, Jeremiah Mahler wrote:
> > Jarkko,
> > 
> > On Sat, Nov 07, 2015 at 01:41:37PM +0200, Jarkko Sakkinen wrote:
> > > On Sat, Nov 07, 2015 at 12:55:43PM +0200, Jarkko Sakkinen wrote:
> > > > On Fri, Nov 06, 2015 at 06:55:18PM -0800, Jeremiah Mahler wrote:
> > > > > Jarkko,
> > > > > 
[...]
> > > I pushed a fix over here:
> > > 
> > > https://github.com/jsakkine/linux-tpmdd/tree/fixes
> > > 
> > This fix does work since it effectively avoids the call to
> > __compat_only_sysfs_link_entry_to_kobj().
> > 
> > Have you tested cases where __compat_only_sysfs_link_entry_to_kobj()
> > is actually used?
> 
> Yes, of course I have. And I checked your DSDT and my assumption was
> correct. There was no DSM in the ACPI object.
> 
> However, there is probably another regression but it is caused by some
> patch that was added earlier. I strongly believe it is not caused by any
> of my 4.4 patches.
> 
> I think what was happening with you was that
> __compat_only_sysfs_link_entry_to_kobj() was returning -ENOENT, which it
> should do when target is not found. This was propagated to tpm_tis and
> it probably messes up clean up somehow.
> 
> I have to test my hypothesis as soon as possible. The fix that I pushed
> is still valid no matter which way the things are.
> 
> > [...]
> > 
> > -- 
> > - Jeremiah Mahler
> 
> /Jarkko

It sounds like you have the problem figured out and have a good fix.
If I can do anything else to help let me know :-)
Jarkko Sakkinen Nov. 8, 2015, 12:49 a.m. UTC | #10
On Sat, Nov 07, 2015 at 03:11:47PM -0800, Jeremiah Mahler wrote:
> Jarkko,
> 
> On Sun, Nov 08, 2015 at 12:31:09AM +0200, Jarkko Sakkinen wrote:
> > On Sat, Nov 07, 2015 at 10:08:56AM -0800, Jeremiah Mahler wrote:
> > > Jarkko,
> > > 
> > > On Sat, Nov 07, 2015 at 01:41:37PM +0200, Jarkko Sakkinen wrote:
> > > > On Sat, Nov 07, 2015 at 12:55:43PM +0200, Jarkko Sakkinen wrote:
> > > > > On Fri, Nov 06, 2015 at 06:55:18PM -0800, Jeremiah Mahler wrote:
> > > > > > Jarkko,
> > > > > > 
> [...]
> > > > I pushed a fix over here:
> > > > 
> > > > https://github.com/jsakkine/linux-tpmdd/tree/fixes
> > > > 
> > > This fix does work since it effectively avoids the call to
> > > __compat_only_sysfs_link_entry_to_kobj().
> > > 
> > > Have you tested cases where __compat_only_sysfs_link_entry_to_kobj()
> > > is actually used?
> > 
> > Yes, of course I have. And I checked your DSDT and my assumption was
> > correct. There was no DSM in the ACPI object.
> > 
> > However, there is probably another regression but it is caused by some
> > patch that was added earlier. I strongly believe it is not caused by any
> > of my 4.4 patches.
> > 
> > I think what was happening with you was that
> > __compat_only_sysfs_link_entry_to_kobj() was returning -ENOENT, which it
> > should do when target is not found. This was propagated to tpm_tis and
> > it probably messes up clean up somehow.
> > 
> > I have to test my hypothesis as soon as possible. The fix that I pushed
> > is still valid no matter which way the things are.
> > 
> > > [...]
> > > 
> > > -- 
> > > - Jeremiah Mahler
> > 
> > /Jarkko
> 
> It sounds like you have the problem figured out and have a good fix.
> If I can do anything else to help let me know :-)

Turns out that after all the second issue that I described was also
because of this patch.

Clean up was not done properly when that function. I revised my fix.

If you want to help, check that the revised patch works I'll add
Tested-by to the patch. Thank you!

> -- 
> - Jeremiah Mahler

/Jarkko

------------------------------------------------------------------------------
Jeremiah Mahler Nov. 8, 2015, 3:04 a.m. UTC | #11
Jarkko,

On Sun, Nov 08, 2015 at 02:49:06AM +0200, Jarkko Sakkinen wrote:
> On Sat, Nov 07, 2015 at 03:11:47PM -0800, Jeremiah Mahler wrote:
> > Jarkko,
> > 
[...]
> > 
> > It sounds like you have the problem figured out and have a good fix.
> > If I can do anything else to help let me know :-)
> 
> Turns out that after all the second issue that I described was also
> because of this patch.
> 
> Clean up was not done properly when that function. I revised my fix.
> 
> If you want to help, check that the revised patch works I'll add
> Tested-by to the patch. Thank you!
> 
> > -- 
> > - Jeremiah Mahler
> 
> /Jarkko

I tested the patch (link below) and it works with one small caveat.
The patch would not apply because line 231 uses list_add_tail_rcu
instead of list_add_rcu.  I am working from commit 9b774d5cf2d where
the problem started.  Your repo likely has other changes involved.

https://github.com/jsakkine/linux-tpmdd/commit/73ea7e0b8045f9610c3274bcefaf89b7a05ee781
Jarkko Sakkinen Nov. 8, 2015, 7:46 a.m. UTC | #12
On Sat, Nov 07, 2015 at 07:04:04PM -0800, Jeremiah Mahler wrote:
> Jarkko,
> 
> On Sun, Nov 08, 2015 at 02:49:06AM +0200, Jarkko Sakkinen wrote:
> > On Sat, Nov 07, 2015 at 03:11:47PM -0800, Jeremiah Mahler wrote:
> > > Jarkko,
> > > 
> [...]
> > > 
> > > It sounds like you have the problem figured out and have a good fix.
> > > If I can do anything else to help let me know :-)
> > 
> > Turns out that after all the second issue that I described was also
> > because of this patch.
> > 
> > Clean up was not done properly when that function. I revised my fix.
> > 
> > If you want to help, check that the revised patch works I'll add
> > Tested-by to the patch. Thank you!
> > 
> > > -- 
> > > - Jeremiah Mahler
> > 
> > /Jarkko
> 
> I tested the patch (link below) and it works with one small caveat.
> The patch would not apply because line 231 uses list_add_tail_rcu
> instead of list_add_rcu.  I am working from commit 9b774d5cf2d where
> the problem started.  Your repo likely has other changes involved.
> 
> https://github.com/jsakkine/linux-tpmdd/commit/73ea7e0b8045f9610c3274bcefaf89b7a05ee781

Yes, that branch also fix that changes one list_add_rcu() to
list_add_tail_rcu(). I'll add Tested-by to the commit message. Thanks
again for the good work.

> -- 
> - Jeremiah Mahler

/Jarkko

------------------------------------------------------------------------------
Jason Gunthorpe Nov. 9, 2015, 10:32 p.m. UTC | #13
On Fri, Nov 06, 2015 at 04:23:56PM -0800, Jeremiah Mahler wrote:
> On Fri, Oct 16, 2015 at 09:40:22PM +0300, Jarkko Sakkinen wrote:
> > Added a new function __compat_only_sysfs_link_group_to_kobj() that adds
> > a symlink from attribute or group to a kobject. This needed for
> > maintaining backwards compatibility with PPI attributes in the TPM
> > driver.

> One problem with this patch is that it adds a bunch of code but none of
> it is used.  So if there is a bug, git bisect will point to a later
> patch instead of this one.

It is fairly standard practice to split cross-subsystem patches like
this, it aids review. git bisect is not a perfect tool.

Jason

------------------------------------------------------------------------------
Presto, an open source distributed SQL query engine for big data, initially
developed by Facebook, enables you to easily query your data on Hadoop in a 
more interactive manner. Teradata is also now providing full enterprise
support for Presto. Download a free open source copy now.
http://pubads.g.doubleclick.net/gampad/clk?id=250295911&iu=/4140
diff mbox

Patch

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index 39a0199..e123659 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -352,3 +352,47 @@  void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name,
 	}
 }
 EXPORT_SYMBOL_GPL(sysfs_remove_link_from_group);
+
+/**
+ * __compat_only_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 __compat_only_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(__compat_only_sysfs_link_entry_to_kobj);
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 9f65758..ea090ea 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -268,6 +268,9 @@  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 __compat_only_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);
 
@@ -451,6 +454,14 @@  static inline void sysfs_remove_link_from_group(struct kobject *kobj,
 {
 }
 
+static inline int __compat_only_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)
 {