[RESEND] PCI: endpoint: Fix clearing start entry in configfs
diff mbox series

Message ID 1576844677-24933-1-git-send-email-hayashi.kunihiko@socionext.com
State Accepted
Commit f58d5f53c89479c12ad719c1960176442add5aaa
Headers show
Series
  • [RESEND] PCI: endpoint: Fix clearing start entry in configfs
Related show

Commit Message

Kunihiko Hayashi Dec. 20, 2019, 12:24 p.m. UTC
The value of 'start' entry is no change whenever writing 0 to configfs.
So the endpoint that stopped once can't restart.

The following command lines are an example restarting endpoint and
reprogramming configurations after receiving bus-reset.

    echo 0 > controllers/66000000.pcie-ep/start
    rm controllers/66000000.pcie-ep/func1
    ln -s functions/pci_epf_test/func1 controllers/66000000.pcie-ep/
    echo 1 > controllers/66000000.pcie-ep/start

However, the first 'echo' can't set 0 to 'start', so the last 'echo' can't
restart endpoint.

Fixes: d74679911610 ("PCI: endpoint: Introduce configfs entry for configuring EP functions")
Cc: Kishon Vijay Abraham I <kishon@ti.com>
Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
 drivers/pci/endpoint/pci-ep-cfs.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Lorenzo Pieralisi Feb. 21, 2020, 2:15 p.m. UTC | #1
On Fri, Dec 20, 2019 at 09:24:37PM +0900, Kunihiko Hayashi wrote:
> The value of 'start' entry is no change whenever writing 0 to configfs.
> So the endpoint that stopped once can't restart.
> 
> The following command lines are an example restarting endpoint and
> reprogramming configurations after receiving bus-reset.
> 
>     echo 0 > controllers/66000000.pcie-ep/start
>     rm controllers/66000000.pcie-ep/func1
>     ln -s functions/pci_epf_test/func1 controllers/66000000.pcie-ep/
>     echo 1 > controllers/66000000.pcie-ep/start
> 
> However, the first 'echo' can't set 0 to 'start', so the last 'echo' can't
> restart endpoint.

I think your description is not correct - pci_epc_group->start is
just used to check if an endpoint has been started or not (in
pci_epc_epf_unlink() and that's a WARN_ON) but nonetheless this
looks like a bug and ought to be fixed.

I need Kishon's ACK to proceed.

Thanks,
Lorenzo

> Fixes: d74679911610 ("PCI: endpoint: Introduce configfs entry for configuring EP functions")
> Cc: Kishon Vijay Abraham I <kishon@ti.com>
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> ---
>  drivers/pci/endpoint/pci-ep-cfs.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c
> index d1288a0..4fead88 100644
> --- a/drivers/pci/endpoint/pci-ep-cfs.c
> +++ b/drivers/pci/endpoint/pci-ep-cfs.c
> @@ -58,6 +58,7 @@ static ssize_t pci_epc_start_store(struct config_item *item, const char *page,
>  
>  	if (!start) {
>  		pci_epc_stop(epc);
> +		epc_group->start = 0;
>  		return len;
>  	}
>  
> -- 
> 2.7.4
>
Kunihiko Hayashi Feb. 25, 2020, 10:05 a.m. UTC | #2
Hi Lorenzo,

On Fri, 21 Feb 2020 14:15:53 +0000 <lorenzo.pieralisi@arm.com> wrote:

> On Fri, Dec 20, 2019 at 09:24:37PM +0900, Kunihiko Hayashi wrote:
> > The value of 'start' entry is no change whenever writing 0 to configfs.
> > So the endpoint that stopped once can't restart.
> > 
> > The following command lines are an example restarting endpoint and
> > reprogramming configurations after receiving bus-reset.
> > 
> >     echo 0 > controllers/66000000.pcie-ep/start
> >     rm controllers/66000000.pcie-ep/func1
> >     ln -s functions/pci_epf_test/func1 controllers/66000000.pcie-ep/
> >     echo 1 > controllers/66000000.pcie-ep/start
> > 
> > However, the first 'echo' can't set 0 to 'start', so the last 'echo' can't
> > restart endpoint.
> 
> I think your description is not correct - pci_epc_group->start is
> just used to check if an endpoint has been started or not (in
> pci_epc_epf_unlink() and that's a WARN_ON) but nonetheless this
> looks like a bug and ought to be fixed.

When pci_epc_group->start keeps 1 after starting this controller with
'echo 1 > start', we can never unlink the func1 from the controller
because of WARN_ON.

I think that unlink/re-link play initialization role of configfs
through 'unbind' and 'bind' functions. However, we can't re-initialize
configfs.

If this is the intended behavior, my patch will make no sense.

> I need Kishon's ACK to proceed.

I think so, too.

Thank you,

> 
> Thanks,
> Lorenzo
> 
> > Fixes: d74679911610 ("PCI: endpoint: Introduce configfs entry for configuring EP functions")
> > Cc: Kishon Vijay Abraham I <kishon@ti.com>
> > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> > ---
> >  drivers/pci/endpoint/pci-ep-cfs.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c
> > index d1288a0..4fead88 100644
> > --- a/drivers/pci/endpoint/pci-ep-cfs.c
> > +++ b/drivers/pci/endpoint/pci-ep-cfs.c
> > @@ -58,6 +58,7 @@ static ssize_t pci_epc_start_store(struct config_item *item, const char *page,
> >  
> >  	if (!start) {
> >  		pci_epc_stop(epc);
> > +		epc_group->start = 0;
> >  		return len;
> >  	}
> >  
> > -- 
> > 2.7.4
> > 

---
Best Regards,
Kunihiko Hayashi
Lorenzo Pieralisi Feb. 25, 2020, 10:18 a.m. UTC | #3
On Tue, Feb 25, 2020 at 07:05:07PM +0900, Kunihiko Hayashi wrote:
> Hi Lorenzo,
> 
> On Fri, 21 Feb 2020 14:15:53 +0000 <lorenzo.pieralisi@arm.com> wrote:
> 
> > On Fri, Dec 20, 2019 at 09:24:37PM +0900, Kunihiko Hayashi wrote:
> > > The value of 'start' entry is no change whenever writing 0 to configfs.
> > > So the endpoint that stopped once can't restart.
> > > 
> > > The following command lines are an example restarting endpoint and
> > > reprogramming configurations after receiving bus-reset.
> > > 
> > >     echo 0 > controllers/66000000.pcie-ep/start
> > >     rm controllers/66000000.pcie-ep/func1
> > >     ln -s functions/pci_epf_test/func1 controllers/66000000.pcie-ep/
> > >     echo 1 > controllers/66000000.pcie-ep/start
> > > 
> > > However, the first 'echo' can't set 0 to 'start', so the last 'echo' can't
> > > restart endpoint.
> > 
> > I think your description is not correct - pci_epc_group->start is
> > just used to check if an endpoint has been started or not (in
> > pci_epc_epf_unlink() and that's a WARN_ON) but nonetheless this
> > looks like a bug and ought to be fixed.
> 
> When pci_epc_group->start keeps 1 after starting this controller with
> 'echo 1 > start', we can never unlink the func1 from the controller
> because of WARN_ON.

To me "I can never unlink" means that it can't be done, which
is not what's happening. What's happening is that unlinking triggers
a warning, which is different.

> I think that unlink/re-link play initialization role of configfs
> through 'unbind' and 'bind' functions. However, we can't re-initialize
> configfs.
> 
> If this is the intended behavior, my patch will make no sense.

Your patch makes sense, your commit log does not, see above.

> > I need Kishon's ACK to proceed.

Yes - then I am happy to merge this patch with a rewritten
commit log.

Thanks,
Lorenzo

> I think so, too.
> 
> Thank you,
> 
> > 
> > Thanks,
> > Lorenzo
> > 
> > > Fixes: d74679911610 ("PCI: endpoint: Introduce configfs entry for configuring EP functions")
> > > Cc: Kishon Vijay Abraham I <kishon@ti.com>
> > > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> > > ---
> > >  drivers/pci/endpoint/pci-ep-cfs.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c
> > > index d1288a0..4fead88 100644
> > > --- a/drivers/pci/endpoint/pci-ep-cfs.c
> > > +++ b/drivers/pci/endpoint/pci-ep-cfs.c
> > > @@ -58,6 +58,7 @@ static ssize_t pci_epc_start_store(struct config_item *item, const char *page,
> > >  
> > >  	if (!start) {
> > >  		pci_epc_stop(epc);
> > > +		epc_group->start = 0;
> > >  		return len;
> > >  	}
> > >  
> > > -- 
> > > 2.7.4
> > > 
> 
> ---
> Best Regards,
> Kunihiko Hayashi
>
Kishon Vijay Abraham I Feb. 25, 2020, 11:12 a.m. UTC | #4
Hi,

On 25/02/20 3:48 pm, Lorenzo Pieralisi wrote:
> On Tue, Feb 25, 2020 at 07:05:07PM +0900, Kunihiko Hayashi wrote:
>> Hi Lorenzo,
>>
>> On Fri, 21 Feb 2020 14:15:53 +0000 <lorenzo.pieralisi@arm.com> wrote:
>>
>>> On Fri, Dec 20, 2019 at 09:24:37PM +0900, Kunihiko Hayashi wrote:
>>>> The value of 'start' entry is no change whenever writing 0 to configfs.
>>>> So the endpoint that stopped once can't restart.
>>>>
>>>> The following command lines are an example restarting endpoint and
>>>> reprogramming configurations after receiving bus-reset.
>>>>
>>>>     echo 0 > controllers/66000000.pcie-ep/start
>>>>     rm controllers/66000000.pcie-ep/func1
>>>>     ln -s functions/pci_epf_test/func1 controllers/66000000.pcie-ep/
>>>>     echo 1 > controllers/66000000.pcie-ep/start
>>>>
>>>> However, the first 'echo' can't set 0 to 'start', so the last 'echo' can't
>>>> restart endpoint.
>>>
>>> I think your description is not correct - pci_epc_group->start is
>>> just used to check if an endpoint has been started or not (in
>>> pci_epc_epf_unlink() and that's a WARN_ON) but nonetheless this
>>> looks like a bug and ought to be fixed.
>>
>> When pci_epc_group->start keeps 1 after starting this controller with
>> 'echo 1 > start', we can never unlink the func1 from the controller
>> because of WARN_ON.
> 
> To me "I can never unlink" means that it can't be done, which
> is not what's happening. What's happening is that unlinking triggers
> a warning, which is different.
> 
>> I think that unlink/re-link play initialization role of configfs
>> through 'unbind' and 'bind' functions. However, we can't re-initialize
>> configfs.
>>
>> If this is the intended behavior, my patch will make no sense.
> 
> Your patch makes sense, your commit log does not, see above.
> 
>>> I need Kishon's ACK to proceed.
> 
> Yes - then I am happy to merge this patch with a rewritten
> commit log.

I think all this patch does is fixes an in-correct WARN_ON. The start
and stop of endpoint should happen irrespective of this patch. Once the
commit log is fixed to indicate that

Acked-by: Kishon Vijay Abraham I <kishon@ti.com>

Thanks
Kishon
> 
> Thanks,
> Lorenzo
> 
>> I think so, too.
>>
>> Thank you,
>>
>>>
>>> Thanks,
>>> Lorenzo
>>>
>>>> Fixes: d74679911610 ("PCI: endpoint: Introduce configfs entry for configuring EP functions")
>>>> Cc: Kishon Vijay Abraham I <kishon@ti.com>
>>>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
>>>> ---
>>>>  drivers/pci/endpoint/pci-ep-cfs.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c
>>>> index d1288a0..4fead88 100644
>>>> --- a/drivers/pci/endpoint/pci-ep-cfs.c
>>>> +++ b/drivers/pci/endpoint/pci-ep-cfs.c
>>>> @@ -58,6 +58,7 @@ static ssize_t pci_epc_start_store(struct config_item *item, const char *page,
>>>>  
>>>>  	if (!start) {
>>>>  		pci_epc_stop(epc);
>>>> +		epc_group->start = 0;
>>>>  		return len;
>>>>  	}
>>>>  
>>>> -- 
>>>> 2.7.4
>>>>
>>
>> ---
>> Best Regards,
>> Kunihiko Hayashi
>>
Kunihiko Hayashi Feb. 26, 2020, 1:20 a.m. UTC | #5
Hi, Lorenzo Kishon,

On Tue, 25 Feb 2020 16:42:51 +0530 <kishon@ti.com> wrote:

> Hi,
> 
> On 25/02/20 3:48 pm, Lorenzo Pieralisi wrote:
> > On Tue, Feb 25, 2020 at 07:05:07PM +0900, Kunihiko Hayashi wrote:
> >> Hi Lorenzo,
> >>
> >> On Fri, 21 Feb 2020 14:15:53 +0000 <lorenzo.pieralisi@arm.com> wrote:
> >>
> >>> On Fri, Dec 20, 2019 at 09:24:37PM +0900, Kunihiko Hayashi wrote:
> >>>> The value of 'start' entry is no change whenever writing 0 to configfs.
> >>>> So the endpoint that stopped once can't restart.
> >>>>
> >>>> The following command lines are an example restarting endpoint and
> >>>> reprogramming configurations after receiving bus-reset.
> >>>>
> >>>>     echo 0 > controllers/66000000.pcie-ep/start
> >>>>     rm controllers/66000000.pcie-ep/func1
> >>>>     ln -s functions/pci_epf_test/func1 controllers/66000000.pcie-ep/
> >>>>     echo 1 > controllers/66000000.pcie-ep/start
> >>>>
> >>>> However, the first 'echo' can't set 0 to 'start', so the last 'echo' can't
> >>>> restart endpoint.
> >>>
> >>> I think your description is not correct - pci_epc_group->start is
> >>> just used to check if an endpoint has been started or not (in
> >>> pci_epc_epf_unlink() and that's a WARN_ON) but nonetheless this
> >>> looks like a bug and ought to be fixed.
> >>
> >> When pci_epc_group->start keeps 1 after starting this controller with
> >> 'echo 1 > start', we can never unlink the func1 from the controller
> >> because of WARN_ON.
> > 
> > To me "I can never unlink" means that it can't be done, which
> > is not what's happening. What's happening is that unlinking triggers
> > a warning, which is different.
> > 
> >> I think that unlink/re-link play initialization role of configfs
> >> through 'unbind' and 'bind' functions. However, we can't re-initialize
> >> configfs.
> >>
> >> If this is the intended behavior, my patch will make no sense.
> > 
> > Your patch makes sense, your commit log does not, see above.

I misunderstood the role of the patch. I need to fix the commit log.

> > 
> >>> I need Kishon's ACK to proceed.
> > 
> > Yes - then I am happy to merge this patch with a rewritten
> > commit log.
> 
> I think all this patch does is fixes an in-correct WARN_ON. The start
> and stop of endpoint should happen irrespective of this patch. Once the
> commit log is fixed to indicate that
> 
> Acked-by: Kishon Vijay Abraham I <kishon@ti.com>

Okay, I'll rewrite it next.

Thank you,

> 
> Thanks
> Kishon
> > 
> > Thanks,
> > Lorenzo
> > 
> >> I think so, too.
> >>
> >> Thank you,
> >>
> >>>
> >>> Thanks,
> >>> Lorenzo
> >>>
> >>>> Fixes: d74679911610 ("PCI: endpoint: Introduce configfs entry for configuring EP functions")
> >>>> Cc: Kishon Vijay Abraham I <kishon@ti.com>
> >>>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> >>>> ---
> >>>>  drivers/pci/endpoint/pci-ep-cfs.c | 1 +
> >>>>  1 file changed, 1 insertion(+)
> >>>>
> >>>> diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c
> >>>> index d1288a0..4fead88 100644
> >>>> --- a/drivers/pci/endpoint/pci-ep-cfs.c
> >>>> +++ b/drivers/pci/endpoint/pci-ep-cfs.c
> >>>> @@ -58,6 +58,7 @@ static ssize_t pci_epc_start_store(struct config_item *item, const char *page,
> >>>>  
> >>>>  	if (!start) {
> >>>>  		pci_epc_stop(epc);
> >>>> +		epc_group->start = 0;
> >>>>  		return len;
> >>>>  	}
> >>>>  
> >>>> -- 
> >>>> 2.7.4
> >>>>
> >>
> >> ---
> >> Best Regards,
> >> Kunihiko Hayashi
> >>

---
Best Regards,
Kunihiko Hayashi

Patch
diff mbox series

diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c
index d1288a0..4fead88 100644
--- a/drivers/pci/endpoint/pci-ep-cfs.c
+++ b/drivers/pci/endpoint/pci-ep-cfs.c
@@ -58,6 +58,7 @@  static ssize_t pci_epc_start_store(struct config_item *item, const char *page,
 
 	if (!start) {
 		pci_epc_stop(epc);
+		epc_group->start = 0;
 		return len;
 	}