Discussion:
[PATCH] rules: load drivers only on "add" events
(too old to reply)
Dmitry Torokhov
2017-09-11 18:47:38 UTC
Permalink
Raw Message
Previously we were loading kernel modules on all device events save
for "remove". With the introduction of KOBJ_BIND/KOBJ_UNBIND this causes
issues, as driver modules that have devices bound to their drivers get
immediately reloaded, and it appears to the user that module unloading
does not work.

Let's change the rules to only load modules on "add" events instead.
---

As a side note, I am confused about handling authorship information in
systemd repository. The commit 9a39e1ce314d1a6f8a754f6dab040019239666a9
adding handling of bind/unbind actions is clearly taken from the email I
posted on systemd-devel mailing list on the 31st, however it lists
Lennart's name as the author?

rules/80-drivers.rules | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/rules/80-drivers.rules b/rules/80-drivers.rules
index 8551f47a4..16fa5d8e3 100644
--- a/rules/80-drivers.rules
+++ b/rules/80-drivers.rules
@@ -1,6 +1,6 @@
# do not edit this file, it will be overwritten on update

-ACTION=="remove", GOTO="drivers_end"
+ACTION!="add", GOTO="drivers_end"

ENV{MODALIAS}=="?*", RUN{builtin}+="kmod load $env{MODALIAS}"
SUBSYSTEM=="tifm", ENV{TIFM_CARD_TYPE}=="SD", RUN{builtin}+="kmod load tifm_sd"
--
2.14.1.581.gf28d330327-goog
--
Dmitry
systemd github import bot
2017-09-11 19:04:34 UTC
Permalink
Raw Message
Patchset imported to github.
To create a pull request, one of the main developers has to initiate one via:
<https://github.com/systemd/systemd/compare/master...systemd-mailing-devs:20170911184738.GA11495%40dtor-ws>

--
Generated by https://github.com/haraldh/mail2git
Mantas Mikulėnas
2017-09-11 19:23:44 UTC
Permalink
Raw Message
Post by Dmitry Torokhov
As a side note, I am confused about handling authorship information in
systemd repository. The commit 9a39e1ce314d1a6f8a754f6dab040019239666a9
adding handling of bind/unbind actions is clearly taken from the email I
posted on systemd-devel mailing list on the 31st, however it lists
Lennart's name as the author?
Many systemd developers only merge Git branches, not emailed patches, so
yours was automatically imported into a Git branch, from which Lennart made
a GitHub pull request.

The original pull-request had one commit authored by you. However, when
"squash" merge is used, GitHub appears to always use the PR submitter's
information in the final squashed commit. (I suppose it has to choose
*something*.)

Sorry about that; it seems that imported branches should be only merged in
full, not squashed.

Of course, a future-proof way to avoid this would be to open a GH pull
request yourself... (See the auto-reply from earlier.)
Post by Dmitry Torokhov
--
Mantas Mikulėnas <***@gmail.com>
Sent from my phone
Lennart Poettering
2017-09-12 11:38:00 UTC
Permalink
Raw Message
Post by Dmitry Torokhov
Previously we were loading kernel modules on all device events save
for "remove". With the introduction of KOBJ_BIND/KOBJ_UNBIND this causes
issues, as driver modules that have devices bound to their drivers get
immediately reloaded, and it appears to the user that module unloading
does not work.
Let's change the rules to only load modules on "add" events instead.
---
As a side note, I am confused about handling authorship information in
systemd repository. The commit 9a39e1ce314d1a6f8a754f6dab040019239666a9
adding handling of bind/unbind actions is clearly taken from the email I
posted on systemd-devel mailing list on the 31st, however it lists
Lennart's name as the author?
Oh sorry. As Mantas pointed out, the preferred way to accept patches
is via github PRs these days. To be friendly to submitters used to
old-style kernel submissions we have a bot running that permits us to
create a PR for your from patches submitted to the mailing list. PRs
created by the bot are assigned to the person who clicked on the bot's
email, which in this case was me. In github these PRs hence show up as
owned by me, but the invidual commits in them are still attributed to
you. But then, for single-patch PRs we don't do merges but
rebase+fforward, which github has an explicit button in its UI
for. But that rebase is kinda broken conceptually by them: it rewrites
the commits of the PR as a single commit attributed to the person who
clicked the button, losing all of the original attribution.

Quite frankly, it's a mess, and a serious brokeness in the github
UI. Anyway, sorry for the confusion, it was definitely not my
intention to steal your commits.

I figure in future we should keep in mind to never use github's
"rebase" merge button for PRs that include commits from people who are
not the PR owners. We should use real merges for all such cases to
avoid such fuck-ups.

For your new patch the bot's notification is this btw:

https://lists.freedesktop.org/archives/systemd-devel/2017-September/039513.html

And I clicked on the link it provides now, so further discussion
should now happen in this github PR:

https://github.com/systemd/systemd/pull/6802

Anyway, patch looks fine. Will merge as soon as the CI passes (and
this time with a real merge so that the attribution isn't lost)

And please consider submitting patches as github PRs right-away,
that's our preferred workflow these days.

Lennart
--
Lennart Poettering, Red Hat
Mantas Mikulėnas
2017-09-12 11:57:57 UTC
Permalink
Raw Message
Post by Lennart Poettering
But then, for single-patch PRs we don't do merges but
rebase+fforward, which github has an explicit button in its UI
for. But that rebase is kinda broken conceptually by them: it rewrites
the commits of the PR as a single commit attributed to the person who
clicked the button, losing all of the original attribution.
I think single commit is a different option (squash+merge); using
rebase+merge should still be ok, it keeps the individual commits and only
rewrites the "committer" info.
--
Mantas Mikulėnas <***@gmail.com>
Dmitry Torokhov
2017-09-12 17:16:27 UTC
Permalink
Raw Message
Post by Lennart Poettering
Post by Dmitry Torokhov
Previously we were loading kernel modules on all device events save
for "remove". With the introduction of KOBJ_BIND/KOBJ_UNBIND this causes
issues, as driver modules that have devices bound to their drivers get
immediately reloaded, and it appears to the user that module unloading
does not work.
Let's change the rules to only load modules on "add" events instead.
---
As a side note, I am confused about handling authorship information in
systemd repository. The commit 9a39e1ce314d1a6f8a754f6dab040019239666a9
adding handling of bind/unbind actions is clearly taken from the email I
posted on systemd-devel mailing list on the 31st, however it lists
Lennart's name as the author?
Oh sorry. As Mantas pointed out, the preferred way to accept patches
is via github PRs these days. To be friendly to submitters used to
old-style kernel submissions we have a bot running that permits us to
create a PR for your from patches submitted to the mailing list. PRs
created by the bot are assigned to the person who clicked on the bot's
email, which in this case was me. In github these PRs hence show up as
owned by me, but the invidual commits in them are still attributed to
you. But then, for single-patch PRs we don't do merges but
rebase+fforward, which github has an explicit button in its UI
for. But that rebase is kinda broken conceptually by them: it rewrites
the commits of the PR as a single commit attributed to the person who
clicked the button, losing all of the original attribution.
Quite frankly, it's a mess, and a serious brokeness in the github
UI. Anyway, sorry for the confusion, it was definitely not my
intention to steal your commits.
No worries, this commit is not something I would worry about too much, I
just wanted to raise the issue in general.
Post by Lennart Poettering
I figure in future we should keep in mind to never use github's
"rebase" merge button for PRs that include commits from people who are
not the PR owners. We should use real merges for all such cases to
avoid such fuck-ups.
https://lists.freedesktop.org/archives/systemd-devel/2017-September/039513.html
And I clicked on the link it provides now, so further discussion
https://github.com/systemd/systemd/pull/6802
Anyway, patch looks fine. Will merge as soon as the CI passes (and
this time with a real merge so that the attribution isn't lost)
And please consider submitting patches as github PRs right-away,
that's our preferred workflow these days.
OK, if I have more changes I'll do it through github (I do have an
account there, it's just my workflow and tools all geared towards
mailing list patch submission).

Thanks!
--
Dmitry
Loading...