Hi, Lorenzo Kishon,
On Tue, 25 Feb 2020 16:42:51 +0530 <kishon@xxxxxx> 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@xxxxxxx> 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@xxxxxx>
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@xxxxxx>
> >>>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@xxxxxxxxxxxxx>
> >>>> ---
> >>>> 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
