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 <[email protected]>
> ---
> 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.
[...]
--
- Jeremiah Mahler
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
> > > ---
> > > 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
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?
[...]
--
- Jeremiah Mahler
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
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 :-)
--
- Jeremiah Mahler
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
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
--
- Jeremiah Mahler
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