On Tue, Jun 13, 2023 at 12:21 PM Elliot Berman [off-list ref] wrote:
......

I believe some includes could be removed without issue.

+#define mbox_chan_to_msgq(chan) (container_of(chan->mbox, struct gh_msgq, mbox))
+

container_of need not be in brackets.

+static irqreturn_t gh_msgq_rx_irq_handler(int irq, void *data)
+{
+       struct gh_msgq *msgq = data;
+       struct gh_msgq_rx_data rx_data;
+       enum gh_error gh_error;
+       bool ready = true;
+

Please limit the scope of rx_data and gh_error by moving them inside
the while() body.

+       while (ready) {
+               gh_error = gh_hypercall_msgq_recv(msgq->rx_ghrsc->capid,
+                               &rx_data.data, sizeof(rx_data.data),
+                               &rx_data.length, &ready);
+               if (gh_error != GH_ERROR_OK) {
+                       if (gh_error != GH_ERROR_MSGQUEUE_EMPTY)
+                               dev_warn(msgq->mbox.dev, "Failed to receive data: %d\n", gh_error);
+                       break;
+               }
+               if (likely(gh_msgq_chan(msgq)->cl))
+                       mbox_chan_received_data(gh_msgq_chan(msgq), &rx_data);
+       }
+
+       return IRQ_HANDLED;
+}
+

+static int gh_msgq_send_data(struct mbox_chan *chan, void *data)
+{
+       struct gh_msgq *msgq = mbox_chan_to_msgq(chan);
+       struct gh_msgq_tx_data *msgq_data = data;
+       u64 tx_flags = 0;
+       enum gh_error gh_error;
+       bool ready;
+
+       if (!msgq->tx_ghrsc)
+               return -EOPNOTSUPP;
+

If we hit this error, the fix will still be in the upper layer.
So please drop the check and, if needed, add one in the client driver.

+       if (msgq_data->push)
+               tx_flags |= GH_HYPERCALL_MSGQ_TX_FLAGS_PUSH;
+
+       gh_error = gh_hypercall_msgq_send(msgq->tx_ghrsc->capid, msgq_data->length, msgq_data->data,
+                                               tx_flags, &ready);
+
+       /**
+        * unlikely because Linux tracks state of msgq and should not try to
+        * send message when msgq is full.
+        */
+       if (unlikely(gh_error == GH_ERROR_MSGQUEUE_FULL))
+               return -EAGAIN;
+

If it is not expected to hit, please remove the check.
If there can be a 'race' like situation, still remove this and try to
find an appropriate place to avoid the race.

+       /**
+        * Propagate all other errors to client. If we return error to mailbox
+        * framework, then no other messages can be sent and nobody will know
+        * to retry this message.
+        */
+       msgq->last_ret = gh_error_remap(gh_error);
+
+       /**
+        * This message was successfully sent, but message queue isn't ready to
+        * accept more messages because it's now full. Mailbox framework
+        * requires that we only report that message was transmitted when
+        * we're ready to transmit another message. We'll get that in the form
+        * of tx IRQ once the other side starts to drain the msgq.
+        */
+       if (gh_error == GH_ERROR_OK) {
+               if (!ready)
+                       return 0;
+       } else {
+               dev_err(msgq->mbox.dev, "Failed to send data: %d (%d)\n", gh_error, msgq->last_ret);
+       }
+
+       /**
+        * We can send more messages.

            ... until we can not (when the platform specific queue is full).

            Mailbox framework requires that tx done
+        * happens asynchronously to sending the message.

hence the mailbox api needs to track each transfer's stage.

            Gunyah message queues
+        * tell us right away on the hypercall return whether we can send more
+        * messages. To work around this, defer the txdone to a tasklet.
+        */

If not here, you would still have to put the 'defer' somewhere in the
upper layer.
So it is not exactly a "workaround".

+       tasklet_schedule(&msgq->txdone_tasklet);
+
+       return 0;
+}
+

controller.h and client.h aren't supposed to be in the same header.
The Client and the Controller driver could include them respectively
before this header.

Cheers.