[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3] ns16550-Add-command-line-parsing-adjustments
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@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=2I 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 bitsSince 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 16Please 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 |