Re: [PATCH 1/2 V3] PCI: introduce device assignment interface and refactory related code

[Alex Williamson <alex.williamson@redhat.com>]

On Fri, 2014-07-11 at 23:07 +0800, Ethan Zhao wrote:
> 
> > 在 2014年7月11日，下午10:50，Alex Williamson <alex.williamson@redhat.com> 写道：
> > 
> > On Fri, 2014-07-11 at 22:20 +0800, Ethan Zhao wrote:
> >>> 在 2014年7月11日，下午9:13，Alex Williamson <alex.williamson@redhat.com> 写道：
> >>> 
> >>>> On Fri, 2014-07-11 at 20:30 +0800, Ethan Zhao wrote:
> >>>> This patch introduces two new device assignment functions
> >>>> 
> >>>> pci_iov_assign_device(),
> >>>> pci_iov_deassign_device()
> >>>> 
> >>>> along with the existed one
> >>>> 
> >>>> pci_vfs_assigned()
> >>>> 
> >>>> They construct the VFs assignment management interface, used to assign/
> >>>> deassign device to VM and query the VFs reference counter. instead of
> >>>> direct manipulation of device flag.
> >>>> 
> >>>> This patch refashioned the related code and make them atomic.
> >>>> 
> >>>> v3: change the naming of device assignment helpers, because they work
> >>>> for all kind of PCI device, not only SR-IOV (david.vrabel@citrix.com)
> >>>> 
> >>>> v2: reorder the patchset and make it bisectable and atomic, steps clear
> >>>> between interface defination and implemenation according to the
> >>>> suggestion from alex.williamson@redhat.com
> >>>> 
> >>>> Signed-off-by: Ethan Zhao <ethan.zhao@oracle.com>
> >>>> ---
> >>> 
> >>> - Use a cover patch to describe the series
> >> Hmmm,
> >>> - Version information goes here, below the "---", not above it
> >>> 
> >>> - I stand by the patch breakdown I suggested originally, there are too
> >>> many logical changes here in patch 1.  There are easily 3 separate
> >>> patches here.
> >> You prefer v1 again ?
> > 
> > That's not at all what I'm suggesting.  In response to v1 I suggested a
> > specific reordering that would allow the series to be bisectable.  I'm
> > surprised to see v2 ignored that and combined the first 3 suggested
> > patches.
> If what I understand right. Your points are.
> 1. To be bisectable.
>     Combine the code into 2 parches, is really bisect able, without 2, 1 works right.
>     If 1 was broken into 3, any of them is lost, buggy. They must work together.

No, I never said 2 patches.  I said to fix and abstract the usage, then
change the implementation.  Fixing and abstracting the usage should be
multiple patches.  You should not be concerned about patches in a series
getting lost, you should be concerned that each step along the way is
functional.

>  2. Define and abstract first then implement.
>    1 &. 2 follows this point.
> > 
> >>> - Renaming s/sriov/iov/ doesn't address the problem David raised.  The
> >>> name is still synonymous with SR-IOV and it's defined on
> >>> drivers/pci/iov.c, which is only built with CONFIG_PCI_IOV.
> >> This is real issue, the interface should work without CONFIG_PCI_IOV defined.
> >> Should be fixed.
> >> 
> >>> - PCI_DEV_FLAGS_ASSIGNED should be unused after this series, why is it
> >>> not removed?
> >> you wanna do all things in one step ? Be practical. That is not my original purpose.
> > 
> > I never suggested collapsing steps.  Maybe I'd understand the original
> > purpose better if there was an overall description in a cover patch.
> Compose a cover letter seems necessary to describe patch set if they are too complex.

Any series with more than a single patch should have a cover letter.

> > 
> >>>> drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c |   17 ++---------------
> >>>> drivers/pci/iov.c                                  |   20 ++++++++++++++++++++
> >>>> drivers/xen/xen-pciback/pci_stub.c                 |    4 ++--
> >>>> include/linux/pci.h                                |    4 ++++
> >>>> virt/kvm/assigned-dev.c                            |    2 +-
> >>>> virt/kvm/iommu.c                                   |    4 ++--
> >>> 
> >>> - This patch touch 4 separate logical code areas, who do you expect to
> >>> ack and commit it?  Split it up.
> >>   Such would go back to the v1, four patches, if one lost to be committed, kernel could be buggy.  
> > 
> > There seems to be confusion between multiple patches, which is fine, and
> > the patch ordering from v1, which is wrong.
> > 
> Do you want them work standalone, if no, the confusion is nothing.

Each step in a series should be functional.  There's absolutely no
requirement that any individual patch from a series can be pulled out
separately and work on it's own.  Patches build on each other.

> >>>> 6 files changed, 31 insertions(+), 20 deletions(-)
> >>>> 
> >>>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> >>>> index 02c11a7..781040e 100644
> >>>> --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> >>>> +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> >>>> @@ -693,22 +693,9 @@ complete_reset:
> >>>> static bool i40e_vfs_are_assigned(struct i40e_pf *pf)
> >>>> {
> >>>>   struct pci_dev *pdev = pf->pdev;
> >>>> -    struct pci_dev *vfdev;
> >>>> -
> >>>> -    /* loop through all the VFs to see if we own any that are assigned */
> >>>> -    vfdev = pci_get_device(PCI_VENDOR_ID_INTEL, I40E_DEV_ID_VF , NULL);
> >>>> -    while (vfdev) {
> >>>> -        /* if we don't own it we don't care */
> >>>> -        if (vfdev->is_virtfn && pci_physfn(vfdev) == pdev) {
> >>>> -            /* if it is assigned we cannot release it */
> >>>> -            if (vfdev->dev_flags & PCI_DEV_FLAGS_ASSIGNED)
> >>>> -                return true;
> >>>> -        }
> >>>> 
> >>>> -        vfdev = pci_get_device(PCI_VENDOR_ID_INTEL,
> >>>> -                       I40E_DEV_ID_VF,
> >>>> -                       vfdev);
> >>>> -    }
> >>>> +    if (pci_vfs_assigned(pdev))
> >>>> +        return true;
> >>>> 
> >>>>   return false;
> >>>> }
> >>>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> >>>> index de7a747..090f827 100644
> >>>> --- a/drivers/pci/iov.c
> >>>> +++ b/drivers/pci/iov.c
> >>>> @@ -644,6 +644,26 @@ int pci_vfs_assigned(struct pci_dev *dev)
> >>>> EXPORT_SYMBOL_GPL(pci_vfs_assigned);
> >>>> 
> >>>> /**
> >>>> + * pci_iov_assign_device - assign device to VM
> >>>> + * @pdev: the device to be assigned.
> >>>> + */
> >>>> +void pci_iov_assign_device(struct pci_dev *pdev)
> >>>> +{
> >>>> +    pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
> >>>> +}
> >>>> +EXPORT_SYMBOL_GPL(pci_iov_assign_device);
> >>>> +
> >>>> +/**
> >>>> + * pci_iov_deassign_device - deasign device from VM
> >>>> + * @pdev: the device to be deassigned.
> >>>> + */
> >>>> +void pci_iov_deassign_device(struct pci_dev *pdev)
> >>>> +{
> >>>> +    pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
> >>>> +}
> >>>> +EXPORT_SYMBOL_GPL(pci_iov_deassign_device);
> >>>> +
> >>>> +/**
> >>>> * pci_sriov_set_totalvfs -- reduce the TotalVFs available
> >>>> * @dev: the PCI PF device
> >>>> * @numvfs: number that should be used for TotalVFs supported
> >>>> diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
> >>>> index 62fcd48..27e00d1 100644
> >>>> --- a/drivers/xen/xen-pciback/pci_stub.c
> >>>> +++ b/drivers/xen/xen-pciback/pci_stub.c
> >>>> @@ -133,7 +133,7 @@ static void pcistub_device_release(struct kref *kref)
> >>>>   xen_pcibk_config_free_dyn_fields(dev);
> >>>>   xen_pcibk_config_free_dev(dev);
> >>>> 
> >>>> -    dev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
> >>>> +    pci_iov_deassign_device(dev);
> >>>>   pci_dev_put(dev);
> >>>> 
> >>>>   kfree(psdev);
> >>>> @@ -404,7 +404,7 @@ static int pcistub_init_device(struct pci_dev *dev)
> >>>>   dev_dbg(&dev->dev, "reset device\n");
> >>>>   xen_pcibk_reset_device(dev);
> >>>> 
> >>>> -    dev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
> >>>> +    pci_iov_assign_device(dev);
> >>>>   return 0;
> >>>> 
> >>>> config_release:
> >>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
> >>>> index aab57b4..5ece6d6 100644
> >>>> --- a/include/linux/pci.h
> >>>> +++ b/include/linux/pci.h
> >>>> @@ -1603,6 +1603,8 @@ int pci_num_vf(struct pci_dev *dev);
> >>>> int pci_vfs_assigned(struct pci_dev *dev);
> >>>> int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
> >>>> int pci_sriov_get_totalvfs(struct pci_dev *dev);
> >>>> +void pci_iov_assign_device(struct pci_dev *dev);
> >>>> +void pci_iov_deassign_device(struct pci_dev *dev);
> >>>> #else
> >>>> static inline int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn)
> >>>> { return -ENODEV; }
> >>>> @@ -1614,6 +1616,8 @@ static inline int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs)
> >>>> { return 0; }
> >>>> static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
> >>>> { return 0; }
> >>>> +static inline void pci_iov_assign_device(struct pci_dev *dev) { }
> >>>> +static inline void pci_iov_deassign_device(struct pci_dev *dev) { }
> >>>> #endif
> >>>> 
> >>>> #if defined(CONFIG_HOTPLUG_PCI) || defined(CONFIG_HOTPLUG_PCI_MODULE)
> >>>> diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> >>>> index bf06577..7fac58d 100644
> >>>> --- a/virt/kvm/assigned-dev.c
> >>>> +++ b/virt/kvm/assigned-dev.c
> >>>> @@ -302,7 +302,7 @@ static void kvm_free_assigned_device(struct kvm *kvm,
> >>>>   else
> >>>>       pci_restore_state(assigned_dev->dev);
> >>>> 
> >>>> -    assigned_dev->dev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
> >>>> +    pci_iov_deassign_device(assigned_dev->dev);
> >>>> 
> >>>>   pci_release_regions(assigned_dev->dev);
> >>>>   pci_disable_device(assigned_dev->dev);
> >>>> diff --git a/virt/kvm/iommu.c b/virt/kvm/iommu.c
> >>>> index 0df7d4b..641ee73 100644
> >>>> --- a/virt/kvm/iommu.c
> >>>> +++ b/virt/kvm/iommu.c
> >>>> @@ -194,7 +194,7 @@ int kvm_assign_device(struct kvm *kvm,
> >>>>           goto out_unmap;
> >>>>   }
> >>>> 
> >>>> -    pdev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
> >>>> +    pci_iov_assign_device(pdev);
> >>>> 
> >>>>   dev_info(&pdev->dev, "kvm assign device\n");
> >>>> 
> >>>> @@ -220,7 +220,7 @@ int kvm_deassign_device(struct kvm *kvm,
> >>>> 
> >>>>   iommu_detach_device(domain, &pdev->dev);
> >>>> 
> >>>> -    pdev->dev_flags &= ~PCI_DEV_FLAGS_ASSIGNED;
> >>>> +    pci_iov_deassign_device(pdev);
> >>>> 
> >>>>   dev_info(&pdev->dev, "kvm deassign device\n");
> > 
> > 
> > 




------------------------------------------------------------------------------
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired