From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Neftin, Sasha" Subject: Re: [net-next 01/11] igc: Add skeletal frame for Intel(R) 2.5G Ethernet Controller support Date: Wed, 24 Oct 2018 11:50:31 +0300 Message-ID: <7408c5f9-559d-7bda-d4e4-ab8ee2a244fd@intel.com> References: <20181017222322.2171-1-jeffrey.t.kirsher@intel.com> <20181017222322.2171-2-jeffrey.t.kirsher@intel.com> <20181018101405.4998dc01@cakuba.netronome.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: davem@davemloft.net, netdev@vger.kernel.org, nhorman@redhat.com, sassmann@redhat.com, sasha.neftin@intel.com To: Jakub Kicinski , Jeff Kirsher Return-path: Received: from mga11.intel.com ([192.55.52.93]:64955 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727591AbeJXRRr (ORCPT ); Wed, 24 Oct 2018 13:17:47 -0400 In-Reply-To: <20181018101405.4998dc01@cakuba.netronome.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 10/18/2018 20:14, Jakub Kicinski wrote: > On Wed, 17 Oct 2018 15:23:12 -0700, Jeff Kirsher wrote: >> From: Sasha Neftin >> >> This patch adds the beginning framework onto which I am going to add >> the igc driver which supports the Intel(R) I225-LM/I225-V 2.5G >> Ethernet Controller. >> >> Signed-off-by: Sasha Neftin >> Tested-by: Aaron Brown >> Signed-off-by: Jeff Kirsher > > bunch of minor nit picks > >> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h >> new file mode 100644 >> index 000000000000..afe595cfcf63 >> --- /dev/null >> +++ b/drivers/net/ethernet/intel/igc/igc.h >> @@ -0,0 +1,29 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* Copyright (c) 2018 Intel Corporation */ >> + >> +#ifndef _IGC_H_ >> +#define _IGC_H_ >> + >> +#include >> + >> +#include >> +#include >> +#include >> + >> +#include >> + >> +#include >> + >> +#define IGC_ERR(args...) pr_err("igc: " args) > > Looks like you're reimplementing pr_fmt() > yes, it is convenient for a debug. Same methodological approach I saw in other drivers. >> +#define PFX "igc: " >> + >> +#include >> +#include >> +#include > > Splitting the includes looks fairly weird. Also, you're not using any > of them here. > Good catch. I'll remove splits and unused "include"'s. All "include"'s will be add per demand. I will send patch to fix that. >> +/* main */ >> +extern char igc_driver_name[]; >> +extern char igc_driver_version[]; >> + >> +#endif /* _IGC_H_ */ >> diff --git a/drivers/net/ethernet/intel/igc/igc_hw.h b/drivers/net/ethernet/intel/igc/igc_hw.h >> new file mode 100644 >> index 000000000000..aa68b4516700 >> --- /dev/null >> +++ b/drivers/net/ethernet/intel/igc/igc_hw.h >> @@ -0,0 +1,10 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* Copyright (c) 2018 Intel Corporation */ >> + >> +#ifndef _IGC_HW_H_ >> +#define _IGC_HW_H_ >> + >> +#define IGC_DEV_ID_I225_LM 0x15F2 >> +#define IGC_DEV_ID_I225_V 0x15F3 >> + >> +#endif /* _IGC_HW_H_ */ >> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c >> new file mode 100644 >> index 000000000000..753749ce5ae0 >> --- /dev/null >> +++ b/drivers/net/ethernet/intel/igc/igc_main.c >> @@ -0,0 +1,146 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* Copyright (c) 2018 Intel Corporation */ >> + >> +#include >> +#include >> + >> +#include "igc.h" >> +#include "igc_hw.h" >> + >> +#define DRV_VERSION "0.0.1-k" > > You can just use the kernel version, it works pretty well in presence > of backports. > Good idea, I think I will do it too. >> +#define DRV_SUMMARY "Intel(R) 2.5G Ethernet Linux Driver" >> + >> +MODULE_AUTHOR("Intel Corporation, "); >> +MODULE_DESCRIPTION(DRV_SUMMARY); >> +MODULE_LICENSE("GPL v2"); >> +MODULE_VERSION(DRV_VERSION); >> + >> +char igc_driver_name[] = "igc"; >> +char igc_driver_version[] = DRV_VERSION; >> +static const char igc_driver_string[] = DRV_SUMMARY; >> +static const char igc_copyright[] = >> + "Copyright(c) 2018 Intel Corporation."; >> + >> +static const struct pci_device_id igc_pci_tbl[] = { >> + { PCI_VDEVICE(INTEL, IGC_DEV_ID_I225_LM) }, >> + { PCI_VDEVICE(INTEL, IGC_DEV_ID_I225_V) }, >> + /* required last entry */ >> + {0, } >> +}; >> + >> +MODULE_DEVICE_TABLE(pci, igc_pci_tbl); >> + >> +/** >> + * igc_probe - Device Initialization Routine >> + * @pdev: PCI device information struct >> + * @ent: entry in igc_pci_tbl >> + * >> + * Returns 0 on success, negative on failure >> + * >> + * igc_probe initializes an adapter identified by a pci_dev structure. >> + * The OS initialization, configuring the adapter private structure, >> + * and a hardware reset occur. >> + */ >> +static int igc_probe(struct pci_dev *pdev, >> + const struct pci_device_id *ent) >> +{ >> + int err, pci_using_dac; >> + >> + err = pci_enable_device_mem(pdev); >> + if (err) >> + return err; >> + >> + pci_using_dac = 0; >> + err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(64)); >> + if (!err) { >> + err = dma_set_coherent_mask(&pdev->dev, >> + DMA_BIT_MASK(64)); >> + if (!err) >> + pci_using_dac = 1; > > You never use this pci_using_dac. dma_set_mask_and_coherent() maybe? > Right. I saw already somebody sent out patch to fix that. >> + } else { >> + err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32)); >> + if (err) { >> + err = dma_set_coherent_mask(&pdev->dev, >> + DMA_BIT_MASK(32)); >> + if (err) { >> + IGC_ERR("Wrong DMA configuration, aborting\n"); >> + goto err_dma; >> + } >> + } >> + } >> + >> + err = pci_request_selected_regions(pdev, >> + pci_select_bars(pdev, >> + IORESOURCE_MEM), >> + igc_driver_name); >> + if (err) >> + goto err_pci_reg; >> + >> + pci_set_master(pdev); >> + err = pci_save_state(pdev); > > And you never look at err? > Same as above. >> + return 0; >> + >> +err_pci_reg: >> +err_dma: > > The error label should be named after what it points to, not where it's > coming from. > >> + pci_disable_device(pdev); >> + return err; >> +} >> + >> +/** >> + * igc_remove - Device Removal Routine >> + * @pdev: PCI device information struct >> + * >> + * igc_remove is called by the PCI subsystem to alert the driver >> + * that it should release a PCI device. This could be caused by a >> + * Hot-Plug event, or because the driver is going to be removed from >> + * memory. >> + */ >> +static void igc_remove(struct pci_dev *pdev) >> +{ >> + pci_release_selected_regions(pdev, >> + pci_select_bars(pdev, IORESOURCE_MEM)); >> + >> + pci_disable_device(pdev); >> +} >> + >> +static struct pci_driver igc_driver = { >> + .name = igc_driver_name, >> + .id_table = igc_pci_tbl, >> + .probe = igc_probe, >> + .remove = igc_remove, >> +}; >> + >> +/** >> + * igc_init_module - Driver Registration Routine >> + * >> + * igc_init_module is the first routine called when the driver is >> + * loaded. All it does is register with the PCI subsystem. >> + */ >> +static int __init igc_init_module(void) >> +{ >> + int ret; >> + >> + pr_info("%s - version %s\n", >> + igc_driver_string, igc_driver_version); >> + >> + pr_info("%s\n", igc_copyright); >> + >> + ret = pci_register_driver(&igc_driver); >> + return ret; > > Why the variable? > we can return 'pci_register_driver' here, but variable is more clearly. >> +} >> + >> +module_init(igc_init_module); >> + >> +/** >> + * igc_exit_module - Driver Exit Cleanup Routine >> + * >> + * igc_exit_module is called just before the driver is removed >> + * from memory. >> + */ >> +static void __exit igc_exit_module(void) >> +{ >> + pci_unregister_driver(&igc_driver); >> +} >> + >> +module_exit(igc_exit_module); >> +/* igc_main.c */ > > I'd argue most editors make it fairly clear which file one is > editing, hence making this sort of comments entirely superfluous :) > Thanks for your comments.