Re: [PATCH RESEND] PCI: endpoint: Fix clearing start entry in configfs

[Kunihiko Hayashi <hayashi.kunihiko@socionext.com>]

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