Re: introduce device_is_attached()
On 04/16/12 19:41, Christoph Egger wrote: On 16.04.12 19:12, Matt Thomas wrote: On Apr 16, 2012, at 9:52 AM, Christoph Egger wrote: Hi, I want to introduce a new function to sys/devices.h: bool device_is_attached(device_t parent, cfdata_t cf); I'd prefer device_is_attached_p Ok, I will rename it. The purpose is for bus drivers who wants to attach children and ensure that only one instance of it will attach. 'parent' is the bus driver and 'cf' is the child device as passed to the submatch callback via config_search_loc(). The return value is true if the child is already attached. Can it be used in driver match routines so they don't need to keep a local to prevent multiple matches? Oh, good point. I think this is possible and will make struct amdnb_misc_softc completely superflous. I will give this a try tomorrow. Ok, I can confirm it is also usable in the child driver match routines. However, the local is still needed for the rescan hook. Christoph
Re: introduce device_is_attached()
On Mon, Apr 16, 2012 at 07:49:28PM +0100, Iain Hibbert wrote: I'm kind of with David Young, surely this is what the softc is for.. so that the parent can keep track of which resources it has allocated (and by inference, not reallocate them to another device) I agree with this too; numerous drivers/frameworks use the above scheme. If you add this function, please refactor these to follow this new (superfluous) idiom. - Jukka.
Re: introduce device_is_attached()
On 04/16/12 19:37, David Young wrote:
On Mon, Apr 16, 2012 at 06:52:28PM +0200, Christoph Egger wrote:
Hi,
I want to introduce a new function to sys/devices.h:
bool device_is_attached(device_t parent, cfdata_t cf);
The purpose is for bus drivers who wants to attach children
and ensure that only one instance of it will attach.
'parent' is the bus driver and 'cf' is the child device
as passed to the submatch callback via config_search_loc().
The return value is true if the child is already attached.
I implemented a reference usage of it in amdnb_misc.c to ensure
that amdtemp only attaches once on rescan.
Don't add that function. Just use a small amdnb_misc softc to track
whether or not the amdtemp is attached or not:
struct amdnb_misc_softc {
device_t sc_amdtemp;
};
This is what pchb(4) did before the revert. It doesn't meet the needs
of attaching multiple drivers to the same pci device.
Don't pass a pci_attach_args to amdtemp. Just pass it the chipset tag
and PCI tag if that is all that it needs.
Why is pci_attach_args a no-go ?
I'm not sure I fully understand the purpose of amdnb_miscbus.
Are all of the functions that do/will attach at amdnb_miscbus
configuration-space only functions, or are they something else? Please
explain what amdnb_miscbus is for.
Drivers attaching to amdnb_miscbus are all pci drivers which use
different capabilities of the northbridge device.
Their match/attach routines need the same 'aux' parameter as it
is passed to amdnb_misc(4).
amdtemp(4) uses some PCI registers of the northbridge device to read
the cpu temperature.
I have a local driver which uses a different feature of the same
northbridge device.
To access the device I need the same chipset tag and pci tag.
Instead of making amdtemp(4) a mess I chose to implement different
features in different small drivers.
This also simplifies handling of erratas: If a feature doesn't work
then just don't let match/attach the corresponding driver.
Christoph
Re: introduce device_is_attached()
On Tue, 17 Apr 2012, Christoph Egger wrote: On 04/16/12 19:37, David Young wrote: I'm not sure I fully understand the purpose of amdnb_miscbus. Are all of the functions that do/will attach at amdnb_miscbus configuration-space only functions, or are they something else? Please explain what amdnb_miscbus is for. Drivers attaching to amdnb_miscbus are all pci drivers which use different capabilities of the northbridge device. Their match/attach routines need the same 'aux' parameter as it is passed to amdnb_misc(4). amdtemp(4) uses some PCI registers of the northbridge device to read the cpu temperature. I have a local driver which uses a different feature of the same northbridge device. To access the device I need the same chipset tag and pci tag. Instead of making amdtemp(4) a mess I chose to implement different features in different small drivers. This also simplifies handling of erratas: If a feature doesn't work then just don't let match/attach the corresponding driver. Oh that problem. It's been what, ten years now and the design of our PCI attachment framework is still a giant pain that needs to be worked around. I first suffered from this when I started working on the PCI code for SPARCs. They run OFW, which should be used to probe and enumerate the PCI buses. All the important information about device enumeration is encoded as properties on the OFW device tree, and that is what should be used to identify and attach drivers instead of probing the PCI config space registers directly. Important interrupt routing information is encoded in the device tree which is not available from the PCI registers, so it is necessary to correlate OFW device nodes with PCI driver instances to make the machine work properly. Unfortunately, our PCI framework wants to take over all aspects of device identification and probing and it's very difficult for the PCI bus driver to manipulate attachment of or provide extra information to child devices. I wanted to go and rewrite all that stuff so it worked properly, but instead we ended up using all sorts of nasty mechanisms to pass information around behind the PCI framework's back. As far as I'm concerned, device_is_attached*() is a hack to work around the inadequacies of certain bus frameworks. Someone should give the PCI code an overhaul to allow the parent device to identify the contents of individual PCI slots control attachment of drivers there, similar to the way the SBus framwork operates. That way you could keep track of each driver that attaches. Eduardo
introduce device_is_attached()
Hi,
I want to introduce a new function to sys/devices.h:
bool device_is_attached(device_t parent, cfdata_t cf);
The purpose is for bus drivers who wants to attach children
and ensure that only one instance of it will attach.
'parent' is the bus driver and 'cf' is the child device
as passed to the submatch callback via config_search_loc().
The return value is true if the child is already attached.
I implemented a reference usage of it in amdnb_misc.c to ensure
that amdtemp only attaches once on rescan.
Any comments to the patch?
Christoph
Index: sys/sys/device.h
===
RCS file: /cvsroot/src/sys/sys/device.h,v
retrieving revision 1.140
diff -u -p -r1.140 device.h
--- sys/sys/device.h13 Nov 2011 22:05:58 - 1.140
+++ sys/sys/device.h16 Apr 2012 16:38:11 -
@@ -528,6 +528,9 @@ voiddevice_active_deregister(device_t,
bool device_is_a(device_t, const char *);
+bool device_is_attached(device_t, cfdata_t);
+
+
device_t device_find_by_xname(const char *);
device_t device_find_by_driver_unit(const char *, int);
Index: sys/kern/subr_device.c
===
RCS file: /cvsroot/src/sys/kern/subr_device.c,v
retrieving revision 1.2
diff -u -p -r1.2 subr_device.c
--- sys/kern/subr_device.c 31 Jan 2010 15:10:12 - 1.2
+++ sys/kern/subr_device.c 16 Apr 2012 16:38:11 -
@@ -176,3 +176,32 @@ device_is_a(device_t dev, const char *dn
return strcmp(dev-dv_cfdriver-cd_name, dname) == 0;
}
+
+/*
+ * device_is_attached:
+ *
+ * Returns true if a driver already attached to a device.
+ * Useful for bus drivers to figure if driver already attached
+ * to prevent drivers attaching twice.
+ */
+bool
+device_is_attached(device_t parent, cfdata_t cf)
+{
+ device_t dev;
+ deviter_t di;
+ bool attached = false;
+
+ for (dev = deviter_first(di, DEVITER_F_LEAVES_FIRST); dev != NULL;
+ dev = deviter_next(di))
+ {
+ if (device_parent(dev) != parent)
+ continue;
+ if (device_is_a(dev, cf-cf_name)) {
+ attached = true;
+ break;
+ }
+ }
+ deviter_release(di);
+
+ return attached;
+}
Index: sys/arch/x86/pci/amdnb_misc.c
===
RCS file: /cvsroot/src/sys/arch/x86/pci/amdnb_misc.c,v
retrieving revision 1.2
diff -u -p -r1.2 amdnb_misc.c
--- sys/arch/x86/pci/amdnb_misc.c 16 Apr 2012 16:07:24 - 1.2
+++ sys/arch/x86/pci/amdnb_misc.c 16 Apr 2012 16:38:11 -
@@ -78,38 +78,13 @@ amdnb_misc_match(device_t parent, cfdata
static int
amdnb_misc_search(device_t parent, cfdata_t cf, const int *locs, void *aux)
{
- device_t dev;
- deviter_t di;
- bool attach;
-
if (!config_match(parent, cf, aux))
return 0;
- attach = true;
-
/* Figure out if found child 'cf' is already attached.
* No need to attach it twice.
*/
-
- /* XXX: I only want to iterate over the children of *this* device.
-* Can we introduce a
-* deviter_first_child(di, parent, DEVITER_F_LEAVES_ONLY)
-* or even better, can we introduce a query function that returns
-* if a child is already attached?
-*/
- for (dev = deviter_first(di, DEVITER_F_LEAVES_FIRST); dev != NULL;
- dev = deviter_next(di))
- {
- if (device_parent(dev) != parent)
- continue;
- if (device_is_a(dev, cf-cf_name)) {
- attach = false;
- break;
- }
- }
- deviter_release(di);
-
- if (!attach)
+ if (device_is_attached(parent, cf))
return 0;
config_attach_loc(parent, cf, locs, aux, NULL);
Re: introduce device_is_attached()
On Apr 16, 2012, at 9:52 AM, Christoph Egger wrote: Hi, I want to introduce a new function to sys/devices.h: bool device_is_attached(device_t parent, cfdata_t cf); I'd prefer device_is_attached_p The purpose is for bus drivers who wants to attach children and ensure that only one instance of it will attach. 'parent' is the bus driver and 'cf' is the child device as passed to the submatch callback via config_search_loc(). The return value is true if the child is already attached. Can it be used in driver match routines so they don't need to keep a local to prevent multiple matches?
Re: introduce device_is_attached()
Along these same lines, it might be nice if there were a way to determine if _any_ instances of a device were attached. This is particularly useful for device-driver modules, which should prevent themselves from being unloaded if there are active instances. A quick check of some device-driver modules show that they simply call config_fini_component() which updates autoconfig tables, but doesn't seem to check if a driver instance exists... On Mon, 16 Apr 2012, Christoph Egger wrote: Hi, I want to introduce a new function to sys/devices.h: bool device_is_attached(device_t parent, cfdata_t cf); The purpose is for bus drivers who wants to attach children and ensure that only one instance of it will attach. 'parent' is the bus driver and 'cf' is the child device as passed to the submatch callback via config_search_loc(). The return value is true if the child is already attached. I implemented a reference usage of it in amdnb_misc.c to ensure that amdtemp only attaches once on rescan. Any comments to the patch? Christoph !DSPAM:4f8c4e7e1986566716057! - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | Customer Service | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at juniper.net | | Kernel Developer | | pgoyette at netbsd.org | -
Re: introduce device_is_attached()
On Mon, Apr 16, 2012 at 06:52:28PM +0200, Christoph Egger wrote:
Hi,
I want to introduce a new function to sys/devices.h:
bool device_is_attached(device_t parent, cfdata_t cf);
The purpose is for bus drivers who wants to attach children
and ensure that only one instance of it will attach.
'parent' is the bus driver and 'cf' is the child device
as passed to the submatch callback via config_search_loc().
The return value is true if the child is already attached.
I implemented a reference usage of it in amdnb_misc.c to ensure
that amdtemp only attaches once on rescan.
Don't add that function. Just use a small amdnb_misc softc to track
whether or not the amdtemp is attached or not:
struct amdnb_misc_softc {
device_t sc_amdtemp;
};
Don't pass a pci_attach_args to amdtemp. Just pass it the chipset tag
and PCI tag if that is all that it needs.
I'm not sure I fully understand the purpose of amdnb_miscbus.
Are all of the functions that do/will attach at amdnb_miscbus
configuration-space only functions, or are they something else? Please
explain what amdnb_miscbus is for.
Dave
--
David Young
dyo...@pobox.comUrbana, IL(217) 721-9981
Re: introduce device_is_attached()
On 16.04.12 19:12, Matt Thomas wrote: On Apr 16, 2012, at 9:52 AM, Christoph Egger wrote: Hi, I want to introduce a new function to sys/devices.h: bool device_is_attached(device_t parent, cfdata_t cf); I'd prefer device_is_attached_p Ok, I will rename it. The purpose is for bus drivers who wants to attach children and ensure that only one instance of it will attach. 'parent' is the bus driver and 'cf' is the child device as passed to the submatch callback via config_search_loc(). The return value is true if the child is already attached. Can it be used in driver match routines so they don't need to keep a local to prevent multiple matches? Oh, good point. I think this is possible and will make struct amdnb_misc_softc completely superflous. I will give this a try tomorrow. Christoph
Re: introduce device_is_attached()
On Mon, 16 Apr 2012, Christoph Egger wrote: On 16.04.12 19:12, Matt Thomas wrote: On Apr 16, 2012, at 9:52 AM, Christoph Egger wrote: Hi, I want to introduce a new function to sys/devices.h: bool device_is_attached(device_t parent, cfdata_t cf); I'd prefer device_is_attached_p Ok, I will rename it. The purpose is for bus drivers who wants to attach children and ensure that only one instance of it will attach. 'parent' is the bus driver and 'cf' is the child device as passed to the submatch callback via config_search_loc(). The return value is true if the child is already attached. Can it be used in driver match routines so they don't need to keep a local to prevent multiple matches? Oh, good point. I think this is possible and will make struct amdnb_misc_softc completely superflous. I will give this a try tomorrow. I'm kind of with David Young, surely this is what the softc is for.. so that the parent can keep track of which resources it has allocated (and by inference, not reallocate them to another device) but you could also use the device_properties to store such information? iain
re: introduce device_is_attached()
I want to introduce a new function to sys/devices.h:
bool device_is_attached(device_t parent, cfdata_t cf);
this doesn't seem to match what the function actually does, which
is answer the question does this parent bus have a child attached
with the same driver name as in cf-cf_name.
that doesn't handle the more interesting cases i can think of such
as does this pci bus have something at device 4 function 1, which
is a question i'd expect device_is_attached{,_p}() to answer. ie,
it is limited to fake busses AFAICT.
i don't think we should adopt this function in sys/device.h.
.mrg.
