* [PATCH RESEND] PCI: endpoint: Fix clearing start entry in configfs
@ 2019-12-20 12:24 Kunihiko Hayashi
2020-02-21 14:15 ` Lorenzo Pieralisi
0 siblings, 1 reply; 6+ messages in thread
From: Kunihiko Hayashi @ 2019-12-20 12:24 UTC (permalink / raw)
To: Kishon Vijay Abraham I, Lorenzo Pieralisi
Cc: linux-pci, linux-kernel, Kunihiko Hayashi
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(+)
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH RESEND] PCI: endpoint: Fix clearing start entry in configfs
2019-12-20 12:24 [PATCH RESEND] PCI: endpoint: Fix clearing start entry in configfs Kunihiko Hayashi
@ 2020-02-21 14:15 ` Lorenzo Pieralisi
2020-02-25 10:05 ` Kunihiko Hayashi
0 siblings, 1 reply; 6+ messages in thread
From: Lorenzo Pieralisi @ 2020-02-21 14:15 UTC (permalink / raw)
To: Kunihiko Hayashi, Kishon Vijay Abraham I; +Cc: linux-pci, linux-kernel
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RESEND] PCI: endpoint: Fix clearing start entry in configfs
2020-02-21 14:15 ` Lorenzo Pieralisi
@ 2020-02-25 10:05 ` Kunihiko Hayashi
2020-02-25 10:18 ` Lorenzo Pieralisi
0 siblings, 1 reply; 6+ messages in thread
From: Kunihiko Hayashi @ 2020-02-25 10:05 UTC (permalink / raw)
To: Lorenzo Pieralisi, Kishon Vijay Abraham I; +Cc: linux-pci, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RESEND] PCI: endpoint: Fix clearing start entry in configfs
2020-02-25 10:05 ` Kunihiko Hayashi
@ 2020-02-25 10:18 ` Lorenzo Pieralisi
2020-02-25 11:12 ` Kishon Vijay Abraham I
0 siblings, 1 reply; 6+ messages in thread
From: Lorenzo Pieralisi @ 2020-02-25 10:18 UTC (permalink / raw)
To: Kunihiko Hayashi; +Cc: Kishon Vijay Abraham I, linux-pci, linux-kernel
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RESEND] PCI: endpoint: Fix clearing start entry in configfs
2020-02-25 10:18 ` Lorenzo Pieralisi
@ 2020-02-25 11:12 ` Kishon Vijay Abraham I
2020-02-26 1:20 ` Kunihiko Hayashi
0 siblings, 1 reply; 6+ messages in thread
From: Kishon Vijay Abraham I @ 2020-02-25 11:12 UTC (permalink / raw)
To: Lorenzo Pieralisi, Kunihiko Hayashi; +Cc: linux-pci, linux-kernel
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
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RESEND] PCI: endpoint: Fix clearing start entry in configfs
2020-02-25 11:12 ` Kishon Vijay Abraham I
@ 2020-02-26 1:20 ` Kunihiko Hayashi
0 siblings, 0 replies; 6+ messages in thread
From: Kunihiko Hayashi @ 2020-02-26 1:20 UTC (permalink / raw)
To: Kishon Vijay Abraham I, Lorenzo Pieralisi; +Cc: linux-pci, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-02-26 1:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-20 12:24 [PATCH RESEND] PCI: endpoint: Fix clearing start entry in configfs Kunihiko Hayashi
2020-02-21 14:15 ` Lorenzo Pieralisi
2020-02-25 10:05 ` Kunihiko Hayashi
2020-02-25 10:18 ` Lorenzo Pieralisi
2020-02-25 11:12 ` Kishon Vijay Abraham I
2020-02-26 1:20 ` Kunihiko Hayashi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).