| 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: Mon, 27 Jun 2011 13:38:25 -0700
From: "Matt Carlson" <mcarlson@...adcom.com>
To: "Jon Mason" <jdmason@...zu.us>,
"davem@...emloft.net" <davem@...emloft.net>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>
cc: "Matthew Carlson" <mcarlson@...adcom.com>
Subject: Re: [PATCH 15/19] tg3: remove unnecessary read of
PCI_CAP_ID_EXP
On Mon, Jun 27, 2011 at 01:23:04PM -0700, Jon Mason wrote:
> On Mon, Jun 27, 2011 at 1:42 PM, Matt Carlson <mcarlson@...adcom.com> wrote:
> > On Mon, Jun 27, 2011 at 10:47:34AM -0700, Jon Mason wrote:
> >> The PCIE capability offset is saved during PCI bus walking. Use the
> >> value from pci_dev instead of checking in the driver and saving it off
> >> the the driver specific structure. It will remove an unnecessary search
> >> in the PCI configuration space if this value is referenced instead of
> >> reacquiring it. Also, there is no need to set a driver specific flag to
> >> show whether the device is PCIE. pci_is_pcie can be used for this
> >> purpose (thus removing the need for this flag).
> >>
> >> Signed-off-by: Jon Mason <jdmason@...zu.us>
> >
> > I like the direction this is taking the code, but there are hidden dangers
> > here you might not be aware of. BCM5785 devices are effectively PCIe
> > devices, and should follow PCIe codepaths, but do not have a PCIe
> > capabilities section.
>
> Ah, that could cause a problem. Good catch!
>
> > Instead, can we keep the PCI_EXPRESS flag, but change all occurrances of
> > 'tp->pcie_cap' to either pci_pcie_cap(tp->pdev) or
> > pci_is_pcie(tp->pdev)? We should probably add the following next to the
> > TG3_FLAG_PCI_EXPRESS enumeration too:
> >
> > /* BCM5785 + pci_is_pcie() */
>
> Yes, I can revert that 1/2 of the patch, make the change above, and resubmit.
>
> FYI, this e-mail didn't CC netdev. You might wanna nack it there :)
Yes. You are right. Added netdev to the recipient list.
Consider it NAK'd. :)
> Thanks,
> Jon
>
> >
> >> ---
> >> drivers/net/tg3.c | 53 ++++++++++++++++++++++++-----------------------------
> >> drivers/net/tg3.h | 4 ----
> >> 2 files changed, 24 insertions(+), 33 deletions(-)
> >>
> >> diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
> >> index 97cd02d..5ecbc5c 100644
> >> --- a/drivers/net/tg3.c
> >> +++ b/drivers/net/tg3.c
> >> @@ -2679,11 +2679,11 @@ static int tg3_power_down_prepare(struct tg3 *tp)
> >> u16 lnkctl;
> >>
> >> pci_read_config_word(tp->pdev,
> >> - tp->pcie_cap + PCI_EXP_LNKCTL,
> >> + tp->pdev->pcie_cap + PCI_EXP_LNKCTL,
> >> &lnkctl);
> >> lnkctl |= PCI_EXP_LNKCTL_CLKREQ_EN;
> >> pci_write_config_word(tp->pdev,
> >> - tp->pcie_cap + PCI_EXP_LNKCTL,
> >> + tp->pdev->pcie_cap + PCI_EXP_LNKCTL,
> >> lnkctl);
> >> }
> >>
> >> @@ -3485,7 +3485,7 @@ relink:
> >> u16 oldlnkctl, newlnkctl;
> >>
> >> pci_read_config_word(tp->pdev,
> >> - tp->pcie_cap + PCI_EXP_LNKCTL,
> >> + tp->pdev->pcie_cap + PCI_EXP_LNKCTL,
> >> &oldlnkctl);
> >> if (tp->link_config.active_speed == SPEED_100 ||
> >> tp->link_config.active_speed == SPEED_10)
> >> @@ -3494,7 +3494,7 @@ relink:
> >> newlnkctl = oldlnkctl | PCI_EXP_LNKCTL_CLKREQ_EN;
> >> if (newlnkctl != oldlnkctl)
> >> pci_write_config_word(tp->pdev,
> >> - tp->pcie_cap + PCI_EXP_LNKCTL,
> >> + tp->pdev->pcie_cap + PCI_EXP_LNKCTL,
> >> newlnkctl);
> >> }
> >>
> >> @@ -4604,7 +4604,7 @@ static void tg3_dump_state(struct tg3 *tp)
> >> return;
> >> }
> >>
> >> - if (tg3_flag(tp, PCI_EXPRESS)) {
> >> + if (pci_is_pcie(tp->pdev)) {
> >> /* Read up to but not including private PCI registers */
> >> for (i = 0; i < TG3_PCIE_TLDLPL_PORT; i += sizeof(u32))
> >> regs[i / sizeof(u32)] = tr32(i);
> >> @@ -7064,7 +7064,7 @@ static void tg3_restore_pci_state(struct tg3 *tp)
> >> pci_write_config_word(tp->pdev, PCI_COMMAND, tp->pci_cmd);
> >>
> >> if (GET_ASIC_REV(tp->pci_chip_rev_id) != ASIC_REV_5785) {
> >> - if (tg3_flag(tp, PCI_EXPRESS))
> >> + if (pci_is_pcie(tp->pdev))
> >> pcie_set_readrq(tp->pdev, tp->pcie_readrq);
> >> else {
> >> pci_write_config_byte(tp->pdev, PCI_CACHE_LINE_SIZE,
> >> @@ -7172,7 +7172,7 @@ static int tg3_chip_reset(struct tg3 *tp)
> >> /* do the reset */
> >> val = GRC_MISC_CFG_CORECLK_RESET;
> >>
> >> - if (tg3_flag(tp, PCI_EXPRESS)) {
> >> + if (pci_is_pcie(tp->pdev)) {
> >> /* Force PCIe 1.0a mode */
> >> if (GET_ASIC_REV(tp->pci_chip_rev_id) != ASIC_REV_5785 &&
> >> !tg3_flag(tp, 57765_PLUS) &&
> >> @@ -7226,7 +7226,7 @@ static int tg3_chip_reset(struct tg3 *tp)
> >>
> >> udelay(120);
> >>
> >> - if (tg3_flag(tp, PCI_EXPRESS) && tp->pcie_cap) {
> >> + if (pci_is_pcie(tp->pdev)) {
> >> u16 val16;
> >>
> >> if (tp->pci_chip_rev_id == CHIPREV_ID_5750_A0) {
> >> @@ -7244,7 +7244,7 @@ static int tg3_chip_reset(struct tg3 *tp)
> >>
> >> /* Clear the "no snoop" and "relaxed ordering" bits. */
> >> pci_read_config_word(tp->pdev,
> >> - tp->pcie_cap + PCI_EXP_DEVCTL,
> >> + tp->pdev->pcie_cap + PCI_EXP_DEVCTL,
> >> &val16);
> >> val16 &= ~(PCI_EXP_DEVCTL_RELAX_EN |
> >> PCI_EXP_DEVCTL_NOSNOOP_EN);
> >> @@ -7255,14 +7255,14 @@ static int tg3_chip_reset(struct tg3 *tp)
> >> if (!tg3_flag(tp, CPMU_PRESENT))
> >> val16 &= ~PCI_EXP_DEVCTL_PAYLOAD;
> >> pci_write_config_word(tp->pdev,
> >> - tp->pcie_cap + PCI_EXP_DEVCTL,
> >> + tp->pdev->pcie_cap + PCI_EXP_DEVCTL,
> >> val16);
> >>
> >> pcie_set_readrq(tp->pdev, tp->pcie_readrq);
> >>
> >> /* Clear error status */
> >> pci_write_config_word(tp->pdev,
> >> - tp->pcie_cap + PCI_EXP_DEVSTA,
> >> + tp->pdev->pcie_cap + PCI_EXP_DEVSTA,
> >> PCI_EXP_DEVSTA_CED |
> >> PCI_EXP_DEVSTA_NFED |
> >> PCI_EXP_DEVSTA_FED |
> >> @@ -7325,7 +7325,7 @@ static int tg3_chip_reset(struct tg3 *tp)
> >>
> >> tg3_mdio_start(tp);
> >>
> >> - if (tg3_flag(tp, PCI_EXPRESS) &&
> >> + if (pci_is_pcie(tp->pdev) &&
> >> tp->pci_chip_rev_id != CHIPREV_ID_5750_A0 &&
> >> GET_ASIC_REV(tp->pci_chip_rev_id) != ASIC_REV_5785 &&
> >> !tg3_flag(tp, 57765_PLUS)) {
> >> @@ -8062,7 +8062,7 @@ static int tg3_reset_hw(struct tg3 *tp, int reset_phy)
> >> * chips and don't even touch the clocks if the CPMU is present.
> >> */
> >> if (!tg3_flag(tp, CPMU_PRESENT)) {
> >> - if (!tg3_flag(tp, PCI_EXPRESS))
> >> + if (!pci_is_pcie(tp->pdev))
> >> tp->pci_clock_ctrl |= CLOCK_CTRL_DELAY_PCI_GRANT;
> >> tw32_f(TG3PCI_CLOCK_CTRL, tp->pci_clock_ctrl);
> >> }
> >> @@ -8339,7 +8339,7 @@ static int tg3_reset_hw(struct tg3 *tp, int reset_phy)
> >> }
> >> }
> >>
> >> - if (tg3_flag(tp, PCI_EXPRESS))
> >> + if (pci_is_pcie(tp->pdev))
> >> rdmac_mode |= RDMAC_MODE_FIFO_LONG_BURST;
> >>
> >> if (tg3_flag(tp, HW_TSO_1) ||
> >> @@ -12873,7 +12873,7 @@ static void __devinit tg3_get_eeprom_hw_cfg(struct tg3 *tp)
> >> (cfg2 & NIC_SRAM_DATA_CFG_2_APD_EN))
> >> tp->phy_flags |= TG3_PHYFLG_ENABLE_APD;
> >>
> >> - if (tg3_flag(tp, PCI_EXPRESS) &&
> >> + if (pci_is_pcie(tp->pdev) &&
> >> GET_ASIC_REV(tp->pci_chip_rev_id) != ASIC_REV_5785 &&
> >> !tg3_flag(tp, 57765_PLUS)) {
> >> u32 cfg3;
> >> @@ -13777,12 +13777,9 @@ static int __devinit tg3_get_invariants(struct tg3 *tp)
> >> pci_read_config_dword(tp->pdev, TG3PCI_PCISTATE,
> >> &pci_state_reg);
> >>
> >> - tp->pcie_cap = pci_find_capability(tp->pdev, PCI_CAP_ID_EXP);
> >> - if (tp->pcie_cap != 0) {
> >> + if (pci_is_pcie(tp->pdev)) {
> >> u16 lnkctl;
> >>
> >> - tg3_flag_set(tp, PCI_EXPRESS);
> >> -
> >> tp->pcie_readrq = 4096;
> >> if (GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5719 ||
> >> GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5720)
> >> @@ -13791,7 +13788,7 @@ static int __devinit tg3_get_invariants(struct tg3 *tp)
> >> pcie_set_readrq(tp->pdev, tp->pcie_readrq);
> >>
> >> pci_read_config_word(tp->pdev,
> >> - tp->pcie_cap + PCI_EXP_LNKCTL,
> >> + tp->pdev->pcie_cap + PCI_EXP_LNKCTL,
> >> &lnkctl);
> >> if (lnkctl & PCI_EXP_LNKCTL_CLKREQ_EN) {
> >> if (GET_ASIC_REV(tp->pci_chip_rev_id) ==
> >> @@ -13807,8 +13804,6 @@ static int __devinit tg3_get_invariants(struct tg3 *tp)
> >> } else if (tp->pci_chip_rev_id == CHIPREV_ID_5717_A0) {
> >> tg3_flag_set(tp, L1PLLPD_EN);
> >> }
> >> - } else if (GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5785) {
> >> - tg3_flag_set(tp, PCI_EXPRESS);
> >> } else if (!tg3_flag(tp, 5705_PLUS) ||
> >> tg3_flag(tp, 5780_CLASS)) {
> >> tp->pcix_cap = pci_find_capability(tp->pdev, PCI_CAP_ID_PCIX);
> >> @@ -13829,7 +13824,7 @@ static int __devinit tg3_get_invariants(struct tg3 *tp)
> >> * posted to the chip in order.
> >> */
> >> if (pci_dev_present(tg3_write_reorder_chipsets) &&
> >> - !tg3_flag(tp, PCI_EXPRESS))
> >> + !pci_is_pcie(tp->pdev))
> >> tg3_flag_set(tp, MBOX_WRITE_REORDER);
> >>
> >> pci_read_config_byte(tp->pdev, PCI_CACHE_LINE_SIZE,
> >> @@ -13903,7 +13898,7 @@ static int __devinit tg3_get_invariants(struct tg3 *tp)
> >> if (tg3_flag(tp, PCIX_TARGET_HWBUG))
> >> tp->write32 = tg3_write_indirect_reg32;
> >> else if (GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5701 ||
> >> - (tg3_flag(tp, PCI_EXPRESS) &&
> >> + (pci_is_pcie(tp->pdev) &&
> >> tp->pci_chip_rev_id == CHIPREV_ID_5750_A0)) {
> >> /*
> >> * Back to back register writes can cause problems on these
> >> @@ -14390,7 +14385,7 @@ static u32 __devinit tg3_calc_dma_bndry(struct tg3 *tp, u32 val)
> >> */
> >> if (GET_ASIC_REV(tp->pci_chip_rev_id) != ASIC_REV_5700 &&
> >> GET_ASIC_REV(tp->pci_chip_rev_id) != ASIC_REV_5701 &&
> >> - !tg3_flag(tp, PCI_EXPRESS))
> >> + !pci_is_pcie(tp->pdev))
> >> goto out;
> >>
> >> #if defined(CONFIG_PPC64) || defined(CONFIG_IA64) || defined(CONFIG_PARISC)
> >> @@ -14422,7 +14417,7 @@ static u32 __devinit tg3_calc_dma_bndry(struct tg3 *tp, u32 val)
> >> * other than 5700 and 5701 which do not implement the
> >> * boundary bits.
> >> */
> >> - if (tg3_flag(tp, PCIX_MODE) && !tg3_flag(tp, PCI_EXPRESS)) {
> >> + if (tg3_flag(tp, PCIX_MODE) && !pci_is_pcie(tp->pdev)) {
> >> switch (cacheline_size) {
> >> case 16:
> >> case 32:
> >> @@ -14447,7 +14442,7 @@ static u32 __devinit tg3_calc_dma_bndry(struct tg3 *tp, u32 val)
> >> DMA_RWCTRL_WRITE_BNDRY_384_PCIX);
> >> break;
> >> }
> >> - } else if (tg3_flag(tp, PCI_EXPRESS)) {
> >> + } else if (pci_is_pcie(tp->pdev)) {
> >> switch (cacheline_size) {
> >> case 16:
> >> case 32:
> >> @@ -14622,7 +14617,7 @@ static int __devinit tg3_test_dma(struct tg3 *tp)
> >> if (tg3_flag(tp, 57765_PLUS))
> >> goto out;
> >>
> >> - if (tg3_flag(tp, PCI_EXPRESS)) {
> >> + if (pci_is_pcie(tp->pdev)) {
> >> /* DMA read watermark not used on PCIE */
> >> tp->dma_rwctrl |= 0x00180000;
> >> } else if (!tg3_flag(tp, PCIX_MODE)) {
> >> @@ -14880,7 +14875,7 @@ static char * __devinit tg3_phy_string(struct tg3 *tp)
> >>
> >> static char * __devinit tg3_bus_string(struct tg3 *tp, char *str)
> >> {
> >> - if (tg3_flag(tp, PCI_EXPRESS)) {
> >> + if (pci_is_pcie(tp->pdev)) {
> >> strcpy(str, "PCI Express");
> >> return str;
> >> } else if (tg3_flag(tp, PCIX_MODE)) {
> >> diff --git a/drivers/net/tg3.h b/drivers/net/tg3.h
> >> index bedc3b4..ede661f 100644
> >> --- a/drivers/net/tg3.h
> >> +++ b/drivers/net/tg3.h
> >> @@ -2857,7 +2857,6 @@ enum TG3_FLAGS {
> >> TG3_FLAG_IS_5788,
> >> TG3_FLAG_MAX_RXPEND_64,
> >> TG3_FLAG_TSO_CAPABLE,
> >> - TG3_FLAG_PCI_EXPRESS,
> >> TG3_FLAG_ASF_NEW_HANDSHAKE,
> >> TG3_FLAG_HW_AUTONEG,
> >> TG3_FLAG_IS_NIC,
> >> @@ -3022,10 +3021,7 @@ struct tg3 {
> >>
> >> int pm_cap;
> >> int msi_cap;
> >> - union {
> >> int pcix_cap;
> >> - int pcie_cap;
> >> - };
> >> int pcie_readrq;
> >>
> >> struct mii_bus *mdio_bus;
> >> --
> >> 1.7.5.4
> >>
> >>
> >
> >
>
--
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