PCI: endpoint: Don't stop EP controller by EP function

[PATCH] PCI: endpoint: Don't stop EP controller by EP function

Posted by Shunsuke Mie 3 years, 10 months ago

For multi-function endpoint device, an ep function shouldn't stop EP
controller. Nomally the controller is stopped via configfs.

Fixes: 349e7a85b25f ("PCI: endpoint: functions: Add an EP function to test PCI")
Signed-off-by: Shunsuke Mie <mie@igel.co.jp>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 5b833f00e980..a5ed779b0a51 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -627,7 +627,6 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
 
 	cancel_delayed_work(&epf_test->cmd_handler);
 	pci_epf_test_clean_dma_chan(epf_test);
-	pci_epc_stop(epc);
 	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
 		epf_bar = &epf->bar[bar];
 
-- 
2.17.1

Re: [PATCH] PCI: endpoint: Don't stop EP controller by EP function

Posted by Bjorn Helgaas 3 years, 9 months ago

On Wed, Jun 22, 2022 at 01:09:24PM +0900, Shunsuke Mie wrote:
> For multi-function endpoint device, an ep function shouldn't stop EP
> controller. Nomally the controller is stopped via configfs.

Can you please clarify this for me?

An endpoint function by itself wouldn't stop an endpoint controller.

I assume that some *operation* on an endpoint function currently stops
the endpoint controller, but that operation should not stop the
controller?

I guess the operation is an "unbind" that detaches an EPF device from
an EPC device?

> Fixes: 349e7a85b25f ("PCI: endpoint: functions: Add an EP function to test PCI")
> Signed-off-by: Shunsuke Mie <mie@igel.co.jp>
> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 5b833f00e980..a5ed779b0a51 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -627,7 +627,6 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
>  
>  	cancel_delayed_work(&epf_test->cmd_handler);
>  	pci_epf_test_clean_dma_chan(epf_test);
> -	pci_epc_stop(epc);
>  	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
>  		epf_bar = &epf->bar[bar];
>  
> -- 
> 2.17.1
>

Re: [PATCH] PCI: endpoint: Don't stop EP controller by EP function

Posted by Shunsuke Mie 3 years, 9 months ago

2022年7月6日(水) 7:40 Bjorn Helgaas <helgaas@kernel.org>:

>
> On Wed, Jun 22, 2022 at 01:09:24PM +0900, Shunsuke Mie wrote:
> > For multi-function endpoint device, an ep function shouldn't stop EP
> > controller. Nomally the controller is stopped via configfs.
>
> Can you please clarify this for me?
>
> An endpoint function by itself wouldn't stop an endpoint controller.
> I assume that some *operation* on an endpoint function currently stops
> the endpoint controller, but that operation should not stop the
> controller?
>
> I guess the operation is an "unbind" that detaches an EPF device from
> an EPC device?
It is likely that after all of the endpoint functions are unbound, the
controller can be stopped safely, but I'm not sure if it is desired behavior
for endpoint framework.

Kishon, could you please comment?

> > Fixes: 349e7a85b25f ("PCI: endpoint: functions: Add an EP function to test PCI")
> > Signed-off-by: Shunsuke Mie <mie@igel.co.jp>
> > ---
> >  drivers/pci/endpoint/functions/pci-epf-test.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > index 5b833f00e980..a5ed779b0a51 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > @@ -627,7 +627,6 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
> >
> >       cancel_delayed_work(&epf_test->cmd_handler);
> >       pci_epf_test_clean_dma_chan(epf_test);
> > -     pci_epc_stop(epc);
> >       for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> >               epf_bar = &epf->bar[bar];
> >
> > --
> > 2.17.1
> >

Thanks,
Shunsuke

Re: [PATCH] PCI: endpoint: Don't stop EP controller by EP function

Posted by Bjorn Helgaas 3 years, 9 months ago

On Wed, Jul 06, 2022 at 11:37:29AM +0900, Shunsuke Mie wrote:
> 2022年7月6日(水) 7:40 Bjorn Helgaas <helgaas@kernel.org>:
> > On Wed, Jun 22, 2022 at 01:09:24PM +0900, Shunsuke Mie wrote:
> > > For multi-function endpoint device, an ep function shouldn't stop EP
> > > controller. Nomally the controller is stopped via configfs.
> >
> > Can you please clarify this for me?
> >
> > An endpoint function by itself wouldn't stop an endpoint controller.
> > I assume that some *operation* on an endpoint function currently stops
> > the endpoint controller, but that operation should not stop the
> > controller?
> >
> > I guess the operation is an "unbind" that detaches an EPF device from
> > an EPC device?
>
> It is likely that after all of the endpoint functions are unbound, the
> controller can be stopped safely, but I'm not sure if it is desired behavior
> for endpoint framework.

I'm not asking about the patch itself.  I'm asking about the commit
log because "an EP function shouldn't stop EP controller" doesn't
quite make sense in English.

I suspect it should say something like "unbinding one endpoint
function of a multi-function device from the endpoint controller
should not stop the controller."

But I don't know enough about EPF/EPC binding to know whether that
makes sense either.

> Kishon, could you please comment?
> 
> > > Fixes: 349e7a85b25f ("PCI: endpoint: functions: Add an EP function to test PCI")
> > > Signed-off-by: Shunsuke Mie <mie@igel.co.jp>
> > > ---
> > >  drivers/pci/endpoint/functions/pci-epf-test.c | 1 -
> > >  1 file changed, 1 deletion(-)
> > >
> > > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > > index 5b833f00e980..a5ed779b0a51 100644
> > > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > > @@ -627,7 +627,6 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
> > >
> > >       cancel_delayed_work(&epf_test->cmd_handler);
> > >       pci_epf_test_clean_dma_chan(epf_test);
> > > -     pci_epc_stop(epc);
> > >       for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> > >               epf_bar = &epf->bar[bar];
> > >
> > > --
> > > 2.17.1
> > >
> 
> Thanks,
> Shunsuke

Re: [PATCH] PCI: endpoint: Don't stop EP controller by EP function

Posted by Shunsuke Mie 3 years, 9 months ago

2022年7月6日(水) 12:08 Bjorn Helgaas <helgaas@kernel.org>:
>
> On Wed, Jul 06, 2022 at 11:37:29AM +0900, Shunsuke Mie wrote:
> > 2022年7月6日(水) 7:40 Bjorn Helgaas <helgaas@kernel.org>:
> > > On Wed, Jun 22, 2022 at 01:09:24PM +0900, Shunsuke Mie wrote:
> > > > For multi-function endpoint device, an ep function shouldn't stop EP
> > > > controller. Nomally the controller is stopped via configfs.
> > >
> > > Can you please clarify this for me?
> > >
> > > An endpoint function by itself wouldn't stop an endpoint controller.
> > > I assume that some *operation* on an endpoint function currently stops
> > > the endpoint controller, but that operation should not stop the
> > > controller?
> > >
> > > I guess the operation is an "unbind" that detaches an EPF device from
> > > an EPC device?
> >
> > It is likely that after all of the endpoint functions are unbound, the
> > controller can be stopped safely, but I'm not sure if it is desired behavior
> > for endpoint framework.
>
> I'm not asking about the patch itself.  I'm asking about the commit
> log because "an EP function shouldn't stop EP controller" doesn't
> quite make sense in English.
I'm sorry.

> I suspect it should say something like "unbinding one endpoint
> function of a multi-function device from the endpoint controller
> should not stop the controller."
Yes, it is correct and represents the commit clearly.

> But I don't know enough about EPF/EPC binding to know whether that
> makes sense either.
>
> > Kishon, could you please comment?
> >
> > > > Fixes: 349e7a85b25f ("PCI: endpoint: functions: Add an EP function to test PCI")
> > > > Signed-off-by: Shunsuke Mie <mie@igel.co.jp>
> > > > ---
> > > >  drivers/pci/endpoint/functions/pci-epf-test.c | 1 -
> > > >  1 file changed, 1 deletion(-)
> > > >
> > > > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > > > index 5b833f00e980..a5ed779b0a51 100644
> > > > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > > > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > > > @@ -627,7 +627,6 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
> > > >
> > > >       cancel_delayed_work(&epf_test->cmd_handler);
> > > >       pci_epf_test_clean_dma_chan(epf_test);
> > > > -     pci_epc_stop(epc);
> > > >       for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> > > >               epf_bar = &epf->bar[bar];
> > > >
> > > > --
> > > > 2.17.1
> > > >
> >
> > Thanks,
> > Shunsuke

Re: [PATCH] PCI: endpoint: Don't stop EP controller by EP function

Posted by Bjorn Helgaas 3 years, 9 months ago

On Wed, Jul 06, 2022 at 12:15:38PM +0900, Shunsuke Mie wrote:
> 2022年7月6日(水) 12:08 Bjorn Helgaas <helgaas@kernel.org>:
> > On Wed, Jul 06, 2022 at 11:37:29AM +0900, Shunsuke Mie wrote:
> > > 2022年7月6日(水) 7:40 Bjorn Helgaas <helgaas@kernel.org>:
> > > > On Wed, Jun 22, 2022 at 01:09:24PM +0900, Shunsuke Mie wrote:
> > > > > For multi-function endpoint device, an ep function shouldn't stop EP
> > > > > controller. Nomally the controller is stopped via configfs.
> > > >
> > > > Can you please clarify this for me?
> > > >
> > > > An endpoint function by itself wouldn't stop an endpoint controller.
> > > > I assume that some *operation* on an endpoint function currently stops
> > > > the endpoint controller, but that operation should not stop the
> > > > controller?
> > > >
> > > > I guess the operation is an "unbind" that detaches an EPF device from
> > > > an EPC device?
> > >
> > > It is likely that after all of the endpoint functions are unbound, the
> > > controller can be stopped safely, but I'm not sure if it is desired behavior
> > > for endpoint framework.
> >
> > I'm not asking about the patch itself.  I'm asking about the commit
> > log because "an EP function shouldn't stop EP controller" doesn't
> > quite make sense in English.
> I'm sorry.
> 
> > I suspect it should say something like "unbinding one endpoint
> > function of a multi-function device from the endpoint controller
> > should not stop the controller."
> Yes, it is correct and represents the commit clearly.

Thanks!  I updated the commit log to the following:

  PCI: endpoint: Don't stop controller when unbinding endpoint function

  Unbinding an endpoint function from the endpoint controller shouldn't stop
  the controller.  This is especially a problem for multi-function endpoints
  where other endpoints may still be active.

  Don't stop the controller when unbinding one of its endpoints.  Normally
  the controller is stopped via configfs.

Re: [PATCH] PCI: endpoint: Don't stop EP controller by EP function

Posted by Bjorn Helgaas 3 years, 10 months ago

On Wed, Jun 22, 2022 at 01:09:24PM +0900, Shunsuke Mie wrote:
> For multi-function endpoint device, an ep function shouldn't stop EP
> controller. Nomally the controller is stopped via configfs.
> 
> Fixes: 349e7a85b25f ("PCI: endpoint: functions: Add an EP function to test PCI")
> Signed-off-by: Shunsuke Mie <mie@igel.co.jp>

Fixed typo and applied with Kishon's ack to pci/endpoint for v5.20, thanks!

> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 5b833f00e980..a5ed779b0a51 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -627,7 +627,6 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
>  
>  	cancel_delayed_work(&epf_test->cmd_handler);
>  	pci_epf_test_clean_dma_chan(epf_test);
> -	pci_epc_stop(epc);
>  	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
>  		epf_bar = &epf->bar[bar];
>  
> -- 
> 2.17.1
>

Re: [PATCH] PCI: endpoint: Don't stop EP controller by EP function

Posted by Kishon Vijay Abraham I 3 years, 10 months ago


On 22/06/22 09:39, Shunsuke Mie wrote:
> For multi-function endpoint device, an ep function shouldn't stop EP
> controller. Nomally the controller is stopped via configfs.

%s/Nomally/Normally/
> 
> Fixes: 349e7a85b25f ("PCI: endpoint: functions: Add an EP function to test PCI")
> Signed-off-by: Shunsuke Mie <mie@igel.co.jp>

Thank you for the fix!

Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 5b833f00e980..a5ed779b0a51 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -627,7 +627,6 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
>  
>  	cancel_delayed_work(&epf_test->cmd_handler);
>  	pci_epf_test_clean_dma_chan(epf_test);
> -	pci_epc_stop(epc);
>  	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
>  		epf_bar = &epf->bar[bar];
>

Re: [PATCH] PCI: endpoint: Don't stop EP controller by EP function

Posted by Shunsuke Mie 3 years, 10 months ago

Hi Kishon,

Thank you for your ack.

2022年6月22日(水) 14:10 Kishon Vijay Abraham I <kishon@ti.com>:
>
>
>
> On 22/06/22 09:39, Shunsuke Mie wrote:
> > For multi-function endpoint device, an ep function shouldn't stop EP
> > controller. Nomally the controller is stopped via configfs.
>
> %s/Nomally/Normally/
Oops, sorry. Should I resend this patch with fixing the typo?

> > Fixes: 349e7a85b25f ("PCI: endpoint: functions: Add an EP function to test PCI")
> > Signed-off-by: Shunsuke Mie <mie@igel.co.jp>
>
> Thank you for the fix!
>
> Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
> > ---
> >  drivers/pci/endpoint/functions/pci-epf-test.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > index 5b833f00e980..a5ed779b0a51 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > @@ -627,7 +627,6 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
> >
> >       cancel_delayed_work(&epf_test->cmd_handler);
> >       pci_epf_test_clean_dma_chan(epf_test);
> > -     pci_epc_stop(epc);
> >       for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> >               epf_bar = &epf->bar[bar];
> >

Thanks.
Shunsuke Mie