Re: [PATCH v2 03/11] xen/x86: introduce "cpufreq=amd-cppc" xen cmdline

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2025-02-11 07:08, Jan Beulich wrote:
> On 06.02.2025 09:32, Penny Zheng wrote:
> > --- /dev/null
> > +++ b/xen/arch/x86/acpi/cpufreq/amd-cppc.c
> > @@ -0,0 +1,64 @@
> > +static bool __init amd_cppc_handle_option(const char *s, const char *end)
> > +{
> > +    int ret;
> > +
> > +    ret = parse_boolean("verbose", s, end);
> > +    if ( ret >= 0 )
> > +    {
> > +        cpufreq_verbose = ret;
> > +        return true;
> > +    }
> > +
> > +    return false;
> > +}
> > +
> > +int __init amd_cppc_cmdline_parse(const char *s, const char *e)
> > +{
> > +    do
> > +    {
> > +        const char *end = strpbrk(s, ",;");
> > +
> > +        if ( !amd_cppc_handle_option(s, end) )
> You have an incoming "e" here. What if the comma / semicolon you find
> is past where that points? (I understand you've copied that from
> hwp_cmdline_parse(), and should have raised the question already there
> when reviewing the respective patch. It also looks as if behavior-
> wise all would be okay here. It's just that this very much looks like
> a buffer overrun on the first and second glance.)
It's been a while since I worked on HWP.  I think my assumption was that 
you are inside a nul terminated string, and command line option parsing 
functions can handle end == NULL, so it would all work out.  A too long 
string crossing " " or ";" would not match.
> > +        {
> > +            printk(XENLOG_WARNING
> > +                   "cpufreq/amd-cppc: option '%.*s' not recognized\n",
> > +                   (int)((end ?: e) - s), s);
> > +
> > +            return -EINVAL;
> > +        }
> > +
> > +        s = end ? ++end : end;
> The increment is odd here (again inherited from the HWP function), as
> "end" is about to go out of scope.
For HWP, I copied from setup_cpufreq_option() which does similar.

You'd prefer:
s = end ? end + 1 : NULL;

That is more explicit which is good.  I considered using NULL back when 
working on that.  I think I when with the current form to match 
setup_cpufreq_option().
Regards,
Jason