linux-kernel - Re: [PATCH 02/11] ASoC: ab8500: Revert back to find_next

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <s5h4n5zette.wl%tiwai@suse.de>
Date:	Mon, 23 Dec 2013 11:38:05 +0100
From:	Takashi Iwai <tiwai@...e.de>
To:	broonie@...nel.org
Cc:	Lee Jones <lee.jones@...aro.org>,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linus.walleij@...aro.org, alsa-devel@...a-project.org
Subject: Re: [PATCH 02/11] ASoC: ab8500: Revert back to find_next_bit()

At Thu, 19 Dec 2013 18:29:09 +0100,
Takashi Iwai wrote:
> 
> At Thu, 19 Dec 2013 16:48:24 +0000,
> Lee Jones wrote:
> > 
> > > > Commit '166a34d ASoC: ab8500: Fix invalid cast to long pointer'
> > > > rather carelessly converts find_next_bit() to fls() (find last
> > > > bit set), which are not the same.
> > > 
> > > Does it break on the real machines?
> > 
> > It does, that's how I found the bug.
> > 
> > > fls() behaves differently from find_next_bit(), of course, but in this
> > > case, it should work same in the end, since there are at most two
> > > bits.
> > 
> > Unfortunately it doesn't work at all. This patch brings audio back to
> > a working state. It took me the best part of a day to track down the
> > issue. :(
> 
> Hmm, then isn't the original code rather buggy?
> 
> Check the values set there by ffs(), fls() and the original
> find_next_bit(), especially whether find_next_bit() gives a valid
> value fitting with the mask bits.

Meanwhile, it's not good to keep the broken code in the upstream, and
the bug is obvious.  Below is the fixed patch, applicable with git am
scissors option.  Lee, let me know if this still doesn't fix.


thanks,

Takashi

-- 8< --
From: Takashi Iwai <tiwai@...e.de>
Subject: [PATCH] ASoC: ab8500: Back to first_find_bit() & co

ffs() and fls() give the off-by-one value in comparison with
find_*_bit() helpers, and find_first_bit() and find_next_bit() are
more intuitive in the case of two bits.  So, back to find_*_bit() but
call them properly without invalid cast.  This fixes the slot
assignment regression introduced by commit 166a34d27fca.

Signed-off-by: Takashi Iwai <tiwai@...e.de>
---
 sound/soc/codecs/ab8500-codec.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/sound/soc/codecs/ab8500-codec.c b/sound/soc/codecs/ab8500-codec.c
index 1ad92cbf0b24..cf53c1e2fabc 100644
--- a/sound/soc/codecs/ab8500-codec.c
+++ b/sound/soc/codecs/ab8500-codec.c
@@ -2242,6 +2242,7 @@ static int ab8500_codec_set_dai_tdm_slot(struct snd_soc_dai *dai,
 {
 	struct snd_soc_codec *codec = dai->codec;
 	unsigned int val, mask, slot, slots_active;
+	unsigned long tmp;
 
 	mask = BIT(AB8500_DIGIFCONF2_IF0WL0) |
 		BIT(AB8500_DIGIFCONF2_IF0WL1);
@@ -2308,21 +2309,22 @@ static int ab8500_codec_set_dai_tdm_slot(struct snd_soc_dai *dai,
 	dev_dbg(dai->codec->dev, "%s: Slots, active, TX: %d\n", __func__,
 		slots_active);
 
+	tmp = tx_mask;
 	switch (slots_active) {
 	case 0:
 		break;
 	case 1:
-		slot = ffs(tx_mask);
+		slot = find_first_bit(&tmp, 32);
 		snd_soc_update_bits(codec, AB8500_DASLOTCONF1, mask, slot);
 		snd_soc_update_bits(codec, AB8500_DASLOTCONF3, mask, slot);
 		snd_soc_update_bits(codec, AB8500_DASLOTCONF2, mask, slot);
 		snd_soc_update_bits(codec, AB8500_DASLOTCONF4, mask, slot);
 		break;
 	case 2:
-		slot = ffs(tx_mask);
+		slot = find_first_bit(&tmp, 32);
 		snd_soc_update_bits(codec, AB8500_DASLOTCONF1, mask, slot);
 		snd_soc_update_bits(codec, AB8500_DASLOTCONF3, mask, slot);
-		slot = fls(tx_mask);
+		slot = find_next_bit(&tmp, 32, slot + 1);
 		snd_soc_update_bits(codec, AB8500_DASLOTCONF2, mask, slot);
 		snd_soc_update_bits(codec, AB8500_DASLOTCONF4, mask, slot);
 		break;
@@ -2349,22 +2351,23 @@ static int ab8500_codec_set_dai_tdm_slot(struct snd_soc_dai *dai,
 	dev_dbg(dai->codec->dev, "%s: Slots, active, RX: %d\n", __func__,
 		slots_active);
 
+	tmp = rx_mask;
 	switch (slots_active) {
 	case 0:
 		break;
 	case 1:
-		slot = ffs(rx_mask);
+		slot = find_first_bit(&tmp, 32);
 		snd_soc_update_bits(codec, AB8500_ADSLOTSEL(slot),
 				AB8500_MASK_SLOT(slot),
 				AB8500_ADSLOTSELX_AD_OUT_TO_SLOT(AB8500_AD_OUT3, slot));
 		break;
 	case 2:
-		slot = ffs(rx_mask);
+		slot = find_first_bit(&tmp, 32);
 		snd_soc_update_bits(codec,
 				AB8500_ADSLOTSEL(slot),
 				AB8500_MASK_SLOT(slot),
 				AB8500_ADSLOTSELX_AD_OUT_TO_SLOT(AB8500_AD_OUT3, slot));
-		slot = fls(rx_mask);
+		slot = find_next_bit(&tmp, 32, slot + 1);
 		snd_soc_update_bits(codec,
 				AB8500_ADSLOTSEL(slot),
 				AB8500_MASK_SLOT(slot),
-- 
1.8.5.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/