diff mbox

[5/7] ixgbe: use pcie_capability_read_word() to simplify code

Message ID 1378193715-25328-5-git-send-email-wangyijing@huawei.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Yijing Wang Sept. 3, 2013, 7:35 a.m. UTC
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(-)

Comments

Kirsher, Jeffrey T Sept. 4, 2013, 9:26 a.m. UTC | #1
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.
Bjorn Helgaas Sept. 4, 2013, 4:20 p.m. UTC | #2
[+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
Jacob Keller Sept. 4, 2013, 5:24 p.m. UTC | #3
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 mbox

Patch

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;
 }