Hi
2020. november 28., szombat 15:08 keltezéssel, Hans de Goede írta:
> [...]
> > +static_assert(ARRAY_SIZE(profile_names) == platform_profile_perform+1);
>
> It would be better to add an extra member/entry at the end of the enum
> named platform_profile_no_profiles; and then use that instead of
> platform_profile_perform+1. Also see below where I use this too.
>
I'm not sure if it's just me, but when I read "no_profiles", then "number of probiles"
is not the first thing that comes to mind, maybe _end, _last, _max, etc.
would be harder to mistake for something else? What do you think?
> > +
> > +static ssize_t platform_profile_choices_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + int len = 0;
> > + int err, i;
> > +
> > + err = mutex_lock_interruptible(&profile_lock);
> > + if (err)
> > + return err;
> > +
> > + if (!cur_profile) {
> > + mutex_unlock(&profile_lock);
> > + return -ENODEV;
> > + }
> > +
> > + if (!cur_profile->choices) {
> > + mutex_unlock(&profile_lock);
> > + return sysfs_emit(buf, "\n");
> > + }
>
> If choices is empty, the for below will never print anything and
> the end result is still just emitting "\n", so this whole if
> block is unnecessary and can be removed.
>
> > +
> > + for (i = 0; i < ARRAY_SIZE(profile_names); i++) {
> > + if (cur_profile->choices & BIT(i)) {
>
> Please change the type of choices to an unsigned long array, like this:
>
> unsigned long choices[BITS_TO_LONGS(platform_profile_no_profiles)];
>
Sorry for the ignorant question, but does this play well with initialization?
Can something like this be done easily?
```
struct platform_profile_handler pph = {
.choices = BIT(...) | BIT(...) | etc.
};
```
Or do you need to use something like `__set_bit()` in a function?
> [...]
> > +int platform_profile_register(const struct platform_profile_handler *pprof)
> > +{
> > + int err;
>
> Maybe sanity check the platform_profile_handler a bit here,
> I think it would be ok to check that choices, profile_set and profile_get
> are all not 0 / NULL here and otherwise just return -EINVAL; Doing so
> allows making the code above a bit simpler, also removing some exit
> paths which require an unlock before exiting.
>
> > +
> > + err = mutex_lock_interruptible(&profile_lock);
> > + if (err)
> > + return err;
>
> Please use a regular mutex_lock here, this is called during
> driver probing, so no need to handle Ctrl+C and other signals.
> [...]
I believe insmod/modprobe can be interrupted,
but I agree that's not really of any concern here.
Regards,
Barnabás Pőcze
