[PATCH v3] ns16550-Add-command-line-parsing-adjustments

[PATCH v3] ns16550-Add-command-line-parsing-adjustments

[Swapnil Paratey]

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

Increase length of opt_com1 and opt_com2 buffers.

Signed-off-by: Swapnil Paratey <swapnil.paratey@amd.com>

---
Changed since v2:
 * Added name=value specification for com1 and com2 command line
   parameter input for UART devices
   Syntax: "com1=(positional parameters),(name-value parameters)"
 * Maintained previous positional specification for UART parameters
 * All parameters should be comma-separated

Changed since v1:
 * Changed opt_com1 and opt_com2 array size to 64 (power of 2).
 * Added descriptions for reg_width and reg_shift in
   docs/misc/xen-command-line.markdown
 * Changed subject to ns16550 from 16550 for better tracking.
---
 docs/misc/xen-command-line.markdown |  37 ++++++
 xen/common/kernel.c                 |   2 +-
 xen/drivers/char/ns16550.c          | 224 +++++++++++++++++++++++++++++++++---
 xen/include/xen/serial.h            |   2 +
 4 files changed, 250 insertions(+), 15 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 4daf5b5..0d9fdf1 100644
--- 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>
+ * `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
+
+The following are examples of correct specifications:
+`com1=115200,8n1,0x3f8,4`
+`com1=115200,8n1,0x3f8,4,reg_width=4,reg_shift=2`
+`com1=baud=115200,parity=n,stop_bits=1,io_base=0x3f8,reg_width=4`
+
 ### conring\_size
 > `= <size>`
 
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 4b87c60..0cee43a 100644
--- 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;
     const char *p = cmdline;
     const struct kernel_param *param;
     int bool_assert;
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index e4de3b4..ca5c515 100644
--- 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] = "";
 string_param("com1", opt_com1);
 string_param("com2", opt_com2);
 
+typedef enum e_serial_param_type {
+    BAUD=0,
+    BRIDGEBDF,
+    CLOCKHZ,
+    DATABITS,
+    DEVICE,
+    IO_BASE,
+    IRQ,
+    PARITY,
+    PORTBDF,
+    REG_SHIFT,
+    REG_WIDTH,
+    STOPBITS,
+    __MAX_SERIAL_PARAM /* introduce more parameters before this line */
+} serial_param_type;
+
 static struct ns16550 {
     int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq;
     u64 io_base;   /* I/O port or memory-mapped I/O address. */
@@ -77,6 +93,29 @@ static struct ns16550 {
 #endif
 } ns16550_com[2] = { { 0 } };
 
+struct serial_param_var
+{
+    char *sp_name;
+    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[] = {
+    {"baud", BAUD},
+    {"bridge", BRIDGEBDF},
+    {"clock_hz", CLOCKHZ},
+    {"data_bits", DATABITS},
+    {"dev", DEVICE},
+    {"io_base", IO_BASE},
+    {"irq", IRQ},
+    {"parity", PARITY},
+    {"port", PORTBDF},
+    {"reg_shift", REG_SHIFT},
+    {"reg_width", REG_WIDTH},
+    {"stop_bits", STOPBITS},
+};
+
 #ifdef CONFIG_HAS_PCI
 struct ns16550_config {
     u16 vendor_id;
@@ -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)
+{
+    char *param_name;
+    int i;
+
+    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);
+
+    /* 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)
 {
     int baud;
+    const char *conf;
+    char *name_val_pos;
 
-    /* No user-specified configuration? */
-    if ( (conf == NULL) || (*conf == '\0') )
+    conf = (const char *)*str;
+    name_val_pos = strchr(*str, '=');
+
+    /* 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 */
+
+    /* parse positional parameters here */
     if ( strncmp(conf, "auto", 4) == 0 )
     {
         uart->baud = BAUD_AUTO;
@@ -1132,13 +1215,13 @@ static void __init ns16550_parse_port_config(
         if ( strncmp(conf, "pci", 3) == 0 )
         {
             if ( pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com) )
-                return;
+                return 0;
             conf += 3;
         }
         else if ( strncmp(conf, "amt", 3) == 0 )
         {
             if ( pci_uart_config(uart, 0, uart - ns16550_com) )
-                return;
+                return 0;
             conf += 3;
         }
         else
@@ -1157,7 +1240,7 @@ static void __init ns16550_parse_port_config(
         conf = parse_pci(conf, NULL, &uart->ps_bdf[0],
                          &uart->ps_bdf[1], &uart->ps_bdf[2]);
         if ( !conf )
-            PARSE_ERR("Bad port PCI coordinates");
+            PARSE_ERR_RET("Bad port PCI coordinates");
         uart->ps_bdf_enable = 1;
     }
 
@@ -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)
+{
+    serial_param_type parsed_param;
+    char *token, *start;
+    char param_value[MAX_PARAM_VALUE_LENGTH];
+    bool dev_set;
+
+    dev_set = 0;
+    start = str;
+
+    while (start != NULL) /* strsep gives NULL when there are no tokens found */
+    {
+        /* when no tokens are found, start will be NULL */
+        token = strsep(&start, ",");
+
+        parsed_param = get_token_value(token, param_value);
+        switch(parsed_param)
+        {
+            case BAUD:
+                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]))
+                    PARSE_ERR_RET("Bad port PCI coordinates\n");
+                uart->ps_bdf_enable = 1;
+                break;
+            case CLOCKHZ:
+                uart->clock_hz = (simple_strtoul(param_value, NULL, 0) << 4);
+                break;
+            case DEVICE:
+                if ((strncmp(param_value, "pci", 3) == 0))
+                {
+                    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");
+                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);
+                break;
+        }
+    }
+
+    return 0;
+}
+
+static void __init ns16550_parse_port_config(
+    struct ns16550 *uart, const char *conf)
+{
+    char cmdline[MAX_CMDLINE_LENGTH];
+    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 */
+
+    /* 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:
     /* Sanity checks. */
     if ( (uart->baud != BAUD_AUTO) &&
@@ -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.");
     if ( (uart->stop_bits < 1) || (uart->stop_bits > 2) )
         PARSE_ERR("%d stop bits are unsupported.", uart->stop_bits);
     if ( uart->io_base == 0 )
diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h
index 1212a12..0fd1d76 100644
--- 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
 
 /* Number of characters we buffer for an interrupt-driven transmitter. */
 extern unsigned int serial_txbufsz;
-- 
2.7.4


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

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

[Jan Beulich]

>>> On 31.03.17 at 17:42, <swapnil.paratey@amd.com> wrote:

The title needs improvement - it doesn't really reflect what the
patch does.

> 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.

> +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.

>  {
>      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.

> +{
> +    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?

> +                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?

> +    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.

Jan

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

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

[Paratey, Swapnil]

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@amd.com>  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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Jan Beulich]

>>> On 10.04.17 at 20:47, <sparatey@amd.com> wrote:
> On 4/3/2017 6:55 AM, Jan Beulich wrote:
>>>>> On 31.03.17 at 17:42,<swapnil.paratey@amd.com>  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

Sounds good (albeit it sounds like you don't want to do the
requested splitting).

> Please let me know about
> - how to track the evolution of the patch (whether to mention [PATCH v4])

Definitely v4, yes.

> - whether this patch should mention ChangeSets from the previous versions

At least changed from v3 to v4 should be described, including the
change of title.

> - if the subject should be more precise and whether I should respect Git's
> 60-character recommended limit for patch subjects.

I don't think we normally pay particular attention to this, but it
doesn't hurt to not have overly long titles.

>>> --- 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.

Being able to fit all sub-options is indeed the primary criteria. If 128
fulfills this without being excessively much larger, the number should
be fine.

>>> -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.

Ah, I see. I think nowadays we'd prefer to avoid such hidden return
(or other control flow) statements, unless that unduly uglifies the
code.

>>> @@ -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.

This is a rather bogus argument imo: Boolean return values are in
no way worse. The common expectation to functions returning int
is that they can return actual values (perhaps -E... error codes).
To merely indicate success or failure, using true/false respectively
is quite common an approach.

>>> +            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.

I would think that my question indicated pretty clearly my position
on this: Silently ignoring user settings is almost as undesirable as
conflicting settings leading to misbehavior. Following your reply it
may indeed not be necessary to error out in this case, but a
warning should still be issued imo.

>>> +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).

I think this level of detail that you're asking for goes too far - you
should really work this out yourself (after all, as long as you can
explain _why_ you used a certain size, and as long as that
explanation can be followed, it's not like such choices can't be
left to your own judgment; you don't need reviewer feedback on
every nitty gritty detail). Buffers should be large enough to
accommodate all reasonable input, and they shouldn't be
excessively much larger than that. I don't think an overallocation
of 5 characters would come anywhere near "excessive".

>>> --- 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.

Judging by your patch, both were used only in ns16550.c. Hence
besides the names being too generic for that purpose, they would
belong into that file. If there is a connection to be made to
kernel.c, then I can see value in adding such constants, but in a
header suitable for their names and purposes. For example I don't
consider it reasonable for kernel.c to include serial.h - it should
have no need to know of serial driver specifics.

Jan

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

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

[Paratey, Swapnil]

Hi Jan,

I have a question about __initconst that you mentioned.

On 4/3/2017 6:55 AM, Jan Beulich wrote:

>>>> On 31.03.17 at 17:42, <swapnil.paratey@amd.com> wrote:
> The title needs improvement - it doesn't really reflect what the
> patch does.
>
>> 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.
>
>> +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).

Adding an __initconst before the variable name (or after it) makes
sp_vars go into the .init.data section if I check through
"objdump -t xen-syms | grep sp_vars"

I'm not being able to see an init.rodata section at all for any
other variable to emulate similar behavior i.e.
doing an "objdump -t xen-syms | grep .init.rodata"
doesn't show any results (whereas .init.data shows many).

The header file for __initconst defines it as .init.rodata but
sp_vars ends up in init.data

I compile Xen by going into xen/xen and running make -j`nproc`

Please let me know if I'm misunderstanding something.
What behavior I should expect and what is expected for adding
these string literals. Pointing to an example would be helpful.

Thanks,

Swapnil

>
>> @@ -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.
>
>>   {
>>       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.
>
>> +{
>> +    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?
>
>> +                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?
>
>> +    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.
>
> Jan
>

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

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

[Jan Beulich]

>>> On 17.04.17 at 17:51, <sparatey@amd.com> wrote:
> I have a question about __initconst that you mentioned.

Sure, but please trim your replies.

> On 4/3/2017 6:55 AM, Jan Beulich wrote:
>>>>> On 31.03.17 at 17:42, <swapnil.paratey@amd.com> wrote:
>>> +/* 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).
> 
> Adding an __initconst before the variable name (or after it) makes
> sp_vars go into the .init.data section if I check through
> "objdump -t xen-syms | grep sp_vars"
> 
> I'm not being able to see an init.rodata section at all for any
> other variable to emulate similar behavior i.e.
> doing an "objdump -t xen-syms | grep .init.rodata"
> doesn't show any results (whereas .init.data shows many).
> 
> The header file for __initconst defines it as .init.rodata but
> sp_vars ends up in init.data
> 
> I compile Xen by going into xen/xen and running make -j`nproc`
> 
> Please let me know if I'm misunderstanding something.
> What behavior I should expect and what is expected for adding
> these string literals. Pointing to an example would be helpful.

See the linker script: .init.rodata (and a few sub-flavors) get
merged into .init.data. We don't _currently_ have a need to
separate r/o from r/w init data, but source code should
nevertheless correctly reflect const-ness. Hence you looking
at xen-syms instead of ns16550.o is misleading you.

As to examples - please simply grep for other uses of __initconst.

Jan


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