Re: [PATCH v11 00/11] PCI: brcmstb: enable PCIe for STB chips

[Lorenzo Pieralisi]

On Mon, Sep 07, 2020 at 11:29:06AM -0700, Florian Fainelli wrote:
> 
> 
> On 9/7/2020 10:43 AM, Jim Quinlan wrote:
> > On Mon, Sep 7, 2020 at 5:16 AM Lorenzo Pieralisi
> >  wrote:
> > > 
> > > On Thu, Aug 27, 2020 at 09:29:59AM -0400, Jim Quinlan wrote:
> > > > On Thu, Aug 27, 2020 at 2:35 AM Christoph Hellwig  wrote:
> > > > > 
> > > > > On Tue, Aug 25, 2020 at 10:40:27AM -0700, Florian Fainelli wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On 8/24/2020 12:30 PM, Jim Quinlan wrote:
> > > > > > > 
> > > > > > > Patchset Summary:
> > > > > > > Enhance a PCIe host controller driver.  Because of its 
> > > > > > > unusual design
> > > > > > > we are foced to change dev->dma_pfn_offset into a more 
> > > > > > > general role
> > > > > > > allowing multiple offsets.  See the 'v1' notes below for more 
> > > > > > > info.
> > > > > > 
> > > > > > We are version 11 and counting, and it is not clear to me whether 
> > > > > > there is
> > > > > > any chance of getting these patches reviewed and hopefully merged 
> > > > > > for the
> > > > > > 5.10 merge window.
> > > > > > 
> > > > > > There are a lot of different files being touched, so what would be 
> > > > > > the
> > > > > > ideal way of routing those changes towards inclusion?
> > > > > 
> > > > > FYI, I offered to take the dma-mapping bits through the dma-mapping 
> > > > > tree.
> > > > > I have a bit of a backlog, but plan to review and if Jim is ok with 
> > > > > that
> > > > > apply the current version.
> > > > Sounds good to me.
> > > 
> > > Hi Jim,
> > > 
> > > is the dependency now solved ? Should we review/take this series as
> > > is for v5.10 through the PCI tree ?
> > Hello Lorenzo,
> > 
> > We are still working out a regression with the DMA offset commit on
> > the RaspberryPi.  Nicolas has found the root cause and we are now
> > devising a solution.
> 
> Maybe we can parallelize the PCIe driver review while the DMA changes
> are being worked on in Christoph's branch. Lorenzo, are you fine with
> the PCIe changes proper?

I will have a look - the main contentious point was about the DMA
changes - if Christoph is happy with them I am OK with them
too - I hope there is not anything controversial in the host
bridge driver itself but I will look into it.

Lorenzo
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v11 00/11] PCI: brcmstb: enable PCIe for STB chips

[Lorenzo Pieralisi]

On Thu, Aug 27, 2020 at 09:29:59AM -0400, Jim Quinlan wrote:
> On Thu, Aug 27, 2020 at 2:35 AM Christoph Hellwig  wrote:
> >
> > On Tue, Aug 25, 2020 at 10:40:27AM -0700, Florian Fainelli wrote:
> > > Hi,
> > >
> > > On 8/24/2020 12:30 PM, Jim Quinlan wrote:
> > >>
> > >> Patchset Summary:
> > >>Enhance a PCIe host controller driver.  Because of its unusual design
> > >>we are foced to change dev->dma_pfn_offset into a more general role
> > >>allowing multiple offsets.  See the 'v1' notes below for more info.
> > >
> > > We are version 11 and counting, and it is not clear to me whether there is
> > > any chance of getting these patches reviewed and hopefully merged for the
> > > 5.10 merge window.
> > >
> > > There are a lot of different files being touched, so what would be the
> > > ideal way of routing those changes towards inclusion?
> >
> > FYI, I offered to take the dma-mapping bits through the dma-mapping tree.
> > I have a bit of a backlog, but plan to review and if Jim is ok with that
> > apply the current version.
> Sounds good to me.

Hi Jim,

is the dependency now solved ? Should we review/take this series as
is for v5.10 through the PCI tree ?

Thanks,
Lorenzo
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] PCI: hv: Fix panic by calling hv_pci_remove_slots() earlier

[Lorenzo Pieralisi]

On Tue, Aug 06, 2019 at 08:41:17PM +, Dexuan Cui wrote:
> > From: linux-hyperv-ow...@vger.kernel.org
> >  On Behalf Of Bjorn Helgaas
> > Sent: Tuesday, August 6, 2019 1:16 PM
> > To: Dexuan Cui 
> > 
> > Thanks for updating this.  But you didn't update the subject line,
> > which is really still a little too low-level.  Maybe Lorenzo will fix
> > this.  Something like this, maybe?
> > 
> >   PCI: hv: Avoid use of hv_pci_dev->pci_slot after freeing it
> 
> This is better. Thanks!
> 
> I hope Lorenzo can help to fix this so I could avoid a v3. :-)

You should have fixed it yourself, this time I will.

Thanks,
Lorenzo

Re: [PATCH v2] PCI: hv: Fix a use-after-free bug in hv_eject_device_work()

[Lorenzo Pieralisi]

On Fri, Jun 21, 2019 at 11:45:23PM +, Dexuan Cui wrote:
> 
> The commit 05f151a73ec2 itself is correct, but it exposes this
> use-after-free bug, which is caught by some memory debug options.
> 
> Add a Fixes tag to indicate the dependency.
> 
> Fixes: 05f151a73ec2 ("PCI: hv: Fix a memory leak in hv_eject_device_work()")
> Signed-off-by: Dexuan Cui 
> Cc: sta...@vger.kernel.org
> ---
> 
> In v2:
> Replaced "hpdev->hbus" with "hbus", since we have the new "hbus" variable. 
> [Michael Kelley]
> 
>  drivers/pci/controller/pci-hyperv.c | 15 +--
>  1 file changed, 9 insertions(+), 6 deletions(-)

Applied to pci/hv for v5.3, thanks.

Lorenzo

> diff --git a/drivers/pci/controller/pci-hyperv.c 
> b/drivers/pci/controller/pci-hyperv.c
> index 808a182830e5..5dadc964ad3b 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -1880,6 +1880,7 @@ static void hv_pci_devices_present(struct 
> hv_pcibus_device *hbus,
>  static void hv_eject_device_work(struct work_struct *work)
>  {
>   struct pci_eject_response *ejct_pkt;
> + struct hv_pcibus_device *hbus;
>   struct hv_pci_dev *hpdev;
>   struct pci_dev *pdev;
>   unsigned long flags;
> @@ -1890,6 +1891,7 @@ static void hv_eject_device_work(struct work_struct 
> *work)
>   } ctxt;
>  
>   hpdev = container_of(work, struct hv_pci_dev, wrk);
> + hbus = hpdev->hbus;
>  
>   WARN_ON(hpdev->state != hv_pcichild_ejecting);
>  
> @@ -1900,8 +1902,7 @@ static void hv_eject_device_work(struct work_struct 
> *work)
>* because hbus->pci_bus may not exist yet.
>*/
>   wslot = wslot_to_devfn(hpdev->desc.win_slot.slot);
> - pdev = pci_get_domain_bus_and_slot(hpdev->hbus->sysdata.domain, 0,
> -wslot);
> + pdev = pci_get_domain_bus_and_slot(hbus->sysdata.domain, 0, wslot);
>   if (pdev) {
>   pci_lock_rescan_remove();
>   pci_stop_and_remove_bus_device(pdev);
> @@ -1909,9 +1910,9 @@ static void hv_eject_device_work(struct work_struct 
> *work)
>   pci_unlock_rescan_remove();
>   }
>  
> - spin_lock_irqsave(>hbus->device_list_lock, flags);
> + spin_lock_irqsave(>device_list_lock, flags);
>   list_del(>list_entry);
> - spin_unlock_irqrestore(>hbus->device_list_lock, flags);
> + spin_unlock_irqrestore(>device_list_lock, flags);
>  
>   if (hpdev->pci_slot)
>   pci_destroy_slot(hpdev->pci_slot);
> @@ -1920,7 +1921,7 @@ static void hv_eject_device_work(struct work_struct 
> *work)
>   ejct_pkt = (struct pci_eject_response *)
>   ejct_pkt->message_type.type = PCI_EJECTION_COMPLETE;
>   ejct_pkt->wslot.slot = hpdev->desc.win_slot.slot;
> - vmbus_sendpacket(hpdev->hbus->hdev->channel, ejct_pkt,
> + vmbus_sendpacket(hbus->hdev->channel, ejct_pkt,
>sizeof(*ejct_pkt), (unsigned long),
>VM_PKT_DATA_INBAND, 0);
>  
> @@ -1929,7 +1930,9 @@ static void hv_eject_device_work(struct work_struct 
> *work)
>   /* For the two refs got in new_pcichild_device() */
>   put_pcichild(hpdev);
>   put_pcichild(hpdev);
> - put_hvpcibus(hpdev->hbus);
> + /* hpdev has been freed. Do not use it any more. */
> +
> + put_hvpcibus(hbus);
>  }
>  
>  /**
> -- 
> 2.17.1
> 

Re: [PATCH 3/3] PCI: hv: Add pci_destroy_slot() in pci_devices_present_work(), if necessary

[Lorenzo Pieralisi]

On Mon, Mar 04, 2019 at 09:34:49PM +, Dexuan Cui wrote:
> When we hot-remove a device, usually the host sends us a PCI_EJECT message,
> and a PCI_BUS_RELATIONS message with bus_rel->device_count == 0. But when
> we do the quick hot-add/hot-remove test, the host may not send us the
> PCI_EJECT message, if the guest has not fully finished the initialization
> by sending the PCI_RESOURCES_ASSIGNED* message to the host, so it's
> potentially unsafe to only depend on the pci_destroy_slot() in
> hv_eject_device_work(), though create_root_hv_pci_bus() ->
> hv_pci_assign_slots() is not called in this case. Note: in this case, the
> host still sends the guest a PCI_BUS_RELATIONS message with
> bus_rel->device_count == 0.
> 
> And, in the quick hot-add/hot-remove test, we can have such a race: before
> pci_devices_present_work() -> new_pcichild_device() adds the new device
> into hbus->children, we may have already received the PCI_EJECT message,
> and hence the taklet handler hv_pci_onchannelcallback() may fail to find
> the "hpdev" by get_pcichild_wslot(hbus, dev_message->wslot.slot), so
> hv_pci_eject_device() is NOT called; later create_root_hv_pci_bus() ->
> hv_pci_assign_slots() creates the slot, and the PCI_BUS_RELATIONS message
> with bus_rel->device_count == 0 removes the device from hbus->children, and
> we end up being unable to remove the slot in hv_pci_remove() ->
> hv_pci_remove_slots().
> 
> The patch removes the slot in pci_devices_present_work() when the device
> is removed. This can address the above race. Note 1:
> pci_devices_present_work() and hv_eject_device_work() run in the
> singled-threaded hbus->wq, so there is not a double-remove issue for the
> slot. Note 2: we can't offload hv_pci_eject_device() from
> hv_pci_onchannelcallback() to the workqueue, because we need
> hv_pci_onchannelcallback() synchronously call hv_pci_eject_device() to
> poll the channel's ringbuffer to work around the
> "hangs in hv_compose_msi_msg()" issue: see
> commit de0aa7b2f97d ("PCI: hv: Fix 2 hang issues in hv_compose_msi_msg()")

This commit log is unreadable, sorry. Indentation, punctuation and
formatting are just a mess, try to read it, you will notice by
yourself.

I basically reformatted it completely and pushed the series to
pci/controller-fixes but that's the last time I do it since I am not an
editor, next time I won't merge it.

More importantly, these patches are marked for stable, given the series
of fixes that triggered this series please ensure it was tested
thoroughly because it is honestly complicate to understand and I do not
want to backport further fixes to stable kernels on top of this.

Please have a look and report back.

Thanks,
Lorenzo

> Fixes: a15f2c08c708 ("PCI: hv: support reporting serial number as slot 
> information")
> Signed-off-by: Dexuan Cui 
> Cc: sta...@vger.kernel.org
> ---
>  drivers/pci/controller/pci-hyperv.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/pci/controller/pci-hyperv.c 
> b/drivers/pci/controller/pci-hyperv.c
> index b489412e3502..82acd6155adf 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -1776,6 +1776,10 @@ static void pci_devices_present_work(struct 
> work_struct *work)
>   hpdev = list_first_entry(, struct hv_pci_dev,
>list_entry);
>   list_del(>list_entry);
> +
> + if (hpdev->pci_slot)
> + pci_destroy_slot(hpdev->pci_slot);
> +
>   put_pcichild(hpdev);
>   }
>  
> -- 
> 2.19.1
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/3] PCI: hv: Fix a memory leak in hv_eject_device_work()

[Lorenzo Pieralisi]

On Tue, Mar 26, 2019 at 06:01:32PM +, Dexuan Cui wrote:

[...]

> > > Have we reached a conclusion on this ? I would like to merge this series
> > > given that it is fixing bugs and it has hung in the balance for quite
> > > a while but it looks like Michael is not too happy about these patches
> > > and I need a maintainer ACK to merge them.
> > >
> > > Thanks,
> > > Lorenzo
> > 
> > Dexuan and I have discussed the topic extensively offline.  The patch works
> > in its current form, and I'll agree to it.
> > 
> > Reviewed-by:  Michael Kelley 
> 
> Thanks, Michael!
> 
> Hi Lorenzo,
> All the 3 patches have got Michael's Reviewed-by.

Good, I asked because I do not want to merge patches with review
questions open, thanks for the clarifications.

> Previously, Stephen Hemminger, one of the Hyper-V driver maintainers, 
> provided his Reviewed-by in the " [PATCH 0/3]" mail:
> https://lkml.org/lkml/2019/3/5/521

I will reformat/rewrite the logs and notify you when queued.

Thanks,
Lorenzo
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/3] PCI: hv: Fix a memory leak in hv_eject_device_work()

[Lorenzo Pieralisi]

On Thu, Mar 21, 2019 at 12:12:03AM +, Dexuan Cui wrote:
> > From: Michael Kelley 
> > Sent: Wednesday, March 20, 2019 2:38 PM
> > 
> > From: Dexuan Cui 
> > >
> > > After a device is just created in new_pcichild_device(), hpdev->refs is 
> > > set
> > > to 2 (i.e. the initial value of 1 plus the get_pcichild()).
> > >
> > > When we hot remove the device from the host, in Linux VM we first call
> > > hv_pci_eject_device(), which increases hpdev->refs by get_pcichild() and
> > > then schedules a work of hv_eject_device_work(), so hpdev->refs becomes 3
> > > (let's ignore the paired get/put_pcichild() in other places). But in
> > > hv_eject_device_work(), currently we only call put_pcichild() twice,
> > > meaning the 'hpdev' struct can't be freed in put_pcichild(). This patch
> > > adds one put_pcichild() to fix the memory leak.
> > >
> > > BTW, the device can also be removed when we run "rmmod pci-hyperv". On
> > this
> > > path (hv_pci_remove() -> hv_pci_bus_exit() -> hv_pci_devices_present()),
> > > hpdev->refs is 2, and we do correctly call put_pcichild() twice in
> > > pci_devices_present_work().
> > 
> > Exiting new_pcichild_device() with hpdev->refs set to 2 seems OK to me.
> > There is the reference in the hbus->children list, and there is the 
> > reference that
> > is returned to the caller.  
> So IMO the "normal" reference count should be 2. :-) IMO only when a 
> hv_pci_dev
> device is about to be destroyed, its reference count can drop to less than 2, 
> i.e. first temporarily drop to 1 (meaning the hv_pci_dev device is removed 
> from
> hbus->children), and then drop to zero (meaning kfree(hpdev) is called).
> 
> > But what is strange is that pci_devices_present_work()
> > overwrites the reference returned in local variable hpdev without doing a
> > put_pcichild().
> I suppose you mean:
> 
> /* First, mark all existing children as reported missing. */
> spin_lock_irqsave(>device_list_lock, flags);
> list_for_each_entry(hpdev, >children, list_entry) {
> hpdev->reported_missing = true;
> }
> spin_unlock_irqrestore(>device_list_lock, flags)
> 
> This is not strange to me, because, in pci_devices_present_work(), at first we
> don't know which devices are about to disappear, so we pre-mark all devices to
> be potentially missing like that; if a device is still on the bus, we'll mark 
> its
> hpdev->reported_missing to false later; only after we know exactly which
> devices are missing, we should call put_pcichild() against them. All these
> seem natural to me.
> 
> > It seems like the "normal" reference count should be 1 when the
> > child device is not being manipulated, not 2.
> What does "not being manipulated" mean?
> 
> > The fix would be to add a call to
> > put_pcichild() when the return value from new_pcichild_device() is
> > overwritten.
> In pci_devices_present_work(), we NEVER "overwrite" the "hpdev" returned
> from new_pcichild_device(): the "reported_missing" field of the new hpdev
> is implicitly initialized to false in new_pcichild_device().
> 
> > Then remove the call to put_pcichild() in pci_device_present_work() when
> > missing
> > children are moved to the local list. The children have been moved from one
> > list
> > to another, so there's no need to decrement the reference count.  Then when
> > everything in the local list is deleted, the reference is correctly 
> > decremented,
> > presumably freeing the memory.
> > 
> > With this approach, the code in hv_eject_device_work() is correct.  There's
> > one call to put_pcichild() to reflect removing the child device from the 
> > hbus->
> > children list, and one call to put_pcichild() to pair with the 
> > get_pcichild() in
> > hv_pci_eject_device().
> Please refer to my replies above. IMO we should fix
> hv_eject_device_work() rather than pci_devices_present_work().

Have we reached a conclusion on this ? I would like to merge this series
given that it is fixing bugs and it has hung in the balance for quite
a while but it looks like Michael is not too happy about these patches
and I need a maintainer ACK to merge them.

Thanks,
Lorenzo

> Thanks
> -- Dexuan
>  
> > Your patch works, but to me it leaves the ref count in an unnatural state
> > most of the time.
> > 
> > Michael
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 1/2] PCI: hv: Replace hv_vp_set with hv_vpset

[Lorenzo Pieralisi]

On Wed, Feb 27, 2019 at 04:53:37PM +0100, Vitaly Kuznetsov wrote:
> Maya Nakamura  writes:
> 
> > Remove a duplicate definition of VP set (hv_vp_set) and use the common
> > definition (hv_vpset) that is used in other places.
> >
> > Change the order of the members in struct hv_pcibus_device so that the
> > declaration of retarget_msi_interrupt_params is the last member. Struct
> > hv_vpset, which contains a flexible array, is nested two levels deep in
> > struct hv_pcibus_device via retarget_msi_interrupt_params.
> >
> > Add a comment that retarget_msi_interrupt_params should be the last member
> > of struct hv_pcibus_device.
> >
> > Signed-off-by: Maya Nakamura 
> > ---
> > Change in v3:
> > - Correct the v2 change log.
> >
> > Change in v2:
> > - Update the commit message.
> >
> >  drivers/pci/controller/pci-hyperv.c | 25 -
> >  1 file changed, 12 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pci-hyperv.c 
> > b/drivers/pci/controller/pci-hyperv.c
> > index 9ba4d12c179c..da8b58d8630d 100644
> > --- a/drivers/pci/controller/pci-hyperv.c
> > +++ b/drivers/pci/controller/pci-hyperv.c
> > @@ -393,12 +393,6 @@ struct hv_interrupt_entry {
> >  
> >  #define HV_VP_SET_BANK_COUNT_MAX   5 /* current implementation limit */
> >  
> > -struct hv_vp_set {
> > -   u64 format; /* 0 (HvGenericSetSparse4k) */
> > -   u64 valid_banks;
> > -   u64 masks[HV_VP_SET_BANK_COUNT_MAX];
> > -};
> > -
> >  /*
> >   * flags for hv_device_interrupt_target.flags
> >   */
> > @@ -410,7 +404,7 @@ struct hv_device_interrupt_target {
> > u32 flags;
> > union {
> > u64  vp_mask;
> > -   struct hv_vp_set vp_set;
> > +   struct hv_vpset vp_set;
> > };
> >  };
> >  
> > @@ -460,12 +454,16 @@ struct hv_pcibus_device {
> > struct msi_controller msi_chip;
> > struct irq_domain *irq_domain;
> >  
> > -   /* hypercall arg, must not cross page boundary */
> > -   struct retarget_msi_interrupt retarget_msi_interrupt_params;
> > -
> > spinlock_t retarget_msi_interrupt_lock;
> >  
> > struct workqueue_struct *wq;
> > +
> > +   /* hypercall arg, must not cross page boundary */
> > +   struct retarget_msi_interrupt retarget_msi_interrupt_params;
> > +
> 
> One more thing here (and again sorry for being late): this structure is
> being used as Hyper-V hypercall argument and these have special
> requirements on alignment (8 bytes). struct retarget_msi_interrupt is
> packed and depending on your environment/compiler you may or may not see
> the issue but I has able to get HVCALL_RETARGET_INTERRUPT failing with
> the return value of 4 (HV_STATUS_INVALID_ALIGNMENT).
> 
> I suggest we add __aligned(8) to 'struct retarget_msi_interrupt'
> definition (I haven't tested this but should work). This is not a new
> issue, it existed before your patch, but the code movement you do may
> change the exposure.

I am happy to apply this small fix _before_ this patch but if you want
me to merge them for v5.1 this must be put together and tested quite
quickly.

For the time being I am not dropping this patch from the PCI queue,
waiting for an update from you guys.

Thanks,
Lorenzo

> 
> 
> > +   /*
> > +* Don't put anything here: retarget_msi_interrupt_params must be last
> > +*/
> >  };
> >  
> >  /*
> > @@ -955,12 +953,13 @@ static void hv_irq_unmask(struct irq_data *data)
> >  */
> > params->int_target.flags |=
> > HV_DEVICE_INTERRUPT_TARGET_PROCESSOR_SET;
> > -   params->int_target.vp_set.valid_banks =
> > +   params->int_target.vp_set.valid_bank_mask =
> > (1ull << HV_VP_SET_BANK_COUNT_MAX) - 1;
> >  
> > /*
> >  * var-sized hypercall, var-size starts after vp_mask (thus
> > -* vp_set.format does not count, but vp_set.valid_banks does).
> > +* vp_set.format does not count, but vp_set.valid_bank_mask
> > +* does).
> >  */
> > var_size = 1 + HV_VP_SET_BANK_COUNT_MAX;
> >  
> > @@ -974,7 +973,7 @@ static void hv_irq_unmask(struct irq_data *data)
> > goto exit_unlock;
> > }
> >  
> > -   params->int_target.vp_set.masks[cpu_vmbus / 64] |=
> > +   params->int_target.vp_set.bank_contents[cpu_vmbus / 64] 
> > |=
> > (1ULL << (cpu_vmbus & 63));
> > }
> > } else {
> 
> -- 
> Vitaly
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 2/2] PCI: hv: Refactor hv_irq_unmask() to use cpumask_to_vpset()

[Lorenzo Pieralisi]

On Wed, Feb 27, 2019 at 01:34:44PM +0100, Vitaly Kuznetsov wrote:
> Maya Nakamura  writes:
> 
> > Remove the duplicate implementation of cpumask_to_vpset() and use the
> > shared implementation. Export hv_max_vp_index, which is required by
> > cpumask_to_vpset().
> >
> > Apply changes to hv_irq_unmask() based on feedback.
> >
> 
> I just noticed an issue with this patch, sorry I've missed it before. I
> don't see the commit in Linus' tree, not sure if we should amend this
> one or a follow-up patch is needed.

I will drop this patch from the PCI queue, it does not make sense
to merge a patch adding a bug and fix it up later with a subsequent
one given that it is not upstream yet.

Lorenzo

> > Signed-off-by: Maya Nakamura 
> > ---
> > Changes in v3:
> >  - Modify to catch all failures from cpumask_to_vpset().
> >  - Correct the v2 change log about the commit message.
> >
> > Changes in v2:
> >  - Remove unnecessary nr_bank initialization.
> >  - Delete two unnecessary dev_err()'s.
> >  - Unlock before returning.
> >  - Update the commit message.
> >
> >  arch/x86/hyperv/hv_init.c   |  1 +
> >  drivers/pci/controller/pci-hyperv.c | 38 +
> >  2 files changed, 18 insertions(+), 21 deletions(-)
> >
> > diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> > index 7abb09e2eeb8..7f2eed1fc81b 100644
> > --- a/arch/x86/hyperv/hv_init.c
> > +++ b/arch/x86/hyperv/hv_init.c
> > @@ -96,6 +96,7 @@ void  __percpu **hyperv_pcpu_input_arg;
> >  EXPORT_SYMBOL_GPL(hyperv_pcpu_input_arg);
> >  
> >  u32 hv_max_vp_index;
> > +EXPORT_SYMBOL_GPL(hv_max_vp_index);
> >  
> >  static int hv_cpu_init(unsigned int cpu)
> >  {
> > diff --git a/drivers/pci/controller/pci-hyperv.c 
> > b/drivers/pci/controller/pci-hyperv.c
> > index da8b58d8630d..a78def332bbc 100644
> > --- a/drivers/pci/controller/pci-hyperv.c
> > +++ b/drivers/pci/controller/pci-hyperv.c
> > @@ -391,8 +391,6 @@ struct hv_interrupt_entry {
> > u32 data;
> >  };
> >  
> > -#define HV_VP_SET_BANK_COUNT_MAX   5 /* current implementation limit */
> > -
> >  /*
> >   * flags for hv_device_interrupt_target.flags
> >   */
> > @@ -908,12 +906,12 @@ static void hv_irq_unmask(struct irq_data *data)
> > struct retarget_msi_interrupt *params;
> > struct hv_pcibus_device *hbus;
> > struct cpumask *dest;
> > +   cpumask_var_t tmp;
> > struct pci_bus *pbus;
> > struct pci_dev *pdev;
> > unsigned long flags;
> > u32 var_size = 0;
> > -   int cpu_vmbus;
> > -   int cpu;
> > +   int cpu, nr_bank;
> > u64 res;
> >  
> > dest = irq_data_get_effective_affinity_mask(data);
> > @@ -953,29 +951,27 @@ static void hv_irq_unmask(struct irq_data *data)
> >  */
> > params->int_target.flags |=
> > HV_DEVICE_INTERRUPT_TARGET_PROCESSOR_SET;
> > -   params->int_target.vp_set.valid_bank_mask =
> > -   (1ull << HV_VP_SET_BANK_COUNT_MAX) - 1;
> > +
> > +   if (!alloc_cpumask_var(, GFP_KERNEL)) {
> 
> We can't use GFP_KERNEL here: this is happening under
> hbus->retarget_msi_interrupt_lock spinlock, we should use GFP_ATOMIC
> instead. It may, however, make sense to add the cpumask to a
> pre-allocated structure (e.g. struct hv_pcibus_device) to make sure the
> allocation never fails.
> 
> > +   res = 1;
> > +   goto exit_unlock;
> > +   }
> > +
> > +   cpumask_and(tmp, dest, cpu_online_mask);
> > +   nr_bank = cpumask_to_vpset(>int_target.vp_set, tmp);
> > +   free_cpumask_var(tmp);
> > +
> > +   if (nr_bank <= 0) {
> > +   res = 1;
> > +   goto exit_unlock;
> > +   }
> >  
> > /*
> >  * var-sized hypercall, var-size starts after vp_mask (thus
> >  * vp_set.format does not count, but vp_set.valid_bank_mask
> >  * does).
> >  */
> > -   var_size = 1 + HV_VP_SET_BANK_COUNT_MAX;
> > -
> > -   for_each_cpu_and(cpu, dest, cpu_online_mask) {
> > -   cpu_vmbus = hv_cpu_number_to_vp_number(cpu);
> > -
> > -   if (cpu_vmbus >= HV_VP_SET_BANK_COUNT_MAX * 64) {
> > -   dev_err(>hdev->device,
> > -   "too high CPU %d", cpu_vmbus);
> > -   res = 1;
> > -   goto exit_unlock;
> > -   }
> > -
> > -   params->int_target.vp_set.bank_contents[cpu_vmbus / 64] 
> > |=
> > -   (1ULL << (cpu_vmbus & 63));
> > -   }
> > +   var_size = 1 + nr_bank;
> > } else {
> > for_each_cpu_and(cpu, dest, cpu_online_mask) {
> > params->int_target.vp_mask |=
> 
> -- 
> Vitaly
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 0/2] PCI: hv: Refactor hv_irq_unmask() to use hv_vpset and cpumask_to_vpset()

[Lorenzo Pieralisi]

On Mon, Jan 28, 2019 at 11:15:37PM -0800, Maya Nakamura wrote:
> This patchset removes a duplicate definition of VP set (hv_vp_set) and
> uses the common definition (hv_vpset) that is used in other places. It
> changes the order of the members in struct hv_pcibus_device due to
> flexible array in hv_vpset.
> 
> It also removes the duplicate implementation of cpumask_to_vpset(), uses
> the shared implementation, and exports hv_max_vp_index, which is
> required by cpumask_to_vpset().
> 
> Maya Nakamura (2):
>   PCI: hv: Replace hv_vp_set with hv_vpset
>   PCI: hv: Refactor hv_irq_unmask() to use cpumask_to_vpset()
> 
>  arch/x86/hyperv/hv_init.c   |  1 +
>  drivers/pci/controller/pci-hyperv.c | 59 +
>  2 files changed, 28 insertions(+), 32 deletions(-)

Applied to pci/hv for v5.1, thanks.

Lorenzo
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/2] PCI: hv: Replace hv_vp_set with hv_vpset

[Lorenzo Pieralisi]

On Mon, Jan 28, 2019 at 09:49:32PM -0800, Maya Nakamura wrote:
> On Sun, Jan 27, 2019 at 05:11:48AM +, Michael Kelley wrote:
> > From: Maya Nakamura   Sent: Saturday, January 
> > 26, 2019 12:52 AM
> > > 
> > > Remove a duplicate definition of VP set (hv_vp_set) and use the common
> > > definition (hv_vpset) that is used in other places.
> > > 
> > > Change the order of the members in struct hv_pcibus_device so that the
> > > declaration of retarget_msi_interrupt_params is the last member. Struct
> > > hv_vpset, which contains a flexible array, is nested two levels deep in
> > > struct hv_pcibus_device via retarget_msi_interrupt_params.
> > > 
> > > Add a comment that retarget_msi_interrupt_params should be the last member
> > > of struct hv_pcibus_device.
> > > 
> > > Signed-off-by: Maya Nakamura 
> > > ---
> > > Change in v2:
> > > - None
> > > 
> > 
> > Right -- there was no code change.  But it's customary to note that
> > you updated the commit message.
> > 
> Thank you for your feedback. I will edit the change log in v3.
> 
> > Reviewed-by:  Michael Kelley 

I will add Michael's tag to v3 (unless Michael is not happy with that),
it is missing there.

Lorenzo
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 0/2] PCI: hv: Refactor hv_irq_unmask() to use hv_vpset and cpumask_to_vpset()

[Lorenzo Pieralisi]

On Mon, Jan 28, 2019 at 11:15:37PM -0800, Maya Nakamura wrote:
> This patchset removes a duplicate definition of VP set (hv_vp_set) and
> uses the common definition (hv_vpset) that is used in other places. It
> changes the order of the members in struct hv_pcibus_device due to
> flexible array in hv_vpset.
> 
> It also removes the duplicate implementation of cpumask_to_vpset(), uses
> the shared implementation, and exports hv_max_vp_index, which is
> required by cpumask_to_vpset().
> 
> Maya Nakamura (2):
>   PCI: hv: Replace hv_vp_set with hv_vpset
>   PCI: hv: Refactor hv_irq_unmask() to use cpumask_to_vpset()
> 
>  arch/x86/hyperv/hv_init.c   |  1 +
>  drivers/pci/controller/pci-hyperv.c | 59 +
>  2 files changed, 28 insertions(+), 32 deletions(-)

I need a maintainer ACK to merge this series.

Lorenzo
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/2] PCI: hv: Replace hv_vp_set with hv_vpset

[Lorenzo Pieralisi]

On Wed, Feb 13, 2019 at 02:20:29AM +, Michael Kelley wrote:
> From: Lorenzo Pieralisi   Sent: Tuesday, February 
> 12, 2019 8:35 AM
> >
> > On Mon, Jan 28, 2019 at 09:49:32PM -0800, Maya Nakamura wrote:
> > > On Sun, Jan 27, 2019 at 05:11:48AM +, Michael Kelley wrote:
> > > > From: Maya Nakamura   Sent: Saturday, 
> > > > January 26,
> > 2019 12:52 AM
> > > > >
> > > > > Remove a duplicate definition of VP set (hv_vp_set) and use the common
> > > > > definition (hv_vpset) that is used in other places.
> > > > >
> > > > > Change the order of the members in struct hv_pcibus_device so that the
> > > > > declaration of retarget_msi_interrupt_params is the last member. 
> > > > > Struct
> > > > > hv_vpset, which contains a flexible array, is nested two levels deep 
> > > > > in
> > > > > struct hv_pcibus_device via retarget_msi_interrupt_params.
> > > > >
> > > > > Add a comment that retarget_msi_interrupt_params should be the last 
> > > > > member
> > > > > of struct hv_pcibus_device.
> > > > >
> > > > > Signed-off-by: Maya Nakamura 
> > > > > ---
> > > > > Change in v2:
> > > > > - None
> > > > >
> > > >
> > > > Right -- there was no code change.  But it's customary to note that
> > > > you updated the commit message.
> > > >
> > > Thank you for your feedback. I will edit the change log in v3.
> > >
> > > > Reviewed-by:  Michael Kelley 
> > 
> > Are you really sure there is no behavioural change ? What piece of
> > code allocates hv_vpset.bank_contents[] memory with this patch applied ?
> > 
> > I suspect the current code does not use hv_vpset for this specific
> > reason, ie allocate struct hv_vp_set.masks array memory statically.
> > 
> 
> There is indeed no behavior change.   A full page of memory is
> allocated in hv_pci_probe() so that we can be sure that the Hyper-V
> hypercall arguments don't cross a page boundary.   This page allows
> more than enough space for the hv_vpset.bank_contents[] to grow
> as needed (with one bit allocated in the masks for up to the limit
> of 8192 CPUs allowed by Linux).   A flexible array is used because
> the hv_vpset structure is also used in some MMU hypercalls that
> have two variable size arrays.

I see, thanks for explaining.

Lorenzo
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/2] PCI: hv: Replace hv_vp_set with hv_vpset

[Lorenzo Pieralisi]

On Mon, Jan 28, 2019 at 09:49:32PM -0800, Maya Nakamura wrote:
> On Sun, Jan 27, 2019 at 05:11:48AM +, Michael Kelley wrote:
> > From: Maya Nakamura   Sent: Saturday, January 
> > 26, 2019 12:52 AM
> > > 
> > > Remove a duplicate definition of VP set (hv_vp_set) and use the common
> > > definition (hv_vpset) that is used in other places.
> > > 
> > > Change the order of the members in struct hv_pcibus_device so that the
> > > declaration of retarget_msi_interrupt_params is the last member. Struct
> > > hv_vpset, which contains a flexible array, is nested two levels deep in
> > > struct hv_pcibus_device via retarget_msi_interrupt_params.
> > > 
> > > Add a comment that retarget_msi_interrupt_params should be the last member
> > > of struct hv_pcibus_device.
> > > 
> > > Signed-off-by: Maya Nakamura 
> > > ---
> > > Change in v2:
> > > - None
> > > 
> > 
> > Right -- there was no code change.  But it's customary to note that
> > you updated the commit message.
> > 
> Thank you for your feedback. I will edit the change log in v3.
> 
> > Reviewed-by:  Michael Kelley 

Are you really sure there is no behavioural change ? What piece of
code allocates hv_vpset.bank_contents[] memory with this patch applied ?

I suspect the current code does not use hv_vpset for this specific
reason, ie allocate struct hv_vp_set.masks array memory statically.

Lorenzo
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] PCI: hv: Add hv_pci_remove_slots() when we unload the driver

[Lorenzo Pieralisi]

On Thu, Feb 07, 2019 at 08:36:32PM +, Dexuan Cui wrote:
> 
> When we unload pci-hyperv, the host doesn't send us a PCI_EJECT message.
> In this case we also need to make sure the sysfs pci slot directory
> is removed, otherwise "cat /sys/bus/pci/slots/2/address" will trigger
> "BUG: unable to handle kernel paging request". And, if we unload/reload
> the driver several times, we'll have multiple pci slot directories in
> /sys/bus/pci/slots/ like this:
> 
> root@localhost:~# ls -rtl  /sys/bus/pci/slots/
> total 0
> drwxr-xr-x 2 root root 0 Feb  7 10:49 2
> drwxr-xr-x 2 root root 0 Feb  7 10:49 2-1
> drwxr-xr-x 2 root root 0 Feb  7 10:51 2-2
> 
> The patch adds the missing code, and in hv_eject_device_work() it also
> moves pci_destroy_slot() to an earlier place where we hold the pci lock.

This patch fixes three bugs:

1) set hpdev->pci_slot to NULL
2) move code destroying the slot inside a locked region in
   hv_eject_device_work()
3) Add missing slots removal code in hv_pci_remove()

We need three patches, not one.

(1) and (2), I am not entirely sure we want them in stable kernels,
since they are potential bugs, waiting for your input.

Lorenzo

> Fixes: a15f2c08c708 ("PCI: hv: support reporting serial number as slot 
> information")
> Signed-off-by: Dexuan Cui 
> Cc: sta...@vger.kernel.org
> Cc: Stephen Hemminger 
> ---
>  drivers/pci/controller/pci-hyperv.c | 23 ---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-hyperv.c 
> b/drivers/pci/controller/pci-hyperv.c
> index 9ba4d12c179c..6b4773727525 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -1491,6 +1491,21 @@ static void hv_pci_assign_slots(struct 
> hv_pcibus_device *hbus)
>   }
>  }
>  
> +/*
> + * Remove entries in sysfs pci slot directory.
> + */
> +static void hv_pci_remove_slots(struct hv_pcibus_device *hbus)
> +{
> + struct hv_pci_dev *hpdev;
> +
> + list_for_each_entry(hpdev, >children, list_entry) {
> + if (!hpdev->pci_slot)
> + continue;
> + pci_destroy_slot(hpdev->pci_slot);
> + hpdev->pci_slot = NULL;
> + }
> +}
> +
>  /**
>   * create_root_hv_pci_bus() - Expose a new root PCI bus
>   * @hbus:Root PCI bus, as understood by this driver
> @@ -1887,6 +1902,10 @@ static void hv_eject_device_work(struct work_struct 
> *work)
>   pci_lock_rescan_remove();
>   pci_stop_and_remove_bus_device(pdev);
>   pci_dev_put(pdev);
> + if (hpdev->pci_slot) {
> + pci_destroy_slot(hpdev->pci_slot);
> + hpdev->pci_slot = NULL;
> + }
>   pci_unlock_rescan_remove();
>   }
>  
> @@ -1894,9 +1913,6 @@ static void hv_eject_device_work(struct work_struct 
> *work)
>   list_del(>list_entry);
>   spin_unlock_irqrestore(>hbus->device_list_lock, flags);
>  
> - if (hpdev->pci_slot)
> - pci_destroy_slot(hpdev->pci_slot);
> -
>   memset(, 0, sizeof(ctxt));
>   ejct_pkt = (struct pci_eject_response *)
>   ejct_pkt->message_type.type = PCI_EJECTION_COMPLETE;
> @@ -2682,6 +2698,7 @@ static int hv_pci_remove(struct hv_device *hdev)
>   pci_lock_rescan_remove();
>   pci_stop_root_bus(hbus->pci_bus);
>   pci_remove_root_bus(hbus->pci_bus);
> + hv_pci_remove_slots(hbus);
>   pci_unlock_rescan_remove();
>   hbus->state = hv_pcibus_removed;
>   }
> -- 
> 2.19.1
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] PCI: hv: Fix return value check in hv_pci_assign_slots()

[Lorenzo Pieralisi]

[+DaveM, netdev]

On Thu, Sep 20, 2018 at 06:40:31AM +, Wei Yongjun wrote:
> In case of error, the function pci_create_slot() returns ERR_PTR() and
> never returns NULL. The NULL test in the return value check should be
> replaced with IS_ERR().
>
> Fixes: a15f2c08c708 ("PCI: hv: support reporting serial number as slot 
> information")
> Signed-off-by: Wei Yongjun 
> ---
>  drivers/pci/controller/pci-hyperv.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/pci-hyperv.c 
> b/drivers/pci/controller/pci-hyperv.c
> index ee80e79..9ba4d12 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -1484,8 +1484,10 @@ static void hv_pci_assign_slots(struct 
> hv_pcibus_device *hbus)
>   snprintf(name, SLOT_NAME_SIZE, "%u", hpdev->desc.ser);
>   hpdev->pci_slot = pci_create_slot(hbus->pci_bus, slot_nr,
> name, NULL);
> - if (!hpdev->pci_slot)
> + if (IS_ERR(hpdev->pci_slot)) {
>   pr_warn("pci_create slot %s failed\n", name);
> + hpdev->pci_slot = NULL;
> + }
>   }
>  }
>

FYI

I am dropping this patch from the PCI queue since the original series
is now queued in net-next.

Lorenzo
IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] PCI: hv: Fix return value check in hv_pci_assign_slots()

[Lorenzo Pieralisi]

[+DaveM, netdev]

Removed erroneously added disclaimer, apologies.

On Thu, Sep 20, 2018 at 06:40:31AM +, Wei Yongjun wrote:
> In case of error, the function pci_create_slot() returns ERR_PTR() and
> never returns NULL. The NULL test in the return value check should be
> replaced with IS_ERR().
> 
> Fixes: a15f2c08c708 ("PCI: hv: support reporting serial number as slot 
> information")
> Signed-off-by: Wei Yongjun 
> ---
>  drivers/pci/controller/pci-hyperv.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pci-hyperv.c 
> b/drivers/pci/controller/pci-hyperv.c
> index ee80e79..9ba4d12 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -1484,8 +1484,10 @@ static void hv_pci_assign_slots(struct 
> hv_pcibus_device *hbus)
>   snprintf(name, SLOT_NAME_SIZE, "%u", hpdev->desc.ser);
>   hpdev->pci_slot = pci_create_slot(hbus->pci_bus, slot_nr,
> name, NULL);
> - if (!hpdev->pci_slot)
> + if (IS_ERR(hpdev->pci_slot)) {
>   pr_warn("pci_create slot %s failed\n", name);
> + hpdev->pci_slot = NULL;
> + }
>   }
>  }

I am dropping this patch from the PCI queue since the original series
is now queued in net-next.

Lorenzo

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 0/2] hv_netvsc: associate VF and PV device by serial number

[Lorenzo Pieralisi]

On Fri, Sep 14, 2018 at 12:54:55PM -0700, Stephen Hemminger wrote:
> The Hyper-V implementation of PCI controller has concept of 32 bit serial 
> number
> (not to be confused with PCI-E serial number).  This value is sent in the 
> protocol
> from the host to indicate SR-IOV VF device is attached to a synthetic NIC.
> 
> Using the serial number (instead of MAC address) to associate the two devices
> avoids lots of potential problems when there are duplicate MAC addresses from
> tunnels or layered devices.
> 
> The patch set is broken into two parts, one is for the PCI controller
> and the other is for the netvsc device. Normally, these go through different
> trees but sending them together here for better review. The PCI changes
> were submitted previously, but the main review comment was "why do you
> need this?". This is why.

The question was more whether we should convert this serial number into
a PCI slot number (that has user space visibility and that is what you are
after) to improve the current matching, I do not question why you need
it, just for the records.

Lorenzo

> v2 - slot name can be shorter.
>  remove locking when creating pci_slots; see comment for explaination
> 
> Stephen Hemminger (2):
>   PCI: hv: support reporting serial number as slot information
>   hv_netvsc: pair VF based on serial number
> 
>  drivers/net/hyperv/netvsc.c |  3 ++
>  drivers/net/hyperv/netvsc_drv.c | 58 -
>  drivers/pci/controller/pci-hyperv.c | 37 ++
>  3 files changed, 73 insertions(+), 25 deletions(-)
> 
> -- 
> 2.18.0
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 2/2] hv_netvsc: pair VF based on serial number

[Lorenzo Pieralisi]

On Fri, Sep 14, 2018 at 12:54:57PM -0700, Stephen Hemminger wrote:
> Matching network device based on MAC address is problematic
> since a non VF network device can be creted with a duplicate MAC
> address causing confusion and problems.  The VMBus API does provide
> a serial number that is a better matching method.
> 
> Signed-off-by: Stephen Hemminger 
> ---
>  drivers/net/hyperv/netvsc.c |  3 ++
>  drivers/net/hyperv/netvsc_drv.c | 58 +++--
>  2 files changed, 36 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> index 31c3d77b4733..fe01e141c8f8 100644
> --- a/drivers/net/hyperv/netvsc.c
> +++ b/drivers/net/hyperv/netvsc.c
> @@ -1203,6 +1203,9 @@ static void netvsc_send_vf(struct net_device *ndev,
>  
>   net_device_ctx->vf_alloc = nvmsg->msg.v4_msg.vf_assoc.allocated;
>   net_device_ctx->vf_serial = nvmsg->msg.v4_msg.vf_assoc.serial;
> + netdev_info(ndev, "VF slot %u %s\n",
> + net_device_ctx->vf_serial,
> + net_device_ctx->vf_alloc ? "added" : "removed");
>  }
>  
>  static  void netvsc_receive_inband(struct net_device *ndev,
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index 1121a1ec407c..9dedc1463e88 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -1894,20 +1894,6 @@ static void netvsc_link_change(struct work_struct *w)
>   rtnl_unlock();
>  }
>  
> -static struct net_device *get_netvsc_bymac(const u8 *mac)
> -{
> - struct net_device_context *ndev_ctx;
> -
> - list_for_each_entry(ndev_ctx, _dev_list, list) {
> - struct net_device *dev = hv_get_drvdata(ndev_ctx->device_ctx);
> -
> - if (ether_addr_equal(mac, dev->perm_addr))
> - return dev;
> - }
> -
> - return NULL;
> -}
> -
>  static struct net_device *get_netvsc_byref(struct net_device *vf_netdev)
>  {
>   struct net_device_context *net_device_ctx;
> @@ -2036,26 +2022,48 @@ static void netvsc_vf_setup(struct work_struct *w)
>   rtnl_unlock();
>  }
>  
> +/* Find netvsc by VMBus serial number.
> + * The PCI hyperv controller records the serial number as the slot.
> + */
> +static struct net_device *get_netvsc_byslot(const struct net_device 
> *vf_netdev)
> +{
> + struct device *parent = vf_netdev->dev.parent;
> + struct net_device_context *ndev_ctx;
> + struct pci_dev *pdev;
> +
> + if (!parent || !dev_is_pci(parent))
> + return NULL; /* not a PCI device */
> +
> + pdev = to_pci_dev(parent);
> + if (!pdev->slot) {
> + netdev_notice(vf_netdev, "no PCI slot information\n");
> + return NULL;
> + }
> +
> + list_for_each_entry(ndev_ctx, _dev_list, list) {
> + if (!ndev_ctx->vf_alloc)
> + continue;
> +
> + if (ndev_ctx->vf_serial == pdev->slot->number)
> + return hv_get_drvdata(ndev_ctx->device_ctx);

In patch 1, pdev->slot->number is set to:

PCI_SLOT(wslot_to_devfn(hpdev->desc.win_slot.slot))

so I assume vf_serial is initialized (I have no knowledge of VMBUS and
hyper-V internals) to that value, somehow.

I also do not know how the wslot stuff is handled but I assume 5 bits
(ie dev bits in devfn) are enough.

BTW, I have noticed this patch (and patch 1) are already in -next so I will
drop them from the PCI patch queue.

Lorenzo

> + }
> +
> + netdev_notice(vf_netdev,
> +   "no netdev found for slot %u\n", pdev->slot->number);
> + return NULL;
> +}
> +
>  static int netvsc_register_vf(struct net_device *vf_netdev)
>  {
> - struct net_device *ndev;
>   struct net_device_context *net_device_ctx;
> - struct device *pdev = vf_netdev->dev.parent;
>   struct netvsc_device *netvsc_dev;
> + struct net_device *ndev;
>   int ret;
>  
>   if (vf_netdev->addr_len != ETH_ALEN)
>   return NOTIFY_DONE;
>  
> - if (!pdev || !dev_is_pci(pdev) || dev_is_pf(pdev))
> - return NOTIFY_DONE;
> -
> - /*
> -  * We will use the MAC address to locate the synthetic interface to
> -  * associate with the VF interface. If we don't find a matching
> -  * synthetic interface, move on.
> -  */
> - ndev = get_netvsc_bymac(vf_netdev->perm_addr);
> + ndev = get_netvsc_byslot(vf_netdev);
>   if (!ndev)
>   return NOTIFY_DONE;
>  
> -- 
> 2.18.0
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] PCI: hv: Disable/enable irq rather than bh in hv_compose_msi_msg()

[Lorenzo Pieralisi]

On Sun, Jul 01, 2018 at 06:22:23PM +, Dexuan Cui wrote:
> 
> Commit de0aa7b2f97d ("PCI: hv: Fix 2 hang issues in hv_compose_msi_msg()")
> uses local_bh_disable()/enable(), because hv_pci_onchannelcallback() can
> also run in tasklet context as the channel event callback, and here we
> want to avoid the race.
> 
> With CONFIG_PROVE_LOCKING=y in the recent mainline, or old kernels that
> don't have commit f71b74bca637 ("irq/softirqs: Use lockdep to assert IRQs
> are disabled/enabled"), when the upper layer irq code calls
> hv_compose_msi_msg() with local irq DISABLED, we'll see a warning at the
> beginning of __local_bh_enable_ip():
> 
> IRQs not enabled as expected
>   WARNING: CPU: 0 PID: 408 at kernel/softirq.c:162 __local_bh_enable_ip
> 
> The warning exposes an issue in de0aa7b2f97d: local_bh_enable() can
> potentially call do_softirq(), which is not supposed to run when local
> irq is DISABLED. Let's fix this by using local_irq_save()/restore()
> instead.
> 
> Note: hv_pci_onchannelcallback() is not a hot path because it's only
> called when the PCI device is hot added and removed, which is infrequent.
> 
> Fixes: de0aa7b2f97d ("PCI: hv: Fix 2 hang issues in hv_compose_msi_msg()")
> Signed-off-by: Dexuan Cui 
> Reviewed-by: Haiyang Zhang 
> Cc: 
> Cc: Stephen Hemminger 
> Cc: K. Y. Srinivasan 
> ---
> 
> A trimmed version of the warning is:
> 
> IRQs not enabled as expected
> WARNING: CPU: 0 PID: 408 at kernel/softirq.c:162 
> __local_bh_enable_ip+0xb0/0xe0
> Call Trace:
>  hv_compose_msi_msg+0x209/0x462 [pci_hyperv]
>  irq_chip_compose_msi_msg+0x41/0x50
>  msi_domain_activate+0x1a/0x40
>  __irq_domain_activate_irq+0x59/0x90
>  irq_domain_activate_irq+0x25/0x40
>  __setup_irq+0x3ec/0x730
> request_threaded_irq+0xfa/0x1a0
> mlx4_init_eq_table+0x3c3/0x5f0 [mlx4_core]
> mlx4_setup_hca+0x1db/0x750 [mlx4_core]
> mlx4_load_one+0xad2/0x13b0 [mlx4_core]
> mlx4_init_one+0x578/0x710 [mlx4_core]
> local_pci_probe+0x1e/0x50
> work_for_cpu_fn+0x10/0x20
> process_one_work+0x1d4/0x5a0
> worker_thread+0x1cb/0x3d0
> kthread+0xf5/0x130
> 
> 
> Changes since v1:
>   Updated the changelog only (fixed typos and some inaccuracy)
> 
> 
>  drivers/pci/controller/pci-hyperv.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)

Applied to pci/controller-fixes, to be tentatively sent for an
upcoming -rc, thanks.

Lorenzo

> diff --git a/drivers/pci/controller/pci-hyperv.c 
> b/drivers/pci/controller/pci-hyperv.c
> index ba1d4b5..eb20296 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -1073,6 +1073,7 @@ static void hv_compose_msi_msg(struct irq_data *data, 
> struct msi_msg *msg)
>   struct pci_bus *pbus;
>   struct pci_dev *pdev;
>   struct cpumask *dest;
> + unsigned long flags;
>   struct compose_comp_ctxt comp;
>   struct tran_int_desc *int_desc;
>   struct {
> @@ -1164,14 +1165,15 @@ static void hv_compose_msi_msg(struct irq_data *data, 
> struct msi_msg *msg)
>* the channel callback directly when channel->target_cpu is
>* the current CPU. When the higher level interrupt code
>* calls us with interrupt enabled, let's add the
> -  * local_bh_disable()/enable() to avoid race.
> +  * local_irq_save()/restore() to avoid race:
> +  * hv_pci_onchannelcallback() can also run in tasklet.
>*/
> - local_bh_disable();
> + local_irq_save(flags);
>  
>   if (hbus->hdev->channel->target_cpu == smp_processor_id())
>   hv_pci_onchannelcallback(hbus);
>  
> - local_bh_enable();
> + local_irq_restore(flags);
>  
>   if (hpdev->state == hv_pcichild_ejecting) {
>   dev_err_once(>hdev->device,
> -- 
> 2.7.4
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] pci: host: pci-hyperv: Replace GFP_ATOMIC with GFP_KERNEL in new_pcichild_device

[Lorenzo Pieralisi]

On Sun, Mar 18, 2018 at 10:53:28PM +0800, Jia-Ju Bai wrote:
> new_pcichild_device() is not called in atomic context.
> 
> The call chain ending up at new_pcichild_device() is:
> [1] new_pcichild_device() <- pci_devices_present_work()
> pci_devices_present_work() is only set in INIT_WORK().
> 
> Despite never getting called from atomic context,
> new_pcichild_device() calls kzalloc with GFP_ATOMIC,
> which waits busily for allocation.
> GFP_ATOMIC is not necessary and can be replaced with GFP_KERNEL
> to avoid busy waiting.
> 
> This is found by a static analysis tool named DCNS written by myself.
> 
> Signed-off-by: Jia-Ju Bai 
> ---
>  drivers/pci/host/pci-hyperv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied to pci/hv for v4.19, thanks.

Lorenzo

> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 0fe3ea1..289e31d 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -1500,7 +1500,7 @@ static struct hv_pci_dev *new_pcichild_device(struct 
> hv_pcibus_device *hbus,
>   unsigned long flags;
>   int ret;
>  
> - hpdev = kzalloc(sizeof(*hpdev), GFP_ATOMIC);
> + hpdev = kzalloc(sizeof(*hpdev), GFP_KERNEL);
>   if (!hpdev)
>   return NULL;
>  
> -- 
> 1.9.1
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in hv_compose_msi_msg()

[Lorenzo Pieralisi]

On Wed, Jun 13, 2018 at 10:50:05PM +, Dexuan Cui wrote:
> > From: Bjorn Helgaas 
> > Sent: Wednesday, June 13, 2018 15:15
> > > ...
> > > It looks Lorenzo's pci.git tree has not been updated for 3+ weeks.
> > > I guess Lorenzo may be on vacation.
> > >
> > > @Bjorn, can this patch go through your tree?
> > > Should I resubmit it?
> > 
> > No need to resubmit it, Lorenzo has been out for a bit, but I'm sure
> > he'll pick this up as he catches up.
> OK, I see. Thanks!
>  
> > You might, however, fix the commit log:
> > 
> >   This is not an issue because hv_pci_onchannelcallback() is not slow,
> >   and it not a hot path.
> > 
> > This has at least one typo (I think you mean "and *is* not a hot
> > path").
> Sorry -- yes, it's a typo. I hope Lorenzo can help to fix this, or I can
> resubmit it if Lorenzo or you want me to do it.
>  
> > I also don't understand the sentence as a whole because the
> > hv_pci_onchannelcallback() comment says it's called whenever the host
> > sends a packet to this channel, and that *does* sound like a hot path.
> Sorry for not making it clear.
> The host only sends a packet into the channel of the guest when there
> is a change of device configuration (i.e. hot add or remove a device), or
> the host is responding to the guest's request.
> 
> The change of device configuration is only triggered on-demand by the
> administrator on the host, and the guest's requests are one-off when
> the device is probed.
> 
> So IMO the callback is not a hot path.
> 
> > I also don't understand the "hv_pci_onchannelcallback() is not slow"
> > part.  In other words, you're saying hv_pci_onchannelcallback() is
> > fast and it's not a hot path.  And apparently this has something to do
> > with the difference between local_bh_disable() and local_irq_save()?
> > 
> > Bjorn
> Actually in my original internal version of the patch, I did use 
> local_irq_save/restore().
> 
> hv_pci_onchannelcallback() itself runs fast, but here since it's in a
> loop (i.e. the while (!try_wait_for_completion(_pkt.host_event)
> loop), IIRC I was asked if I really need local_irq_save/restore(),
> and I answered "not really", so later I switched to 
> local_bh_disable()/enable().
> 
> However, recently I found that if we enable CONFIG_PROVE_LOCKING=y,
> the local_bh_enable() can trigger a warning because the function 
> hv_compose_msi_msg() can be called with local IRQs disabled (BTW,
> hv_compose_msi_msg() can also be called with local IRQS enabled in
> another code path):
> 
> IRQs not enabled as expected
>   WARNING: CPU: 0 PID: 408 at kernel/softirq.c:162 __local_bh_enable_ip
> 
> Despite the warning, the code itself can still work correctly, but IMO we'd
> better switch back to local_irq_save/restore(), and hence I made the patch.
> 
> I hope the explanation sounds reasonable. :-)

Sorry for the delay in replying. I need to understand if you are
preventing a spurious lockdep warning or you are fixing a kernel
bug. From your commit log, I assume the former option but I do
not think that's what you are really doing.

Apart from the commit log typos fixes I would like a log that
explains *why* this is not a kernel bug fix rather than a harmless
lockdep warning prevention.

Thanks,
Lorenzo
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] PCI: hv: fix spelling mistake: "reqquest" -> "request"

[Lorenzo Pieralisi]

On Tue, May 08, 2018 at 10:49:46PM +0100, Colin King wrote:
> From: Colin Ian King 
> 
> Trivial fix to spelling mistake in dev_err error message
> 
> Signed-off-by: Colin Ian King 
> ---
>  drivers/pci/host/pci-hyperv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Hi Colin,

patch change was superseded, I will drop it from patchwork, thanks
for sending it anyway.

Lorenzo

> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 50cdefe3f6d3..7e93b4ffb162 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -2071,7 +2071,7 @@ static int hv_pci_protocol_negotiation(struct hv_device 
> *hdev)
>   VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
>   if (ret) {
>   dev_err(>device,
> - "PCI Pass-through VSP failed sending version 
> reqquest: %#x",
> + "PCI Pass-through VSP failed sending version 
> request: %#x",
>   ret);
>   goto exit;
>   }
> -- 
> 2.17.0
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] PCI: hv: Do not wait forever on a device that has disappeared

[Lorenzo Pieralisi]

On Wed, May 23, 2018 at 09:12:01PM +, Dexuan Cui wrote:
> 
> Before the guest finishes the device initialization, the device can be
> removed anytime by the host, and after that the host won't respond to
> the guest's request, so the guest should be prepared to handle this
> case.
> 
> Signed-off-by: Dexuan Cui 
> Cc: Stephen Hemminger 
> Cc: K. Y. Srinivasan 
> ---
>  drivers/pci/host/pci-hyperv.c | 46 
> ---
>  1 file changed, 34 insertions(+), 12 deletions(-)

Applied with a minor tweak to the commit log to pci/hv for v4.18.

Thanks,
Lorenzo

> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index ad6a64d..248765f 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -556,6 +556,26 @@ static void put_pcichild(struct hv_pci_dev *hv_pcidev,
>  static void get_hvpcibus(struct hv_pcibus_device *hv_pcibus);
>  static void put_hvpcibus(struct hv_pcibus_device *hv_pcibus);
>  
> +/*
> + * There is no good way to get notified from vmbus_onoffer_rescind(),
> + * so let's use polling here, since this is not a hot path.
> + */
> +static int wait_for_response(struct hv_device *hdev,
> +  struct completion *comp)
> +{
> + while (true) {
> + if (hdev->channel->rescind) {
> + dev_warn_once(>device, "The device is gone.\n");
> + return -ENODEV;
> + }
> +
> + if (wait_for_completion_timeout(comp, HZ / 10))
> + break;
> + }
> +
> + return 0;
> +}
> +
>  /**
>   * devfn_to_wslot() - Convert from Linux PCI slot to Windows
>   * @devfn:   The Linux representation of PCI slot
> @@ -1569,7 +1589,8 @@ static struct hv_pci_dev *new_pcichild_device(struct 
> hv_pcibus_device *hbus,
>   if (ret)
>   goto error;
>  
> - wait_for_completion(_pkt.host_event);
> + if (wait_for_response(hbus->hdev, _pkt.host_event))
> + goto error;
>  
>   hpdev->desc = *desc;
>   refcount_set(>refs, 1);
> @@ -2070,15 +2091,16 @@ static int hv_pci_protocol_negotiation(struct 
> hv_device *hdev)
>   sizeof(struct pci_version_request),
>   (unsigned long)pkt, VM_PKT_DATA_INBAND,
>   VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> + if (!ret)
> + ret = wait_for_response(hdev, _pkt.host_event);
> +
>   if (ret) {
>   dev_err(>device,
> - "PCI Pass-through VSP failed sending version 
> reqquest: %#x",
> + "PCI Pass-through VSP failed to request 
> version: %d",
>   ret);
>   goto exit;
>   }
>  
> - wait_for_completion(_pkt.host_event);
> -
>   if (comp_pkt.completion_status >= 0) {
>   pci_protocol_version = pci_protocol_versions[i];
>   dev_info(>device,
> @@ -2287,11 +2309,12 @@ static int hv_pci_enter_d0(struct hv_device *hdev)
>   ret = vmbus_sendpacket(hdev->channel, d0_entry, sizeof(*d0_entry),
>  (unsigned long)pkt, VM_PKT_DATA_INBAND,
>  VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> + if (!ret)
> + ret = wait_for_response(hdev, _pkt.host_event);
> +
>   if (ret)
>   goto exit;
>  
> - wait_for_completion(_pkt.host_event);
> -
>   if (comp_pkt.completion_status < 0) {
>   dev_err(>device,
>   "PCI Pass-through VSP failed D0 Entry with status %x\n",
> @@ -2331,11 +2354,10 @@ static int hv_pci_query_relations(struct hv_device 
> *hdev)
>  
>   ret = vmbus_sendpacket(hdev->channel, , sizeof(message),
>  0, VM_PKT_DATA_INBAND, 0);
> - if (ret)
> - return ret;
> + if (!ret)
> + ret = wait_for_response(hdev, );
>  
> - wait_for_completion();
> - return 0;
> + return ret;
>  }
>  
>  /**
> @@ -2405,11 +2427,11 @@ static int hv_send_resources_allocated(struct 
> hv_device *hdev)
>   size_res, (unsigned long)pkt,
>   VM_PKT_DATA_INBAND,
>   VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> + if (!ret)
> + ret = wait_for_response(hdev, _pkt.host_event);
>   if (ret)
>   break;
>  
> - wait_for_completion(_pkt.host_event);
> -
>   if (comp_pkt.completion_status < 0) {
>   ret = -EPROTO;
>   dev_err(>device,
> -- 
> 2.7.4
> 
___
devel mailing list
de...@linuxdriverproject.org

Re: [PATCH] PCI: hv: Do not wait forever on a device that has disappeared

[Lorenzo Pieralisi]

On Thu, May 24, 2018 at 11:55:35PM +, Dexuan Cui wrote:
> > From: Lorenzo Pieralisi <lorenzo.pieral...@arm.com>
> > Sent: Thursday, May 24, 2018 05:41
> > On Wed, May 23, 2018 at 09:12:01PM +, Dexuan Cui wrote:
> > >
> > > Before the guest finishes the device initialization, the device can be
> > > removed anytime by the host, and after that the host won't respond to
> > > the guest's request, so the guest should be prepared to handle this
> > > case.
> > >
> > > --- a/drivers/pci/host/pci-hyperv.c
> > > +++ b/drivers/pci/host/pci-hyperv.c
> > > @@ -556,6 +556,26 @@ static void put_pcichild(struct hv_pci_dev
> > *hv_pcidev,
> > >  static void get_hvpcibus(struct hv_pcibus_device *hv_pcibus);
> > >  static void put_hvpcibus(struct hv_pcibus_device *hv_pcibus);
> > >
> > > +/*
> > > + * There is no good way to get notified from vmbus_onoffer_rescind(),
> > > + * so let's use polling here, since this is not a hot path.
> > > + */
> > > +static int wait_for_response(struct hv_device *hdev,
> > > +  struct completion *comp)
> > > +{
> > > + while (true) {
> > > + if (hdev->channel->rescind) {
> > > + dev_warn_once(>device, "The device is gone.\n");
> > > + return -ENODEV;
> > > + }
> > > +
> > > + if (wait_for_completion_timeout(comp, HZ / 10))
> > > + break;
> > > + }
> > > +
> > > + return 0;
> > 
> > This is pretty racy, isn't it ? Also, I reckon you should consider the
> > timeout return value as an error condition unless I am completely
> > missing the point of what you are doing.
> > 
> > Lorenzo
> 
> Actually, this is not racy: we only exit the loop when 
> 1) the channel is rescinded 
> or
> 2) the channel is not rescinded, and the event is completed.
> 
> wait_for_completion_timeout() returns 0 if timed out: in this case,
> we keep spinning in the loop every 0.1 second, testing the 2 conditions.

Yes sorry, you are right, the exit condition is correct, I am waiting for
maintainers ACK to merge it, I need it as soon as possible if you want
this to make it for v4.18.

Thanks,
Lorenzo

> If the chanel is not rescinded, here we should wait for the event 
> forever, as the host is supposed to respond to us quickly, and the
> event will be completed accordingly. This is what the current code
> does. But, in case the channel is rescinded, we need to exit the loop 
> immediately with an error return value: this is the only change
> made by the patch.
> 
> Ideally, we should not use this ugly "polling" method, and the
> rescind-handler, i.e. vmbus_onoffer_rescind(), should notify 
> wait_for_response(), but as I mentioned, there is no good way
> to get notified from vmbus_onoffer_rescind(), so I'm proposing
> this "polling" method: it's simple and it can work correctly,
> and this is not a hot path.
> 
> Thanks,
> -- Dexuan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/3] PCI: hv: cleanup patches

[Lorenzo Pieralisi]

On Wed, May 23, 2018 at 10:11:11AM -0700, Stephen Hemminger wrote:
> These are minor code cleanups found while reviewing and implementing
> other things in Hyper-V PCI host driver.
> 
> Stephen Hemminger (3):
>   PCI: hv: remove unused reason for refcount handler
>   PCI: hv: convert remove_lock to refcount
>   PCI: hv: use list_for_each_entry
> 
>  drivers/pci/host/pci-hyperv.c | 105 --
>  1 file changed, 37 insertions(+), 68 deletions(-)

Applied to pci/hv for v4.18, thanks.

Lorenzo
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] PCI: hv: Do not wait forever on a device that has disappeared

[Lorenzo Pieralisi]

On Wed, May 23, 2018 at 09:12:01PM +, Dexuan Cui wrote:
> 
> Before the guest finishes the device initialization, the device can be
> removed anytime by the host, and after that the host won't respond to
> the guest's request, so the guest should be prepared to handle this
> case.
> 
> Signed-off-by: Dexuan Cui 
> Cc: Stephen Hemminger 
> Cc: K. Y. Srinivasan 
> ---
>  drivers/pci/host/pci-hyperv.c | 46 
> ---
>  1 file changed, 34 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index ad6a64d..248765f 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -556,6 +556,26 @@ static void put_pcichild(struct hv_pci_dev *hv_pcidev,
>  static void get_hvpcibus(struct hv_pcibus_device *hv_pcibus);
>  static void put_hvpcibus(struct hv_pcibus_device *hv_pcibus);
>  
> +/*
> + * There is no good way to get notified from vmbus_onoffer_rescind(),
> + * so let's use polling here, since this is not a hot path.
> + */
> +static int wait_for_response(struct hv_device *hdev,
> +  struct completion *comp)
> +{
> + while (true) {
> + if (hdev->channel->rescind) {
> + dev_warn_once(>device, "The device is gone.\n");
> + return -ENODEV;
> + }
> +
> + if (wait_for_completion_timeout(comp, HZ / 10))
> + break;
> + }
> +
> + return 0;

This is pretty racy, isn't it ? Also, I reckon you should consider the
timeout return value as an error condition unless I am completely
missing the point of what you are doing.

Lorenzo

> +}
> +
>  /**
>   * devfn_to_wslot() - Convert from Linux PCI slot to Windows
>   * @devfn:   The Linux representation of PCI slot
> @@ -1569,7 +1589,8 @@ static struct hv_pci_dev *new_pcichild_device(struct 
> hv_pcibus_device *hbus,
>   if (ret)
>   goto error;
>  
> - wait_for_completion(_pkt.host_event);
> + if (wait_for_response(hbus->hdev, _pkt.host_event))
> + goto error;
>  
>   hpdev->desc = *desc;
>   refcount_set(>refs, 1);
> @@ -2070,15 +2091,16 @@ static int hv_pci_protocol_negotiation(struct 
> hv_device *hdev)
>   sizeof(struct pci_version_request),
>   (unsigned long)pkt, VM_PKT_DATA_INBAND,
>   VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> + if (!ret)
> + ret = wait_for_response(hdev, _pkt.host_event);
> +
>   if (ret) {
>   dev_err(>device,
> - "PCI Pass-through VSP failed sending version 
> reqquest: %#x",
> + "PCI Pass-through VSP failed to request 
> version: %d",
>   ret);
>   goto exit;
>   }
>  
> - wait_for_completion(_pkt.host_event);
> -
>   if (comp_pkt.completion_status >= 0) {
>   pci_protocol_version = pci_protocol_versions[i];
>   dev_info(>device,
> @@ -2287,11 +2309,12 @@ static int hv_pci_enter_d0(struct hv_device *hdev)
>   ret = vmbus_sendpacket(hdev->channel, d0_entry, sizeof(*d0_entry),
>  (unsigned long)pkt, VM_PKT_DATA_INBAND,
>  VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> + if (!ret)
> + ret = wait_for_response(hdev, _pkt.host_event);
> +
>   if (ret)
>   goto exit;
>  
> - wait_for_completion(_pkt.host_event);
> -
>   if (comp_pkt.completion_status < 0) {
>   dev_err(>device,
>   "PCI Pass-through VSP failed D0 Entry with status %x\n",
> @@ -2331,11 +2354,10 @@ static int hv_pci_query_relations(struct hv_device 
> *hdev)
>  
>   ret = vmbus_sendpacket(hdev->channel, , sizeof(message),
>  0, VM_PKT_DATA_INBAND, 0);
> - if (ret)
> - return ret;
> + if (!ret)
> + ret = wait_for_response(hdev, );
>  
> - wait_for_completion();
> - return 0;
> + return ret;
>  }
>  
>  /**
> @@ -2405,11 +2427,11 @@ static int hv_send_resources_allocated(struct 
> hv_device *hdev)
>   size_res, (unsigned long)pkt,
>   VM_PKT_DATA_INBAND,
>   VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> + if (!ret)
> + ret = wait_for_response(hdev, _pkt.host_event);
>   if (ret)
>   break;
>  
> - wait_for_completion(_pkt.host_event);
> -
>   if (comp_pkt.completion_status < 0) {
>   ret = -EPROTO;
>   dev_err(>device,
> -- 
> 2.7.4
> 
___
devel 

Re: [PATCH v8] PCI: hv: Make sure the bus domain is really unique

[Lorenzo Pieralisi]

On Tue, May 01, 2018 at 05:56:32PM +, Sridhar Pitchai wrote:
> When Linux runs as a guest VM in Hyper-V and Hyper-V adds the virtual PCI
> bus to the guest, Hyper-V always provides unique PCI domain.
> 
> commit 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain")
> overrode unique domain with the serial number of the first device added to
> the virtual PCI bus.
> 
> The reason for that patch was to have a consistent and short name for the
> device, but Hyper-V doesn't provide unique serial numbers. Using non-unique
> serial numbers as domain IDs leads to duplicate device addresses, which
> causes PCI bus registration to fail.
> 
> commit 0c195567a8f6 ("netvsc: transparent VF management") avoids the need
> for commit 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI
> domain").  When scripts were used to configure VF devices, the name of
> the VF needed to be consistent and short, but with commit 0c195567a8f6
> ("netvsc: transparent VF management") all the setup is done in the kernel,
> and we do not need to maintain consistent name.
> 
> Revert commit 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI
> domain") so we can reliably support multiple devices being assigned to
> a guest.
> 
> This revert should only be backported to kernels that contain commit
> 0c195567a8f6 ("netvsc: transparent VF management").

I removed this last paragraph, the information is in the stable tag
below.

> Fixes: 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain")
> Signed-off-by: Sridhar Pitchai <sridhar.pitc...@microsoft.com>
> Cc: sta...@vger.kernel.org # v4.14+
> Reviewed-by: Bjorn Helgaas <bhelg...@google.com>
> 
> ---
> Changes in v8:
> * fix the commit comment. [Lorenzo Pieralisi, Bjorn Helgaas]
> ---
>  drivers/pci/host/pci-hyperv.c | 11 ---
>  1 file changed, 11 deletions(-)

Applied to pci/hv for v4.18, thanks.

Lorenzo

> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 2faf38eab785..ac67e56e451a 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -1518,17 +1518,6 @@ static struct hv_pci_dev *new_pcichild_device(struct 
> hv_pcibus_device *hbus,
>   get_pcichild(hpdev, hv_pcidev_ref_childlist);
>   spin_lock_irqsave(>device_list_lock, flags);
>  
> - /*
> -  * When a device is being added to the bus, we set the PCI domain
> -  * number to be the device serial number, which is non-zero and
> -  * unique on the same VM.  The serial numbers start with 1, and
> -  * increase by 1 for each device.  So device names including this
> -  * can have shorter names than based on the bus instance UUID.
> -  * Only the first device serial number is used for domain, so the
> -  * domain number will not change after the first device is added.
> -  */
> - if (list_empty(>children))
> - hbus->sysdata.domain = desc->ser;
>   list_add_tail(>list_entry, >children);
>   spin_unlock_irqrestore(>device_list_lock, flags);
>   return hpdev;
> -- 
> 2.14.1
> 
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7] Revert "PCI: hv: Use device serial number as PCI domain"

[Lorenzo Pieralisi]

On Thu, Apr 12, 2018 at 03:56:42PM +, Sridhar Pitchai wrote:
> > 
> > >> I am still not happy with this patch.
> > >>
> > >> -  You do not explain at all the dependency on commit 
> 0c195567a8f6 and
> > >>you should because that's fundamental, if that patch is not 
> present
> > >>this revert breaks the kernel as per previous discussions[1].
> > >> -  You are sending this patch to all stable kernels that contain 
> the
> > >>commit you are fixing - some that may not contain the commit 
> above
> > >>(that was merged in v4.14), you are breaking those kernels, 
> if not
> > >>explain me why please
> > 
> > >If there's a dependency on 0c195567a8f6, I totally agree that
> > >needs to be cleared up.  I was assuming that turned out to be
> > >irrelevant.
> > That is right. There is no dependency on 0c195567a8f6. We just need to 
> revert
> > 4a9b0933bdfc.
> 
> > This patch should only be applied to later versions after  
> 0c195567a8f6" (transparent VF).
> > Otherwise it causes long & random names of VF NICs for bonding. That 
> will make bonding
> > config difficult, especially for auto config.
> 
> >Thanks,
> >- Haiyang
> Ok. I will update the patch, as Lorenzo suggested. 

Please update your email client quoting policy so that it does not add
unnecessary indentation spaces, for long threads this gets messy.

If you send a patch to a specific stable version you have to test it
first thing on that stable version before adding a stable tag, reverting
a patch from stable kernels is the last thing we want to do - that's the
reason why I keep complaining.

Thanks,
Lorenzo
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7] Revert "PCI: hv: Use device serial number as PCI domain"

[Lorenzo Pieralisi]

On Thu, Apr 12, 2018 at 02:44:42AM +, Sridhar Pitchai wrote:
> When Linux runs as a guest VM in Hyper-V and Hyper-V adds the virtual PCI
> bus to the guest, Hyper-V always provides unique PCI domain.
> 
> commit 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain")
> overrode unique domain with the serial number of the first device added to
> the virtual PCI bus.
> 
> The reason for that patch was to have a consistent and short name for the
> device, but Hyper-V doesn't provide unique serial numbers. Using non-unique
> serial numbers as domain IDs leads to duplicate device addresses, which
> causes PCI bus registration to fail.
> 
> Revert commit 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI
> domain") so we can reliably support multiple devices being assigned to
> a guest.
> 
> Fixes: 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain")
> Signed-off-by: Sridhar Pitchai 
> Cc: sta...@vger.kernel.org

I am still not happy with this patch.

-  You do not explain at all the dependency on commit 0c195567a8f6 and
   you should because that's fundamental, if that patch is not present
   this revert breaks the kernel as per previous discussions[1].
-  You are sending this patch to all stable kernels that contain the
   commit you are fixing - some that may not contain the commit above
   (that was merged in v4.14), you are breaking those kernels, if not
   explain me why please

You must mark the stable kernels you want this revert to be applied to
eg:

Cc:  # v4.14+

and for kernels that do not contain the 0c195567a8f6 commit you have to
add the dependency. Please read the documentation Greg provided you in
relation to stable kernel rules.

Use:

git tag --contains

to detect in what kernel version the given commits are present.

[1] https://marc.info/?l=linux-pci=152158684221212=2

> Reviewed-by: Bjorn Helgaas 
> 
> ---
> Changes in v7:
> * fix the commit comment. [Bjorn Helgaas]
> ---
>  drivers/pci/host/pci-hyperv.c | 11 ---
>  1 file changed, 11 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 2faf38eab785..ac67e56e451a 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -1518,17 +1518,6 @@ static struct hv_pci_dev *new_pcichild_device(struct 
> hv_pcibus_device *hbus,
>   get_pcichild(hpdev, hv_pcidev_ref_childlist);
>   spin_lock_irqsave(>device_list_lock, flags);
>  
> - /*
> -  * When a device is being added to the bus, we set the PCI domain
> -  * number to be the device serial number, which is non-zero and
> -  * unique on the same VM.  The serial numbers start with 1, and
> -  * increase by 1 for each device.  So device names including this
> -  * can have shorter names than based on the bus instance UUID.
> -  * Only the first device serial number is used for domain, so the
> -  * domain number will not change after the first device is added.
> -  */
> - if (list_empty(>children))
> - hbus->sysdata.domain = desc->ser;
>   list_add_tail(>list_entry, >children);
>   spin_unlock_irqrestore(>device_list_lock, flags);
>   return hpdev;
> -- 
> 2.14.1
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption

[Lorenzo Pieralisi]

On Tue, Mar 20, 2018 at 11:00:36PM +, Sridhar Pitchai wrote:
> Hi Lorenzo,
>Transparent SRIOV is exposing the NIC directly to the kernel via
>para-virtual device, unlike creating a netdev and associating it
>with the bond driver. Further descriptions here,
>
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=0c195567a8f6e82ea5535cd9f1d54a1626dd233e
> 
> Previously, when using the bond driver, unique and persistent VF NIC
> name was required, so we used serial number as PCI domain which is
> included as part of the VF NIC name.  Transparent SRIOV mode puts VF
> NIC based on MAC match as a slave of synthetic NIC, so VF NIC’s name
> is no longer important.

Please read the link I sent you in relation to email formatting.

Then add your description above in a way that anyone not 100% familiar
with hyperv can understand it - that's what the commit log is for.

You are sending this patch to stable kernels, patch above has been in
the kernel from v4.14. The patch you are fixing since v4.11, you ought
to be careful since you do not want to have broken kernel versions owing
to stable patches mismatches, that's why I asked and I will ask again,
are you sure you won't trigger a regression by sending this fix to
stable ?

I assume the bond driver mechanism is now done and dusted.

Thanks,
Lorenzo

> Thanks,
> Sridhar
> 
> On 3/20/18, 11:32 AM, "Lorenzo Pieralisi" <lorenzo.pieral...@arm.com> wrote:
> 
> On Tue, Mar 20, 2018 at 05:56:15PM +, Sridhar Pitchai wrote:
> > Hi Lorenzo,
> 
> > Are we good with the explanation? Can I send the patch with the
> > updated commit comments?
> 
> Almost.
> 
> [...]
> 
> > Since we have the transparent SRIOV mode now, the short VF device 
> name
> > is no longer needed.
> 
> Can you correlate transparent SRIOV mode to the point you are making
> below ? Please explain what transparent SRIOV mode allows you to remove
> and why. The rest of the explanation seems OK.
> 
> Please follow this email format:
> 
> 
> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.kernel.org%2Flkml%2F%23s3-9=04%7C01%7CSridhar.Pitchai%40microsoft.com%7Cc5cdcb7951f64318e52708d58e90e6f2%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636571675366181738%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwifQ%3D%3D%7C-1=yBdqc4NQZsO7O9vfgJsr5olU8GfLNjF5e9EAaCb7vq4%3D=0
> 
> Thanks,
> Lorenzo
> 
> > I still do not understand what this means and how it is related to 
> the
> > patch below, it may be clear to you, it is not to me, at all.
> > 
> > Sridhar >> the patch below, was introduced to make the device name 
> small, by taking only
> > 16bits of the serial number. Since we are not going to have the 
> serial number
> > updated to the BUS id, this has to be removed.
> > 
> > Fixes: 4a9b0933bdfc("PCI:hv:Use device serial number as PCI domain")
> > 
> > Fixes: 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI 
> domain")
> > Sridhr >> yes
> > 
> > I asked you an explicit question. Commit above was added for a 
> reason
> > I assume. This patch implies that kernel has been broken since v4.11
> > which is almost a year ago and nobody every noticed ? Or there are
> > systems where commit above is _necessary_ and this patch would break
> > them ?
> > 
> > I want a detailed explanation that highlights *why* it is safe to 
> apply
> > this patch and send it to stable kernels, commit log above won't do.
> > 
> > Sridhar>> HyperV provides a unique domain ID for PCI BUS. But it is 
> modified by the child
> > device when it is added. This cannot produce a unique domain ID all 
> the time.
> > Here in the bug, we see the collision between the serial number and 
> already
> > existing PCI bus. The cleaner way is never touch the domain ID 
> provided by
> > hyperV during the PCI bus creation. As long as hyperV make sure it 
> provides a
> > unique domain ID for the PCI for a VM it will not break, and HyperV 
> will
> > guarantees that the domain for the PCI bus for a given VM will be 
> always unique.
> > The original patch was also intending to have a unique domain ID 
> for the PCI
> > bus, by taking the serial number of the device, but it is not 
> sufficient, when
>

Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption

[Lorenzo Pieralisi]

On Tue, Mar 20, 2018 at 05:56:15PM +, Sridhar Pitchai wrote:
> Hi Lorenzo,

> Are we good with the explanation? Can I send the patch with the
> updated commit comments?

Almost.

[...]

> Since we have the transparent SRIOV mode now, the short VF device name
> is no longer needed.

Can you correlate transparent SRIOV mode to the point you are making
below ? Please explain what transparent SRIOV mode allows you to remove
and why. The rest of the explanation seems OK.

Please follow this email format:

http://vger.kernel.org/lkml/#s3-9

Thanks,
Lorenzo

> I still do not understand what this means and how it is related to the
> patch below, it may be clear to you, it is not to me, at all.
> 
> Sridhar >> the patch below, was introduced to make the device name small, 
> by taking only
> 16bits of the serial number. Since we are not going to have the serial 
> number
> updated to the BUS id, this has to be removed.
> 
> Fixes: 4a9b0933bdfc("PCI:hv:Use device serial number as PCI domain")
> 
> Fixes: 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain")
> Sridhr >> yes
> 
> I asked you an explicit question. Commit above was added for a reason
> I assume. This patch implies that kernel has been broken since v4.11
> which is almost a year ago and nobody every noticed ? Or there are
> systems where commit above is _necessary_ and this patch would break
> them ?
> 
> I want a detailed explanation that highlights *why* it is safe to apply
> this patch and send it to stable kernels, commit log above won't do.
> 
> Sridhar>> HyperV provides a unique domain ID for PCI BUS. But it is 
> modified by the child
> device when it is added. This cannot produce a unique domain ID all the 
> time.
> Here in the bug, we see the collision between the serial number and 
> already
> existing PCI bus. The cleaner way is never touch the domain ID provided by
> hyperV during the PCI bus creation. As long as hyperV make sure it 
> provides a
> unique domain ID for the PCI for a VM it will not break, and HyperV will
> guarantees that the domain for the PCI bus for a given VM will be always 
> unique.
> The original patch was also intending to have a unique domain ID for the 
> PCI
> bus, by taking the serial number of the device, but it is not sufficient, 
> when
> the device serial number is number which is the domain ID of the existing 
> PCI
> bus.  With the current kernel we can repro this issue by adding a device 
> with a
> serial number matching the existing PCI bus domain id. (in this case that
> happens to be zero).
> 
> 
> Thanks,
> Lorenzo
> 
> Cc: sta...@vger.kernel.org
> Signed-off-by: Sridhar Pitchai 
> ---
> Changes in v3:
> * fix the commit comment. [KY Srinivasan, Michael Kelley]
> ---
>   drivers/pci/host/pci-hyperv.c | 11 ---
>   1 file changed, 11 deletions(-)
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 2faf38e..ac67e56 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -1518,17 +1518,6 @@ static struct hv_pci_dev 
> *new_pcichild_device(struct hv_pcibus_device *hbus,
>   get_pcichild(hpdev, hv_pcidev_ref_childlist);
>   spin_lock_irqsave(>device_list_lock, flags);
>   
> - /*
> -  * When a device is being added to the bus, we set the PCI domain
> -  * number to be the device serial number, which is non-zero and
> -  * unique on the same VM.  The serial numbers start with 1, and
> -  * increase by 1 for each device.  So device names including this
> -  * can have shorter names than based on the bus instance UUID.
> -  * Only the first device serial number is used for domain, so the
> -  * domain number will not change after the first device is added.
> -  */
> - if (list_empty(>children))
> - hbus->sysdata.domain = desc->ser;
>   list_add_tail(>list_entry, >children);
>   spin_unlock_irqrestore(>device_list_lock, flags);
>   return hpdev;
> --
> 2.7.4 
> 
> 
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 3/4] PCI: hv: Remove hbus->enum_sem

[Lorenzo Pieralisi]

On Fri, Mar 16, 2018 at 05:41:27PM +, Dexuan Cui wrote:
> > From: Lorenzo Pieralisi <lorenzo.pieral...@arm.com>
> > Sent: Friday, March 16, 2018 03:54
> > ...
> > Dexuan,
> > while applying/updating these patches I notice this one may be squashed
> > into: https://patchwork.ozlabs.org/patch/886266/
> > 
> > since they logically belong in the same patch. Are you OK with me doing
> > that ? Is my reading correct ?
> > Lorenzo
> 
> I'm OK. 
> I used two patches
> [PATCH v4 1/2] PCI: hv: Serialize the present and eject work items
> [PATCH v4 3/4] PCI: hv: Remove hbus->enum_sem
> only because the first fixed a real issue and hence IMO should go into
> stable kernels, and the second is only a cleanup patch, which doesn't
> need go into stable kernels.
> 
> Either way is ok to me. 
> Please feel free to do whatever you think is better. :-)

OK, patch series reworked and queued in my pci/hv branch please have
a look and let me know if that looks OK for you, I won't ask Bjorn
to move it into -next till you give me the go-ahead.

Thanks,
Lorenzo
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 3/4] PCI: hv: Remove hbus->enum_sem

[Lorenzo Pieralisi]

On Thu, Mar 15, 2018 at 02:21:51PM +, Dexuan Cui wrote:
> Since we serialize the present/eject work items now, we don't need the
> semaphore any more.
> 
> Signed-off-by: Dexuan Cui 
> Reviewed-by: Michael Kelley 
> Acked-by: Haiyang Zhang 
> Cc: Vitaly Kuznetsov 
> Cc: Jack Morgenstein 
> Cc: Stephen Hemminger 
> Cc: K. Y. Srinivasan 
> ---
>  drivers/pci/host/pci-hyperv.c | 17 ++---
>  1 file changed, 2 insertions(+), 15 deletions(-)

Dexuan,

while applying/updating these patches I notice this one may be squashed
into:

https://patchwork.ozlabs.org/patch/886266/

since they logically belong in the same patch. Are you OK with me doing
that ? Is my reading correct ?

Thanks,
Lorenzo

> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 1d2e1cb617f4..0dc2ecdbbe45 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -447,7 +447,6 @@ struct hv_pcibus_device {
>   spinlock_t device_list_lock;/* Protect lists below */
>   void __iomem *cfg_addr;
>  
> - struct semaphore enum_sem;
>   struct list_head resources_for_children;
>  
>   struct list_head children;
> @@ -1648,12 +1647,8 @@ static struct hv_pci_dev *get_pcichild_wslot(struct 
> hv_pcibus_device *hbus,
>   * It must also treat the omission of a previously observed device as
>   * notification that the device no longer exists.
>   *
> - * Note that this function is a work item, and it may not be
> - * invoked in the order that it was queued.  Back to back
> - * updates of the list of present devices may involve queuing
> - * multiple work items, and this one may run before ones that
> - * were sent later. As such, this function only does something
> - * if is the last one in the queue.
> + * Note that this function is serialized with hv_eject_device_work(),
> + * because both are pushed to the ordered workqueue hbus->wq.
>   */
>  static void pci_devices_present_work(struct work_struct *work)
>  {
> @@ -1674,11 +1669,6 @@ static void pci_devices_present_work(struct 
> work_struct *work)
>  
>   INIT_LIST_HEAD();
>  
> - if (down_interruptible(>enum_sem)) {
> - put_hvpcibus(hbus);
> - return;
> - }
> -
>   /* Pull this off the queue and process it if it was the last one. */
>   spin_lock_irqsave(>device_list_lock, flags);
>   while (!list_empty(>dr_list)) {
> @@ -1695,7 +1685,6 @@ static void pci_devices_present_work(struct work_struct 
> *work)
>   spin_unlock_irqrestore(>device_list_lock, flags);
>  
>   if (!dr) {
> - up(>enum_sem);
>   put_hvpcibus(hbus);
>   return;
>   }
> @@ -1782,7 +1771,6 @@ static void pci_devices_present_work(struct work_struct 
> *work)
>   break;
>   }
>  
> - up(>enum_sem);
>   put_hvpcibus(hbus);
>   kfree(dr);
>  }
> @@ -2516,7 +2504,6 @@ static int hv_pci_probe(struct hv_device *hdev,
>   spin_lock_init(>config_lock);
>   spin_lock_init(>device_list_lock);
>   spin_lock_init(>retarget_msi_interrupt_lock);
> - sema_init(>enum_sem, 1);
>   init_completion(>remove_event);
>   hbus->wq = alloc_ordered_workqueue("hv_pci_%x", 0,
>  hbus->sysdata.domain);
> -- 
> 2.7.4
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 1/2] PCI: hv: Serialize the present and eject work items

[Lorenzo Pieralisi]

On Thu, Mar 15, 2018 at 02:20:53PM +, Dexuan Cui wrote:
> When we hot-remove the device, we first receive a PCI_EJECT message and
> then receive a PCI_BUS_RELATIONS message with bus_rel->device_count == 0.
> 
> The first message is offloaded to hv_eject_device_work(), and the second
> is offloaded to pci_devices_present_work(). Both the paths can be running
> list_del(>list_entry), causing general protection fault, because
> system_wq can run them concurrently.
> 
> The patch eliminates the race condition.
> 
> Tested-by: Adrian Suhov 
> Tested-by: Chris Valean 
> Signed-off-by: Dexuan Cui 
> Reviewed-by: Michael Kelley 
> Acked-by: Haiyang Zhang 
> Cc: sta...@vger.kernel.org

I need to know either what commit you are fixing (ie Fixes: tag - which
is preferrable) or you tell me which kernel versions we are targeting
for the stable backport.

Thanks,
Lorenzo

> Cc: Vitaly Kuznetsov 
> Cc: Jack Morgenstein 
> Cc: Stephen Hemminger 
> Cc: K. Y. Srinivasan 
> ---
>  drivers/pci/host/pci-hyperv.c | 17 ++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 2faf38eab785..5e67dff779a7 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -461,6 +461,8 @@ struct hv_pcibus_device {
>   struct retarget_msi_interrupt retarget_msi_interrupt_params;
>  
>   spinlock_t retarget_msi_interrupt_lock;
> +
> + struct workqueue_struct *wq;
>  };
>  
>  /*
> @@ -1770,7 +1772,7 @@ static void hv_pci_devices_present(struct 
> hv_pcibus_device *hbus,
>   spin_unlock_irqrestore(>device_list_lock, flags);
>  
>   get_hvpcibus(hbus);
> - schedule_work(_wrk->wrk);
> + queue_work(hbus->wq, _wrk->wrk);
>  }
>  
>  /**
> @@ -1848,7 +1850,7 @@ static void hv_pci_eject_device(struct hv_pci_dev 
> *hpdev)
>   get_pcichild(hpdev, hv_pcidev_ref_pnp);
>   INIT_WORK(>wrk, hv_eject_device_work);
>   get_hvpcibus(hpdev->hbus);
> - schedule_work(>wrk);
> + queue_work(hpdev->hbus->wq, >wrk);
>  }
>  
>  /**
> @@ -2463,11 +2465,17 @@ static int hv_pci_probe(struct hv_device *hdev,
>   spin_lock_init(>retarget_msi_interrupt_lock);
>   sema_init(>enum_sem, 1);
>   init_completion(>remove_event);
> + hbus->wq = alloc_ordered_workqueue("hv_pci_%x", 0,
> +hbus->sysdata.domain);
> + if (!hbus->wq) {
> + ret = -ENOMEM;
> + goto free_bus;
> + }
>  
>   ret = vmbus_open(hdev->channel, pci_ring_size, pci_ring_size, NULL, 0,
>hv_pci_onchannelcallback, hbus);
>   if (ret)
> - goto free_bus;
> + goto destroy_wq;
>  
>   hv_set_drvdata(hdev, hbus);
>  
> @@ -2536,6 +2544,8 @@ static int hv_pci_probe(struct hv_device *hdev,
>   hv_free_config_window(hbus);
>  close:
>   vmbus_close(hdev->channel);
> +destroy_wq:
> + destroy_workqueue(hbus->wq);
>  free_bus:
>   free_page((unsigned long)hbus);
>   return ret;
> @@ -2615,6 +2625,7 @@ static int hv_pci_remove(struct hv_device *hdev)
>   irq_domain_free_fwnode(hbus->sysdata.fwnode);
>   put_hvpcibus(hbus);
>   wait_for_completion(>remove_event);
> + destroy_workqueue(hbus->wq);
>   free_page((unsigned long)hbus);
>   return 0;
>  }
> -- 
> 2.7.4
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3]PCI: hv: fix PCI-BUS domainID corruption

[Lorenzo Pieralisi]

On Thu, Mar 15, 2018 at 12:03:07AM +, Sridhar Pitchai wrote:
> Whenever PCI bus is added, HyperV guarantees the BUS id is unique. Even

"Whenever a PCI bus is added"

> with that when a first device is added to the bus, it overrides bus domain
> ID with the device serial number. Sometime this can result in BUS ID not

Define "Sometime".

> being unique. In this case, when PCI_BUS and a device to bus is added, the
> first device overwrites the bus domain ID to the device serial number,
> which is 0. Since there exsist a PCI bus with domain ID 0 already the PCI

s/exsist/exist

> bus addition fails. This patch make sure when a device is added to a bus,
> it never updated the bus domain ID. 

s/updated/updates

> Since we have the transparent SRIOV mode now, the short VF device name
> is no longer needed.

I still do not understand what this means and how it is related to the
patch below, it may be clear to you, it is not to me, at all.

> Fixes: 4a9b0933bdfc("PCI:hv:Use device serial number as PCI domain")

Fixes: 4a9b0933bdfc ("PCI: hv: Use device serial number as PCI domain")

I asked you an explicit question. Commit above was added for a reason
I assume. This patch implies that kernel has been broken since v4.11
which is almost a year ago and nobody every noticed ? Or there are
systems where commit above is _necessary_ and this patch would break
them ?

I want a detailed explanation that highlights *why* it is safe to apply
this patch and send it to stable kernels, commit log above won't do.

Thanks,
Lorenzo

> Cc: sta...@vger.kernel.org
> Signed-off-by: Sridhar Pitchai 
> ---
> 
> Changes in v3:
> * fix the commit comment. [KY Srinivasan, Michael Kelley]
> ---
>  drivers/pci/host/pci-hyperv.c | 11 ---
>  1 file changed, 11 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 2faf38e..ac67e56 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -1518,17 +1518,6 @@ static struct hv_pci_dev *new_pcichild_device(struct 
> hv_pcibus_device *hbus,
>   get_pcichild(hpdev, hv_pcidev_ref_childlist);
>   spin_lock_irqsave(>device_list_lock, flags);
>  
> - /*
> -  * When a device is being added to the bus, we set the PCI domain
> -  * number to be the device serial number, which is non-zero and
> -  * unique on the same VM.  The serial numbers start with 1, and
> -  * increase by 1 for each device.  So device names including this
> -  * can have shorter names than based on the bus instance UUID.
> -  * Only the first device serial number is used for domain, so the
> -  * domain number will not change after the first device is added.
> -  */
> - if (list_empty(>children))
> - hbus->sysdata.domain = desc->ser;
>   list_add_tail(>list_entry, >children);
>   spin_unlock_irqrestore(>device_list_lock, flags);
>   return hpdev;
> -- 
> 2.7.4 
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 6/6] PCI: hv: fix 2 hang issues in hv_compose_msi_msg()

[Lorenzo Pieralisi]

On Tue, Mar 13, 2018 at 06:23:39PM +, Dexuan Cui wrote:

[...]

> Hi Lorenzo, Bjorn, and all,
> Do you need more ACKs? Currently Michael and Haiyang reviewed and ack'd 
> the patchset.
> 
> Should I send a v4 that just removes the "CC: sta...@vger.kernel.org" tag
> for patches 1, 2, 4 and 5? I tend to avoid a v4 as I supppose it would be 
> easier if you just remove the tags if you belive it's necessary (IMHO all the
> 6 paches are not big and it would be great if we can have all of them in 
> the old stable kernels, but I respect your decision).

I think you need a v4 since for patches that are actually fixing a bug I
want a Fixes: tag added and I want them to be applicable independently
of other patches in the series. Send them in a separate series if you
prefer - I just want to make sure they apply independently.

As for marking patches for stable kernels, I do not think it is right
to send cosmetic churn to stable kernels anyway, at least that's my
reading of the policy.

You can easily post a v4 with patches reshuffled - I will apply them
accordingly.

Before re-posting please read this to improve formatting (I can do it
for you but while at it you can do it yourself):

https://marc.info/?l=linux-pci=150905742808166=2

Thanks,
Lorenzo
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 6/6] PCI: hv: fix 2 hang issues in hv_compose_msi_msg()

[Lorenzo Pieralisi]

On Tue, Mar 13, 2018 at 06:23:39PM +, Dexuan Cui wrote:
> > From: Dexuan Cui
> > Sent: Wednesday, March 7, 2018 13:40
> > To: Lorenzo Pieralisi <lorenzo.pieral...@arm.com>
> > Cc: bhelg...@google.com; linux-...@vger.kernel.org; KY Srinivasan
> > <k...@microsoft.com>; Stephen Hemminger <sthem...@microsoft.com>;
> > o...@aepfle.de; a...@canonical.com; jasow...@redhat.com; linux-
> > ker...@vger.kernel.org; driverdev-devel@linuxdriverproject.org; Haiyang
> > Zhang <haiya...@microsoft.com>; vkuzn...@redhat.com;
> > marcelo.ce...@canonical.com; Michael Kelley (EOSG)
> > <michael.h.kel...@microsoft.com>; sta...@vger.kernel.org; Jack
> > Morgenstein <ja...@mellanox.com>
> > Subject: RE: [PATCH v3 6/6] PCI: hv: fix 2 hang issues in 
> > hv_compose_msi_msg()
> > 
> > > From: Lorenzo Pieralisi <lorenzo.pieral...@arm.com>
> > > Sent: Wednesday, March 7, 2018 04:35
> > > On Tue, Mar 06, 2018 at 06:21:56PM +, Dexuan Cui wrote:
> > > > 1. With the patch "x86/vector/msi: Switch to global reservation mode"
> > > > (4900be8360), the recent v4.15 and newer kernels always hang for 1-vCPU
> > > > Hyper-V VM with SR-IOV. This is because when we reach
> > > hv_compose_msi_msg()
> > > > by request_irq()  -> request_threaded_irq() -> 
> > > > __setup_irq()->irq_startup()
> > > >  -> __irq_startup() -> irq_domain_activate_irq() -> ... ->
> > > > msi_domain_activate() -> ... -> hv_compose_msi_msg(), local irq is
> > > > disabled in __setup_irq().
> > > >
> > > > Fix this by polling the channel.
> > > >
> > > > 2. If the host is ejecting the VF device before we reach
> > > > hv_compose_msi_msg(), in a UP VM, we can hang in
> > hv_compose_msi_msg()
> > > > forever, because at this time the host doesn't respond to the
> > > > CREATE_INTERRUPT request. This issue also happens to old kernels like
> > > > v4.14, v4.13, etc.
> > >
> > > If you are fixing a problem you should report what commit you are fixing
> > > with a Fixes: tag and add a CC: sta...@vger.kernel.org to the commit log
> > > to send it to stable kernels to which it should be applied; mentioning
> > > kernel versions in the commit log is useless and should be omitted.
> > 
> > Hi Lorenzo,
> > Thanks for your comments!
> > This patch does have a "Cc: sta...@vger.kernel.org" in the sign-off area. 
> > :-)
> > 
> > Here the patch is made to resolve 2 issues:
> > #1 is triggered by the x86 global reservation mode (4900be8360) patch.
> > 4900be8360 in itself is good. It's just that drivers/pci/host/pci-hyperv.c
> > should be fixed.
> > 
> > #2 is a longstanding issue since the first day the pci-hyperv driver was
> > accepted into the kernel.
> > 
> > So IMO actually we don't really need to add a Fixes: tag, which is usually
> > used to specify a specific commit that introduces a bug that is being fixed.
> > 
> > > Side note: you should not have sta...@vger.kernel.org in the email
> > > addresses CC list you are sending the patches to (you mark patches for
> > > stable by adding an appropriate CC tag in the commit log).
> > 
> > Sorry, I didn't know this, but actually I didn't add sta...@vger.kernel.org
> > manually. Instead I used "git send-email" to send this patchset, and it told
> > me "The Cc list above has been expanded by additional addresses found
> > in the patch commit message."
> > 
> > I didn't find a way to disable this behavior of "git send-email" by checking
> > its manual and googling it. This is strange.
> > 
> > > Here:
> > >
> > > git.kernel.org/.../Documentation/process/stable-kernel-rules.rst
> > >
> > > Last but not least, most of the patches in this series do not justify
> > > sending them to stable kernels at all so you should remove the
> > > corresponding tag from the patches.
> > 
> > I hope at least these 2 patches can go into the stable kernels:
> > [PATCH v3 3/6] PCI: hv: serialize the present/eject work items
> > [PATCH v3 6/6] PCI: hv: fix 2 hang issues in hv_compose_msi_msg()
> > Especially the second one, which fixes a real hang issue for UP virtual
> > machines running v4.15 and newer.
> > And,  IMO the patches are small enough (<100 lines) , but definitely
> > the maintainers make the final call.
> > 
> > >
> > > Thanks,
> > > Lorenzo
>

Re: [PATCH] PCI: hv: fix bus domain ID corruption

[Lorenzo Pieralisi]

On Fri, Mar 09, 2018 at 05:02:34AM +, Sridhar Pitchai wrote:
> When PCI BUS is added, PCI_BUS domain ID is set. When PCI_BUS and a device
> added to the bus is racing against each other, the first device tends to
> overwrite the domain ID. In order to avoid the race, this patch make sure
> when a device is added to a bus, it never updated the bus domain ID. Since
> we have the transparent SRIOV mode now, the short VF device name is no
> longer needed.

This commit log may be clear to you, it is not clear to me at all so
you will have to improve it and explain why it is safe to apply this
patch.

> Fixes: 4a9b0933bdfc("PCI:hv:Use device serial number as PCI domain")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Sridhar Pitchai 
> ---
>  drivers/pci/host/pci-hyperv.c | 11 ---
>  1 file changed, 11 deletions(-)

Are you 100% sure this patch won't trigger a regression on current
systems ? I need ACKs/reviews from hyperv maintainers to apply it.

Lorenzo

> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 2faf38e..ac67e56 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -1518,17 +1518,6 @@ static struct hv_pci_dev *new_pcichild_device(struct 
> hv_pcibus_device *hbus,
>   get_pcichild(hpdev, hv_pcidev_ref_childlist);
>   spin_lock_irqsave(>device_list_lock, flags);
>  
> - /*
> -  * When a device is being added to the bus, we set the PCI domain
> -  * number to be the device serial number, which is non-zero and
> -  * unique on the same VM.  The serial numbers start with 1, and
> -  * increase by 1 for each device.  So device names including this
> -  * can have shorter names than based on the bus instance UUID.
> -  * Only the first device serial number is used for domain, so the
> -  * domain number will not change after the first device is added.
> -  */
> - if (list_empty(>children))
> - hbus->sysdata.domain = desc->ser;
>   list_add_tail(>list_entry, >children);
>   spin_unlock_irqrestore(>device_list_lock, flags);
>   return hpdev;
> -- 
> 2.7.4
>  
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 6/6] PCI: hv: fix 2 hang issues in hv_compose_msi_msg()

[Lorenzo Pieralisi]

On Tue, Mar 06, 2018 at 06:21:56PM +, Dexuan Cui wrote:
> 1. With the patch "x86/vector/msi: Switch to global reservation mode"
> (4900be8360), the recent v4.15 and newer kernels always hang for 1-vCPU
> Hyper-V VM with SR-IOV. This is because when we reach hv_compose_msi_msg()
> by request_irq()  -> request_threaded_irq() -> __setup_irq()->irq_startup()
>  -> __irq_startup() -> irq_domain_activate_irq() -> ... ->
> msi_domain_activate() -> ... -> hv_compose_msi_msg(), local irq is
> disabled in __setup_irq().
> 
> Fix this by polling the channel.
> 
> 2. If the host is ejecting the VF device before we reach
> hv_compose_msi_msg(), in a UP VM, we can hang in hv_compose_msi_msg()
> forever, because at this time the host doesn't respond to the
> CREATE_INTERRUPT request. This issue also happens to old kernels like
> v4.14, v4.13, etc.

If you are fixing a problem you should report what commit you are fixing
with a Fixes: tag and add a CC: sta...@vger.kernel.org to the commit log
to send it to stable kernels to which it should be applied; mentioning
kernel versions in the commit log is useless and should be omitted.

Side note: you should not have sta...@vger.kernel.org in the email
addresses CC list you are sending the patches to (you mark patches for
stable by adding an appropriate CC tag in the commit log).

Here:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?h=v4.16-rc4

Last but not least, most of the patches in this series do not justify
sending them to stable kernels at all so you should remove the
corresponding tag from the patches.

Thanks,
Lorenzo

> Fix this by polling the channel for the PCI_EJECT message and
> hpdev->state, and by checking the PCI vendor ID.
> 
> Note: actually the above issues also happen to a SMP VM, if
> "hbus->hdev->channel->target_cpu == smp_processor_id()" is true.
> 
> Signed-off-by: Dexuan Cui 
> Tested-by: Adrian Suhov 
> Tested-by: Chris Valean 
> Cc: sta...@vger.kernel.org
> Cc: Stephen Hemminger 
> Cc: K. Y. Srinivasan 
> Cc: Vitaly Kuznetsov 
> Cc: Jack Morgenstein 
> ---
>  drivers/pci/host/pci-hyperv.c | 58 
> ++-
>  1 file changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 265ba11e53e2..50cdefe3f6d3 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -521,6 +521,8 @@ struct hv_pci_compl {
>   s32 completion_status;
>  };
>  
> +static void hv_pci_onchannelcallback(void *context);
> +
>  /**
>   * hv_pci_generic_compl() - Invoked for a completion packet
>   * @context: Set up by the sender of the packet.
> @@ -665,6 +667,31 @@ static void _hv_pcifront_read_config(struct hv_pci_dev 
> *hpdev, int where,
>   }
>  }
>  
> +static u16 hv_pcifront_get_vendor_id(struct hv_pci_dev *hpdev)
> +{
> + u16 ret;
> + unsigned long flags;
> + void __iomem *addr = hpdev->hbus->cfg_addr + CFG_PAGE_OFFSET +
> +  PCI_VENDOR_ID;
> +
> + spin_lock_irqsave(>hbus->config_lock, flags);
> +
> + /* Choose the function to be read. (See comment above) */
> + writel(hpdev->desc.win_slot.slot, hpdev->hbus->cfg_addr);
> + /* Make sure the function was chosen before we start reading. */
> + mb();
> + /* Read from that function's config space. */
> + ret = readw(addr);
> + /*
> +  * mb() is not required here, because the spin_unlock_irqrestore()
> +  * is a barrier.
> +  */
> +
> + spin_unlock_irqrestore(>hbus->config_lock, flags);
> +
> + return ret;
> +}
> +
>  /**
>   * _hv_pcifront_write_config() - Internal PCI config write
>   * @hpdev:   The PCI driver's representation of the device
> @@ -1107,8 +1134,37 @@ static void hv_compose_msi_msg(struct irq_data *data, 
> struct msi_msg *msg)
>* Since this function is called with IRQ locks held, can't
>* do normal wait for completion; instead poll.
>*/
> - while (!try_wait_for_completion(_pkt.host_event))
> + while (!try_wait_for_completion(_pkt.host_event)) {
> + /* 0x means an invalid PCI VENDOR ID. */
> + if (hv_pcifront_get_vendor_id(hpdev) == 0x) {
> + dev_err_once(>hdev->device,
> +  "the device has gone\n");
> + goto free_int_desc;
> + }
> +
> + /*
> +  * When the higher level interrupt code calls us with
> +  * interrupt disabled, we must poll the channel by calling
> +  * the channel callback directly when channel->target_cpu is
> +  * the current CPU. When the higher level interrupt code
> +  * calls us with interrupt enabled, let's add the
> +