[PATCH v2 1/3] phy: at91: Add support for the USB 2.0 PHY's of SAMA7

[Sergiu.Moga at microchip.com]

On 03.01.2023 17:31, Marek Vasut wrote:
> On 1/3/23 15:30, Sergiu Moga wrote:
>> In order to have USB functionality, drivers for SAMA7's
>> USB 2.0 PHY's have been added. There is one driver
>> for UTMI clock's SFR and RESET required functionalities and
>> one for its three possible subclocks of the phy's themselves.
>> In order for this layout to properly work in conjunction with
>> CCF and DT, the former driver will also act as a clock provider
>> for the three phy's with the help of a custom hook into the
>> driver's of_xlate method.
> 
> [...]
> 
>> +static int sama7_utmi_probe(struct udevice *dev)
>> +{
>> +     struct clk *utmi_parent_clk, *utmi_clk;
>> +     struct regmap *regmap_sfr;
>> +     struct reset_ctl *phy_reset;
>> +     int i;
>> +     char name[16];
>> +
>> +     utmi_parent_clk = devm_clk_get(dev, "utmi_clk");
>> +     if (IS_ERR(utmi_parent_clk))
>> +             return PTR_ERR(utmi_parent_clk);
>> +
>> +     regmap_sfr = syscon_regmap_lookup_by_phandle(dev, "sfr-phandle");
>> +     if (IS_ERR(regmap_sfr))
>> +             return PTR_ERR(regmap_sfr);
>> +
>> +     for (i = 0; i < ARRAY_SIZE(sama7_utmick); i++) {
>> +             snprintf(name, sizeof(name), "usb%d_reset", i);
>> +             phy_reset = devm_reset_control_get(dev, name);
>> +             if (IS_ERR(phy_reset))
>> +                     return PTR_ERR(phy_reset);
>> +
>> +             utmi_clk = sama7_utmi_clk_register(regmap_sfr, phy_reset,
>> +                                                sama7_utmick[i].n,
>> +                                                sama7_utmick[i].p,
>> +                                                sama7_utmick[i].id);
>> +             if (IS_ERR(utmi_clk))
>> +                     return PTR_ERR(utmi_clk);
> 
> Isn't this going to leak memory if i>0 and a failure occurs ? I think it
> will, since sama7_utmi_clk_register() calls kzalloc() .

Yes, you are right. Perhaps something like this should work better 
instead, wouldn't you agree?

  static int sama7_utmi_probe(struct udevice *dev)
  {
-	struct clk *utmi_parent_clk, *utmi_clk;
+	struct clk *utmi_parent_clk, *utmi_clk[ARRAY_SIZE(sama7_utmick)];
+	struct sama7_utmi_clk *uclk;
  	struct regmap *regmap_sfr;
  	struct reset_ctl *phy_reset;
-	int i;
+	int i, j;
  	char name[16];

  	utmi_parent_clk = devm_clk_get(dev, "utmi_clk");
@@ -153,12 +154,18 @@ static int sama7_utmi_probe(struct udevice *dev)
  		if (IS_ERR(phy_reset))
  			return PTR_ERR(phy_reset);

-		utmi_clk = sama7_utmi_clk_register(regmap_sfr, phy_reset,
+		utmi_clk[i] = sama7_utmi_clk_register(regmap_sfr, phy_reset,
  						   sama7_utmick[i].n,
  						   sama7_utmick[i].p,
  						   sama7_utmick[i].id);
-		if (IS_ERR(utmi_clk))
+		if (IS_ERR(utmi_clk)) {
+			for (j = 0; j < i; j++) {
+				uclk = to_sama7_utmi_clk(utmi_clk[j]);
+				kfree(uclk);
+			}
+
  			return PTR_ERR(utmi_clk);
+		}
  	}

  	return 0;