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

[Marek Vasut]

On 1/3/23 17:54, Sergiu.Moga at microchip.com wrote:
> 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);

Do the usual error handling path using goto:

{
...
if (IS_ERR(...))
   goto error_clk_register;
...

error_clk_register:
   kfree();
error_something_else:
   clean_up_other_stuff();
   return ret;
}