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=-15.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham 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 CB3A6C4338F for ; Thu, 29 Jul 2021 09:48:28 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id B1F3060FD7 for ; Thu, 29 Jul 2021 09:48:28 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235488AbhG2Jsa (ORCPT ); Thu, 29 Jul 2021 05:48:30 -0400 Received: from smtp-out1.suse.de ([195.135.220.28]:58454 "EHLO smtp-out1.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235364AbhG2Js1 (ORCPT ); Thu, 29 Jul 2021 05:48:27 -0400 Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id 28CBF223F8; Thu, 29 Jul 2021 09:48:24 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1627552104; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=1GfBweEzJ55yOPJo4QR7hkQ5bfwkIvX22+qPgnRjBnU=; b=RcUzmr7SrgDCkiebeVulq2M/ksxsRnkGEJ9AE1ao0k7AO1rO1RoprHGp1QgXM2nE+Up2GW 8jdLthfvHWDCTW1AlHK196J1S6O8tQRgKG1USUzLTUawiEcP2qqcIwcO7MiVLlmJ40NxY4 pyjrZKW1WBY4Tm9JBET76LyevFR1JEk= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1627552104; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=1GfBweEzJ55yOPJo4QR7hkQ5bfwkIvX22+qPgnRjBnU=; b=7e9HAB8T/apMZRYmWvTpA7Jt2bppgxT5c33UFF0r5QIC2sY1X3Ub8fyUlnvixq8510dEhb Rqc7Xl8ggfgBxjAg== Received: from alsa1.suse.de (alsa1.suse.de [10.160.4.42]) by relay2.suse.de (Postfix) with ESMTP id 142A5A3B81; Thu, 29 Jul 2021 09:48:24 +0000 (UTC) Date: Thu, 29 Jul 2021 11:48:24 +0200 Message-ID: From: Takashi Iwai To: Vitaly Rodionov Cc: Jaroslav Kysela , Takashi Iwai , , , , Lucas Tanure , Stefan Binding Subject: Re: [PATCH v2 13/27] ALSA: hda/cs8409: Dont disable I2C clock between consecutive accesses In-Reply-To: <20210728134408.369396-14-vitalyr@opensource.cirrus.com> References: <20210728134408.369396-1-vitalyr@opensource.cirrus.com> <20210728134408.369396-14-vitalyr@opensource.cirrus.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 28 Jul 2021 15:43:54 +0200, Vitaly Rodionov wrote: > > From: Lucas Tanure > > Only disable I2C clock 25 ms after not being used. > > The current implementation enables and disables the I2C clock for each > I2C transaction. Each enable/disable call requires two verb transactions. > This means each I2C transaction requires a total of four verb transactions > to enable and disable the clock. > However, if there are multiple consecutive I2C transactions, it is not > necessary to enable and disable the clock each time, instead it is more > efficient to enable the clock for the first transaction, and disable it > after the final transaction, which would improve performance. > This is achieved by using a timeout which disables the clock if no request > to enable the clock has occurred for 25 ms. > > Signed-off-by: Lucas Tanure > Signed-off-by: Vitaly Rodionov > Signed-off-by: Stefan Binding > > Changes in v2: > Improved delayed work start/cancel implementation, and re-worked commit message > adding more explanation why this was required. > > > --- > sound/pci/hda/patch_cs8409.c | 56 +++++++++++++++++++++++++----------- > sound/pci/hda/patch_cs8409.h | 4 +++ > 2 files changed, 43 insertions(+), 17 deletions(-) > > diff --git a/sound/pci/hda/patch_cs8409.c b/sound/pci/hda/patch_cs8409.c > index 08205c19698c..fafc0f309e70 100644 > --- a/sound/pci/hda/patch_cs8409.c > +++ b/sound/pci/hda/patch_cs8409.c > @@ -53,7 +53,9 @@ static struct cs8409_spec *cs8409_alloc_spec(struct hda_codec *codec) > if (!spec) > return NULL; > codec->spec = spec; > + spec->codec = codec; > codec->power_save_node = 1; > + INIT_DELAYED_WORK(&spec->i2c_clk_work, cs8409_disable_i2c_clock); > snd_hda_gen_spec_init(&spec->gen); > > return spec; > @@ -72,21 +74,37 @@ static inline void cs8409_vendor_coef_set(struct hda_codec *codec, unsigned int > snd_hda_codec_write(codec, CS8409_PIN_VENDOR_WIDGET, 0, AC_VERB_SET_PROC_COEF, coef); > } > > -/** > +/* > + * cs8409_disable_i2c_clock - Worker that disable the I2C Clock after 25ms without use > + */ > +static void cs8409_disable_i2c_clock(struct work_struct *work) > +{ > + struct cs8409_spec *spec = container_of(work, struct cs8409_spec, i2c_clk_work.work); > + > + mutex_lock(&spec->cs8409_i2c_mux); > + cs8409_vendor_coef_set(spec->codec, 0x0, > + cs8409_vendor_coef_get(spec->codec, 0x0) & 0xfffffff7); > + spec->i2c_clck_enabled = 0; > + mutex_unlock(&spec->cs8409_i2c_mux); Here we have a lock in the work, and this would become a problem in the below... > +} > + > +/* > * cs8409_enable_i2c_clock - Enable I2C clocks > * @codec: the codec instance > - * @enable: Enable or disable I2C clocks > - * > * Enable or Disable I2C clocks. > + * This must be called when the i2c mutex is locked. > */ > -static void cs8409_enable_i2c_clock(struct hda_codec *codec, unsigned int enable) > +static void cs8409_enable_i2c_clock(struct hda_codec *codec) > { > - unsigned int retval; > - unsigned int newval; > + struct cs8409_spec *spec = codec->spec; > + > + cancel_delayed_work_sync(&spec->i2c_clk_work); Here, cancel_delayed_work_sync(). Since it's called inside the mutex mutex (taken in the caller side), it'll lead to a deadlock. (I know the i2c mutex lock is moved to this function in a later patch, but this makes harder to review. Given that it's only about the performance, it'd better to apply this kind of change at the end of series, not in the middle.) In general, making such an async handling really race-free is not that trivial. Even if you call cancel_delayed_work_sync() outside the mutex, it's possible that another task wedges itself between cancel_work() and the mutex lock and re-enables the work. One easier alternative implementation would be rather to allow enable / disable clock reentrant with refcounting. It accepts the function sequence like: enable_clock(); i2c_write(); i2c_write(); ... disable_clock(); while another thread calls enable_clock(); i2c_write(); disable_clock(); at the same time. thanks, Takashi