DRIVER_UNIDENTIFY

[John Baldwin]

How about creating a new driver_if.m entry point that is the complement to
DRIVER_IDENTIFY.  I would call it DRIVER_UNIDENTIFY() and would change
bus_generic_detach() to call it on each driver similar to how
bus_generic_probe() currently calls DRIVER_IDENTIFY() for each driver.  This
would allow drivers that create devices in DRIVER_IDENTIFY() have a place to
remove the device when they are unloaded.

--
John Baldwin <jhb@...>  <><  http://www.FreeBSD.org/~jhb/
"Power Users Use the Power to Serve"  =  http://www.FreeBSD.org
_______________________________________________
freebsd-new-bus@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-new-bus
To unsubscribe, send any mail to "freebsd-new-bus-unsubscribe@..."

[M. Warner Losh]

> How about creating a new driver_if.m entry point that is the complement to
> DRIVER_IDENTIFY.  I would call it DRIVER_UNIDENTIFY() and would change
> bus_generic_detach() to call it on each driver similar to how
> bus_generic_probe() currently calls DRIVER_IDENTIFY() for each driver.  This
> would allow drivers that create devices in DRIVER_IDENTIFY() have a place to
> remove the device when they are unloaded.

I have mixed feelings about this.

First, the identify routine has identified a hunk of hardware and has
placed it into the tree.  The driver disappearing doesn't change the
fact that the hardware is still there.  Adding this new function would
further blur the lines between attached drivers, the hardware and
nodes in the device tree.  Device nodes are expected to be there
unattached.  Once the device driver has detached from the device node,
there's no harm in leaving the node arround as there are no dangling
references.

Second, the bus may be the one that decides what hardware is there.  I
have a SOC chip that has a number of different children of its nexus
that are always there, and will always be there.  I do not want the
children drivers to know anything of their location, etc, since they
cannot know because a different SOC will have the children at a
different location.  Of course, the easy thing here is to never call
the idenfify routine at all for this bus, but this would require
changes to the code.

However, we do currently have a assymetrical arrangement.  There's a
way to add the device, but too many drivers are 'stupid' in how they
add the device.  They neglect to check to make sure that the device
hasn't already been added, which is what causes the grief.

Maybe it would be better to have a better way to add instances such
that if the idenify routine used this better way that it could be
called many times (eg, make it idempotent).  Right now we recommend
that driver writers do the following (which is idempotent):

static void
tscnmi_identify( driver_t* driver, device_t parent )
{
    devclass_t dc;

    dc = devclass_find(DRIVERNAME);
    if (devclass_get_device(dc, 0) == NULL) {
        if (BUS_ADD_CHILD(parent, 0, DRIVERNAME, 0) == 0)
            panic("failed to add " DRIVERNAME);
    }
}

I think a better solution would be:

static void
tscnmi_identify( driver_t* driver, device_t parent )
{
        device_add_child_once(parent, DRIVERNAME);
}

where device_add_child_once would look like the following (run through
style(9)izer):

int
device_add_child_once(device_t parent, char *name)
{
    devclass_t dc;

    dc = devclass_find(name);
    if (devclass_get_device(dc, 0) == NULL)
    return BUS_ADD_CHILD(parent, 0, name, 0);
    return 0;
}
_______________________________________________
freebsd-new-bus@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-new-bus
To unsubscribe, send any mail to "freebsd-new-bus-unsubscribe@..."

[John Baldwin]

On Thursday 15 December 2005 01:13 pm, Warner Losh wrote:

> > How about creating a new driver_if.m entry point that is the complement
> > to DRIVER_IDENTIFY.  I would call it DRIVER_UNIDENTIFY() and would change
> > bus_generic_detach() to call it on each driver similar to how
> > bus_generic_probe() currently calls DRIVER_IDENTIFY() for each driver.
> > This would allow drivers that create devices in DRIVER_IDENTIFY() have a
> > place to remove the device when they are unloaded.
>
> I have mixed feelings about this.
>
> First, the identify routine has identified a hunk of hardware and has
> placed it into the tree.  The driver disappearing doesn't change the
> fact that the hardware is still there.  Adding this new function would
> further blur the lines between attached drivers, the hardware and
> nodes in the device tree.  Device nodes are expected to be there
> unattached.  Once the device driver has detached from the device node,
> there's no harm in leaving the node arround as there are no dangling
> references.
>
> Second, the bus may be the one that decides what hardware is there.  I
> have a SOC chip that has a number of different children of its nexus
> that are always there, and will always be there.  I do not want the
> children drivers to know anything of their location, etc, since they
> cannot know because a different SOC will have the children at a
> different location.  Of course, the easy thing here is to never call
> the idenfify routine at all for this bus, but this would require
> changes to the code.
>
> However, we do currently have a assymetrical arrangement.  There's a
> way to add the device, but too many drivers are 'stupid' in how they
> add the device.  They neglect to check to make sure that the device
> hasn't already been added, which is what causes the grief.
>
> Maybe it would be better to have a better way to add instances such
> that if the idenify routine used this better way that it could be
> called many times (eg, make it idempotent).  Right now we recommend
> that driver writers do the following (which is idempotent):
>
> static void
> tscnmi_identify( driver_t* driver, device_t parent )
> {
>     devclass_t dc;
>
>     dc = devclass_find(DRIVERNAME);
>     if (devclass_get_device(dc, 0) == NULL) {
>         if (BUS_ADD_CHILD(parent, 0, DRIVERNAME, 0) == 0)
>             panic("failed to add " DRIVERNAME);
>     }
> }
>
> I think a better solution would be:
>
> static void
> tscnmi_identify( driver_t* driver, device_t parent )
> {
> device_add_child_once(parent, DRIVERNAME);
> }
>
> where device_add_child_once would look like the following (run through
> style(9)izer):
>
> int
> device_add_child_once(device_t parent, char *name)
> {
>     devclass_t dc;
>
>     dc = devclass_find(name);
>     if (devclass_get_device(dc, 0) == NULL)
>     return BUS_ADD_CHILD(parent, 0, name, 0);
>     return 0;
> }

Actually, those methods enforce one instance in the system.  I want to enforce
one instance per parent device (the example here is acpi_video which attaches
to vgapci in my agp_cvs.patch and if you have multiple video cards, each
might have its own acpi_video driver).  I'm about to fix acpi_video, but was
curious if we could come up with a better overall solution.

--
John Baldwin <jhb@...>  <><  http://www.FreeBSD.org/~jhb/
"Power Users Use the Power to Serve"  =  http://www.FreeBSD.org
_______________________________________________
freebsd-new-bus@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-new-bus
To unsubscribe, send any mail to "freebsd-new-bus-unsubscribe@..."

[M. Warner Losh]

> > How about creating a new driver_if.m entry point that is the complement to
> > DRIVER_IDENTIFY.  I would call it DRIVER_UNIDENTIFY() and would change
> > bus_generic_detach() to call it on each driver similar to how
> > bus_generic_probe() currently calls DRIVER_IDENTIFY() for each driver.  This
> > would allow drivers that create devices in DRIVER_IDENTIFY() have a place to
> > remove the device when they are unloaded.
>
> I have mixed feelings about this.
>
> First, the identify routine has identified a hunk of hardware and has
> placed it into the tree.  The driver disappearing doesn't change the
> fact that the hardware is still there.  Adding this new function would
> further blur the lines between attached drivers, the hardware and
> nodes in the device tree.  Device nodes are expected to be there
> unattached.  Once the device driver has detached from the device node,
> there's no harm in leaving the node arround as there are no dangling
> references.

Thinking about it, however, it would allow for a two different drivers
to be loaded and fight over the hardware...  but you can do that
already today and adding this won't change that.

> Second, the bus may be the one that decides what hardware is there.  I
> have a SOC chip that has a number of different children of its nexus
> that are always there, and will always be there.  I do not want the
> children drivers to know anything of their location, etc, since they
> cannot know because a different SOC will have the children at a
> different location.  Of course, the easy thing here is to never call
> the idenfify routine at all for this bus, but this would require
> changes to the code.
>
> However, we do currently have a assymetrical arrangement.  There's a
> way to add the device, but too many drivers are 'stupid' in how they
> add the device.  They neglect to check to make sure that the device
> hasn't already been added, which is what causes the grief.
>
> Maybe it would be better to have a better way to add instances such
> that if the idenify routine used this better way that it could be
> called many times (eg, make it idempotent).  Right now we recommend
> that driver writers do the following (which is idempotent):
>
> static void
> tscnmi_identify( driver_t* driver, device_t parent )
> {
>     devclass_t dc;
>
>     dc = devclass_find(DRIVERNAME);
>     if (devclass_get_device(dc, 0) == NULL) {
>         if (BUS_ADD_CHILD(parent, 0, DRIVERNAME, 0) == 0)
>             panic("failed to add " DRIVERNAME);
>     }
> }
>
> I think a better solution would be:
>
> static void
> tscnmi_identify( driver_t* driver, device_t parent )
> {
> device_add_child_once(parent, DRIVERNAME);
> }
>
> where device_add_child_once would look like the following (run through
> style(9)izer):
>
> int
> device_add_child_once(device_t parent, char *name)
> {
>     devclass_t dc;
>
>     dc = devclass_find(name);
>     if (devclass_get_device(dc, 0) == NULL)
>     return BUS_ADD_CHILD(parent, 0, name, 0);
>     return 0;
> }

_______________________________________________
freebsd-new-bus@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-new-bus
To unsubscribe, send any mail to "freebsd-new-bus-unsubscribe@..."

[M. Warner Losh]

From: John Baldwin <jhb@...>
Subject: Re: DRIVER_UNIDENTIFY
Date: Thu, 15 Dec 2005 13:33:05 -0500

> On Thursday 15 December 2005 01:13 pm, Warner Losh wrote:
> > > How about creating a new driver_if.m entry point that is the complement
> > > to DRIVER_IDENTIFY.  I would call it DRIVER_UNIDENTIFY() and would change
> > > bus_generic_detach() to call it on each driver similar to how
> > > bus_generic_probe() currently calls DRIVER_IDENTIFY() for each driver.
> > > This would allow drivers that create devices in DRIVER_IDENTIFY() have a
> > > place to remove the device when they are unloaded.
> >
> > I have mixed feelings about this.
> >
> > First, the identify routine has identified a hunk of hardware and has
> > placed it into the tree.  The driver disappearing doesn't change the
> > fact that the hardware is still there.  Adding this new function would
> > further blur the lines between attached drivers, the hardware and
> > nodes in the device tree.  Device nodes are expected to be there
> > unattached.  Once the device driver has detached from the device node,
> > there's no harm in leaving the node arround as there are no dangling
> > references.
> >
> > Second, the bus may be the one that decides what hardware is there.  I
> > have a SOC chip that has a number of different children of its nexus
> > that are always there, and will always be there.  I do not want the
> > children drivers to know anything of their location, etc, since they
> > cannot know because a different SOC will have the children at a
> > different location.  Of course, the easy thing here is to never call
> > the idenfify routine at all for this bus, but this would require
> > changes to the code.
> >
> > However, we do currently have a assymetrical arrangement.  There's a
> > way to add the device, but too many drivers are 'stupid' in how they
> > add the device.  They neglect to check to make sure that the device
> > hasn't already been added, which is what causes the grief.
> >
> > Maybe it would be better to have a better way to add instances such
> > that if the idenify routine used this better way that it could be
> > called many times (eg, make it idempotent).  Right now we recommend
> > that driver writers do the following (which is idempotent):
> >
> > static void
> > tscnmi_identify( driver_t* driver, device_t parent )
> > {
> >     devclass_t dc;
> >
> >     dc = devclass_find(DRIVERNAME);
> >     if (devclass_get_device(dc, 0) == NULL) {
> >         if (BUS_ADD_CHILD(parent, 0, DRIVERNAME, 0) == 0)
> >             panic("failed to add " DRIVERNAME);
> >     }
> > }
> >
> > I think a better solution would be:
> >
> > static void
> > tscnmi_identify( driver_t* driver, device_t parent )
> > {
> > device_add_child_once(parent, DRIVERNAME);
> > }
> >
> > where device_add_child_once would look like the following (run through
> > style(9)izer):
> >
> > int
> > device_add_child_once(device_t parent, char *name)
> > {
> >     devclass_t dc;
> >
> >     dc = devclass_find(name);
> >     if (devclass_get_device(dc, 0) == NULL)
> >     return BUS_ADD_CHILD(parent, 0, name, 0);
> >     return 0;
> > }
>
> Actually, those methods enforce one instance in the system.  I want to enforce
> one instance per parent device (the example here is acpi_video which attaches
> to vgapci in my agp_cvs.patch and if you have multiple video cards, each
> might have its own acpi_video driver).  I'm about to fix acpi_video, but was
> curious if we could come up with a better overall solution.

I thought about writing once_per_bus too that would search the parent
bus' children for one whose devclass matched...

Warner
_______________________________________________
freebsd-new-bus@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-new-bus
To unsubscribe, send any mail to "freebsd-new-bus-unsubscribe@..."