Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <5db6f583-d906-d314-42ef-02f837b40310@foss.st.com>
Date:   Fri, 16 Jun 2023 09:50:24 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH V7 1/3] rpmsg: core: Add signal API support
Content-Language: en-US
To:     Bjorn Andersson <andersson@kernel.org>
CC:     Bjorn Andersson <quic_bjorande@quicinc.com>,
        Sarannya S <quic_sarannya@quicinc.com>, <swboyd@chromium.org>,
        <quic_clew@quicinc.com>, <mathieu.poirier@linaro.org>,
        <linux-kernel@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>,
        <linux-remoteproc@vger.kernel.org>,
        Deepak Kumar Singh <quic_deesin@quicinc.com>
References: <1682160127-18103-1-git-send-email-quic_sarannya@quicinc.com>
 <1682160127-18103-2-git-send-email-quic_sarannya@quicinc.com>
 <20230614155453.dywcrntfjddxojfv@ripper>
 <6e51d6d8-cd3a-b0f2-c044-6282749aae89@foss.st.com>
 <20230615145039.GA3256591@hu-bjorande-lv.qualcomm.com>
 <4d89950d-0376-e355-c70b-d054776e83d4@foss.st.com>
 <20230615164507.mu7fd22poamjth7p@ripper>
From:   Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>
Organization: STMicroelectronics
In-Reply-To: <20230615164507.mu7fd22poamjth7p@ripper>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 6/15/23 18:45, Bjorn Andersson wrote:
> On Thu, Jun 15, 2023 at 06:19:37PM +0200, Arnaud POULIQUEN wrote:
>>
>>
>> On 6/15/23 16:50, Bjorn Andersson wrote:
>>> On Thu, Jun 15, 2023 at 11:01:14AM +0200, Arnaud POULIQUEN wrote:
>>>>
>>>>
>>>> On 6/14/23 17:54, Bjorn Andersson wrote:
>>>>> On Sat, Apr 22, 2023 at 04:12:05PM +0530, Sarannya S wrote:
>>>>>> From: Deepak Kumar Singh <quic_deesin@quicinc.com>
>>>>>>
>>>>>> Some transports like Glink support the state notifications between
>>>>>> clients using flow control signals similar to serial protocol signals.
>>>>>> Local glink client drivers can send and receive flow control status
>>>>>> to glink clients running on remote processors.
>>>>>>
>>>>>> Add APIs to support sending and receiving of flow control status by
>>>>>> rpmsg clients.
>>>>>>
>>>>>> Signed-off-by: Deepak Kumar Singh <quic_deesin@quicinc.com>
>>>>>> Signed-off-by: Sarannya S <quic_sarannya@quicinc.com>
>>>>>> ---
>>>>>>  drivers/rpmsg/rpmsg_core.c     | 21 +++++++++++++++++++++
>>>>>>  drivers/rpmsg/rpmsg_internal.h |  2 ++
>>>>>>  include/linux/rpmsg.h          | 15 +++++++++++++++
>>>>>>  3 files changed, 38 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
>>>>>> index a2207c0..e8bbe05 100644
>>>>>> --- a/drivers/rpmsg/rpmsg_core.c
>>>>>> +++ b/drivers/rpmsg/rpmsg_core.c
>>>>>> @@ -331,6 +331,25 @@ int rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src, u32 dst,
>>>>>>  EXPORT_SYMBOL(rpmsg_trysend_offchannel);
>>>>>>  
>>>>>>  /**
>>>>>> + * rpmsg_set_flow_control() - sets/clears serial flow control signals
>>>>>> + * @ept:	the rpmsg endpoint
>>>>>> + * @enable:	pause/resume incoming data flow	
>>>>>
>>>>> As shown in the discussion, it's still not clear what true/false means.
>>>>> Also, let's try to clarify that it's a request for the other side to do
>>>>> something:
>>>>>
>>>>> * rpmsg_set_flow_control() - request remote to pause/resume transmission
>>>>> * ...
>>>>> * @enable: flow restricted
>>>>> * ...
>>>>>
>>>>>
>>>>> PS. There's a stray space at the end of the line.
>>>>
>>>> The notion of flow restricted seems to me also ambiguous. It does
>>>> not specify if the stream is limited in term of bandwidth or stopped.
>>>>
>>>> What about using XON/XOFF as specified in software flow control[1]
>>>>
>>>> XOFF	Pause transmission
>>>> XON	Resume transmission
>>>>
>>>> or simply pause/resume definitions
>>>>
>>>
>>> I agree, that's still ambiguous.
>>>
>>> I was concerned about expressing it such that the reader would assume
>>> that calling this means there will be no more data coming, but there
>>> might be things in the queues etc. Expressing it in terms of the state
>>> of transmission is clearer.
>>>
>>>
>>> /*
>>>  * rpmsg_set_flow_control() - request remote to pause/resume transmission
>>>  ...
>>>  * @enable: Pause transmission
>>>  ...
>>>  */
>>>
>>> Does that sound okay and clear to you?
>>
>> Much better! I still have a nitpicking point :)
>> What about replacing @enable variable by @pause to align the variable with the
>> usage?
>>  /*
>>   * rpmsg_set_flow_control() - request remote to pause/resume transmission
>>   ...
>>   * @pause: set to 1 to pause transmission, to 0 to resume the transmission
> 
> It's a boolean, so I think with your name change suggestion, together
> with the function description, it should be clear enough what the two
> states (true/false) means.
> 
> * @pause: Pause transmission

Ok for me

Thanks,
Arnaud

> 
> Thanks,
> Bjorn