From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Chris Lew <clew@codeaurora.org>,
gregkh@linuxfoundation.org, davem@davemloft.net,
smohanad@codeaurora.org, jhugo@codeaurora.org,
kvalo@codeaurora.org, hemantk@codeaurora.org,
linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
netdev@vger.kernel.org
Subject: Re: [PATCH v3 6/7] net: qrtr: Add MHI transport layer
Date: Tue, 31 Mar 2020 16:53:26 +0530 [thread overview]
Message-ID: <20200331112326.GB21688@Mani-XPS-13-9360> (raw)
In-Reply-To: <20200330221932.GB215915@minitux>
Hi Bjorn,
On Mon, Mar 30, 2020 at 03:19:32PM -0700, Bjorn Andersson wrote:
> On Mon 30 Mar 02:49 PDT 2020, Manivannan Sadhasivam wrote:
>
> > Hi Chris,
> >
> > On Thu, Mar 26, 2020 at 03:54:42PM -0700, Chris Lew wrote:
> > >
> > >
> > > 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:
> [..]
> > > > > > + 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.
> > >
> >
> > So since we are not getting status_cb for fatal errors, I think we should just
> > remove status_cb, in_reset and timeout code.
> >
>
> Looks reasonable.
>
> [..]
> > > 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.
> > >
> >
> > You mean we can just remove the timeout handling part and return after
> > kref_put()?
> >
>
> If all messages are "generated" by qcom_mhi_qrtr_send() and "released"
> in qcom_mhi_qrtr_ul_callback() I don't think you need the refcounting at
> all.
>
Hmm, you're right. We can move the packet releasing part to ul_callback now.
>
> Presumably though, it would have been nice to not have to carry a
> separate list of packets (and hope that it's in sync with the mhi core)
> and instead have the ul callback somehow allow us to derive the skb to
> be freed.
>
Yep, MHI stack holds the skb in buf_addr member of mhi_result. So, we can just
use below to get the skb in ul_callback:
struct sk_buff *skb = (struct sk_buff *)mhi_res->buf_addr;
This will help us to avoid the use of pkt, ul_pkts list and use the skb directly
everywhere. At the same time I think we can also remove the ul_lock which
was added to protect the ul_pkts list.
Let me know your opinion, I'll just send a series with this modified QRTR MHI
client driver and MHI suspend/resume patches.
Thanks,
Mani
> Regards,
> Bjorn
next prev parent reply other threads:[~2020-03-31 11:23 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-24 6:10 [PATCH v3 0/7] Improvements to MHI Bus Manivannan Sadhasivam
2020-03-24 6:10 ` [PATCH v3 1/7] bus: mhi: core: Pass module owner during client driver registration Manivannan Sadhasivam
2020-03-24 6:10 ` [PATCH v3 2/7] bus: mhi: core: Add support for reading MHI info from device Manivannan Sadhasivam
2020-03-24 14:38 ` Jeffrey Hugo
2020-03-25 19:04 ` Jeffrey Hugo
2020-03-24 6:10 ` [PATCH v3 3/7] bus: mhi: core: Initialize bhie field in mhi_cntrl for RDDM capture Manivannan Sadhasivam
2020-03-25 16:01 ` Jeffrey Hugo
2020-03-24 6:10 ` [PATCH v3 4/7] bus: mhi: core: Drop the references to mhi_dev in mhi_destroy_device() Manivannan Sadhasivam
2020-03-25 16:03 ` Jeffrey Hugo
2020-03-24 6:10 ` [PATCH v3 5/7] bus: mhi: core: Add support for MHI suspend and resume Manivannan Sadhasivam
2020-03-24 6:10 ` [PATCH v3 6/7] net: qrtr: Add MHI transport layer Manivannan Sadhasivam
2020-03-24 20:39 ` Bjorn Andersson
2020-03-25 10:37 ` Manivannan Sadhasivam
2020-03-26 22:54 ` Chris Lew
2020-03-30 9:49 ` Manivannan Sadhasivam
2020-03-30 22:19 ` Bjorn Andersson
2020-03-31 11:23 ` Manivannan Sadhasivam [this message]
2020-03-31 17:40 ` Bjorn Andersson
2020-03-24 6:10 ` [PATCH v3 7/7] net: qrtr: Do not depend on ARCH_QCOM Manivannan Sadhasivam
2020-03-24 20:40 ` Bjorn Andersson
2020-03-26 14:51 ` [PATCH v3 0/7] Improvements to MHI Bus Greg KH
2020-03-26 17:25 ` Manivannan Sadhasivam
2020-03-26 17:42 ` Greg KH
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200331112326.GB21688@Mani-XPS-13-9360 \
--to=manivannan.sadhasivam@linaro.org \
--cc=bjorn.andersson@linaro.org \
--cc=clew@codeaurora.org \
--cc=davem@davemloft.net \
--cc=gregkh@linuxfoundation.org \
--cc=hemantk@codeaurora.org \
--cc=jhugo@codeaurora.org \
--cc=kvalo@codeaurora.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=smohanad@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).