re: ufs: Add freq-table-hz property for UFS device

[Dan Carpenter <dan.carpenter@oracle.com>]

Hello Sahitya Tummala,

The patch 4cff6d991e4a: "ufs: Add freq-table-hz property for UFS
device" from Sep 25, 2014, leads to the following static checker
warning:

	drivers/scsi/ufs/ufshcd-pltfrm.c:138 ufshcd_parse_clock_info()
	warn: passing devm_ allocated variable to kfree. 'clkfreq'

drivers/scsi/ufs/ufshcd-pltfrm.c
   102          clkfreq = devm_kzalloc(dev, sz * sizeof(*clkfreq),
   103                          GFP_KERNEL);
                          ^^^^^^^^^^^^^
   104          if (!clkfreq) {
   105                  dev_err(dev, "%s: no memory\n", "freq-table-hz");

Don't print an error.  It just wastes ram.  Error messages are often
buggy.  If we delete it then the code is shorter and easier to read.
Kmalloc() has its own more useful error message already.

   106                  ret = -ENOMEM;
   107                  goto out;

Out labels are the worst.  The name is too vague.  They are a sign of
either One Err type error handling where you have one label handle
everything or they are a just a do-nothing goto where it returns
directly.

One Err type error handling causes bugs.

Do-nothing gotos are just annoying.  You're reading the code and you see
a "goto out;" and you think "What does "out" do?"  But the name doesn't
provide any hints.  You scroll down to the bottom of the function.  "Oh
it was just a waste of time."  Now you have lost your place and your
train of thought.

Also people constantly forget to set "ret" before the goto out.  If you
just write:


	if (!of_get_property(np, "freq-table-hz", &len)) {
		dev_info(dev, "freq-table-hz property not specified\n");
		return 0;
	}

	if (len <= 0)
		return 0;

Then it's totally clear what we intended to return.  But in the current
code it's ambiguous because maybe we just forgot to set ret?  Who
knows.  Out labels make the code hard to understand.

Anyway, in this case, originally "out" was doing One Err error handling
but now it's just there to waste our time and cause bugs when this code
is modified in the future.

   108          }
   109  
   110          ret = of_property_read_u32_array(np, "freq-table-hz",
   111                          clkfreq, sz);
   112          if (ret && (ret != -EINVAL)) {
   113                  dev_err(dev, "%s: error reading array %d\n",
   114                                  "freq-table-hz", ret);
   115                  goto free_clkfreq;
   116          }
   117  
   118          for (i = 0; i < sz; i += 2) {
   119                  ret = of_property_read_string_index(np,
   120                                  "clock-names", i/2, (const char **)&name);
   121                  if (ret)
   122                          goto free_clkfreq;
   123  
   124                  clki = devm_kzalloc(dev, sizeof(*clki), GFP_KERNEL);
   125                  if (!clki) {
   126                          ret = -ENOMEM;
   127                          goto free_clkfreq;
   128                  }
   129  
   130                  clki->min_freq = clkfreq[i];
   131                  clki->max_freq = clkfreq[i+1];
   132                  clki->name = kstrdup(name, GFP_KERNEL);

Where is name freed?  There are definitely certain error paths where it
is leaked.  Can we use devm_kstrdup() here?

   133                  dev_dbg(dev, "%s: min %u max %u name %s\n", "freq-table-hz",
   134                                  clki->min_freq, clki->max_freq, clki->name);
   135                  list_add_tail(&clki->list, &hba->clk_list_head);
   136          }
   137  free_clkfreq:
   138          kfree(clkfreq);
                ^^^^^^^^^^^^^
Just delete this.

   139  out:
   140          return ret;
   141  }

regards,
dan carpenter