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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.04.17 at 01:01, <swapnil.paratey@xxxxxxx> wrote:
> +static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
> +{
> +    char *token, *start = str;
> +    char *param_value = NULL;
> +    bool dev_set = false;
> +
> +    if ( (str == NULL) || (*str == '\0') )
> +        return true;
> +
> +    do
> +    {
> +        /* When no tokens are found, start will be NULL */
> +        token = strsep(&start, ",");
> +
> +        switch ( get_token(token, &param_value) )
> +        {
> +        case baud:
> +            uart->baud = simple_strtoul(param_value, NULL, 0);
> +            break;
> +
> +        case bridge_bdf:
> +            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 clock_hz:
> +            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 = true;
> +            }
> +            else if ( strncmp(param_value, "amt", 3) == 0 )
> +            {
> +                pci_uart_config(uart, 0, uart - ns16550_com);
> +                dev_set = true;
> +            }
> +            break;
> +
> +        case io_base:
> +            if ( dev_set )
> +            {
> +                printk(XENLOG_WARNING
> +                       "Can't use io_base with dev=pci or dev=amt 
> options\n");
> +                break;
> +            }
> +            uart->io_base = simple_strtoul(param_value, NULL, 0);
> +            break;
> +
> +        case irq:
> +            uart->irq = simple_strtoul(param_value, NULL, 0);
> +            break;
> +
> +        case data_bits:
> +            uart->data_bits = simple_strtoul(param_value, NULL, 0);
> +            break;
> +
> +        case parity:
> +            uart->parity = parse_parity_char(*param_value);
> +            break;
> +
> +        case port_bdf:
> +            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 = true;
> +            break;

Considering you imply no fall through from the "if()" body to what
follows the "if()" here, I think ...

> +        case stop_bits:
> +            uart->stop_bits = simple_strtoul(param_value, NULL, 0);
> +            break;
> +
> +        case reg_shift:
> +            uart->reg_shift = simple_strtoul(param_value, NULL, 0);
> +            break;
> +
> +        case reg_width:
> +            uart->reg_width = simple_strtoul(param_value, NULL, 0);
> +            break;
> +
> +        default:
> +            PARSE_ERR_RET("Invalid parameter: %s\n", token);
> +            break;

... the "break" here should be removed. Or alternatively the "if()"
above (and also the bridge_bdf one) should gain an "else". I'd be
fine with doing the former while committing, but if you prefer the
latter, please submit v7. With either adjustment
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan


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