[PATCH] pci: shpchp: set the bridge busmaster if MSI are enabled

[PATCH] pci: shpchp: set the bridge busmaster if MSI are enabled

[Aleksandr Bezzubikov]

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(+)

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

Re: [PATCH] pci: shpchp: set the bridge busmaster if MSI are enabled

[Marcel Apfelbaum]

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

Re: [PATCH] pci: shpchp: set the bridge busmaster if MSI are enabled

[Marcel Apfelbaum]

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

Re: [PATCH] pci: shpchp: set the bridge busmaster if MSI are enabled

[Alexander Bezzubikov]

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]


-- 
Alexander Bezzubikov

Re: [PATCH] pci: shpchp: set the bridge busmaster if MSI are enabled

[Alex Williamson]

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

Re: [PATCH] pci: shpchp: set the bridge busmaster if MSI are enabled

[Alexander Bezzubikov]

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

-- 
Alexander Bezzubikov

Re: pci: shpchp: set the bridge busmaster if MSI are enabled

[Michael S. Tsirkin]

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,

Re: [PATCH] pci: shpchp: set the bridge busmaster if MSI are enabled

[Bjorn Helgaas]

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
> 

Re: [PATCH] pci: shpchp: set the bridge busmaster if MSI are enabled

[Alexander Bezzubikov]

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
>>



-- 
Aleksandr Bezzubikov

Re: [PATCH] pci: shpchp: set the bridge busmaster if MSI are enabled

[Bjorn Helgaas]

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