Re: [PATCH v5 09/13] mailbox: Add Gunyah message queue mailbox

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On 14/10/2022 01:32, Elliot Berman wrote:
> 
> 
> On 10/12/2022 2:47 PM, Dmitry Baryshkov wrote:
>> On 11/10/2022 03:08, Elliot Berman wrote:
>>> +
>>> +static irqreturn_t gh_msgq_tx_irq_handler(int irq, void *data)
>>> +{
>>> +    struct gunyah_msgq *msgq = data;
>>> +
>>> +    mbox_chan_txdone(gunyah_msgq_chan(msgq), 0);
>>> +
>>> +    return IRQ_HANDLED;
>>> +}
>>> +
>>> +static void gh_msgq_txdone_tasklet(unsigned long data)
>>> +{
>>> +    struct gunyah_msgq *msgq = (struct gunyah_msgq *)data;
>>> +
>>> +    mbox_chan_txdone(gunyah_msgq_chan(msgq), msgq->last_status);
>>
>> I don't quite get this. Why do you need both an IRQ and a tasklet?
>>
> 
> I've now tweaked the code comments now as well to explain a bit better.
> 
> Gunyah tells us in the hypercall itself whether the message queue is 
> full. Once the the message queue is full, Gunyah will let us know when 
> reader starts draining the queue and we can start adding more messages 
> via the tx_irq.
> 
> One point to note: the last message to be sent into the message queue 
> that makes the queue full can be detected. The hypercall reports that 
> the message was sent (GH_ERROR_OK) and the "ready" return value is 
> false. In its current form, the msgq mailbox driver should never make a 
> send hypercall and get GH_ERROR_MSGQUEUE_FULL because the driver 
> properly track when the message queue is full.
> 
> When mailbox driver reports txdone, the implication is that more 
> messages can be sent (not just that the message was transmitted). In 
> typical operation, the msgq mailbox driver can immediately report that 
> the message was sent and no tx_irq happens because the hypercall returns 
> GH_ERROR_OK and ready=true. The mailbox framework doesn't allow txdone 
> directly from the send_data callback. To work around that, Jassi 
> recommended we use tasklet [1]. In the "atypical" case where message 
> queue becomes full, we get GH_ERROR_OK and ready=false. In that case, we 
> don't report txdone right away with the tasklet and instead wait for the 
> tx_irq to know when more messages can be sent.

Can we please get some sort of this information into the comments in the 
source file?

> 
> [1]: Tasklet works because send_data is called from mailbox framework 
> with interrupts disabled. Once interrupts are re-enabled, the txdone is 
> allowed to happen which is also when tasklet runs.
> 
>>> +
>>> +    /**
>>> +     * EAGAIN: message didn't send.
>>> +     * ret = 1: message sent, but now the message queue is full and 
>>> we can't send any more msgs.
>>> +     * Either way, don't report that this message is done.
>>> +     */
>>> +    if (ret == -EAGAIN || ret == 1)
>>> +        return ret;
>>
>> '1' doesn't seem to be a valid return code for _send_data.
>>
>> Also it would be logical to return any error here, not just -EAGAIN.
>>
> 
> 
> If I return error to mailbox framework, then the message is stuck: 
> clients don't know that there was some underlying transport failure. It 
> would be retried if the client sends another message, but there is no 
> guarantee that either retrying later would work (what would have 
> changed?) nor that client would send another message to trigger retry. 
> If the message is malformed or message queue not correctly set up, 
> client would never know. Client should be told that the message wasn't 
> sent.

I see. msg_submit() doesn't propagate the error.

> 
> 
>>> +int gunyah_msgq_init(struct device *parent, struct gunyah_msgq 
>>> *msgq, struct mbox_client *cl,
>>> +             struct gunyah_resource *tx_ghrsc, struct 
>>> gunyah_resource *rx_ghrsc)
>>
>> Are the message queues allocated/created dynamically or statically? If 
>> the later is true, please use devm_request(_threaded)_irq and 
>> devm_kzalloc.
>>
> 
> With the exception of resource manager, message queues are created 
> dynamically.
> 
> P.S. Thanks for all the other suggestions in this and the other patches, 
> I've applied them.
> 
> Thanks,
> Elliot

-- 
With best wishes
Dmitry