Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH regression fix] firmware/dmi: Move product_sku info to the
 end of the modalias
To:     Jean Delvare <jdelvare@suse.com>
Cc:     linux-kernel@vger.kernel.org, stable@vger.kernel.org,
        Kai-Chuan Hsieh <kaichuan.hsieh@canonical.com>,
        Erwan Velu <e.velu@criteo.com>
References: <20210831130508.14511-1-hdegoede@redhat.com>
From:   Hans de Goede <hdegoede@redhat.com>
Message-ID: <0a96f1de-c0a0-40ce-e4dc-0acacfa2c7a8@redhat.com>
Date:   Tue, 31 Aug 2021 16:20:03 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <20210831130508.14511-1-hdegoede@redhat.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

On 8/31/21 3:05 PM, Hans de Goede wrote:
> Commit e26f023e01ef ("firmware/dmi: Include product_sku info to modalias")
> added a new field to the modalias in the middle of the modalias, breaking
> some existing udev/hwdb matches on the whole modalias without a wildcard
> ('*') in between the pvr and rvn fields.
> 
> All modalias matches in e.g. :
> https://github.com/systemd/systemd/blob/main/hwdb.d/60-sensor.hwdb
> deliberately end in ':*' so that new fields can be added at *the end* of
> the modalias, but adding a new field in the middle like this breaks things.
> 
> Move the new sku field to the end of the modalias to fix some hwdb
> entries no longer matching.
> 
> The new sku field has already been put to use in 2 new hwdb entries:
> 
>  sensor:modalias:platform:HID-SENSOR-200073:dmi:*svnDell*:sku0A3E:*
>   ACCEL_LOCATION=base
> 
>  sensor:modalias:platform:HID-SENSOR-200073:dmi:*svnDell*:sku0B0B:*
>   ACCEL_LOCATION=base
> 
> The wildcard use before and after the sku in these matches means that they
> should keep working with the sku moved to the end.
> 
> Note that there is a second instance of in essence the same problem,
> commit f5152f4ded3c ("firmware/dmi: Report DMI Bios & EC firmware release")
> 
> Added 2 new br and efr fields in the middle of the modalias. This too
> breaks some hwdb modalias matches, but this has gone unnoticed for over
> a year. So some newer hwdb modalias matches actually depend on these
> fields being in the middle of the string. Moving these to the end now
> would break 3 hwdb entries, while fixing 8 entries.
> 
> Since there is no good answer for the new br and efr fields I have chosen
> to leave these as is. Instead I'll submit a hwdb update to put a wildcard
> at the place where these fields may or may not be present depending on the
> kernel version.

In case anyone is interested here is the systemd pull-req fixing this from
the hwdb side: https://github.com/systemd/systemd/pull/20599

Regards,

Hans


> BugLink: https://github.com/systemd/systemd/issues/20550
> Link: https://github.com/systemd/systemd/pull/20562
> Fixes: e26f023e01ef ("firmware/dmi: Include product_sku info to modalias")
> Cc: stable@vger.kernel.org
> Cc: Kai-Chuan Hsieh <kaichuan.hsieh@canonical.com>
> Cc: Erwan Velu <e.velu@criteo.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/firmware/dmi-id.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/dmi-id.c b/drivers/firmware/dmi-id.c
> index 4d5421d14a41..940ddf916202 100644
> --- a/drivers/firmware/dmi-id.c
> +++ b/drivers/firmware/dmi-id.c
> @@ -73,6 +73,10 @@ static void ascii_filter(char *d, const char *s)
>  
>  static ssize_t get_modalias(char *buffer, size_t buffer_size)
>  {
> +	/*
> +	 * Note new fields need to be added at the end to keep compatibility
> +	 * with udev's hwdb which does matches on "`cat dmi/id/modalias`*".
> +	 */
>  	static const struct mafield {
>  		const char *prefix;
>  		int field;
> @@ -85,13 +89,13 @@ static ssize_t get_modalias(char *buffer, size_t buffer_size)
>  		{ "svn", DMI_SYS_VENDOR },
>  		{ "pn",  DMI_PRODUCT_NAME },
>  		{ "pvr", DMI_PRODUCT_VERSION },
> -		{ "sku", DMI_PRODUCT_SKU },
>  		{ "rvn", DMI_BOARD_VENDOR },
>  		{ "rn",  DMI_BOARD_NAME },
>  		{ "rvr", DMI_BOARD_VERSION },
>  		{ "cvn", DMI_CHASSIS_VENDOR },
>  		{ "ct",  DMI_CHASSIS_TYPE },
>  		{ "cvr", DMI_CHASSIS_VERSION },
> +		{ "sku", DMI_PRODUCT_SKU },
>  		{ NULL,  DMI_NONE }
>  	};
>  
>