Re: [Xen-devel] [PATCH V5] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.11.13 at 17:45, Aravind Gopalakrishnan 
>>> <Aravind.Gopalakrishnan@xxxxxxx> wrote:
> @@ -432,9 +474,20 @@ static void __init ns16550_endboot(struct serial_port 
> *port)
>  {
>  #ifdef HAS_IOPORTS
>      struct ns16550 *uart = port->uart;
> +    unsigned long sfn, efn;
>  
>      if ( uart->remapped_io_base )
> +    {
> +        sfn = PFN_UP((unsigned long) uart->io_base + (0x8 * (1 << 
> uart->reg_shift)));
> +        efn = PFN_DOWN((unsigned long) uart->io_base + uart->io_size - 1);

So I told you on the previous round that these casts are wrong.
Why did you keep them.

Also, I don't see why you adjust io_base above - you want to cover
the full BAR, no matter what the register shift.

> +        if ( sfn > efn )
> +            BUG();

Ehm - what? Without taking the specifics of BARs into account, this
can validly happen (which is why I commented on it on the previous
version). But now that I remembered that the value comes from a
BAR, it can't happen, and hence you either don't check it at all, or
ASSERT() rather than BUG().

> +                    else
> +                        size = len & PCI_BASE_ADDRESS_MEM_MASK;
> +
> +                    size &= -(size);

Stray parentheses.

> +                        /* 
> +                         * Force length of mmio region to be at least 
> +                         * 8 bytes times (1 << reg_shift)
> +                         */
> +                        if ( size < (0x8 * (1 << uart_config[i].reg_shift)) )

                        if ( size < (8 << uart_config[i].reg_shift) )

Jan


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