Skip to content
This repository has been archived by the owner on Nov 1, 2021. It is now read-only.

session/logind: keep active for pause_device gone #1696

Merged
merged 1 commit into from
May 15, 2019
Merged

session/logind: keep active for pause_device gone #1696

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion backend/session/logind.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ static int pause_device(sd_bus_message *msg, void *userdata,
goto error;
}

if (major == DRM_MAJOR) {
if (major == DRM_MAJOR && strcmp(type, "gone") != 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should only set active = false if strcmp(type, "pause") == 0?

Copy link

Choose a reason for hiding this comment

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

I can test that tonight if you want

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, please do.

We might want to destroy the sub-backend if the device is gone.

Copy link

Choose a reason for hiding this comment

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

Your way seems to work as well, the following log has some debug code from @RedSoxFan as well.

https://mthode.org/dist/logs/tb-disconnect-working.log.xz

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we should only set active = false if strcmp(type, "pause") == 0?

Looking at the documentation at https://www.freedesktop.org/wiki/Software/systemd/logind/ (sorry can't link to an anchor since the id's are not unique...), it looks like there are three values: force, pause, and gone. force is an asynchronous notification that the device was paused, pause is a synchronous notification the device is being paused, and gone is a notification that the device has been removed.

Based on that, I think that the condition is correct as-is. Thoughts?

We might want to destroy the sub-backend if the device is gone.

How would I go about doing that? I haven't really touched wlroots much

Copy link
Member

@emersion emersion May 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Based on that, I think that the condition is correct as-is. Thoughts?

Indeed, you're right!

How would I go about doing that? I haven't really touched wlroots much

wlr_backend_destroy will probably do the trick. Ah, expect we don't have access to the sub-backend here?

Let's just leave it for another PR then.

Copy link

Choose a reason for hiding this comment

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

So, this pr is invalid because it's correct as is? What should be done to address the lock up issue then?

Copy link
Member

Choose a reason for hiding this comment

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

No, this PR LGTM.

assert(session->has_drm);
session->base.active = false;
wlr_signal_emit_safe(&session->base.session_signal, session);
Expand Down