| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20140403211254.GA21931@google.com>
Date: Thu, 3 Apr 2014 15:12:54 -0600
From: Bjorn Helgaas <bhelgaas@...gle.com>
To: Wei Yang <weiyang@...ux.vnet.ibm.com>
Cc: netdev <netdev@...r.kernel.org>, yevgenyp@...lanox.com,
Or Gerlitz <ogerlitz@...lanox.com>,
Amir Vadai <amirv@...lanox.com>,
"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
David Miller <davem@...emloft.net>
Subject: Re: [PATCH] net/mlx4_core: match pci_device_id including dynids
On Thu, Apr 03, 2014 at 02:54:32PM +0800, Wei Yang wrote:
> Here is another one based on your comment, which split mlx4_remove_one into
> two and named a helper __mlx4_remove_one(). mlx4_pci_err_detected() will just
> call __mlx4_remove_one(), which will not release drvdata.
>
> BTW, this is not tested, just want to make sure my understanding is correct.
A couple minor comments below, but in general, yes, this is what I was
thinking.
> From 84a5a9df0604cbea9b70c74b0709258841637946 Mon Sep 17 00:00:00 2001
> From: Wei Yang <weiyang@...ux.vnet.ibm.com>
> Date: Mon, 31 Mar 2014 11:34:57 +0800
> Subject: [PATCH] net/mlx4_core: match pci_device_id including dynids
>
> Fix issue introduced by commit: 97a5221 "net/mlx4_core: pass
> pci_device_id.driver_data to __mlx4_init_one during reset".
>
> pci_match_id() just match the static pci_device_id, which may return NULL if
> someone binds the driver to a device manually using
> /sys/bus/pci/drivers/.../new_id.
>
> This patch wrap up a helper function __mlx4_remove_one() which does the tear
> down function but preserve the drv_data. Functions like
> mlx4_pci_err_detected() and mlx4_restart_one() will call this one with out
> releasing drvdata.
>
> CC: Bjorn Helgaas <bhelgaas@...gle.com>
> CC: Amir Vadai <amirv@...lanox.com>
> Signed-off-by: Wei Yang <weiyang@...ux.vnet.ibm.com>
> Acked-by: Amir Vadai <amirv@...lanox.com>
> ---
> drivers/net/ethernet/mellanox/mlx4/main.c | 149 ++++++++++++++++-------------
> 1 file changed, 80 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c
> index aa54ef7..fd1f288 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/main.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/main.c
> @@ -2268,7 +2268,12 @@ static int __mlx4_init_one(struct pci_dev *pdev, int pci_dev_data)
> /* Allow large DMA segments, up to the firmware limit of 1 GB */
> dma_set_max_seg_size(&pdev->dev, 1024 * 1024 * 1024);
>
> - priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> + dev = pci_get_drvdata(pdev);
> + if (!dev)
> + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> + else
> + priv = mlx4_priv(dev);
Why don't you move the priv kzalloc into mlx4_init_one()? Then it would be
symmetric -- you alloc and call pci_set_drvdata() in mlx4_init_one(), and
you call pci_set_drvdata(NULL) and free it in mlx4_remove_one(). And you
wouldn't need the test here.
> +
> if (!priv) {
> err = -ENOMEM;
> goto err_release_regions;
> @@ -2525,77 +2530,81 @@ static int mlx4_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
> return __mlx4_init_one(pdev, id->driver_data);
> }
>
> -static void mlx4_remove_one(struct pci_dev *pdev)
> +static void __mlx4_remove_one(struct mlx4_dev *dev)
> {
> - struct mlx4_dev *dev = pci_get_drvdata(pdev);
> - struct mlx4_priv *priv = mlx4_priv(dev);
> - int p;
> + /* in SRIOV it is not allowed to unload the pf's
> + * driver while there are alive vf's */
> + if (mlx4_is_master(dev)) {
> + if (mlx4_how_many_lives_vf(dev))
> + printk(KERN_ERR "Removing PF when there are assigned VF's !!!\n");
> + }
> + mlx4_stop_sense(dev);
> + mlx4_unregister_device(dev);
>
> - if (dev) {
> - /* in SRIOV it is not allowed to unload the pf's
> - * driver while there are alive vf's */
> - if (mlx4_is_master(dev)) {
> - if (mlx4_how_many_lives_vf(dev))
> - printk(KERN_ERR "Removing PF when there are assigned VF's !!!\n");
> - }
> - mlx4_stop_sense(dev);
> - mlx4_unregister_device(dev);
> + for (p = 1; p <= dev->caps.num_ports; p++) {
> + mlx4_cleanup_port_info(&priv->port[p]);
> + mlx4_CLOSE_PORT(dev, p);
> + }
>
> - for (p = 1; p <= dev->caps.num_ports; p++) {
> - mlx4_cleanup_port_info(&priv->port[p]);
> - mlx4_CLOSE_PORT(dev, p);
> - }
> + if (mlx4_is_master(dev))
> + mlx4_free_resource_tracker(dev,
> + RES_TR_FREE_SLAVES_ONLY);
>
> - if (mlx4_is_master(dev))
> - mlx4_free_resource_tracker(dev,
> - RES_TR_FREE_SLAVES_ONLY);
> -
> - mlx4_cleanup_counters_table(dev);
> - mlx4_cleanup_qp_table(dev);
> - mlx4_cleanup_srq_table(dev);
> - mlx4_cleanup_cq_table(dev);
> - mlx4_cmd_use_polling(dev);
> - mlx4_cleanup_eq_table(dev);
> - mlx4_cleanup_mcg_table(dev);
> - mlx4_cleanup_mr_table(dev);
> - mlx4_cleanup_xrcd_table(dev);
> - mlx4_cleanup_pd_table(dev);
> + mlx4_cleanup_counters_table(dev);
> + mlx4_cleanup_qp_table(dev);
> + mlx4_cleanup_srq_table(dev);
> + mlx4_cleanup_cq_table(dev);
> + mlx4_cmd_use_polling(dev);
> + mlx4_cleanup_eq_table(dev);
> + mlx4_cleanup_mcg_table(dev);
> + mlx4_cleanup_mr_table(dev);
> + mlx4_cleanup_xrcd_table(dev);
> + mlx4_cleanup_pd_table(dev);
>
> - if (mlx4_is_master(dev))
> - mlx4_free_resource_tracker(dev,
> - RES_TR_FREE_STRUCTS_ONLY);
> -
> - iounmap(priv->kar);
> - mlx4_uar_free(dev, &priv->driver_uar);
> - mlx4_cleanup_uar_table(dev);
> - if (!mlx4_is_slave(dev))
> - mlx4_clear_steering(dev);
> - mlx4_free_eq_table(dev);
> - if (mlx4_is_master(dev))
> - mlx4_multi_func_cleanup(dev);
> - mlx4_close_hca(dev);
> - if (mlx4_is_slave(dev))
> - mlx4_multi_func_cleanup(dev);
> - mlx4_cmd_cleanup(dev);
> -
> - if (dev->flags & MLX4_FLAG_MSI_X)
> - pci_disable_msix(pdev);
> - if (dev->flags & MLX4_FLAG_SRIOV) {
> - mlx4_warn(dev, "Disabling SR-IOV\n");
> - pci_disable_sriov(pdev);
> - }
> + if (mlx4_is_master(dev))
> + mlx4_free_resource_tracker(dev,
> + RES_TR_FREE_STRUCTS_ONLY);
>
> - if (!mlx4_is_slave(dev))
> - mlx4_free_ownership(dev);
> + iounmap(priv->kar);
> + mlx4_uar_free(dev, &priv->driver_uar);
> + mlx4_cleanup_uar_table(dev);
> + if (!mlx4_is_slave(dev))
> + mlx4_clear_steering(dev);
> + mlx4_free_eq_table(dev);
> + if (mlx4_is_master(dev))
> + mlx4_multi_func_cleanup(dev);
> + mlx4_close_hca(dev);
> + if (mlx4_is_slave(dev))
> + mlx4_multi_func_cleanup(dev);
> + mlx4_cmd_cleanup(dev);
> +
> + if (dev->flags & MLX4_FLAG_MSI_X)
> + pci_disable_msix(pdev);
> + if (dev->flags & MLX4_FLAG_SRIOV) {
> + mlx4_warn(dev, "Disabling SR-IOV\n");
> + pci_disable_sriov(pdev);
> + }
> +
> + if (!mlx4_is_slave(dev))
> + mlx4_free_ownership(dev);
> +
> + kfree(dev->caps.qp0_tunnel);
> + kfree(dev->caps.qp0_proxy);
> + kfree(dev->caps.qp1_tunnel);
> + kfree(dev->caps.qp1_proxy);
> + pci_release_regions(pdev);
> + pci_disable_device(pdev);
> +}
>
> - kfree(dev->caps.qp0_tunnel);
> - kfree(dev->caps.qp0_proxy);
> - kfree(dev->caps.qp1_tunnel);
> - kfree(dev->caps.qp1_proxy);
> +static void mlx4_remove_one(struct pci_dev *pdev)
> +{
> + struct mlx4_dev *dev = pci_get_drvdata(pdev);
> + struct mlx4_priv *priv = mlx4_priv(dev);
> + int p;
>
> + if (dev) {
I don't think you should test "dev" here. What scenario is there where
mlx4_remove_one() would be called with a pci_dev that has no drvdata?
> + __mlx4_remove_one(dev);
> kfree(priv);
> - pci_release_regions(pdev);
> - pci_disable_device(pdev);
> pci_set_drvdata(pdev, NULL);
> }
> }
> @@ -2607,7 +2616,7 @@ int mlx4_restart_one(struct pci_dev *pdev)
> int pci_dev_data;
>
> pci_dev_data = priv->pci_dev_data;
> - mlx4_remove_one(pdev);
> + __mlx4_remove_one(pdev);
> return __mlx4_init_one(pdev, pci_dev_data);
> }
>
> @@ -2662,7 +2671,7 @@ MODULE_DEVICE_TABLE(pci, mlx4_pci_table);
> static pci_ers_result_t mlx4_pci_err_detected(struct pci_dev *pdev,
> pci_channel_state_t state)
> {
> - mlx4_remove_one(pdev);
> + __mlx4_remove_one(pdev);
>
> return state == pci_channel_io_perm_failure ?
> PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_NEED_RESET;
> @@ -2670,11 +2679,13 @@ static pci_ers_result_t mlx4_pci_err_detected(struct pci_dev *pdev,
>
> static pci_ers_result_t mlx4_pci_slot_reset(struct pci_dev *pdev)
> {
> - const struct pci_device_id *id;
> - int ret;
> + struct mlx4_dev *dev = pci_get_drvdata(pdev);
> + struct mlx4_priv *priv = mlx4_priv(dev);
> + int pci_dev_data;
> + int ret;
>
> - id = pci_match_id(mlx4_pci_table, pdev);
> - ret = __mlx4_init_one(pdev, id->driver_data);
> + pci_dev_data = priv->pci_dev_data;
> + ret = __mlx4_init_one(pdev, pci_dev_data);
>
> return ret ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED;
> }
> --
> 1.7.9.5
>
>
> --
> Richard Yang
> Help you, Help me
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists