Re: [Xen-devel] [PATCH v5] ns16550: Add support for UART parameters to be specifed with name-value pairs

[Jan Beulich]

>>> On 19.04.17 at 20:57,  wrote:
> Changed since v4:
>  * Changes from [PATCH v4] code review

I'm sorry, but this is not enough.

> @@ -77,6 +94,30 @@ static struct ns16550 {
>  #endif
>  } ns16550_com[2] = { { 0 } };
>  
> +struct serial_param_var {
> +const char name[12];

Pointless const.

> +static enum __init serial_param_type get_token(char *token, char *ext_value)

I don't see the need for the ext_ prefix.

> +{
> +const char *param_name;
> +unsigned int i;
> +
> +param_name = strsep(, "=");
> +if ( param_name == NULL )
> +return num_serial_params;
> +
> +/* Linear search for the parameter. */
> +for ( i = 0; i < ARRAY_SIZE(sp_vars); i++ )
> +{
> +if ( strcmp(sp_vars[i].name, param_name) == 0 )
> +{
> +/*
> + * Token has the param value after the equal to sign.
> + * ext_value is a 16-byte buffer - param_value[16].
> + */
> +strlcpy(ext_value, token, 16);

So why do you copy here, instead of simply passing the pointer
back? This is the more that the caller didn't tell you it 16 bytes of
space allocated for the output value.

> +static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
> +{
> +char *token, *start = str;
> +char param_value[16];
> +bool dev_set = false;
> +
> +if ( (str == NULL) || (*str == '\0') )
> +return true;
> +
> +do
> +{
> +/* When no tokens are found, start will be NULL */
> +token = strsep(, ",");
> +
> +switch( get_token(token, param_value) ) {

Coding style (missing blank and brace on its own line).

> +case baud:
> +uart->baud = simple_strtoul(param_value, NULL, 0);
> +break;
> +
> +case bridge_bdf:
> +if ( !parse_pci(param_value, NULL, >ps_bdf[0],
> +>ps_bdf[1], >ps_bdf[2]) )
> +PARSE_ERR_RET("Bad port PCI coordinates\n");
> +uart->ps_bdf_enable = 1;
> +break;
> +
> +case clock_hz:
> +uart->clock_hz = (simple_strtoul(param_value, NULL, 0) << 4);

Pointless parentheses.

Jan


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

[Xen-devel] [PATCH v5] ns16550: Add support for UART parameters to be specifed with name-value pairs

[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=115200,8n1,0x3f8,4,reg_width=4,reg_shift=2
eg. com1=baud=115200,parity=n,reg_width=4,reg_shift=2,irq=4

Signed-off-by: Swapnil Paratey i

---
Changed since v4:
 * Changes from [PATCH v4] code review

Changed since v3:
 * Changed subject/title of the patch
   Previous name: ns16550-Add-command-line-parsing-adjustments
 * Increased length of opt_com1 and opt_com2 buffers to 128 bytes.
 * Implementation changes from [PATCH v3] code review

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 |  38 ++
 xen/common/kernel.c |   2 +-
 xen/drivers/char/ns16550.c  | 248 +---
 3 files changed, 271 insertions(+), 17 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown 
b/docs/misc/xen-command-line.markdown
index 450b222..53052b2 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -324,6 +324,44 @@ 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`- Similar to bridge-bdf in positional parameters.
+ Used to determine the PCI bridge to access the UART device.
+ Notation is xx:xx.xx :.
+ * `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 :.
+ * `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
 > `= `
 
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 8461871..e1ebb0b 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -49,7 +49,7 @@ static void __init assign_integer_param(
 
 static void __init _cmdline_parse(const char *cmdline)
 {
-char opt[100], *optval, *optkey, *q;
+char opt[128], *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..89e0e4d 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -38,11 +38,28 @@
  * 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[128] = "";
+static char __initdata opt_com2[128] = "";
 string_param("com1", opt_com1);
 string_param("com2", opt_com2);
 
+enum serial_param_type {
+baud,
+bridge_bdf,
+clock_hz,
+data_bits,
+device,
+io_base,
+irq,
+parity,
+port_bdf,
+reg_shift,
+reg_width,
+stop_bits,
+/* List all parameters before this line. */
+