Re: iwi(4) patch

[Jean-Yves Migeon <jym%NetBSD.org@localhost>]

Hi list,

Sorry for this late answer, looks like half of my mails to tech-net@ get 
stuck in the middle of nowhere thanks to my ISP.

David Young wrote:
> On Sun, Mar 08, 2009 at 01:22:19AM +0100, Jean-Yves Migeon wrote:
> > Hi,
> >
> > With a Xen debug kernel (PAE off), iwi(4) is not able to attach during  
> > autoconf ("XENMEM_decrease_reservation failed!" issue) on my laptop.  
> > This should not be fatal to the boot process, but in fact, it is; errors  
> > are (from my PoV) incorrectly handled in if_iwi.c.
> >
> > I have local fixes to it [1]. However, as it is not a part of the kernel  
> > I am familiar with, I would like more experienced people to review my 
> > patch:
> >
> > - some bus_dma fix, with checks against invalid dmap.
> >
> > - move the (RX and TX) rings' allocation code later in iwi_attach() to  
> > avoid NULL pointer dereference if allocation fails (iwi_detach()  
> > manipulates structure variables that are not set in case of a failed  
> > allocation)
> >
> > - avoid double free in case of failure during attach. If an allocation  
> > fails, do not free it directly, this is handled by iwi_detach() routine.
>
> Judging by your patch alone, if iwi_attach() fails midway through,
> it will leak resources, will it not?

Should not, but that's why I asked for a review.

My change to iwi_attach() reflects the ones in iwi_detach(), and sets 
variables that iwi_detach() expects to use (for example, sc->sc_st, or 
the value in different TX/RX rings). The first "half" of iwi_attach() 
are now softc/ifp manipulations and PCI handling.

Only leak I just noticed is in the case of iwi_reset() returning a 
failure, which ends iwi_attach() without unmapping the interrupts. Two 
possible solutions though:
- move more softc/ifp stuff to the top, and maybe have some extra checks 
in iwi_detach()
- call pci_intr_disestablish() in iwi_detach() error path, then return.

Cheers,

--
Jean-Yves Migeon
jym%NetBSD.org@localhost