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

[Michael Ellerman <michael@ellerman.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3003 bytes --]

On Thu, 2006-07-27 at 13:27 -0500, Jake Moilanen wrote:
> Rebased with the IRQ layer rewrite.  The code is not deallocating the
> vectors on a pci_disable_msi().  This is to work around a firmware
> vector release bug.  Plus it is really not needed, as
> irq_create_mapping() just returns mappings to irqs that it knows of.
> 
> Additionally, the patch includes the client architecture bit for MSI,
> and correctly identifying that MSI is edge triggered.  

Hey Jake, just a few niggles below ..

> Index: 2.6-msi/drivers/pci/msi-rtas.c
> ===================================================================
> --- /dev/null
> +++ 2.6-msi/drivers/pci/msi-rtas.c
> @@ -0,0 +1,111 @@
> +/*
> + * Jake Moilanen <moilanen@austin.ibm.com>
> + * Copyright (C) 2006 IBM
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; version 2 of the
> + * License.
> + *
> + */
> +
> +#include <linux/pci.h>
> +#include <linux/irq.h>
> +#include <asm/rtas.h>
> +#include <asm/hw_irq.h>
> +#include <asm/ppc-pci.h>
> +
> +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.

> +	int i;
> +	int rc;
> +	int query_token = rtas_token("ibm,query-interrupt-source-number");
> +	int devfn;
> +	int busno;
> +	u32 *reg;
> +	int reglen;
> +	int ret[3];

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

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

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

> +
> +	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);

cheers

-- 
Michael Ellerman
IBM OzLabs

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 191 bytes --]