Aviad Krawczyk <aviad.krawc...@huawei.com> : [...] > hinic_remove - If insmod failed and someone calls rmmod, we will get a > crash because the resource are already free. Therefore we test if the > device exists, please tell me if you meant to something different
The module won't even proceed through the pci_driver remove method if the probe method failed. See drivers/pci/bus.c::pci_bus_add_device and track 'dev->is_added'. You don't need to believe me: try it. I have no idea where your crash comes from but something does not look quite right. (use module_pci_driver() to save some boilerplate code btw) [...] > The priv data is in type void * because the > caller can use any struct that it wants, like the priv data in Linux > (netdev, irq, tasklet, work..) - I disagree. A driver is a piece of glue between hardware and software. It fills a kernel's contract. It is not supposed to introduce opaque data (even if it's hard to resist). > we can change it but if we will pass different struct > in the future, we will have to change the prototype of the functions. It's fine. If I do something wrong - and at some point I will - I'd rather have it detected at compile time. Nobody wants to waste precious hardware lab testing time because of excess void *. > According to the other void *: > The wq struct is used for cmdq, sq and rq. Therefore the wqe is in type > void *. There are 4 operations get_wqe, write_wqe, read_wqe and put_wqe - > there > is no option that one function will be fed with a wrong pointer because the > caller > should use what it got in get_wqe function. > > When something is used as multiple types, it can be used as void * or union. > Usually, I would prefer union. But, in this case if we will use union, maybe > there is a chance of using the wrong wqe type in the wrong work queue type. union * will at least catch being fed a wrong type. void * won't notice. Let's take a practical example: hinic_sq_get_sges. void hinic_sq_get_sges(void *wqe, struct hinic_sge *sges, int nr_sges) ^^^^^^^^^ { struct hinic_sq_wqe *sq_wqe = (struct hinic_sq_wqe *)wqe; static void free_all_tx_skbs(struct hinic_txq *txq) { struct hinic_dev *nic_dev = netdev_priv(txq->netdev); struct hinic_sq *sq = txq->sq; struct hinic_sq_wqe *wqe; [...] hinic_sq_get_sges(wqe, txq->free_sges, nr_sges); static int free_tx_poll(struct napi_struct *napi, int budget) { [...] struct hinic_sq_wqe *wqe; [...] hinic_sq_get_sges(wqe, txq->free_sges, nr_sges); Why is it: void hinic_sq_get_sges(void *wqe, ... instead of: void hinic_sq_get_sges(struct hinic_sq_wqe *wqe, ... Because of a future change ? -- Ueimor