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

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

 



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





[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux