Re: [PATCH V2 net-next 01/21] net-next/hinic: Initialize hw interface
OK, we will use module_pci_driver although it is not very common in the same segment. On 7/25/2017 11:02 PM, Francois Romieu wrote: > Aviad Krawczyk : > [...] >> module_pci_driver - is not used in other drivers in the same segments, it >> is necessary ? > > /me checks... Ok, there seems to be some overenthusiastic copy'paste. > > See drivers/net/ethernet/intel/ixgb/ixgb_main.c: > [...] > /** > * ixgb_init_module - Driver Registration Routine > * > * ixgb_init_module is the first routine called when the driver is > * loaded. All it does is register with the PCI subsystem. > **/ > > static int __init > ixgb_init_module(void) > { > pr_info("%s - version %s\n", ixgb_driver_string, ixgb_driver_version); > pr_info("%s\n", ixgb_copyright); > > return pci_register_driver(&ixgb_driver); > } > > module_init(ixgb_init_module); > > /** > * ixgb_exit_module - Driver Exit Cleanup Routine > * > * ixgb_exit_module is called just before the driver is removed > * from memory. > **/ > > static void __exit > ixgb_exit_module(void) > { > pci_unregister_driver(&ixgb_driver); > } > > module_exit(ixgb_exit_module); > > Driver version ought to be fed through ethtool, if ever. Copyright message > mildly contributes to a better world. So the whole stuff above could be: > > module_pci_driver(ixgb_driver); >
Re: [PATCH V2 net-next 01/21] net-next/hinic: Initialize hw interface
Aviad Krawczyk : [...] > module_pci_driver - is not used in other drivers in the same segments, it > is necessary ? /me checks... Ok, there seems to be some overenthusiastic copy'paste. See drivers/net/ethernet/intel/ixgb/ixgb_main.c: [...] /** * ixgb_init_module - Driver Registration Routine * * ixgb_init_module is the first routine called when the driver is * loaded. All it does is register with the PCI subsystem. **/ static int __init ixgb_init_module(void) { pr_info("%s - version %s\n", ixgb_driver_string, ixgb_driver_version); pr_info("%s\n", ixgb_copyright); return pci_register_driver(&ixgb_driver); } module_init(ixgb_init_module); /** * ixgb_exit_module - Driver Exit Cleanup Routine * * ixgb_exit_module is called just before the driver is removed * from memory. **/ static void __exit ixgb_exit_module(void) { pci_unregister_driver(&ixgb_driver); } module_exit(ixgb_exit_module); Driver version ought to be fed through ethtool, if ever. Copyright message mildly contributes to a better world. So the whole stuff above could be: module_pci_driver(ixgb_driver); -- Ueimor
Re: [PATCH V2 net-next 01/21] net-next/hinic: Initialize hw interface
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 : > [...] >> 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 ? >
Re: [PATCH V2 net-next 01/21] net-next/hinic: Initialize hw interface
Aviad Krawczyk : [...] > 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
Re: [PATCH V2 net-next 01/21] net-next/hinic: Initialize hw interface
Hi Francois, ERR_PTR / IS ERR - we will change it err_xyz labels - we will change it according to other companies style. hinic_free_hwdev - It is there to mark us changes for VF code. We will remove it, it can't be failed. 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 pci_id_tbl - will be moved to the .c file. void* - usually void * is something to avoid. 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..) - we can change it but if we will pass different struct in the future, we will have to change the prototype of the functions. 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. Another option is to use a wrapper for each wq type operations, so only the basic wq ops will use void *. Then, there will be cmdq, rq, sq operations with the correct wqe type and wq operations that will use void *. I will be glad to hear your opinion about the preferred style and about hinic_remove issue you mentioned above. Thanks for your time and review, Aviad On 7/20/2017 1:27 AM, Francois Romieu wrote: > Aviad Krawczyk : > [...] >> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_hw_dev.c >> b/drivers/net/ethernet/huawei/hinic/hinic_hw_dev.c >> new file mode 100644 >> index 000..fbc9de4 >> --- /dev/null >> +++ b/drivers/net/ethernet/huawei/hinic/hinic_hw_dev.c > [...] >> +/** >> + * hinic_init_hwdev - Initialize the NIC HW >> + * @hwdev: the NIC HW device that is returned from the initialization >> + * @pdev: the NIC pci device >> + * >> + * Return 0 - Success, negative - Failure >> + * >> + * Initialize the NIC HW device and return a pointer to it in the first arg >> + **/ > > Return a pointer and use ERR_PTR / IS_ERR ? > >> +int hinic_init_hwdev(struct hinic_hwdev **hwdev, struct pci_dev *pdev) >> +{ >> +struct hinic_pfhwdev *pfhwdev; >> +struct hinic_hwif *hwif; >> +int err; >> + >> +hwif = devm_kzalloc(&pdev->dev, sizeof(*hwif), GFP_KERNEL); >> +if (!hwif) >> +return -ENOMEM; >> + >> +err = hinic_init_hwif(hwif, pdev); >> +if (err) { >> +dev_err(&pdev->dev, "Failed to init HW interface\n"); >> +return err; >> +} >> + >> +if (!HINIC_IS_PF(hwif) && !HINIC_IS_PPF(hwif)) { >> +dev_err(&pdev->dev, "Unsupported PCI Function type\n"); >> +err = -EFAULT; >> +goto func_type_err; >> +} >> + >> +pfhwdev = devm_kzalloc(&pdev->dev, sizeof(*pfhwdev), GFP_KERNEL); >> +if (!pfhwdev) { >> +err = -ENOMEM; >> +goto pfhwdev_alloc_err; > > Intel, Mellanox, Broadcom, Amazon and friends use "err_xyz" labels. > > Please consider using the same style. > > [...] >> +void hinic_free_hwdev(struct hinic_hwdev *hwdev) >> +{ >> +struct hinic_hwif *hwif = hwdev->hwif; >> +struct pci_dev *pdev = hwif->pdev; >> +struct hinic_pfhwdev *pfhwdev; >> + >> +if (!HINIC_IS_PF(hwif) && !HINIC_IS_PPF(hwif)) { >> +dev_err(&pdev->dev, "unsupported PCI Function type\n"); >> +return; >> +} > > If it succeeded in hinic_init_hwdev, how could it fail here ? > > If it failed in hinic_init_hwdev, hinic_free_hwdev should not even > be called. > > -> remove ? > > [...] >> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_main.c >> b/drivers/net/ethernet/huawei/hinic/hinic_main.c >> new file mode 100644 >> index 000..c61c769 >> --- /dev/null >> +++ b/drivers/net/ethernet/huawei/hinic/hinic_main.c > [...] >> +static void hinic_remove(struct pci_dev *pdev) >> +{ >> +struct net_device *netdev = pci_get_drvdata(pdev); >> +struct hinic_dev *nic_dev; >> + >> +if (!netdev) >> +return; > > Your driver is flawed if this test can ever succeed. > > [...] >> +static int __init hinic_init(void) >> +{ >> +return pci_register_driver(&hinic_driver); >> +} >> + >> +static void __exit hinic_exit(void) >> +{ >> +pci_unregister_driver(&hinic_driver); >> +} > > Use module_pci_driver(hinic_driver). > > Remove hinic_init() and hinic_exit(). > >> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_pci_id_tbl.h >> b/drivers/net/ethernet/huawei/hinic/hinic_pci_id_tbl.h >> new file mode 100644 >> index 000..1d92617 >> --- /dev/null >> +++ b/drivers/net/ethernet/huawei/hinic/hinic_pci_id_tbl.h > [..
Re: [PATCH V2 net-next 01/21] net-next/hinic: Initialize hw interface
Aviad Krawczyk : [...] > diff --git a/drivers/net/ethernet/huawei/hinic/hinic_hw_dev.c > b/drivers/net/ethernet/huawei/hinic/hinic_hw_dev.c > new file mode 100644 > index 000..fbc9de4 > --- /dev/null > +++ b/drivers/net/ethernet/huawei/hinic/hinic_hw_dev.c [...] > +/** > + * hinic_init_hwdev - Initialize the NIC HW > + * @hwdev: the NIC HW device that is returned from the initialization > + * @pdev: the NIC pci device > + * > + * Return 0 - Success, negative - Failure > + * > + * Initialize the NIC HW device and return a pointer to it in the first arg > + **/ Return a pointer and use ERR_PTR / IS_ERR ? > +int hinic_init_hwdev(struct hinic_hwdev **hwdev, struct pci_dev *pdev) > +{ > + struct hinic_pfhwdev *pfhwdev; > + struct hinic_hwif *hwif; > + int err; > + > + hwif = devm_kzalloc(&pdev->dev, sizeof(*hwif), GFP_KERNEL); > + if (!hwif) > + return -ENOMEM; > + > + err = hinic_init_hwif(hwif, pdev); > + if (err) { > + dev_err(&pdev->dev, "Failed to init HW interface\n"); > + return err; > + } > + > + if (!HINIC_IS_PF(hwif) && !HINIC_IS_PPF(hwif)) { > + dev_err(&pdev->dev, "Unsupported PCI Function type\n"); > + err = -EFAULT; > + goto func_type_err; > + } > + > + pfhwdev = devm_kzalloc(&pdev->dev, sizeof(*pfhwdev), GFP_KERNEL); > + if (!pfhwdev) { > + err = -ENOMEM; > + goto pfhwdev_alloc_err; Intel, Mellanox, Broadcom, Amazon and friends use "err_xyz" labels. Please consider using the same style. [...] > +void hinic_free_hwdev(struct hinic_hwdev *hwdev) > +{ > + struct hinic_hwif *hwif = hwdev->hwif; > + struct pci_dev *pdev = hwif->pdev; > + struct hinic_pfhwdev *pfhwdev; > + > + if (!HINIC_IS_PF(hwif) && !HINIC_IS_PPF(hwif)) { > + dev_err(&pdev->dev, "unsupported PCI Function type\n"); > + return; > + } If it succeeded in hinic_init_hwdev, how could it fail here ? If it failed in hinic_init_hwdev, hinic_free_hwdev should not even be called. -> remove ? [...] > diff --git a/drivers/net/ethernet/huawei/hinic/hinic_main.c > b/drivers/net/ethernet/huawei/hinic/hinic_main.c > new file mode 100644 > index 000..c61c769 > --- /dev/null > +++ b/drivers/net/ethernet/huawei/hinic/hinic_main.c [...] > +static void hinic_remove(struct pci_dev *pdev) > +{ > + struct net_device *netdev = pci_get_drvdata(pdev); > + struct hinic_dev *nic_dev; > + > + if (!netdev) > + return; Your driver is flawed if this test can ever succeed. [...] > +static int __init hinic_init(void) > +{ > + return pci_register_driver(&hinic_driver); > +} > + > +static void __exit hinic_exit(void) > +{ > + pci_unregister_driver(&hinic_driver); > +} Use module_pci_driver(hinic_driver). Remove hinic_init() and hinic_exit(). > diff --git a/drivers/net/ethernet/huawei/hinic/hinic_pci_id_tbl.h > b/drivers/net/ethernet/huawei/hinic/hinic_pci_id_tbl.h > new file mode 100644 > index 000..1d92617 > --- /dev/null > +++ b/drivers/net/ethernet/huawei/hinic/hinic_pci_id_tbl.h [...] > +#ifndef HINIC_PCI_ID_TBL_H > +#define HINIC_PCI_ID_TBL_H > + > +#ifndef PCI_VENDOR_ID_HUAWEI > +#define PCI_VENDOR_ID_HUAWEI0x19e5 > +#endif Useless: it duplicates include/linux/pci_ids.h > + > +#ifndef PCI_DEVICE_ID_HI1822_PF > +#define PCI_DEVICE_ID_HI1822_PF 0x1822 > +#endif Please move it to the .c file where it is actually used. Extra: grep -E 'void\ \*' drivers/net/ethernet/huawei/hinic/* makes me nervous. At some point one function will be fed with a wrong pointer and the compiler won't notice it. For instance hinic_sq_read_wqe is only called with &skb. There's no reason to declare it using a 'void **' argument. -- Ueimor
[PATCH V2 net-next 01/21] net-next/hinic: Initialize hw interface
Initialize hw interface as part of the nic initialization for accessing hw. Signed-off-by: Aviad Krawczyk Signed-off-by: Zhao Chen --- Documentation/networking/hinic.txt | 125 drivers/net/ethernet/Kconfig | 1 + drivers/net/ethernet/Makefile | 1 + drivers/net/ethernet/huawei/Kconfig| 19 ++ drivers/net/ethernet/huawei/Makefile | 5 + drivers/net/ethernet/huawei/hinic/Kconfig | 13 ++ drivers/net/ethernet/huawei/hinic/Makefile | 3 + drivers/net/ethernet/huawei/hinic/hinic_dev.h | 33 drivers/net/ethernet/huawei/hinic/hinic_hw_csr.h | 36 drivers/net/ethernet/huawei/hinic/hinic_hw_dev.c | 208 drivers/net/ethernet/huawei/hinic/hinic_hw_dev.h | 42 + drivers/net/ethernet/huawei/hinic/hinic_hw_if.c| 208 drivers/net/ethernet/huawei/hinic/hinic_hw_if.h| 160 drivers/net/ethernet/huawei/hinic/hinic_main.c | 209 + .../net/ethernet/huawei/hinic/hinic_pci_id_tbl.h | 27 +++ 15 files changed, 1090 insertions(+) create mode 100644 Documentation/networking/hinic.txt create mode 100644 drivers/net/ethernet/huawei/Kconfig create mode 100644 drivers/net/ethernet/huawei/Makefile create mode 100644 drivers/net/ethernet/huawei/hinic/Kconfig create mode 100644 drivers/net/ethernet/huawei/hinic/Makefile create mode 100644 drivers/net/ethernet/huawei/hinic/hinic_dev.h create mode 100644 drivers/net/ethernet/huawei/hinic/hinic_hw_csr.h create mode 100644 drivers/net/ethernet/huawei/hinic/hinic_hw_dev.c create mode 100644 drivers/net/ethernet/huawei/hinic/hinic_hw_dev.h create mode 100644 drivers/net/ethernet/huawei/hinic/hinic_hw_if.c create mode 100644 drivers/net/ethernet/huawei/hinic/hinic_hw_if.h create mode 100644 drivers/net/ethernet/huawei/hinic/hinic_main.c create mode 100644 drivers/net/ethernet/huawei/hinic/hinic_pci_id_tbl.h diff --git a/Documentation/networking/hinic.txt b/Documentation/networking/hinic.txt new file mode 100644 index 000..989366a --- /dev/null +++ b/Documentation/networking/hinic.txt @@ -0,0 +1,125 @@ +Linux Kernel Driver for Huawei Intelligent NIC(HiNIC) family + + +Overview: += +HiNIC is a network interface card for the Data Center Area. + +The driver supports a range of link-speed devices (10GbE, 25GbE, 40GbE, etc.). +The driver supports also a negotiated and extendable feature set. + +Some HiNIC devices support SR-IOV. This driver is used for Physical Function +(PF). + +HiNIC devices support MSI-X interrupt vector for each Tx/Rx queue and +adaptive interrupt moderation. + +HiNIC devices support also various offload features such as checksum offload, +TCP Transmit Segmentation Offload(TSO), Receive-Side Scaling(RSS) and +LRO(Large Receive Offload). + + +Supported PCI vendor ID/device IDs: +=== + +19e5:1822 - HiNIC PF + + +Driver Architecture and Source Code: + + +hinic_dev - Implement a Logical Network device that is independent from +specific HW details about HW data structure formats. + +hinic_hwdev - Implement the HW details of the device and include the components +for accessing the PCI NIC. + +hinic_hwdev contains the following components: +=== + +HW Interface: += + +The interface for accessing the pci device (DMA memory and PCI BARs). +(hinic_hw_if.c, hinic_hw_if.h) + +Configuration Status Registers Area that describes the HW Registers on the +configuration and status BAR0. (hinic_hw_csr.h) + +MGMT components: + + +Asynchronous Event Queues(AEQs) - The event queues for receiving messages from +the MGMT modules on the cards. (hinic_hw_eqs.c, hinic_hw_eqs.h) + +Application Programmable Interface commands(API CMD) - Interface for sending +MGMT commands to the card. (hinic_hw_api_cmd.c, hinic_hw_api_cmd.h) + +Management (MGMT) - the PF to MGMT channel that uses API CMD for sending MGMT +commands to the card and receives notifications from the MGMT modules on the +card by AEQs. Also set the addresses of the IO CMDQs in HW. +(hinic_hw_mgmt.c, hinic_hw_mgmt.h) + +IO components: +== + +Completion Event Queues(CEQs) - The completion Event Queues that describe IO +tasks that are finished. (hinic_hw_eqs.c, hinic_hw_eqs.h) + +Work Queues(WQ) - Contain the memory and operations for use by CMD queues and +the Queue Pairs. The WQ is a Memory Block in a Page. The Block contains +pointers to Memory Areas that are the Memory for the Work Queue Elements(WQEs). +(hinic_hw_wq.c, hinic_hw_wq.h) + +Command Queues(CMDQ) - The queues for sending commands for IO management and is +used to set the QPs addresses in HW. The commands completion events are +accumulated on the CEQ that is configured to receive the CMDQ completion ev