Messages in this thread | | | Subject | Re: [PATCH V2 net-next 01/21] net-next/hinic: Initialize hw interface | From | Aviad Krawczyk <> | Date | Tue, 25 Jul 2017 17:50:38 +0300 |
| |
Hi,
hinic_remove - you are right, it is over safe code. We can remove it without any crash. This case has never happened.
void * - I meant the problem is not in rq or sq, it can be in wq. But you confirmed that it is ok to use union instead of the void* in wq. So, we will use union in wq.c and in rq - rq_wqe and in sq - sq_wqe.
module_pci_driver - is not used in other drivers in the same segments, it is necessary?
Thanks, Aviad
On 7/25/2017 2:03 AM, Francois Romieu wrote: > Aviad Krawczyk <aviad.krawczyk@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 ? >
| |