2.5.45 cpqphp driver patch w/ intcphp driver enhancements

Thanks for the patch. I'm a bit busy today, but should be able to test this out later tomorrow if all goes well, sorry in advance for the delay.

That being said, I have a minor nit:

-config HOTPLUG_PCI_COMPAQ
- tristate "Compaq PCI Hotplug driver"
- depends on HOTPLUG_PCI && X86
+config HOTPLUG_PCI_COMPAQ_INTEL
+ tristate "Compaq/Intel PCI Hotplug driver"
+ depends on HOTPLUG_PCI

The device is _still_ a Compaq device, Intel just licensed it from them. Changing the name to an Intel device does Compaq a disservice, and in the past, when I accidentally did this, they complained loudly.

So I'd stick to the Compaq name only :)

I'll get back to you with actual technical comments on the patch hopefully by tomorrow evening.

thanks,

greg k-h

Ok, nice job, this is some good progress. Here are some specific issues I have with the patch (note, I didn't run the code, only read it and built it.)

diff -urN linux-2.5.45-ia64-021031-phpa/drivers/hotplug.org/Kconfig linux-2.5.45-ia64-021031-phpa/drivers/hotplug/Kconfig
@@ -21,12 +21,12 @@
 
 	  When in doubt, say N.
 
-config HOTPLUG_PCI_COMPAQ
-	tristate "Compaq PCI Hotplug driver"
-	depends on HOTPLUG_PCI && X86
+config HOTPLUG_PCI_COMPAQ_INTEL
+	tristate "Compaq/Intel PCI Hotplug driver"
+	depends on HOTPLUG_PCI
 
 

I've already given my comment here. Leave the name as Compaq, but put the Intel name in the help text for those users who have those kinds of systems.

+ifdef CONFIG_HOTPLUG_PCI_COMPAQ_INTEL
+ ifeq ($(CONFIG_HOTPLUG_PCI_COMPAQ_INTEL_PHPRM_LEGACY),y)
+ cpqphp-objs += phprm_legacy.o
+ else
+ cpqphp-objs += phprm_acpi.o
+ EXTRA_CFLAGS += -D_LINUX -I$(TOPDIR)/drivers/acpi -I$(TOPDIR)/drivers/acpi/include -DPHP_ACPI
+ endif
+endif
+

You don't need the first ifdef here.

diff -urN linux-2.5.45-ia64-021031-phpa/drivers/hotplug.org/cpqphp.h linux-2.5.45-ia64-021031-phpa/drivers/hotplug/cpqphp.h
@@ -4,6 +4,9 @@
  * Copyright (c) 1995,2001 Compaq Computer Corporation
  * Copyright (c) 2001 Greg Kroah-Hartman ([email protected])
  * Copyright (c) 2001 IBM
+ * Copyright (C) 2001-2002 Intel
+ *  Copyright (C) Fred Lewis ([email protected])
+ *  Copyright (c) J.I. Lee ([email protected])
 

Keep the spacing the same :)

* Send feedback to <[email protected]>
*

+ *  Updates
+ *  -------
+ *  01/05/01    F. Lewis    Modified and/or moved data structures and data
+ *                          types so that hardware specific access code could
+ *                          be configured as a separate loadable driver.
+ *                          Changed filename.  Removed Compaq specific
+ *                          references.  Other minor changes.
+ *  06/07/02    J.I. Lee    Major revision for PHP Resource management
+ *                          Now common for ia32/ia64, ACPI/legacy.
+ *                          Moved/changed all IA32/Legacy codes to phprm_lagacy.
+ *  10/05/02    J.I. Lee    Major revision for PHP HPC
+ *  11/05/02    J.I. Lee    Integration to x86 cpqphp driver

This driver doesn't need a list of Updates, as these are only done by your group, not anyone else. Please delete this, thanks.

struct pci_resource {
struct pci_resource * next;
- u32 base;
- u32 length;
+ ulong base;
+ ulong length;
};

No, don't use "ulong", here, or anywhere else in the kernel, use the u32 and friends types only.

+ // J.I. TBI: Hide this HPC specifics into hpc_ctlr_handle and
+ // reformat it to SHPC

Try not to use C++ style comments in the code, thanks.

+struct hpc_ops {
+ int (*power_on_slot ) (struct slot *slot);
+ int (*power_off_slot ) (struct slot *slot);
+ void (*set_attention_status) (struct slot *slot, u8 status);
+ int (*get_power_status) (struct slot *slot, u8 *status);
+ int (*get_attention_status) (struct slot *slot, u8 *status);
+ int (*get_latch_status) (struct slot *slot, u8 *status);
+ int (*get_adapter_status) (struct slot *slot, u8 *status);
+ int (*get_max_bus_speed) (struct slot *slot, enum pci_bus_speed *value);
+ int (*get_cur_bus_speed) (struct slot *slot, enum pci_bus_speed *value);
+ int (*get_adapter_speed) (struct slot *slot, enum pci_bus_speed *value);
+ u32 (*get_slot_capabilities)(struct slot *slot);
+ u32 (*get_slot_status) (struct slot *slot);
+ int (*get_power_fault_status)(struct slot *slot);
+ void (*set_led_state) (struct slot * slot, struct php_led_id_state *lis, ...);
+ void (*set_slot_on) (struct slot * slot);
+ void (*set_slot_off) (struct slot * slot);
+ void (*set_slot_serr_off) (struct slot * slot);

- hp_slot = slot->device - ctrl->slot_device_offset;
+ void (*enable_msl_interrupts)(struct slot *slot);
+ void (*acquire_lock) (struct slot *slot);
+ void (*release_lock) (struct slot *slot);
+ void (*release_ctlr) (struct controller *ctrl);

- return read_amber_LED (ctrl, hp_slot);
-}
+ int (*chk_bus_freq) (struct controller *ctrl, u8 slot);
+ int (*is_64bit) (struct controller *ctrl, u8 slot);
+ int (*PCIX_capable) (struct controller *ctrl, u8 slot);
+};

This is a nice idea, but is it really necessary to have all of these functions in the structure? And why the varargs on set_led_state()?

+# ifndef __IN_HPC__
+#define phphpc_power_on_slot(slot) \
+((slot)->hpc_ops->power_on_slot(slot))

Ok, this is just nasty. Don't have functions be one thing if compiled into one file, and another in a different file. That's just _too_ complicated. If you want to use the structure's hpc_ops function, say that is what you are really doing.

+#ifdef DEBUG

+#define _DBG_K_TRACE_ENTRY      ((unsigned int)0x00000001)	/* On function entry */
+#define _DBG_K_TRACE_EXIT       ((unsigned int)0x00000002)	/* On function exit */
+#define _DBG_K_INFO             ((unsigned int)0x00000004)	/* Info messages */
+#define _DBG_K_ERROR            ((unsigned int)0x00000008)	/* Error messages */
+#define _DBG_K_TRACE            (_DBG_K_TRACE_ENTRY|_DBG_K_TRACE_EXIT)
+#define _DBG_K_STANDARD         (_DBG_K_INFO|_DBG_K_ERROR|_DBG_K_TRACE)

+/* Redefine this flagword to set debug level */
+#define _DEBUG_LEVEL _DBG_K_STANDARD
+
+#define _DEFINE_DBG_BUFFER \
+char __dbg_str_buf[256];
+
+#define _DBG_PRINT( dbg_flags, args... ) \

+	if ( _DEBUG_LEVEL & ( dbg_flags ) )              \
+	{                                                \
+	    int len;                                     \

+ len = sprintf( __dbg_str_buf, "%s:%d: %s: ", \
+ __FILE__, __LINE__, __FUNCTION__ ); \
+ sprintf( __dbg_str_buf + len, args ); \
+ printk( KERN_NOTICE "%s\n", __dbg_str_buf ); \
+ }
+
+#define _DBG_ENTER_ROUTINE _DBG_PRINT (_DBG_K_TRACE_ENTRY, "%s", "[Entry]");
+#define _DBG_LEAVE_ROUTINE _DBG_PRINT (_DBG_K_TRACE_EXIT, "%s", "[Exit]");
+#define _DBG_PRINT_ERROR( args... ) _DBG_PRINT (_DBG_K_ERROR, args);
+#define _DBG_PRINT_INFO( args... ) _DBG_PRINT (_DBG_K_INFO, args);
+#else
+#define _DEFINE_DBG_BUFFER
+#define _DBG_PRINT( dbg_flags, args... )
+#define _DBG_ENTER_ROUTINE
+#define _DBG_LEAVE_ROUTINE
+#define _DBG_PRINT_ERROR( args... )
+#define _DBG_PRINT_INFO( args... )
+#endif /* DEBUG */

Wow, that's a lot of debug macros that probably are not needed anymore, right? :)

Also, add a do {} while 0; to your _DBG_PRINT macro if you really want it to stay around.

Also, any reason for using "_" at the start of these macros?

+struct sema_struct {
+ struct semaphore crit_sect;
+ int owner_id;
+};

Ok, this is crazy. The only reason you have a owner_id is to prevent yourself from trying to grab a lock twice. Instead you should know what you are doing and not try to do that. In short, do not do this.

You do this in the following function:

+static void _phphpc_acquire_lock(struct php_ctlr_state_s *pPhpCtlr) +{
+ int self;
+
+ self = current->pid;
+
+ if ((pPhpCtlr->hwlock).owner_id = self)
+ return;
+
+ down(&((pPhpCtlr->hwlock).crit_sect));
+ (pPhpCtlr->hwlock).owner_id = self;
+
+ return;
+}

+#define TIGER_PLATFORM /* Uncomment for Tiger platform (870 chipset) */

Make this a config item if you really want to. Otherwise get rid of it, or at least fix the comment :)

+_DEFINE_DBG_BUFFER /* Debug string buffer for entire HPC defined here */

Why here? What was wrong with a few lines above this?

+#ifdef CONFIG_IA64
+static wait_queue_head_t delay_wait_q_head;
+
+/* delay in jiffies to wait */
+static void longdelay(int delay)
+{
+ init_waitqueue_head(&delay_wait_q_head);
+
+ interruptible_sleep_on_timeout(&delay_wait_q_head, delay);
+}
+#endif /* CONFIG_IA64 */

Why does ia64 need an extra delay? And if it _really_ needs it, please provide a non-ia64 version of the function so you don't need the #ifdef later on in the code where you call this function.

+ struct php_ctlr_state_s *pPhpCtlr = (struct php_ctlr_state_s *) ctrl->hpc_ctlr_handle;

Ugh, please use a Linux variable naming scheme, instead of this.

+ _DBG_ENTER_ROUTINE
+
+ if (!ctrl->hpc_ctlr_handle) {
+ _DBG_PRINT_ERROR("Process %d: Invalid HPC Controller handle!", current->pid);
+ return -1;
+ }
+
+ _phphpc_acquire_lock(ctrl->hpc_ctlr_handle);

So the "global" phphpc_acquire_lock() function isn't to be called here? Why not? This is just causing another level of indirection that isn't needed (not to mention that this doesn't need to be a function call, but just grab the lock instead.)

+ /* turn on board without attaching to the bus */
+ pPhpCtlr->creg->slot_power |= (0x01 << slot);

Do NOT write directly to memory. You do this a LOT. Use the proper functions to do this instead.

The functions you deleted do this correctly :)

+static void
+phphpc_set_led_state(struct slot * slot, struct php_led_id_state * led_id_state, ...) +{
+ u16 word_register;
+ struct php_ctlr_state_s *pPhpCtlr = (struct php_ctlr_state_s *) slot->ctrl->hpc_ctlr_handle;
+ unsigned int shift = 0, bits = 0, mask;
+ struct php_led_id_state *led_status = led_id_state;
+ int DONE = 0;

Ah, you want to make sure we are done? Why all caps here?

+ va_list args;
+ va_start(args, led_id_state);

varargs within the kernel for setting a LED? Isn't this a bit overkill? Can't you just make the individual LED calls instead?

+ return (1);

Where is this return() funtion defined? :)
(hint, don't pass a paramater to return, it's not needed, and isn't the kernel style of programming.)

+void phphpc_get_ctlr_dev_id(struct pci_device_id *hpc_dev_id) +{
+#ifdef CONFIG_IA64
+ struct pci_dev *pdev;
+
+ hpc_dev_id->vendor = PCI_VENDOR_ID_INTEL;
+ pdev = pci_find_subsys(PCI_VENDOR_ID_INTEL, PCI_INTC_P64H2_DID, PCI_ANY_ID, PCI_ANY_ID, NULL);
+ if (pdev != NULL) {
+ hpc_did = PCI_INTC_P64H2_DID;
+ hpc_dev_id->device = PCI_INTC_P64H2_DID;
+ } else {
+ pdev = pci_find_subsys(PCI_VENDOR_ID_INTEL, PCI_INTC_WXB_DID, PCI_ANY_ID, PCI_ANY_ID, NULL);
+ if (pdev != NULL) {
+ hpc_did = PCI_INTC_WXB_DID;
+ hpc_dev_id->device = PCI_INTC_WXB_DID;
+ } else
+ hpc_dev_id->device = 0; // should cause a failing return code
+ // from pci_module_init()
+ }
+#else
+ hpc_dev_id->vendor = PCI_VENDOR_ID_COMPAQ;
+ hpc_dev_id->device = PCI_HPC_ID;
+#endif /* CONFIG_IA64 */
+
+ hpc_dev_id->subvendor = PCI_ANY_ID;
+ hpc_dev_id->subdevice = PCI_ANY_ID;
+ hpc_dev_id->class = 0;
+ hpc_dev_id->class_mask = 0;
+ hpc_dev_id->driver_data = 0;
+
+}

So a Compaq chip will _never_ be on a ia64 machine? Why not just get this info from the pci_dev that was passed to the probe function originally?

+static int phphpc_is_64bit(struct controller *ctrl, u8 slot) +{
+ struct php_ctlr_state_s *pPhpCtlr = (struct php_ctlr_state_s *) ctrl->hpc_ctlr_handle;
+
+ _DBG_ENTER_ROUTINE
+ if (!ctrl->hpc_ctlr_handle) {
+ _DBG_PRINT_ERROR("Process %d: Invalid HPC controller handle!", current->pid);
+ return 0;
+ }
+
+ if (slot >= pPhpCtlr->num_slots) {
+ _DBG_PRINT_ERROR("Process %d: Invalid HPC slot number!", current->pid);
+ return 0;
+ }
+
+ _DBG_LEAVE_ROUTINE
+ return 1;
+}

You don't actually test anything in this function, why not?

+#ifdef CONFIG_IA64
+ longdelay(6 * (HZ / 10));
+#endif /* CONFIG_IA64 */

Ah, see, this can be removed, as was mentioned previously. And why does ia64 need the delay?

+static int phphpc_get_cur_bus_speed (struct slot *slot, enum pci_bus_speed *value) +{
+ struct php_ctlr_state_s *pPhpCtlr = (struct php_ctlr_state_s *) slot->ctrl->hpc_ctlr_handle;
+ u32 pci_misc_config;
+ enum pci_bus_speed bus_speed = PCI_SPEED_UNKNOWN;
+ u16 temp_word;
+ u32 temp_dword;
+ struct pci_dev *dev;
+
+ _DBG_ENTER_ROUTINE
+ if (!slot->ctrl->hpc_ctlr_handle) {
+ _DBG_PRINT_ERROR("Process %d: Invalid HPC controller handle!", current->pid);
+ return -1;
+ }
+
+ if (hpc_did = PCI_INTC_P64H2_DID) {
+ dev = pci_find_subsys(PCI_VENDOR_ID_INTEL, PCI_INTC_P64H2_HUB_PCI, PCI_ANY_ID, PCI_ANY_ID, NULL);

Why not just save off pci_dev, instead of constantly having to find it again. That's why it is passed to the probe() function :)

diff -urN linux-2.5.45-ia64-021031-phpa/drivers/hotplug.org/cpqphp_nvram.c linux-2.5.45-ia64-021031-phpa/drivers/hotplug/cpqphp_nvram.c
@@ -168,6 +168,9 @@
 
 static u32 access_EV (u16 operation, u8 *ev_name, u8 *buffer, u32 *buf_size)
 {
+#ifdef	CONFIG_IA64
+	return 0;
+#else
 

Why not just prevent this file from being compiled in on ia64. That would make things easier instead of having to put #ifdefs in the code.

+/*
+ ************************************************************************
+ * PCI HotPlug support routines..... These should probably be in *
+ * the common pci code, but it's not going in right now (pre 2.4) *
+ ************************************************************************ */

We are post 2.4 right now :)

int cpqhp_set_irq (u8 bus_num, u8 dev_num, u8 int_pin, u8 irq_num) {
+#if defined(CONFIG_HOTPLUG_PCI_COMPAQ_NVRAM) && defined(CONFIG_X86)
int rc;
u16 temp_word;
struct pci_dev fakedev;
@@ -372,35 +359,27 @@
// This should only be for x86 as it sets the Edge Level Control Register outb((u8) (temp_word & 0xFF), 0x4d0);
outb((u8) ((temp_word & 0xFF00) >> 8), 0x4d1);
-
+#endif
return 0;
}

Why doesn't ia64 work properly here?

+ struct pci_bus lpci_bus, *pci_bus;
+ memcpy(&lpci_bus, ctrl->pci_bus, sizeof(lpci_bus));
+ pci_bus = &lpci_bus;
+ pci_bus->number = new_slot->bus;

Why mock up a pci_bus on the stack here? Don't we know it based on the controller's pci_bus? Why did you change this?

Well that's enough to start with :)

thanks,

greg k-h

Hi Greg,

Thanks for the comments.
Most of your comments regarding style/preference are applied. My answers to your questions are below, and let me know your idea.

Isn't it a good time to start logging as a little appreciation and recognition of each individual's contribution ?

I'm toggling between your comment and the kernel's "struct resource" pci_dev res is based on.
In linux/ioport.h, struct resource is defined as unsigned long as follows, and that is used for pci io/mem/dma resources in struct pci_dev. And pci/setup-*.c is also unsigned long. Even I'm leaning toward your's, ulong seems to be the safest bet for the kernel ala "struct kernel" and related are in ulong.
What do you suggest ?

struct resource {
	const char *name;
	unsigned long start, end;
	unsigned long flags;
	struct resource *parent, *sibling, *child;
}

And, some i386 platform might need 64bit range. In this case also, the above structs need to be changed first to u64.

Done, no "//" in my code. Most of "//" are from original cpqphp source. I tried to minimize changes in the original cpqphp codes, unless otherwise needed for functionality.

The last three are removed for now since they are for other type of PCI HPCs.

It is to control *multiple* LEDs in one call, instead of sequence of LED setup calls.

I removed the #ifndef __IN_HPC__ to avoid confusion, but want to keep the macro for convenience sake.

Removed all macros per your recomendation except the two entry/exit macro pair. "_" prefix is also removed too. Allow these simple two entry/exit macros in hpc code only for a while since it's useful in debugging revisions of HPC hardwares on multiple revisions of servers... The rest of the driver codes is relatively mature but HPC codes are always hot because of new revisions of HWs...

This sis not a q&d trick for impaired lock-unlock. This is for controlled multiple lock requests where subsequent lock requests from lower-primitive functions are simply granted success w/o actual lock when mother caller already grabbed the exclusive hw access lock. Lock-unlock pair is managed in callee's side who knows where/when exclusive access is needed to perform certain controller operations. Caller, as consumer of the controller APIs shouldn't worry about the lock.
Caller still can request lock to perform a sequence of functions w/ exclusive hw access. In this case, callees(child functions)' individual lock requests will simply succeed w/o actual lock.
Actual instance of this is in init probe() function only. So, we know where and what :)

Urrg...this secret code name should've been removed :)

That is definition, this is declaration.

#ifdef is removed to allow delay for both i386 and ia64.

OK. only global phphpc_acquire|release_lock().

No direct mem access ??? This memory mapped access is very convenient feature that kernel provides, as many others do. Moreover, coming PCI-Express native hpc registers are all accessed thru memory-map too, by the spec. Am I missing something ?

....

This single set_led_state(slot, green_on, amber_blink, ...) replaces at least 6 LED APIs such as
green_on(slot); green_off(slot); green_blink(slot), amber_on(slot), ...,
and controls LEDs at a single API/lock overhead.
Unfolding to individual LED colors is just one of many implementation choices.

Agree... One more param to this function then.

No one asked me to support compaq controller for ia64 :) Agreed. pci_dev should be used whenever possible as you pointed out. So I changed pci_device_id table in pci_driver (core.c) into an array of id tables covering different cpq, intc controllers. And the above routine is removed.

We need either pci_find_subsystem() or pci_find_device() here because if hpc is "PCI_INTC_P64H2_DID", speed is obtained by "PCI_INTC_P64H2_HUB_PCI" device on the bus. Hence, we have to find "PCI_INTC_P64H2_HUB_PCI" device on the same pci bus. hpc_did is removed and pci_dev->device is used instead.

Then all access_EV caller codes need #ifdef CONFIG_IA64 around this (3 ~ 4 instances).
In fact, if compaq controller makes to IA64 platform, it'll need IA64 version of this code, if needed.

IA64 servers and Intel i386 servers don't need this code. This routine is from original cpqphp, and I saved it for cpq servers.

To support PCI bridge cards where dev and funcs are populated over bridge devices recursively.
Using ctrl->pci_bus->number in current cpqphp and ibmphp is not recursion safe. Do they support bridge card hot-add ?

Way enough :)

Driver is re-hauled again, and verified on two Itanium servers and one xeon server... again.
Can you just send your version of 'cb' so that I can simply run it on the driver with "cb -greg", to get code compliant to your coding style presentation foil ?
Just kidding :-)

thanks,
J.I.

If you really want to. What I have seen in the past is that this either quickly grows quite large, or is ignored. If you really want it there, fine, but at least sync up the dates with ones that reflect that the change made it into the kernel (your multiple dates doesn't show that.)

If you are trying to keep in sync with the struct resource, then use unsigned long (and spell it out, don't use ulong please). If not, then specifically set the length of the variable. In this case, it looks like unsigned long is what you want.

That's all nice and good, but is it really that hard to just sequence the calls? I'd prefer that to varargs trickery which doesn't play nice with the stack.

Then you have changed the names of the other functions that matched up with these macro names?

Ok, that sounds good.

But the old code did not need this type of locking mess. I'd suggest to just stick with a simple semaphore. It is cleaner, smaller, and easier to maintain over time.

But a one time declaration, which kind of means that making a definition for it was useless :)

Is that what you really want? Why? What is the delay for?

thanks.

Yes you are, please read Documentation/IO-mapping.txt. You are getting away with this by the fact that these archs allow a pointer to a remaped area to be directly written to. That's not portable at all.

Please do that, it saves kernel stack space and reduces complexity.

thank you.

So you need to talk to the "main" controller instead? Do you have a spec citation for this anywhere?

No, the "no nvram" functions will be called as defined in the header file.

Yes it would, but as you stated earlier, you aren't doing that work :)

But what about cpq servers who do not specify the NVRAM option? They need them, but it is now broken.

They should, I haven't seen any instances where this would not work.

thanks,

greg k-h