| Message ID | f31523d7270d4a1f82d96b7891ed13e6@bfs.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | AW: [PATCH 2/2] tty: serial: samsung_tty: Fix a memory leak in s3c24xx_serial_getclk() when iterating clk | expand |
On 09/06/2023 10:57, Walter Harms wrote: > > while we are here .... > > perhaps INT_MAX from kernel.h ? > > int deviation = (1 << 30) - 1; > > the part before looks a bit strange > > if (ourport->info->has_divslot) { > unsigned long div = rate / req_baud; > > /* The UDIVSLOT register on the newer UARTs allows us to > * get a divisor adjustment of 1/16th on the baud clock. > * > * We don't keep the UDIVSLOT value (the 16ths we > * calculated by not multiplying the baud by 16) as it > * is easy enough to recalculate. > */ > > quot = div / 16; > baud = rate / div; > because > baud=rate/rate/req_baud = req_baud > can this be simplyfied ? (or is the numeric required ?) > > > Homebrew abs() kernel.h has a abs() can we use it here ? > > if (calc_deviation < 0) > calc_deviation = -calc_deviation; > > to the patch: > > + /* > + * If we find a better clk, release the previous one, if > + * any. > + */ > + if (!IS_ERR(*best_clk)) > + clk_put(*best_clk); > > the intentions are good. *best_clk is user supplied (and should be NULL) > filled & released in the next round but IMHO must be valid (is clk). > so no need to check. (ntl clk_put seems to handle NULL and ERR ) > if (!clk || WARN_ON_ONCE(IS_ERR(clk))) > return; Don't top-post. Anyway, I don't understand what you want to say here. Best regards, Krzysztof
Le 09/06/2023 à 10:57, Walter Harms a écrit : > > while we are here .... > > perhaps INT_MAX from kernel.h ? from include/vdso/limits.h > int deviation = (1 << 30) - 1; I don't know the initial intent for this value, but it is not the same as MAX_INT. > > the part before looks a bit strange > > if (ourport->info->has_divslot) { > unsigned long div = rate / req_baud; > > /* The UDIVSLOT register on the newer UARTs allows us to > * get a divisor adjustment of 1/16th on the baud clock. > * > * We don't keep the UDIVSLOT value (the 16ths we > * calculated by not multiplying the baud by 16) as it > * is easy enough to recalculate. > */ > > quot = div / 16; > baud = rate / div; > because > baud=rate/rate/req_baud = req_baud In math yes. In integer computation, no. rate = 20000 req_baud = 9600 div = rate / req_baud ==> 2 baud = rate / div; ==> 20000 / 2 = 10000 9600 <> 10000 I don't know if it is the intent, but it is the way it works. And knowing that: calc_deviation = req_baud - baud; I guess that it is the way it is expected to work. With your reasoning, calc_deviation would be always 0. > can this be simplyfied ? (or is the numeric required ?) > > > Homebrew abs() kernel.h has a abs() can we use it here ? include/linux/math.h > > if (calc_deviation < 0) > calc_deviation = -calc_deviation; Ok, why not. > > to the patch: > > + /* > + * If we find a better clk, release the previous one, if > + * any. > + */ > + if (!IS_ERR(*best_clk)) > + clk_put(*best_clk); > > the intentions are good. *best_clk is user supplied (and should be NULL) ??? Why should it be NULL? There is only one caller, and the value id &clk, knowing that: struct clk *clk = ERR_PTR(-EINVAL); The code could be changed to have an initial NULL value, but it would'nt bring that much added value, in my PoV. It would only save a test which is just fine as-is. > filled & released in the next round but IMHO must be valid (is clk). > so no need to check. (ntl clk_put seems to handle NULL and ERR ) > if (!clk || WARN_ON_ONCE(IS_ERR(clk))) > return; My point with "if (!IS_ERR(*best_clk))" is to handle the initial iteration when *best_clk is ERR_PTR(-EINVAL). clk_put() can handle it, but it would WARN in the normal path, so it sounds strange to me. CJ > > JM2C > wh > ________________________________________ > Von: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > Gesendet: Freitag, 9. Juni 2023 06:45:39 > An: Krzysztof Kozlowski; Alim Akhtar; Greg Kroah-Hartman; Jiri Slaby; Thomas Abraham; Kukjin Kim > Cc: linux-kernel@vger.kernel.org; kernel-janitors@vger.kernel.org; Christophe JAILLET; linux-arm-kernel@lists.infradead.org; linux-samsung-soc@vger.kernel.org; linux-serial@vger.kernel.org > Betreff: [PATCH 2/2] tty: serial: samsung_tty: Fix a memory leak in s3c24xx_serial_getclk() when iterating clk > > When the best clk is searched, we iterate over all possible clk. > > If we find a better match, the previous one, if any, needs to be freed. > If a better match has already been found, we still need to free the new > one, otherwise it leaks. > > Fixes: 5f5a7a5578c5 ("serial: samsung: switch to clkdev based clock lookup") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > This patch is speculative. Review with care. > > I think that some clk_put() are also missing somewhere else in the driver > but won't be able to investigate further. > --- > drivers/tty/serial/samsung_tty.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c > index dd751e7010e3..c07877dd25fa 100644 > --- a/drivers/tty/serial/samsung_tty.c > +++ b/drivers/tty/serial/samsung_tty.c > @@ -1488,10 +1488,18 @@ static unsigned int s3c24xx_serial_getclk(struct s3c24xx_uart_port *ourport, > calc_deviation = -calc_deviation; > > if (calc_deviation < deviation) { > + /* > + * If we find a better clk, release the previous one, if > + * any. > + */ > + if (!IS_ERR(*best_clk)) > + clk_put(*best_clk); > *best_clk = clk; > best_quot = quot; > *clk_num = cnt; > deviation = calc_deviation; > + } else { > + clk_put(clk); > } > } > > -- > 2.34.1 > >
diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c index dd751e7010e3..c07877dd25fa 100644 --- a/drivers/tty/serial/samsung_tty.c +++ b/drivers/tty/serial/samsung_tty.c @@ -1488,10 +1488,18 @@ static unsigned int s3c24xx_serial_getclk(struct s3c24xx_uart_port *ourport, calc_deviation = -calc_deviation; if (calc_deviation < deviation) { + /* + * If we find a better clk, release the previous one, if + * any. + */ + if (!IS_ERR(*best_clk)) + clk_put(*best_clk); *best_clk = clk; best_quot = quot; *clk_num = cnt; deviation = calc_deviation; + } else { + clk_put(clk); } }
while we are here .... perhaps INT_MAX from kernel.h ? int deviation = (1 << 30) - 1; the part before looks a bit strange if (ourport->info->has_divslot) { unsigned long div = rate / req_baud; /* The UDIVSLOT register on the newer UARTs allows us to * get a divisor adjustment of 1/16th on the baud clock. * * We don't keep the UDIVSLOT value (the 16ths we * calculated by not multiplying the baud by 16) as it * is easy enough to recalculate. */ quot = div / 16; baud = rate / div; because baud=rate/rate/req_baud = req_baud can this be simplyfied ? (or is the numeric required ?) Homebrew abs() kernel.h has a abs() can we use it here ? if (calc_deviation < 0) calc_deviation = -calc_deviation; to the patch: + /* + * If we find a better clk, release the previous one, if + * any. + */ + if (!IS_ERR(*best_clk)) + clk_put(*best_clk); the intentions are good. *best_clk is user supplied (and should be NULL) filled & released in the next round but IMHO must be valid (is clk). so no need to check. (ntl clk_put seems to handle NULL and ERR ) if (!clk || WARN_ON_ONCE(IS_ERR(clk))) return; JM2C wh