[PATCH] PPC64 PCI Hot Plug Controller Driver-review request

Linda Xie lxie at us.ibm.com
Fri Oct 24 01:28:52 EST 2003

Hi Anton,

Thank you very much for viewing the code. Please see my responses below
starting with LINDA.


[ Anton Blanchard <anton at samba.org> writes: ]

>> The attached patch is the latest version of PPC64 Hot Plug Controller
>> Driver for IBM pSeries platforms.

>Looking good. Here are a few questions and suggestions.

>I couldnt find where in the scheme of things that we do configure-connector.
>Did that fragment of code end up on the mailing list?

LINDA: Things like configure-connector and isolate/unisolate are done
      by user-land tools.

@@ -135,6 +136,9 @@

>Could you move these right below the functions themselves?

LINDA: They will be reomved from ppc_syms.c and put in
            the definition files.

+void ppc64php_lock(void)
+            dbg("Entry %s\n", __FUNCTION__);
+            down(&ppc64php_sem);
+            dbg("Exit %s\n", __FUNCTION__);

>I prefer not to wrap standard locking functions but just use down etc
>directly. Its very easy to forget if you have a sleeping lock or not
>when its hidden away.

>I noticed some of the other hotplug drivers do this, but I still dont
>think its a good idea :)

LINDA:  I will remove them and call down/up directly.

+static inline int atoi(const char *s, int base)
+        int i=0;
+        while (isdigit(*s))
+                i = i*base + *(s++) - '0';
+        return i;

>Could you use something generic here like simple_strtoul?

LINDA: I will remove it and use simple_strtoul.

+static inline int ppc64php_get_sensor_state(int index, int *state)
+            int rc;
+        rc = rtas_get_sensor(DR_ENTITY_SENSE, index, state);
+        if (rc) {
+                if (rc ==  NEED_POWER || rc == PWR_ONLY) {
+                                    dbg("%s: slot must be power up to get
+                                                __FUNCTION__);
+                }
+                else if (rc == ERR_SENSE_USE) {
+                                    dbg("%s: slot is unusable\n",
+                }

>Very minor, but could you place the else on the same line as the closing

LINDA: I will change it.

+ struct pci_dev *ppc64php_find_pci_dev(struct device_node *dn)
+ static struct pci_dev *ppc64php_find_bridge_pdev(struct slot *slot)
+ static struct pci_dev *ppc64php_find_adapter_pdev(struct slot *slot)

>Im wondering if these fit better in the generic ppc64 code. We can worry
>about that later if and when someone else wants to use them.

LINDA: Sounds good.

+static int disable_slot(struct hotplug_slot *hotplug_slot)
+            dbg("%s - Entry: slot[%s]\n",
+                        __FUNCTION__, slot->name);


+            dbg("%s - Exit: rc[%d]\n",  __FUNCTION__, retval);

>I think it would be a bit more readable if you didnt have a debug entry
>for every function entry and exit. I notice it seems to be the way to
>go in some of the hotplug drivers :(

LINDA: I will remove some of them.

** Sent via the linuxppc64-dev mail list. See http://lists.linuxppc.org/

More information about the Linuxppc64-dev mailing list