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=-2.2 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=no 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 AFCD9C433DF for ; Thu, 11 Jun 2020 20:38:02 +0000 (UTC) Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 397D92073E for ; Thu, 11 Jun 2020 20:38:02 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=alsa-project.org header.i=@alsa-project.org header.b="f9FeDxdf" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 397D92073E Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.intel.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=alsa-devel-bounces@alsa-project.org Received: from alsa1.perex.cz (alsa1.perex.cz [207.180.221.201]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa0.perex.cz (Postfix) with ESMTPS id 9E1381671; Thu, 11 Jun 2020 22:37:10 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 9E1381671 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1591907880; bh=YVAb8p+ZwpFzxqqGbkxy+d4P2qvycTILgkYIQr7pUgc=; h=Subject:To:References:From:Date:In-Reply-To:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=f9FeDxdfEKts9XwP9PiRqHK5lAHrQLQEFQa2aF8OTkB42uvzohntRhYb2ukoQwbxF TsNLomcrkiugGsYtBOkL94vrA2t9rpDd6v0oEzFZGtiu3o8CrLTA9ws5oHsU78JlXg qCVZVMr7Vxe3FFYKPDE56p1UKB99hR4eFSC7ELuY= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id 1E80FF80278; Thu, 11 Jun 2020 22:37:10 +0200 (CEST) Received: by alsa1.perex.cz (Postfix, from userid 50401) id 3EE5EF8028C; Thu, 11 Jun 2020 22:37:08 +0200 (CEST) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id BA97EF800CC for ; Thu, 11 Jun 2020 22:37:03 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz BA97EF800CC IronPort-SDR: v/mc14H2sot7BhRp2L2VMJvsHroUwlXfrTmdzE7gturrfzbyaslw16ZZhYTqIiRP688B7a9Vl9 b3FibWYdrEiQ== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Jun 2020 13:37:00 -0700 IronPort-SDR: pepvTIbg9SIlqOWtoQ4OtOupzy32cfxyZsYPgMKHnzW0Mdk4DZZjFbx+yJ6d345eaUwBBgSk2T cxGwt5K3WxoA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.73,501,1583222400"; d="scan'208";a="260628282" Received: from mgesquiv-mobl.amr.corp.intel.com (HELO [10.254.106.112]) ([10.254.106.112]) by orsmga007.jf.intel.com with ESMTP; 11 Jun 2020 13:36:59 -0700 Subject: Re: [PATCH] ASoC: SOF: Intel: hda: unsolicited RIRB response To: Takashi Iwai , Ranjani Sridharan References: <1591883073-17190-1-git-send-email-brent.lu@intel.com> From: Pierre-Louis Bossart Message-ID: Date: Thu, 11 Jun 2020 15:36:58 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Cc: "alsa-devel@alsa-project.org" , "commit_signer:6/16=38%, authored:6/16=38%, added_lines:123/248=50%, removed_lines:36/84=43%, Kai Vehmanen DRIVERS \)" , Bard Liao , "Rojewski, Cezary" , Takashi Iwai , Keyon Jie , "authored:2/16=12%, added_lines:21/248=8%, removed_lines:5/84=6%, \), Liam Girdwood DRIVERS \)" , Mark Brown , Zhu Yingjiang , "sound-open-firmware@alsa-project.orgDRIVERS" , "Daniel Baluta DRIVERS \)" , "Lu, Brent" , "linux-kernel@vger.kernel.org" X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" >>>> I added debug messages to print the RIRBWP register and realize >>>> that >>>> response could come between the read of RIRBWP in the >>>> snd_hdac_bus_update_rirb() function and the interrupt clear in the >>>> hda_dsp_stream_interrupt() function. The response is not handled >>>> but >>>> the interrupt is already cleared. It will cause timeout unless more >>>> responses coming to RIRB. >>> >>> Now I noticed that the legacy driver already addressed it recently >>> via >>> commit 6d011d5057ff >>> ALSA: hda: Clear RIRB status before reading WP >>> >>> We should have checked SOF at the same time, too... >> >> Thanks, Takashi. But the legacy driver but doesnt remove the loop. The >> loop added in the SOF driver was based on the legacy driver and >> specifically to handle missed stream interrupts. Is there any harm in >> keeping the loop? > > A loop there might be safer to keep, indeed. That's basically for a > difference kind of race, and it can still happen theoretically. > > Though, SOF is with the threaded interrupt, and it's interesting how > the behavior differs. I can imagine that, if a thread irq is running > while a new IRQ is re-triggered, the hard irq handler won't queue it > again. But I might be wrong here, need some checks. IIRC we added this loop before merging all interrupt handling in one thread, somehow the MSI mode never worked reliably without this change, so maybe we don't need this loop any longer. I'd really prefer it if we didn't tie the RIRB handing change to this loop change, removing the loop should only be done with *a lot of testing*. 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=-2.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=no 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 3B43EC433E0 for ; Thu, 11 Jun 2020 20:37:03 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 16E03207ED for ; Thu, 11 Jun 2020 20:37:03 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726287AbgFKUhB (ORCPT ); Thu, 11 Jun 2020 16:37:01 -0400 Received: from mga05.intel.com ([192.55.52.43]:30161 "EHLO mga05.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725782AbgFKUhB (ORCPT ); Thu, 11 Jun 2020 16:37:01 -0400 IronPort-SDR: PXV7M5sLYG8V5PksoWwDKYbcxT3ryHKibXqu5a8YBqazzYMD08Zp/sRZoGXgQQjF1FaXOQ8pyE pxir9525G3gw== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Jun 2020 13:37:00 -0700 IronPort-SDR: pepvTIbg9SIlqOWtoQ4OtOupzy32cfxyZsYPgMKHnzW0Mdk4DZZjFbx+yJ6d345eaUwBBgSk2T cxGwt5K3WxoA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.73,501,1583222400"; d="scan'208";a="260628282" Received: from mgesquiv-mobl.amr.corp.intel.com (HELO [10.254.106.112]) ([10.254.106.112]) by orsmga007.jf.intel.com with ESMTP; 11 Jun 2020 13:36:59 -0700 Subject: Re: [PATCH] ASoC: SOF: Intel: hda: unsolicited RIRB response To: Takashi Iwai , Ranjani Sridharan Cc: "Lu, Brent" , "alsa-devel@alsa-project.org" , "authored:2/16=12%,added_lines:21/248=8%,removed_lines:5/84=6%,),Liam" Girdwood DRIVERS ")" , "commit_signer:6/16=38%,authored:6/16=38%,added_lines:123/248=50% ,removed_lines:36/84=43%,Kai" Vehmanen DRIVERS ")" , "Daniel Baluta DRIVERS )" , Mark Brown , Jaroslav Kysela , Takashi Iwai , "Rojewski, Cezary" , Zhu Yingjiang , Keyon Jie , Bard Liao , "sound-open-firmware@alsa-project.orgDRIVERS" , "linux-kernel@vger.kernel.org" References: <1591883073-17190-1-git-send-email-brent.lu@intel.com> From: Pierre-Louis Bossart Message-ID: Date: Thu, 11 Jun 2020 15:36:58 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org >>>> I added debug messages to print the RIRBWP register and realize >>>> that >>>> response could come between the read of RIRBWP in the >>>> snd_hdac_bus_update_rirb() function and the interrupt clear in the >>>> hda_dsp_stream_interrupt() function. The response is not handled >>>> but >>>> the interrupt is already cleared. It will cause timeout unless more >>>> responses coming to RIRB. >>> >>> Now I noticed that the legacy driver already addressed it recently >>> via >>> commit 6d011d5057ff >>> ALSA: hda: Clear RIRB status before reading WP >>> >>> We should have checked SOF at the same time, too... >> >> Thanks, Takashi. But the legacy driver but doesnt remove the loop. The >> loop added in the SOF driver was based on the legacy driver and >> specifically to handle missed stream interrupts. Is there any harm in >> keeping the loop? > > A loop there might be safer to keep, indeed. That's basically for a > difference kind of race, and it can still happen theoretically. > > Though, SOF is with the threaded interrupt, and it's interesting how > the behavior differs. I can imagine that, if a thread irq is running > while a new IRQ is re-triggered, the hard irq handler won't queue it > again. But I might be wrong here, need some checks. IIRC we added this loop before merging all interrupt handling in one thread, somehow the MSI mode never worked reliably without this change, so maybe we don't need this loop any longer. I'd really prefer it if we didn't tie the RIRB handing change to this loop change, removing the loop should only be done with *a lot of testing*.