[rfc,1/3,v2,E1000-devel,rfc,1/4] igb: Add igb_cleanup_vf()

Move virtual finction cleanup code into igb_cleanup_vf() and for the sake
of symmetry rename igb_probe_vfs() as igb_init_vf().

Although these functions aren't entirely symmetrical it should aid
maintenance by making the relationship between initialisation and cleanup
more obvious.

Note that there appears to be no way for adapter->vfs_allocated_count to be
non-zero for the case where CONFIG_PCI_IOV is not set, so reseting this
value was moved to inside the relvant #ifdef.

Signed-off-by: Simon Horman <horms@verge.net.au>

--- 
Thu, 05 Nov 2009 11:58:48 +1100
* Initial post

Wed, 25 Nov 2009 15:43:45 +1100
* Actually remove adapter->vfs_allocated_count = 0 from outside of
  igb_cleanup_vf()
* Up-port to current net-next

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Nov 24, 2009 at 22:32, Simon Horman <horms@verge.net.au> wrote:
> Move virtual finction cleanup code into igb_cleanup_vf() and for the sake
> of symmetry rename igb_probe_vfs() as igb_init_vf().
>
> Although these functions aren't entirely symmetrical it should aid
> maintenance by making the relationship between initialisation and cleanup
> more obvious.
>
> Note that there appears to be no way for adapter->vfs_allocated_count to be
> non-zero for the case where CONFIG_PCI_IOV is not set, so reseting this
> value was moved to inside the relvant #ifdef.
>
> Signed-off-by: Simon Horman <horms@verge.net.au>
>
> ---
> Thu, 05 Nov 2009 11:58:48 +1100
> * Initial post
>
> Wed, 25 Nov 2009 15:43:45 +1100
> * Actually remove adapter->vfs_allocated_count = 0 from outside of
>  igb_cleanup_vf()
> * Up-port to current net-next

Thanks Simon and congrats on the new addition!  I have added this
three patch series to my tree for review and testing.

On Tue, Nov 24, 2009 at 11:40:36PM -0800, Jeff Kirsher wrote:
> On Tue, Nov 24, 2009 at 22:32, Simon Horman <horms@verge.net.au> wrote:
> > Move virtual finction cleanup code into igb_cleanup_vf() and for the sake
> > of symmetry rename igb_probe_vfs() as igb_init_vf().
> >
> > Although these functions aren't entirely symmetrical it should aid
> > maintenance by making the relationship between initialisation and cleanup
> > more obvious.
> >
> > Note that there appears to be no way for adapter->vfs_allocated_count to be
> > non-zero for the case where CONFIG_PCI_IOV is not set, so reseting this
> > value was moved to inside the relvant #ifdef.
> >
> > Signed-off-by: Simon Horman <horms@verge.net.au>
> >
> > ---
> > Thu, 05 Nov 2009 11:58:48 +1100
> > * Initial post
> >
> > Wed, 25 Nov 2009 15:43:45 +1100
> > * Actually remove adapter->vfs_allocated_count = 0 from outside of
> >  igb_cleanup_vf()
> > * Up-port to current net-next
> 
> Thanks Simon and congrats on the new addition!  I have added this
> three patch series to my tree for review and testing.

Thanks
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Simon Horman wrote:
> Move virtual finction cleanup code into igb_cleanup_vf() and for the sake
> of symmetry rename igb_probe_vfs() as igb_init_vf().
> 
> Although these functions aren't entirely symmetrical it should aid
> maintenance by making the relationship between initialisation and cleanup
> more obvious.
> 
> Note that there appears to be no way for adapter->vfs_allocated_count to be
> non-zero for the case where CONFIG_PCI_IOV is not set, so reseting this
> value was moved to inside the relvant #ifdef.
> 
> Signed-off-by: Simon Horman <horms@verge.net.au>

The one thing I noticed with this patch is that it didn't remove the vf 
cleanup from igb_remove.  You may want to go back and also add the 
replacement of that function with igb_cleanup_vf to this patch.

Thanks,

Alex

> --- 
> Thu, 05 Nov 2009 11:58:48 +1100
> * Initial post
> 
> Wed, 25 Nov 2009 15:43:45 +1100
> * Actually remove adapter->vfs_allocated_count = 0 from outside of
>   igb_cleanup_vf()
> * Up-port to current net-next
> Index: net-next-2.6/drivers/net/igb/igb_main.c
> ===================================================================
> --- net-next-2.6.orig/drivers/net/igb/igb_main.c	2009-11-25 16:59:19.000000000 +1100
> +++ net-next-2.6/drivers/net/igb/igb_main.c	2009-11-25 17:05:03.000000000 +1100
> @@ -92,6 +92,7 @@ void igb_update_stats(struct igb_adapter
>  static int igb_probe(struct pci_dev *, const struct pci_device_id *);
>  static void __devexit igb_remove(struct pci_dev *pdev);
>  static int igb_sw_init(struct igb_adapter *);
> +static void igb_cleanup_vf(struct igb_adapter * adapter);
>  static int igb_open(struct net_device *);
>  static int igb_close(struct net_device *);
>  static void igb_configure_tx(struct igb_adapter *);
> @@ -701,22 +702,7 @@ static void igb_set_interrupt_capability
>  
>  	/* If we can't do MSI-X, try MSI */
>  msi_only:
> -#ifdef CONFIG_PCI_IOV
> -	/* disable SR-IOV for non MSI-X configurations */
> -	if (adapter->vf_data) {
> -		struct e1000_hw *hw = &adapter->hw;
> -		/* disable iov and allow time for transactions to clear */
> -		pci_disable_sriov(adapter->pdev);
> -		msleep(500);
> -
> -		kfree(adapter->vf_data);
> -		adapter->vf_data = NULL;
> -		wr32(E1000_IOVCTL, E1000_IOVCTL_REUSE_VFQ);
> -		msleep(100);
> -		dev_info(&adapter->pdev->dev, "IOV Disabled\n");
> -	}
> -#endif
> -	adapter->vfs_allocated_count = 0;
> +	igb_cleanup_vf(adapter);
>  	adapter->rss_queues = 1;
>  	adapter->flags |= IGB_FLAG_QUEUE_PAIRS;
>  	adapter->num_rx_queues = 1;
> @@ -1755,7 +1741,7 @@ static void __devexit igb_remove(struct 
>  }
>  
>  /**
> - * igb_probe_vfs - Initialize vf data storage and add VFs to pci config space
> + * igb_init_vf - Initialize vf data storage and add VFs to pci config space
>   * @adapter: board private structure to initialize
>   *
>   * This function initializes the vf specific data storage and then attempts to
> @@ -1763,7 +1749,7 @@ static void __devexit igb_remove(struct 
>   * mor expensive time wise to disable SR-IOV than it is to allocate and free
>   * the memory for the VFs.
>   **/
> -static void __devinit igb_probe_vfs(struct igb_adapter * adapter)
> +static void __devinit igb_init_vf(struct igb_adapter * adapter)
>  {
>  #ifdef CONFIG_PCI_IOV
>  	struct pci_dev *pdev = adapter->pdev;
> @@ -1909,6 +1895,35 @@ static void igb_init_hw_timer(struct igb
>  }
>  
>  /**
> + * igb_cleanup_vf - Clean up vf data and remove vfs from pci config space
> + * @adapter: board private structure to initialize
> + *
> + * This function cleans-up the vf specific data storage and then attempts to
> + * deallocate the VFs.
> + **/
> +static void igb_cleanup_vf(struct igb_adapter * adapter)
> +{
> +#ifdef CONFIG_PCI_IOV
> +	struct e1000_hw *hw = &adapter->hw;
> +
> +	if (!adapter->vf_data)
> +		return;
> +
> +	/* disable iov and allow time for transactions to clear */
> +	pci_disable_sriov(adapter->pdev);
> +	msleep(500);
> +
> +	kfree(adapter->vf_data);
> +	adapter->vf_data = NULL;
> +	adapter->vfs_allocated_count = 0;
> +
> +	wr32(E1000_IOVCTL, E1000_IOVCTL_REUSE_VFQ);
> +	msleep(100);
> +	dev_info(&adapter->pdev->dev, "IOV Disabled\n");
> +#endif
> +}
> +
> +/**
>   * igb_sw_init - Initialize general software structures (struct igb_adapter)
>   * @adapter: board private structure to initialize
>   *
> @@ -1955,7 +1970,7 @@ static int __devinit igb_sw_init(struct 
>  	}
>  
>  	igb_init_hw_timer(adapter);
> -	igb_probe_vfs(adapter);
> +	igb_init_vf(adapter);
>  
>  	/* Explicitly disable IRQ since the NIC can be in any state. */
>  	igb_irq_disable(adapter);
> 

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, Nov 25, 2009 at 02:27:33PM -0800, Alexander Duyck wrote:
> Simon Horman wrote:
> >Move virtual finction cleanup code into igb_cleanup_vf() and for the sake
> >of symmetry rename igb_probe_vfs() as igb_init_vf().
> >
> >Although these functions aren't entirely symmetrical it should aid
> >maintenance by making the relationship between initialisation and cleanup
> >more obvious.
> >
> >Note that there appears to be no way for adapter->vfs_allocated_count to be
> >non-zero for the case where CONFIG_PCI_IOV is not set, so reseting this
> >value was moved to inside the relvant #ifdef.
> >
> >Signed-off-by: Simon Horman <horms@verge.net.au>
> 
> The one thing I noticed with this patch is that it didn't remove the
> vf cleanup from igb_remove.  You may want to go back and also add
> the replacement of that function with igb_cleanup_vf to this patch.

Ooops, yes that would be a good idea.
I'll send a revised patch after testing it.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html