Re: [Xen-devel] [PATCH v3] ns16550-Add-command-line-parsing-adjustments

["Paratey, Swapnil" <sparatey@xxxxxxx>]

Hi Jan,

Thank you very much for your review comments.
I need a few clarifications to move forward.


On 4/3/2017 6:55 AM, Jan Beulich wrote:
> > > > On 31.03.17 at 17:42,<swapnil.paratey@xxxxxxx>  wrote:
> The title needs improvement - it doesn't really reflect what the
> patch does.
I apologize for this. I kept the name same since it was the third version
of the patch and thought that it would help in tracking the evolution of
the patch.

The new proposed title:
- ns16550: Support for UART parameters to be specified as name-value pairs

Please let me know about
- how to track the evolution of the patch (whether to mention [PATCH v4])
- whether this patch should mention ChangeSets from the previous versions
- if the subject should be more precise and whether I should respect Git's
60-character recommended limit for patch subjects.

> > Add name=value parsing options for com1 and com2 to add flexibility
> > in setting register values for MMIO UART devices.
> >
> > Maintain backward compatibility with previous positional parameter
> > specfications.
> >
> > eg. com1=115200,8n1,0x3f8,4
> > eg. com1=baud=115200,parity=n,reg_width=4,reg_shift=2,irq=4
> > eg. com1=115200,8n1,0x3f8,4,reg_width=4,reg_shift=2
> I would have been nice if you split the new format handling from
> the addition of the new sub-options.
>
> > --- a/docs/misc/xen-command-line.markdown
> > +++ b/docs/misc/xen-command-line.markdown
> > @@ -324,6 +324,43 @@ Both option `com1` and `com2` follow the same format.
> >   
> >   A typical setup for most situations might be `com1=115200,8n1`
> >   
> > +In addition to the above positional specification for UART parameters,
> > +name=value pair specfications are also supported. This is used to add
> > +flexibility for UART devices which require additional UART parameter
> > +configurations.
> > +
> > +The comma separation still delineates positional parameters. Hence,
> > +unless the parameter is explicitly specified with name=value option, it
> > +will be considered a positional parameter.
> > +
> > +The syntax consists of
> > +com1=(comma-separated positional parameters),(comma separated name-value pairs)
> > +
> > +The accepted name keywords for name=value pairs are
> > + * `baud` - accepts integer baud rate (eg. 115200) or `auto`
> > + * `bridge`- accepts xx:xx:xx. Similar to bridge-bdf in positional parameters.
> > +             notation is <bus>:<device>:<function>
> > + * `clock_hz`- accepts large integers to setup UART clock frequencies.
> > +               Do note - these values are multiplied by 16.
> > + * `data_bits` - integer between 5 and 8
> > + * `dev` - accepted values are `pci` OR `amt`. If this option
> > +           is used to specify if the serial device is pci-based. The io_base
> > +           cannot be specified when `dev=pci` or `dev=amt` is used.
> > + * `io_base` - accepts integer which specified IO base port for UART registers
> > + * `irq` - IRQ number to use
> > + * `parity` - accepted values are same as positional parameters
> > + * `port` - used to specify which port the PCI serial device is located on
> > +            notation is xx:xx:xx <bus>:<device>:<function>
> Everywhere above PCI device specifications wrongly use : instead
> of . as separator between device and function.
>
> > + * `reg_shift` - register shifts required to set UART registers
> > + * `reg_width` - register width required to set UART registers
> > +                 (only accepts 1 and 4)
> > + * `stop_bits` - only accepts 1 or 2 for the number of stop bits
> Since these are all new anyway, can we please use - instead of _
> as separator characters inside sub-option names? Dashed are
> slightly easier to type than underscores on most keyboards.
>
> > --- a/xen/common/kernel.c
> > +++ b/xen/common/kernel.c
> > @@ -48,7 +48,7 @@ static void __init assign_integer_param(
> >   
> >   void __init cmdline_parse(const char *cmdline)
> >   {
> > -    char opt[100], *optval, *optkey, *q;
> > +    char opt[512], *optval, *optkey, *q;
> Why not MAX_CMDLINE_LENGTH? But anyway both this and ...
>
> > --- a/xen/drivers/char/ns16550.c
> > +++ b/xen/drivers/char/ns16550.c
> > @@ -38,11 +38,27 @@
> >    * can be specified in place of a numeric baud rate. Polled mode is specified
> >    * by requesting irq 0.
> >    */
> > -static char __initdata opt_com1[30] = "";
> > -static char __initdata opt_com2[30] = "";
> > +static char __initdata opt_com1[MAX_CMDLINE_LENGTH] = "";
> > +static char __initdata opt_com2[MAX_CMDLINE_LENGTH] = "";
> ... this seems to be excessive growth.
Is 128 bytes a good number? It easily accommodates all the UART command line
parameters.

> > +typedef enum e_serial_param_type {
> > +    BAUD=0,
> Stray "=0". Also I don't think enumerator identifiers should be all
> capitals.
>
> > +    BRIDGEBDF,
> > +    CLOCKHZ,
> > +    DATABITS,
> > +    DEVICE,
> > +    IO_BASE,
> > +    IRQ,
> > +    PARITY,
> > +    PORTBDF,
> > +    REG_SHIFT,
> > +    REG_WIDTH,
> > +    STOPBITS,
> > +    __MAX_SERIAL_PARAM /* introduce more parameters before this line */
> Stray double underscores.
>
> > @@ -77,6 +93,29 @@ static struct ns16550 {
> >   #endif
> >   } ns16550_com[2] = { { 0 } };
> >   
> > +struct serial_param_var
> > +{
> > +    char *sp_name;
> const
>
> > +    serial_param_type sp_type;
> > +};
> > +
> > +/* enum struct keeping a table of all accepted parameter names
> > + * for parsing cmdline for serial port com1 and com2 */
> > +static struct serial_param_var sp_vars[] = {
> const ... __initconst plus you should aim at arranging for the
> string literals below to also get placed in .init.rodata (instead of
> .rodata).
>
> > @@ -1083,26 +1122,70 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, 
> > unsigned int idx)
> >   }
> >   #endif
> >   
> > +/* used to parse name value pairs and return which value it is
> > + * along with pointer for the extracted value - ext_value */
> > +static serial_param_type get_token_value(char *token, char *ext_value)
> __init
>
> And the name is misleading - you obtain a token type and value.
> Perhaps match_token() or get_token()?
>
> > +{
> > +    char *param_name;
> const
>
> > +    int i;
> unsigned int
>
> > +
> > +    param_name = strsep(&token, "=");
> > +    if ( param_name == NULL )
> > +        return __MAX_SERIAL_PARAM;
> > +    /* token has the param value after the equal to sign */
> > +    strlcpy(ext_value, token, MAX_PARAM_VALUE_LENGTH);
> I think you'd better copy only once you found a match in the
> table. And the size restriction would better be reflected in the
> respective function parameter type (using ARRAY_SIZE() here).
>
> > +    /* linear search for the parameter */
> > +    for ( i = 0; i < ARRAY_SIZE(sp_vars); i++ )
> > +    {
> > +        if ( strcmp(sp_vars[i].sp_name, param_name) == 0 )
> > +            return sp_vars[i].sp_type;
> > +    }
> > +
> > +    return __MAX_SERIAL_PARAM;
> > +}
> > +
> >   #define PARSE_ERR(_f, _a...)                 \
> >       do {                                     \
> >           printk( "ERROR: " _f "\n" , ## _a ); \
> >           return;                              \
> >       } while ( 0 )
> >   
> > -static void __init ns16550_parse_port_config(
> > -    struct ns16550 *uart, const char *conf)
> > +#define PARSE_ERR_RET(_f, _a...)             \
> > +    do {                                     \
> > +        printk( "ERROR: " _f "\n" , ## _a ); \
> > +        return 1;                            \
> > +    } while ( 0 )
> > +
> > +
> > +int parse_positional(struct ns16550 *uart, char **str)
> static ... __init
>
> Why is the return type of this function not void? All return
> statements uniformly return zero.
There are two places where PARSE_ERR_RET has been used and it is
returning 1 instead of 0 (to exit the ns16550_parse_port_config
function if the parsing wasn't correct). This is used to maintain
previous behavior - if the parsing for pci is not correct, it
should exit the function without going through config_parsed.

> >   {
> >       int baud;
> > +    const char *conf;
> > +    char *name_val_pos;
> >   
> > -    /* No user-specified configuration? */
> > -    if ( (conf == NULL) || (*conf == '\0') )
> > +    conf = (const char *)*str;
> Pointless cast.
>
> > +    name_val_pos = strchr(*str, '=');
> Why don't you use conf here?
>
> > +
> > +    /* finding the end of the positional parameters */
> > +    if (name_val_pos)
> >       {
> > -        /* Some platforms may automatically probe the UART configuartion. */
> > -        if ( uart->baud != 0 )
> > -            goto config_parsed;
> > -        return;
> > +        while (name_val_pos > *str)
> > +        {
> > +            name_val_pos--; /* working backwards from the = sign */
> > +            if (*name_val_pos == ',')
> > +            {
> > +                *name_val_pos = '\0';
> > +                name_val_pos++;
> > +                break;
> > +            }
> > +        }
> >       }
> >   
> > +    *str = name_val_pos;
> > +    if (conf == *str) return 0; /* when there are no positional parameters */
> Coding style for all the control statements above (more further down).
> Also the return statement here goes on its own line.
>
> > @@ -1165,11 +1248,122 @@ static void __init ns16550_parse_port_config(
> >       {
> >           if ( !parse_pci(conf, NULL, &uart->pb_bdf[0],
> >                           &uart->pb_bdf[1], &uart->pb_bdf[2]) )
> > -            PARSE_ERR("Bad bridge PCI coordinates");
> > +            PARSE_ERR_RET("Bad bridge PCI coordinates");
> >           uart->pb_bdf_enable = 1;
> >       }
> >   #endif
> >   
> > +    return 0;
> > +}
> > +
> > +int parse_namevalue_pairs(char *str, struct ns16550 *uart)
> static ... __init
>
> Looking at the return values, this perhaps would better return bool.
This is based on the understanding that (return 0) implies a successful
return from a function whereas a non-zero value means an error.

If we use true/false bools, should I end the function with (return false)
for a successful return OR should I use (return true) with
if ( !parse_namevalue_pairs) to check if the function returned SUCCESS?

The aim of using int was to ensure readability and understanding
successful returns from a function.

Please let me know which bool implementation to use.

> > +{
> > +    serial_param_type parsed_param;
> > +    char *token, *start;
> > +    char param_value[MAX_PARAM_VALUE_LENGTH];
> > +    bool dev_set;
> > +
> > +    dev_set = 0;
> false (and true below)
>
> > +    start = str;
> Please make both of these the initializers of the respective variables.
>
>
> > +    while (start != NULL) /* strsep gives NULL when there are no tokens found 
> > */
> You didn't call strsep() yet when you first come here, so perhaps this
> would better be do ... while ()?
>
> > +    {
> > +        /* when no tokens are found, start will be NULL */
> > +        token = strsep(&start, ",");
> > +
> > +        parsed_param = get_token_value(token, param_value);
> > +        switch(parsed_param)
> I don't see the need for the intermediate variable here.
>
> > +        {
> > +            case BAUD:
> case labels indent the same as the opening brace.
>
> > +                uart->baud = simple_strtoul(param_value, NULL, 0);
> > +                break;
> > +            case BRIDGEBDF:
> > +                if ( !parse_pci(param_value, NULL, &uart->ps_bdf[0],
> > +                        &uart->ps_bdf[1], &uart->ps_bdf[2]))
> Indentation.
>
> > +                    PARSE_ERR_RET("Bad port PCI coordinates\n");
> > +                uart->ps_bdf_enable = 1;
> true
>
> > +                break;
> > +            case CLOCKHZ:
> > +                uart->clock_hz = (simple_strtoul(param_value, NULL, 0) << 4);
> > +                break;
> > +            case DEVICE:
> > +                if ((strncmp(param_value, "pci", 3) == 0))
> Stray pair of parentheses.
>
> > +                {
> > +                    pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com);
> > +                    dev_set = 1;
> > +                }
> > +                else if (strncmp(param_value, "amt", 3) == 0)
> > +                {
> > +                    pci_uart_config(uart, 0, uart - ns16550_com);
> > +                    dev_set = 1;
> > +                }
> > +                break;
> > +            case IO_BASE:
> > +                if (dev_set == 1)
> > +                    PARSE_ERR_RET("Can't use io_base with dev=pci or dev=amt 
> > options\n");
> Doesn't this apply the other way around then too?
In current code behavior, mentioning pci in the command line sets the io_base
automatically. We cannot specify io_base and pci simultaneously in the
same line with the current code. Using name-value pairs gives the user a
chance to specify both of these options together.

For example, a user can write "dev=pci,io_base=0xfedca000" OR
"io_base=0xfedca000,dev=pci"

In the case where a user has mentioned pci and then set the io_base too,
the new user-specified io_base option will be written over the pci-probed
io_base which will render the PCI UART device without an io_base and
hence unusable - since we have the wrong io_base for the PCI device.
This is why I have included the message for this condition.

However, if a user wants to write the io_base and then specify pci,
the pci_uart_config function will happily overwrite the io_base without
the user knowing that the io_base has been written over by the pci-probing
code (pci_uart_config) and the PCI UART device will work (provided
pci_uart_config succeeds in setting up the PCI UART device).

Hence, only the first case has been addressed here - to prevent the user from
over-writing over their own pci settings. The other way around doesn't
stop the user from receiving console messages.

Please let me know if this behavior is acceptable or something has to change.

> > +                uart->io_base = simple_strtoul(param_value, NULL, 0);
> > +                break;
> > +            case IRQ:
> > +                uart->irq = simple_strtoul(param_value, NULL, 0);
> > +                break;
> > +            case DATABITS:
> > +                uart->data_bits = simple_strtoul(param_value, NULL, 0);
> > +                break;
> > +            case PARITY:
> > +                uart->parity = parse_parity_char(*param_value);
> > +                break;
> > +            case PORTBDF:
> > +                if ( !parse_pci(param_value, NULL, &uart->pb_bdf[0],
> > +                        &uart->pb_bdf[1], &uart->pb_bdf[2]))
> > +                    PARSE_ERR_RET("Bad port PCI coordinates\n");
> > +                uart->pb_bdf_enable = 1;
> > +                break;
> > +            case STOPBITS:
> > +                uart->stop_bits = simple_strtoul(param_value, NULL, 0);
> > +                break;
> > +            case REG_WIDTH:
> > +                uart->reg_width = simple_strtoul(param_value, NULL, 0);
> > +                break;
> > +            case REG_SHIFT:
> > +                uart->reg_shift = simple_strtoul(param_value, NULL, 0);
> > +                break;
> > +            default:
> > +                printk("Invalid parameter: %s\n", token);
> PARSE_ERR_RET()?
>
> > +static void __init ns16550_parse_port_config(
> > +    struct ns16550 *uart, const char *conf)
> > +{
> > +    char cmdline[MAX_CMDLINE_LENGTH];
> This isn't the entire cmdline, is it?
This is only the UART command line options. For example,
when we specify
com1=115200,8n1,0x3f8,4
cmdline is only "115200,8n1,0x3f8,4" without the "com1="

However, the opt buffer (which I have also set to 512 - switching to 128 now),
includes the 5 characters of "com1=". This buffer is in the cmdline_parse
function in xen/common/kernel.c

Should this "com1=" case be handled? How should the buffer lengths be set here?

Previous code had 30 character buffer for both the buffers (despite one holding
5 more characters than the other).

> > +    char *str;
> > +
> > +    /* No user-specified configuration? */
> > +    if ( (conf == NULL) || (*conf == '\0') )
> > +    {
> > +        /* Some platforms may automatically probe the UART configuartion. */
> > +        if ( uart->baud != 0 )
> > +            goto config_parsed;
> > +        return;
> > +    }
> > +
> > +    strlcpy(cmdline, conf, MAX_CMDLINE_LENGTH);
> > +    str = cmdline; /* good idea to use another pointer and keep cmdline alone 
> > */
> Because of? Also comment style (not just here).
>
> > +    /* parse positional parameters and get pointer for name-value pairs */
> > +    if ( parse_positional(uart, &str) )
> > +        return;
> > +
> > +    if ( (str == NULL) || (*str == '\0') )
> > +        goto config_parsed;
> > +
> > +    if ( parse_namevalue_pairs(str, uart) )
> > +        return;
> > +
> >    config_parsed:
> Please avoid goto outside of error cleanup path where they're not
> really making code structure a lot better. The two if()s above can
> be easily re-arranged to do so, and I think the other goto a few lines
> up also wouldn't be difficult to eliminate.
>
> > @@ -1177,6 +1371,8 @@ static void __init ns16550_parse_port_config(
> >           PARSE_ERR("Baud rate %d outside supported range.", uart->baud);
> >       if ( (uart->data_bits < 5) || (uart->data_bits > 8) )
> >           PARSE_ERR("%d data bits are unsupported.", uart->data_bits);
> > +    if ( (uart->reg_width != 1) && (uart->reg_width != 4) )
> > +        PARSE_ERR("red_width accepted values: 1 or 4.");
> "reg_width ..."
>
> Also please avoid the full stop.
>
> > --- a/xen/include/xen/serial.h
> > +++ b/xen/include/xen/serial.h
> > @@ -20,6 +20,8 @@ void serial_set_rx_handler(int handle, serial_rx_fn fn);
> >   
> >   /* Number of characters we buffer for a polling receiver. */
> >   #define serial_rxbufsz 32
> > +#define MAX_CMDLINE_LENGTH 512
> > +#define MAX_PARAM_VALUE_LENGTH 16
> Please don't put constants needed in only a single source file into
> a header, even less so with such generic names.
Buffer lengths are used in two files
- xen/common/kernel.c - cmdline_parse - opt buffer
(5 extra characters than cmdline mentioned in the next line)
- xen/drivers/char/ns16550.c - ns16550_parse_port_config - cmdline

Should these lengths be mentioned somewhere common OR an association be 
created
between them? I spent some time understanding why my code was eating up 5
characters out of nowhere ("com1=") when execution passed between these 
two.
If it was linked through a #define or a common macro, this might have be 
avoided.
However, if not the case, can I use static int const on the same page since
we want to be type-safe (numbers used in array indexes) and want limited 
scope?
> Jan
Thank you for the review comments,

--

Swapnil


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel