Messages in this thread | | | Date | Mon, 16 Feb 2004 18:35:14 +0000 | From | Christoph Hellwig <> | Subject | Re: [PATCH] PPC64 PCI Hotplug Driver for RPA |
| |
On Mon, Feb 16, 2004 at 12:21:56PM -0600, John Rose wrote: > +#if !defined(CONFIG_HOTPLUG_PCI_MODULE) > + #define MY_NAME "rpaphp" > +#else > + #define MY_NAME THIS_MODULE->name > +#endif
Umm, what's this? Checking CONFIG_FOO_MODULE is basically always wrong and especially in this case. Just use "rpaphp" always.
> +static int num_slots = 0;
No need to initialized variables to 0
> +static int enable_slot (struct hotplug_slot *slot); > +static int disable_slot (struct hotplug_slot *slot); > +static int set_attention_status (struct hotplug_slot *slot, u8 value); > +static int get_power_status (struct hotplug_slot *slot, u8 *value); > +static int get_attention_status (struct hotplug_slot *slot, u8 *value); > +static int get_adapter_status (struct hotplug_slot *slot, u8 *value); > +static int get_max_bus_speed (struct hotplug_slot *hotplug_slot, enum pci_bus_speed *value); > +static int get_cur_bus_speed (struct hotplug_slot *hotplug_slot, enum pci_bus_speed *value);
The larger whitespace before the opening brace aren't exatly linux codingstyle..
> +static struct pci_dev *rpaphp_find_bridge_pdev(struct slot *slot) > +{ > + struct pci_dev *retval_dev = NULL; > + > + retval_dev = rpaphp_find_pci_dev(slot->dn); > + > + return retval_dev; > +}
This is horribly verbose. Why not simply
static struct pci_dev *rpaphp_find_bridge_pdev(struct slot *slot) { return rpaphp_find_pci_dev(slot->dn); }
dito for rpaphp_find_adapter_pdev
In fact this is only used once so the wrapper looks rather useless.
> +/* Inline functions to check the sanity of a pointer that is passed to us */ > +static inline int slot_paranoia_check(struct slot *slot, const char *function) > +{ > + if (!slot) { > + dbg("%s - slot == NULL\n", function); > + return -1; > + } > + > + if (!slot->hotplug_slot) { > + dbg("%s - slot->hotplug_slot == NULL!\n", function); > + return -1; > + } > + return 0; > +} > + > +static inline struct slot *get_slot(struct hotplug_slot *hotplug_slot, const char *function) > +{ > + struct slot *slot; > + > + if (!hotplug_slot) { > + dbg("%s - hotplug_slot == NULL\n", function); > + return NULL; > + }
If you have a method that per specification doesn't get a NULL pointer adding these kinds of checks is bad. Getting a NULL pointer would be against the codified guaranteeds and your system already is bad trouble - better panic ASAP by dereferencing the NULL pointer than waiting longer and possibly corrupting data.
> +static int init_slots (void) > +{ > + int retval = 0; > + > + retval = rpaphp_add_slot(NULL); > + > + return retval; > +}
Same strange verbosity as above.
> +static int __init rpaphp_init(void) > +{ > + int retval = 0; > + > + info(DRIVER_DESC " version: " DRIVER_VERSION "\n"); > + > + rpaphp_debug = debug; > + > + /* read all the PRA info from the system */ > + retval = init_rpa(); > + > + return retval;
Again.. Btw, why do you have rpaphp_debug and debug? Just using one is much less confusing.
> +static void __exit rpaphp_exit(void) > +{ > + cleanup_slots(); > +}
Why the wrapping?
> + } > + else {
linux coding style says this is
} else {
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |