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

[Abhishek]

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?

Thanks,
--Abhishek