Re: [PATCH 5/7] ixgbe: use pcie_capability_read_word() to simplifycode

[Keller, Jacob E]

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@xxxxxxxxxx>
> > Cc: e1000-devel@xxxxxxxxxxxxxxxxxxxxx
> > Cc: netdev@xxxxxxxxxxxxxxx
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > ---
> >  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


¢éì®&Þ~º&¶¬–+-±éÝ¥Šw®žË±Êâmébžìdz¹Þ)í…æèw*jg¬±¨¶‰šŽŠÝj/êäz¹ÞŠà2ŠÞ¨è­Ú&¢)ß«a¶Úþø®G«éh®æj:+v‰¨Šwè†Ù>Wš±êÞiÛaxPjØm¶ŸÿÃ-»+ƒùdš_