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

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

Merged
merged 2 commits into from Sep 12, 2017

Conversation

superm1
Copy link
Member

@superm1 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
Copy link
Member Author

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

g_steal_pointer() maybe?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, using that thanks!

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

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to check for NULL here?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

I prefer the trigraph too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Adjusted

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

Choose a reason for hiding this comment

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

NULL instead of error as error is already set?

Copy link
Member

Choose a reason for hiding this comment

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

Agree

Copy link
Member Author

Choose a reason for hiding this comment

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Check for error?

Copy link
Member Author

Choose a reason for hiding this comment

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Check for error?

Copy link
Member Author

Choose a reason for hiding this comment

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

Checked in new plugin


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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Checked in new plugin

@ybernat
Copy link
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) ||
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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...

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Works for me.


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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Adjusted

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

You also probably want to g_warning the error too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added both.

@hughsie
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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;
Copy link
Member

Choose a reason for hiding this comment

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

that works too! :)

Copy link
Member

Choose a reason for hiding this comment

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

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);
Copy link
Member

Choose a reason for hiding this comment

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

spaces?

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Member Author

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 superm1 force-pushed the wip/superm1/wmi-thunderbolt-forcepower branch from b06ae1d to fa5d5c8 Compare September 12, 2017 13:42
@superm1
Copy link
Member Author

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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Added

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

This is replaced by the WMI force power interface.
@superm1 superm1 force-pushed the wip/superm1/wmi-thunderbolt-forcepower branch from 18fda40 to 9573ed0 Compare September 12, 2017 15:21
{
if (!fu_plugin_thunderbolt_power_set (FU_PLUGIN (data), FALSE, NULL))
g_warning ("failed to reset thunderbolt power");
return FALSE;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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

@superm1 superm1 force-pushed the wip/superm1/wmi-thunderbolt-forcepower branch from 41888dc to 95f1c9b Compare September 12, 2017 17:52
G_DEFINE_AUTOPTR_CLEANUP_FUNC(GUdevDevice, g_object_unref)
#endif

/* empirically mesaured amount of time for the TBT device to come and go */
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed


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

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

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

Choose a reason for hiding this comment

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

No brackets reqd.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed