Re: [PATCH 2/2] external interrupts: IOC4 driver

[Christoph Hellwig <hch@infradead.org>]

> +config EXTINT_SGI_IOC4
> +	tristate "Device driver for SGI IOC4 external interrupts"
> +	depends on (IA64_GENERIC || IA64_SGI_SN2) && EXTINT && BLK_DEV_SGIIOC4

Is the ioc4 core abstraction config symbol really BLK_DEV_SGIIOC4?
That probably wants fixing in a separate patch.

> +	  This option enables support for the external interrupt ingest
> +	  and generation capabilities of SGI IOC4 IO controllers.  If
> +	  you have an SGI Altix with an IOC4 based IO card, say Y.
> +	  Otherwise, say N.

Is there any Altix without an ioc4?

> + */
> +static ssize_t ioc4_extint_get_modelist(struct extint_device *ed, char *buf) {

opening brace on a separate line please.

> +#if PAGE_SIZE <= IOC4_A_INT_OUT_LENGTH
> +	/* Only set up INT_OUT register alias page if the system page size
> +	 * is equal to or less than the register alias page size.  Otherwise
> +	 * the user would have access to registers other than INT_OUT.
> +	 */
> +	a_int_out = pci_resource_start(ied->idd->idd_pdev, 0) +
> +	    IOC4_A_INT_OUT_OFFSET;
> +	if (!a_int_out) {
> +		printk(KERN_WARNING
> +		       "%s: Unable to get IOC4 int_out alias mapping "
> +		       "for pci_dev 0x%p.\n", __FUNCTION__, ied->idd->idd_pdev);
> +		goto skip_alias;
> +	}
> +	if (!request_region(a_int_out, IOC4_A_INT_OUT_LENGTH,
> +			    "ioc4_a_int_out")) {

This looks rather bad.  So the driver silently has less functionality
when using a bigger page size?

> +	/* Enable interrupt input */
> +	ret = ioc4_extint_input_enable(ied);
> +	if (ret)
> +		goto out_enable;
> +
> +	return 0;
> +
> +out_enable:
> +	extint_device_unregister(idd->idd_extint_data);
> +out_register:
> +	ioc4_extint_device_destroy(ied);
> +out_device:
> +	ioc4_extint_input_teardown(ied);
> +	ioc4_extint_output_teardown(ied);
> +	kfree(ied);
> +out:
> +	return ret;
> +}
> +
> +static int ioc4_extint_remove(struct ioc4_driver_data *idd)
> +{
> +	struct extint_device *ed = idd->idd_extint_data;
> +	struct ioc4_extint_device *ied;
> +
> +	/* If probe failed, avoid trying to remove */
> +	if (ed)
> +		ied = extint_get_devdata(ed);
> +	else
> +		return -ENXIO;

This should at lease be written:

	if (!ed)
		return -ENXIO;
	ied = extint_get_devdata(ed);

but I don't understand how it can happen anyway.  ->remove shoould
never be called unless ->probe initialized the device fully and
returned 0