Re: [PATCH v2] cpupower : Handle set and info subcommands correctly

[shuah]

On 10/14/19 3:38 AM, Abhishek wrote:
> Hi Shuah,
>
>
> On 10/04/2019 03:45 AM, shuah wrote:
> > Hi Abhishek,
> >
> > On 10/3/19 8:38 AM, shuah wrote:
> > > On 9/13/19 2:07 AM, Abhishek Goel wrote:
> > > > Cpupower tool has set and info options which are being used only by
> > > > x86 machines. This patch removes support for these two subcommands
> > > > from generic cpupower utility. Thus, these two subcommands will now be
> > > > available only for intel.
> > > > This removes the ambiguous error message while using set option in case
> > > > of using non-intel systems.
> > > >
> > > > Without this patch on a non-intel box:
> > > >
> > > > root@ubuntu:~# cpupower info
> > > > System does not support Intel's performance bias setting
> > > >
> > > > root@ubuntu:~# cpupower set -b 10
> > > > Error setting perf-bias value on CPU
> > > >
> > > > With this patch on a non-intel box:
> > > >
> > > > root@ubuntu:~# cpupower info
> > > > Supported commands are:
> > > > ÂÂÂÂÂÂÂÂ frequency-info
> > > > ÂÂÂÂÂÂÂÂ frequency-set
> > > > ÂÂÂÂÂÂÂÂ idle-info
> > > > ÂÂÂÂÂÂÂÂ idle-set
> > > > ÂÂÂÂÂÂÂÂ monitor
> > > > ÂÂÂÂÂÂÂÂ help
> > > >
> > > > Same result for set subcommand.
> > > >
> > > > This patch does not affect results on a intel box.
> > > >
> > > > Signed-off-by: Abhishek Goel <huntbag@xxxxxxxxxxxxxxxxxx>
> > > > Acked-by: Thomas Renninger <trenn@xxxxxxx>
> > > > ---
> > > >
> > > > changes from v1:
> > > > ÂÂÂÂInstead of bailing out early in set and info commands, in V2, we
> > > > ÂÂÂÂare cutting out support for these two commands for non-intel
> > > > ÂÂÂÂsystems.
> > >
> > > thanks. I will get this in for 5.4-rc3 veru likely. Definitely in 5.4
> >
> > Okay I almost applied this and decided it needs improvements.
> >
> > I don't like using #if defined(__x86_64__) || defined(__i386__)
> >
> > tools/power/cpupower/utils/cpupower.c main() already does this
> > dynamically using uname(). Please use the same logic do this,
> > instead of adding compile time code.
> >
> > thanks,
> > -- Shuah
>
> Do you want the decision to be taken in cpupower-set and cpupower-info
> file as was done in v1 but using uname() by identifying the architecture
> there itself?

As I recall my objection to the v1 was that it is making the call very
early and bails out. Ideally, I would like to see the change as part
of set/info handling and then print out appropriate message.

thanks,
-- Shuah