DriverDev-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Dan Carpenter <dan.carpenter@oracle.com>
To: Coiby Xu <coiby.xu@gmail.com>
Cc: devel@driverdev.osuosl.org,
	Willem de Bruijn <willemdebruijn.kernel@gmail.com>,
	"supporter:QLOGIC QLGE 10Gb ETHERNET DRIVER"
	<GR-Linux-NIC-Dev@marvell.com>,
	Manish Chopra <manishc@marvell.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Shung-Hsi Yu <shung-hsi.yu@suse.com>,
	open list <linux-kernel@vger.kernel.org>,
	Benjamin Poirier <benjamin.poirier@gmail.com>,
	"open list:QLOGIC QLGE 10Gb ETHERNET DRIVER"
	<netdev@vger.kernel.org>
Subject: Re: [PATCH v2 2/7] staging: qlge: Initialize devlink health dump framework
Date: Wed, 14 Oct 2020 16:08:46 +0300
Message-ID: <20201014130846.GU1042@kadam> (raw)
In-Reply-To: <20201014104306.63756-3-coiby.xu@gmail.com>

On Wed, Oct 14, 2020 at 06:43:01PM +0800, Coiby Xu wrote:
>  static int qlge_probe(struct pci_dev *pdev,
>  		      const struct pci_device_id *pci_entry)
>  {
>  	struct net_device *ndev = NULL;
>  	struct qlge_adapter *qdev = NULL;
> +	struct devlink *devlink;
>  	static int cards_found;
>  	int err = 0;
>  
> -	ndev = alloc_etherdev_mq(sizeof(struct qlge_adapter),
> +	devlink = devlink_alloc(&qlge_devlink_ops, sizeof(struct qlge_adapter));
> +	if (!devlink)
> +		return -ENOMEM;
> +
> +	qdev = devlink_priv(devlink);
> +
> +	ndev = alloc_etherdev_mq(sizeof(struct qlge_netdev_priv),
>  				 min(MAX_CPUS,
>  				     netif_get_num_default_rss_queues()));
>  	if (!ndev)
> -		return -ENOMEM;
> +		goto devlink_free;
>  
> -	err = qlge_init_device(pdev, ndev, cards_found);
> -	if (err < 0) {
> -		free_netdev(ndev);
> -		return err;

In the old code, if qlge_init_device() fails then it frees "ndev".

> -	}
> +	qdev->ndev = ndev;
> +	err = qlge_init_device(pdev, qdev, cards_found);
> +	if (err < 0)
> +		goto devlink_free;

But the patch introduces a new resource leak.

>  
> -	qdev = netdev_priv(ndev);
>  	SET_NETDEV_DEV(ndev, &pdev->dev);
>  	ndev->hw_features = NETIF_F_SG |
>  		NETIF_F_IP_CSUM |
> @@ -4611,8 +4619,14 @@ static int qlge_probe(struct pci_dev *pdev,
>  		qlge_release_all(pdev);
>  		pci_disable_device(pdev);
>  		free_netdev(ndev);
> -		return err;
> +		goto devlink_free;
>  	}
> +
> +	err = devlink_register(devlink, &pdev->dev);
> +	if (err)
> +		goto devlink_free;
> +
> +	qlge_health_create_reporters(qdev);
>  	/* Start up the timer to trigger EEH if
>  	 * the bus goes dead
>  	 */
> @@ -4623,6 +4637,10 @@ static int qlge_probe(struct pci_dev *pdev,
>  	atomic_set(&qdev->lb_count, 0);
>  	cards_found++;
>  	return 0;
> +
> +devlink_free:
> +	devlink_free(devlink);
> +	return err;
>  }

The best way to write error handling code is keep tracke of the most
recent allocation which was allocated successfully.

	one = alloc();
	if (!one)
		return -ENOMEM;  //  <-- nothing allocated successfully

	two = alloc();
	if (!two) {
		ret = -ENOMEM;
		goto free_one; // <-- one was allocated successfully
                               // Notice that the label name says what
			       // the goto does.
	}

	three = alloc();
	if (!three) {
		ret = -ENOMEM;
		goto free_two; // <-- two allocated, two freed.
	}

	...

	return 0;

free_two:
	free(two);
free_one:
	free(one);

	return ret;

In the old code qlge_probe() freed things before returning, and that's
fine if there is only two allocations in the function but when there are
three or more allocations, then use gotos to unwind.

Ideally there would be a ql_deinit_device() function to mirror the
ql_init_device() function.  The ql_init_device() is staging quality
code with leaks and bad label names.  It should be re-written to free
things one step at a time instead of calling ql_release_all().

Anyway, let's not introduce new leaks at least.

regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

  reply index

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-14 10:42 [PATCH v2 0/7] staging: qlge: Re-writing the debugging features Coiby Xu
2020-10-14 10:43 ` [PATCH v2 1/7] staging: qlge: replace ql_* with qlge_* to avoid namespace clashes with other qlogic drivers Coiby Xu
2020-10-15  1:01   ` Benjamin Poirier
2020-10-15  4:26     ` Coiby Xu
2020-10-15  4:32       ` Coiby Xu
2020-10-16 23:16     ` Coiby Xu
2020-10-18 11:02       ` Benjamin Poirier
2020-10-19  2:12         ` Coiby Xu
2020-10-14 10:43 ` [PATCH v2 2/7] staging: qlge: Initialize devlink health dump framework Coiby Xu
2020-10-14 13:08   ` Dan Carpenter [this message]
2020-10-15  4:22     ` Coiby Xu
2020-10-20  8:36   ` Shung-Hsi Yu
2020-10-20  8:55     ` Shung-Hsi Yu
2020-10-14 10:43 ` [PATCH v2 3/7] staging: qlge: coredump via devlink health reporter Coiby Xu
2020-10-14 10:43 ` [PATCH v2 4/7] staging: qlge: support force_coredump option for devlink health dump Coiby Xu
2020-10-14 10:43 ` [PATCH v2 5/7] staging: qlge: remove mpi_core_to_log which sends coredump to the kernel ring buffer Coiby Xu
2020-10-14 10:43 ` [PATCH v2 6/7] staging: qlge: clean up debugging code in the QL_ALL_DUMP ifdef land Coiby Xu
2020-10-14 10:43 ` [PATCH v2 7/7] staging: qlge: add documentation for debugging qlge Coiby Xu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201014130846.GU1042@kadam \
    --to=dan.carpenter@oracle.com \
    --cc=GR-Linux-NIC-Dev@marvell.com \
    --cc=benjamin.poirier@gmail.com \
    --cc=coiby.xu@gmail.com \
    --cc=devel@driverdev.osuosl.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=manishc@marvell.com \
    --cc=netdev@vger.kernel.org \
    --cc=shung-hsi.yu@suse.com \
    --cc=willemdebruijn.kernel@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DriverDev-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/driverdev-devel/0 driverdev-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 driverdev-devel driverdev-devel/ https://lore.kernel.org/driverdev-devel \
		driverdev-devel@linuxdriverproject.org devel@driverdev.osuosl.org
	public-inbox-index driverdev-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linuxdriverproject.driverdev-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git