|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3] ns16550-Add-command-line-parsing-adjustments
>>> On 31.03.17 at 17:42, <swapnil.paratey@xxxxxxx> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |