Message ID | 20230705123018.30903-2-johan+linaro@kernel.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:9f45:0:b0:3ea:f831:8777 with SMTP id v5csp1842234vqx; Wed, 5 Jul 2023 05:47:41 -0700 (PDT) X-Google-Smtp-Source: APBJJlHLLytOGicmdQlEPV+t0uLiT04AueECFQMWT+DruXlkSJbyFwFC0ejobfBzzy/r9kYxUNtU X-Received: by 2002:a05:6870:2893:b0:1b3:6ea7:7475 with SMTP id gy19-20020a056870289300b001b36ea77475mr16533484oab.0.1688561261576; Wed, 05 Jul 2023 05:47:41 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1688561261; cv=none; d=google.com; s=arc-20160816; b=ZkEySbQkfu+gr3nGBjNUwkRKZr+kVqLARvk52fKrnsyqfNyCgRq+KlkTkyNSSQ9oFA Hc2pbMajrkUl6YkVc49GoPZJLUAl2FH4Q8hcu719yPUDB1x2AIDKOdDuukWh7y69a6hu JQplQwimx6HFZ8ZbJKvqr+ZTqgqHp0RzMnTpNROa7G7QxCkeEU+NiPU5aWzdwmhb1rjU 65iwe1tlWgfgoq36bN/vu812DsPUw+0/gLbtjjp9obLKKz4ZFeX7TZs8Q1OvNteWt++B FC9ccokK+ml3PlSJhBj7PanTNDYs2oSqbveVXto/1RJdaYLupYlNFylZTw9il2Xf31vk tv7Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=KqSBwE7BQr9lNwMcCs1LL8WiQVG4n1/jQzSi1ZMENW0=; fh=XKNopzo9Dap4S70gl2ZbkKSseUmtTmPvDd0RQ04DZiw=; b=r/eSP0nhCSyiqweRWRxVByJGt3ajl0IVD6SZZGSDPtQDtqheRnXw6HU9Ej2JT7mmkJ ip4N+lhtkPP+Q4UjdRsD0oTdVq6C8Y8ifH1dgWZ46u5xNZMgxuWLaSTFCn+JvaXO90qP rUhkUnAtBO1jAgJjRXsVg86Mu4mr7zFxqftUF6ypOZoUIx3cDDEKhlfweQOxHsxWcfGC fCDmOSILLYTCZFBtae3ez2MNMbMSvE9Uz1veFbWHRCww+5icZAd6euBqNsq7pIPXDOZP oUaG6sGrPISfja8APVu6Fy/kkR3xdDH7KMVDtkfnMS8eS8/RZfym1sBQ9fXdVgTtweov Ijvg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=L0Dgj5tm; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id m20-20020a17090b069400b0025bf86c41absi1460196pjz.151.2023.07.05.05.47.27; Wed, 05 Jul 2023 05:47:41 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=L0Dgj5tm; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231847AbjGEMdT (ORCPT <rfc822;tebrre53rla2o@gmail.com> + 99 others); Wed, 5 Jul 2023 08:33:19 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:44998 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231566AbjGEMdM (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 5 Jul 2023 08:33:12 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C613E11D; Wed, 5 Jul 2023 05:33:10 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 3073E6155D; Wed, 5 Jul 2023 12:33:10 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 83AA8C433CA; Wed, 5 Jul 2023 12:33:09 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1688560389; bh=CMTp5PjcrdGB1YgiokfJfbw4f+gtghxZ4pFtHuNvMII=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=L0Dgj5tmXqHoyyYmXI4/RmaTjUN4wyzU/xOCwCBvvGoE39qgLwvw+J1h106R3nsDs Sef6fGSlgU22yQq+WQMbyo9fpKjJiBIMrTbYCxBXWou4lKdL5kFZ6b4IBLSP0/T2A3 uqoVQUKZd6PlWEPqZE+n6mw0SJbDbCbssW5sgI3z9/5jwEDHeQIto4znL7GBKASZgW FAhE0hSZ0ngFp3V44O8SMuKy+rHr743Uz5Ci4GeaIhkMbTCXU3Nl/w0MvM7SRsFbST KxcuBFBhR9Lfvz0mvlhZGOBNNNv6ojq8fZzldhjMt9Y1u1t89wnokWr25IGUghR+7M 21ZSaShu23tpg== Received: from johan by xi.lan with local (Exim 4.96) (envelope-from <johan+linaro@kernel.org>) id 1qH1hK-000848-0Q; Wed, 05 Jul 2023 14:33:30 +0200 From: Johan Hovold <johan+linaro@kernel.org> To: Mark Brown <broonie@kernel.org>, Vinod Koul <vkoul@kernel.org> Cc: Bard Liao <yung-chuan.liao@linux.intel.com>, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>, Sanyog Kale <sanyog.r.kale@intel.com>, Srinivas Kandagatla <srinivas.kandagatla@linaro.org>, Banajit Goswami <bgoswami@quicinc.com>, Liam Girdwood <lgirdwood@gmail.com>, Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>, alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org, Johan Hovold <johan+linaro@kernel.org>, stable@vger.kernel.org, Rander Wang <rander.wang@linux.intel.com> Subject: [PATCH 1/8] soundwire: fix enumeration completion Date: Wed, 5 Jul 2023 14:30:11 +0200 Message-Id: <20230705123018.30903-2-johan+linaro@kernel.org> X-Mailer: git-send-email 2.39.3 In-Reply-To: <20230705123018.30903-1-johan+linaro@kernel.org> References: <20230705123018.30903-1-johan+linaro@kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1770584812928236988?= X-GMAIL-MSGID: =?utf-8?q?1770584812928236988?= |
Series |
ASoC/soundwire/qdsp6/wcd: fix leaks and probe deferral
|
|
Commit Message
Johan Hovold
July 5, 2023, 12:30 p.m. UTC
The soundwire subsystem uses two completion structures that allow
drivers to wait for soundwire device to become enumerated on the bus and
initialised by their drivers, respectively.
The code implementing the signalling is currently broken as it does not
signal all current and future waiters and also uses the wrong
reinitialisation function, which can potentially lead to memory
corruption if there are still waiters on the queue.
Not signalling future waiters specifically breaks sound card probe
deferrals as codec drivers can not tell that the soundwire device is
already attached when being reprobed. Some codec runtime PM
implementations suffer from similar problems as waiting for enumeration
during resume can also timeout despite the device already having been
enumerated.
Fixes: fb9469e54fa7 ("soundwire: bus: fix race condition with enumeration_complete signaling")
Fixes: a90def068127 ("soundwire: bus: fix race condition with initialization_complete signaling")
Cc: stable@vger.kernel.org # 5.7
Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc: Rander Wang <rander.wang@linux.intel.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
drivers/soundwire/bus.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
Comments
On 7/5/23 14:30, Johan Hovold wrote: > The soundwire subsystem uses two completion structures that allow > drivers to wait for soundwire device to become enumerated on the bus and > initialised by their drivers, respectively. > > The code implementing the signalling is currently broken as it does not > signal all current and future waiters and also uses the wrong > reinitialisation function, which can potentially lead to memory > corruption if there are still waiters on the queue. That change sounds good, but I am not following the two paragraphs below. > Not signalling future waiters specifically breaks sound card probe > deferrals as codec drivers can not tell that the soundwire device is > already attached when being reprobed. What makes you say that? There is a test in the probe and the codec driver will absolutely be notified, see bus_type.c if (drv->ops && drv->ops->update_status) { ret = drv->ops->update_status(slave, slave->status); if (ret < 0) dev_warn(dev, "%s: update_status failed with status %d\n", __func__, ret); } > Some codec runtime PM > implementations suffer from similar problems as waiting for enumeration > during resume can also timeout despite the device already having been > enumerated. I am not following this either. Are you saying the wait_for_completion() times out because of the init_completion/reinit_completion confusion, or something else. > Fixes: fb9469e54fa7 ("soundwire: bus: fix race condition with enumeration_complete signaling") > Fixes: a90def068127 ("soundwire: bus: fix race condition with initialization_complete signaling") > Cc: stable@vger.kernel.org # 5.7 > Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Cc: Rander Wang <rander.wang@linux.intel.com> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > drivers/soundwire/bus.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c > index 1ea6a64f8c4a..66e5dba919fa 100644 > --- a/drivers/soundwire/bus.c > +++ b/drivers/soundwire/bus.c > @@ -908,8 +908,8 @@ static void sdw_modify_slave_status(struct sdw_slave *slave, > "initializing enumeration and init completion for Slave %d\n", > slave->dev_num); > > - init_completion(&slave->enumeration_complete); > - init_completion(&slave->initialization_complete); > + reinit_completion(&slave->enumeration_complete); > + reinit_completion(&slave->initialization_complete); > > } else if ((status == SDW_SLAVE_ATTACHED) && > (slave->status == SDW_SLAVE_UNATTACHED)) { > @@ -917,7 +917,7 @@ static void sdw_modify_slave_status(struct sdw_slave *slave, > "signaling enumeration completion for Slave %d\n", > slave->dev_num); > > - complete(&slave->enumeration_complete); > + complete_all(&slave->enumeration_complete); > } > slave->status = status; > mutex_unlock(&bus->bus_lock); > @@ -1941,7 +1941,7 @@ int sdw_handle_slave_status(struct sdw_bus *bus, > "signaling initialization completion for Slave %d\n", > slave->dev_num); > > - complete(&slave->initialization_complete); > + complete_all(&slave->initialization_complete); > > /* > * If the manager became pm_runtime active, the peripherals will be
On Wed, Jul 05, 2023 at 02:53:17PM +0200, Pierre-Louis Bossart wrote: > On 7/5/23 14:30, Johan Hovold wrote: > > The soundwire subsystem uses two completion structures that allow > > drivers to wait for soundwire device to become enumerated on the bus and > > initialised by their drivers, respectively. > > > > The code implementing the signalling is currently broken as it does not > > signal all current and future waiters and also uses the wrong > > reinitialisation function, which can potentially lead to memory > > corruption if there are still waiters on the queue. > > That change sounds good, but I am not following the two paragraphs below. > > > Not signalling future waiters specifically breaks sound card probe > > deferrals as codec drivers can not tell that the soundwire device is > > already attached when being reprobed. > > What makes you say that? There is a test in the probe and the codec > driver will absolutely be notified, see bus_type.c > > if (drv->ops && drv->ops->update_status) { > ret = drv->ops->update_status(slave, slave->status); > if (ret < 0) > dev_warn(dev, "%s: update_status failed with status %d\n", __func__, > ret); > } I'm talking about signalling the codec driver using the soundwire device via the completion structs. Unless the underling device is detached and reattached, trying to wait for completion a second time will currently timeout instead of returning immediately. This affects codecs like rt5682, which wait for completion in component probe (see rt5682_probe()). > > Some codec runtime PM > > implementations suffer from similar problems as waiting for enumeration > > during resume can also timeout despite the device already having been > > enumerated. > > I am not following this either. Are you saying the wait_for_completion() > times out because of the init_completion/reinit_completion confusion, or > something else. It times out because the completion counter is not saturated unless you use complete_all(). Drivers that wait unconditionally in resume, will time out the second time they are runtime resumed unless the underlying device has been detached and reattached in the meantime (e.g. wsa881x_runtime_resume()). Johan
On 7/5/23 16:30, Johan Hovold wrote: > On Wed, Jul 05, 2023 at 02:53:17PM +0200, Pierre-Louis Bossart wrote: >> On 7/5/23 14:30, Johan Hovold wrote: >>> The soundwire subsystem uses two completion structures that allow >>> drivers to wait for soundwire device to become enumerated on the bus and >>> initialised by their drivers, respectively. >>> >>> The code implementing the signalling is currently broken as it does not >>> signal all current and future waiters and also uses the wrong >>> reinitialisation function, which can potentially lead to memory >>> corruption if there are still waiters on the queue. >> >> That change sounds good, but I am not following the two paragraphs below. >> >>> Not signalling future waiters specifically breaks sound card probe >>> deferrals as codec drivers can not tell that the soundwire device is >>> already attached when being reprobed. >> >> What makes you say that? There is a test in the probe and the codec >> driver will absolutely be notified, see bus_type.c >> >> if (drv->ops && drv->ops->update_status) { >> ret = drv->ops->update_status(slave, slave->status); >> if (ret < 0) >> dev_warn(dev, "%s: update_status failed with status %d\n", __func__, >> ret); >> } > > I'm talking about signalling the codec driver using the soundwire device > via the completion structs. Unless the underling device is detached and > reattached, trying to wait for completion a second time will currently > timeout instead of returning immediately. > > This affects codecs like rt5682, which wait for completion in component > probe (see rt5682_probe()). > >>> Some codec runtime PM >>> implementations suffer from similar problems as waiting for enumeration >>> during resume can also timeout despite the device already having been >>> enumerated. >> >> I am not following this either. Are you saying the wait_for_completion() >> times out because of the init_completion/reinit_completion confusion, or >> something else. > > It times out because the completion counter is not saturated unless you > use complete_all(). > > Drivers that wait unconditionally in resume, will time out the second > time they are runtime resumed unless the underlying device has been > detached and reattached in the meantime (e.g. wsa881x_runtime_resume()). Makes sense. The default on Intel platforms is to reset the bus in all resume cases, that forces the attachment so we never saw the issue. For this patch: Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index 1ea6a64f8c4a..66e5dba919fa 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -908,8 +908,8 @@ static void sdw_modify_slave_status(struct sdw_slave *slave, "initializing enumeration and init completion for Slave %d\n", slave->dev_num); - init_completion(&slave->enumeration_complete); - init_completion(&slave->initialization_complete); + reinit_completion(&slave->enumeration_complete); + reinit_completion(&slave->initialization_complete); } else if ((status == SDW_SLAVE_ATTACHED) && (slave->status == SDW_SLAVE_UNATTACHED)) { @@ -917,7 +917,7 @@ static void sdw_modify_slave_status(struct sdw_slave *slave, "signaling enumeration completion for Slave %d\n", slave->dev_num); - complete(&slave->enumeration_complete); + complete_all(&slave->enumeration_complete); } slave->status = status; mutex_unlock(&bus->bus_lock); @@ -1941,7 +1941,7 @@ int sdw_handle_slave_status(struct sdw_bus *bus, "signaling initialization completion for Slave %d\n", slave->dev_num); - complete(&slave->initialization_complete); + complete_all(&slave->initialization_complete); /* * If the manager became pm_runtime active, the peripherals will be