Re: [PATCH][2/2] RTAS MSI

[Jake Moilanen <moilanen@austin.ibm.com>]

> > +int rtas_enable_msi(struct pci_dev* pdev)
> > +{
> > +	static int seq_num = 1;
> 
> Do we really want seq_num to be static? By my reading of PAPR we only
> need to maintain the seq number across calls that return -2/990x, and we
> handle that all inside this function. If it does need to be unique for
> _all_ calls, then I don't see how seq_num being static is going to work,
> a different cpu could stomp on the seq_num value between calls, which
> presumably would make firmware mad.

Bah...I thought I fixed this a long time ago....must have gotten dropped
somewhere in the mix.

> > +	int reglen;
> > +	int ret[3];
> 
> You only need ret[2] I think, the first return value (status) is handled
> inside rtas_call for you.

Yup...

> > +	int dummy;
> > +	int n_intr;
> > +	int last_virq = NO_IRQ;
> > +	int virq;
> > +	unsigned int addr;
> > +	unsigned long buid = -1;
> 
> No need to set buid to -1 as you unconditionally assign to it later.

Agreed

> > +	struct device_node * dn;
> > +
> > +	dn = pci_device_to_OF_node(pdev);
> > +
> > +	if (!of_find_property(dn, "ibm,req#msi", &dummy))
> > +		return -ENOENT;
> 
> You don't need dummy, just pass NULL.

Yup.

> > +
> > +	reg = (u32 *) get_property(dn, "reg", &reglen);
> > +	if (reg == NULL || reglen < 20)
> > +		return -ENXIO;
> > +
> > +	devfn = (reg[0] >> 8) & 0xff;
> > +	busno = (reg[0] >> 16) & 0xff;
> > +
> > +	buid = get_phb_buid(dn->parent);
> > +	addr = (busno << 16) | (devfn << 8);
> 
> Why do we need to read the reg here, can't we just use the existing
> fields? ie:
> 
>         addr = (pdev->bus->number << 16) | (pdev->devfn << 8);

Patch coming w/ all these fixes.