Re: [PATCH 1/2] MIPS: SiByte: Set 32-bit bus mask for BCM1250 PCI

[Maciej W. Rozycki]

On Wed, 7 Nov 2018, Christoph Hellwig wrote:

> > +static int sb1250_bus_dma_mask(struct pci_dev *dev, void *data)
> > +{
> > +	struct sb1250_bus_dma_mask_exclude *exclude = data;
> > +
> > +	if (!exclude->set && (dev->vendor == PCI_VENDOR_ID_SIBYTE &&
> > +			      dev->device == PCI_DEVICE_ID_BCM1250_HT)) {
> > +		exclude->start = dev->subordinate->number;
> > +		exclude->end = pci_bus_max_busnr(dev->subordinate);
> > +		exclude->set = true;
> > +		dev_dbg(&dev->dev, "not disabling DAC for [bus %02x-%02x]",
> > +			exclude->start, exclude->end);
> > +	} else if (!exclude->set ||
> > +		   (exclude->set && (dev->bus->number < exclude->start ||
> > +				     dev->bus->number > exclude->end))) {
> > +		dev_dbg(&dev->dev, "disabling DAC for device");
> > +		dev->dev.bus_dma_mask = DMA_BIT_MASK(32);
> > +	} else {
> > +		dev_dbg(&dev->dev, "not disabling DAC for device");
> > +	}
> > +	return 0;
> 
> Hmm, these conditions look very hard to read to me.  Wouldn't this
> have the same effect?
> 
> 	if (exclude->set)
> 		return;

 Nope, `exclude->set' only means we already know what range to exclude 
(and that gets set mid-way through scanning as the HT bridge is 
encountered).  Then if it's unset, we know we are (still) outside that 
range.

 Maybe I can flatten the conditions at the small cost of executing some 
code unnecessarily.  But that won't be a big deal as this stuff is only 
executed once at boot and isn't performance critical.

 It'll have to wait until next week though as I'll be travelling 
throughout the rest of this and won't be able to test anything.

  Maciej