Re: 2.5.45 cpqphp driver patch w/ intcphp driver enhancements

Re: 2.5.45 cpqphp driver patch w/ intcphp driver enhancements

[Greg KH]

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.

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

Re: 2.5.45 cpqphp driver patch w/ intcphp driver enhancements

[Greg KH]

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

Re: 2.5.45 cpqphp driver patch w/ intcphp driver enhancements

[Greg KH]

On Thu, Nov 14, 2002 at 03:52:12PM -0800, Lee, Jung-Ik wrote:
> 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.
> 
> 
> > This driver doesn't need a list of Updates, as these are only done by
> > your group, not anyone else.  Please delete this, thanks.
> 
> Isn't it a good time to start logging as a little appreciation and
> recognition of each individual's contribution ?

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.)

> >  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.
> 
> 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 ?

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.

> > And why the varargs on set_led_state()?
> 
> It is to control *multiple* LEDs in one call, instead of sequence of LED
> setup calls.

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.

> > +# 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.
> 
> I removed the #ifndef __IN_HPC__ to avoid confusion, but want to keep the
> macro for convenience sake.

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

> 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... 

Ok, that sounds good.

> > +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.
> 
> 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 :)

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.

> > +_DEFINE_DBG_BUFFER		/* Debug string buffer for 
> > entire HPC defined here */
> > 
> > Why here?  What was wrong with a few lines above this?
> 
> That is definition, this is declaration.

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

> > +#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.
> 
> #ifdef is removed to allow delay for both i386 and ia64.

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

> > +	_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.)
> 
> OK. only global phphpc_acquire|release_lock().

thanks.

> > +	/* 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.
> 
> 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 ?

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.

> 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. 

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

> 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.

thank you.

> > +	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 :)
> 
> 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.

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

> >  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.
> 
> Then all access_EV caller codes need #ifdef CONFIG_IA64 around this (3 ~ 4
> instances).

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

> In fact, if compaq controller makes to IA64 platform, it'll need IA64
> version of this code, if needed.

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

> >  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?
> 
> IA64 servers and Intel i386 servers don't need this code.
> This routine is from original cpqphp, and I saved it for cpq servers.

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

> > +	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?
> 
> 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 ?

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

thanks,

greg k-h

RE: 2.5.45 cpqphp driver patch w/ intcphp driver enhancements

[Lee, Jung-Ik]

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.


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

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

>  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.

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.


> +	//  reformat it to SHPC
>
> Try not to use C++ style comments in the code, thanks.

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.


> +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?

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


> And why the varargs on set_led_state()?

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


> +# 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.

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


> 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?

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... 

> +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.

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 :)


> +#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 :)

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


> +_DEFINE_DBG_BUFFER		/* Debug string buffer for 
> entire HPC defined here */
> 
> Why here?  What was wrong with a few lines above this?

That is definition, this is declaration.


> +#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.

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


> +	_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.)

OK. only global phphpc_acquire|release_lock().


> +	/* 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.

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 ?

> +static void
> +phphpc_set_led_state(struct slot * slot, struct 
> php_led_id_state * led_id_state, ...)
> +{
....
> +	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?

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. 


> +	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.)

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

> +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?

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.


> +	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 :)

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.


>  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.

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.


>  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?

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


> +	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?

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 ?

> Well that's enough to start with :)

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.