Hi
2020. november 16., hétfő 0:04 keltezéssel, Mark Pearson írta:
> [...]
> >> +static ssize_t platform_profile_store(struct device *dev,
> >> + struct device_attribute *attr,
> >> + const char *buf, size_t count)
> >> +{
> >> + int err, i;
> >> +
> >> + mutex_lock(&profile_lock);
> >> + if (!cur_profile) {
> >> + mutex_unlock(&profile_lock);
> >> + return -EOPNOTSUPP;
> >
> > I'm not sure why you modified the errno. ENODEV seemed to me like a perfectly
> > fine error to return if `cur_profile` is not set. `platform_profile_choices_show()`
> > returns ENODEV, so there is a bit of inconsistency.
> > (same applies to `platform_profile_show()`)
> My thinking was it seemed a better message. I can't really see any
> conditions when you'd get here (a driver would have registered a driver
> and then not provided a profile?) but it seemed that if that was
> possible it was more because changing the settings wasn't supported.
>
> I managed to convince myself it made more sense - but have no issue with
> changing it back.
> >
> >
> >> + }
> >> +
> >> + /* Scan for a matching profile */
> >> + i = sysfs_match_string(profile_names, buf);
> >> + if (i < 0) {
> >> + mutex_unlock(&profile_lock);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + if (!cur_profile->profile_set) {
> >> + mutex_unlock(&profile_lock);
> >> + return -EOPNOTSUPP;
> >> + }
> >> +
One thing I have just noticed is that I believe the selected profile should be
checked against `cur_profile->choices`, don't you think? I have assumed for
some reason that this check is done, and `profile_set` is only called with values
that the handler supports (they are in `choices`) up until now, but I see
that that's not what's happening.
> >> + err = cur_profile->profile_set(i);
> >> + mutex_unlock(&profile_lock);
> >> + if (err)
> >> + return err;
> >> +
> >> + return count;
> >> +}
> [...]
> >> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
> >> new file mode 100644
> >> index 000000000000..f6592434c8ce
> >> --- /dev/null
> >> +++ b/include/linux/platform_profile.h
> >> @@ -0,0 +1,36 @@
> [...]
> >
> > By the way, I still feel like the enum values are "too vague" and should be
> > prefixed with `platform_`. If you're not opposed to that change.
> Sure - I can change that. For me it made the names long but I don't mind
> changing them.
Short and succinct names a good, but I hope you can see why I'm saying what I'm
saying. I believe these names should be "properly namespaced" since they are in
a "kernel-global" header file.
> >
> >
> >> +};
> >> +
> >> [...]
Regards,
Barnabás Pőcze
