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

[Tom Lendacky <thomas.lendacky@amd.com>]

On Friday, November 22, 2013 04:59:21 PM Jan Beulich wrote:
> >>> On 22.11.13 at 17:45, Aravind Gopalakrishnan 
<Aravind.Gopalakrishnan@amd.com> 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.

This is one of the situations that Aravind mentioned previously. If the
full BAR is covered then Dom0 fails to boot/come up since it sees the device
(02:00.5 below), tries to access/configure it and fails.

02:00.0 Ethernet controller [0200]: Broadcom Corporation NetXtreme
BCM5725 Gigabit Ethernet PCIe [14e4:1643] (rev 10)
02:00.5 Communication controller [0780]: Broadcom Corporation Device 
[14e4:160a] (rev 01)
02:00.6 IPMI SMIC interface [0c07]: Broadcom Corporation Device [14e4:16b9] 
(rev 10)

I'm not sure what the options are here since we can't hide the device
from Dom0 to prevent it from trying to access/configure the device and
restricting the full BAR causes Dom0 to fail.

Thoughts?

Tom

> 
> > +        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
-- 
Tom

Thomas Lendacky
Advanced Micro Devices, Inc.