From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from z5.mailgun.us ([104.130.96.5]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kfpCY-00030k-Lj for ath11k@lists.infradead.org; Thu, 19 Nov 2020 19:02:39 +0000 MIME-Version: 1.0 Date: Thu, 19 Nov 2020 11:02:35 -0800 From: Bhaumik Bhatt Subject: Re: [PATCH] net: qrtr: Unprepare MHI channels during remove In-Reply-To: <2019fe3c-55c5-61fe-758c-1e9952e1cb33@codeaurora.org> References: <1605723625-11206-1-git-send-email-bbhatt@codeaurora.org> <5e94c0be-9402-7309-5d65-857a27d1f491@codeaurora.org> <2019fe3c-55c5-61fe-758c-1e9952e1cb33@codeaurora.org> Message-ID: <647d1520d0bcefa7ff02d2ef5ee81bd1@codeaurora.org> List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: bbhatt@codeaurora.org Content-Transfer-Encoding: base64 Content-Type: text/plain; charset="utf-8"; Format="flowed" Sender: "ath11k" Errors-To: ath11k-bounces+kvalo=adurom.com@lists.infradead.org To: Jeffrey Hugo Cc: Loic Poulain , linux-arm-msm@vger.kernel.org, cjhuang@codeaurora.org, linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org, clew@codeaurora.org, netdev@vger.kernel.org, manivannan.sadhasivam@linaro.org, hemantk@codeaurora.org, jhugo=codeaurora.org@codeaurora.org, ath11k@lists.infradead.org, kvalo@codeaurora.org T24gMjAyMC0xMS0xOCAxMTozNCBBTSwgSmVmZnJleSBIdWdvIHdyb3RlOgo+IE9uIDExLzE4LzIw MjAgMTI6MTQgUE0sIExvaWMgUG91bGFpbiB3cm90ZToKPj4gCj4+IAo+PiBMZcKgbWVyLiAxOCBu b3YuIDIwMjAgw6AgMTk6MzQsIEplZmZyZXkgSHVnbyA8amh1Z29AY29kZWF1cm9yYS5vcmcgCj4+ IDxtYWlsdG86amh1Z29AY29kZWF1cm9yYS5vcmc+PiBhIMOpY3JpdMKgOgo+PiAKPj4gICAgIE9u IDExLzE4LzIwMjAgMTE6MjAgQU0sIEJoYXVtaWsgQmhhdHQgd3JvdGU6Cj4+ICAgICAgPiBSZXNl dCBNSEkgZGV2aWNlIGNoYW5uZWxzIHdoZW4gZHJpdmVyIHJlbW92ZSBpcyBjYWxsZWQgZHVlIHRv Cj4+ICAgICAgPiBtb2R1bGUgdW5sb2FkIG9yIGFueSBjcmFzaCBzY2VuYXJpby4gVGhpcyB3aWxs IG1ha2Ugc3VyZSB0aGF0Cj4+ICAgICAgPiBNSEkgY2hhbm5lbHMgbm8gbG9uZ2VyIHJlbWFpbiBl bmFibGVkIGZvciB0cmFuc2ZlcnMgc2luY2UgdGhlCj4+ICAgICAgPiBNSEkgc3RhY2sgZG9lcyBu b3QgdGFrZSBjYXJlIG9mIHRoaXMgYW55bW9yZSBhZnRlciB0aGUgCj4+IGF1dG8tc3RhcnQKPj4g ICAgICA+IGNoYW5uZWxzIGZlYXR1cmUgd2FzIHJlbW92ZWQuCj4+ICAgICAgPgo+PiAgICAgID4g U2lnbmVkLW9mZi1ieTogQmhhdW1payBCaGF0dCA8YmJoYXR0QGNvZGVhdXJvcmEub3JnCj4+ICAg ICA8bWFpbHRvOmJiaGF0dEBjb2RlYXVyb3JhLm9yZz4+Cj4+ICAgICAgPiAtLS0KPj4gICAgICA+ wqAgwqBuZXQvcXJ0ci9taGkuYyB8IDEgKwo+PiAgICAgID7CoCDCoDEgZmlsZSBjaGFuZ2VkLCAx IGluc2VydGlvbigrKQo+PiAgICAgID4KPj4gICAgICA+IGRpZmYgLS1naXQgYS9uZXQvcXJ0ci9t aGkuYyBiL25ldC9xcnRyL21oaS5jCj4+ICAgICAgPiBpbmRleCA3MTAwZjBiLi4yYmYyYjE5IDEw MDY0NAo+PiAgICAgID4gLS0tIGEvbmV0L3FydHIvbWhpLmMKPj4gICAgICA+ICsrKyBiL25ldC9x cnRyL21oaS5jCj4+ICAgICAgPiBAQCAtMTA0LDYgKzEwNCw3IEBAIHN0YXRpYyB2b2lkIHFjb21f bWhpX3FydHJfcmVtb3ZlKHN0cnVjdAo+PiAgICAgbWhpX2RldmljZSAqbWhpX2RldikKPj4gICAg ICA+wqAgwqAgwqAgwqBzdHJ1Y3QgcXJ0cl9taGlfZGV2ICpxZGV2ID0gCj4+IGRldl9nZXRfZHJ2 ZGF0YSgmbWhpX2Rldi0+ZGV2KTsKPj4gICAgICA+Cj4+ICAgICAgPsKgIMKgIMKgIMKgcXJ0cl9l bmRwb2ludF91bnJlZ2lzdGVyKCZxZGV2LT5lcCk7Cj4+ICAgICAgPiArwqAgwqAgwqBtaGlfdW5w cmVwYXJlX2Zyb21fdHJhbnNmZXIobWhpX2Rldik7Cj4+ICAgICAgPsKgIMKgIMKgIMKgZGV2X3Nl dF9kcnZkYXRhKCZtaGlfZGV2LT5kZXYsIE5VTEwpOwo+PiAgICAgID7CoCDCoH0KPj4gICAgICA+ Cj4+ICAgICAgPgo+PiAKPj4gICAgIEkgYWRtaXQsIEkgZGlkbid0IHBheSBtdWNoIGF0dGVudGlv biB0byB0aGUgYXV0by1zdGFydCBiZWluZyAKPj4gcmVtb3ZlZCwKPj4gICAgIGJ1dCB0aGlzIHNl ZW1zIG9kZCB0byBtZS4KPj4gCj4+ICAgICBBcyBhIGNsaWVudCwgdGhlIE1ISSBkZXZpY2UgaXMg YmVpbmcgcmVtb3ZlZCwgbGlrZWx5IGJlY2F1c2Ugb2YgCj4+IHNvbWUKPj4gICAgIGZhY3RvciBv dXRzaWRlIG9mIG15IGNvbnRyb2wsIGJ1dCBJIHN0aWxsIG5lZWQgdG8gY2xlYW4gaXQgdXA/wqAg Cj4+IFRoaXMKPj4gICAgIHJlYWxseSBmZWVscyBsaWtlIHNvbWV0aGluZyBNSEkgc2hvdWxkIGJl IGhhbmRsaW5nLgo+PiAKPj4gCj4+IEkgdGhpbmsgdGhpcyBpcyBqdXN0IGFib3V0IGJhbGFuY2lu ZyBvcGVyYXRpb25zLCB3aGF0IGlzIGRvbmUgaW4gcHJvYmUgCj4+IHNob3VsZCBiZSB1bmRvbmUg aW4gcmVtb3ZlLCBzbyBoZXJlIGNoYW5uZWxzIGFyZSBzdGFydGVkIGluIHByb2JlIGFuZCAKPj4g c3RvcHBlZC9yZXNldCBpbiByZW1vdmUuCj4gCj4gSSB1bmRlcnN0YW5kIHRoYXQgcGVyc3BlY3Rp dmUsIGJ1dCB0aGF0IGRvZXNuJ3QgcXVpdGUgbWF0Y2ggd2hhdCBpcwo+IGdvaW5nIG9uIGhlcmUu ICBSZWdhcmRsZXNzIG9mIGlmIHRoZSBjaGFubmVsIHdhcyBzdGFydGVkIChwcmVwYXJlZCkgaW4K PiBwcm9iZSwgaXQgbm93IG5lZWRzIHRvIGJlIHN0b3BwZWQgaW4gcmVtb3ZlLiAgVGhhdCBub3Qg YmFsYW5jZWQgaW4gYWxsCj4gY2FzZXMKPiAKPiBMZXRzIGFzc3VtZSwgaW4gcmVzcG9uc2UgdG8g cHJvYmUoKSwgbXkgY2xpZW50IGRyaXZlciBnb2VzIGFuZCBjcmVhdGVzCj4gc29tZSBvdGhlciBv YmplY3QsIG1heWJlIGEgc29ja2V0LiAgSW4gcmVzcG9uc2UgdG8gdGhhdCBzb2NrZXQgYmVpbmcK PiBvcGVuZWQvYWN0aXZhdGVkIGJ5IHRoZSBjbGllbnQgb2YgbXkgZHJpdmVyLCBJIGdvIGFuZCBz dGFydCB0aGUgbWhpCj4gY2hhbm5lbC4gIE5vdywgbm9ybWFsbHksIHdoZW4gdGhlIHNvY2tldCBp cyBjbG9zZWQvZGVhY3RpdmF0ZWQsIEkgc3RvcAo+IHRoZSBNSEkgY2hhbm5lbC4gIEluIHRoaXMg Y2FzZSwgc3RvcHBpbmcgdGhlIE1ISSBjaGFubmVsIGluIHJlbW92ZSgpCj4gaXMgdW5iYWxhbmNl ZCB3aXRoIHJlc3BlY3QgdG8gcHJvYmUoKSwgYnV0IGlzIG5vdyBhIHJlcXVpcmVtZW50Lgo+IAo+ IE5vdyB5b3UgbWF5IGFyZ3VlLCBJIHNob3VsZCBjbG9zZSB0aGUgb2JqZWN0IGluIHJlc3BvbnNl IHRvIHJlbW92ZSwKPiB3aGljaCB3aWxsIHRoZW4gdHJpZ2dlciB0aGUgc3RvcCBvbiB0aGUgY2hh bm5lbC4gIFRoYXQgZG9lc24ndCBhcHBseQo+IHRvIGV2ZXJ5dGhpbmcuICBGb3IgZXhhbXBsZSwg eW91IGNhbm5vdCBjbG9zZSBhbiBvcGVuIGZpbGUgaW4gdGhlCj4ga2VybmVsLiBZb3UgbmVlZCB0 byB3YWl0IGZvciB1c2Vyc3BhY2UgdG8gY2xvc2UgaXQuICBCeSB0aGUgdGltZSB0aGF0Cj4gaGFw cGVucywgdGhlIG1oaV9kZXYgaXMgbG9uZyBnb25lIEkgZXhwZWN0Lgo+IAo+IFNvIGlmLCBzb21l aG93LCB0aGUgY2xpZW50IGRyaXZlciBpcyB0aGUgb25lIGNhdXNpbmcgdGhlIHJlbW92ZSB0bwo+ IG9jY3VyLCB0aGVuIHllcyBpdCBzaG91bGQgcHJvYmFibHkgYmUgdGhlIG9uZSBkb2luZyB0aGUg c3RvcCwgYnV0Cj4gdGhhdCdzIGEgbmFycm93IHNldCBvZiBjb25kaXRpb25zLCBhbmQgSSB0aGlu ayBoYXZpbmcgdGhhdCByZXF1aXJlbWVudAo+IGZvciBhbGwgc2NlbmFyaW9zIGlzIGxpbWl0aW5n LgpJdCBzaG91bGQgYmUgdGhlIGNsaWVudCdzIHJlc3BvbnNpYmlsaXR5IHRvIHBlcmZvcm0gYSBj bGVhbi11cCB0aG91Z2guCgpXZSBjYW5ub3QgYXNzdW1lIHRoYXQgdGhlIHJlbW92ZSgpIGNhbGwg d2FzIGR1ZSB0byBmYWN0b3JzIG91dHNpZGUgb2YgCnRoZQpjbGllbnQncyBjb250cm9sIGF0IGFs bCB0aW1lcy4gWW91IG1heSBub3Qga25vdyBpZiB0aGUgcmVtb3ZlKCkgd2FzIGR1ZSAKdG8KZGV2 aWNlIGFjdHVhbGx5IGNyYXNoaW5nIG9yIGp1c3QgYW4gdW5iaW5kL21vZHVsZSB1bmxvYWQuIFNv LCBpdCB3b3VsZCAKYmUKYmV0dGVyIGlmIHlvdSBjYWxsIGl0IGFzIHRoZSBkZXZpY2Ugc2hvdWxk IGlkZWFsbHkgbm90IGJlIGxlZnQgd2l0aCBhIApzdGFsZQpjaGFubmVsIGNvbnRleHQuCgpXZSBo YWQgYW4gaXNzdWUgd2hlcmUgYSBjbGllbnQgd2FzIGlzc3VpbmcgYSBkcml2ZXIgdW5iaW5kIHdp dGhvdXQgCnVucHJlcGFyaW5nCnRoZSBNSEkgY2hhbm5lbHMgYW5kIHdpdGhvdXQgTG9pYydzIHBh dGNoIFsxXSwgd2Ugd291bGQgbm90IGlzc3VlIGEgCmNoYW5uZWwKUkVTRVQgdG8gdGhlIGRldmlj ZSByZXN1bHRpbmcgaW4gaW5jb21pbmcgZGF0YSB0byB0aGUgaG9zdCBvbiB0aG9zZSAKY2hhbm5l bHMKYWZ0ZXIgaG9zdCBjbGVhbi11cCBhbmQgYW4gdW5tYXBwZWQgbWVtb3J5IGFjY2VzcyBhbmQg a2VybmVsIHBhbmljLgoKSWYgTUhJIGRldiB3aWxsIGJlIGdvbmUgdGhhdCBOVUxML3N0YXR1cyBj aGVjayBtdXN0IGJlIHByZXNlbnQgaW4gCnNvbWV0aGluZyB0aGF0CnVzZXJzcGFjZSBjb3VsZCBw b3RlbnRpYWxseSB1c2UuCgpbMV0gCmh0dHBzOi8vZ2l0Lmtlcm5lbC5vcmcvcHViL3NjbS9saW51 eC9rZXJuZWwvZ2l0L25leHQvbGludXgtbmV4dC5naXQvY29tbWl0L2RyaXZlcnMvYnVzL21oaT9o PW5leHQtMjAyMDExMTkmaWQ9YTdmNDIyZjJmODllN2Q0OGFhNjZlNjQ4ODQ0NGE0YzdmMDEyNjlk NQoKVGhhbmtzLApCaGF1bWlrCi0tLQpUaGUgUXVhbGNvbW0gSW5ub3ZhdGlvbiBDZW50ZXIsIElu Yy4gaXMgYSBtZW1iZXIgb2YgdGhlIENvZGUgQXVyb3JhIApGb3J1bSwKYSBMaW51eCBGb3VuZGF0 aW9uIENvbGxhYm9yYXRpdmUgUHJvamVjdAoKLS0gCmF0aDExayBtYWlsaW5nIGxpc3QKYXRoMTFr QGxpc3RzLmluZnJhZGVhZC5vcmcKaHR0cDovL2xpc3RzLmluZnJhZGVhZC5vcmcvbWFpbG1hbi9s aXN0aW5mby9hdGgxMWsK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-18.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D1875C56201 for ; Thu, 19 Nov 2020 19:03:00 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 696992220B for ; Thu, 19 Nov 2020 19:03:00 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=mg.codeaurora.org header.i=@mg.codeaurora.org header.b="p3bt++Xy" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727192AbgKSTCj (ORCPT ); Thu, 19 Nov 2020 14:02:39 -0500 Received: from m42-4.mailgun.net ([69.72.42.4]:63058 "EHLO m42-4.mailgun.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727197AbgKSTCi (ORCPT ); Thu, 19 Nov 2020 14:02:38 -0500 DKIM-Signature: a=rsa-sha256; v=1; c=relaxed/relaxed; d=mg.codeaurora.org; q=dns/txt; s=smtp; t=1605812557; h=Message-ID: References: In-Reply-To: Reply-To: Subject: Cc: To: From: Date: Content-Transfer-Encoding: Content-Type: MIME-Version: Sender; bh=QM3CLPvjkfi6YERoSbKWzDa4BPjcQSDZ5ZJSSMwwfYw=; b=p3bt++XyKz71hb7chrxZ08QOj9dt6dcyJ4t6hhiZUMRSfOTvdxu7KLq98uzpnu4K7LnDPlTw PHC4CYTQRMqDpKVp2u4PVEytHvatSsVmdxyFQpor1FfqWf2WNMJqsJ9q9ANI9WTL0lFI1xXt 0yDuhOut5ElbmFHYVWXEUkucLIs= X-Mailgun-Sending-Ip: 69.72.42.4 X-Mailgun-Sid: WyI1MzIzYiIsICJsaW51eC1hcm0tbXNtQHZnZXIua2VybmVsLm9yZyIsICJiZTllNGEiXQ== Received: from smtp.codeaurora.org (ec2-35-166-182-171.us-west-2.compute.amazonaws.com [35.166.182.171]) by smtp-out-n07.prod.us-east-1.postgun.com with SMTP id 5fb6c14cfa67d9becf71a9f1 (version=TLS1.2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256); Thu, 19 Nov 2020 19:02:36 GMT Sender: bbhatt=codeaurora.org@mg.codeaurora.org Received: by smtp.codeaurora.org (Postfix, from userid 1001) id 29221C43463; Thu, 19 Nov 2020 19:02:36 +0000 (UTC) Received: from mail.codeaurora.org (localhost.localdomain [127.0.0.1]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) (Authenticated sender: bbhatt) by smtp.codeaurora.org (Postfix) with ESMTPSA id 2AC29C433C6; Thu, 19 Nov 2020 19:02:35 +0000 (UTC) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Date: Thu, 19 Nov 2020 11:02:35 -0800 From: Bhaumik Bhatt To: Jeffrey Hugo Cc: Loic Poulain , ath11k@lists.infradead.org, cjhuang@codeaurora.org, clew@codeaurora.org, hemantk@codeaurora.org, kvalo@codeaurora.org, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-wireless@vger.kernel.org, manivannan.sadhasivam@linaro.org, netdev@vger.kernel.org, jhugo=codeaurora.org@codeaurora.org Subject: Re: [PATCH] net: qrtr: Unprepare MHI channels during remove Organization: Qualcomm Innovation Center, Inc. Reply-To: bbhatt@codeaurora.org Mail-Reply-To: bbhatt@codeaurora.org In-Reply-To: <2019fe3c-55c5-61fe-758c-1e9952e1cb33@codeaurora.org> References: <1605723625-11206-1-git-send-email-bbhatt@codeaurora.org> <5e94c0be-9402-7309-5d65-857a27d1f491@codeaurora.org> <2019fe3c-55c5-61fe-758c-1e9952e1cb33@codeaurora.org> Message-ID: <647d1520d0bcefa7ff02d2ef5ee81bd1@codeaurora.org> X-Sender: bbhatt@codeaurora.org User-Agent: Roundcube Webmail/1.3.9 Precedence: bulk List-ID: X-Mailing-List: linux-arm-msm@vger.kernel.org On 2020-11-18 11:34 AM, Jeffrey Hugo wrote: > On 11/18/2020 12:14 PM, Loic Poulain wrote: >> >> >> Le mer. 18 nov. 2020 à 19:34, Jeffrey Hugo > > a écrit : >> >> On 11/18/2020 11:20 AM, Bhaumik Bhatt wrote: >> > Reset MHI device channels when driver remove is called due to >> > module unload or any crash scenario. This will make sure that >> > MHI channels no longer remain enabled for transfers since the >> > MHI stack does not take care of this anymore after the >> auto-start >> > channels feature was removed. >> > >> > Signed-off-by: Bhaumik Bhatt > > >> > --- >> >   net/qrtr/mhi.c | 1 + >> >   1 file changed, 1 insertion(+) >> > >> > diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c >> > index 7100f0b..2bf2b19 100644 >> > --- a/net/qrtr/mhi.c >> > +++ b/net/qrtr/mhi.c >> > @@ -104,6 +104,7 @@ 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); >> > +     mhi_unprepare_from_transfer(mhi_dev); >> >       dev_set_drvdata(&mhi_dev->dev, NULL); >> >   } >> > >> > >> >> I admit, I didn't pay much attention to the auto-start being >> removed, >> but this seems odd to me. >> >> As a client, the MHI device is being removed, likely because of >> some >> factor outside of my control, but I still need to clean it up?  >> This >> really feels like something MHI should be handling. >> >> >> I think this is just about balancing operations, what is done in probe >> should be undone in remove, so here channels are started in probe and >> stopped/reset in remove. > > I understand that perspective, but that doesn't quite match what is > going on here. Regardless of if the channel was started (prepared) in > probe, it now needs to be stopped in remove. That not balanced in all > cases > > Lets assume, in response to probe(), my client driver goes and creates > some other object, maybe a socket. In response to that socket being > opened/activated by the client of my driver, I go and start the mhi > channel. Now, normally, when the socket is closed/deactivated, I stop > the MHI channel. In this case, stopping the MHI channel in remove() > is unbalanced with respect to probe(), but is now a requirement. > > Now you may argue, I should close the object in response to remove, > which will then trigger the stop on the channel. That doesn't apply > to everything. For example, you cannot close an open file in the > kernel. You need to wait for userspace to close it. By the time that > happens, the mhi_dev is long gone I expect. > > So if, somehow, the client driver is the one causing the remove to > occur, then yes it should probably be the one doing the stop, but > that's a narrow set of conditions, and I think having that requirement > for all scenarios is limiting. It should be the client's responsibility to perform a clean-up though. We cannot assume that the remove() call was due to factors outside of the client's control at all times. You may not know if the remove() was due to device actually crashing or just an unbind/module unload. So, it would be better if you call it as the device should ideally not be left with a stale channel context. We had an issue where a client was issuing a driver unbind without unpreparing the MHI channels and without Loic's patch [1], we would not issue a channel RESET to the device resulting in incoming data to the host on those channels after host clean-up and an unmapped memory access and kernel panic. If MHI dev will be gone that NULL/status check must be present in something that userspace could potentially use. [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/bus/mhi?h=next-20201119&id=a7f422f2f89e7d48aa66e6488444a4c7f01269d5 Thanks, Bhaumik --- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project