On Wed, Mar 08, 2017 at 12:40:59PM +0100, Alexander Gordeev wrote:
> On Wed, Mar 08, 2017 at 11:10:26AM +0100, Andrew Jones wrote:
> > > > > -void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr)
> > > > > +void pci_bar_set_addr(struct pci_dev *dev,
> > > > > + unsigned int bar_num, phys_addr_t addr)
> > > > > {
> > > > > int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
> > > > >
> > > > > + CHECK_BAR_NUM(bar_num);
> > > > > + assert(addr != INVALID_PHYS_ADDR);
> > > >
> > > > Shouldn't this be something like
> > > >
> > > > assert((addr & PHYS_MASK) == addr)
> > >
> > > Nope. At this stage PCI bus is scanned and pci_dev::resource[]
> > > is initialized with proper values. A value of INVALID_PHYS_ADDR in
> > > dev->resource[bar_num] indicates BAR #bar_num is not implemented.
> > > Thus, we do not want to screw up this assumption with a test case
> > > trying to write INVALID_PHYS_ADDR to a the BAR.
> >
> > Of course, by why would any other invalid phys addr be OK? Hmm, maybe
> > it should be allowed for test case purposes.
>
> Yeah, I think it should be allowed.
>
> > > > But that means we need to ensure all architectures define PHYS_MASK...
> > > >
> > > > > + assert(dev->resource[bar_num] != INVALID_PHYS_ADDR);
> > > > > + if (pci_bar_is64(dev, bar_num)) {
> > > > > + assert(bar_num + 1 < PCI_BAR_NUM);
> > > >
> > > > Use CHECK_BAR_NUM(bar_num + 1)
> > > >
> > > > > + assert(dev->resource[bar_num] == dev->resource[bar_num + 1]);
> > > >
> > > > Can this ever happen without the unit test explicitly messing things up?
>
> No, unless there is really something wrong with the code.
>
> > No answer for this. I don't see the point of an assert that can never
> > fail.
>
> So this is code integrity assert, not test case input check.
> Do you want me to remove it?
Yes please. We don't generally defend the lib code from itself. We use
asserts for anything the user could influence and for some test case
inputs, in order to help the test writer avoid going down obviously
buggy paths. Although, like above, we also try to give the test writer
freedom to do crazy things, as that's the whole point of a testsuite :-)
Thanks,
drew
