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

Add support for the new kernel API for force power #245

Merged
merged 2 commits into from Sep 12, 2017

Conversation

3 participants
@superm1
Collaborator

superm1 commented Sep 12, 2017

This is a WIP with more testing still needed. Early feedback welcome however.

It wraps around this API being introduced in kernel 4.14:
http://git.infradead.org/linux-platform-drivers-x86.git/commit/d08e3b522bdaccde031a394039b299bfd8c18492

@superm1 superm1 requested review from gicmo and hughsie Sep 12, 2017

@superm1

This comment has been minimized.

Collaborator

superm1 commented Sep 12, 2017

I've only got single controller configurations available to me. Here are the tests I've done so far:

  1. Start with controller in low power mode (nothing plugged in). Start fwupd, make sure device is detected and returns to low power mode after settling time.
  2. Start with controller in high power mode (force powered outside fwupd or something plugged in). Start fwupd make sure that if something is unplugged or it's force powered off that the device stays that way.

Those both work correct for me in my tests. I still need to test that the update stuff works properly.

g_list_free (devices);
if (built_path)
return g_strdup (built_path);

This comment has been minimized.

@ybernat

ybernat Sep 12, 2017

Contributor

g_steal_pointer() maybe?

This comment has been minimized.

@ybernat

ybernat Sep 12, 2017

Contributor

IIUC, we don't need the if in this case

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

Yep, using that thanks!

"force_power", NULL);
if (g_file_test (built_path, G_FILE_TEST_IS_REGULAR))
break;
built_path = NULL;

This comment has been minimized.

@ybernat

ybernat Sep 12, 2017

Contributor

Is this triggers the free from g_autofree? I assume not. So an explicit g_free() is needed here.
Maybe you want to have a local g_autofree instead and if the path test succeeded to find the file, g_steal_pointer() it to the outer pointer.

This comment has been minimized.

@hughsie

hughsie Sep 12, 2017

Owner

I also think this is a confusing function. Maybe we could just use G_DEFINE_AUTOPTR_CLEANUP_FUNC to define a function that frees the list and unrefs the contents. Then you could just do g_steal_pointer (&built_path) in the exists case and rely on the autofree otherwise.

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

Take a look what I did in the commit I just pushed. I think this should be clearer and take care of the potential memory leak.

devices = g_udev_client_query_by_subsystem (data->udev, "wmi");
for (GList *l = devices; l != NULL; l = l->next) {
GUdevDevice *device = l->data;
basepath = g_udev_device_get_sysfs_path (device);

This comment has been minimized.

@ybernat

ybernat Sep 12, 2017

Contributor

Do we need to check for NULL here?

This comment has been minimized.

@hughsie

hughsie Sep 12, 2017

Owner

g_strcmp0 does that, it's the 0 bit at the end :)

This comment has been minimized.

@ybernat

ybernat Sep 12, 2017

Contributor

Not sure I understood your comment. If g_build_path gets basepath which is NULL, will it return an empty path or "/"?
If the latter, the g_file_test will succeed but for the wrong path.

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

I think may as well add an extra guard to this to test for NULL. The glib documentation isn't very clear about what happens in the case of a null string (only an empty one).

if (enable)
ret = write (fd, "1", 1);
else
ret = write (fd, "0", 1);

This comment has been minimized.

@ybernat

ybernat Sep 12, 2017

Contributor

or ret = write (fd, enable ? "1" : "0", 1);
Really a matter of taste.

This comment has been minimized.

@hughsie

hughsie Sep 12, 2017

Owner

I prefer the trigraph too.

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

Adjusted

g_io_error_from_errno (errno),
"could not write to force_power': %s",
g_strerror (errno));
g_close (fd, error);

This comment has been minimized.

@ybernat

ybernat Sep 12, 2017

Contributor

NULL instead of error as error is already set?

This comment has been minimized.

@hughsie

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

Adjusted

@@ -729,6 +817,13 @@ fu_plugin_coldplug (FuPlugin *plugin, GError **error)
fu_plugin_thunderbolt_add (plugin, device);
}
if (g_list_length (devices) == 0) {
fu_plugin_thunderbolt_set_force_power (plugin, TRUE, error);

This comment has been minimized.

@ybernat

ybernat Sep 12, 2017

Contributor

Check for error?

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

Adjusted (in coldplug of new plugin)

devpath);
return FALSE;
if (fu_plugin_thunderbolt_can_force_power (plugin)) {
fu_plugin_thunderbolt_set_force_power (plugin, TRUE, error);

This comment has been minimized.

@ybernat

ybernat Sep 12, 2017

Contributor

Check for error?

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

Checked in new plugin

if (udevice == NULL) {
if (force_powered)
fu_plugin_thunderbolt_set_force_power (plugin, FALSE, error);

This comment has been minimized.

@ybernat

ybernat Sep 12, 2017

Contributor

Check for error? Especially as it can fail the next set error.

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

Checked in new plugin

@ybernat

This comment has been minimized.

Contributor

ybernat commented Sep 12, 2017

I've only got single controller configurations available to me

I don't think there is a multiple-controller configuration available right now besides things like Mac Pro (which this work isn't relevant to, as much as I understand).
Your test cases looks like they cover the logic we wanted to implement, so LGTM.

@@ -326,13 +325,12 @@ fu_dell_toggle_flash (FuDevice *device, GError **error, gboolean enable)
if (!fu_device_has_flag (device, FWUPD_DEVICE_FLAG_UPDATABLE))
return TRUE;
tmp = fu_device_get_plugin (device);
if (!((g_strcmp0 (tmp, "thunderbolt") == 0) ||

This comment has been minimized.

@hughsie

hughsie Sep 12, 2017

Owner

What happens if we're using new fwupd on an old kernel?

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

I keep going back and forth on this. The way the dell plugin works to force power is too unconditional and does't fit in the safely designed logic used here. I'd like to have the dell plugin to fall back upon if possible. If I do come up with a way to split this up to thunderbolt/thunderbolt-power with an intra-plugin API I'll retrofit the Dell plugin the same way instead.

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

Since all of the (smarter) logic around when to force power and when not to lives in thunderbolt-power, what do you think about #ifdef(HAVE_DELL) in the new plugin to allow a fallback?

Otherwise I think the the way to go is some sort of inter-plugin API with thunderbolt <-thunderbolt-power-> dell and duplicating most of the logic in thunderbolt-power in the dell plugin too.

This comment has been minimized.

@hughsie

hughsie Sep 12, 2017

Owner

I'd much rather avoid HAVE_DELL if at all possible. What would you need to duplicate? I was under the impression that the dell plugin would be doing less, not more...

This comment has been minimized.

@hughsie

hughsie Sep 12, 2017

Owner

Wild idea: could you use the new CONFLICTS thing and split out a dell_legacy plugin that does the tbt force power stuff on old kernels? Maybe we're just okay with requiring a new kernel for thunderbolt flashing things to work, but I've also got a neuron wired to the RHEL process too.

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

Well the dell plugin really should have been following this approach (only power on when it's needed to rather than all the time). I'm leaning on since you need a new kernel for thunderbolt flashing, one newer kernel version for thunderbolt at bootup working right, what's one newer one for thunderbolt force power?

Fedora & Ubuntu will quickly pick up a new kernel.
I think with RHEL the driver is contained enough that it should be backportable.

I'm gonna go with kill the TBT force power from Dell plugin and say require this kernel interface instead.

This comment has been minimized.

@hughsie

hughsie Sep 12, 2017

Owner

Works for me.

path = fu_plugin_thunderbolt_get_force_power_path (plugin);
if (path == NULL) {
return FALSE;

This comment has been minimized.

@hughsie

hughsie Sep 12, 2017

Owner

If we're returning FALSE we need to set a GError.

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

In the way this commit was done this didn't work well (since it was an implicit test for support). In the new plugin approach I moved this over to have an explicit test for support and set the GError in this path.

/* resets force power to disabled on g_timeout */
static gboolean
fu_plugin_thunderbolt_reset_force_power (gpointer data)

This comment has been minimized.

@hughsie

hughsie Sep 12, 2017

Owner

If this is a callback, the convention I've used elsewhere is ending the function name with _cb.

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

Adjusted

static gboolean
fu_plugin_thunderbolt_reset_force_power (gpointer data)
{
fu_plugin_thunderbolt_set_force_power ((FuPlugin *) data, FALSE, NULL);

This comment has been minimized.

@hughsie

hughsie Sep 12, 2017

Owner

Not FU_PLUGIN()? That way you get a typecheck.

This comment has been minimized.

@hughsie

hughsie Sep 12, 2017

Owner

You also probably want to g_warning the error too.

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

Added both.

@hughsie

This comment has been minimized.

Owner

hughsie commented Sep 12, 2017

On a more general note, would it be clearer to move all this functionality to a new plugin, perhaps thunderbolt-power rather than sprinkling code all through the thunderbolt plugin. That way the new plugin could just self disable if the kernel support isn't in place. The device_register, coldplug_prepare and update_prepare vfuncs give you everything you need I think. This way you could also put the sysfs path in the new plugin private data too.

@superm1

This comment has been minimized.

Collaborator

superm1 commented Sep 12, 2017

On a more general note, would it be clearer to move all this functionality to a new plugin, perhaps thunderbolt-power rather than sprinkling code all through the thunderbolt plugin. That way the new plugin could just self disable if the kernel support isn't in place. The device_register, coldplug_prepare and update_prepare vfuncs give you everything you need I think. This way you could also put the sysfs path in the new plugin private data too.

Yes and no. The set of logic that we discussed to make this work is very prescriptive about when to force power and it's not unconditional. It will require some thought how to map to the existing intra-plugin API.

  • device_register would work fine to set the CanForcePower (similar to how the Dell plugin did it previously)
  • coldplug_prepare won't yet know whether any TBT controllers were on the system unless it duplicates the exact same gudev code thunderbolt plugin does at coldplug to walk the udev db looking for them.
  • coldplug_cleanup might fit better to how the logic works there and only run the udev database walking code if needed.
  • update_prepare As you pointed out this would need to know the sysfs path of the device. This would clean up the update routine (particularly the error paths).
@hughsie

This comment has been minimized.

Owner

hughsie commented Sep 12, 2017

coldplug_prepare won't yet know whether any TBT controllers were on the system

Are the registered callbacks not fired on coldplug? If we could guarantee the thunderbolt plugin was coldplugged before thunderbolt-power (i.e. we enforce a plugin order), would that work?

@superm1

This comment has been minimized.

Collaborator

superm1 commented Sep 12, 2017

Are the registered callbacks not fired on coldplug? If we could guarantee the thunderbolt plugin was coldplugged before thunderbolt-power (i.e. we enforce a plugin order), would that work?

Oh I think I see what you mean. Rather than look for the registered callback, look for the lack of a registered callback. If plugin order is enforced and no callback has come through the thunderbolt-power plugin will know to run during it's coldplug routine.

@hughsie

This comment has been minimized.

Owner

hughsie commented Sep 12, 2017

Exactly that. I can have a go at defining plugin deps, it'll be very similar for what I did in gnome-software.

@superm1

This comment has been minimized.

Collaborator

superm1 commented Sep 12, 2017

That would be great. I'll retrofit this over to a new plugin and apply all your guys' comments. Thanks!

@hughsie

This comment has been minimized.

Owner

hughsie commented Sep 12, 2017

Would #247 do?

devices = g_udev_client_query_by_subsystem (data->udev, "wmi");
for (GList* l = devices; l != NULL; l = l->next) {
g_autoptr(GUdevDevice) device = l->data;

This comment has been minimized.

@hughsie

hughsie Sep 12, 2017

Owner

that works too! :)

This comment has been minimized.

@hughsie

hughsie Sep 12, 2017

Owner

Nope, forget that, it doesn't work. If you break out of thefor early the remaining GUdevDevice's are not unref'd.

"failed to open %s", path);
return FALSE;
}
ret = write (fd, enable ? "1" : "0", 1);

This comment has been minimized.

@hughsie

hughsie Sep 12, 2017

Owner

spaces?

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

Ah that's what I get for copying and pasting from you guys. My editor didn't fixup, I'll fix in another commit.

@superm1

This comment has been minimized.

Collaborator

superm1 commented Sep 12, 2017

@ybernat 00f3531 is of particular interest since we were discussing that about the kernel module. Because the kernel module emits a change event this allows modprobe intel-wmi-thunderbolt after fwupd is running and letting it force power to find the device still.

@superm1

This comment has been minimized.

Collaborator

superm1 commented Sep 12, 2017

OK so other than what to do with the old Dell way to do this, this passes all my unit tests (including flashing an update with nothing plugged in).

G_DEFINE_AUTOPTR_CLEANUP_FUNC(GUdevDevice, g_object_unref)
#endif
#define TBT_NEW_DEVICE_TIMEOUT 2

This comment has been minimized.

@hughsie

hughsie Sep 12, 2017

Owner

maybe a comment explaining what unit this number is, and what it means?

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

Added

fu_plugin_thunderbolt_power_supported (FuPlugin *plugin)
{
g_autofree gchar *path = NULL;
path = fu_plugin_thunderbolt_power_get_path (plugin);

This comment has been minimized.

@hughsie

hughsie Sep 12, 2017

Owner

I thought the idea was to move the path to the plugin FuPluginData struct?

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

Ah right, I forgot about that. Adjusted it, by doing that no longer store a boolean object and calculate the path every usage.

}
/* driver went away */
if (data->force_path)

This comment has been minimized.

@ybernat

ybernat Sep 12, 2017

Contributor

I'm not sure I get it.
The only place this is set is line 72 above and then we don't reach this.
If this is from a previous run of this method, then line 72 need to g_free force_path before assigning to it.

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

There's two ways to enter the function:

  1. plugin initialization
  2. intel-wmi-thunderbolt module change event

If you initialize with the module loaded this gets set during init, and then you unload the module it needs to be unset when it's called again. If the module gets loaded again, you would have another change event and it would get set again.

If you initialize without the module loaded this hasn't yet been set. You load the module and it gets set. If you unload again it needs to get unset.

This comment has been minimized.

@ybernat

ybernat Sep 12, 2017

Contributor

I still think it's better to move this before the loop, otherwise, if the module generates change event for any other reason (or got removed and reloaded before this had a chance to run) there is still a leak.

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

That's true. I'll move it.

"force_power", NULL);
if (g_file_test (built_path, G_FILE_TEST_IS_REGULAR)) {
data->force_path = g_steal_pointer (&built_path);
return;

This comment has been minimized.

@ybernat

ybernat Sep 12, 2017

Contributor

And then what with the rest of the devices in the list? Leak?

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

The rest of the devices should free automatically from g_autofree and g_autoptr usage. I adjusted the scope for the g_autofree to be within the for loop. It should free when the for loop is exited (and function exited).

This comment has been minimized.

@ybernat

ybernat Sep 12, 2017

Contributor

devices, the list itself, has g_autoptr guard, but here we are talking about each of the l->data inside it. If freeing the list frees the inside pointers too, the issue is much worse where it auto free device on each iteration without setting the original l->data to NULL.

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

Oh I think I finally understand you. Good eye with this.
g_object_unref needs to be called on every element in devices. If the for loop bails early this won't happen.

I had this right before calling g_list_foreach previously and convinced myself that wasn't needed.

dell: drop thunderbolt force power code
This is replaced by the WMI force power interface.
{
if (!fu_plugin_thunderbolt_power_set (FU_PLUGIN (data), FALSE, NULL))
g_warning ("failed to reset thunderbolt power");
return FALSE;

This comment has been minimized.

@ybernat

ybernat Sep 12, 2017

Contributor

Always FALSE? I'd think always TRUE if it must return anything or using the result of set.

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

For the g_timeout to go away on the reset callback you have to return FALSE. It's for g_timeout's to call repeatedly on an interval instead of just once when you return TRUE.

This comment has been minimized.

@ybernat

ybernat Sep 12, 2017

Contributor

Oh, sorry. Next time I'll look it up myself before asking...
Thanks!

This comment has been minimized.

@hughsie

hughsie Sep 12, 2017

Owner

Note, you can use G_SOURCE_REMOVE which makes it a bit clearer in this kind of situation. I'm bad at using it myself.

if (g_str_equal (action, "change")) {
fu_plugin_thunderbolt_power_get_path (plugin);
fu_plugin_set_enabled (plugin, TRUE);
fu_plugin_recoldplug (plugin);

This comment has been minimized.

@ybernat

ybernat Sep 12, 2017

Contributor

And Dell-force-power plugin will be disabled by this?

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

Currently; yes. I'd like to find a way to have it fall back to Dell interface (my preference if #ifdef (HAVE_DELL) and code living in this plugin), but thinking about ways to let the 3 plugins work together.

The dell approach should really follow the exact same logic we discussed for this (only force powering on when it needs to - not every time).

This comment has been minimized.

@hughsie

hughsie Sep 12, 2017

Owner

Can't the existing dell plugin just do the "dumb" algorithm like it's doing now?

G_DEFINE_AUTOPTR_CLEANUP_FUNC(GUdevDevice, g_object_unref)
#endif
/* empirically mesaured amount of time for the TBT device to come and go */

This comment has been minimized.

@hughsie

hughsie Sep 12, 2017

Owner

sp: mesaured, also need to specify it's seconds somewhere. I think in other places of the code it just has TIMEOUT 2 /* s */

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

Fixed

/* in case driver went away */
if (data->force_path != NULL)
g_free (data->force_path);

This comment has been minimized.

@hughsie

hughsie Sep 12, 2017

Owner

you need to do data->force_path = NULL as well I think otherwise you're pointing to free'd memory if the list below doesn't match.

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

Fixed

fu_plugin_thunderbolt_power_supported (FuPlugin *plugin)
{
FuPluginData *data = fu_plugin_get_data (plugin);
return (data->force_path != NULL);

This comment has been minimized.

@hughsie

hughsie Sep 12, 2017

Owner

No brackets reqd.

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

Fixed

FuPluginData *data = fu_plugin_get_data (plugin);
g_object_unref (data->udev);
if (data->force_path)
g_free (data->force_path);

This comment has been minimized.

@hughsie

hughsie Sep 12, 2017

Owner

you don't need the if here, g_free(NULL) is explicitly okay.

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

Fixed

if (g_strcmp0 (fu_device_get_plugin (device), "thunderbolt") == 0 &&
fu_plugin_thunderbolt_power_supported (plugin)) {
if (data->needs_forcepower)
data->needs_forcepower = FALSE;

This comment has been minimized.

@hughsie

hughsie Sep 12, 2017

Owner

Does it need the if here? All paths lead to FALSE, right?

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

Fixed

return FALSE;
data->needs_forcepower = TRUE;
g_usleep (TBT_NEW_DEVICE_TIMEOUT * G_USEC_PER_SEC);

This comment has been minimized.

@hughsie

hughsie Sep 12, 2017

Owner

Can you add a comment here explaining why the sleep is required please.

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

Fixed

if (data->needs_forcepower &&
!fu_plugin_thunderbolt_power_set (plugin, FALSE, error))
return FALSE;

This comment has been minimized.

@hughsie

hughsie Sep 12, 2017

Owner

Weird indent here.

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

Fixed

if (data->needs_forcepower) {
if (!fu_plugin_thunderbolt_power_set (plugin, TRUE, error))
return FALSE;
g_timeout_add (TBT_NEW_DEVICE_TIMEOUT * 10000,

This comment has been minimized.

@hughsie

hughsie Sep 12, 2017

Owner

No saving the return value here worries me a bit. I think it might be safer to do data->timeout_id = g_timeout_add( and then in destroy do if (data->timeout_id != 0) g_source_remove (data->timeout_id) -- this way if we unload the plugin after doing coldplug but before doing the callback we don't explode. I think you might also need to do the g_source_remove thing in this function too for the recoldplug stuff too.

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

Fixed

@@ -0,0 +1,19 @@
cargs = ['-DG_LOG_DOMAIN="FuPluginThunderboltpower"']

This comment has been minimized.

@hughsie

hughsie Sep 12, 2017

Owner

Probably FuPluginThunderboltPower for convention here.

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

Fixed

/* intel-wmi-thunderbolt has been loaded/unloaded */
if (g_str_equal (action, "change")) {
fu_plugin_thunderbolt_power_get_path (plugin);
fu_plugin_set_enabled (plugin, TRUE);

This comment has been minimized.

@ybernat

ybernat Sep 12, 2017

Contributor

TRUE or only if get_path succeeded finding it? I mean, it needs to be disabled for unload, isn't it?

This comment has been minimized.

@superm1

superm1 Sep 12, 2017

Collaborator

It should get disabled from the recoldplug, but you're right may as well not schedule a re-coldplug if it's not going to be supported.

@hughsie

Once rebased, LGTM.

Use the intel-wmi-thunderbolt kernel module to force power
When available on a system this module will allow force powering a TBT device with nothing plugged in.
@superm1

This comment has been minimized.

Collaborator

superm1 commented Sep 12, 2017

OK, i'll give one more last test with recent changes and merge then.

@superm1 superm1 merged commit 8f17e1c into master Sep 12, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@superm1 superm1 deleted the wip/superm1/wmi-thunderbolt-forcepower branch Sep 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment