| Message ID | 1499498863-6865-1-git-send-email-dev@kresin.me (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hi, On 8 July 2017 at 09:27, Mathias Kresin <dev@kresin.me> wrote: > If clk_get returns an error, rt2x00dev->clk is set to NULL. In > contrast to the common clock framework provided clk_get_rate(), at > least the ramips and bcm63xx legacy implementation of the clk API > access the rate member of the clk struct without a NULL check. This > results into a kernel panic if we do not have a (SoC) clock. > > Call clk_get_rate only if we have a clock to fix the issues. This > approach is similar to what is done in the kernel at various places. > Usually clk_get_rate() is only called if clk_get_rate() doesn't return Did you mean clk_get() as the second one? > an error. > > Signed-off-by: Mathias Kresin <dev@kresin.me> Tbh, I'd rather have this fixed at the source than adding a workaround to consumers, to have a consistent API across implementations (with drivers/clk/clk.c as the reference). And there don't seem that many, at least searching for clk_get_rate gave me only two handful of implementations of which many already check for NULL. Regards Jonas
08.07.2017 13:15, Jonas Gorski: > Hi, > > On 8 July 2017 at 09:27, Mathias Kresin <dev@kresin.me> wrote: >> If clk_get returns an error, rt2x00dev->clk is set to NULL. In >> contrast to the common clock framework provided clk_get_rate(), at >> least the ramips and bcm63xx legacy implementation of the clk API >> access the rate member of the clk struct without a NULL check. This >> results into a kernel panic if we do not have a (SoC) clock. >> >> Call clk_get_rate only if we have a clock to fix the issues. This >> approach is similar to what is done in the kernel at various places. >> Usually clk_get_rate() is only called if clk_get_rate() doesn't return > > Did you mean clk_get() as the second one? Yes I do. ... is only called if clk_get() doesn't return an error. > >> an error. >> >> Signed-off-by: Mathias Kresin <dev@kresin.me> > > Tbh, I'd rather have this fixed at the source than adding a workaround > to consumers, to have a consistent API across implementations (with > drivers/clk/clk.c as the reference). > > And there don't seem that many, at least searching for clk_get_rate > gave me only two handful of implementations of which many already > check for NULL. I do not necessarily see an error in the archs legacy clock implementation. It rather looks like it works with the common clock framework due the lucky coincident that someone has added (an extra) null check. I only had a brief look at the common clock framework, but it seams to me one should not pass the error returned by clk_get() and/or call clk_get_rate() at all if there was an error. That is what I've already seen and mentioned in the commit message. The only difference here is that we do not have the error check and the clk_get_rate() call in the same function. rt2800_clk_is_20mhz() isn't aware that clk_get() failed. rt2x00dev->clk is set back to NULL in rt2x00soc_probe() in case of an clk_get error. Mathias
On Sat, Jul 08, 2017 at 03:37:11PM +0200, Mathias Kresin wrote: > 08.07.2017 13:15, Jonas Gorski: > >to consumers, to have a consistent API across implementations (with > >drivers/clk/clk.c as the reference). > > > >And there don't seem that many, at least searching for clk_get_rate > >gave me only two handful of implementations of which many already > >check for NULL. > > I do not necessarily see an error in the archs legacy clock > implementation. It rather looks like it works with the common clock > framework due the lucky coincident that someone has added (an extra) > null check. > > I only had a brief look at the common clock framework, but it seams > to me one should not pass the error returned by clk_get() and/or > call clk_get_rate() at all if there was an error. That is what I've > already seen and mentioned in the commit message. > The only difference here is that we do not have the error check and > the clk_get_rate() call in the same function. rt2800_clk_is_20mhz() > isn't aware that clk_get() failed. rt2x00dev->clk is set back to > NULL in rt2x00soc_probe() in case of an clk_get error. If we need to check clk before clk_get_rate() call, we can remove NULL assignment from rt2x00soc_probe() and use IS_ERR_OR_NULL() on rt2800_clk_is_20mhz() . Thanks Stanislaw
Mathias Kresin <dev@kresin.me> writes: > If clk_get returns an error, rt2x00dev->clk is set to NULL. In > contrast to the common clock framework provided clk_get_rate(), at > least the ramips and bcm63xx legacy implementation of the clk API > access the rate member of the clk struct without a NULL check. This > results into a kernel panic if we do not have a (SoC) clock. > > Call clk_get_rate only if we have a clock to fix the issues. This > approach is similar to what is done in the kernel at various places. > Usually clk_get_rate() is only called if clk_get_rate() doesn't return > an error. > > Signed-off-by: Mathias Kresin <dev@kresin.me> > --- > > Resend, the first mail had the wrong list in cc. Then you should mark this as v2: https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#patch_version_missing
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c index d11c7b2..2a525b9 100644 --- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c +++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c @@ -2059,6 +2059,9 @@ static void rt2800_config_lna_gain(struct rt2x00_dev *rt2x00dev, static inline bool rt2800_clk_is_20mhz(struct rt2x00_dev *rt2x00dev) { + if (!rt2x00dev->clk) + return 0; + return clk_get_rate(rt2x00dev->clk) == 20000000; }
If clk_get returns an error, rt2x00dev->clk is set to NULL. In contrast to the common clock framework provided clk_get_rate(), at least the ramips and bcm63xx legacy implementation of the clk API access the rate member of the clk struct without a NULL check. This results into a kernel panic if we do not have a (SoC) clock. Call clk_get_rate only if we have a clock to fix the issues. This approach is similar to what is done in the kernel at various places. Usually clk_get_rate() is only called if clk_get_rate() doesn't return an error. Signed-off-by: Mathias Kresin <dev@kresin.me> --- Resend, the first mail had the wrong list in cc. drivers/net/wireless/ralink/rt2x00/rt2800lib.c | 3 +++ 1 file changed, 3 insertions(+)