* [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).