| Message ID | 1576844677-24933-1-git-send-email-hayashi.kunihiko@socionext.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RESEND] PCI: endpoint: Fix clearing start entry in configfs | expand |
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 >
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
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 >
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 >>
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
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; }
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(+)