From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1753377AbdGLP3l (ORCPT <rfc822;w@1wt.eu>);
        Wed, 12 Jul 2017 11:29:41 -0400
Received: from vps0.lunn.ch ([178.209.37.122]:52124 "EHLO vps0.lunn.ch"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1752744AbdGLP3i (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Wed, 12 Jul 2017 11:29:38 -0400
Date: Wed, 12 Jul 2017 17:29:33 +0200
From: Andrew Lunn <andrew@lunn.ch>
To: Aviad Krawczyk <aviad.krawczyk@huawei.com>
Cc: davem@davemloft.net, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org, bc.y@huawei.com, victor.gissin@huawei.com,
        zhaochen6@huawei.com, tony.qu@huawei.com
Subject: Re: [PATCH net 01/20] net/hinic: Initialize hw interface
Message-ID: <20170712152933.GF2557@lunn.ch>
References: <cover.1499865197.git.aviad.krawczyk@huawei.com>
 <281a172bd66298acb1a176a03a53710c1b777fb6.1499865197.git.aviad.krawczyk@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <281a172bd66298acb1a176a03a53710c1b777fb6.1499865197.git.aviad.krawczyk@huawei.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> +
> +#define HINIC_DRV_NAME		"HiNIC"
> +#define HINIC_DRV_VERSION	"1.0"

Hi Aviad

Please don't add a driver version. There was a discussion about this
recently, how pointless it is.

> +/**
> + * 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
> + **/
> +int hinic_init_hwdev(struct hinic_hwdev **hwdev, struct pci_dev *pdev)
> +{
> +	struct hinic_pfhwdev *pfhwdev;
> +	struct hinic_hwif *hwif;
> +	int err;
> +
> +	hwif = kzalloc(sizeof(*hwif), GFP_KERNEL);

Using the devm_ functions makes your cleanup code simpler when
handling memory.

> +/**
> + * nic_dev_init - Initialize the NIC device
> + * @pdev: the NIC pci device
> + *
> + * Return 0 - Success, negative - Failure
> + **/
> +static int nic_dev_init(struct pci_dev *pdev)
> +{
> +	struct hinic_dev *nic_dev;
> +	struct net_device *netdev;
> +	struct hinic_hwdev *hwdev;
> +	int err, num_qps;
> +
> +	err = hinic_init_hwdev(&hwdev, pdev);
> +	if (err) {
> +		dev_err(&pdev->dev, "Failed to initialize HW device\n");
> +		return err;
> +	}
> +
> +	num_qps = hinic_hwdev_num_qps(hwdev);
> +	if (num_qps <= 0) {
> +		dev_err(&pdev->dev, "Invalid number of QPS\n");
> +		err = -EINVAL;
> +		goto num_qps_err;
> +	}
> +
> +	netdev = alloc_etherdev_mq(sizeof(*nic_dev), num_qps);
> +	if (!netdev) {
> +		pr_err("Failed to allocate Ethernet device\n");

Above you used dev_err, here you used pr_err(). Please be consistent.

> +		err = -ENOMEM;
> +		goto alloc_etherdev_err;
> +	}
> +
> +	netdev->netdev_ops = &hinic_netdev_ops;
> +
> +	nic_dev = (struct hinic_dev *)netdev_priv(netdev);
> +	nic_dev->hwdev = hwdev;
> +	nic_dev->netdev = netdev;
> +	nic_dev->msg_enable = MSG_ENABLE_DEFAULT;
> +
> +	pci_set_drvdata(pdev, netdev);
> +
> +	netif_carrier_off(netdev);
> +
> +	err = register_netdev(netdev);
> +	if (err) {
> +		netif_err(nic_dev, probe, netdev, "Failed to register netdev\n");

probably not a good idea to use netif_err, if register_netdev just
failed. dev_err() would be better.