[Linux-ia64] Re: 2.5.45 cpqphp driver patch w/ intcphp driver enhancements

[Greg KH <greg@kroah.com>]

On Mon, Nov 11, 2002 at 04:20:16PM -0800, Lee, Jung-Ik wrote:
> Hi Greg,
> 
> Here's PCI hotplug driver patch to cpqphp driver in 2.5.45.
> It took a while since this patch required integration of intcphp driver and
> cpqphp driver, which basically nullified all previous validation/regression
> on intcphp, hence driver had to go thru it again... It's been verified on
> three servers (Itanium2(Tiger), Itanium1(Lion), and i386 Intel server) in
> 2.5.39 and 2.5.45 kernels.

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
--- linux-2.5.45-ia64-021031-phpa/drivers/hotplug.org/Kconfig	Wed Oct 30 16:42:29 2002
+++ linux-2.5.45-ia64-021031-phpa/drivers/hotplug/Kconfig	Tue Nov  5 12:40:27 2002
@@ -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
--- linux-2.5.45-ia64-021031-phpa/drivers/hotplug.org/cpqphp.h	Wed Oct 30 16:43:38 2002
+++ linux-2.5.45-ia64-021031-phpa/drivers/hotplug/cpqphp.h	Mon Nov 11 14:53:17 2002
@@ -4,6 +4,9 @@
  * Copyright (c) 1995,2001 Compaq Computer Corporation
  * Copyright (c) 2001 Greg Kroah-Hartman (greg@kroah.com)
  * Copyright (c) 2001 IBM
+ * Copyright (C) 2001-2002 Intel
+ *  Copyright (C) Fred Lewis (frederick.v.lewis@intel.com)
+ *  Copyright (c) J.I. Lee (jung-ik.lee@intel.com)

Keep the spacing the same :)

  * Send feedback to <greg@kroah.com>
  *
+ *  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
--- linux-2.5.45-ia64-021031-phpa/drivers/hotplug.org/cpqphp_nvram.c	Wed Oct 30 16:43:39 2002
+++ linux-2.5.45-ia64-021031-phpa/drivers/hotplug/cpqphp_nvram.c	Tue Nov  5 21:20:15 2002
@@ -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