From owner-freebsd-hackers  Sat Jan 15 21:39:17 2000
Delivered-To: freebsd-hackers@freebsd.org
Received: from laurasia.com.au (lauras.lnk.telstra.net [139.130.93.142])
	by hub.freebsd.org (Postfix) with ESMTP id AC2681520B
	for <hackers@freebsd.org>; Sat, 15 Jan 2000 21:39:09 -0800 (PST)
	(envelope-from mike@laurasia.com.au)
Received: (from mike@localhost)
	by laurasia.com.au (8.9.1a/8.9.1) id NAA20602;
	Sun, 16 Jan 2000 13:38:24 +0800 (WST)
Date: Sun, 16 Jan 2000 13:38:24 +0800 (WST)
From: Michael Kennett <mike@laurasia.com.au>
Message-Id: <200001160538.NAA20602@laurasia.com.au>
To: hackers@freebsd.org, winter@jurai.net
Subject: Re: Use of newbus in sys/pci/pci.c
Sender: owner-freebsd-hackers@FreeBSD.ORG
Precedence: bulk
X-Loop: FreeBSD.ORG

> On Sat, 15 Jan 2000, Michael Kennett wrote:
> > I don't understand the line that extracts the ivars from the child
> > device. Isn't it the case that the ivars are a property of the bus
> > device (dev)?
> 
> No since the parent may not be a PCI device.  In any case we're dealing
> with a device (child) that wishes to make a resource allocation; the
> parent device provides methods for this and its methods store the relevent
> information on the structure assigned to the device IVARS (by the parent).
> 
> The IVARS belong to the device, not the parent.

Thanks for the explanation - I can see that now in the code (function
pci_add_children()).

> 
> I've probably not helped much but the code is correct.
> 

I'm still not convinced :-).

Let me explain my concerns.  The child device does not necessarily have
to be a direct child of the pci bus (i.e. it is not necessarily true
that device_get_parent(child) == dev).  This can occur, for example,  with
the pci-isa bridge which just passes through resource requests from the
ISA bus to the PCI bus. For the isab bridge, this is documented in
sys/pci/pcisupport.c:

static device_method_t isab_methods[] = {
	/* ... [edited] ... */
	DEVMETHOD(bus_alloc_resource,	bus_generic_alloc_resource),
	/* ... [edited] ... */
};

From sys/kern/subr_bus.c  we have:

struct resource *
bus_generic_alloc_resource(device_t dev, device_t child, int type, int *rid,
			   u_long start, u_long end, u_long count, u_int flags)
{
	/* Propagate up the bus hierarchy until someone handles it. */
	if (dev->parent)
		return (BUS_ALLOC_RESOURCE(dev->parent, child, type, rid, 
					   start, end, count, flags));
	else
		return (NULL);
}


So, after a resource allocation request has come through the pci-isa
bridge, the child device is no longer a direct child of the bus (i.e.
it is not true that device_get_parent(child)==dev).

As a consequence, in the sys/pci/pci.c code, the line

	struct pci_devinfo *dinfo = device_get_ivars(child);

can be extracting ivars from a (grand)child device that has not been
initialised with the pci_devinfo ivar structure.

This is dangerous!

It appears that the only reason the code works is because of the
sentinel in resource_list_alloc():

struct resource *
resource_list_alloc(struct resource_list *rl .... /* edited */
{
    struct resource_list_entry *rle = 0;
    int passthrough = (device_get_parent(child) != bus);
    int isdefault = (start == 0UL && end == ~0UL);

    if (passthrough) {
        return BUS_ALLOC_RESOURCE(device_get_parent(bus), child,
				  type, rid,
				  start, end, count, flags);
    }

    rle = resource_list_find(rl, type, *rid);

    /* ... edited */
}


In the case of a passthrough,  the resource list extracted from the
ivars are never referenced.  In other words, the incorrect cast never
blows up :-)


Wouldn't the code below be safer and clearer?  It should also be
functionally equivalent to the existing code.

static struct resource *
pci_alloc_resource(device_t dev, device_t child, int type, int *rid,
		   u_long start, u_long end, u_long count, u_int flags)
{
    int passthrough = (device_get_parent(child) != dev);
    if (passthrough) {
        return BUS_ALLOC_RESOURCE(device_get_parent(bus), child,
                                  type, rid, start, end, count, flags);
    }
    else {
        struct pci_devinfo *dinfo = device_get_ivars(child);
        struct resource_list *rl = &dinfo->resources;

        return resource_list_alloc(rl, dev, child, type, rid,
            start, end, count, flags);
    }
}


Regards,

Mike Kennett
(mike@laurasia.com.au)

PS. Sorry about the faulty dates - using an old version of elm.  Main machine
is down due to power surges/lightening  last night.



To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-hackers" in the body of the message