linux-kernel - Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CABb+yY2x1Wb=RZhq+eUnM0wkcih4YkFDrKRgqhs1erGQGH3tdw@mail.gmail.com>
Date:   Tue, 25 Jul 2017 21:37:31 +0530
From:   Jassi Brar <jassisinghbrar@...il.com>
To:     Anup Patel <anup.patel@...adcom.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will.deacon@....com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Scott Branden <sbranden@...adcom.com>,
        Ray Jui <rjui@...adcom.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        Devicetree List <devicetree@...r.kernel.org>,
        BCM Kernel Feedback <bcm-kernel-feedback-list@...adcom.com>
Subject: Re: [PATCH v2 6/7] mailbox: bcm-flexrm-mailbox: Set msg_queue_len for
 each channel

On Tue, Jul 25, 2017 at 11:11 AM, Anup Patel <anup.patel@...adcom.com> wrote:
> On Mon, Jul 24, 2017 at 10:06 PM, Jassi Brar <jassisinghbrar@...il.com> wrote:
>> On Mon, Jul 24, 2017 at 9:26 AM, Anup Patel <anup.patel@...adcom.com> wrote:
>>> Hi Jassi,
>>>
>>> Sorry for the delayed response...
>>>
>>> On Fri, Jul 21, 2017 at 9:16 PM, Jassi Brar <jassisinghbrar@...il.com> wrote:
>>>> Hi Anup,
>>>>
>>>> On Fri, Jul 21, 2017 at 12:25 PM, Anup Patel <anup.patel@...adcom.com> 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@...adcom.com>
>>>>> Reviewed-by: Scott Branden <scott.branden@...adcom.com>
>>>>> ---
>>>>>  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