hwdb: sensors: Fix some modalias matches no longer working with newer kernels #20599
Conversation
jwrdegoede
changed the title
|
Hmm, so the ":" are used as field separators, so that we clearly can identify where a new field begins, and match the field type. Now, your matches want to ignore some fields in the middle, that might or might not exist, but then you don't look for the Or in other words: you'll now match field values which end in field names. I think that is kinda ugly though, since the beginning of fields is where the interesing stuff is. or in other words: instead of ":" I think it would be better to use ":" to skip over the fields that might or might not exist. |
|
That said, maybe not looking for the field separators sucks either way. And hence maybe we should just duplicate the lines, and insist on the field separators both before and after the variable part i.e.: Duplicate this line: so that it becomes: |
poettering
added
the
reviewed/needs-rework 🔨
|
On 9/1/21 2:03 PM, Lennart Poettering wrote:
This solution only solves the new br and efr fields (which are next to each other), there also is the issue So if we want to fix that too, then your suggestion would require adding 3 match lines for what used Quite ugly. I can either copy the entire block 3 times and add the So I would prefer to keep this as is. @poettering, If you really do want me to respin this, then please let me know how you want to deal with the sku issue, do you want to use 3 lines for each entry, or do you want to ignore the sku issue, relying solely on the kernel-side fix to hit the stable series and use 2 lines for each entry ? |
|
The kernel fix to move the new sku field to the end of the modalias has been merged and also added to the 5.13.y and 5.14.y stable series. @poettering, ping, can you please let me know how you want to proceed with this? (see my previous comment) |
yuwata
added
needs-review
and removed
reviewed/needs-rework 🔨
|
Hmm, i am unsure. I wonder if one way out is to add a NEWS entry that asks people to backport the kernel fix, and then adjust our hwdb to just use the "right" order. I mean, it's a trivial fix, it should not be asking too much? |
The kernels with the new behavior have only been out for about a year. That means a lot of people will still be using the old kernels. OTOH that is long enough that quite a few people have the new ones too. Expecting people who for some reason want to try, or are getting (through there distro) a new systemd to patch there kernels seems a bit far fetched and not very userfriendly. I still believe that my original pull-req is the best solution here, I know that just inserting a |
|
@poettering ping? The changes to move the sku to the end have landend in both the 5.13.y and 5.14.y stable series. So that only leaves the (older) problem of the So with that I could implement your original suggestion to add 2 matches for each currently affected match inserting a I still think this is overkill, but I would like to see this fixed/merged one way or the other, so if you want me to respin this using the 2 matches for each old affected match approach let me know and I'll happily do that. |
|
so, i was silent because i couldn't make my mind up. hence, i guess let's do it your way. But please, add some comments to the hwdb entries, that explain why we have these checks so lose. and maybe one day we can remove/tighten them again once we stopped caring for the then really old kernels. |
|
hence, looks good to merge, but please add the suggested comments. |
poettering
added
reviewed/needs-rework 🔨
jwrdegoede
force-pushed
the
hwdb-sensors-modalias-fix
branch
from
1e4ec7e to
7706e03
Compare
|
I've just pushed out a new version adding the requested comments. |
… kernels Kernels >= 5.8 have added new fields to the dmi/id/modalias file in the middle of the modalias (instead of adding them at the end). Specifically new ":br<value>:" and (optional) ":efr<value>:" fields have been added between the ":bd<value>:" and ":svn<value>:" fields. Note the 5.13.0 and 5.14.0 kernels also added a new ":sku<value>:" field between the ":pvr<value>:" and ":rvn<value>:" fields, this has been fixed in later 5.13.y and 5.14.y releases, by moving the sku field to the end: https://lore.kernel.org/lkml/20210831130508.14511-1-hdegoede@redhat.com/ Unfortunately the same cannot be done for the new br and efr fields since those have been added more then a year ago and hwdb even already has some newer entries relying on the new br field being there (and thus not working with older kernels). Fix the issue with the br and efr fields through the following changes: 1. Replace any matches on ":br<value>" from newer entries with an '*' 2. Replace "bd<value>:svn<value>" matches with: "bd<value>:*svn<value>" inserting an '*' where newer kernels will have the new br + efr fields This makes these matches working with old as well as new kernels. Link: systemd#20550 Link: systemd#20562
jwrdegoede
force-pushed
the
hwdb-sensors-modalias-fix
branch
from
7706e03 to
3211bc2
Compare
|
I've just pushed a new version replacing the "newer" in the comment with ">= 5.8" to make this explicit. |
poettering
added
good-to-merge/waiting-for-ci 👍
|
lgtm |
Recent kernels have added new fields to the dmi/id/modalias file in the middle of the modalias (instead of adding them at the end).
Specifically new
:br<value>:and (optional):efr<value>:fields have been added between the:bd<value>:and:svn<value>:fields and a new:sku<value>:field has been added between the:pvr<value>:and:rvn<value>:fields.A kernel-patch moving the sku field to the end of the modalias has been posted upstream:
https://lore.kernel.org/lkml/20210831130508.14511-1-hdegoede@redhat.com/
Unfortunately the same cannot be done for the new br and efr fields since those have been added more then a year ago and hwdb even already has some newer entries relying on the new br field being there (and thus not working with older kernels).
To fix all this, this commit makes the following changes:
:br<value>from newer entries with an*bd<value>:svn<value>matches with:bd<value>:*svn<value>inserting an*where newer kernels will have the new br + efr fieldspvr<value>:rvn<value>matches with:pvr<value>:*rvn<value>inserting an*where newer kernels will have the new sku fieldThis makes these matches working with old as well as new kernels, including with kernels which have the fix to move the sku field to the end.
Link: #20550
Link: #20562