lkml.org 
[lkml]   [2020]   [Mar]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3 6/7] net: qrtr: Add MHI transport layer
From
Date


On 3/25/2020 3:37 AM, Manivannan Sadhasivam wrote:
> Hi Bjorn,
>
> + Chris Lew
>
> On Tue, Mar 24, 2020 at 01:39:52PM -0700, Bjorn Andersson wrote:
>> On Mon 23 Mar 23:10 PDT 2020, Manivannan Sadhasivam wrote:
>>
>>> MHI is the transport layer used for communicating to the external modems.
>>> Hence, this commit adds MHI transport layer support to QRTR for
>>> transferring the QMI messages over IPC Router.
>>>
>>> Cc: "David S. Miller" <davem@davemloft.net>
>>> Cc: netdev@vger.kernel.org
>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>> ---
>>> net/qrtr/Kconfig | 7 ++
>>> net/qrtr/Makefile | 2 +
>>> net/qrtr/mhi.c | 208 ++++++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 217 insertions(+)
>>> create mode 100644 net/qrtr/mhi.c
>>>
>>> diff --git a/net/qrtr/Kconfig b/net/qrtr/Kconfig
>>> index 63f89cc6e82c..8eb876471564 100644
>>> --- a/net/qrtr/Kconfig
>>> +++ b/net/qrtr/Kconfig
>>> @@ -29,4 +29,11 @@ config QRTR_TUN
>>> implement endpoints of QRTR, for purpose of tunneling data to other
>>> hosts or testing purposes.
>>>
>>> +config QRTR_MHI
>>> + tristate "MHI IPC Router channels"
>>> + depends on MHI_BUS
>>> + help
>>> + Say Y here to support MHI based ipcrouter channels. MHI is the
>>> + transport used for communicating to external modems.
>>> +
>>> endif # QRTR
>>> diff --git a/net/qrtr/Makefile b/net/qrtr/Makefile
>>> index 1c6d6c120fb7..3dc0a7c9d455 100644
>>> --- a/net/qrtr/Makefile
>>> +++ b/net/qrtr/Makefile
>>> @@ -5,3 +5,5 @@ obj-$(CONFIG_QRTR_SMD) += qrtr-smd.o
>>> qrtr-smd-y := smd.o
>>> obj-$(CONFIG_QRTR_TUN) += qrtr-tun.o
>>> qrtr-tun-y := tun.o
>>> +obj-$(CONFIG_QRTR_MHI) += qrtr-mhi.o
>>> +qrtr-mhi-y := mhi.o
>>> diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c
>>> new file mode 100644
>>> index 000000000000..90af208f34c1
>>> --- /dev/null
>>> +++ b/net/qrtr/mhi.c
>>> @@ -0,0 +1,208 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright (c) 2018-2020, The Linux Foundation. All rights reserved.
>>> + */
>>> +
>>> +#include <linux/mhi.h>
>>> +#include <linux/mod_devicetable.h>
>>> +#include <linux/module.h>
>>> +#include <linux/skbuff.h>
>>> +#include <net/sock.h>
>>> +
>>> +#include "qrtr.h"
>>> +
>>> +struct qrtr_mhi_dev {
>>> + struct qrtr_endpoint ep;
>>> + struct mhi_device *mhi_dev;
>>> + struct device *dev;
>>> + spinlock_t ul_lock; /* lock to protect ul_pkts */
>>> + struct list_head ul_pkts;
>>> + atomic_t in_reset;
>>> +};
>>> +
>>> +struct qrtr_mhi_pkt {
>>> + struct list_head node;
>>> + struct sk_buff *skb;
>>> + struct kref refcount;
>>> + struct completion done;
>>> +};
>>> +
>>> +static void qrtr_mhi_pkt_release(struct kref *ref)
>>> +{
>>> + struct qrtr_mhi_pkt *pkt = container_of(ref, struct qrtr_mhi_pkt,
>>> + refcount);
>>> + struct sock *sk = pkt->skb->sk;
>>> +
>>> + consume_skb(pkt->skb);
>>> + if (sk)
>>> + sock_put(sk);
>>> +
>>> + kfree(pkt);
>>> +}
>>> +
>>> +/* From MHI to QRTR */
>>> +static void qcom_mhi_qrtr_dl_callback(struct mhi_device *mhi_dev,
>>> + struct mhi_result *mhi_res)
>>> +{
>>> + struct qrtr_mhi_dev *qdev = dev_get_drvdata(&mhi_dev->dev);
>>> + int rc;
>>> +
>>> + if (!qdev || mhi_res->transaction_status)
>>> + return;
>>> +
>>> + rc = qrtr_endpoint_post(&qdev->ep, mhi_res->buf_addr,
>>> + mhi_res->bytes_xferd);
>>> + if (rc == -EINVAL)
>>> + dev_err(qdev->dev, "invalid ipcrouter packet\n");
>>
>> Perhaps this should be a debug print, perhaps rate limited. But either
>> way it's relevant for any transport, so I think you should skip it here
>> - and potentially move it into qrtr_endpoint_post() in some form.
>>
>
> I agree with moving this to qrtr_endpoint_post() but I don't think it is a
> good idea to make it as a debug print. It is an error log and should stay
> as it is. Only in this MHI transport driver, the return value is not getting
> used but in others it is.
>

This print has been useful for catching transport errors. Only issue I
see with moving this to qrtr_endpoint_post() is that there are multiple
points of failure in qrtr_endpoint_post() and logging it here makes it
easier.

>>> +}
>>> +
>>> +/* From QRTR to MHI */
>>> +static void qcom_mhi_qrtr_ul_callback(struct mhi_device *mhi_dev,
>>> + struct mhi_result *mhi_res)
>>> +{
>>> + struct qrtr_mhi_dev *qdev = dev_get_drvdata(&mhi_dev->dev);
>>> + struct qrtr_mhi_pkt *pkt;
>>> + unsigned long flags;
>>> +
>>> + spin_lock_irqsave(&qdev->ul_lock, flags);
>>> + pkt = list_first_entry(&qdev->ul_pkts, struct qrtr_mhi_pkt, node);
>>> + list_del(&pkt->node);
>>> + complete_all(&pkt->done);
>>
>> You should be able to release the lock after popping the item off the
>> list, then complete and refcount it.
>>
>
> Okay.
>
>>> +
>>> + kref_put(&pkt->refcount, qrtr_mhi_pkt_release);
>>> + spin_unlock_irqrestore(&qdev->ul_lock, flags);
>>> +}
>>> +
>>> +static void qcom_mhi_qrtr_status_callback(struct mhi_device *mhi_dev,
>>> + enum mhi_callback mhi_cb)
>>> +{
>>> + struct qrtr_mhi_dev *qdev = dev_get_drvdata(&mhi_dev->dev);
>>> + struct qrtr_mhi_pkt *pkt;
>>> + unsigned long flags;
>>> +
>>> + if (mhi_cb != MHI_CB_FATAL_ERROR)
>>> + return;
>>> +
>>> + atomic_inc(&qdev->in_reset);
>>
>> You have ul_lock close at hand in both places where you access in_reset,
>> so I think it would be better to just use that, instead of an atomic.
>>
>
> Okay.
>

Does this version of MHI give the MHI_CB_FATAL_ERROR as a status
callback? We added this as an early notifier workaround. The in_reset
code probably isn't required with the current feature set.

>>> + spin_lock_irqsave(&qdev->ul_lock, flags);
>>> + list_for_each_entry(pkt, &qdev->ul_pkts, node)
>>> + complete_all(&pkt->done);
>
> Chris, shouldn't we require list_del(&pkt->node) here?
>

No this isn't a full cleanup, with the "early notifier" we just
unblocked any threads waiting for the ul_callback. Those threads will
wake, check in_reset, return an error back to the caller. Any list
cleanup will be done in the ul_callbacks that the mhi bus will do for
each queued packet right before device remove.

Again to simplify the code, we can probable remove the in_reset handling
since it's not required with the current feature set.

>>> + spin_unlock_irqrestore(&qdev->ul_lock, flags);
>>> +}
>>> +
>>> +/* Send data over MHI */
>>> +static int qcom_mhi_qrtr_send(struct qrtr_endpoint *ep, struct sk_buff *skb)
>>> +{
>>> + struct qrtr_mhi_dev *qdev = container_of(ep, struct qrtr_mhi_dev, ep);
>>> + struct qrtr_mhi_pkt *pkt;
>>> + int rc;
>>> +
>>> + rc = skb_linearize(skb);
>>> + if (rc) {
>>> + kfree_skb(skb);
>>> + return rc;
>>> + }
>>> +
>>> + pkt = kzalloc(sizeof(*pkt), GFP_KERNEL);
>>> + if (!pkt) {
>>> + kfree_skb(skb);
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + init_completion(&pkt->done);
>>> + kref_init(&pkt->refcount);
>>> + kref_get(&pkt->refcount);
>>> + pkt->skb = skb;
>>> +
>>> + spin_lock_bh(&qdev->ul_lock);
>>> + list_add_tail(&pkt->node, &qdev->ul_pkts);
>>> + rc = mhi_queue_skb(qdev->mhi_dev, DMA_TO_DEVICE, skb, skb->len,
>>> + MHI_EOT);
>>
>> Do you want to continue doing this when qdev->in_reset? Wouldn't it be
>> better to bail early if the remote end is dying?
>>
>
> Now I'm thinking why we are not decrementing in_reset anywhere! Incase of
> SYS_ERR, the status_cb will get processed and in_reset will be set but
> it will stay so even when after MHI gets reset.
>
> Chris, can you please clarify?
>

Early notify of reset, if we do get SYS_ERR, we expect remove to be
called *soon*.

>>> + if (rc) {
>>> + list_del(&pkt->node);
>>> + /* Reference count needs to be dropped 2 times */
>>> + kref_put(&pkt->refcount, qrtr_mhi_pkt_release);
>>> + kref_put(&pkt->refcount, qrtr_mhi_pkt_release);
>>> + kfree_skb(skb);
>>> + spin_unlock_bh(&qdev->ul_lock);
>>> + return rc;
>>> + }
>>> +
>>> + spin_unlock_bh(&qdev->ul_lock);
>>> + if (skb->sk)
>>> + sock_hold(skb->sk);
>>> +
>>> + rc = wait_for_completion_interruptible_timeout(&pkt->done, HZ * 5);
>>> + if (atomic_read(&qdev->in_reset))
>>> + rc = -ECONNRESET;
>>> + else if (rc == 0)
>>> + rc = -ETIMEDOUT;
>>
>> Is this recoverable? The message will remain on the list and may be
>> delivered at a later point(?), but qrtr and the app will learn that the
>> message was lost - which is presumably considered fatal.
>>
>> Is it guaranteed that qcom_mhi_qrtr_ul_callback() will happen later and
>> find the head of the list?
>>
>
> There are 2 scenarios:
>
> 1. If the completion interrupt happens after timeout, ul_callback()
> will be called. But it will only try to fetch the current head of ul_pkts.
> In most cases, we can hope that the completion interrupt will happen before
> next queue_skb().
>
> 2. If we don't get completion interrupt, timeout will happen and at the final
> stage (during mhi_driver_remove()), MHI stack will go over the pending TREs
> for all channels in queue and call ul_callback() with -ENOTCONN. But in the
> callback, we don't have any idea of the pkt which was not successfully
> transferred to the device and currently just fetching first entry.
>
> Now I'm seeing some issue here which I missed earlier. If the completion
> interrupt never happens then the corresponding pkt will never get freed and
> therefore we have a leak. Eventhough the ul_callback() will get called during
> mhi_driver_remove() for pending TREs, we don't exactly fetch the right pkt.
>
> Chris, our assumption of the ul_callback() gets called irrespective of
> transfer status is wrong. I think this code needs a bit of rework.
>
>>
>> The reason for my question is that without this you have one of two
>> scenarios;
>> 1) the message is put on the list, decremented in
>> qcom_mhi_qrtr_ul_callback() then we get back here and decrement it
>> again.
>> 2) the message is put on the list, then qcom_mhi_qrtr_status_callback()
>> happens and all messages are released - presumably then
>> qcom_mhi_qrtr_ul_callback() won't happen.
>>
>>
>> So if the third case (where we return here and then later
>> qcom_mhi_qrtr_ul_callback() must find this particular packet at the
>> front of the queue) can't happen, then you can just skip the entire
>> refcounting.
>>
>> Further more, you could carry qrtr_mhi_pkt on the stack.
>>
>>
>> ...or to flip this around, is there a reason to wait here at all? What
>> would happen if you just return immediately after calling
>> mhi_queue_skb()? Wouldn't that provide you better throughput?
>>
>
> Chris would be best person to answer this question.
>

Yea - after working with MHI for a bit now, I think we can definitely
return after queueing the skb.

The IPC-Router Protocol, wasn't really designed with the ability to
recover from dropped packets since SMD/GLINK are "Lossless". I figured
it would be better for the client to definitely know that the packet
reached the other side which is why it blocks until ul callback.

I thought having the client get an error on timeout and resend the
packet would be better than silently dropping it. In practice, we've
really only seen the timeout or ul_callback errors on unrecoverable
errors so I think the timeout handling can definitely be redone.

Thanks,
Chris

>>> + else if (rc > 0)
>>> + rc = 0;
>>> +
>>> + kref_put(&pkt->refcount, qrtr_mhi_pkt_release);
>>> +
>>> + return rc;
>>> +}
>>> +
>>> +static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev,
>>> + const struct mhi_device_id *id)
>>> +{
>>> + struct qrtr_mhi_dev *qdev;
>>> + u32 net_id;
>>> + int rc;
>>> +
>>> + qdev = devm_kzalloc(&mhi_dev->dev, sizeof(*qdev), GFP_KERNEL);
>>> + if (!qdev)
>>> + return -ENOMEM;
>>> +
>>> + qdev->mhi_dev = mhi_dev;
>>> + qdev->dev = &mhi_dev->dev;
>>> + qdev->ep.xmit = qcom_mhi_qrtr_send;
>>> + atomic_set(&qdev->in_reset, 0);
>>> +
>>> + net_id = QRTR_EP_NID_AUTO;
>>
>> Just pass QRTR_EP_NID_AUTO directly in the function call below.
>>
>
> Okay.
>
> Thanks,
> Mani
>
>> Regards,
>> Bjorn
>>
>>> +
>>> + INIT_LIST_HEAD(&qdev->ul_pkts);
>>> + spin_lock_init(&qdev->ul_lock);
>>> +
>>> + dev_set_drvdata(&mhi_dev->dev, qdev);
>>> + rc = qrtr_endpoint_register(&qdev->ep, net_id);
>>> + if (rc)
>>> + return rc;
>>> +
>>> + dev_dbg(qdev->dev, "Qualcomm MHI QRTR driver probed\n");
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void qcom_mhi_qrtr_remove(struct mhi_device *mhi_dev)
>>> +{
>>> + struct qrtr_mhi_dev *qdev = dev_get_drvdata(&mhi_dev->dev);
>>> +
>>> + qrtr_endpoint_unregister(&qdev->ep);
>>> + dev_set_drvdata(&mhi_dev->dev, NULL);
>>> +}
>>> +
>>> +static const struct mhi_device_id qcom_mhi_qrtr_id_table[] = {
>>> + { .chan = "IPCR" },
>>> + {}
>>> +};
>>> +MODULE_DEVICE_TABLE(mhi, qcom_mhi_qrtr_id_table);
>>> +
>>> +static struct mhi_driver qcom_mhi_qrtr_driver = {
>>> + .probe = qcom_mhi_qrtr_probe,
>>> + .remove = qcom_mhi_qrtr_remove,
>>> + .dl_xfer_cb = qcom_mhi_qrtr_dl_callback,
>>> + .ul_xfer_cb = qcom_mhi_qrtr_ul_callback,
>>> + .status_cb = qcom_mhi_qrtr_status_callback,
>>> + .id_table = qcom_mhi_qrtr_id_table,
>>> + .driver = {
>>> + .name = "qcom_mhi_qrtr",
>>> + },
>>> +};
>>> +
>>> +module_mhi_driver(qcom_mhi_qrtr_driver);
>>> +
>>> +MODULE_DESCRIPTION("Qualcomm IPC-Router MHI interface driver");
>>> +MODULE_LICENSE("GPL v2");
>>> --
>>> 2.17.1
>>>

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project

\
 
 \ /
  Last update: 2020-03-26 23:58    [W:0.136 / U:2.588 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site