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

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