Date:   Tue, 22 Aug 2017 17:57:19 +0530
From:   Sricharan R <sricharan@...eaurora.org>
To:     Arun Kumar Neelakantam <aneela@...eaurora.org>, ohad@...ery.com,
        bjorn.andersson@...aro.org, linux-remoteproc@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 04/18] rpmsg: glink: Move the common glink protocol
 implementation to glink_native.c

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

Powered by blists - more mailing lists