Re: [PATCH 04/18] rpmsg: glink: Move the common glink protocol implementation to glink_native.c

[Sricharan R]

Hi Arun,
  Thanks for the review.

On 8/22/2017 11:28 AM, Arun Kumar Neelakantam wrote:
> 
> 
> On 8/16/2017 10:48 PM, Sricharan R wrote:
>> +
>> +struct glink_msg {
>> +ÂÂÂ __le16 cmd;
>> +ÂÂÂ __le16 param1;
>> +ÂÂÂ __le32 param2;
>> +ÂÂÂ u8 data[];
>> +} __packed;
> 
> why we are using extra u8 data[] member here ?
> 
 Just as a zero-sized placeholder, to read a variable length header if required.

>> +
>> +/**
>> + * struct glink_defer_cmd - deferred incoming control message
>> + * @node:ÂÂÂ list node
>> + * @msg:ÂÂÂ message header
>> + * data:ÂÂÂ payload of the message
>> + *
>> + * Copy of a received control message, to be added to @rx_queue and processed
>> + * by @rx_work of @glink_rpm.
>> + */
>> +struct glink_defer_cmd {
>> +ÂÂÂ struct list_head node;
>> +
>> +ÂÂÂ struct glink_msg msg;
>> +ÂÂÂ u8 data[];
>> +};
>> +
>> +/**
>> + * struct glink_rpm - driver context, relates to one remote subsystem
> 
> glink_rpm to qcom_glink
> 
 ok, will change.

>> +static int qcom_glink_tx(struct qcom_glink *glink,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂ const void *hdr, size_t hlen,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂ const void *data, size_t dlen, bool wait)
>> +{
>> +ÂÂÂ unsigned int tlen = hlen + dlen;
>> +ÂÂÂ int ret;
>> +
>> +ÂÂÂ /* Reject packets that are too big */
>> +ÂÂÂ if (tlen >= glink->tx_pipe->length)
>> +ÂÂÂÂÂÂÂ return -EINVAL;
> 
> we need to add support for split packets, in some cases packets may be greater than 16K.

 ok, the plan was to add that in next set of patches and keep this series to a minimum.

>> +
>> +ÂÂÂ if (WARN(tlen % 8, "Unaligned TX request"))
>> +ÂÂÂÂÂÂÂ return -EINVAL;
>> +
>> +ÂÂÂ ret = mutex_lock_interruptible(&glink->tx_lock);
>> +ÂÂÂ if (ret)
>> +ÂÂÂÂÂÂÂ return ret;
>> +
>> +ÂÂÂ while (qcom_glink_tx_avail(glink) < tlen) {
>> +ÂÂÂÂÂÂÂ if (!wait) {
>> +ÂÂÂÂÂÂÂÂÂÂÂ ret = -ENOMEM;
> 
> This condition is either reader is slow or not reading data, should we return -EAGAIN here instead of -ENOMEM?

 ok, will change.

>> +ÂÂÂÂÂÂÂÂÂÂÂ goto out;
>> +ÂÂÂÂÂÂÂ }
>> +
>> +ÂÂÂÂÂÂÂ msleep(10);
>> +ÂÂÂ }
>> +
>> +ÂÂÂ qcom_glink_tx_write(glink, hdr, hlen, data, dlen);
>> +
>> +ÂÂÂ mbox_send_message(glink->mbox_chan, NULL);
>> +ÂÂÂ mbox_client_txdone(glink->mbox_chan, 0);
>> +
>> +out:
>> +ÂÂÂ mutex_unlock(&glink->tx_lock);
>> +
>> +ÂÂÂ return ret;
>> +}
>> +
> 
> 
>> +/**
>> + * qcom_glink_send_open_req() - send a RPM_CMD_OPEN request to the remote
>> + * @glink:
>> + * @channel:
> 
> Missed information for @ glink and @channel

 ok, will add.

>> + *
>> + * Allocates a local channel id and sends a RPM_CMD_OPEN message to the remote.
>> + * Will return with refcount held, regardless of outcome.
>> + *
>> + * Returns 0 on success, negative errno otherwise.
>> + */
>> +static int qcom_glink_send_open_req(struct qcom_glink *glink,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct glink_channel *channel)
> 
> 
>> +static irqreturn_t qcom_glink_native_intr(int irq, void *data)
>> +{
>> +ÂÂÂ struct qcom_glink *glink = data;
>> +ÂÂÂ struct glink_msg msg;
>> +ÂÂÂ unsigned int param1;
>> +ÂÂÂ unsigned int param2;
>> +ÂÂÂ unsigned int avail;
>> +ÂÂÂ unsigned int cmd;
>> +ÂÂÂ int ret;
>> +
>> +ÂÂÂ for (;;) {
>> +ÂÂÂÂÂÂÂ avail = qcom_glink_rx_avail(glink);
>> +ÂÂÂÂÂÂÂ if (avail < sizeof(msg))
>> +ÂÂÂÂÂÂÂÂÂÂÂ break;
>> +
>> +ÂÂÂÂÂÂÂ qcom_glink_rx_peak(glink, &msg, sizeof(msg));
>> +
>> +ÂÂÂÂÂÂÂ cmd = le16_to_cpu(msg.cmd);
>> +ÂÂÂÂÂÂÂ param1 = le16_to_cpu(msg.param1);
>> +ÂÂÂÂÂÂÂ param2 = le32_to_cpu(msg.param2);
>> +
>> +ÂÂÂÂÂÂÂ switch (cmd) {
>> +ÂÂÂÂÂÂÂ case RPM_CMD_VERSION:
>> +ÂÂÂÂÂÂÂ case RPM_CMD_VERSION_ACK:
>> +ÂÂÂÂÂÂÂ case RPM_CMD_CLOSE:
>> +ÂÂÂÂÂÂÂ case RPM_CMD_CLOSE_ACK:
>> +ÂÂÂÂÂÂÂÂÂÂÂ ret = qcom_glink_rx_defer(glink, 0);
>> +ÂÂÂÂÂÂÂÂÂÂÂ break;
>> +ÂÂÂÂÂÂÂ case RPM_CMD_OPEN_ACK:
>> +ÂÂÂÂÂÂÂÂÂÂÂ ret = qcom_glink_rx_open_ack(glink, param1);
>> +ÂÂÂÂÂÂÂÂÂÂÂ qcom_glink_rx_advance(glink, ALIGN(sizeof(msg), 8));
>> +ÂÂÂÂÂÂÂÂÂÂÂ break;
>> +ÂÂÂÂÂÂÂ case RPM_CMD_OPEN:
>> +ÂÂÂÂÂÂÂÂÂÂÂ ret = qcom_glink_rx_defer(glink, param2);
>> +ÂÂÂÂÂÂÂÂÂÂÂ break;
>> +ÂÂÂÂÂÂÂ case RPM_CMD_TX_DATA:
>> +ÂÂÂÂÂÂÂ case RPM_CMD_TX_DATA_CONT:
>> +ÂÂÂÂÂÂÂÂÂÂÂ ret = qcom_glink_rx_data(glink, avail);
>> +ÂÂÂÂÂÂÂÂÂÂÂ break;
>> +ÂÂÂÂÂÂÂ case RPM_CMD_READ_NOTIF:
>> +ÂÂÂÂÂÂÂÂÂÂÂ qcom_glink_rx_advance(glink, ALIGN(sizeof(msg), 8));
>> +
>> +ÂÂÂÂÂÂÂÂÂÂÂ mbox_send_message(glink->mbox_chan, NULL);
>> +ÂÂÂÂÂÂÂÂÂÂÂ mbox_client_txdone(glink->mbox_chan, 0);
>> +
>> +ÂÂÂÂÂÂÂÂÂÂÂ ret = 0;
>> +ÂÂÂÂÂÂÂÂÂÂÂ break;
>> +ÂÂÂÂÂÂÂ default:
>> +ÂÂÂÂÂÂÂÂÂÂÂ dev_err(glink->dev, "unhandled rx cmd: %d\n", cmd);
> 
> please add more information in error log to find the remote peripheral also other wise after adding SMEM transport it will be difficult to find for which peripheral the error came.

 ok, you refer to logging the "edge" name, that should be taken care by the dev_err print ?
 That said, i think the dev name in glink smem can be modified to reflect the edge name
 instead of simply printing the remoteproc name. Will change that.

>> +ÂÂÂÂÂÂÂÂÂÂÂ ret = -EINVAL;
>> +ÂÂÂÂÂÂÂÂÂÂÂ break;
>> +ÂÂÂÂÂÂÂ }
>> +
>> +ÂÂÂÂÂÂÂ if (ret)
>> +ÂÂÂÂÂÂÂÂÂÂÂ break;
>> +ÂÂÂ }
>> +
>> +ÂÂÂ return IRQ_HANDLED;
>> +}
>> +
> 
>> +struct qcom_glink *qcom_glink_native_probe(struct device *dev,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct qcom_glink_pipe *rx,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct qcom_glink_pipe *tx)
>> +{
>> +ÂÂÂ int irq;
>> +ÂÂÂ int ret;
>> +ÂÂÂ struct qcom_glink *glink;
>> +
>> +ÂÂÂ glink = devm_kzalloc(dev, sizeof(*glink), GFP_KERNEL);
>> +ÂÂÂ if (!glink)
>> +ÂÂÂÂÂÂÂ return ERR_PTR(-ENOMEM);
>> +
>> +ÂÂÂ glink->dev = dev;
>> +ÂÂÂ glink->tx_pipe = tx;
>> +ÂÂÂ glink->rx_pipe = rx;
>> +
>> +ÂÂÂ mutex_init(&glink->tx_lock);
>> +ÂÂÂ spin_lock_init(&glink->rx_lock);
>> +ÂÂÂ INIT_LIST_HEAD(&glink->rx_queue);
>> +ÂÂÂ INIT_WORK(&glink->rx_work, qcom_glink_work);
>> +
>> +ÂÂÂ mutex_init(&glink->idr_lock);
>> +ÂÂÂ idr_init(&glink->lcids);
>> +ÂÂÂ idr_init(&glink->rcids);
>> +
>> +ÂÂÂ glink->mbox_client.dev = dev;
>> +ÂÂÂ glink->mbox_chan = mbox_request_channel(&glink->mbox_client, 0);
>> +ÂÂÂ if (IS_ERR(glink->mbox_chan)) {
>> +ÂÂÂÂÂÂÂ if (PTR_ERR(glink->mbox_chan) != -EPROBE_DEFER)
>> +ÂÂÂÂÂÂÂÂÂÂÂ dev_err(dev, "failed to acquire IPC channel\n");
>> +ÂÂÂÂÂÂÂ return ERR_CAST(glink->mbox_chan);
>> +ÂÂÂ }
>> +
>> +ÂÂÂ irq = of_irq_get(dev->of_node, 0);
>> +ÂÂÂ ret = devm_request_irq(dev, irq,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ qcom_glink_native_intr,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ IRQF_NO_SUSPEND | IRQF_SHARED,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "glink-native", glink);
>> +ÂÂÂ if (ret) {
>> +ÂÂÂÂÂÂÂ dev_err(dev, "failed to request IRQ\n");
>> +ÂÂÂÂÂÂÂ return ERR_PTR(ret);
>> +ÂÂÂ }
> 
> These IRQs are wake-up-capable, please use enable_irq_wale() also

 ok.

Regards,
 Sricharan

-- 
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus