Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for each channel

From: Jassi Brar
Date: Tue Jul 25 2017 - 12:07:38 EST


On Tue, Jul 25, 2017 at 11:11 AM, Anup Patel <anup.patel@xxxxxxxxxxxx> wrote:
> On Mon, Jul 24, 2017 at 10:06 PM, Jassi Brar <jassisinghbrar@xxxxxxxxx> wrote:
>> On Mon, Jul 24, 2017 at 9:26 AM, Anup Patel <anup.patel@xxxxxxxxxxxx> wrote:
>>> Hi Jassi,
>>>
>>> Sorry for the delayed response...
>>>
>>> On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar <jassisinghbrar@xxxxxxxxx> wrote:
>>>> Hi Anup,
>>>>
>>>> On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel <anup.patel@xxxxxxxxxxxx> wrote:
>>>>> The Broadcom FlexRM ring (i.e. mailbox channel) can handle
>>>>> larger number of messages queued in one FlexRM ring hence
>>>>> this patch sets msg_queue_len for each mailbox channel to
>>>>> be same as RING_MAX_REQ_COUNT.
>>>>>
>>>>> Signed-off-by: Anup Patel <anup.patel@xxxxxxxxxxxx>
>>>>> Reviewed-by: Scott Branden <scott.branden@xxxxxxxxxxxx>
>>>>> ---
>>>>> drivers/mailbox/bcm-flexrm-mailbox.c | 5 ++++-
>>>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c b/drivers/mailbox/bcm-flexrm-mailbox.c
>>>>> index 9873818..20055a0 100644
>>>>> --- a/drivers/mailbox/bcm-flexrm-mailbox.c
>>>>> +++ b/drivers/mailbox/bcm-flexrm-mailbox.c
>>>>> @@ -1683,8 +1683,11 @@ static int flexrm_mbox_probe(struct platform_device *pdev)
>>>>> ret = -ENOMEM;
>>>>> goto fail_free_debugfs_root;
>>>>> }
>>>>> - for (index = 0; index < mbox->num_rings; index++)
>>>>> + for (index = 0; index < mbox->num_rings; index++) {
>>>>> + mbox->controller.chans[index].msg_queue_len =
>>>>> + RING_MAX_REQ_COUNT;
>>>>> mbox->controller.chans[index].con_priv = &mbox->rings[index];
>>>>> + }
>>>>>
>>>> While writing mailbox.c I wasn't unaware that there is the option to
>>>> choose the queue length at runtime.
>>>> The idea was to keep the code as simple as possible. I am open to
>>>> making it a runtime thing, but first, please help me understand how
>>>> that is useful here.
>>>>
>>>> I understand FlexRm has a ring buffer of RING_MAX_REQ_COUNT(1024)
>>>> elements. Any message submitted to mailbox api can be immediately
>>>> written onto the ringbuffer if there is some space.
>>>> Is there any mechanism to report back to a client driver, if its
>>>> message in ringbuffer failed "to be sent"?
>>>> If there isn't any, then I think, in flexrm_last_tx_done() you should
>>>> simply return true if there is some space left in the rung-buffer,
>>>> false otherwise.
>>>
>>> Yes, we have error code in "struct brcm_message" to report back
>>> errors from send_message. In our mailbox clients, we check
>>> return value of mbox_send_message() and also the error code
>>> in "struct brcm_message".
>>>
>> I meant after the message has been accepted in the ringbuffer but the
>> remote failed to receive it.
>
> Yes, even this case is handled.
>
> In case of IO errors after message has been put in ring buffer, we get
> completion message with error code and mailbox client drivers will
> receive back "struct brcm_message" with error set.
>
> You can refer flexrm_process_completions() for more details.
>
>> There seems no such provision. IIANW, then you should be able to
>> consider every message as "sent successfully" once it is in the ring
>> buffer i.e, immediately after mbox_send_message() returns 0.
>> In that case I would think you don't need more than a couple of
>> entries out of MBOX_TX_QUEUE_LEN ?
>
> What I am trying to suggest is that we can take upto 1024 messages
> in a FlexRM ring but the MBOX_TX_QUEUE_LEN limits us queuing
> more messages. This issue manifest easily when multiple CPUs
> queues to same FlexRM ring (i.e. same mailbox channel).
>
OK then, I guess we have to make the queue length a runtime decision.

BTW, is it a practical use case that needs to queue upto 1024
requests? Or are you just testing?

Thanks