From: Peter Seiderer <ps.report@gmx.net>
To: Mark Brown <broonie@kernel.org>
Cc: alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org,
Takashi Iwai <tiwai@suse.com>,
Liam Girdwood <lgirdwood@gmail.com>,
Annaliese McDermond <nh6z@nh6z.net>
Subject: Re: [alsa-devel] [PATCH v1] ASoC: tlv320aic32x4: handle regmap_read error gracefully
Date: Thu, 9 Jan 2020 23:21:25 +0100 [thread overview]
Message-ID: <20200109232125.6ecd6cf7@gmx.net> (raw)
In-Reply-To: <20200109203819.GG3702@sirena.org.uk>
Hello Mark,
On Thu, 9 Jan 2020 20:38:19 +0000, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Dec 27, 2019 at 04:20:56PM +0100, Peter Seiderer wrote:
> > @@ -338,7 +338,8 @@ static unsigned long clk_aic32x4_div_recalc_rate(struct clk_hw *hw,
>
> > unsigned int val;
>
> > - regmap_read(div->regmap, div->reg, &val);
> > + if (regmap_read(div->regmap, div->reg, &val))
Unrelated to your question, but I would change this line (on next patch
iteration) to (as all other return value checked places for regmap_read
in the same file):
if (regmap_read(div->regmap, div->reg, &val) < 0)
> > + return 0;
>
> Is this the best fix - shouldn't we be returning an error here? We
> don't know what the value programmed into the device actually is so zero
> might be wrong, and we still have the risk that the value we read from
> the device may be zero if the device is misprogrammed.
clk_aic32x4_div_recalc_rate() is used for clk_ops aic32x4_div_ops.recalc_rate,
did not check/or see on first sight if there is a error handling on the usage
of recalc_rate, but did take a look at some other places where the error
handling seems to be to return zero, e.g. sound/soc/codecs/da7219.c
sound/soc/intel/skylake/skl-ssp-clk.c, etc.
The error occurred with linux-5.3.18, with the earlier versions on regmap_read
failure val was (by chance) near 2^31 and evaluated with (val & AIC32X4_DIV_MASK)
to 96 (or similar)...but with 5.3.18 evaluated to 0 and the line
return DIV_ROUND_UP(parent_rate, val & AIC32X4_DIV_MASK);
produced the 'Division by zero'...
Regards,
Peter
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
WARNING: multiple messages have this Message-ID (diff)
From: Peter Seiderer <ps.report@gmx.net>
To: Mark Brown <broonie@kernel.org>
Cc: linux-kernel@vger.kernel.org, alsa-devel@alsa-project.org,
Annaliese McDermond <nh6z@nh6z.net>,
Takashi Iwai <tiwai@suse.com>, Jaroslav Kysela <perex@perex.cz>,
Liam Girdwood <lgirdwood@gmail.com>
Subject: Re: [PATCH v1] ASoC: tlv320aic32x4: handle regmap_read error gracefully
Date: Thu, 9 Jan 2020 23:21:25 +0100 [thread overview]
Message-ID: <20200109232125.6ecd6cf7@gmx.net> (raw)
In-Reply-To: <20200109203819.GG3702@sirena.org.uk>
Hello Mark,
On Thu, 9 Jan 2020 20:38:19 +0000, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Dec 27, 2019 at 04:20:56PM +0100, Peter Seiderer wrote:
> > @@ -338,7 +338,8 @@ static unsigned long clk_aic32x4_div_recalc_rate(struct clk_hw *hw,
>
> > unsigned int val;
>
> > - regmap_read(div->regmap, div->reg, &val);
> > + if (regmap_read(div->regmap, div->reg, &val))
Unrelated to your question, but I would change this line (on next patch
iteration) to (as all other return value checked places for regmap_read
in the same file):
if (regmap_read(div->regmap, div->reg, &val) < 0)
> > + return 0;
>
> Is this the best fix - shouldn't we be returning an error here? We
> don't know what the value programmed into the device actually is so zero
> might be wrong, and we still have the risk that the value we read from
> the device may be zero if the device is misprogrammed.
clk_aic32x4_div_recalc_rate() is used for clk_ops aic32x4_div_ops.recalc_rate,
did not check/or see on first sight if there is a error handling on the usage
of recalc_rate, but did take a look at some other places where the error
handling seems to be to return zero, e.g. sound/soc/codecs/da7219.c
sound/soc/intel/skylake/skl-ssp-clk.c, etc.
The error occurred with linux-5.3.18, with the earlier versions on regmap_read
failure val was (by chance) near 2^31 and evaluated with (val & AIC32X4_DIV_MASK)
to 96 (or similar)...but with 5.3.18 evaluated to 0 and the line
return DIV_ROUND_UP(parent_rate, val & AIC32X4_DIV_MASK);
produced the 'Division by zero'...
Regards,
Peter
next prev parent reply other threads:[~2020-01-09 22:22 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-27 15:20 [alsa-devel] [PATCH v1] ASoC: tlv320aic32x4: handle regmap_read error gracefully Peter Seiderer
2019-12-27 15:20 ` Peter Seiderer
2019-12-27 22:52 ` [alsa-devel] " Mark Brown
2019-12-27 22:52 ` Mark Brown
2020-01-06 20:45 ` [alsa-devel] " Peter Seiderer
2020-01-06 20:45 ` Peter Seiderer
2020-01-09 20:35 ` [alsa-devel] " Mark Brown
2020-01-09 20:35 ` Mark Brown
2020-01-09 20:38 ` [alsa-devel] " Mark Brown
2020-01-09 20:38 ` Mark Brown
2020-01-09 22:21 ` Peter Seiderer [this message]
2020-01-09 22:21 ` Peter Seiderer
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=20200109232125.6ecd6cf7@gmx.net \
--to=ps.report@gmx.net \
--cc=alsa-devel@alsa-project.org \
--cc=broonie@kernel.org \
--cc=lgirdwood@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=nh6z@nh6z.net \
--cc=tiwai@suse.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: link
Be 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.