Re: [PATCH v1 1/1] treewide: Align match_string() with sysfs_match_string()

From: Andi Shyti <andi.shyti@kernel.org>
Date: 2024-06-04 13:41:31
Also in: alsa-devel, cgroups, dri-devel, intel-gfx, intel-xe, keyrings, linux-acpi, linux-bcachefs, linux-clk, linux-crypto, linux-fbdev, linux-gpio, linux-hardening, linux-hwmon, linux-i2c, linux-ide, linux-integrity, linux-leds, linux-mediatek, linux-mm, linux-mmc, linux-omap, linux-pci, linux-phy, linux-pm, linux-rockchip, linux-security-module, linux-sound, linux-staging, linux-sunxi, linux-tegra, linux-usb, linux-wireless, linuxppc-dev, lkml, netdev, nouveau, platform-driver-x86

Hi Andy,

On Sun, Jun 02, 2024 at 06:57:12PM +0300, Andy Shevchenko wrote:

Make two APIs look similar. Hence convert match_string() to be
a 2-argument macro. In order to avoid unneeded churn, convert
all users as well. There is no functional change intended.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

nice patch, I checked some (maybe most) of your changes. There
are a few unrelated changes which I don't mind, but there are two
errors where the error value changes from ENODEV to EINVAL.

Find the comments through the line.

...

quoted hunk ↗ jump to hunk

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 1b7e82a0ad2e..b6f52f44625f 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c

@@ -1117,9 +1117,9 @@ static ssize_t store_energy_performance_preference(
 	if (ret != 1)
 		return -EINVAL;
 
-	ret = match_string(energy_perf_strings, -1, str_preference);
+	ret = __match_string(energy_perf_strings, -1, str_preference);
 	if (ret < 0)
-		return -EINVAL;
+		return ret;

a bit of unrelated changes here, but I guess no one will complain :-)

 
 	mutex_lock(&amd_pstate_limits_lock);
 	ret = amd_pstate_set_energy_pref_index(cpudata, ret);

...

quoted hunk ↗ jump to hunk

diff --git a/drivers/mmc/host/sdhci-xenon-phy.c b/drivers/mmc/host/sdhci-xenon-phy.c
index cc9d28b75eb9..1865e26ae736 100644
--- a/drivers/mmc/host/sdhci-xenon-phy.c
+++ b/drivers/mmc/host/sdhci-xenon-phy.c

@@ -135,15 +135,14 @@ struct xenon_emmc_phy_regs {
 	u32 logic_timing_val;
 };
 
-static const char * const phy_types[] = {
-	"emmc 5.0 phy",
-	"emmc 5.1 phy"
-};
-
 enum xenon_phy_type_enum {
 	EMMC_5_0_PHY,
 	EMMC_5_1_PHY,
-	NR_PHY_TYPES
+};
+
+static const char * const phy_types[] = {
+	[EMMC_5_0_PHY] = "emmc 5.0 phy",
+	[EMMC_5_1_PHY] = "emmc 5.1 phy",
 };

Another unrelated cleanup, but I don't complain

 enum soc_pad_ctrl_type {

...

-	tablet_found = match_string(tablet_chassis_types,
-				    ARRAY_SIZE(tablet_chassis_types),
-				    chassis_type) >= 0;
-	if (!tablet_found)
-		return -ENODEV;
+	ret = match_string(tablet_chassis_types, chassis_type);
+	if (ret < 0)
+		return ret;

This is a logical change though, because we are changing from
-ENODEV to -EINVAL. Even if it might look the right thing, but
still, it's a logical change.

quoted hunk ↗ jump to hunk

 
 	ret = hp_wmi_perform_query(HPWMI_SYSTEM_DEVICE_MODE, HPWMI_READ,
 				   system_device_mode, zero_if_sup(system_device_mode),

@@ -490,9 +487,7 @@ static bool is_omen_thermal_profile(void)
 	if (!board_name)
 		return false;
 
-	return match_string(omen_thermal_profile_boards,
-			    ARRAY_SIZE(omen_thermal_profile_boards),
-			    board_name) >= 0;
+	return match_string(omen_thermal_profile_boards, board_name) >= 0;
 }
 
 static int omen_get_thermal_policy_version(void)

...

quoted hunk ↗ jump to hunk

diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_rfim.c b/drivers/thermal/intel/int340x_thermal/processor_thermal_rfim.c
index e56db75a94fb..dbd176b0fb1f 100644
--- a/drivers/thermal/intel/int340x_thermal/processor_thermal_rfim.c
+++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_rfim.c

@@ -111,7 +111,7 @@ static ssize_t suffix##_show(struct device *dev,\
 		match_strs = (const char **)fivr_strings;\
 		mmio_regs = tgl_fivr_mmio_regs;\
 	} \
-	ret = match_string(match_strs, -1, attr->attr.name);\
+	ret = __match_string(match_strs, -1, attr->attr.name);\
 	if (ret < 0)\
 		return ret;\
 	reg_val = readl((void __iomem *) (proc_priv->mmio_base + mmio_regs[ret].offset));\

@@ -145,7 +145,7 @@ static ssize_t suffix##_store(struct device *dev,\
 		mmio_regs = tgl_fivr_mmio_regs;\
 	} \
 	\
-	ret = match_string(match_strs, -1, attr->attr.name);\
+	ret = __match_string(match_strs, -1, attr->attr.name);\
 	if (ret < 0)\
 		return ret;\
 	if (mmio_regs[ret].read_only)\

diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_wt_req.c b/drivers/thermal/intel/int340x_thermal/processor_thermal_wt_req.c
index f298e7442662..57f456befb34 100644
--- a/drivers/thermal/intel/int340x_thermal/processor_thermal_wt_req.c
+++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_wt_req.c

@@ -50,7 +50,7 @@ static ssize_t workload_type_store(struct device *dev,
 	if (ret != 1)
 		return -EINVAL;
 
-	ret = match_string(workload_types, -1, str_preference);
+	ret = __match_string(workload_types, -1, str_preference);

We could even thing of a "match_string_terminated" (or a better
name), but maybe it's too much?

 	if (ret < 0)
 		return ret;

...

-	c->auth_hash_algo = match_string(hash_algo_name, HASH_ALGO__LAST,
-					 c->auth_hash_name);
-	if ((int)c->auth_hash_algo < 0) {
+	err = __match_string(hash_algo_name, HASH_ALGO__LAST, c->auth_hash_name);
+	if (err < 0) {
 		ubifs_err(c, "Unknown hash algo %s specified",
 			  c->auth_hash_name);
-		return -EINVAL;
+		return err;

This is correct!

 	}
+	c->auth_hash_algo = err;
 
 	snprintf(hmac_name, CRYPTO_MAX_ALG_NAME, "hmac(%s)",
 		 c->auth_hash_name);

...

+int __match_string(const char * const *array, size_t n, const char *string);
+
+/**
+ * match_string - matches given string in an array
+ * @_a: array of strings
+ * @_s: string to match with
+ *
+ * Helper for __match_string(). Calculates the size of @a automatically.

/@a/@_a/

+ */
+#define match_string(_a, _s) __match_string(_a, ARRAY_SIZE(_a), _s)
+

...

quoted hunk ↗ jump to hunk

diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 6239777090c4..e3fc94b4c7e5 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c

@@ -1820,9 +1820,9 @@ static int param_set_audit(const char *val, const struct kernel_param *kp)
 	if (apparmor_initialized && !aa_current_policy_admin_capable(NULL))
 		return -EPERM;
 
-	i = match_string(audit_mode_names, AUDIT_MAX_INDEX, val);
+	i = __match_string(audit_mode_names, AUDIT_MAX_INDEX, val);

pity here... this could have been a match_string, but the
MAX_INDEX is hardcoded outside the enum.

 	if (i < 0)
-		return -EINVAL;
+		return i;
 
 	aa_g_audit = i;
 	return 0;

...

quoted hunk ↗ jump to hunk

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 16dad4a45443..7064f4cae549 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c

@@ -769,14 +769,13 @@ static int dapm_connect_mux(struct snd_soc_dapm_context *dapm,
 		item = 0;
 	}
 
-	i = match_string(e->texts, e->items, control_name);
+	i = __match_string(e->texts, e->items, control_name);
 	if (i < 0)
-		return -ENODEV;
+		return i;

Also this return value is wrong.

Andi

 
 	path->name = e->texts[i];
 	path->connect = (i == item);
 	return 0;
-
 }
 
 /* set up initial codec paths */
-- 
2.43.0.rc1.1336.g36b5255a03ac

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help