| Message ID | KL1P15301MB0006EE546A7C2567B61EA783BF6B0@KL1P15301MB0006.APCP153.PROD.OUTLOOK.COM |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: hv: Fix a __local_bh_enable_ip warning in hv_compose_msi_msg() | expand |
> -----Original Message----- > From: Dexuan Cui > Sent: Tuesday, May 22, 2018 8:18 PM > To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Bjorn Helgaas > <bhelgaas@google.com>; linux-pci@vger.kernel.org; KY Srinivasan > <kys@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>; > olaf@aepfle.de; apw@canonical.com; jasowang@redhat.com > Cc: linux-kernel@vger.kernel.org; driverdev-devel@linuxdriverproject.org; > Haiyang Zhang <haiyangz@microsoft.com>; vkuznets@redhat.com; > marcelo.cerri@canonical.com > Subject: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in > hv_compose_msi_msg() > > > 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. > > With CONFIG_PROVE_LOCKING=y in the latest mainline, or old kernels that > don't have commit f71b74bca637 ("irq/softirqs: Use lockdep to assert IRQs are > disabled/enabled"), it turns out can we trigger a warning at the beginning of > __local_bh_enable_ip(), because the upper layer irq code can call > hv_compose_msi_msg() with local irqs disabled. > > Let's fix the warning by switching to local_irq_save()/restore(). This is not an > issue because hv_pci_onchannelcallback() is not slow, and it not a hot path. > > Fixes: de0aa7b2f97d ("PCI: hv: Fix 2 hang issues in hv_compose_msi_msg()") > Signed-off-by: Dexuan Cui <decui@microsoft.com> > Cc: <stable@vger.kernel.org> > Cc: Stephen Hemminger <sthemmin@microsoft.com> > Cc: K. Y. Srinivasan <kys@microsoft.com> > --- Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com> Thanks you.
> From: Haiyang Zhang > Sent: Friday, May 25, 2018 12:52 > To: Dexuan Cui <decui@microsoft.com>; Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com>; Bjorn Helgaas <bhelgaas@google.com>; > linux-pci@vger.kernel.org; KY Srinivasan <kys@microsoft.com>; Stephen > Hemminger <sthemmin@microsoft.com>; olaf@aepfle.de; > apw@canonical.com; jasowang@redhat.com > Cc: linux-kernel@vger.kernel.org; driverdev-devel@linuxdriverproject.org; > vkuznets@redhat.com; marcelo.cerri@canonical.com > Subject: RE: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in > hv_compose_msi_msg() > > > From: Dexuan Cui > > Sent: Tuesday, May 22, 2018 8:18 PM > > To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Bjorn Helgaas > > <bhelgaas@google.com>; linux-pci@vger.kernel.org; KY Srinivasan > > <kys@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>; > > olaf@aepfle.de; apw@canonical.com; jasowang@redhat.com > > Cc: linux-kernel@vger.kernel.org; driverdev-devel@linuxdriverproject.org; > > Haiyang Zhang <haiyangz@microsoft.com>; vkuznets@redhat.com; > > marcelo.cerri@canonical.com > > Subject: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in > > hv_compose_msi_msg() > > > > > > 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. > > > > With CONFIG_PROVE_LOCKING=y in the latest mainline, or old kernels that > > don't have commit f71b74bca637 ("irq/softirqs: Use lockdep to assert IRQs > are > > disabled/enabled"), it turns out can we trigger a warning at the beginning of > > __local_bh_enable_ip(), because the upper layer irq code can call > > hv_compose_msi_msg() with local irqs disabled. > > > > Let's fix the warning by switching to local_irq_save()/restore(). This is not an > > issue because hv_pci_onchannelcallback() is not slow, and it not a hot path. > > > > Fixes: de0aa7b2f97d ("PCI: hv: Fix 2 hang issues in hv_compose_msi_msg()") > > Signed-off-by: Dexuan Cui <decui@microsoft.com> > > Cc: <stable@vger.kernel.org> > > Cc: Stephen Hemminger <sthemmin@microsoft.com> > > Cc: K. Y. Srinivasan <kys@microsoft.com> > > --- > > Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com> > > Thanks you. Hi Lorenzo, Can I have your reply to this patch? -- Dexuan
> From: Dexuan Cui > Sent: Wednesday, June 6, 2018 17:15 > To: Haiyang Zhang <haiyangz@microsoft.com>; Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com>; Bjorn Helgaas <bhelgaas@google.com>; > linux-pci@vger.kernel.org; KY Srinivasan <kys@microsoft.com>; Stephen > Hemminger <sthemmin@microsoft.com>; olaf@aepfle.de; > apw@canonical.com; jasowang@redhat.com > Cc: linux-kernel@vger.kernel.org; driverdev-devel@linuxdriverproject.org; > vkuznets@redhat.com; marcelo.cerri@canonical.com > Subject: RE: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in > hv_compose_msi_msg() > > > From: Haiyang Zhang > > Sent: Friday, May 25, 2018 12:52 > > To: Dexuan Cui <decui@microsoft.com>; Lorenzo Pieralisi > > <lorenzo.pieralisi@arm.com>; Bjorn Helgaas <bhelgaas@google.com>; > > linux-pci@vger.kernel.org; KY Srinivasan <kys@microsoft.com>; Stephen > > Hemminger <sthemmin@microsoft.com>; olaf@aepfle.de; > > apw@canonical.com; jasowang@redhat.com > > Cc: linux-kernel@vger.kernel.org; driverdev-devel@linuxdriverproject.org; > > vkuznets@redhat.com; marcelo.cerri@canonical.com > > Subject: RE: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in > > hv_compose_msi_msg() > > > > > From: Dexuan Cui > > > Sent: Tuesday, May 22, 2018 8:18 PM > > > To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Bjorn Helgaas > > > <bhelgaas@google.com>; linux-pci@vger.kernel.org; KY Srinivasan > > > <kys@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>; > > > olaf@aepfle.de; apw@canonical.com; jasowang@redhat.com > > > Cc: linux-kernel@vger.kernel.org; driverdev-devel@linuxdriverproject.org; > > > Haiyang Zhang <haiyangz@microsoft.com>; vkuznets@redhat.com; > > > marcelo.cerri@canonical.com > > > Subject: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in > > > hv_compose_msi_msg() > > > > > > > > > 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. > > > > > > With CONFIG_PROVE_LOCKING=y in the latest mainline, or old kernels that > > > don't have commit f71b74bca637 ("irq/softirqs: Use lockdep to assert IRQs > > are > > > disabled/enabled"), it turns out can we trigger a warning at the beginning > of > > > __local_bh_enable_ip(), because the upper layer irq code can call > > > hv_compose_msi_msg() with local irqs disabled. > > > > > > Let's fix the warning by switching to local_irq_save()/restore(). This is not an > > > issue because hv_pci_onchannelcallback() is not slow, and it not a hot path. > > > > > > Fixes: de0aa7b2f97d ("PCI: hv: Fix 2 hang issues in > hv_compose_msi_msg()") > > > Signed-off-by: Dexuan Cui <decui@microsoft.com> > > > Cc: <stable@vger.kernel.org> > > > Cc: Stephen Hemminger <sthemmin@microsoft.com> > > > Cc: K. Y. Srinivasan <kys@microsoft.com> > > > --- > > > > Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com> > > > > Thanks you. > > Hi Lorenzo, > > Can I have your reply to this patch? > > -- Dexuan 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? Thanks, -- Dexuan
On Wed, Jun 13, 2018 at 08:32:13PM +0000, Dexuan Cui wrote: > > From: Dexuan Cui > > Sent: Wednesday, June 6, 2018 17:15 > > To: Haiyang Zhang <haiyangz@microsoft.com>; Lorenzo Pieralisi > > <lorenzo.pieralisi@arm.com>; Bjorn Helgaas <bhelgaas@google.com>; > > linux-pci@vger.kernel.org; KY Srinivasan <kys@microsoft.com>; Stephen > > Hemminger <sthemmin@microsoft.com>; olaf@aepfle.de; > > apw@canonical.com; jasowang@redhat.com > > Cc: linux-kernel@vger.kernel.org; driverdev-devel@linuxdriverproject.org; > > vkuznets@redhat.com; marcelo.cerri@canonical.com > > Subject: RE: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in > > hv_compose_msi_msg() > > > > > From: Haiyang Zhang > > > Sent: Friday, May 25, 2018 12:52 > > > To: Dexuan Cui <decui@microsoft.com>; Lorenzo Pieralisi > > > <lorenzo.pieralisi@arm.com>; Bjorn Helgaas <bhelgaas@google.com>; > > > linux-pci@vger.kernel.org; KY Srinivasan <kys@microsoft.com>; Stephen > > > Hemminger <sthemmin@microsoft.com>; olaf@aepfle.de; > > > apw@canonical.com; jasowang@redhat.com > > > Cc: linux-kernel@vger.kernel.org; driverdev-devel@linuxdriverproject.org; > > > vkuznets@redhat.com; marcelo.cerri@canonical.com > > > Subject: RE: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in > > > hv_compose_msi_msg() > > > > > > > From: Dexuan Cui > > > > Sent: Tuesday, May 22, 2018 8:18 PM > > > > To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Bjorn Helgaas > > > > <bhelgaas@google.com>; linux-pci@vger.kernel.org; KY Srinivasan > > > > <kys@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>; > > > > olaf@aepfle.de; apw@canonical.com; jasowang@redhat.com > > > > Cc: linux-kernel@vger.kernel.org; driverdev-devel@linuxdriverproject.org; > > > > Haiyang Zhang <haiyangz@microsoft.com>; vkuznets@redhat.com; > > > > marcelo.cerri@canonical.com > > > > Subject: [PATCH] PCI: hv: Fix a __local_bh_enable_ip warning in > > > > hv_compose_msi_msg() > > > > > > > > > > > > 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. > > > > > > > > With CONFIG_PROVE_LOCKING=y in the latest mainline, or old kernels that > > > > don't have commit f71b74bca637 ("irq/softirqs: Use lockdep to assert IRQs > > > are > > > > disabled/enabled"), it turns out can we trigger a warning at the beginning > > of > > > > __local_bh_enable_ip(), because the upper layer irq code can call > > > > hv_compose_msi_msg() with local irqs disabled. > > > > > > > > Let's fix the warning by switching to local_irq_save()/restore(). This is not an > > > > issue because hv_pci_onchannelcallback() is not slow, and it not a hot path. > > > > > > > > Fixes: de0aa7b2f97d ("PCI: hv: Fix 2 hang issues in > > hv_compose_msi_msg()") > > > > Signed-off-by: Dexuan Cui <decui@microsoft.com> > > > > Cc: <stable@vger.kernel.org> > > > > Cc: Stephen Hemminger <sthemmin@microsoft.com> > > > > Cc: K. Y. Srinivasan <kys@microsoft.com> > > > > --- > > > > > > Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com> > > > > > > Thanks you. > > > > Hi Lorenzo, > > > > Can I have your reply to this patch? > > > > -- Dexuan > > 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. 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"). 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. 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
> From: Bjorn Helgaas <helgaas@kernel.org> > 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(&comp.comp_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. :-) Thanks, -- Dexuan
On Wed, Jun 13, 2018 at 10:50:05PM +0000, Dexuan Cui wrote: > > From: Bjorn Helgaas <helgaas@kernel.org> > > 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(&comp.comp_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
> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Sent: Friday, June 29, 2018 02:39 > To: Dexuan Cui <decui@microsoft.com> > On Wed, Jun 13, 2018 at 10:50:05PM +0000, Dexuan Cui wrote: > > > From: Bjorn Helgaas <helgaas@kernel.org> > > > 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(&comp.comp_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. Now my understanding is: 1) When hv_compose_msi_msg() is called with local irq ENABLED by the upper level irq code, the current code is good and the lockdep warning is not triggered. 2) When hv_compose_msi_msg() is called with local irq DISABLED by the upper level irq code, the current code *is* buggg: local_bh_enable() can potentially call do_softirq(), which is not supposed to run when local irq is DISABLED. I think the lockdep warning is triggered for this reason. In summary, now I realized the warning is not spurious, and here at the first place I should not use local_bh_disable()/enable(), which are not supposed to be used when local irq can be DISABLED. > 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. > > Lorenzo Now I realized there *is* a bug. I'm going to send a v2 with a new changelog, though the changed code will remain the same. Thanks, -- Dexuan
diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c index 50cdefe..ad6a64d 100644 --- a/drivers/pci/host/pci-hyperv.c +++ b/drivers/pci/host/pci-hyperv.c @@ -1051,6 +1051,7 @@ static u32 hv_compose_msi_req_v2( */ static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg) { + unsigned long flags; struct irq_cfg *cfg = irqd_cfg(data); struct hv_pcibus_device *hbus; struct hv_pci_dev *hpdev; @@ -1148,14 +1149,14 @@ 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. */ - 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(&hbus->hdev->device,
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. With CONFIG_PROVE_LOCKING=y in the latest mainline, or old kernels that don't have commit f71b74bca637 ("irq/softirqs: Use lockdep to assert IRQs are disabled/enabled"), it turns out can we trigger a warning at the beginning of __local_bh_enable_ip(), because the upper layer irq code can call hv_compose_msi_msg() with local irqs disabled. Let's fix the warning by switching to local_irq_save()/restore(). This is not an issue because hv_pci_onchannelcallback() is not slow, and it not a hot path. Fixes: de0aa7b2f97d ("PCI: hv: Fix 2 hang issues in hv_compose_msi_msg()") Signed-off-by: Dexuan Cui <decui@microsoft.com> Cc: <stable@vger.kernel.org> Cc: Stephen Hemminger <sthemmin@microsoft.com> Cc: K. Y. Srinivasan <kys@microsoft.com> --- 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 drivers/pci/host/pci-hyperv.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)