From patchwork Fri Jul 12 14:51:54 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kalle Valo X-Patchwork-Id: 2826983 Return-Path: X-Original-To: patchwork-linux-wireless@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 127CB9F7D6 for ; Fri, 12 Jul 2013 14:52:08 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id C230C2012C for ; Fri, 12 Jul 2013 14:52:06 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 625C320127 for ; Fri, 12 Jul 2013 14:52:05 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932612Ab3GLOwD (ORCPT ); Fri, 12 Jul 2013 10:52:03 -0400 Received: from wolverine01.qualcomm.com ([199.106.114.254]:4479 "EHLO wolverine01.qualcomm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932588Ab3GLOwC (ORCPT ); Fri, 12 Jul 2013 10:52:02 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=qca.qualcomm.com; i=@qca.qualcomm.com; q=dns/txt; s=qcdkim; t=1373640722; x=1405176722; h=from:to:cc:subject:references:date:in-reply-to: message-id:mime-version; bh=PSshTGACBTfFmzq2WMyscPN5oTE/DwlI/J/wsFxd/qA=; b=oYmYOZwjp71dhbA8vXjYXLEoeUPd6b5eHJra3T1oYRjnnKrKf5cE8UZN q3sL7Ns0wdFlTUIS2s6h8YT3DcANHVtpt4gasUg1uDeUO21YFhEdQwFX4 DHSY8cOWBFKesy7ptq3yrpAEVk9JEnkSLEd9CsNPac1AzagRiSb1gfyrN A=; X-IronPort-AV: E=Sophos;i="4.89,653,1367996400"; d="scan'208";a="62353464" Received: from ironmsg03-l.qualcomm.com ([172.30.48.18]) by wolverine01.qualcomm.com with ESMTP; 12 Jul 2013 07:52:01 -0700 X-IronPort-AV: E=Sophos;i="4.89,653,1367996400"; d="scan'208";a="503832676" Received: from nasanexhc07.na.qualcomm.com ([172.30.39.190]) by Ironmsg03-L.qualcomm.com with ESMTP/TLS/RC4-SHA; 12 Jul 2013 07:52:01 -0700 Received: from potku.qca.qualcomm.com (172.30.39.5) by qcmail1.qualcomm.com (172.30.39.190) with Microsoft SMTP Server (TLS) id 14.2.318.4; Fri, 12 Jul 2013 07:52:00 -0700 From: Kalle Valo To: Ben Greear CC: , Subject: Re: [ath9k-devel] [PATCH] ath10k: Fix crash when using v1 hardware. References: <1372804925-1701-1-git-send-email-greearb@candelatech.com> <87y59d5tgu.fsf@kamboji.qca.qualcomm.com> <51DEC9A7.2080207@candelatech.com> Date: Fri, 12 Jul 2013 17:51:54 +0300 In-Reply-To: <51DEC9A7.2080207@candelatech.com> (Ben Greear's message of "Thu, 11 Jul 2013 08:05:11 -0700") Message-ID: <87d2qn4yr9.fsf@kamboji.qca.qualcomm.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.3 (gnu/linux) MIME-Version: 1.0 X-Originating-IP: [172.30.39.5] Sender: linux-wireless-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-wireless@vger.kernel.org X-Spam-Status: No, score=-7.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,T_DKIM_INVALID,UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Ben Greear writes: > On 07/11/2013 02:36 AM, Kalle Valo wrote: >> greearb@candelatech.com writes: >> >>> + /* On v1 hardware at least, setup can fail, causing ce_id_state to >>> + * be cleaned up, but this method is still called a few times. Check >>> + * for NULL here so we don't crash. Probably a better fix is to stop >>> + * the ath10k_pci_ce_tasklet sooner. >>> + */ >>> + if (WARN_ONCE(!ce_state, "ce_id_to_state[%i] is NULL\n", ce_id)) >>> + return; >>> + >>> + ctrl_addr = ce_state->ctrl_addr; >>> + >> >> The tests you add look like workarounds. I would prefer to try fix these >> by going to the source of the problem. Maybe we should add >> ath10k_pci_wake() and ath10k_do_pci_wake()? Doh, dropped an essential word from a sentence, again. That's what I get when trying to do multiple things at the same time. What I was trying to say: Maybe we should add proper error hanling to ath10k_pci_wake() and ath10k_do_pci_wake() and that way avoid this? > These are work-arounds, but you should not let a bad piece of > hardware/firmware crash the entire OS just because you don't want to > do sanity checking on the values you get from the firmware. Perhaps > there is a better fix for the code above, but the warning splat should > still provide incentive to get it right, while not crashing the OS in > the meantime. Sure, the driver should not crash if HW is not functioning correctly. I'm just saying that adding odd checks at random places is not the "kernel way" to do things, only GTK people do that ;) I was thinking that the proper way is to check that wakeup succeeds and not enable interrupts or something like that. >> Can you enable few debug logs, like ATH10K_DBG_PCI, and post them? That >> would give more hint there things are going wrong. > > Yes, I can do that. Thanks. Oh, did you mention something that ath10k detect the device as hw2.0? Maybe the PCI ids are wrong? Then you could also force the same workaround for hw2.0 as hw1.0 has: --- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html --- a/drivers/net/wireless/ath/ath10k/pci.c +++ b/drivers/net/wireless/ath/ath10k/pci.c @@ -2145,10 +2145,8 @@ static int ath10k_pci_probe(struct pci_dev *pdev, switch (pci_dev->device) { case QCA988X_1_0_DEVICE_ID: - set_bit(ATH10K_PCI_FEATURE_HW_1_0_WARKAROUND, ar_pci->features); - break; case QCA988X_2_0_DEVICE_ID: - set_bit(ATH10K_PCI_FEATURE_MSI_X, ar_pci->features); + set_bit(ATH10K_PCI_FEATURE_HW_1_0_WARKAROUND, ar_pci->features); break; default: ret = -ENODEV;