re: serial: sc16is7xx

re: serial: sc16is7xx

[Dan Carpenter]

Hello Jon Ringle,

The patch dfeae619d781: "serial: sc16is7xx" from Apr 24, 2014, leads
to the following static checker warning:

	drivers/tty/serial/sc16is7xx.c:1168 sc16is7xx_probe()
	warn: 's->clk' isn't an ERR_PTR

drivers/tty/serial/sc16is7xx.c
  1167  out_clk:
  1168          if (!IS_ERR(s->clk))
                     ^^^^^^^^^^^^^
This check isn't needed because we return directly but also I don't
think we ever set s->clk so it is leaked on the remove() path as well.

  1169                  clk_disable_unprepare(s->clk);
  1170  
  1171          return ret;
  1172  }

regards,
dan carpenter

re: serial: sc16is7xx

[Jon Ringle]

On Fri, 25 Jul 2014, Dan Carpenter wrote:

> Hello Jon Ringle,
> 
> The patch dfeae619d781: "serial: sc16is7xx" from Apr 24, 2014, leads
> to the following static checker warning:
> 
> 	drivers/tty/serial/sc16is7xx.c:1168 sc16is7xx_probe()
> 	warn: 's->clk' isn't an ERR_PTR
> 
> drivers/tty/serial/sc16is7xx.c
>   1167  out_clk:
>   1168          if (!IS_ERR(s->clk))
>                      ^^^^^^^^^^^^^
> This check isn't needed because we return directly but also I don't
> think we ever set s->clk so it is leaked on the remove() path as well.
> 
>   1169                  clk_disable_unprepare(s->clk);
>   1170  
>   1171          return ret;
>   1172  }

I'm not too familiar with the clk interface, but we have this code:

| static int sc16is7xx_probe(struct device *dev,
| 			   struct sc16is7xx_devtype *devtype,
| 			   struct regmap *regmap, int irq, unsigned long flags)
| {
| 	unsigned long freq, *pfreq = dev_get_platdata(dev);
| 	struct clk *clk;
| 	int i, ret;
| 	struct sc16is7xx_port *s;
| 
| 	if (IS_ERR(regmap))
| 		return PTR_ERR(regmap);
| 
| 	/* Alloc port structure */
| 	s = devm_kzalloc(dev, sizeof(*s) +
| 			 sizeof(struct sc16is7xx_one) * devtype->nr_uart,
| 			 GFP_KERNEL);
| 	if (!s) {
| 		dev_err(dev, "Error allocating port structure\n");
| 		return -ENOMEM;
| 	}
| 
| 	clk = devm_clk_get(dev, NULL);

should this be s->clk = devm_clk_get(dev, NULL); ??

| 	if (IS_ERR(clk)) {
| 		if (pfreq)
| 			freq = *pfreq;
| 		else
| 			return PTR_ERR(clk);
| 	} else {
| 		freq = clk_get_rate(clk);
| 	}

Would the above change make the clk_disable_unprepare(s->clk) then 
correct? Or should the clk member of struct sc16is7xx_port be removed?

Jon