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
| ||
|
Date: Tue, 11 Sep 2012 08:01:06 +0200 From: Francois Romieu <romieu@...zoreil.com> To: Sony Chacko <sony.chacko@...gic.com> Cc: davem@...emloft.net, netdev@...r.kernel.org, Dept_NX_Linux_NIC_Driver@...gic.com, Anirban Chakraborty <anirban.chakraborty@...gic.com> Subject: Re: [PATCH 01/12] qlcnic: Refactoring - template based hardware interface Sony Chacko <sony.chacko@...gic.com> : > Modify 82xx driver to support new adapter - Qlogic 83XX CNA > Separate adapter specific hardware accesses routines > Create template based hardware interface > > Signed-off-by: Anirban Chakraborty <anirban.chakraborty@...gic.com> > Signed-off-by: Sony Chacko <sony.chacko@...gic.com> > --- > drivers/net/ethernet/qlogic/qlcnic/qlcnic.h | 67 ++++- > drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c | 4 +- > .../net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c | 32 ++- > drivers/net/ethernet/qlogic/qlcnic/qlcnic_hdr.h | 55 +--- > drivers/net/ethernet/qlogic/qlcnic/qlcnic_hw.c | 125 ++++++-- > drivers/net/ethernet/qlogic/qlcnic/qlcnic_hw.h | 36 +++ > drivers/net/ethernet/qlogic/qlcnic/qlcnic_init.c | 1 + > drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c | 309 +++++++++----------- > 8 files changed, 359 insertions(+), 270 deletions(-) > create mode 100644 drivers/net/ethernet/qlogic/qlcnic/qlcnic_hw.h > > diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h b/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h > index eaa1db9..a4ae965 100644 > --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h > +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h > @@ -439,6 +439,8 @@ struct qlcnic_fw_dump { > struct qlcnic_dump_template_hdr *tmpl_hdr; > }; > > +struct qlcnic_hardware_ops; > + qlcnic_hardware_ops is defined in this same file and it only uses pointers to yet unknown types. You can define it here. [...] > @@ -552,8 +556,9 @@ struct qlcnic_recv_context { > /* HW context creation */ > > #define QLCNIC_OS_CRB_RETRY_COUNT 4000 > -#define QLCNIC_CDRP_SIGNATURE_MAKE(pcifn, version) \ > - (((pcifn) & 0xff) | (((version) & 0xff) << 8) | (0xcafe << 16)) > +#define QLCNIC_CDRP_SIGNATURE_MAKE(ahw) \ > + (((ahw->pci_func) & 0xff) | (((ahw->fw_hal_version) & \ > + 0xff) << 8) | (0xcafe << 16)) It could be a proper function and it could belong to the (only) file where it is actually used (once). [...] > +static inline void > +qlcnic_read_crb(struct qlcnic_adapter *adapter, char *buf, > + loff_t offset, size_t size) ? static inline void qlcnic_read_crb(struct qlcnic_adapter *adapter, char *buf, loff_t offset, size_t size) [...] > diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c > index 9e9e78a..c81fcf8 100644 > --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c > +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c [...] > @@ -603,8 +608,7 @@ qlcnic_set_pauseparam(struct net_device *netdev, > else > qlcnic_gb_unset_rx_flowctl(val); > > - QLCWR32(adapter, QLCNIC_NIU_GB_MAC_CONFIG_0(port), > - val); > + QLCWR32(adapter, QLCNIC_NIU_GB_MAC_CONFIG_0(port), val); This change does not fit the patch description. [...] > diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_hdr.h b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_hdr.h > index 28a6b28..0bee9f7 100644 > --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_hdr.h > +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_hdr.h [...] Most of the changes in this file does not fit the patch description. You could as well make a separate patch dedicated to removing unused stuff. 5...] > diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_hw.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_hw.c > index b528e52..ee4cd43 100644 > --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_hw.c > +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_hw.c [...] > @@ -880,7 +889,7 @@ qlcnic_pci_get_crb_addr_2M(struct qlcnic_adapter *adapter, > m = &crb_128M_2M_map[CRB_BLK(off)].sub_block[CRB_SUBBLK(off)]; > > if (m->valid && (m->start_128M <= off) && (m->end_128M > off)) { > - *addr = adapter->ahw->pci_base0 + m->start_2M + > + *addr = ahw->pci_base0 + m->start_2M + > (off - m->start_128M); *addr = ahw->pci_base0 + m->start_2M + off - m->start_128M; [...] > @@ -1105,7 +1116,7 @@ correct: > i = 0; > writel(TA_CTL_ENABLE, (mem_crb + TEST_AGT_CTRL)); > writel((TA_CTL_START | TA_CTL_ENABLE), > - (mem_crb + TEST_AGT_CTRL)); > + (mem_crb + TEST_AGT_CTRL)); writel(TA_CTL_START | TA_CTL_ENABLE, mem_crb + TEST_AGT_CTRL); The topic of the patch does not ask for a change here and these lines don't even appear in the context diff of such changes. It could be isolated in a separate patch (where you would probably be suggested to remove the useless parenthesis in "(TA_CTL_START | TA_CTL_ENABLE)" as well). [...] > @@ -1786,3 +1800,54 @@ error: > vfree(fw_dump->data); > return -EINVAL; > } > + > +void qlcnic_get_func_no(struct qlcnic_adapter *adapter) > +{ > + void __iomem *msix_base_addr; > + u32 func; > + u32 msix_base; > + > + pci_read_config_dword(adapter->pdev, QLCNIC_MSIX_TABLE_OFFSET, &func); > + msix_base_addr = adapter->ahw->pci_base0 + QLCNIC_MSIX_BASE; > + msix_base = readl(msix_base_addr); > + func = (func - msix_base)/QLCNIC_MSIX_TBL_PGSIZE; > + adapter->ahw->pci_func = func; > +} It is not used in this patch. #2 adds a declaration for it in qlcnic.h, a function pointer for it in qlcnic_hardware_ops, then uses it. #5 renames it to qlcnic_82xx_get_func_no. Reviewers won't complain if you keep the patchset at the smallest size for a given set of changes, especially as the qlcnic_82xx_ prefix is used a few lines below. > + > +void qlcnic_get_ocm_win(struct qlcnic_hardware_context *ahw) > +{ > + u32 addr; > + > + addr = QLCNIC_PCIX_PS_REG(PCIX_OCM_WINDOW_REG(ahw->pci_func)); > + ahw->ocm_win_crb = qlcnic_get_ioaddr(ahw, addr); > +} Either it's hidden behind some clever macro or it isn't used at all in the whole patchset. > + > +void qlcnic_82xx_read_crb(struct qlcnic_adapter *adapter, char *buf, > + loff_t offset, size_t size) > +{ > + u32 data; > + u64 qmdata; > + > + if (ADDR_IN_RANGE(offset, QLCNIC_PCI_CAMQM, QLCNIC_PCI_CAMQM_END)) { > + qlcnic_pci_camqm_read_2M(adapter, offset, &qmdata); > + memcpy(buf, &qmdata, size); > + } else { > + data = QLCRD32(adapter, offset); > + memcpy(buf, &data, size); > + } > +} Nit: reduce the scope of data and qmdata. [...] > diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_hw.h b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_hw.h > new file mode 100644 > index 0000000..7dab9e2 > --- /dev/null > +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_hw.h > @@ -0,0 +1,36 @@ > +#ifndef __QLCNIC_HW_H > +#define __QLCNIC_HW_H > + > +#include "qlcnic.h" > + > +/* List of PCI device IDs */ > +#define PCI_DEVICE_ID_QLOGIC_QLE824X 0x8020 > +#define QLCNIC_P3P_BAR0_LENGTH 0x00200000UL > + > +#define QLCNIC_BAR_LENGTH(dev_id, bar) \ > +do { \ > + if (dev_id == PCI_DEVICE_ID_QLOGIC_QLE824X)\ > + *bar = QLCNIC_P3P_BAR0_LENGTH; \ > + else \ > + *bar = 0; \ > +} while (0) It is used only once in the whole patchset. > diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_init.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_init.c > index 0bcda9c..350bf79 100644 > --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_init.c > +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_init.c [...] > @@ -343,9 +312,9 @@ static const struct net_device_ops qlcnic_netdev_failed_ops = { > }; > > static struct qlcnic_nic_template qlcnic_ops = { > - .config_bridged_mode = qlcnic_config_bridged_mode, > - .config_led = qlcnic_config_led, > - .start_firmware = qlcnic_start_firmware > + .config_bridged_mode = qlcnic_82xx_config_bridged_mode, > + .config_led = qlcnic_82xx_config_led, > + .start_firmware = qlcnic_82xx_start_firmware > }; You should really consider aligning as: static struct qlcnic_nic_template qlcnic_ops = { .config_bridged_mode = qlcnic_82xx_config_bridged_mode, .config_led = qlcnic_82xx_config_led, .start_firmware = qlcnic_82xx_start_firmware }; (off to work) -- Ueimor -- 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