| Message ID | 1500387145-4216-1-git-send-email-zuban32s@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On 18/07/2017 17:12, Aleksandr Bezzubikov wrote: > An MSI-based SHPC built in PCI bridges can configure hotplugged devices > only if they notify the bridge with MSI. > But they can't trigger interrupt without the bridge being busmaster, > that's why it should be enabled. Hi Aleksandr, Hot-plugging an empty bridge does require making it bus-master, otherwise we end up with an unusable pci brigde. > > Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com> > --- > drivers/pci/hotplug/shpchp_hpc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/pci/hotplug/shpchp_hpc.c b/drivers/pci/hotplug/shpchp_hpc.c > index de0ea47..e5824c7 100644 > --- a/drivers/pci/hotplug/shpchp_hpc.c > +++ b/drivers/pci/hotplug/shpchp_hpc.c > @@ -1062,6 +1062,8 @@ int shpc_init(struct controller *ctrl, struct pci_dev *pdev) > if (rc) { > ctrl_info(ctrl, "Can't get msi for the hotplug controller\n"); > ctrl_info(ctrl, "Use INTx for the hotplug controller\n"); > + } else { > + pci_set_master(pdev); > } > > rc = request_irq(ctrl->pci_dev->irq, shpc_isr, IRQF_SHARED, > I am not really familiar with the shpc code, but the change looks OK to me. Reviewed-by: Marcel Apfelbaum <marcel@redhat.com> Thanks, Marcel
Just a friendly reminder - I wanted to check if you want something else to get this patch merged. [Somehow the first reminder attempt was rejected as a virus by linux-pci mail server, so my apologize for possible duplication]
On Sun, 23 Jul 2017 23:18:01 +0300 Alexander Bezzubikov <zuban32s@gmail.com> wrote: > Just a friendly reminder - I wanted to check if you want something > else to get this patch merged. > [Somehow the first reminder attempt was rejected as a virus by > linux-pci mail server, > so my apologize for possible duplication] Please note: http://www.spinics.net/lists/linux-pci/msg62941.html As potentially a long standing issue, I would expect this to be reviewed and evaluated for the v4.14 cycle. Thanks, Alex
2017-07-24 17:56 GMT+03:00 Alex Williamson <alex.williamson@redhat.com>: > > On Sun, 23 Jul 2017 23:18:01 +0300 > Alexander Bezzubikov <zuban32s@gmail.com> wrote: > > > Just a friendly reminder - I wanted to check if you want something > > else to get this patch merged. > > [Somehow the first reminder attempt was rejected as a virus by > > linux-pci mail server, > > so my apologize for possible duplication] > > > Please note: > > http://www.spinics.net/lists/linux-pci/msg62941.html > Missed it, sorry. > > As potentially a long standing issue, I would expect this to be > reviewed and evaluated for the v4.14 cycle. Thanks, > > Alex Ok, nothing urgent. Thanks
On 19/07/2017 16:36, Marcel Apfelbaum wrote: > On 18/07/2017 17:12, Aleksandr Bezzubikov wrote: >> An MSI-based SHPC built in PCI bridges can configure hotplugged devices >> only if they notify the bridge with MSI. >> But they can't trigger interrupt without the bridge being busmaster, >> that's why it should be enabled. > > Hi Aleksandr, > > Hot-plugging an empty bridge does require making it bus-master, > otherwise we end up with an unusable pci brigde. > >> >> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com> >> --- >> drivers/pci/hotplug/shpchp_hpc.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/pci/hotplug/shpchp_hpc.c >> b/drivers/pci/hotplug/shpchp_hpc.c >> index de0ea47..e5824c7 100644 >> --- a/drivers/pci/hotplug/shpchp_hpc.c >> +++ b/drivers/pci/hotplug/shpchp_hpc.c >> @@ -1062,6 +1062,8 @@ int shpc_init(struct controller *ctrl, struct >> pci_dev *pdev) >> if (rc) { >> ctrl_info(ctrl, "Can't get msi for the hotplug >> controller\n"); >> ctrl_info(ctrl, "Use INTx for the hotplug controller\n"); >> + } else { >> + pci_set_master(pdev); >> } >> rc = request_irq(ctrl->pci_dev->irq, shpc_isr, IRQF_SHARED, >> > > > I am not really familiar with the shpc code, > but the change looks OK to me. > > Reviewed-by: Marcel Apfelbaum <marcel@redhat.com> > Hi, Since is a fix: Cc: stable@vger.kernel.org Thanks, Marcel > Thanks, > Marcel
On Tue, Jul 18, 2017 at 05:12:25PM +0300, Aleksandr Bezzubikov wrote: > An MSI-based SHPC built in PCI bridges can configure hotplugged devices > only if they notify the bridge with MSI. > But they can't trigger interrupt without the bridge being busmaster, > that's why it should be enabled. > > Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com> > Reviewed-by: Marcel Apfelbaum <marcel@redhat.com> Given there's pci_enable_msi above, not enabling bus master seems like a very strange thing to do. Acked-by: Michael S. Tsirkin <mst@redhat.com> This also looks like a good candidate for stable. > --- > drivers/pci/hotplug/shpchp_hpc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/pci/hotplug/shpchp_hpc.c b/drivers/pci/hotplug/shpchp_hpc.c > index de0ea47..e5824c7 100644 > --- a/drivers/pci/hotplug/shpchp_hpc.c > +++ b/drivers/pci/hotplug/shpchp_hpc.c > @@ -1062,6 +1062,8 @@ int shpc_init(struct controller *ctrl, struct pci_dev *pdev) > if (rc) { > ctrl_info(ctrl, "Can't get msi for the hotplug controller\n"); > ctrl_info(ctrl, "Use INTx for the hotplug controller\n"); > + } else { > + pci_set_master(pdev); > } > > rc = request_irq(ctrl->pci_dev->irq, shpc_isr, IRQF_SHARED,
On Tue, Jul 18, 2017 at 05:12:25PM +0300, Aleksandr Bezzubikov wrote: > An MSI-based SHPC built in PCI bridges can configure hotplugged devices > only if they notify the bridge with MSI. I think you're referring to the events listed in SHPC r1.0, sec 4.7.3, table 4-24, right? Attention Button Press, Isolated Power Fault, Card Presence Change, MRS Sensor Change, etc? So IIUC, this is really about the bridge itself generating MSIs about slot-related events, not the hot-added devices generating MSIs. I agree this patch makes sense, I'm just trying to clarify the changelog. > But they can't trigger interrupt without the bridge being busmaster, > that's why it should be enabled. > > Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com> > --- > drivers/pci/hotplug/shpchp_hpc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/pci/hotplug/shpchp_hpc.c b/drivers/pci/hotplug/shpchp_hpc.c > index de0ea47..e5824c7 100644 > --- a/drivers/pci/hotplug/shpchp_hpc.c > +++ b/drivers/pci/hotplug/shpchp_hpc.c > @@ -1062,6 +1062,8 @@ int shpc_init(struct controller *ctrl, struct pci_dev *pdev) > if (rc) { > ctrl_info(ctrl, "Can't get msi for the hotplug controller\n"); > ctrl_info(ctrl, "Use INTx for the hotplug controller\n"); > + } else { > + pci_set_master(pdev); > } > > rc = request_irq(ctrl->pci_dev->irq, shpc_isr, IRQF_SHARED, > -- > 2.7.4 >
2017-08-03 1:35 GMT+03:00 Bjorn Helgaas <helgaas@kernel.org>: > On Tue, Jul 18, 2017 at 05:12:25PM +0300, Aleksandr Bezzubikov wrote: >> An MSI-based SHPC built in PCI bridges can configure hotplugged devices >> only if they notify the bridge with MSI. > > I think you're referring to the events listed in SHPC r1.0, sec 4.7.3, > table 4-24, right? Attention Button Press, Isolated Power Fault, Card > Presence Change, MRS Sensor Change, etc? > > So IIUC, this is really about the bridge itself generating MSIs about > slot-related events, not the hot-added devices generating MSIs. > You're right, it's definitely about the bridge's built-in SHPC that generates MSIs. > I agree this patch makes sense, I'm just trying to clarify the > changelog. > >> But they can't trigger interrupt without the bridge being busmaster, >> that's why it should be enabled. >> >> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com> >> --- >> drivers/pci/hotplug/shpchp_hpc.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/pci/hotplug/shpchp_hpc.c b/drivers/pci/hotplug/shpchp_hpc.c >> index de0ea47..e5824c7 100644 >> --- a/drivers/pci/hotplug/shpchp_hpc.c >> +++ b/drivers/pci/hotplug/shpchp_hpc.c >> @@ -1062,6 +1062,8 @@ int shpc_init(struct controller *ctrl, struct pci_dev *pdev) >> if (rc) { >> ctrl_info(ctrl, "Can't get msi for the hotplug controller\n"); >> ctrl_info(ctrl, "Use INTx for the hotplug controller\n"); >> + } else { >> + pci_set_master(pdev); >> } >> >> rc = request_irq(ctrl->pci_dev->irq, shpc_isr, IRQF_SHARED, >> -- >> 2.7.4 >>
On Thu, Aug 03, 2017 at 01:48:51AM +0300, Alexander Bezzubikov wrote: > 2017-08-03 1:35 GMT+03:00 Bjorn Helgaas <helgaas@kernel.org>: > > On Tue, Jul 18, 2017 at 05:12:25PM +0300, Aleksandr Bezzubikov wrote: > >> An MSI-based SHPC built in PCI bridges can configure hotplugged devices > >> only if they notify the bridge with MSI. > > > > I think you're referring to the events listed in SHPC r1.0, sec 4.7.3, > > table 4-24, right? Attention Button Press, Isolated Power Fault, Card > > Presence Change, MRS Sensor Change, etc? > > > > So IIUC, this is really about the bridge itself generating MSIs about > > slot-related events, not the hot-added devices generating MSIs. > > > > You're right, it's definitely about the bridge's built-in SHPC > that generates MSIs. Great, thanks for confirming that. I applied this to pci/hotplug for v4.14 with the following changelog: PCI: shpchp: Enable bridge bus mastering if MSI is enabled An SHPC may generate MSIs to notify software about slot or controller events (SHPC spec r1.0, sec 4.7). A PCI device can only generate an MSI if it has bus mastering enabled. Enable bus mastering if the bridge contains an SHPC that uses MSI for event notifications. Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com> [bhelgaas: changelog] Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com> Acked-by: Michael S. Tsirkin <mst@redhat.com> Cc: stable@vger.kernel.org
diff --git a/drivers/pci/hotplug/shpchp_hpc.c b/drivers/pci/hotplug/shpchp_hpc.c index de0ea47..e5824c7 100644 --- a/drivers/pci/hotplug/shpchp_hpc.c +++ b/drivers/pci/hotplug/shpchp_hpc.c @@ -1062,6 +1062,8 @@ int shpc_init(struct controller *ctrl, struct pci_dev *pdev) if (rc) { ctrl_info(ctrl, "Can't get msi for the hotplug controller\n"); ctrl_info(ctrl, "Use INTx for the hotplug controller\n"); + } else { + pci_set_master(pdev); } rc = request_irq(ctrl->pci_dev->irq, shpc_isr, IRQF_SHARED,
An MSI-based SHPC built in PCI bridges can configure hotplugged devices only if they notify the bridge with MSI. But they can't trigger interrupt without the bridge being busmaster, that's why it should be enabled. Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com> --- drivers/pci/hotplug/shpchp_hpc.c | 2 ++ 1 file changed, 2 insertions(+)