Re: [PATCH 1/2] ibmebus: dynamic addiiton/removal of adapters, uevent, root device based on struct device
> This adds two sysfs attributes to /sys/bus/ibmebus which can
> be used to notify the ebus driver of added / removed ebus
> devices in the OF device tree.
We are seeing several build errors when attempting to apply this to
2.6.21-rc2:
CC arch/powerpc/kernel/ibmebus.o
arch/powerpc/kernel/ibmebus.c: In function "ibmebus_register_device_common":
arch/powerpc/kernel/ibmebus.c:192: warning: ignoring return value of
"device_create_file", declared with attribute warn_unused_result
arch/powerpc/kernel/ibmebus.c: At top level:
arch/powerpc/kernel/ibmebus.c:373: error: "of_device_uevent" undeclared here
(not in a function)
arch/powerpc/kernel/ibmebus.c: In function "ibmebus_store_probe":
arch/powerpc/kernel/ibmebus.c:399: error: assignment of read-only location
arch/powerpc/kernel/ibmebus.c:401: error: assignment of read-only location
arch/powerpc/kernel/ibmebus.c: In function "ibmebus_store_remove":
arch/powerpc/kernel/ibmebus.c:434: error: assignment of read-only location
arch/powerpc/kernel/ibmebus.c:436: error: assignment of read-only location
arch/powerpc/kernel/ibmebus.c: In function "ibmebus_bus_init":
arch/powerpc/kernel/ibmebus.c:474: warning: ignoring return value of
"bus_create_file", declared with attribute warn_unused_result
arch/powerpc/kernel/ibmebus.c:475: warning: ignoring return value of
"bus_create_file", declared with attribute warn_unused_result
make[1]: *** [arch/powerpc/kernel/ibmebus.o] Error 1
make: *** [arch/powerpc/kernel] Error 2
make: *** Waiting for unfinished jobs
Here is a sample error from the patch referenced above:
+static ssize_t ibmebus_store_probe(struct bus_type *bus,
+ const char *buf, size_t count)
+{
+ struct device_node *dn = NULL;
+ struct ibmebus_dev *dev;
+ char *loc_code;
+
+ buf[count] = '\0';
The "buf" variable is declared const, and then immediately written to.
Thanks-
John
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] ibmebus: dynamic addiiton/removal of adapters, uevent, root device based on struct device
This adds two sysfs attributes to /sys/bus/ibmebus which can
be used to notify the ebus driver of added / removed ebus
devices in the OF device tree.
We are seeing several build errors when attempting to apply this to
2.6.21-rc2:
CC arch/powerpc/kernel/ibmebus.o
arch/powerpc/kernel/ibmebus.c: In function ibmebus_register_device_common:
arch/powerpc/kernel/ibmebus.c:192: warning: ignoring return value of
device_create_file, declared with attribute warn_unused_result
arch/powerpc/kernel/ibmebus.c: At top level:
arch/powerpc/kernel/ibmebus.c:373: error: of_device_uevent undeclared here
(not in a function)
arch/powerpc/kernel/ibmebus.c: In function ibmebus_store_probe:
arch/powerpc/kernel/ibmebus.c:399: error: assignment of read-only location
arch/powerpc/kernel/ibmebus.c:401: error: assignment of read-only location
arch/powerpc/kernel/ibmebus.c: In function ibmebus_store_remove:
arch/powerpc/kernel/ibmebus.c:434: error: assignment of read-only location
arch/powerpc/kernel/ibmebus.c:436: error: assignment of read-only location
arch/powerpc/kernel/ibmebus.c: In function ibmebus_bus_init:
arch/powerpc/kernel/ibmebus.c:474: warning: ignoring return value of
bus_create_file, declared with attribute warn_unused_result
arch/powerpc/kernel/ibmebus.c:475: warning: ignoring return value of
bus_create_file, declared with attribute warn_unused_result
make[1]: *** [arch/powerpc/kernel/ibmebus.o] Error 1
make: *** [arch/powerpc/kernel] Error 2
make: *** Waiting for unfinished jobs
Here is a sample error from the patch referenced above:
+static ssize_t ibmebus_store_probe(struct bus_type *bus,
+ const char *buf, size_t count)
+{
+ struct device_node *dn = NULL;
+ struct ibmebus_dev *dev;
+ char *loc_code;
+
+ buf[count] = '\0';
The buf variable is declared const, and then immediately written to.
Thanks-
John
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] ibmebus: dynamic add/remove, uevent, root device and whitespace
Looks great! Signed-off-by: Joachim Fenkes <[EMAIL PROTECTED]> Acked-by: John Rose <[EMAIL PROTECTED]> - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] ibmebus: dynamic add/remove, uevent, root device and whitespace
Looks great! Signed-off-by: Joachim Fenkes [EMAIL PROTECTED] Acked-by: John Rose [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ehea: dynamic add / remove port
On Wed, 2007-02-21 at 10:06, Jan-Bernd Themann wrote: > This patch introduces functionality to dynamically add / remove > ehea ports via an userspace DLPAR tool. It creates a subnode for > each logical port in the sysfs. Looks great! Signed-off-by: Jan-Bernd Themann <[EMAIL PROTECTED]> Acked-by: John Rose <[EMAIL PROTECTED]> - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ehea: dynamic add / remove port
On Wed, 2007-02-21 at 10:06, Jan-Bernd Themann wrote: This patch introduces functionality to dynamically add / remove ehea ports via an userspace DLPAR tool. It creates a subnode for each logical port in the sysfs. Looks great! Signed-off-by: Jan-Bernd Themann [EMAIL PROTECTED] Acked-by: John Rose [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2.6.21-rc1] ibmebus: Support dynamic addition and removal of adapters
> If the probe operation succeeds, the respective device will show up > beneath > /sys/bus/ibmebus/devices. This approach is not particularly synchronous. Take the case of an add failure: how long would an application wait before deciding that the new device is not going to appear? It might be preferable to have the write to the probe function fail. For example, if we can't find the device node, return -EINVAL or something. > This is already taken care of by of_device_register(). If we are scrapping the use of of_device for ibmebus devices, can we still create a devspec file for OF correlation purposes? Thanks- John - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2.6.21-rc1] ibmebus: Support dynamic addition and removal of adapters
If the probe operation succeeds, the respective device will show up beneath /sys/bus/ibmebus/devices. This approach is not particularly synchronous. Take the case of an add failure: how long would an application wait before deciding that the new device is not going to appear? It might be preferable to have the write to the probe function fail. For example, if we can't find the device node, return -EINVAL or something. This is already taken care of by of_device_register(). If we are scrapping the use of of_device for ibmebus devices, can we still create a devspec file for OF correlation purposes? Thanks- John - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2.6.21-rc1] ehea: dynamic add / remove port
Hi- Sounds good. A couple of questions/comments: > I think it is not necessary to have a special entry/kobject for each logical > port. I suggest we use SET_NETDEV_DEV to create links to all ethernet devices > that represent each a logical port. This should be in sync with all other > ethernet drivers. Port attributes like the "logical port id" that might be > need by the userspace application can be added to the net:ethX entry > (link created with SET_NETDEV_DEV) as additional attributes. As we develop the userspace tool, we are finding a need to know the following for each logical port: 1) the linux interface/device (a symlink would do this) 2) the logical port number - were you suggesting communicating this through the name of the symlink, or in the directory of the symlink target? 3) the open firmware path for the logical port. This is typically communicated through a sysfs attribute file "devspec", but we can't do this without a new kobject for the port. > > > > Second, the probe and remove functions do not communicate whether an add > > or remove was successful. Combine this with the lack of port > > information in the adapter sysfs directory, and the userspace tool has > > no way of verifying a dynamic add/remove. > > True. I suggest we return error codes (-EIO / -EINVAL) in case the adding > or removing of ports fails. This is possible as we know instantly if the > operation failed or not. It is a synchronus operation. Sounds good. Thanks! John - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2.6.21-rc1] ehea: dynamic add / remove port
Hi- Sounds good. A couple of questions/comments: I think it is not necessary to have a special entry/kobject for each logical port. I suggest we use SET_NETDEV_DEV to create links to all ethernet devices that represent each a logical port. This should be in sync with all other ethernet drivers. Port attributes like the logical port id that might be need by the userspace application can be added to the net:ethX entry (link created with SET_NETDEV_DEV) as additional attributes. As we develop the userspace tool, we are finding a need to know the following for each logical port: 1) the linux interface/device (a symlink would do this) 2) the logical port number - were you suggesting communicating this through the name of the symlink, or in the directory of the symlink target? 3) the open firmware path for the logical port. This is typically communicated through a sysfs attribute file devspec, but we can't do this without a new kobject for the port. Second, the probe and remove functions do not communicate whether an add or remove was successful. Combine this with the lack of port information in the adapter sysfs directory, and the userspace tool has no way of verifying a dynamic add/remove. True. I suggest we return error codes (-EIO / -EINVAL) in case the adding or removing of ports fails. This is possible as we know instantly if the operation failed or not. It is a synchronus operation. Sounds good. Thanks! John - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2.6.21-rc1] ibmebus: Support dynamic addition and removal of adapters
Hi-
Looks good. Questions: how can the user space tools verify the success
of an add or remove?
Also, will /sys/bus/ibmebus exist even if the system booted with no LHEA
nodes?
One more comment below.
@@ -161,7 +161,9 @@ static void __devinit ibmebus_dev_releas
static ssize_t ibmebusdev_show_name(struct device *dev,
struct device_attribute *attr, char *buf)
{
- return sprintf(buf, "%s\n", to_ibmebus_dev(dev)->name);
+ struct ibmebus_dev *ebus_dev = to_ibmebus_dev(dev);
+ char *name = (char*)get_property(ebus_dev->ofdev.node, "name", NULL);
+ return sprintf(buf, "%s\n", name);
}
static DEVICE_ATTR(name, S_IRUSR | S_IRGRP | S_IROTH, ibmebusdev_show_name,
NULL);
Can we also have an attribute "devspec" that communicates the open
firmware path through sysfs? User space DLPAR tools need a way to make
this correlation. This is consistent with other dynamic-capable sysfs
objects (cpus, etc). I assume you could get this string with something
like ebus_dev->ofdev.node->full_name. For example:
# cat /sys/bus/ibmebus/$ebus_device/devspec
/[EMAIL PROTECTED]/
Thanks-
John
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2.6.21-rc1] ehea: dynamic add / remove port
> Second, the probe and remove functions do not communicate whether an add > or remove was successful. Combine this with the lack of port > information in the adapter sysfs directory, and the userspace tool has > no way of verifying a dynamic add/remove. One way to communicate a return code is by making the sysfs interface file read/write, and having the read callback give a return code. For an example of this, you can look at drivers/pci/rpadlpar_sysfs.c and rpadlpar_core.c. It would still be nice to have some way from the adapter sysfs directory to list/examine logical ports. Symlinks would work, but it would be even nicer to have a new kobject per port with attribute files for logical id, state, etc. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2.6.21-rc1] ehea: dynamic add / remove port
Second, the probe and remove functions do not communicate whether an add or remove was successful. Combine this with the lack of port information in the adapter sysfs directory, and the userspace tool has no way of verifying a dynamic add/remove. One way to communicate a return code is by making the sysfs interface file read/write, and having the read callback give a return code. For an example of this, you can look at drivers/pci/rpadlpar_sysfs.c and rpadlpar_core.c. It would still be nice to have some way from the adapter sysfs directory to list/examine logical ports. Symlinks would work, but it would be even nicer to have a new kobject per port with attribute files for logical id, state, etc. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2.6.21-rc1] ibmebus: Support dynamic addition and removal of adapters
Hi-
Looks good. Questions: how can the user space tools verify the success
of an add or remove?
Also, will /sys/bus/ibmebus exist even if the system booted with no LHEA
nodes?
One more comment below.
@@ -161,7 +161,9 @@ static void __devinit ibmebus_dev_releas
static ssize_t ibmebusdev_show_name(struct device *dev,
struct device_attribute *attr, char *buf)
{
- return sprintf(buf, %s\n, to_ibmebus_dev(dev)-name);
+ struct ibmebus_dev *ebus_dev = to_ibmebus_dev(dev);
+ char *name = (char*)get_property(ebus_dev-ofdev.node, name, NULL);
+ return sprintf(buf, %s\n, name);
}
static DEVICE_ATTR(name, S_IRUSR | S_IRGRP | S_IROTH, ibmebusdev_show_name,
NULL);
Can we also have an attribute devspec that communicates the open
firmware path through sysfs? User space DLPAR tools need a way to make
this correlation. This is consistent with other dynamic-capable sysfs
objects (cpus, etc). I assume you could get this string with something
like ebus_dev-ofdev.node-full_name. For example:
# cat /sys/bus/ibmebus/$ebus_device/devspec
/[EMAIL PROTECTED]/
Thanks-
John
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2.6.21-rc1] ehea: dynamic add / remove port
Hi-
A few high level comments, then some really insignificant ones.
First, is there a reason why we shouldn't have a sysfs entry/kobject for
each logical port? How is it possible to determine, from the adapter
sysfs directory, the current number of ports for that adapter? A port
sysfs directory could include attributes like the OF path to the port,
the state of the port, etc etc.
Second, the probe and remove functions do not communicate whether an add
or remove was successful. Combine this with the lack of port
information in the adapter sysfs directory, and the userspace tool has
no way of verifying a dynamic add/remove.
+ dn_log_port_id = (u32*)get_property(eth_dn, "ibm,hea-port-no",
+ NULL);
+ if (!dn_log_port_id) {
+ ehea_error("bad device node: dn_log_port_id=%p",
+ dn_log_port_id);
Wouldn't this print NULL every time for dn_log_port_id? Would the OF
path for eth_dn make for a more useful error msg?
+ ehea_info("%s -> logial port id #%d",
Spelling :)
if (port_setup_ok)
- ret = 0;/* At least some ports are setup correctly */
+ return 0; /* At least some ports are setup correctly */
else
- ret = -EINVAL;
+ return -EINVAL;
The else is unnecessary.
static int __devexit ehea_remove(struct ibmebus_dev *dev)
{
struct ehea_adapter *adapter = dev->ofdev.dev.driver_data;
u64 hret;
int i;
- for (i = 0; i < adapter->num_ports; i++)
+ for (i = 0; i < EHEA_MAX_PORTS; i++)
if (adapter->port[i]) {
ehea_shutdown_single_port(adapter->port[i]);
adapter->port[i] = NULL;
}
Else break?
Thanks-
John
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2.6.21-rc1] ehea: dynamic add / remove port
Hi-
A few high level comments, then some really insignificant ones.
First, is there a reason why we shouldn't have a sysfs entry/kobject for
each logical port? How is it possible to determine, from the adapter
sysfs directory, the current number of ports for that adapter? A port
sysfs directory could include attributes like the OF path to the port,
the state of the port, etc etc.
Second, the probe and remove functions do not communicate whether an add
or remove was successful. Combine this with the lack of port
information in the adapter sysfs directory, and the userspace tool has
no way of verifying a dynamic add/remove.
+ dn_log_port_id = (u32*)get_property(eth_dn, ibm,hea-port-no,
+ NULL);
+ if (!dn_log_port_id) {
+ ehea_error(bad device node: dn_log_port_id=%p,
+ dn_log_port_id);
Wouldn't this print NULL every time for dn_log_port_id? Would the OF
path for eth_dn make for a more useful error msg?
+ ehea_info(%s - logial port id #%d,
Spelling :)
if (port_setup_ok)
- ret = 0;/* At least some ports are setup correctly */
+ return 0; /* At least some ports are setup correctly */
else
- ret = -EINVAL;
+ return -EINVAL;
The else is unnecessary.
static int __devexit ehea_remove(struct ibmebus_dev *dev)
{
struct ehea_adapter *adapter = dev-ofdev.dev.driver_data;
u64 hret;
int i;
- for (i = 0; i adapter-num_ports; i++)
+ for (i = 0; i EHEA_MAX_PORTS; i++)
if (adapter-port[i]) {
ehea_shutdown_single_port(adapter-port[i]);
adapter-port[i] = NULL;
}
Else break?
Thanks-
John
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix sparsemem on Cell (take 3)
> I dropped this on the floor over Christmas. This has had a few smoke > tests on ppc64 and i386 and is ready for -mm. Against 2.6.20-rc2-mm1. Could this break ia64, given that it uses memmap_init_zone()? - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix sparsemem on Cell (take 3)
I dropped this on the floor over Christmas. This has had a few smoke tests on ppc64 and i386 and is ready for -mm. Against 2.6.20-rc2-mm1. Could this break ia64, given that it uses memmap_init_zone()? - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 8/8] PCI Error Recovery: PPC64 core recovery routines
Hi Paul- > I'm suggesting that the rpaphp code has a struct pci_driver whose > id_table and probe function are such that it will claim the EADS > bridges. (It would probably be best to match on vendor=IBM and > class=PCI-PCI bridge and let the probe function figure out which of > the bridges it gets asked about are actually EADS bridges.) Wouldn't this leave out hotplug-capable adapters who have direct PHB parents, since these parent PHBs don't have pci_devs? Thoughts? Thanks- John - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 8/8] PCI Error Recovery: PPC64 core recovery routines
Hi Paul- I'm suggesting that the rpaphp code has a struct pci_driver whose id_table and probe function are such that it will claim the EADS bridges. (It would probably be best to match on vendor=IBM and class=PCI-PCI bridge and let the probe function figure out which of the bridges it gets asked about are actually EADS bridges.) Wouldn't this leave out hotplug-capable adapters who have direct PHB parents, since these parent PHBs don't have pci_devs? Thoughts? Thanks- John - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 8/8] PCI Error Recovery: PPC64 core recovery routines
> Not sure that I agree with this. Not all PCI hotplug slots have EADS > devices as parents. Ahem, "PCI hotplug" above should read "EEH-enabled". Sorry :) John - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 8/8] PCI Error Recovery: PPC64 core recovery routines
Hi Paul- > > 2) As a result, the code to call hot-unplug is a bit messy. In > >particular, there's a bit of hoop-jumping when hotplug is built as > >as a module (and said hoops were wrecked recently when I moved the > >code around, out of the rpaphp directory). > > One way to clean this up would be to make rpaphp the driver for the > EADS bridges (from the pci code's point of view). Then it would > automatically get included in the error recovery process and could do > whatever it should. Not sure that I agree with this. Not all PCI hotplug slots have EADS devices as parents. This sort of topography dependency is something we're trying to break in rpaphp. Rpaphp could set this up for devices that do have EADS parents, but then we've only covered a subset of EEH-capable devices. Anyway, isn't the OS forbidden from performing most of the expected function of such a driver for EADS devices: Enable the device Access device configuration space Discover resources (addresses and IRQ numbers) provided by the device Allocate these resources Communicate with the device Disable the device Thanks- John - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 8/8] PCI Error Recovery: PPC64 core recovery routines
Hi Paul- 2) As a result, the code to call hot-unplug is a bit messy. In particular, there's a bit of hoop-jumping when hotplug is built as as a module (and said hoops were wrecked recently when I moved the code around, out of the rpaphp directory). One way to clean this up would be to make rpaphp the driver for the EADS bridges (from the pci code's point of view). Then it would automatically get included in the error recovery process and could do whatever it should. Not sure that I agree with this. Not all PCI hotplug slots have EADS devices as parents. This sort of topography dependency is something we're trying to break in rpaphp. Rpaphp could set this up for devices that do have EADS parents, but then we've only covered a subset of EEH-capable devices. Anyway, isn't the OS forbidden from performing most of the expected function of such a driver for EADS devices: Enable the device Access device configuration space Discover resources (addresses and IRQ numbers) provided by the device Allocate these resources Communicate with the device Disable the device Thanks- John - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 8/8] PCI Error Recovery: PPC64 core recovery routines
Not sure that I agree with this. Not all PCI hotplug slots have EADS devices as parents. Ahem, PCI hotplug above should read EEH-enabled. Sorry :) John - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 8/8] PCI Error Recovery: PPC64 core recovery routines
Hi Linas-
I like the idea of splitting the recovery stuff into its own driver. A
few comments on the last reorg patch:
> Index: linux-2.6.13-rc6-git9/arch/ppc64/kernel/eeh.c
...
> +static int
> +eeh_slot_availability(struct device_node *dn)
...
> +void eeh_restore_bars(struct device_node *dn)
Inconsistent spacing in new code...
> --- /dev/null 1970-01-01 00:00:00.0 +
> +++ linux-2.6.13-rc6-git9/arch/ppc64/kernel/eeh_driver.c 2005-08-23
> 14:34:44.0 -0500
> @@ -0,0 +1,361 @@
> +/*
> + * PCI Hot Plug Controller Driver for RPA-compliant PPC64 platform.
This probably isn't the right header description for this file :)
> +
> +/**
> + * rpaphp_find_slot - find and return the slot holding the device
> + * @dev: pci device for which we want the slot structure.
> + */
> +static struct slot *rpaphp_find_slot(struct pci_dev *dev)
> +{
> + struct list_head *tmp, *n;
> + struct slot *slot;
> +
> + list_for_each_safe(tmp, n, _slot_head) {
> + struct pci_bus *bus;
> +
> + slot = list_entry(tmp, struct slot, rpaphp_slot_list);
> +
> + /* PHB's don't have bridges. */
> + bus = slot->bus;
> + if (bus == NULL)
> + continue;
> +
> + /* The PCI device could be the slot itself. */
> + if (bus->self == dev)
> + return slot;
> +
> + if (pci_search_bus_for_dev (bus, dev))
> + return slot;
> + }
> + return NULL;
> +}
This function breaks if rpaphp is compiled as a module. It's probably
bad for kernel code to depend on symbols exported from modules. This
raises two larger questions: Where should this new driver sit, and
should it be possible to compile it as a module as well?
> +/* --- */
> +/**
> + * handle_eeh_events -- reset a PCI device after hard lockup.
> + *
...
> +int eeh_reset_device (struct pci_dev *dev, struct device_node *dn, int
> reconfig)
Header doesn't match the function :)
> +{
> + struct hotplug_slot *frozen_slot= NULL;
> + struct hotplug_slot_ops *frozen_ops= NULL;
> +
> + if (!dev)
> + return 1;
> +
> + if (reconfig) {
> + frozen_slot = pci_hp_find_slot(dev);
> + if (frozen_slot)
> + frozen_ops = frozen_slot->ops;
> + }
> +
> + if (frozen_ops) frozen_ops->disable_slot (frozen_slot);
> +
> + /* Reset the pci controller. (Asserts RST#; resets config space).
> + * Reconfigure bridges and devices */
> + rtas_set_slot_reset (dn->child);
> +
> + /* Walk over all functions on this device */
> + struct device_node *peer = dn->child;
> + while (peer) {
> + rtas_configure_bridge(peer);
> + eeh_restore_bars(peer);
> + peer = peer->sibling;
> + }
> +
> + /* Give the system 5 seconds to finish running the user-space
> + * hotplug scripts, e.g. ifdown for ethernet. Yes, this is a hack,
> + * but if we don't do this, weird things happen.
> + */
> + if (frozen_ops) {
> + ssleep (5);
> + frozen_ops->enable_slot (frozen_slot);
> + }
> + return 0;
> +}
This dependence on struct hotplug_slot might be problematic as we
restrict the registration of "PCI hotplug" slots to exclude PHBs, VIO,
and embedded slots. Noticed your comment to this effect. I can work
with you offline on this.
> +
> +/* The longest amount of time to wait for a pci device
> + * to come back on line, in seconds.
> + */
> +#define MAX_WAIT_FOR_RECOVERY 15
> +
> +int handle_eeh_events (struct eeh_event *event)
> +{
> +
...
> + frozen_device = pci_bus_to_OF_node(dev->bus);
> + if (!frozen_device)
> + {
> + printk (KERN_ERR "EEH: Cannot find PCI controller for %s\n",
> + pci_name(dev));
> +
> + return 1;
> + }
> + BUG_ON (frozen_device->phb==NULL);
> +
> + /* We get "permanent failure" messages on empty slots.
> + * These are false alarms. Empty slots have no child dn. */
> + if ((event->state == pci_channel_io_perm_failure) && (frozen_device ==
> NULL))
The second part of this conditional will never be true, as this has just
been checked above.
> + if (frozen_device)
> + freeze_count = frozen_device->eeh_freeze_count;
This conditional will always be true, as this has also ben checked
above.
> Index: linux-2.6.13-rc6-git9/include/asm-ppc64/prom.h
> ===
> --- linux-2.6.13-rc6-git9.orig/include/asm-ppc64/prom.h 2005-08-19
> 15:11:39.0 -0500
> +++ linux-2.6.13-rc6-git9/include/asm-ppc64/prom.h2005-08-23
> 13:31:52.0 -0500
> @@ -135,11 +135,17 @@
> int busno; /* for pci devices */
> int bussubno; /* for pci devices */
> int devfn;
Re: [patch 8/8] PCI Error Recovery: PPC64 core recovery routines
Hi Linas-
I like the idea of splitting the recovery stuff into its own driver. A
few comments on the last reorg patch:
Index: linux-2.6.13-rc6-git9/arch/ppc64/kernel/eeh.c
...
+static int
+eeh_slot_availability(struct device_node *dn)
...
+void eeh_restore_bars(struct device_node *dn)
Inconsistent spacing in new code... /nit
--- /dev/null 1970-01-01 00:00:00.0 +
+++ linux-2.6.13-rc6-git9/arch/ppc64/kernel/eeh_driver.c 2005-08-23
14:34:44.0 -0500
@@ -0,0 +1,361 @@
+/*
+ * PCI Hot Plug Controller Driver for RPA-compliant PPC64 platform.
This probably isn't the right header description for this file :)
+
+/**
+ * rpaphp_find_slot - find and return the slot holding the device
+ * @dev: pci device for which we want the slot structure.
+ */
+static struct slot *rpaphp_find_slot(struct pci_dev *dev)
+{
+ struct list_head *tmp, *n;
+ struct slot *slot;
+
+ list_for_each_safe(tmp, n, rpaphp_slot_head) {
+ struct pci_bus *bus;
+
+ slot = list_entry(tmp, struct slot, rpaphp_slot_list);
+
+ /* PHB's don't have bridges. */
+ bus = slot-bus;
+ if (bus == NULL)
+ continue;
+
+ /* The PCI device could be the slot itself. */
+ if (bus-self == dev)
+ return slot;
+
+ if (pci_search_bus_for_dev (bus, dev))
+ return slot;
+ }
+ return NULL;
+}
This function breaks if rpaphp is compiled as a module. It's probably
bad for kernel code to depend on symbols exported from modules. This
raises two larger questions: Where should this new driver sit, and
should it be possible to compile it as a module as well?
+/* --- */
+/**
+ * handle_eeh_events -- reset a PCI device after hard lockup.
+ *
...
+int eeh_reset_device (struct pci_dev *dev, struct device_node *dn, int
reconfig)
Header doesn't match the function :)
+{
+ struct hotplug_slot *frozen_slot= NULL;
+ struct hotplug_slot_ops *frozen_ops= NULL;
+
+ if (!dev)
+ return 1;
+
+ if (reconfig) {
+ frozen_slot = pci_hp_find_slot(dev);
+ if (frozen_slot)
+ frozen_ops = frozen_slot-ops;
+ }
+
+ if (frozen_ops) frozen_ops-disable_slot (frozen_slot);
+
+ /* Reset the pci controller. (Asserts RST#; resets config space).
+ * Reconfigure bridges and devices */
+ rtas_set_slot_reset (dn-child);
+
+ /* Walk over all functions on this device */
+ struct device_node *peer = dn-child;
+ while (peer) {
+ rtas_configure_bridge(peer);
+ eeh_restore_bars(peer);
+ peer = peer-sibling;
+ }
+
+ /* Give the system 5 seconds to finish running the user-space
+ * hotplug scripts, e.g. ifdown for ethernet. Yes, this is a hack,
+ * but if we don't do this, weird things happen.
+ */
+ if (frozen_ops) {
+ ssleep (5);
+ frozen_ops-enable_slot (frozen_slot);
+ }
+ return 0;
+}
This dependence on struct hotplug_slot might be problematic as we
restrict the registration of PCI hotplug slots to exclude PHBs, VIO,
and embedded slots. Noticed your comment to this effect. I can work
with you offline on this.
+
+/* The longest amount of time to wait for a pci device
+ * to come back on line, in seconds.
+ */
+#define MAX_WAIT_FOR_RECOVERY 15
+
+int handle_eeh_events (struct eeh_event *event)
+{
+
...
+ frozen_device = pci_bus_to_OF_node(dev-bus);
+ if (!frozen_device)
+ {
+ printk (KERN_ERR EEH: Cannot find PCI controller for %s\n,
+ pci_name(dev));
+
+ return 1;
+ }
+ BUG_ON (frozen_device-phb==NULL);
+
+ /* We get permanent failure messages on empty slots.
+ * These are false alarms. Empty slots have no child dn. */
+ if ((event-state == pci_channel_io_perm_failure) (frozen_device ==
NULL))
The second part of this conditional will never be true, as this has just
been checked above.
+ if (frozen_device)
+ freeze_count = frozen_device-eeh_freeze_count;
This conditional will always be true, as this has also ben checked
above.
Index: linux-2.6.13-rc6-git9/include/asm-ppc64/prom.h
===
--- linux-2.6.13-rc6-git9.orig/include/asm-ppc64/prom.h 2005-08-19
15:11:39.0 -0500
+++ linux-2.6.13-rc6-git9/include/asm-ppc64/prom.h2005-08-23
13:31:52.0 -0500
@@ -135,11 +135,17 @@
int busno; /* for pci devices */
int bussubno; /* for pci devices */
int devfn; /* for pci devices */
- int eeh_mode; /* See eeh.h for possible EEH_MODEs */
- int
Re: [RFC][PATCH] split PCI probing code [1/9]
I like it. I'm a little hung up on the fact that actual device probing happens in config.c rather than probe.c (if I'm correct). Regardless, this patch set cleans things up nicely. John - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: pci_size() error condition
> It was always effectual for IO where the mask is 0x. Okay, point taken :) So for cases of base == maxbase, why would we ever want to return a nonzero value? What is the intended purpose of the second part of that conditional? - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: pci_size() error condition
It was always effectual for IO where the mask is 0x. Okay, point taken :) So for cases of base == maxbase, why would we ever want to return a nonzero value? What is the intended purpose of the second part of that conditional? - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
pci_size() error condition
Can anyone lend an explanation of the following?
/* base == maxbase can be valid only if the BAR has
already been programmed with all 1s. */
if (base == maxbase && ((base | size) & mask) != mask) {
printk("%s: 2 returning 0\n", __FUNCTION__);
return 0;
}
Before a recent change, mask was a 64-bit number. The second part of
the if statement would always resolve to true, since the 32-bit bitop
would never equal the 64-bit mask. So the second part of the if
statement was ineffectual up until very recently.
After the recent change, this is no longer the case. Nonzero PCI sizes
are generating bogus resource records for some PPC64 devices. So for
cases of base == maxbase, why would we ever want to return a nonzero
value? What is the intended purpose of the second part of that
conditional?
Thanks-
John
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
pci_size() error condition
Can anyone lend an explanation of the following?
/* base == maxbase can be valid only if the BAR has
already been programmed with all 1s. */
if (base == maxbase ((base | size) mask) != mask) {
printk(%s: 2 returning 0\n, __FUNCTION__);
return 0;
}
Before a recent change, mask was a 64-bit number. The second part of
the if statement would always resolve to true, since the 32-bit bitop
would never equal the 64-bit mask. So the second part of the if
statement was ineffectual up until very recently.
After the recent change, this is no longer the case. Nonzero PCI sizes
are generating bogus resource records for some PPC64 devices. So for
cases of base == maxbase, why would we ever want to return a nonzero
value? What is the intended purpose of the second part of that
conditional?
Thanks-
John
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] PCI Hotplug: remove incorrect rpaphp firmware dependency
> Er, use the result of the get_children_props() call only if it _failed_?
> I suspect that wasn't your intention. This makes my G5 boot again:
Here's an alternate fix for the ppc64 crash during boot. This corrects
the offending function to use more conventional error codes. I'll
follow up with return code cleanups for the entire module, and for RTAS
code, since these are probably too big for 2.6.11.
Please apply, if appropriate.
Thanks-
John
Signed-off-by: John Rose <[EMAIL PROTECTED]>
diff -puN drivers/pci/hotplug/rpaphp_core.c~01_rpaphp_is_php_fix
drivers/pci/hotplug/rpaphp_core.c
--- 2_6_linus/drivers/pci/hotplug/rpaphp_core.c~01_rpaphp_is_php_fix
2005-02-07 18:06:29.0 -0600
+++ 2_6_linus-johnrose/drivers/pci/hotplug/rpaphp_core.c2005-02-07
18:10:15.0 -0600
@@ -224,7 +224,7 @@ static int get_children_props(struct dev
if (!indexes || !names || !types || !domains) {
/* Slot does not have dynamically-removable children */
- return 1;
+ return -EINVAL;
}
if (drc_indexes)
*drc_indexes = indexes;
@@ -260,7 +260,7 @@ int rpaphp_get_drc_props(struct device_n
}
rc = get_children_props(dn->parent, , , , );
- if (rc) {
+ if (rc < 0) {
return 1;
}
@@ -307,7 +307,7 @@ static int is_php_dn(struct device_node
int rc;
rc = get_children_props(dn, indexes, names, _types, power_domains);
- if (rc) {
+ if (rc >= 0) {
if (is_php_type((char *) _types[1])) {
*types = drc_types;
return 1;
@@ -331,7 +331,7 @@ static int is_dr_dn(struct device_node *
rc = get_children_props(dn->parent, indexes, names, types,
power_domains);
- return (rc == 0);
+ return (rc >= 0);
}
static inline int is_vdevice_root(struct device_node *dn)
_
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] PCI Hotplug: remove incorrect rpaphp firmware dependency
Could we please get David's fix in for 2.6.11, since it's apparently affecting boot in some situations? Thanks- John On Mon, 2005-02-07 at 11:41, John Rose wrote: > > Er, use the result of the get_children_props() call only if it _failed_? > > I suspect that wasn't your intention. This makes my G5 boot again: > > Doh, good catch! This was an oversight while patching multiple trees > for this bug. Previous versions of that function use 1 for success. > Sigh. BTW, you're running an RPA module on your G5? > > Thanks- > John - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] PCI Hotplug: remove incorrect rpaphp firmware dependency
Could we please get David's fix in for 2.6.11, since it's apparently affecting boot in some situations? Thanks- John On Mon, 2005-02-07 at 11:41, John Rose wrote: Er, use the result of the get_children_props() call only if it _failed_? I suspect that wasn't your intention. This makes my G5 boot again: Doh, good catch! This was an oversight while patching multiple trees for this bug. Previous versions of that function use 1 for success. Sigh. BTW, you're running an RPA module on your G5? Thanks- John - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] PCI Hotplug: remove incorrect rpaphp firmware dependency
Er, use the result of the get_children_props() call only if it _failed_?
I suspect that wasn't your intention. This makes my G5 boot again:
Here's an alternate fix for the ppc64 crash during boot. This corrects
the offending function to use more conventional error codes. I'll
follow up with return code cleanups for the entire module, and for RTAS
code, since these are probably too big for 2.6.11.
Please apply, if appropriate.
Thanks-
John
Signed-off-by: John Rose [EMAIL PROTECTED]
diff -puN drivers/pci/hotplug/rpaphp_core.c~01_rpaphp_is_php_fix
drivers/pci/hotplug/rpaphp_core.c
--- 2_6_linus/drivers/pci/hotplug/rpaphp_core.c~01_rpaphp_is_php_fix
2005-02-07 18:06:29.0 -0600
+++ 2_6_linus-johnrose/drivers/pci/hotplug/rpaphp_core.c2005-02-07
18:10:15.0 -0600
@@ -224,7 +224,7 @@ static int get_children_props(struct dev
if (!indexes || !names || !types || !domains) {
/* Slot does not have dynamically-removable children */
- return 1;
+ return -EINVAL;
}
if (drc_indexes)
*drc_indexes = indexes;
@@ -260,7 +260,7 @@ int rpaphp_get_drc_props(struct device_n
}
rc = get_children_props(dn-parent, indexes, names, types, domains);
- if (rc) {
+ if (rc 0) {
return 1;
}
@@ -307,7 +307,7 @@ static int is_php_dn(struct device_node
int rc;
rc = get_children_props(dn, indexes, names, drc_types, power_domains);
- if (rc) {
+ if (rc = 0) {
if (is_php_type((char *) drc_types[1])) {
*types = drc_types;
return 1;
@@ -331,7 +331,7 @@ static int is_dr_dn(struct device_node *
rc = get_children_props(dn-parent, indexes, names, types,
power_domains);
- return (rc == 0);
+ return (rc = 0);
}
static inline int is_vdevice_root(struct device_node *dn)
_
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
