| Message ID | 20210514141214.2120-7-shiraz.saleem@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add Intel Ethernet Protocol Driver for RDMA (irdma) | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | fail | Series longer than 15 patches |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 2 maintainers not CCed: jesse.brandeburg@intel.com intel-wired-lan@lists.osuosl.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 1 this patch: 1 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: line length of 85 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On Fri, May 14, 2021 at 09:11:58AM -0500, Shiraz Saleem wrote: > Convert i40e to use the auxiliary bus infrastructure to export > the RDMA functionality of the device to the RDMA driver. > Register i40e client auxiliary RDMA device on the auxiliary bus per > PCIe device function for the new auxiliary rdma driver (irdma) to > attach to. > > The global i40e_register_client and i40e_unregister_client symbols > will be obsoleted once irdma replaces i40iw in the kernel > for the X722 device. > > Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com> > --- > drivers/net/ethernet/intel/Kconfig | 1 + > drivers/net/ethernet/intel/i40e/i40e.h | 2 + > drivers/net/ethernet/intel/i40e/i40e_client.c | 152 ++++++++++++++++++++++---- > drivers/net/ethernet/intel/i40e/i40e_main.c | 1 + > 4 files changed, 136 insertions(+), 20 deletions(-) The amount of obfuscation in this driver is astonishing. I would expect that after this series, the i40e_client_add_*() would be cleaned, for example simple grep of I40E_CLIENT_VERSION_MAJOR shows that i40e_register_client() still have no-go code. Thanks
On Wed, May 19, 2021 at 03:51:46PM +0300, Leon Romanovsky wrote: > On Fri, May 14, 2021 at 09:11:58AM -0500, Shiraz Saleem wrote: > > Convert i40e to use the auxiliary bus infrastructure to export > > the RDMA functionality of the device to the RDMA driver. > > Register i40e client auxiliary RDMA device on the auxiliary bus per > > PCIe device function for the new auxiliary rdma driver (irdma) to > > attach to. > > > > The global i40e_register_client and i40e_unregister_client symbols > > will be obsoleted once irdma replaces i40iw in the kernel > > for the X722 device. > > > > Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com> > > drivers/net/ethernet/intel/Kconfig | 1 + > > drivers/net/ethernet/intel/i40e/i40e.h | 2 + > > drivers/net/ethernet/intel/i40e/i40e_client.c | 152 ++++++++++++++++++++++---- > > drivers/net/ethernet/intel/i40e/i40e_main.c | 1 + > > 4 files changed, 136 insertions(+), 20 deletions(-) > > The amount of obfuscation in this driver is astonishing. > > I would expect that after this series, the i40e_client_add_*() would > be cleaned, for example simple grep of I40E_CLIENT_VERSION_MAJOR > shows that i40e_register_client() still have no-go code. While it would be nice to see i40e fully cleaned I think we agreed to largely ignore it as-is so long as the new driver's aux implementation was sane. Jason
On Wed, May 19, 2021 at 10:43:49AM -0300, Jason Gunthorpe wrote: > On Wed, May 19, 2021 at 03:51:46PM +0300, Leon Romanovsky wrote: > > On Fri, May 14, 2021 at 09:11:58AM -0500, Shiraz Saleem wrote: > > > Convert i40e to use the auxiliary bus infrastructure to export > > > the RDMA functionality of the device to the RDMA driver. > > > Register i40e client auxiliary RDMA device on the auxiliary bus per > > > PCIe device function for the new auxiliary rdma driver (irdma) to > > > attach to. > > > > > > The global i40e_register_client and i40e_unregister_client symbols > > > will be obsoleted once irdma replaces i40iw in the kernel > > > for the X722 device. > > > > > > Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com> > > > drivers/net/ethernet/intel/Kconfig | 1 + > > > drivers/net/ethernet/intel/i40e/i40e.h | 2 + > > > drivers/net/ethernet/intel/i40e/i40e_client.c | 152 ++++++++++++++++++++++---- > > > drivers/net/ethernet/intel/i40e/i40e_main.c | 1 + > > > 4 files changed, 136 insertions(+), 20 deletions(-) > > > > The amount of obfuscation in this driver is astonishing. > > > > I would expect that after this series, the i40e_client_add_*() would > > be cleaned, for example simple grep of I40E_CLIENT_VERSION_MAJOR > > shows that i40e_register_client() still have no-go code. > > While it would be nice to see i40e fully cleaned I think we agreed to > largely ignore it as-is so long as the new driver's aux implementation > was sane. It is hard to say, the code is so obfuscated with many layers in between. For example, I tried to follow where and how they use IDA index that is used in aux device creation and went lost. Sometimes they take it from PF, sometimes from the client from different allocation pool. If client logic goes, we will see less code which should be similar for netdev and RDMA. It is not the case now. Thanks > > Jason
> Subject: Re: [PATCH v5 06/22] i40e: Register auxiliary devices to provide RDMA > > On Wed, May 19, 2021 at 03:51:46PM +0300, Leon Romanovsky wrote: > > On Fri, May 14, 2021 at 09:11:58AM -0500, Shiraz Saleem wrote: > > > Convert i40e to use the auxiliary bus infrastructure to export the > > > RDMA functionality of the device to the RDMA driver. > > > Register i40e client auxiliary RDMA device on the auxiliary bus per > > > PCIe device function for the new auxiliary rdma driver (irdma) to > > > attach to. > > > > > > The global i40e_register_client and i40e_unregister_client symbols > > > will be obsoleted once irdma replaces i40iw in the kernel for the > > > X722 device. > > > > > > Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com> > > > drivers/net/ethernet/intel/Kconfig | 1 + > > > drivers/net/ethernet/intel/i40e/i40e.h | 2 + > > > drivers/net/ethernet/intel/i40e/i40e_client.c | 152 ++++++++++++++++++++++- > --- > > > drivers/net/ethernet/intel/i40e/i40e_main.c | 1 + > > > 4 files changed, 136 insertions(+), 20 deletions(-) > > > > The amount of obfuscation in this driver is astonishing. > > > > I would expect that after this series, the i40e_client_add_*() would > > be cleaned, for example simple grep of I40E_CLIENT_VERSION_MAJOR shows > > that i40e_register_client() still have no-go code. > > While it would be nice to see i40e fully cleaned I think we agreed to largely ignore it > as-is so long as the new driver's aux implementation was sane. > And for stuff like i40e_register_client and associated, I will send a cleanup patch after this series is merged to remove it. Shiraz
diff --git a/drivers/net/ethernet/intel/Kconfig b/drivers/net/ethernet/intel/Kconfig index d8a12da..eae1b42 100644 --- a/drivers/net/ethernet/intel/Kconfig +++ b/drivers/net/ethernet/intel/Kconfig @@ -241,6 +241,7 @@ config I40E tristate "Intel(R) Ethernet Controller XL710 Family support" imply PTP_1588_CLOCK depends on PCI + select AUXILIARY_BUS help This driver supports Intel(R) Ethernet Controller XL710 Family of devices. For more information on how to identify your adapter, go diff --git a/drivers/net/ethernet/intel/i40e/i40e.h b/drivers/net/ethernet/intel/i40e/i40e.h index 85d3dd3a..b9417dc 100644 --- a/drivers/net/ethernet/intel/i40e/i40e.h +++ b/drivers/net/ethernet/intel/i40e/i40e.h @@ -870,6 +870,8 @@ struct i40e_netdev_priv { struct i40e_vsi *vsi; }; +extern struct ida i40e_client_ida; + /* struct that defines an interrupt vector */ struct i40e_q_vector { struct i40e_vsi *vsi; diff --git a/drivers/net/ethernet/intel/i40e/i40e_client.c b/drivers/net/ethernet/intel/i40e/i40e_client.c index 32f3fac..251da69 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_client.c +++ b/drivers/net/ethernet/intel/i40e/i40e_client.c @@ -12,6 +12,7 @@ static struct i40e_client *registered_client; static LIST_HEAD(i40e_devices); static DEFINE_MUTEX(i40e_device_mutex); +DEFINE_IDA(i40e_client_ida); static int i40e_client_virtchnl_send(struct i40e_info *ldev, struct i40e_client *client, @@ -275,6 +276,57 @@ void i40e_client_update_msix_info(struct i40e_pf *pf) cdev->lan_info.msix_entries = &pf->msix_entries[pf->iwarp_base_vector]; } +static void i40e_auxiliary_dev_release(struct device *dev) +{ + struct i40e_auxiliary_device *i40e_aux_dev = + container_of(dev, struct i40e_auxiliary_device, aux_dev.dev); + + ida_free(&i40e_client_ida, i40e_aux_dev->aux_dev.id); + kfree(i40e_aux_dev); +} + +static int i40e_register_auxiliary_dev(struct i40e_info *ldev, const char *name) +{ + struct i40e_auxiliary_device *i40e_aux_dev; + struct pci_dev *pdev = ldev->pcidev; + struct auxiliary_device *aux_dev; + int ret; + + i40e_aux_dev = kzalloc(sizeof(*i40e_aux_dev), GFP_KERNEL); + if (!i40e_aux_dev) + return -ENOMEM; + + i40e_aux_dev->ldev = ldev; + + aux_dev = &i40e_aux_dev->aux_dev; + aux_dev->name = name; + aux_dev->dev.parent = &pdev->dev; + aux_dev->dev.release = i40e_auxiliary_dev_release; + ldev->aux_dev = aux_dev; + + ret = ida_alloc(&i40e_client_ida, GFP_KERNEL); + if (ret < 0) { + kfree(i40e_aux_dev); + return ret; + } + aux_dev->id = ret; + + ret = auxiliary_device_init(aux_dev); + if (ret < 0) { + ida_free(&i40e_client_ida, aux_dev->id); + kfree(i40e_aux_dev); + return ret; + } + + ret = auxiliary_device_add(aux_dev); + if (ret) { + auxiliary_device_uninit(aux_dev); + return ret; + } + + return ret; +} + /** * i40e_client_add_instance - add a client instance struct to the instance list * @pf: pointer to the board struct @@ -286,9 +338,6 @@ static void i40e_client_add_instance(struct i40e_pf *pf) struct netdev_hw_addr *mac = NULL; struct i40e_vsi *vsi = pf->vsi[pf->lan_vsi]; - if (!registered_client || pf->cinst) - return; - cdev = kzalloc(sizeof(*cdev), GFP_KERNEL); if (!cdev) return; @@ -308,11 +357,8 @@ static void i40e_client_add_instance(struct i40e_pf *pf) cdev->lan_info.fw_build = pf->hw.aq.fw_build; set_bit(__I40E_CLIENT_INSTANCE_NONE, &cdev->state); - if (i40e_client_get_params(vsi, &cdev->lan_info.params)) { - kfree(cdev); - cdev = NULL; - return; - } + if (i40e_client_get_params(vsi, &cdev->lan_info.params)) + goto free_cdev; mac = list_first_entry(&cdev->lan_info.netdev->dev_addrs.list, struct netdev_hw_addr, list); @@ -324,7 +370,17 @@ static void i40e_client_add_instance(struct i40e_pf *pf) cdev->client = registered_client; pf->cinst = cdev; - i40e_client_update_msix_info(pf); + cdev->lan_info.msix_count = pf->num_iwarp_msix; + cdev->lan_info.msix_entries = &pf->msix_entries[pf->iwarp_base_vector]; + + if (i40e_register_auxiliary_dev(&cdev->lan_info, "intel_rdma_iwarp")) + goto free_cdev; + + return; + +free_cdev: + kfree(cdev); + pf->cinst = NULL; } /** @@ -345,7 +401,7 @@ void i40e_client_del_instance(struct i40e_pf *pf) **/ void i40e_client_subtask(struct i40e_pf *pf) { - struct i40e_client *client = registered_client; + struct i40e_client *client; struct i40e_client_instance *cdev; struct i40e_vsi *vsi = pf->vsi[pf->lan_vsi]; int ret = 0; @@ -359,9 +415,11 @@ void i40e_client_subtask(struct i40e_pf *pf) test_bit(__I40E_CONFIG_BUSY, pf->state)) return; - if (!client || !cdev) + if (!cdev || !cdev->client) return; + client = cdev->client; + /* Here we handle client opens. If the client is down, and * the netdev is registered, then open the client. */ @@ -423,16 +481,8 @@ int i40e_lan_add_device(struct i40e_pf *pf) pf->hw.pf_id, pf->hw.bus.bus_id, pf->hw.bus.device, pf->hw.bus.func); - /* If a client has already been registered, we need to add an instance - * of it to our new LAN device. - */ - if (registered_client) - i40e_client_add_instance(pf); + i40e_client_add_instance(pf); - /* Since in some cases register may have happened before a device gets - * added, we can schedule a subtask to go initiate the clients if - * they can be launched at probe time. - */ set_bit(__I40E_CLIENT_SERVICE_REQUESTED, pf->state); i40e_service_event_schedule(pf); @@ -449,9 +499,13 @@ int i40e_lan_add_device(struct i40e_pf *pf) **/ int i40e_lan_del_device(struct i40e_pf *pf) { + struct auxiliary_device *aux_dev = pf->cinst->lan_info.aux_dev; struct i40e_device *ldev, *tmp; int ret = -ENODEV; + auxiliary_device_delete(aux_dev); + auxiliary_device_uninit(aux_dev); + /* First, remove any client instance. */ i40e_client_del_instance(pf); @@ -732,6 +786,64 @@ static int i40e_client_update_vsi_ctxt(struct i40e_info *ldev, return err; } +int i40e_client_device_register(struct i40e_info *ldev) +{ + struct i40e_client *client; + struct i40e_pf *pf; + + if (!ldev) { + pr_err("Failed to reg client dev: ldev ptr NULL\n"); + return -EINVAL; + } + + client = ldev->client; + pf = ldev->pf; + if (!client) { + pr_err("Failed to reg client dev: client ptr NULL\n"); + return -EINVAL; + } + + if (!ldev->ops || !client->ops) { + pr_err("Failed to reg client dev: client dev peer_ops/ops NULL\n"); + return -EINVAL; + } + + pf->cinst->client = ldev->client; + set_bit(__I40E_CLIENT_SERVICE_REQUESTED, pf->state); + i40e_service_event_schedule(pf); + + return 0; +} +EXPORT_SYMBOL_GPL(i40e_client_device_register); + +void i40e_client_device_unregister(struct i40e_info *ldev) +{ + struct i40e_pf *pf = ldev->pf; + struct i40e_client_instance *cdev = pf->cinst; + + while (test_and_set_bit(__I40E_SERVICE_SCHED, pf->state)) + usleep_range(500, 1000); + + if (!cdev || !cdev->client || !cdev->client->ops || + !cdev->client->ops->close) { + dev_err(&pf->pdev->dev, "Cannot close client device\n"); + return; + } + if (test_bit(__I40E_CLIENT_INSTANCE_OPENED, &cdev->state)) { + cdev->client->ops->close(&cdev->lan_info, cdev->client, false); + clear_bit(__I40E_CLIENT_INSTANCE_OPENED, &cdev->state); + i40e_client_release_qvlist(&cdev->lan_info); + } + + pf->cinst->client = NULL; + clear_bit(__I40E_SERVICE_SCHED, pf->state); +} +EXPORT_SYMBOL_GPL(i40e_client_device_unregister); + +/* Retain these legacy global registration/unregistration calls till i40iw is + * removed from the kernel. The irdma unified driver does not use these + * exported symbols. + */ /** * i40e_register_client - Register a i40e client driver with the L2 driver * @client: pointer to the i40e_client struct diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c index 704e4748..9db1968 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_main.c +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c @@ -16270,6 +16270,7 @@ static void __exit i40e_exit_module(void) { pci_unregister_driver(&i40e_driver); destroy_workqueue(i40e_wq); + ida_destroy(&i40e_client_ida); i40e_dbg_exit(); } module_exit(i40e_exit_module);
Convert i40e to use the auxiliary bus infrastructure to export the RDMA functionality of the device to the RDMA driver. Register i40e client auxiliary RDMA device on the auxiliary bus per PCIe device function for the new auxiliary rdma driver (irdma) to attach to. The global i40e_register_client and i40e_unregister_client symbols will be obsoleted once irdma replaces i40iw in the kernel for the X722 device. Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com> --- drivers/net/ethernet/intel/Kconfig | 1 + drivers/net/ethernet/intel/i40e/i40e.h | 2 + drivers/net/ethernet/intel/i40e/i40e_client.c | 152 ++++++++++++++++++++++---- drivers/net/ethernet/intel/i40e/i40e_main.c | 1 + 4 files changed, 136 insertions(+), 20 deletions(-)