Messages in this thread | | | Date | Tue, 23 Aug 2005 17:17:45 -0500 (CDT) | From | Brent Casavant <> | Subject | Re: [PATCH 2/2] external interrupts: IOC4 driver |
| |
On Sat, 20 Aug 2005, Christoph Hellwig wrote:
> > +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.
Oops. Yes, in the past BLK_DEV_SGIIOC4 was the core abstraction config symbol. That was fixed a release or two ago, but obviously I've been working on the external interrupt stuff for a while. Fixed.
> > + 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?
Yes, the Altix 330 uses third-party chips to provide base I/O functions. Other future Altix products will similarly lack IOC4 (at least in their base configuration).
> > +static ssize_t ioc4_extint_get_modelist(struct extint_device *ed, char *buf) { > > opening brace on a separate line please.
Sorry. Lindent took to mungeing my source (indenting in structure definitions in particular) in unreadable ways, so I stopped using it. Old coding styles die hard and all.
> > +#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?
For security correctness we cannot allow this page to be mapped by anything larger than a 16K page, as that would allow the user access to control registers which are not harmless if exposed.
However, the loss of functionality isn't silent. In ioc4_extint_mmap() we return -ENXIO if the mapping cannot be performed for the aforementioned reason. A well-written application will check for failures from mmap(2).
> > +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
It was just one of many things where I coded defensively and intended to revisit it later. Unfortunately the revisit never took place. Thanks for the catch -- fixed now.
Thanks, Brent
-- Brent Casavant If you had nothing to fear, bcasavan@sgi.com how then could you be brave? Silicon Graphics, Inc. -- Queen Dama, Source Wars - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |