From: Takashi Iwai <tiwai@suse.de> To: Vitaly Rodionov <vitalyr@opensource.cirrus.com> Cc: Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>, <alsa-devel@alsa-project.org>, <patches@opensource.cirrus.com>, <linux-kernel@vger.kernel.org>, Lucas Tanure <tanureal@opensource.cirrus.com> Subject: Re: [PATCH 13/27] ALSA: hda/cs8409: Dont disable I2C clock between consecutive accesses Date: Tue, 27 Jul 2021 08:47:47 +0200 [thread overview] Message-ID: <s5h4kcgfe24.wl-tiwai@suse.de> (raw) In-Reply-To: <20210726174640.6390-14-vitalyr@opensource.cirrus.com> On Mon, 26 Jul 2021 19:46:26 +0200, Vitaly Rodionov wrote: > > From: Lucas Tanure <tanureal@opensource.cirrus.com> > > Only disable I2C clock 25 ms after not being used This is error-prone and might be racy, I'm afraid. e.g. the offloaded work isn't canceled when unbinding the driver, so it'll lead to a use-after-free. And the work doesn't take a mutex so it may conflict with re-enabling side. Above all, there is no description "why" this has to be done so. Is it about the performance, or any other reason? thanks, Takashi > > Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com> > Signed-off-by: Vitaly Rodionov <vitalyr@opensource.cirrus.com> > --- > sound/pci/hda/patch_cs8409.c | 42 +++++++++++++++++++++++------------- > sound/pci/hda/patch_cs8409.h | 4 ++++ > 2 files changed, 31 insertions(+), 15 deletions(-) > > diff --git a/sound/pci/hda/patch_cs8409.c b/sound/pci/hda/patch_cs8409.c > index 08205c19698c..335bcdc69106 100644 > --- a/sound/pci/hda/patch_cs8409.c > +++ b/sound/pci/hda/patch_cs8409.c > @@ -53,6 +53,7 @@ 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; > snd_hda_gen_spec_init(&spec->gen); > > @@ -72,21 +73,34 @@ 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); > + > + cs8409_vendor_coef_set(spec->codec, 0x0, > + cs8409_vendor_coef_get(spec->codec, 0x0) & 0xfffffff7); > + spec->i2c_clck_enabled = 0; > +} > + > +/* > * cs8409_enable_i2c_clock - Enable I2C clocks > * @codec: the codec instance > - * @enable: Enable or disable I2C clocks > - * > * Enable or Disable I2C clocks. > */ > -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); > > - retval = cs8409_vendor_coef_get(codec, 0x0); > - newval = (enable) ? (retval | 0x8) : (retval & 0xfffffff7); > - cs8409_vendor_coef_set(codec, 0x0, newval); > + if (!spec->i2c_clck_enabled) { > + cs8409_vendor_coef_set(codec, 0x0, cs8409_vendor_coef_get(codec, 0x0) | 0x8); > + spec->i2c_clck_enabled = 1; > + } > + queue_delayed_work(system_power_efficient_wq, &spec->i2c_clk_work, msecs_to_jiffies(25)); > } > > /** > @@ -134,7 +148,7 @@ static int cs8409_i2c_read(struct hda_codec *codec, unsigned int i2c_address, un > if (spec->cs42l42_suspended) > return -EPERM; > > - cs8409_enable_i2c_clock(codec, 1); > + cs8409_enable_i2c_clock(codec); > cs8409_vendor_coef_set(codec, CS8409_I2C_ADDR, i2c_address); > > if (paged) { > @@ -157,8 +171,6 @@ static int cs8409_i2c_read(struct hda_codec *codec, unsigned int i2c_address, un > /* Register in bits 15-8 and the data in 7-0 */ > read_data = cs8409_vendor_coef_get(codec, CS8409_I2C_QREAD); > > - cs8409_enable_i2c_clock(codec, 0); > - > return read_data & 0x0ff; > } > > @@ -182,7 +194,7 @@ static int cs8409_i2c_write(struct hda_codec *codec, unsigned int i2c_address, u > if (spec->cs42l42_suspended) > return -EPERM; > > - cs8409_enable_i2c_clock(codec, 1); > + cs8409_enable_i2c_clock(codec); > cs8409_vendor_coef_set(codec, CS8409_I2C_ADDR, i2c_address); > > if (paged) { > @@ -203,8 +215,6 @@ static int cs8409_i2c_write(struct hda_codec *codec, unsigned int i2c_address, u > return -EIO; > } > > - cs8409_enable_i2c_clock(codec, 0); > - > return 0; > } > > @@ -705,6 +715,8 @@ void cs8409_cs42l42_fixups(struct hda_codec *codec, const struct hda_fixup *fix, > spec->cs42l42_mic_jack_in = 0; > spec->cs42l42_suspended = 1; > > + INIT_DELAYED_WORK(&spec->i2c_clk_work, cs8409_disable_i2c_clock); > + > /* Basic initial sequence for specific hw configuration */ > snd_hda_sequence_write(codec, cs8409_cs42l42_init_verbs); > > diff --git a/sound/pci/hda/patch_cs8409.h b/sound/pci/hda/patch_cs8409.h > index bf0e8a4cc4cc..542582c213d2 100644 > --- a/sound/pci/hda/patch_cs8409.h > +++ b/sound/pci/hda/patch_cs8409.h > @@ -11,6 +11,7 @@ > > #include <linux/pci.h> > #include <sound/tlv.h> > +#include <linux/workqueue.h> > #include <sound/hda_codec.h> > #include "hda_local.h" > #include "hda_auto_parser.h" > @@ -267,6 +268,7 @@ struct cs8409_cir_param { > > struct cs8409_spec { > struct hda_gen_spec gen; > + struct hda_codec *codec; > > unsigned int gpio_mask; > unsigned int gpio_dir; > @@ -278,6 +280,8 @@ struct cs8409_spec { > s8 vol[CS42L42_VOLUMES]; > > struct mutex cs8409_i2c_mux; > + unsigned int i2c_clck_enabled; > + struct delayed_work i2c_clk_work; > > /* verb exec op override */ > int (*exec_verb)(struct hdac_device *dev, unsigned int cmd, unsigned int flags, > -- > 2.25.1 >
WARNING: multiple messages have this Message-ID (diff)
From: Takashi Iwai <tiwai@suse.de> To: Vitaly Rodionov <vitalyr@opensource.cirrus.com> Cc: alsa-devel@alsa-project.org, Lucas Tanure <tanureal@opensource.cirrus.com>, patches@opensource.cirrus.com, Takashi Iwai <tiwai@suse.com>, linux-kernel@vger.kernel.org Subject: Re: [PATCH 13/27] ALSA: hda/cs8409: Dont disable I2C clock between consecutive accesses Date: Tue, 27 Jul 2021 08:47:47 +0200 [thread overview] Message-ID: <s5h4kcgfe24.wl-tiwai@suse.de> (raw) In-Reply-To: <20210726174640.6390-14-vitalyr@opensource.cirrus.com> On Mon, 26 Jul 2021 19:46:26 +0200, Vitaly Rodionov wrote: > > From: Lucas Tanure <tanureal@opensource.cirrus.com> > > Only disable I2C clock 25 ms after not being used This is error-prone and might be racy, I'm afraid. e.g. the offloaded work isn't canceled when unbinding the driver, so it'll lead to a use-after-free. And the work doesn't take a mutex so it may conflict with re-enabling side. Above all, there is no description "why" this has to be done so. Is it about the performance, or any other reason? thanks, Takashi > > Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com> > Signed-off-by: Vitaly Rodionov <vitalyr@opensource.cirrus.com> > --- > sound/pci/hda/patch_cs8409.c | 42 +++++++++++++++++++++++------------- > sound/pci/hda/patch_cs8409.h | 4 ++++ > 2 files changed, 31 insertions(+), 15 deletions(-) > > diff --git a/sound/pci/hda/patch_cs8409.c b/sound/pci/hda/patch_cs8409.c > index 08205c19698c..335bcdc69106 100644 > --- a/sound/pci/hda/patch_cs8409.c > +++ b/sound/pci/hda/patch_cs8409.c > @@ -53,6 +53,7 @@ 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; > snd_hda_gen_spec_init(&spec->gen); > > @@ -72,21 +73,34 @@ 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); > + > + cs8409_vendor_coef_set(spec->codec, 0x0, > + cs8409_vendor_coef_get(spec->codec, 0x0) & 0xfffffff7); > + spec->i2c_clck_enabled = 0; > +} > + > +/* > * cs8409_enable_i2c_clock - Enable I2C clocks > * @codec: the codec instance > - * @enable: Enable or disable I2C clocks > - * > * Enable or Disable I2C clocks. > */ > -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); > > - retval = cs8409_vendor_coef_get(codec, 0x0); > - newval = (enable) ? (retval | 0x8) : (retval & 0xfffffff7); > - cs8409_vendor_coef_set(codec, 0x0, newval); > + if (!spec->i2c_clck_enabled) { > + cs8409_vendor_coef_set(codec, 0x0, cs8409_vendor_coef_get(codec, 0x0) | 0x8); > + spec->i2c_clck_enabled = 1; > + } > + queue_delayed_work(system_power_efficient_wq, &spec->i2c_clk_work, msecs_to_jiffies(25)); > } > > /** > @@ -134,7 +148,7 @@ static int cs8409_i2c_read(struct hda_codec *codec, unsigned int i2c_address, un > if (spec->cs42l42_suspended) > return -EPERM; > > - cs8409_enable_i2c_clock(codec, 1); > + cs8409_enable_i2c_clock(codec); > cs8409_vendor_coef_set(codec, CS8409_I2C_ADDR, i2c_address); > > if (paged) { > @@ -157,8 +171,6 @@ static int cs8409_i2c_read(struct hda_codec *codec, unsigned int i2c_address, un > /* Register in bits 15-8 and the data in 7-0 */ > read_data = cs8409_vendor_coef_get(codec, CS8409_I2C_QREAD); > > - cs8409_enable_i2c_clock(codec, 0); > - > return read_data & 0x0ff; > } > > @@ -182,7 +194,7 @@ static int cs8409_i2c_write(struct hda_codec *codec, unsigned int i2c_address, u > if (spec->cs42l42_suspended) > return -EPERM; > > - cs8409_enable_i2c_clock(codec, 1); > + cs8409_enable_i2c_clock(codec); > cs8409_vendor_coef_set(codec, CS8409_I2C_ADDR, i2c_address); > > if (paged) { > @@ -203,8 +215,6 @@ static int cs8409_i2c_write(struct hda_codec *codec, unsigned int i2c_address, u > return -EIO; > } > > - cs8409_enable_i2c_clock(codec, 0); > - > return 0; > } > > @@ -705,6 +715,8 @@ void cs8409_cs42l42_fixups(struct hda_codec *codec, const struct hda_fixup *fix, > spec->cs42l42_mic_jack_in = 0; > spec->cs42l42_suspended = 1; > > + INIT_DELAYED_WORK(&spec->i2c_clk_work, cs8409_disable_i2c_clock); > + > /* Basic initial sequence for specific hw configuration */ > snd_hda_sequence_write(codec, cs8409_cs42l42_init_verbs); > > diff --git a/sound/pci/hda/patch_cs8409.h b/sound/pci/hda/patch_cs8409.h > index bf0e8a4cc4cc..542582c213d2 100644 > --- a/sound/pci/hda/patch_cs8409.h > +++ b/sound/pci/hda/patch_cs8409.h > @@ -11,6 +11,7 @@ > > #include <linux/pci.h> > #include <sound/tlv.h> > +#include <linux/workqueue.h> > #include <sound/hda_codec.h> > #include "hda_local.h" > #include "hda_auto_parser.h" > @@ -267,6 +268,7 @@ struct cs8409_cir_param { > > struct cs8409_spec { > struct hda_gen_spec gen; > + struct hda_codec *codec; > > unsigned int gpio_mask; > unsigned int gpio_dir; > @@ -278,6 +280,8 @@ struct cs8409_spec { > s8 vol[CS42L42_VOLUMES]; > > struct mutex cs8409_i2c_mux; > + unsigned int i2c_clck_enabled; > + struct delayed_work i2c_clk_work; > > /* verb exec op override */ > int (*exec_verb)(struct hdac_device *dev, unsigned int cmd, unsigned int flags, > -- > 2.25.1 >
next prev parent reply other threads:[~2021-07-27 6:47 UTC|newest] Thread overview: 63+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-07-26 17:46 [PATCH 00/27] ALSA: hda/cirrus: Split generic cirrus HDA codecs and CS8490 bridge into separate modules Vitaly Rodionov 2021-07-26 17:46 ` Vitaly Rodionov 2021-07-26 17:46 ` [PATCH 01/27] ALSA: hda/cirrus: Move CS8409 HDA bridge to separate module Vitaly Rodionov 2021-07-26 17:46 ` Vitaly Rodionov 2021-07-27 6:39 ` Takashi Iwai 2021-07-27 6:39 ` Takashi Iwai 2021-07-26 17:46 ` [PATCH 02/27] ALSA: hda/cs8409: Move arrays of configuration to a new file Vitaly Rodionov 2021-07-26 17:46 ` Vitaly Rodionov 2021-07-26 17:46 ` [PATCH 03/27] ALSA: hda/cs8409: Use enums for register names and coefficients Vitaly Rodionov 2021-07-26 17:46 ` Vitaly Rodionov 2021-07-26 17:46 ` [PATCH 04/27] ALSA: hda/cs8409: Mask all CS42L42 interrupts on initialization Vitaly Rodionov 2021-07-26 17:46 ` Vitaly Rodionov 2021-07-26 17:46 ` [PATCH 05/27] ALSA: hda/cs8409: Reduce HS pops/clicks for Cyborg Vitaly Rodionov 2021-07-26 17:46 ` Vitaly Rodionov 2021-07-26 17:46 ` [PATCH 06/27] ALSA: hda/cs8409: Disable unnecessary Ring Sense for Cyborg/Warlock/Bullseye Vitaly Rodionov 2021-07-26 17:46 ` Vitaly Rodionov 2021-07-26 17:46 ` [PATCH 07/27] ALSA: hda/cs8409: Disable unsolicited responses during suspend Vitaly Rodionov 2021-07-26 17:46 ` Vitaly Rodionov 2021-07-26 17:46 ` [PATCH 08/27] ALSA: hda/cs8409: Disable unsolicited response for the first boot Vitaly Rodionov 2021-07-26 17:46 ` Vitaly Rodionov 2021-07-26 17:46 ` [PATCH 09/27] ALSA: hda/cs8409: Mask CS42L42 wake events Vitaly Rodionov 2021-07-26 17:46 ` Vitaly Rodionov 2021-07-26 17:46 ` [PATCH 10/27] ALSA: hda/cs8409: Simplify CS42L42 jack detect Vitaly Rodionov 2021-07-26 17:46 ` Vitaly Rodionov 2021-07-26 17:46 ` [PATCH 11/27] ALSA: hda/cs8409: Prevent I2C access during suspend time Vitaly Rodionov 2021-07-26 17:46 ` Vitaly Rodionov 2021-07-26 17:46 ` [PATCH 12/27] ALSA: hda/cs8409: Generalize volume controls Vitaly Rodionov 2021-07-26 17:46 ` Vitaly Rodionov 2021-07-26 17:46 ` [PATCH 13/27] ALSA: hda/cs8409: Dont disable I2C clock between consecutive accesses Vitaly Rodionov 2021-07-26 17:46 ` Vitaly Rodionov 2021-07-27 6:47 ` Takashi Iwai [this message] 2021-07-27 6:47 ` Takashi Iwai 2021-07-26 17:46 ` [PATCH 14/27] ALSA: hda/cs8409: Avoid setting the same I2C address for every access Vitaly Rodionov 2021-07-26 17:46 ` Vitaly Rodionov 2021-07-26 17:46 ` [PATCH 15/27] ALSA: hda/cs8409: Avoid re-setting the same page as the last access Vitaly Rodionov 2021-07-26 17:46 ` Vitaly Rodionov 2021-07-26 17:46 ` [PATCH 16/27] ALSA: hda/cs8409: Support i2c bulk read/write functions Vitaly Rodionov 2021-07-26 17:46 ` Vitaly Rodionov 2021-07-26 17:46 ` [PATCH 17/27] ALSA: hda/cs8409: Separate CS8409, CS42L42 and project functions Vitaly Rodionov 2021-07-26 17:46 ` Vitaly Rodionov 2021-07-26 17:46 ` [PATCH 18/27] ALSA: hda/cs8409: Move codec properties to its own struct Vitaly Rodionov 2021-07-26 17:46 ` Vitaly Rodionov 2021-07-26 17:46 ` [PATCH 19/27] ALSA: hda/cs8409: Support multiple sub_codecs for Suspend/Resume/Unsol events Vitaly Rodionov 2021-07-26 17:46 ` Vitaly Rodionov 2021-07-26 17:46 ` [PATCH 20/27] ALSA: hda/cs8409: Add Support to disable jack type detection for CS42L42 Vitaly Rodionov 2021-07-26 17:46 ` Vitaly Rodionov 2021-07-26 17:46 ` [PATCH 21/27] ALSA: hda/cs8409: Add support for dolphin Vitaly Rodionov 2021-07-26 17:46 ` Vitaly Rodionov 2021-07-27 0:25 ` kernel test robot 2021-07-27 0:25 ` kernel test robot 2021-07-27 0:25 ` kernel test robot 2021-07-26 17:46 ` [PATCH 22/27] ALSA: hda/cs8409: Enable Full Scale Volume for Line Out Codec on Dolphin Vitaly Rodionov 2021-07-26 17:46 ` Vitaly Rodionov 2021-07-26 17:46 ` [PATCH 23/27] ALSA: hda/cs8409: Set fixed sample rate of 48kHz for CS42L42 Vitaly Rodionov 2021-07-26 17:46 ` Vitaly Rodionov 2021-07-26 17:46 ` [PATCH 24/27] ALSA: hda/cs8409: Use timeout rather than retries for I2C transaction waits Vitaly Rodionov 2021-07-26 17:46 ` Vitaly Rodionov 2021-07-26 17:46 ` [PATCH 25/27] ALSA: hda/cs8409: Remove unnecessary delays Vitaly Rodionov 2021-07-26 17:46 ` Vitaly Rodionov 2021-07-26 17:46 ` [PATCH 26/27] ALSA: hda/cs8409: Follow correct CS42L42 power down sequence for suspend Vitaly Rodionov 2021-07-26 17:46 ` Vitaly Rodionov 2021-07-26 17:46 ` [PATCH 27/27] ALSA: hda/cs8409: Unmute/Mute codec when stream starts/stops Vitaly Rodionov 2021-07-26 17:46 ` Vitaly Rodionov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=s5h4kcgfe24.wl-tiwai@suse.de \ --to=tiwai@suse.de \ --cc=alsa-devel@alsa-project.org \ --cc=linux-kernel@vger.kernel.org \ --cc=patches@opensource.cirrus.com \ --cc=perex@perex.cz \ --cc=tanureal@opensource.cirrus.com \ --cc=tiwai@suse.com \ --cc=vitalyr@opensource.cirrus.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.