Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

thunderbolt-power: Use bolt force power API if available #684

Merged
merged 1 commit into from Aug 28, 2018

Conversation

superm1
Copy link
Member

@superm1 superm1 commented Aug 23, 2018

This is new support for bolt supported by
https://gitlab.freedesktop.org/bolt/bolt/merge_requests/101

@superm1 superm1 requested review from gicmo and hughsie Aug 23, 2018
@superm1
Copy link
Member Author

@superm1 superm1 commented Aug 23, 2018

@superm1 superm1 force-pushed the wip/superm1/bolt-force-power branch 2 times, most recently from 7a1c878 to e8d7fd1 Compare Aug 23, 2018

if (g_unix_fd_list_get_length (fds) != 1) {
g_set_error (error, G_IO_ERROR, G_IO_ERROR_INVALID_ARGUMENT,
"invalid number of file descriptors retunred: %d",
Copy link
Contributor

@YehezkelShB YehezkelShB Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

retunred - typo

Copy link
Member Author

@superm1 superm1 Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed.

g_debug ("Setting force power to %d using bolt", enable);
if (enable)
return fu_plugin_thunderbolt_power_bolt_force_power (plugin, error);
if (data->bolt_fd > 0)
Copy link
Contributor

@YehezkelShB YehezkelShB Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we can be sure we'll never get 0 fd from dbus. But don't you need to zero it here so it'll be zero if enable fails next time?

Copy link
Contributor

@westeri westeri Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also zero is a valid file descriptor. Typically -1 is used to represent invalid one.

Copy link
Member Author

@superm1 superm1 Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, adjusted the usage in the code.

g_unix_fd_list_get_length (fds));
return FALSE;
}
data->bolt_fd = g_unix_fd_list_get (fds, 0, NULL);
Copy link
Member

@hughsie hughsie Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused why we need to use fd's like a handle -- why isn't bolt just using "NameOwnerChanged" to detect clients using ForcePower falling off the bus? The normal way clients do this kind of thing is with two methods, e.g. ForcePower() and UnForcePower() using a uint32_t "token" that allows a client to refcount. Falling off the bus is the same as UnForcePower()-ing for all tokens from that sender.

Copy link
Collaborator

@gicmo gicmo Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I am a bit confused :) I personally think that using an fd as "token" that can be easily released via close is more elegant than having an extra dbus method plus another uint token. On the bolt side the implementation also quite straight forward, create a named pipe with a GIOChannel as the even watch (and that is all for book-keeping). One cool thing about the named pipe is that we can recover from boltd crashes and restarts (--replace) because we just re-open all the named pipes that were not closed (and that works really well in my tests).
But maybe I am missing something, do you see any big disadvantage?

-1,
NULL,
NULL);
data->bolt_supported = (val != NULL);
Copy link
Collaborator

@gicmo gicmo Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently that call will return successfully with an empty array even if boltd did not detect support for force power, i.e. the wmi sysfs device is missing. I could change it to return an error (NOT_SUPPORTED or something) in that case, but I wonder if I should not change the "PowerState" enum property to include a unsupported value. Any opinions?

Copy link
Member Author

@superm1 superm1 Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the wmi sysfs is missing this plugin will fail to load at coldplug (it still looks for this file itself for fallback currently).

It's entirely possible however that the kernel unloads the module at runtime so I would prefer to keep the test as to whether the bolt daemon has this functionality. If the module comes and goes the calls will succeed or fail when tried.

Copy link
Member

@hughsie hughsie Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if it should be a different plugin entirely...

Copy link
Member Author

@superm1 superm1 Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, keeping them together in the same fwupd plugin is actually ideal in my mind. The whole reason for this bolt feature is so that only one userspace is accessing the kernel method to avoid software stomping on each other. If bolt is there, it should be used. If not, use the kernel directly. Eventually when new enough bolt is everywhere that we care about, I would actually prefer to drop the direct kernel support from fwupd.

If bolt goes into it's own plugin (say bolt-thunderbolt-power and this plugin renames to kernel-thunderbolt-power) the model for when to use the kernel-thunderbolt-power plugin becomes more complicated. There isn't really a device registered signal to look for, so how will fwupd know which to prefer?

Copy link
Member Author

@superm1 superm1 Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about this some more this morning, and I think you're right - we need some assurance from bolt that it can actually force power. Returning an unsupported from PowerState property is sufficient as long as a fresh GDbusProxy is used every time it's checked.

Copy link
Collaborator

@gicmo gicmo Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@superm1 I will go and implement that now then. I also think that GDBusProxy should keep track of property updates, but in any case a manual call to the org.freedesktop.DBus.Properties.Get method should always work, i.e. something like:

g_dbus_proxy_call (proxy, 
                   "org.freedesktop.DBus.Properties.Get", 
                   g_variant_new ("(ss)", BOLT_DBUS_INTERFACE, "PowerState"),
[...]

Copy link
Member Author

@superm1 superm1 Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than the client need to know to look for magic PowerState property enums (ie !=unsupported), maybe a third method ForcePowerSupported would be better? Then that can instantly check whether the kernel module is loaded and give a boolean answer.

Up to you, i'll adjust the PR for whichever you decide.

Copy link
Collaborator

@gicmo gicmo Aug 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this some more and decided to extract the whole force power related properties/methods into its own interface: org.freedesktop.bolt1.Power. Both methods (ForcePower, ListGuards), the property (PowerState as State) and additionally a new boolean Supported property are exposed there (and the methods removed from the bolt1.Manager interface). I think for this PR changing BOLT_DBUS_INTERFACE to org.freedesktop.bolt1.Power should suffice; the new Supported boolean, that can be used to check if force-powering is supported or not, should come in handy.

I am also add support for monitoring changes to the intel-wmi-thunderbolt module so that the Supported property is updated accordingly. For this I am using the bind and unbind uevents (see here) — I hope that is a good way to do that.

One more thing: it seems to be totally possible to unload the module, even with force_power is set to 1 and boltd will not yet do the right thing in that case. This needs a few more changes.

@superm1
Copy link
Member Author

@superm1 superm1 commented Aug 24, 2018

@gicmo Once you add an unsupported enum, I think we also need a way to be notified if force power changes to/from supported (eg kernel module unload/load) that we can subscribe to.

@superm1 superm1 force-pushed the wip/superm1/bolt-force-power branch 2 times, most recently from ae1192c to 1638839 Compare Aug 24, 2018
@gicmo
Copy link
Collaborator

@gicmo gicmo commented Aug 24, 2018

@superm1 I was thinking that subscribing to GDBusProxy's g-properties-changed should work. Maybe I am missing something?

@superm1
Copy link
Member Author

@superm1 superm1 commented Aug 24, 2018

Yeah that should work.

@@ -199,9 +318,11 @@ fu_plugin_init (FuPlugin *plugin)
G_CALLBACK (udev_uevent_cb), plugin);
/* initially set to true, will wait for a device_register to reset */
data->needs_forcepower = TRUE;
/* will reset when neeeded */
Copy link
Contributor

@westeri westeri Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo 'neeeded' -> 'needed'

Copy link
Member Author

@superm1 superm1 Aug 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, fixed.

@superm1 superm1 force-pushed the wip/superm1/bolt-force-power branch from 1638839 to d4ed8e3 Compare Aug 27, 2018
@superm1
Copy link
Member Author

@superm1 superm1 commented Aug 27, 2018

@gicmo thanks. I've adjusted for your changes and use the Supported property now. I've done some testing with/without that bolt PR in place to make sure the old kernel way works and also that loading intel-wmi-thunderbolt after fwupd started up works (which did prefer to and use bolt still).

@hughsie
Copy link
Member

@hughsie hughsie commented Aug 28, 2018

LGTM now, if that matters.

@superm1
Copy link
Member Author

@superm1 superm1 commented Aug 28, 2018

OK thanks. @gicmo merged it in bolt, so I'll merge it here too then.

@superm1 superm1 merged commit c07ce5b into master Aug 28, 2018
2 checks passed
@superm1 superm1 deleted the wip/superm1/bolt-force-power branch Aug 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants