Skip to contentSkip to conversation

clk: add gpio gated clock

DISCUSSION Dan Carpenter 2 participants 2 messages Oct 1, 2014 – Oct 2, 2014 Kernel janitors

lore
Share & tools
lore
#0 lore raw

Hello Jyri Sarha,

The patch c873d14d30b8: "clk: add gpio gated clock" from Sep 5, 2014, leads to the following static checker warning:

drivers/clk/clk-gpio-gate.c:123 clk_register_gpio_gate()
warn: passing devm_ allocated variable to kfree. 'clk_gpio'

drivers/clk/clk-gpio-gate.c
97 if (dev)
98 clk_gpio = devm_kzalloc(dev, sizeof(struct clk_gpio),
99 GFP_KERNEL);
100 else
101 clk_gpio = kzalloc(sizeof(struct clk_gpio), GFP_KERNEL);
102
103 if (!clk_gpio) {
104 clk = ERR_PTR(-ENOMEM);
105 goto clk_register_gpio_gate_err;
106 }
107

   108          init.name = name;
   109          init.ops = &clk_gpio_gate_ops;
   110          init.flags = flags | CLK_IS_BASIC;
   111          init.parent_names = (parent_name ? &parent_name : NULL);
   112          init.num_parents = (parent_name ? 1 : 0);
   113  
   114          clk_gpio->gpiod = gpiod;
   115          clk_gpio->hw.init = &init;
   116  
   117          clk = clk_register(dev, &clk_gpio->hw);
   118  
   119          if (!IS_ERR(clk))
   120                  return clk;
   121  
   122          if (!dev)
   123                  kfree(clk_gpio);
                        ^^^^^^^^^^^^^^^

It's complaing about this. Obviously there is an "if (dev)" on the allocation side. Also Smatch is supposed to be able to follow logic like that. The other thing which may affect this, is that this is dead code. Smatch tends to deliberately print error messages in dead code because Smatch is confused by dead code.

Anyway, the point is that this API sucks. If it ever stops being dead code then it's going to be a source of bugs.

Also in the kernel we tend to frown on putting in code before there is a user for it. So lets delete all the "if (dev) " code because that pointer is always NULL.

regards,
dan carpenter

#1 lore raw
Quoted 46 lines · Dan Carpenter

Hello Jyri Sarha,

The patch c873d14d30b8: "clk: add gpio gated clock" from Sep 5, 2014, leads to the following static checker warning:

drivers/clk/clk-gpio-gate.c:123 clk_register_gpio_gate()
warn: passing devm_ allocated variable to kfree. 'clk_gpio'

drivers/clk/clk-gpio-gate.c

     97          if (dev)
     98                  clk_gpio = devm_kzalloc(dev, sizeof(struct clk_gpio),
     99                                          GFP_KERNEL);
    100          else
    101                  clk_gpio = kzalloc(sizeof(struct clk_gpio), GFP_KERNEL);
    102
    103          if (!clk_gpio) {
    104                  clk = ERR_PTR(-ENOMEM);
    105                  goto clk_register_gpio_gate_err;
    106          }
    107
    108          init.name = name;
    109          init.ops = &clk_gpio_gate_ops;
    110          init.flags = flags | CLK_IS_BASIC;
    111          init.parent_names = (parent_name ? &parent_name : NULL);
    112          init.num_parents = (parent_name ? 1 : 0);
    113
    114          clk_gpio->gpiod = gpiod;
    115          clk_gpio->hw.init = &init;
    116
    117          clk = clk_register(dev, &clk_gpio->hw);
    118
    119          if (!IS_ERR(clk))
    120                  return clk;
    121
    122          if (!dev)
    123                  kfree(clk_gpio);
                         ^^^^^^^^^^^^^^^

It's complaing about this. Obviously there is an "if (dev)" on the allocation side. Also Smatch is supposed to be able to follow logic like that. The other thing which may affect this, is that this is dead code. Smatch tends to deliberately print error messages in dead code because Smatch is confused by dead code.

Anyway, the point is that this API sucks. If it ever stops being dead code then it's going to be a source of bugs.

Quoted 7 lines

Also in the kernel we tend to frown on putting in code before there is a user for it. So lets delete all the "if (dev) " code because that pointer is always NULL.

regards,
dan carpenter

In theory someone could call clk_register_gpio_gate() directly with a device pointer, but well he can call kfree() then too. I'll mail a patch in a minute.

Best regards,
Jyri