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

[Kishon Vijay Abraham I <kishon@ti.com>]

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
>>