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
