* [PATCH v2] serial: samsung: Fixed wrong comparison for baudclk_rate @ 2012-05-15 12:37 Kyoungil Kim 2012-05-15 12:45 ` Russell King - ARM Linux 0 siblings, 1 reply; 3+ messages in thread From: Kyoungil Kim @ 2012-05-15 12:37 UTC (permalink / raw) To: linux-arm-kernel, linux-serial, linux-samsung-soc Cc: 'Kukjin Kim', 'Alan Cox', 'Kyoungil Kim', 'Jun-Ho Yoon' port->baudclk_rate should be compared to the rate of port->baudclk, because port->baudclk_rate was assigned as the rate of port->baudclk previously. So to check that the current baudclk rate is same as previous rate, the target of comparison sholud be the rate of port->baudclk. Signed-off-by: Jun-Ho, Yoon <junho78.yoon@samsung.com> Signed-off-by: Kyoungil Kim <ki0351.kim@samsung.com> --- drivers/tty/serial/samsung.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c index d8b0aee..6a6a86a 100644 --- a/drivers/tty/serial/samsung.c +++ b/drivers/tty/serial/samsung.c @@ -1014,10 +1014,10 @@ static int s3c24xx_serial_cpufreq_transition(struct notifier_block *nb, * a disturbance in the clock-rate over the change. */ - if (IS_ERR(port->clk)) + if (IS_ERR(port->baudclk) || port->baudclk == NULL) goto exit; - if (port->baudclk_rate == clk_get_rate(port->clk)) + if (port->baudclk_rate == clk_get_rate(port->baudclk)) goto exit; if (val == CPUFREQ_PRECHANGE) { -- 1.7.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] serial: samsung: Fixed wrong comparison for baudclk_rate 2012-05-15 12:37 [PATCH v2] serial: samsung: Fixed wrong comparison for baudclk_rate Kyoungil Kim @ 2012-05-15 12:45 ` Russell King - ARM Linux 2012-05-17 6:55 ` Kukjin Kim 0 siblings, 1 reply; 3+ messages in thread From: Russell King - ARM Linux @ 2012-05-15 12:45 UTC (permalink / raw) To: Kyoungil Kim Cc: linux-arm-kernel, linux-serial, linux-samsung-soc, 'Kukjin Kim', 'Jun-Ho Yoon', 'Alan Cox' On Tue, May 15, 2012 at 09:37:16PM +0900, Kyoungil Kim wrote: > port->baudclk_rate should be compared to the rate of port->baudclk, > because port->baudclk_rate was assigned as the rate of port->baudclk previously. > So to check that the current baudclk rate is same as previous rate, > the target of comparison sholud be the rate of port->baudclk. > > Signed-off-by: Jun-Ho, Yoon <junho78.yoon@samsung.com> > Signed-off-by: Kyoungil Kim <ki0351.kim@samsung.com> > --- > drivers/tty/serial/samsung.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c > index d8b0aee..6a6a86a 100644 > --- a/drivers/tty/serial/samsung.c > +++ b/drivers/tty/serial/samsung.c > @@ -1014,10 +1014,10 @@ static int s3c24xx_serial_cpufreq_transition(struct notifier_block *nb, > * a disturbance in the clock-rate over the change. > */ > > - if (IS_ERR(port->clk)) > + if (IS_ERR(port->baudclk) || port->baudclk == NULL) Still no. Why are you wanting to detect a NULL baud clock? As I said, drivers have no business interpreting anything but IS_ERR(clk) as being an error. They should not make any other assumptions. Now that I look at this driver, it makes this mistake all over the place. This needs to be fixed. Something like the below should do it. Please check. diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c index d8b0aee..6a952b1 100644 --- a/drivers/tty/serial/samsung.c +++ b/drivers/tty/serial/samsung.c @@ -529,7 +529,7 @@ static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level, switch (level) { case 3: - if (!IS_ERR(ourport->baudclk) && ourport->baudclk != NULL) + if (!IS_ERR(ourport->baudclk)) clk_disable(ourport->baudclk); clk_disable(ourport->clk); @@ -538,7 +538,7 @@ static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level, case 0: clk_enable(ourport->clk); - if (!IS_ERR(ourport->baudclk) && ourport->baudclk != NULL) + if (!IS_ERR(ourport->baudclk)) clk_enable(ourport->baudclk); break; @@ -713,9 +713,9 @@ static void s3c24xx_serial_set_termios(struct uart_port *port, if (ourport->baudclk != clk) { s3c24xx_serial_setsource(port, clk_sel); - if (ourport->baudclk != NULL && !IS_ERR(ourport->baudclk)) { + if (!IS_ERR(ourport->baudclk)) { clk_disable(ourport->baudclk); - ourport->baudclk = NULL; + ourport->baudclk = ERR_PTR(-EINVAL); } clk_enable(clk); @@ -1160,6 +1160,9 @@ static ssize_t s3c24xx_serial_show_clksrc(struct device *dev, struct uart_port *port = s3c24xx_dev_to_port(dev); struct s3c24xx_uart_port *ourport = to_ourport(port); + if (IS_ERR(ourport->baudclk)) + return -EINVAL; + return snprintf(buf, PAGE_SIZE, "* %s\n", ourport->baudclk->name); } @@ -1200,6 +1203,7 @@ static int s3c24xx_serial_probe(struct platform_device *pdev) return -ENODEV; } + ourport->baudclk = ERR_PTR(-EINVAL); ourport->info = ourport->drv_data->info; ourport->cfg = (pdev->dev.platform_data) ? (struct s3c2410_uartcfg *)pdev->dev.platform_data : ^ permalink raw reply related [flat|nested] 3+ messages in thread
* RE: [PATCH v2] serial: samsung: Fixed wrong comparison for baudclk_rate 2012-05-15 12:45 ` Russell King - ARM Linux @ 2012-05-17 6:55 ` Kukjin Kim 0 siblings, 0 replies; 3+ messages in thread From: Kukjin Kim @ 2012-05-17 6:55 UTC (permalink / raw) To: 'Russell King - ARM Linux', 'Kyoungil Kim' Cc: linux-arm-kernel, linux-serial, linux-samsung-soc, 'Jun-Ho Yoon', 'Alan Cox' Russell King - ARM Linux wrote: > > On Tue, May 15, 2012 at 09:37:16PM +0900, Kyoungil Kim wrote: > > port->baudclk_rate should be compared to the rate of port->baudclk, > > because port->baudclk_rate was assigned as the rate of port->baudclk > previously. > > So to check that the current baudclk rate is same as previous rate, > > the target of comparison sholud be the rate of port->baudclk. > > > > Signed-off-by: Jun-Ho, Yoon <junho78.yoon@samsung.com> > > Signed-off-by: Kyoungil Kim <ki0351.kim@samsung.com> > > --- > > drivers/tty/serial/samsung.c | 4 ++-- > > 1 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c > > index d8b0aee..6a6a86a 100644 > > --- a/drivers/tty/serial/samsung.c > > +++ b/drivers/tty/serial/samsung.c > > @@ -1014,10 +1014,10 @@ static int > s3c24xx_serial_cpufreq_transition(struct notifier_block *nb, > > * a disturbance in the clock-rate over the change. > > */ > > > > - if (IS_ERR(port->clk)) > > + if (IS_ERR(port->baudclk) || port->baudclk == NULL) > > Still no. Why are you wanting to detect a NULL baud clock? > > As I said, drivers have no business interpreting anything but IS_ERR(clk) > as being an error. They should not make any other assumptions. > > Now that I look at this driver, it makes this mistake all over the place. > This needs to be fixed. Something like the below should do it. Please > check. > Russell, thanks for your kindly pointing out. Kyoungil, could you please re-submit for this as per Russell's suggestion (with Russell's patch)? Thanks. Best regards, Kgene. -- Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd. > diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c > index d8b0aee..6a952b1 100644 > --- a/drivers/tty/serial/samsung.c > +++ b/drivers/tty/serial/samsung.c > @@ -529,7 +529,7 @@ static void s3c24xx_serial_pm(struct uart_port *port, > unsigned int level, > > switch (level) { > case 3: > - if (!IS_ERR(ourport->baudclk) && ourport->baudclk != NULL) > + if (!IS_ERR(ourport->baudclk)) > clk_disable(ourport->baudclk); > > clk_disable(ourport->clk); > @@ -538,7 +538,7 @@ static void s3c24xx_serial_pm(struct uart_port *port, > unsigned int level, > case 0: > clk_enable(ourport->clk); > > - if (!IS_ERR(ourport->baudclk) && ourport->baudclk != NULL) > + if (!IS_ERR(ourport->baudclk)) > clk_enable(ourport->baudclk); > > break; > @@ -713,9 +713,9 @@ static void s3c24xx_serial_set_termios(struct > uart_port *port, > if (ourport->baudclk != clk) { > s3c24xx_serial_setsource(port, clk_sel); > > - if (ourport->baudclk != NULL && !IS_ERR(ourport->baudclk)) { > + if (!IS_ERR(ourport->baudclk)) { > clk_disable(ourport->baudclk); > - ourport->baudclk = NULL; > + ourport->baudclk = ERR_PTR(-EINVAL); > } > > clk_enable(clk); > @@ -1160,6 +1160,9 @@ static ssize_t s3c24xx_serial_show_clksrc(struct > device *dev, > struct uart_port *port = s3c24xx_dev_to_port(dev); > struct s3c24xx_uart_port *ourport = to_ourport(port); > > + if (IS_ERR(ourport->baudclk)) > + return -EINVAL; > + > return snprintf(buf, PAGE_SIZE, "* %s\n", ourport->baudclk->name); > } > > @@ -1200,6 +1203,7 @@ static int s3c24xx_serial_probe(struct > platform_device *pdev) > return -ENODEV; > } > > + ourport->baudclk = ERR_PTR(-EINVAL); > ourport->info = ourport->drv_data->info; > ourport->cfg = (pdev->dev.platform_data) ? > (struct s3c2410_uartcfg *)pdev->dev.platform_data : ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2012-05-17 6:55 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-05-15 12:37 [PATCH v2] serial: samsung: Fixed wrong comparison for baudclk_rate Kyoungil Kim 2012-05-15 12:45 ` Russell King - ARM Linux 2012-05-17 6:55 ` Kukjin Kim
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).