| Message ID | 20200131135009.31477-15-manivannan.sadhasivam@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add MHI bus support | expand |
On Fri, 31 Jan 2020 19:20:07 +0530, Manivannan Sadhasivam wrote: > +/* 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); > + > + kref_put(&pkt->refcount, qrtr_mhi_pkt_release); Which kref_get() does this pair with? Looks like qcom_mhi_qrtr_send() will release a reference after completion, too. > + spin_unlock_irqrestore(&qdev->ul_lock, flags); > +}
Hi Jakub, On Mon, Feb 03, 2020 at 10:12:25AM -0800, Jakub Kicinski wrote: > On Fri, 31 Jan 2020 19:20:07 +0530, Manivannan Sadhasivam wrote: > > +/* 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); > > + > > + kref_put(&pkt->refcount, qrtr_mhi_pkt_release); > > Which kref_get() does this pair with? > > Looks like qcom_mhi_qrtr_send() will release a reference after > completion, too. > Yikes, there is some issue here... Acutally the issue is not in what you referred above but the overall kref handling itself. Please see below. kref_put() should be present in qcom_mhi_qrtr_ul_callback() as it will decrement the refcount which got incremented in qcom_mhi_qrtr_send(). It should be noted that kref_init() will fix the refcount to 1 and kref_get() will increment to 2. So for properly releasing the refcount to 0, we need to call kref_put() twice. So if all goes well, the refcount will get decremented twice in qcom_mhi_qrtr_ul_callback() as well as in qcom_mhi_qrtr_send() and we are good. But, if the transfer has failed ie., when qcom_mhi_qrtr_ul_callback() doesn't get called, then we are leaking the refcount. I need to rework the kref handling code in next iteration. Thanks for triggering this! Regards, Mani > > + spin_unlock_irqrestore(&qdev->ul_lock, flags); > > +}
On 2/4/2020 12:19 AM, Manivannan Sadhasivam wrote: > Hi Jakub, > > On Mon, Feb 03, 2020 at 10:12:25AM -0800, Jakub Kicinski wrote: >> On Fri, 31 Jan 2020 19:20:07 +0530, Manivannan Sadhasivam wrote: >>> +/* 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); >>> + >>> + kref_put(&pkt->refcount, qrtr_mhi_pkt_release); >> Which kref_get() does this pair with? >> >> Looks like qcom_mhi_qrtr_send() will release a reference after >> completion, too. >> > Yikes, there is some issue here... > > Acutally the issue is not in what you referred above but the overall kref > handling itself. Please see below. > > kref_put() should be present in qcom_mhi_qrtr_ul_callback() as it will > decrement the refcount which got incremented in qcom_mhi_qrtr_send(). It > should be noted that kref_init() will fix the refcount to 1 and kref_get() will > increment to 2. So for properly releasing the refcount to 0, we need to call > kref_put() twice. > > So if all goes well, the refcount will get decremented twice in > qcom_mhi_qrtr_ul_callback() as well as in qcom_mhi_qrtr_send() and we are good. > > But, if the transfer has failed ie., when qcom_mhi_qrtr_ul_callback() doesn't > get called, then we are leaking the refcount. I need to rework the kref handling > code in next iteration. > > Thanks for triggering this! > > Regards, > Mani > >>> + spin_unlock_irqrestore(&qdev->ul_lock, flags); >>> +} Hi Mani, I'm not sure if this was changed in your patches but MHI is supposed to give a ul_callback() for any packet that is successfully queued. In the case of the transfer failing, the ul_callback() should still be called so there should be no refcount leaking. It is an essential assumption I made, if that no longer holds true then the entire driver needs to be reworked. Thanks, Chris
On 2/10/2020 7:50 PM, Manivannan Sadhasivam wrote: > Hi Chris, > > On Thu, Feb 06, 2020 at 04:14:19PM -0800, Chris Lew wrote: >> On 2/4/2020 12:19 AM, Manivannan Sadhasivam wrote: >>> Hi Jakub, >>> >>> On Mon, Feb 03, 2020 at 10:12:25AM -0800, Jakub Kicinski wrote: >>>> On Fri, 31 Jan 2020 19:20:07 +0530, Manivannan Sadhasivam wrote: >>>>> +/* 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); >>>>> + >>>>> + kref_put(&pkt->refcount, qrtr_mhi_pkt_release); >>>> Which kref_get() does this pair with? >>>> >>>> Looks like qcom_mhi_qrtr_send() will release a reference after >>>> completion, too. >>>> >>> Yikes, there is some issue here... >>> >>> Acutally the issue is not in what you referred above but the overall kref >>> handling itself. Please see below. >>> >>> kref_put() should be present in qcom_mhi_qrtr_ul_callback() as it will >>> decrement the refcount which got incremented in qcom_mhi_qrtr_send(). It >>> should be noted that kref_init() will fix the refcount to 1 and kref_get() will >>> increment to 2. So for properly releasing the refcount to 0, we need to call >>> kref_put() twice. >>> >>> So if all goes well, the refcount will get decremented twice in >>> qcom_mhi_qrtr_ul_callback() as well as in qcom_mhi_qrtr_send() and we are good. >>> >>> But, if the transfer has failed ie., when qcom_mhi_qrtr_ul_callback() doesn't >>> get called, then we are leaking the refcount. I need to rework the kref handling >>> code in next iteration. >>> >>> Thanks for triggering this! >>> >>> Regards, >>> Mani >>> >>>>> + spin_unlock_irqrestore(&qdev->ul_lock, flags); >>>>> +} >> Hi Mani, >> >> I'm not sure if this was changed in your patches but MHI is supposed to give a >> ul_callback() for any packet that is successfully queued. In the case of the >> transfer failing, the ul_callback() should still be called so there should >> be no refcount leaking. It is an essential assumption I made, if that no longer >> holds true then the entire driver needs to be reworked. >> > Your assumption is correct. Only when the packet gets queued into the transfer > ring, the ul_xfer_cb will be called irrespective of the transfer state (success > or failure). But when the mhi_queue_transfer() returns even before queuing any > packet, then we need to decrease the refcount in the error path. > > Please correct me if I'm wrong. The error path for mhi_queue_transfer directly frees the packet structure since no other context has a reference to those structs. If you wanted to clean it up and converge using kref release to free, I think that would work. There are some things you'll have to re-arrange like at what point the packet is added to the ul pkts list. > Thanks, > Mani > >> Thanks, >> Chris >> >> -- >> >> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
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..c85041a22f85 --- /dev/null +++ b/net/qrtr/mhi.c @@ -0,0 +1,207 @@ +// 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"); +} + +/* 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); + + 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); + spin_lock_irqsave(&qdev->ul_lock, flags); + list_for_each_entry(pkt, &qdev->ul_pkts, node) + complete_all(&pkt->done); + 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_transfer(qdev->mhi_dev, DMA_TO_DEVICE, skb, skb->len, + MHI_EOT); + if (rc) { + list_del(&pkt->node); + kfree_skb(skb); + kfree(pkt); + 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; + 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; + + 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_driver(qcom_mhi_qrtr_driver, mhi_driver_register, + mhi_driver_unregister); + +MODULE_DESCRIPTION("Qualcomm IPC-Router MHI interface driver"); +MODULE_LICENSE("GPL v2");
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 | 207 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 216 insertions(+) create mode 100644 net/qrtr/mhi.c