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".
+ [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?
> + /* 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
> + 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.
> + 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
