From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail-bk0-f52.google.com ([209.85.214.52]:59640 "EHLO mail-bk0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751716Ab3GaFuj convert rfc822-to-8bit (ORCPT ); Wed, 31 Jul 2013 01:50:39 -0400 Received: by mail-bk0-f52.google.com with SMTP id e11so87273bkh.11 for ; Tue, 30 Jul 2013 22:50:37 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <87d2pzuc90.fsf@kamboji.qca.qualcomm.com> References: <1374129572-6079-1-git-send-email-michal.kazior@tieto.com> <87d2pzuc90.fsf@kamboji.qca.qualcomm.com> Date: Wed, 31 Jul 2013 07:50:37 +0200 Message-ID: (sfid-20130731_075043_235096_A6297197) Subject: Re: [PATCH] ath10k: move irq setup From: Michal Kazior To: Kalle Valo Cc: ath10k@lists.infradead.org, linux-wireless@vger.kernel.org Content-Type: text/plain; charset=UTF-8 Sender: linux-wireless-owner@vger.kernel.org List-ID: On 30 July 2013 20:35, Kalle Valo wrote: > Michal Kazior writes: > >> There was a slight race during PCI shutdown. Since >> interrupts weren't really stopped (only Copy >> Engine interrupts were disabled through device hw >> registers) it was possible for a firmware >> indication (crash) interrupt to come in after >> tasklets were synced/killed. This would cause >> memory corruption and a panic in most cases. It >> was also possible for interrupt to come before CE >> was initialized during device probing. >> >> Interrupts are required for BMI phase so they are enabled as soon as >> power_up() is called but are freed upon both power_down() and stop() >> so there's asymmetry here. As by design stop() cannot be followed by >> start() it is okay. Both power_down() and stop() should be merged >> later on to avoid confusion. > > Why are the interrupts freed both in power_down() and stop()? I don't > get that. > > What if we call disable_irq() in power_down() instead? power_down() must call free_irq(), because power_up() calls request_irq() (if you want the symmetry). If anything, the stop() should call disable_irq(), but wouldn't that mean start() should call enable_irq()? But than, irqs are needed before start().. I did think about disable_irq() but AFAIR you need to enable_irq() later on (so either way you need to keep track of the irq state or you'll get a ton of WARN_ONs from the system). I'll double check that and report back later >> Before this can be really properly fixed var/hw >> init code split is necessary. >> >> Signed-off-by: Michal Kazior >> --- >> >> Please note: this is based on my (still under >> review at the time of posting) previous patchests: >> device setup refactor and recovery. >> >> I'm posting this before those patchsets are merged >> so anyone interested in testing this fix (I can't >> reproduce the problem on my setup) can give it a >> try. > > This was reported by Ben, right? So this sould have a Reported-by line > attributing him. Yes. I'll fix that, provided we get through the review with the patch :) >> @@ -1783,16 +1792,24 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar) >> return 0; >> >> err_ce: >> + /* XXX: Until var/hw init is split it's impossible to fix the ordering >> + * here so we must call stop_intr() here too to prevent interrupts after >> + * CE is teared down. It's okay to double call the stop_intr() >> */ > > "FIXME:" Ok. >> exit: >> + ar_pci->intr_started = ret == 0; > > A bit too clever for the sake of readibility for my taste, but I guess > it's ok. > >> --- a/drivers/net/wireless/ath/ath10k/pci.h >> +++ b/drivers/net/wireless/ath/ath10k/pci.h >> @@ -198,6 +198,7 @@ struct ath10k_pci { >> * interrupts. >> */ >> int num_msi_intrs; >> + bool intr_started; > > Adding a new state variable makes me worried. I really would prefer a > solution which would not require that. I know that. That's why I mentioned in the commit log that it is more of a workaround than a real fix. Me, I don't like this either but a real fix requires a lot of rework from what I can tell. This bug can be triggered more easily now apparently after recovery patches went in. I'm not experiencing it but I get reports of rare panics when a machine is left idle for a very long time with interfaces brought down. > Also if we call request_irq() in ath10k_pci_probe() we should also call > free_irq() in ath10k_pci_remove() for symmetry. Just doing a temporary > hack will most likely stay forever :) With the patch interrupts are temporarily enabled&disabled for probe_fw() during pci_probe() and are then not requested until mac80211 start(). Pozdrawiam / Best regards, MichaƂ Kazior. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail-bk0-x232.google.com ([2a00:1450:4008:c01::232]) by merlin.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1V4PJP-0003TO-Q7 for ath10k@lists.infradead.org; Wed, 31 Jul 2013 05:51:04 +0000 Received: by mail-bk0-f50.google.com with SMTP id ik8so86681bkc.37 for ; Tue, 30 Jul 2013 22:50:37 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <87d2pzuc90.fsf@kamboji.qca.qualcomm.com> References: <1374129572-6079-1-git-send-email-michal.kazior@tieto.com> <87d2pzuc90.fsf@kamboji.qca.qualcomm.com> Date: Wed, 31 Jul 2013 07:50:37 +0200 Message-ID: Subject: Re: [PATCH] ath10k: move irq setup From: Michal Kazior List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Sender: "ath10k" Errors-To: ath10k-bounces+kvalo=adurom.com@lists.infradead.org To: Kalle Valo Cc: linux-wireless@vger.kernel.org, ath10k@lists.infradead.org T24gMzAgSnVseSAyMDEzIDIwOjM1LCBLYWxsZSBWYWxvIDxrdmFsb0BxY2EucXVhbGNvbW0uY29t PiB3cm90ZToKPiBNaWNoYWwgS2F6aW9yIDxtaWNoYWwua2F6aW9yQHRpZXRvLmNvbT4gd3JpdGVz Ogo+Cj4+IFRoZXJlIHdhcyBhIHNsaWdodCByYWNlIGR1cmluZyBQQ0kgc2h1dGRvd24uIFNpbmNl Cj4+IGludGVycnVwdHMgd2VyZW4ndCByZWFsbHkgc3RvcHBlZCAob25seSBDb3B5Cj4+IEVuZ2lu ZSBpbnRlcnJ1cHRzIHdlcmUgZGlzYWJsZWQgdGhyb3VnaCBkZXZpY2UgaHcKPj4gcmVnaXN0ZXJz KSBpdCB3YXMgcG9zc2libGUgZm9yIGEgZmlybXdhcmUKPj4gaW5kaWNhdGlvbiAoY3Jhc2gpIGlu dGVycnVwdCB0byBjb21lIGluIGFmdGVyCj4+IHRhc2tsZXRzIHdlcmUgc3luY2VkL2tpbGxlZC4g VGhpcyB3b3VsZCBjYXVzZQo+PiBtZW1vcnkgY29ycnVwdGlvbiBhbmQgYSBwYW5pYyBpbiBtb3N0 IGNhc2VzLiBJdAo+PiB3YXMgYWxzbyBwb3NzaWJsZSBmb3IgaW50ZXJydXB0IHRvIGNvbWUgYmVm b3JlIENFCj4+IHdhcyBpbml0aWFsaXplZCBkdXJpbmcgZGV2aWNlIHByb2JpbmcuCj4+Cj4+IElu dGVycnVwdHMgYXJlIHJlcXVpcmVkIGZvciBCTUkgcGhhc2Ugc28gdGhleSBhcmUgZW5hYmxlZCBh cyBzb29uIGFzCj4+IHBvd2VyX3VwKCkgaXMgY2FsbGVkIGJ1dCBhcmUgZnJlZWQgdXBvbiBib3Ro IHBvd2VyX2Rvd24oKSBhbmQgc3RvcCgpCj4+IHNvIHRoZXJlJ3MgYXN5bW1ldHJ5IGhlcmUuIEFz IGJ5IGRlc2lnbiBzdG9wKCkgY2Fubm90IGJlIGZvbGxvd2VkIGJ5Cj4+IHN0YXJ0KCkgaXQgaXMg b2theS4gQm90aCBwb3dlcl9kb3duKCkgYW5kIHN0b3AoKSBzaG91bGQgYmUgbWVyZ2VkCj4+IGxh dGVyIG9uIHRvIGF2b2lkIGNvbmZ1c2lvbi4KPgo+IFdoeSBhcmUgdGhlIGludGVycnVwdHMgZnJl ZWQgYm90aCBpbiBwb3dlcl9kb3duKCkgYW5kIHN0b3AoKT8gSSBkb24ndAo+IGdldCB0aGF0Lgo+ Cj4gV2hhdCBpZiB3ZSBjYWxsIGRpc2FibGVfaXJxKCkgaW4gcG93ZXJfZG93bigpIGluc3RlYWQ/ Cgpwb3dlcl9kb3duKCkgbXVzdCBjYWxsIGZyZWVfaXJxKCksIGJlY2F1c2UgcG93ZXJfdXAoKSBj YWxscwpyZXF1ZXN0X2lycSgpIChpZiB5b3Ugd2FudCB0aGUgc3ltbWV0cnkpLiBJZiBhbnl0aGlu ZywgdGhlIHN0b3AoKQpzaG91bGQgY2FsbCBkaXNhYmxlX2lycSgpLCBidXQgd291bGRuJ3QgdGhh dCBtZWFuIHN0YXJ0KCkgc2hvdWxkIGNhbGwKZW5hYmxlX2lycSgpPyBCdXQgdGhhbiwgaXJxcyBh cmUgbmVlZGVkIGJlZm9yZSBzdGFydCgpLi4KCkkgZGlkIHRoaW5rIGFib3V0IGRpc2FibGVfaXJx KCkgYnV0IEFGQUlSIHlvdSBuZWVkIHRvIGVuYWJsZV9pcnEoKQpsYXRlciBvbiAoc28gZWl0aGVy IHdheSB5b3UgbmVlZCB0byBrZWVwIHRyYWNrIG9mIHRoZSBpcnEgc3RhdGUgb3IKeW91J2xsIGdl dCBhIHRvbiBvZiBXQVJOX09OcyBmcm9tIHRoZSBzeXN0ZW0pLiBJJ2xsIGRvdWJsZSBjaGVjayB0 aGF0CmFuZCByZXBvcnQgYmFjayBsYXRlcgoKCj4+IEJlZm9yZSB0aGlzIGNhbiBiZSByZWFsbHkg cHJvcGVybHkgZml4ZWQgdmFyL2h3Cj4+IGluaXQgY29kZSBzcGxpdCBpcyBuZWNlc3NhcnkuCj4+ Cj4+IFNpZ25lZC1vZmYtYnk6IE1pY2hhbCBLYXppb3IgPG1pY2hhbC5rYXppb3JAdGlldG8uY29t Pgo+PiAtLS0KPj4KPj4gUGxlYXNlIG5vdGU6IHRoaXMgaXMgYmFzZWQgb24gbXkgKHN0aWxsIHVu ZGVyCj4+IHJldmlldyBhdCB0aGUgdGltZSBvZiBwb3N0aW5nKSBwcmV2aW91cyBwYXRjaGVzdHM6 Cj4+IGRldmljZSBzZXR1cCByZWZhY3RvciBhbmQgcmVjb3ZlcnkuCj4+Cj4+IEknbSBwb3N0aW5n IHRoaXMgYmVmb3JlIHRob3NlIHBhdGNoc2V0cyBhcmUgbWVyZ2VkCj4+IHNvIGFueW9uZSBpbnRl cmVzdGVkIGluIHRlc3RpbmcgdGhpcyBmaXggKEkgY2FuJ3QKPj4gcmVwcm9kdWNlIHRoZSBwcm9i bGVtIG9uIG15IHNldHVwKSBjYW4gZ2l2ZSBpdCBhCj4+IHRyeS4KPgo+IFRoaXMgd2FzIHJlcG9y dGVkIGJ5IEJlbiwgcmlnaHQ/IFNvIHRoaXMgc291bGQgaGF2ZSBhIFJlcG9ydGVkLWJ5IGxpbmUK PiBhdHRyaWJ1dGluZyBoaW0uCgpZZXMuIEknbGwgZml4IHRoYXQsIHByb3ZpZGVkIHdlIGdldCB0 aHJvdWdoIHRoZSByZXZpZXcgd2l0aCB0aGUgcGF0Y2ggOikKCgo+PiBAQCAtMTc4MywxNiArMTc5 MiwyNCBAQCBzdGF0aWMgaW50IGF0aDEwa19wY2lfaGlmX3Bvd2VyX3VwKHN0cnVjdCBhdGgxMGsg KmFyKQo+PiAgICAgICByZXR1cm4gMDsKPj4KPj4gIGVycl9jZToKPj4gKyAgICAgLyogWFhYOiBV bnRpbCB2YXIvaHcgaW5pdCBpcyBzcGxpdCBpdCdzIGltcG9zc2libGUgdG8gZml4IHRoZSBvcmRl cmluZwo+PiArICAgICAgKiBoZXJlIHNvIHdlIG11c3QgY2FsbCBzdG9wX2ludHIoKSBoZXJlIHRv byB0byBwcmV2ZW50IGludGVycnVwdHMgYWZ0ZXIKPj4gKyAgICAgICogQ0UgaXMgdGVhcmVkIGRv d24uIEl0J3Mgb2theSB0byBkb3VibGUgY2FsbCB0aGUgc3RvcF9pbnRyKCkKPj4gKi8KPgo+ICJG SVhNRToiCgpPay4KCgoKPj4gIGV4aXQ6Cj4+ICsgICAgIGFyX3BjaS0+aW50cl9zdGFydGVkID0g cmV0ID09IDA7Cj4KPiBBIGJpdCB0b28gY2xldmVyIGZvciB0aGUgc2FrZSBvZiByZWFkaWJpbGl0 eSBmb3IgbXkgdGFzdGUsIGJ1dCBJIGd1ZXNzCj4gaXQncyBvay4KPgo+PiAtLS0gYS9kcml2ZXJz L25ldC93aXJlbGVzcy9hdGgvYXRoMTBrL3BjaS5oCj4+ICsrKyBiL2RyaXZlcnMvbmV0L3dpcmVs ZXNzL2F0aC9hdGgxMGsvcGNpLmgKPj4gQEAgLTE5OCw2ICsxOTgsNyBAQCBzdHJ1Y3QgYXRoMTBr X3BjaSB7Cj4+ICAgICAgICAqIGludGVycnVwdHMuCj4+ICAgICAgICAqLwo+PiAgICAgICBpbnQg bnVtX21zaV9pbnRyczsKPj4gKyAgICAgYm9vbCBpbnRyX3N0YXJ0ZWQ7Cj4KPiBBZGRpbmcgYSBu ZXcgc3RhdGUgdmFyaWFibGUgbWFrZXMgbWUgd29ycmllZC4gSSByZWFsbHkgd291bGQgcHJlZmVy IGEKPiBzb2x1dGlvbiB3aGljaCB3b3VsZCBub3QgcmVxdWlyZSB0aGF0LgoKSSBrbm93IHRoYXQu IFRoYXQncyB3aHkgSSBtZW50aW9uZWQgaW4gdGhlIGNvbW1pdCBsb2cgdGhhdCBpdCBpcyBtb3Jl Cm9mIGEgd29ya2Fyb3VuZCB0aGFuIGEgcmVhbCBmaXguIE1lLCBJIGRvbid0IGxpa2UgdGhpcyBl aXRoZXIgYnV0IGEKcmVhbCBmaXggcmVxdWlyZXMgYSBsb3Qgb2YgcmV3b3JrIGZyb20gd2hhdCBJ IGNhbiB0ZWxsLgoKVGhpcyBidWcgY2FuIGJlIHRyaWdnZXJlZCBtb3JlIGVhc2lseSBub3cgYXBw YXJlbnRseSBhZnRlciByZWNvdmVyeQpwYXRjaGVzIHdlbnQgaW4uIEknbSBub3QgZXhwZXJpZW5j aW5nIGl0IGJ1dCBJIGdldCByZXBvcnRzIG9mIHJhcmUKcGFuaWNzIHdoZW4gYSBtYWNoaW5lIGlz IGxlZnQgaWRsZSBmb3IgYSB2ZXJ5IGxvbmcgdGltZSB3aXRoCmludGVyZmFjZXMgYnJvdWdodCBk b3duLgoKCj4gQWxzbyBpZiB3ZSBjYWxsIHJlcXVlc3RfaXJxKCkgaW4gYXRoMTBrX3BjaV9wcm9i ZSgpIHdlIHNob3VsZCBhbHNvIGNhbGwKPiBmcmVlX2lycSgpIGluIGF0aDEwa19wY2lfcmVtb3Zl KCkgZm9yIHN5bW1ldHJ5LiBKdXN0IGRvaW5nIGEgdGVtcG9yYXJ5Cj4gaGFjayB3aWxsIG1vc3Qg bGlrZWx5IHN0YXkgZm9yZXZlciA6KQoKV2l0aCB0aGUgcGF0Y2ggaW50ZXJydXB0cyBhcmUgdGVt cG9yYXJpbHkgZW5hYmxlZCZkaXNhYmxlZCBmb3IKcHJvYmVfZncoKSBkdXJpbmcgcGNpX3Byb2Jl KCkgYW5kIGFyZSB0aGVuIG5vdCByZXF1ZXN0ZWQgdW50aWwKbWFjODAyMTEgc3RhcnQoKS4KCgpQ b3pkcmF3aWFtIC8gQmVzdCByZWdhcmRzLApNaWNoYcWCIEthemlvci4KCl9fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmF0aDEwayBtYWlsaW5nIGxpc3QKYXRo MTBrQGxpc3RzLmluZnJhZGVhZC5vcmcKaHR0cDovL2xpc3RzLmluZnJhZGVhZC5vcmcvbWFpbG1h bi9saXN0aW5mby9hdGgxMGsK