Re: [PATCH v2 2/2] PM / devfreq: Add governor flags to clarify the features

Re: [PATCH v2 2/2] PM / devfreq: Add governor flags to clarify the features

[Dmitry Osipenko]

13.07.2020 11:31, Chanwoo Choi пишет:
> DEVFREQ supports the default governors like performance, powersave and also
> allows the devfreq driver to add their own governor like tegra30-devfreq.c
> according to their requirement. In result, some sysfs attributes are
> useful or not useful. Prior to that the user can access all sysfs attributes
> regardless of availability.
> 
> So, clarify the access permission of sysfs attributes according to governor.
> When adding the devfreq governor, can specify the available attribute
> information by using DEVFREQ_GOV_ATTR_* constant variable. The user can
> read or write the sysfs attributes in accordance to the specified attributes.
> 
> /* Devfreq governor flags for attributes and features */
> [Definition for sysfs attributes]
> - DEVFREQ_GOV_ATTR_GOVERNOR
> - DEVFREQ_GOV_ATTR_AVAIL_GOVERNORS
> - DEVFREQ_GOV_ATTR_AVAIL_FREQUENCIES
> - DEVFREQ_GOV_ATTR_CUR_FREQ
> - DEVFREQ_GOV_ATTR_TARGET_FREQ
> - DEVFREQ_GOV_ATTR_MIN_FREQ
> - DEVFREQ_GOV_ATTR_MAX_FREQ
> - DEVFREQ_GOV_ATTR_TRANS_STAT
> - DEVFREQ_GOV_ATTR_POLLING_INTERVAL
> - DEVFREQ_GOV_ATTR_TIMER
> 
> Also, the devfreq governor is able to have the specific flag as follows
> in order to implement the specific feature. For example, Devfreq allows
> user to change the governors on runtime via sysfs interface.
> But, if devfreq device uses 'passive' governor, don't allow user to change
> the governor. For this case, define the DEVFREQ_GOV_FLAT_IMMUTABLE
> and set it to flag of passive governor.
> 
> [Definition for governor flag]
> - DEVFREQ_GOV_FLAG_IMMUTABLE
> : If immutable flag is set, governor is never changeable to other governors.
> - DEVFREQ_GOV_FLAG_IRQ_DRIVEN
> : Devfreq core won't schedule polling work for this governor if value is set.
> 
> [Table of governor flag for devfreq governors]
> ------------------------------------------------------------------------------
>                       | simple    | perfor | power | user | passive | tegra30
> 		      | ondemand  | mance  | save  | space|         |
> ------------------------------------------------------------------------------
> governor              | O         | O      | O     | O    | O       | O
> available_governors   | O         | O      | O     | O    | O       | O
> available_frequencies | O         | O      | O     | O    | O       | O
> cur_freq              | O         | O      | O     | O    | O       | O
> target_freq           | O         | O      | O     | O    | O       | O
> min_freq              | O         | O      | O     | O    | O       | O
> max_freq              | O         | O      | O     | O    | O       | O
> trans_stat            | O         | O      | O     | O    | O       | O
>                       --------------------------------------------------------
> polling_interval      | O         | X      | X     | X    | X       | O
> timer                 | O         | X      | X     | X    | X       | X
> ------------------------------------------------------------------------------
> immutable             | X         | X      | X     | X    | O       | O
> interrupt_driven      | X(polling)| X      | X     | X    | X       | O (irq)
> ------------------------------------------------------------------------------
> 
> Signed-off-by: Chanwoo Choi <cw00.choi-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---

Hello, Chanwoo! I tested this series on NVIDIA Tegra30 and everything
working fine!

Reviewed-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Tested-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>


BTW, I'm curious what do you think about hiding the unsupported debugfs
attributes per-device instead of returning the -EACCES?

Re: [PATCH v2 2/2] PM / devfreq: Add governor flags to clarify the features

[Chanwoo Choi]

On 7/13/20 7:37 PM, Dmitry Osipenko wrote:
> 13.07.2020 11:31, Chanwoo Choi пишет:
>> DEVFREQ supports the default governors like performance, powersave and also
>> allows the devfreq driver to add their own governor like tegra30-devfreq.c
>> according to their requirement. In result, some sysfs attributes are
>> useful or not useful. Prior to that the user can access all sysfs attributes
>> regardless of availability.
>>
>> So, clarify the access permission of sysfs attributes according to governor.
>> When adding the devfreq governor, can specify the available attribute
>> information by using DEVFREQ_GOV_ATTR_* constant variable. The user can
>> read or write the sysfs attributes in accordance to the specified attributes.
>>
>> /* Devfreq governor flags for attributes and features */
>> [Definition for sysfs attributes]
>> - DEVFREQ_GOV_ATTR_GOVERNOR
>> - DEVFREQ_GOV_ATTR_AVAIL_GOVERNORS
>> - DEVFREQ_GOV_ATTR_AVAIL_FREQUENCIES
>> - DEVFREQ_GOV_ATTR_CUR_FREQ
>> - DEVFREQ_GOV_ATTR_TARGET_FREQ
>> - DEVFREQ_GOV_ATTR_MIN_FREQ
>> - DEVFREQ_GOV_ATTR_MAX_FREQ
>> - DEVFREQ_GOV_ATTR_TRANS_STAT
>> - DEVFREQ_GOV_ATTR_POLLING_INTERVAL
>> - DEVFREQ_GOV_ATTR_TIMER
>>
>> Also, the devfreq governor is able to have the specific flag as follows
>> in order to implement the specific feature. For example, Devfreq allows
>> user to change the governors on runtime via sysfs interface.
>> But, if devfreq device uses 'passive' governor, don't allow user to change
>> the governor. For this case, define the DEVFREQ_GOV_FLAT_IMMUTABLE
>> and set it to flag of passive governor.
>>
>> [Definition for governor flag]
>> - DEVFREQ_GOV_FLAG_IMMUTABLE
>> : If immutable flag is set, governor is never changeable to other governors.
>> - DEVFREQ_GOV_FLAG_IRQ_DRIVEN
>> : Devfreq core won't schedule polling work for this governor if value is set.
>>
>> [Table of governor flag for devfreq governors]
>> ------------------------------------------------------------------------------
>>                       | simple    | perfor | power | user | passive | tegra30
>> 		      | ondemand  | mance  | save  | space|         |
>> ------------------------------------------------------------------------------
>> governor              | O         | O      | O     | O    | O       | O
>> available_governors   | O         | O      | O     | O    | O       | O
>> available_frequencies | O         | O      | O     | O    | O       | O
>> cur_freq              | O         | O      | O     | O    | O       | O
>> target_freq           | O         | O      | O     | O    | O       | O
>> min_freq              | O         | O      | O     | O    | O       | O
>> max_freq              | O         | O      | O     | O    | O       | O
>> trans_stat            | O         | O      | O     | O    | O       | O
>>                       --------------------------------------------------------
>> polling_interval      | O         | X      | X     | X    | X       | O
>> timer                 | O         | X      | X     | X    | X       | X
>> ------------------------------------------------------------------------------
>> immutable             | X         | X      | X     | X    | O       | O
>> interrupt_driven      | X(polling)| X      | X     | X    | X       | O (irq)
>> ------------------------------------------------------------------------------
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> ---
> 
> Hello, Chanwoo! I tested this series on NVIDIA Tegra30 and everything
> working fine!
> 
> Reviewed-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Tested-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> 
> BTW, I'm curious what do you think about hiding the unsupported debugfs

Do you mean that sysfs?

> attributes per-device instead of returning the -EACCES?

I considered the hiding of sysfs node too instead of -EACCES.


But,
For a long time, devfreq showed the sysfs interface of all devfreq devices
regardless of the kind of devfreq governor. It means that devfreq keeps
the ABI interface. If devfreq hides the unsupported sysfs node
according to the type of governor, it will break the ABI.

Although I knew that maybe performance/powersave/userspace didn't use
the 'polling_interval' node, I just returned -EACCESS.

Thanks for suggesting your opinion. We can discuss it more.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 2/2] PM / devfreq: Add governor flags to clarify the features

[Dmitry Osipenko]

13.07.2020 15:26, Chanwoo Choi пишет:
...
>> BTW, I'm curious what do you think about hiding the unsupported debugfs
> 
> Do you mean that sysfs?

Yes, sysfs :)

>> attributes per-device instead of returning the -EACCES?
> 
> I considered the hiding of sysfs node too instead of -EACCES.

If there is no real userspace (used by a non-developer crowd) that
relies on the attributes presence, then it could be fine to change the
behaviour, IMO.

I know that PowerTOP utility uses the 'trans_stat' attribute, but not
sure about the other attributes.

> But,
> For a long time, devfreq showed the sysfs interface of all devfreq devices
> regardless of the kind of devfreq governor. It means that devfreq keeps
> the ABI interface. If devfreq hides the unsupported sysfs node
> according to the type of governor, it will break the ABI.

I didn't notice that it's an ABI already [1]. Should be better not to
change the ABI if there is userspace already relying on the old
behaviour, otherwise it may be okay to make changes until it will be too
late, also given that this is still a "testing" ABI.

[1] https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-class-devfreq

Although, the doc doesn't say anything about -EACCES/-EINVAL, so isn't
it an ABI change already? Doesn't doc need to be updated in order to
reflect the ABI change?

For example, doc says that userspace shouldn't care about attribute
values which are irrelevant for a selected governor, like in the case of
the 'polling_interval' attribute. The doc doesn't say that userspace may
get a error.

> Although I knew that maybe performance/powersave/userspace didn't use
> the 'polling_interval' node, I just returned -EACCESS.

The 'polling_interval', 'min/max_freq' and the new 'timer' attributes
are all the governor attributes.

Would be nice to have a per-device `governor/` directory containing all
the governor-specific attributes (without the unrelated attributes), but
perhaps it's a bit too late to change it now?