Re: [External] Re: [PATCH v4 2/3] ACPI: platform-profile: Add platform profile support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thanks Barnabas,

On 2020-11-27 2:14 p.m., Barnabás Pőcze wrote:
Hi


2020. november 26., csütörtök 17:51 keltezéssel, Mark Pearson írta:

[...]
+static const char * const profile_names[] = {
+	[platform_profile_low] = "low-power",
+	[platform_profile_cool] = "cool",
+	[platform_profile_quiet] = "quiet",
+	[platform_profile_balance] = "balance",

Documentation says "balanced".
Ah, my bad. I will correct.


+	[platform_profile_perform] = "performance",
+};
[...]
+static ssize_t platform_profile_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	enum platform_profile_option profile = platform_profile_balance;
+	int err;
+
+	err = mutex_lock_interruptible(&profile_lock);
+	if (err)
+		return err;
+
+	if (!cur_profile) {
+		mutex_unlock(&profile_lock);
+		return -ENODEV;
+	}
+
+	if (!cur_profile->profile_get) {
+		mutex_unlock(&profile_lock);
+		return -EOPNOTSUPP;
+	}
+
+	err = cur_profile->profile_get(&profile);
+	mutex_unlock(&profile_lock);
+	if (err < 0)
+		return err;
+

In `platform_profile_store()`, you do
```
err = cur_profile->profile_set(i);
if (err)
   return err;
```
but here you do `if (err < 0)`, why?
At one point I had this function returning the profile, but then it ended up getting messy for handling errors. I changed it but didn't change the error handling - I'll tidy this up. Thanks!


+	/* Check that profile is valid index */
+	if ((profile < 0) || (profile >= ARRAY_SIZE(profile_names)))
+		return sysfs_emit(buf, "\n");
+

I'd write `if (WARN_ON(profile < 0 ....))` since that is serious error in my
opinion which should be logged. I am also not sure if
Will do. I've not had any experience with the WARN_ON macro before - thanks for the suggestion.


+	return sysfs_emit(buf, "%s\n", profile_names[profile]);
+}
[...]
+int platform_profile_unregister(void)
+{
+	int err;
+
+	err = mutex_lock_interruptible(&profile_lock);
+	if (err)
+		return err;
+

I know it was me who said to prefer `mutex_lock_interruptible()`, but in this
particular instance I believe `mutex_lock()` would be preferable to avoid the case
where the module unloading is interrupted, and thus the profile handler is not
unregistered properly. This could be handled in each module that uses this
interface, however, I believe it'd be better to handle it here.
Agreed (and I know Hans makes the same point in his email). Your suggestion makes sense, I'll switch back to mutex_lock for the register and unregister functions.


+	if (!cur_profile) {
+		mutex_unlock(&profile_lock);
+		return -ENODEV;
+	}
+
+	sysfs_remove_group(acpi_kobj, &platform_profile_group);
+	cur_profile = NULL;
+	mutex_unlock(&profile_lock);
+	return 0;
+}
[...]


Regards,
Barnabás Pőcze

Thank you for the review!
Mark



[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux