| David Brazdil | 0f672f6 | 2019-12-10 10:32:29 +0000 | [diff] [blame^] | 1 | Contributions are solicited in particular to remedy the following issues: |
| 2 | |
| 3 | cpcihp: |
| 4 | |
| 5 | * There are no implementations of the ->hardware_test, ->get_power and |
| 6 | ->set_power callbacks in struct cpci_hp_controller_ops. Why were they |
| 7 | introduced? Can they be removed from the struct? |
| 8 | |
| 9 | cpqphp: |
| 10 | |
| 11 | * The driver spawns a kthread cpqhp_event_thread() which is woken by the |
| 12 | hardirq handler cpqhp_ctrl_intr(). Convert this to threaded IRQ handling. |
| 13 | The kthread is also woken from the timer pushbutton_helper_thread(), |
| 14 | convert it to call irq_wake_thread(). Use pciehp as a template. |
| 15 | |
| 16 | * A large portion of cpqphp_ctrl.c and cpqphp_pci.c concerns resource |
| 17 | management. Doesn't this duplicate functionality in the core? |
| 18 | |
| 19 | ibmphp: |
| 20 | |
| 21 | * Implementations of hotplug_slot_ops callbacks such as get_adapter_present() |
| 22 | in ibmphp_core.c create a copy of the struct slot on the stack, then perform |
| 23 | the actual operation on that copy. Determine if this overhead is necessary, |
| 24 | delete it if not. The functions also perform a NULL pointer check on the |
| 25 | struct hotplug_slot, this seems superfluous. |
| 26 | |
| 27 | * Several functions access the pci_slot member in struct hotplug_slot even |
| 28 | though pci_hotplug.h declares it private. See get_max_bus_speed() for an |
| 29 | example. Either the pci_slot member should no longer be declared private |
| 30 | or ibmphp should store a pointer to its bus in struct slot. Probably the |
| 31 | former. |
| 32 | |
| 33 | * The functions get_max_adapter_speed() and get_bus_name() are commented out. |
| 34 | Can they be deleted? There are also forward declarations at the top of |
| 35 | ibmphp_core.c as well as pointers in ibmphp_hotplug_slot_ops, likewise |
| 36 | commented out. |
| 37 | |
| 38 | * ibmphp_init_devno() takes a struct slot **, it could instead take a |
| 39 | struct slot *. |
| 40 | |
| 41 | * The return value of pci_hp_register() is not checked. |
| 42 | |
| 43 | * iounmap(io_mem) is called in the error path of ebda_rsrc_controller() |
| 44 | and once more in the error path of its caller ibmphp_access_ebda(). |
| 45 | |
| 46 | * The various slot data structures are difficult to follow and need to be |
| 47 | simplified. A lot of functions are too large and too complex, they need |
| 48 | to be broken up into smaller, manageable pieces. Negative examples are |
| 49 | ebda_rsrc_controller() and configure_bridge(). |
| 50 | |
| 51 | * A large portion of ibmphp_res.c and ibmphp_pci.c concerns resource |
| 52 | management. Doesn't this duplicate functionality in the core? |
| 53 | |
| 54 | sgi_hotplug: |
| 55 | |
| 56 | * Several functions access the pci_slot member in struct hotplug_slot even |
| 57 | though pci_hotplug.h declares it private. See sn_hp_destroy() for an |
| 58 | example. Either the pci_slot member should no longer be declared private |
| 59 | or sgi_hotplug should store a pointer to it in struct slot. Probably the |
| 60 | former. |
| 61 | |
| 62 | shpchp: |
| 63 | |
| 64 | * There is only a single implementation of struct hpc_ops. Can the struct be |
| 65 | removed and its functions invoked directly? This has already been done in |
| 66 | pciehp with commit 82a9e79ef132 ("PCI: pciehp: remove hpc_ops"). Clarify |
| 67 | if there was a specific reason not to apply the same change to shpchp. |
| 68 | |
| 69 | * The ->get_mode1_ECC_cap callback in shpchp_hpc_ops is never invoked. |
| 70 | Why was it introduced? Can it be removed? |
| 71 | |
| 72 | * The hardirq handler shpc_isr() queues events on a workqueue. It can be |
| 73 | simplified by converting it to threaded IRQ handling. Use pciehp as a |
| 74 | template. |