Re: [PATCH V2 1/8] powerpc/powernv: Add a virtual irqchip for opal events

[Neelesh Gupta <neelegup@linux.vnet.ibm.com>]

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

Hi Alistair,

With all the patches applied on top of 'v4.0-rc7', I see this issue during
the boot itself http://pastebin.hursley.ibm.com/918

Few compile warnings and minor comments.
drivers/tty/hvc/hvc_opal.c: In function ‘hvc_opal_probe’:
drivers/tty/hvc/hvc_opal.c:174:6: warning: unused variable ‘rc’ 
[-Wunused-variable]
   int rc;
       ^
drivers/tty/hvc/hvc_opal.c: At top level:
drivers/tty/hvc/hvc_opal.c:65:13: warning: ‘hvc_opal_event_registered’ 
defined but not used [-Wunused-variable]
  static bool hvc_opal_event_registered;



On 04/10/2015 01:54 PM, Alistair Popple wrote:
> Whenever an interrupt is received for opal the linux kernel gets a
> bitfield indicating certain events that have occurred and need handling
> by the various device drivers. Currently this is handled using a
> notifier interface where we call every device driver that has
> registered to receive opal events.
>
> This approach has several drawbacks. For example each driver has to do
> its own checking to see if the event is relevant as well as event
> masking. There is also no easy method of recording the number of times
> we receive particular events.
>
> This patch solves these issues by exposing opal events via the
> standard interrupt APIs by adding a new interrupt chip and
> domain. Drivers can then register for the appropriate events using
> standard kernel calls such as irq_of_parse_and_map().
>
> Signed-off-by: Alistair Popple <alistair@popple.id.au>
> ---
>
> +
> +static int __init opal_event_init(void)
> +{
> +	struct device_node *dn, *opal_node;
> +	const __be32 *irqs;
> +	int i, irqlen;
> +
> +	opal_node = of_find_node_by_path("/ibm,opal");
> +	if (!opal_node) {
> +		pr_warn("opal: Node not found\n");
> +		return -ENODEV;
> +	}
> +
> +	dn = of_find_compatible_node(NULL, NULL, "ibm,opal-event");
> +
> +	/* If dn is NULL it means the domain won't be linked to a DT
> +	 * node so therefore irq_of_parse_and_map(...) wont work. But
> +	 * that shouldn't be problem because if we're running a
> +	 * version of skiboot that doesn't have the dn then the
> +	 * devices won't have the correct properties and will have to
> +	 * fall back to the legacy method (opal_event_request(...))
> +	 * anyway. */
> +	opal_event_irqchip.domain =
> +		irq_domain_add_linear(dn, 64, &opal_event_domain_ops,

A macro would be better, which is maximum event bits we have.

> +				      &opal_event_irqchip);
> +	if (IS_ERR(opal_event_irqchip.domain)) {
> +		pr_warn("opal: Unable to create irq domain\n");
> +		return PTR_ERR(opal_event_irqchip.domain);
> +	}
> +
> +	/* Get interrupt property */
> +	irqs = of_get_property(opal_node, "opal-interrupts", &irqlen);

of_node_put()
Should decrement the refcount of the nodes 'opal_node' and 'dn' (if !NULL)
before returning from the function.

> +	opal_irq_count = irqs ? (irqlen / 4) : 0;
> +	pr_debug("Found %d interrupts reserved for OPAL\n", opal_irq_count);
> +
> +	/* Install interrupt handlers */
> +	opal_irqs = kcalloc(opal_irq_count, sizeof(unsigned int), GFP_KERNEL);

Safer to use 'sizeof(*opal_irqs)'

Neelesh.

> +	for (i = 0; irqs && i < opal_irq_count; i++, irqs++) {
> +		unsigned int irq, virq;
> +		int rc;
> +
> +		/* Get hardware and virtual IRQ */
> +		irq = be32_to_cpup(irqs);
> +		virq = irq_create_mapping(NULL, irq);
> +		if (virq == NO_IRQ) {
> +			pr_warn("Failed to map irq 0x%x\n", irq);
> +			continue;
> +		}
> +
> +		/* Install interrupt handler */
> +		rc = request_irq(virq, opal_interrupt, 0, "opal", NULL);
> +		if (rc) {
> +			irq_dispose_mapping(virq);
> +			pr_warn("Error %d requesting irq %d (0x%x)\n",
> +				 rc, virq, irq);
> +			continue;
> +		}
> +
> +		/* Cache IRQ */
> +		opal_irqs[i] = virq;
> +	}
> +
> +	return 0;
> +}
> +machine_core_initcall(powernv, opal_event_init);
> +
>


[-- Attachment #2: Type: text/html, Size: 5057 bytes --]