| Message ID | 1378193715-25328-5-git-send-email-wangyijing@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, 2013-09-03 at 15:35 +0800, Yijing Wang wrote: > use pcie_capability_read_word() to simplify code. > > Signed-off-by: Yijing Wang <wangyijing@huawei.com> > Cc: e1000-devel@lists.sourceforge.net > Cc: netdev@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > --- > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 ++---- > 1 files changed, 2 insertions(+), 4 deletions(-) Thanks Yijing, I will add it to my queue.
[+cc Jacob, Jeff] On Tue, Sep 03, 2013 at 03:35:13PM +0800, Yijing Wang wrote: > use pcie_capability_read_word() to simplify code. > > Signed-off-by: Yijing Wang <wangyijing@huawei.com> > Cc: e1000-devel@lists.sourceforge.net > Cc: netdev@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > --- > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 ++---- > 1 files changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > index bad8f14..bfa0b06 100644 > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > @@ -152,7 +152,6 @@ MODULE_VERSION(DRV_VERSION); > static int ixgbe_read_pci_cfg_word_parent(struct ixgbe_adapter *adapter, > u32 reg, u16 *value) > { > - int pos = 0; > struct pci_dev *parent_dev; > struct pci_bus *parent_bus; > > @@ -164,11 +163,10 @@ static int ixgbe_read_pci_cfg_word_parent(struct ixgbe_adapter *adapter, > if (!parent_dev) > return -1; > > - pos = pci_find_capability(parent_dev, PCI_CAP_ID_EXP); > - if (!pos) > + if (!pci_is_pcie(parent_dev)) > return -1; > > - pci_read_config_word(parent_dev, pos + reg, value); > + pcie_capability_read_word(parent_dev, reg, value); > return 0; > } > Here's the caller of ixgbe_read_pci_cfg_word_parent(): /* Get the negotiated link width and speed from PCI config space of the * parent, as this device is behind a switch */ err = ixgbe_read_pci_cfg_word_parent(adapter, 18, &link_status); This should be using PCI_EXP_LNKSTA instead of "18". But it would be even better if we could drop ixgbe_get_parent_bus_info() completely. It seems redundant after merging Jacob's new pcie_get_minimum_link() stuff [1]. ixgbe_disable_pcie_master() looks like it should be using pcie_capability_read_word() with PCI_EXP_DEVSTA instead of using IXGBE_PCI_DEVICE_STATUS. If fact, it looks like it could use the new pci_wait_for_pending_transaction() interface [2]. It looks like all the #defines in the "PCI Bus Info" block (IXGBE_PCI_DEVICE_STATUS, IXGBE_PCI_DEVICE_STATUS_TRANSACTION_PENDING, IXGBE_PCI_LINK_STATUS, etc.) [3] are really for PCIe-generic things. If so, the IXGBE-specific ones should be dropped in favor of the generic ones. [1] http://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c?id=e027d1aec4bb49030646d2c186a721f94372d7f2 [2] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/pci/pci.c?id=3775a209d38aa3a0c7ed89a7d0f529e0230f280e [3] http://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/tree/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h#n1833 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2013-09-04 at 10:20 -0600, Bjorn Helgaas wrote: > [+cc Jacob, Jeff] > > On Tue, Sep 03, 2013 at 03:35:13PM +0800, Yijing Wang wrote: > > use pcie_capability_read_word() to simplify code. > > > > Signed-off-by: Yijing Wang <wangyijing@huawei.com> > > Cc: e1000-devel@lists.sourceforge.net > > Cc: netdev@vger.kernel.org > > Cc: linux-kernel@vger.kernel.org > > --- > > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 ++---- > > 1 files changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > index bad8f14..bfa0b06 100644 > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > @@ -152,7 +152,6 @@ MODULE_VERSION(DRV_VERSION); > > static int ixgbe_read_pci_cfg_word_parent(struct ixgbe_adapter *adapter, > > u32 reg, u16 *value) > > { > > - int pos = 0; > > struct pci_dev *parent_dev; > > struct pci_bus *parent_bus; > > > > @@ -164,11 +163,10 @@ static int ixgbe_read_pci_cfg_word_parent(struct ixgbe_adapter *adapter, > > if (!parent_dev) > > return -1; > > > > - pos = pci_find_capability(parent_dev, PCI_CAP_ID_EXP); > > - if (!pos) > > + if (!pci_is_pcie(parent_dev)) > > return -1; > > > > - pci_read_config_word(parent_dev, pos + reg, value); > > + pcie_capability_read_word(parent_dev, reg, value); > > return 0; > > } > > > > Here's the caller of ixgbe_read_pci_cfg_word_parent(): > > /* Get the negotiated link width and speed from PCI config space of the > * parent, as this device is behind a switch > */ > err = ixgbe_read_pci_cfg_word_parent(adapter, 18, &link_status); > > This should be using PCI_EXP_LNKSTA instead of "18". Absolutely.. Not sure why I didn't do this originally..... > > But it would be even better if we could drop ixgbe_get_parent_bus_info() > completely. It seems redundant after merging Jacob's new > pcie_get_minimum_link() stuff [1]. I don't know if we can fully drop it. We need this in order to read the parent device on some quad port Ethernet adapters which have an internal PCIe switch to link two parts together. There are a few places we read the parent cfg word. At least one for sure.. but maybe others.. Can't recall. The parent bus info is still used to print out the slot width and speed. I don't know if it would be better to just print the response from get_minimum_link or still print the actual slot. > > ixgbe_disable_pcie_master() looks like it should be using > pcie_capability_read_word() with PCI_EXP_DEVSTA instead of using > IXGBE_PCI_DEVICE_STATUS. If fact, it looks like it could use the > new pci_wait_for_pending_transaction() interface [2]. > I can look at doing this. I know it was done this way for historic reasons, (and likely for code share with non Linux drivers.. but that's not really an excuse) > It looks like all the #defines in the "PCI Bus Info" block > (IXGBE_PCI_DEVICE_STATUS, IXGBE_PCI_DEVICE_STATUS_TRANSACTION_PENDING, > IXGBE_PCI_LINK_STATUS, etc.) [3] are really for PCIe-generic things. If > so, the IXGBE-specific ones should be dropped in favor of the generic > ones. Agreed. Thanks, Jake
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c index bad8f14..bfa0b06 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c @@ -152,7 +152,6 @@ MODULE_VERSION(DRV_VERSION); static int ixgbe_read_pci_cfg_word_parent(struct ixgbe_adapter *adapter, u32 reg, u16 *value) { - int pos = 0; struct pci_dev *parent_dev; struct pci_bus *parent_bus; @@ -164,11 +163,10 @@ static int ixgbe_read_pci_cfg_word_parent(struct ixgbe_adapter *adapter, if (!parent_dev) return -1; - pos = pci_find_capability(parent_dev, PCI_CAP_ID_EXP); - if (!pos) + if (!pci_is_pcie(parent_dev)) return -1; - pci_read_config_word(parent_dev, pos + reg, value); + pcie_capability_read_word(parent_dev, reg, value); return 0; }
use pcie_capability_read_word() to simplify code. Signed-off-by: Yijing Wang <wangyijing@huawei.com> Cc: e1000-devel@lists.sourceforge.net Cc: netdev@vger.kernel.org Cc: linux-kernel@vger.kernel.org --- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-)