> 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
([EMAIL PROTECTED])
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 [EMAIL PROTECTED]
with "unsubscribe freebsd-hackers" in the body of the message
