Re: [PATCH] ARM: OMAP2+: omap-usb-host: Fix memory leaks

[Roger Quadros]

Hi Tony,

On 05/31/2013 12:00 AM, Tony Lindgren wrote:
> Hi Roger,
> 
> * Roger Quadros <rogerq@xxxxxx> [130524 06:12]:
>> Fix memory leaks in the error path.
>> Also, use platform_device_register_full() to allocate
>> the platform devices and set platform data.
> 
> If you need this for the v3.10-rc, you should describe why this patch
> is needed and ideally have some oops or regression causing commit
> listed. Care to update the description for that?

There is no oops or regression happening. Just that there will be a
small memory leak if any of the memory allocations fail or if the
platform device is destroyed.

If it doesn't look that serious to you then it can wait.
But if we move to device tree only boot then this patch and the file
"usb-host.c" might not be required at all.

cheers,
-roger

>  
>> Signed-off-by: Roger Quadros <rogerq@xxxxxx>
>> ---
>>  arch/arm/mach-omap2/usb-host.c |  106 +++++++++++++++++++++-------------------
>>  1 files changed, 56 insertions(+), 50 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/usb-host.c b/arch/arm/mach-omap2/usb-host.c
>> index aa27d7f..609b330 100644
>> --- a/arch/arm/mach-omap2/usb-host.c
>> +++ b/arch/arm/mach-omap2/usb-host.c
>> @@ -28,6 +28,7 @@
>>  #include <linux/io.h>
>>  #include <linux/gpio.h>
>>  #include <linux/usb/phy.h>
>> +#include <linux/usb/nop-usb-xceiv.h>
>>  
>>  #include "soc.h"
>>  #include "omap_device.h"
>> @@ -560,7 +561,8 @@ static int usbhs_add_regulator(char *name, char *dev_id, char *dev_supply,
>>  	struct regulator_init_data *reg_data;
>>  	struct fixed_voltage_config *config;
>>  	struct platform_device *pdev;
>> -	int ret;
>> +	struct platform_device_info pdevinfo;
>> +	int ret = -ENOMEM;
>>  
>>  	supplies = kzalloc(sizeof(*supplies), GFP_KERNEL);
>>  	if (!supplies)
>> @@ -571,7 +573,7 @@ static int usbhs_add_regulator(char *name, char *dev_id, char *dev_supply,
>>  
>>  	reg_data = kzalloc(sizeof(*reg_data), GFP_KERNEL);
>>  	if (!reg_data)
>> -		return -ENOMEM;
>> +		goto err_data;
>>  
>>  	reg_data->constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS;
>>  	reg_data->consumer_supplies = supplies;
>> @@ -580,39 +582,53 @@ static int usbhs_add_regulator(char *name, char *dev_id, char *dev_supply,
>>  	config = kmemdup(&hsusb_reg_config, sizeof(hsusb_reg_config),
>>  			GFP_KERNEL);
>>  	if (!config)
>> -		return -ENOMEM;
>> +		goto err_config;
>> +
>> +	config->supply_name = kstrdup(name, GFP_KERNEL);
>> +	if (!config->supply_name)
>> +		goto err_supplyname;
>>  
>> -	config->supply_name = name;
>>  	config->gpio = gpio;
>>  	config->enable_high = polarity;
>>  	config->init_data = reg_data;
>>  
>>  	/* create a regulator device */
>> -	pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);
>> -	if (!pdev)
>> -		return -ENOMEM;
>> +	memset(&pdevinfo, 0, sizeof(pdevinfo));
>> +	pdevinfo.name = reg_name;
>> +	pdevinfo.id = PLATFORM_DEVID_AUTO;
>> +	pdevinfo.data = config;
>> +	pdevinfo.size_data = sizeof(*config);
>>  
>> -	pdev->id = PLATFORM_DEVID_AUTO;
>> -	pdev->name = reg_name;
>> -	pdev->dev.platform_data = config;
>> +	pdev = platform_device_register_full(&pdevinfo);
>> +	if (IS_ERR(pdev)) {
>> +		ret = PTR_ERR(pdev);
>> +		pr_err("%s: Failed registering regulator %s for %s : %d\n",
>> +				__func__, name, dev_id, ret);
>> +		goto err_register;
>> +	}
>>  
>> -	ret = platform_device_register(pdev);
>> -	if (ret)
>> -		pr_err("%s: Failed registering regulator %s for %s\n",
>> -				__func__, name, dev_id);
>> +	return 0;
>>  
>> +err_register:
>> +	kfree(config->supply_name);
>> +err_supplyname:
>> +	kfree(config);
>> +err_config:
>> +	kfree(reg_data);
>> +err_data:
>> +	kfree(supplies);
>>  	return ret;
>>  }
>>  
>> +#define MAX_STR 20
>> +
>>  int usbhs_init_phys(struct usbhs_phy_data *phy, int num_phys)
>>  {
>> -	char *rail_name;
>> -	int i, len;
>> +	char rail_name[MAX_STR];
>> +	int i;
>>  	struct platform_device *pdev;
>>  	char *phy_id;
>> -
>> -	/* the phy_id will be something like "nop_usb_xceiv.1" */
>> -	len = strlen(nop_name) + 3; /* 3 -> ".1" and NULL terminator */
>> +	struct platform_device_info pdevinfo;
>>  
>>  	for (i = 0; i < num_phys; i++) {
>>  
>> @@ -627,25 +643,26 @@ int usbhs_init_phys(struct usbhs_phy_data *phy, int num_phys)
>>  			!gpio_is_valid(phy->vcc_gpio))
>>  			continue;
>>  
>> -		/* create a NOP PHY device */
>> -		pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);
>> -		if (!pdev)
>> -			return -ENOMEM;
>> -
>> -		pdev->id = phy->port;
>> -		pdev->name = nop_name;
>> -		pdev->dev.platform_data = phy->platform_data;
>> -
>> -		phy_id = kmalloc(len, GFP_KERNEL);
>> -		if (!phy_id)
>> +		phy_id = kmalloc(MAX_STR, GFP_KERNEL);
>> +		if (!phy_id) {
>> +			pr_err("%s: kmalloc() failed\n", __func__);
>>  			return -ENOMEM;
>> +		}
>>  
>> -		scnprintf(phy_id, len, "nop_usb_xceiv.%d\n",
>> -					pdev->id);
>> -
>> -		if (platform_device_register(pdev)) {
>> -			pr_err("%s: Failed to register device %s\n",
>> -				__func__,  phy_id);
>> +		/* create a NOP PHY device */
>> +		memset(&pdevinfo, 0, sizeof(pdevinfo));
>> +		pdevinfo.name = nop_name;
>> +		pdevinfo.id = phy->port;
>> +		pdevinfo.data = phy->platform_data;
>> +		pdevinfo.size_data = sizeof(struct nop_usb_xceiv_platform_data);
>> +
>> +		scnprintf(phy_id, MAX_STR, "nop_usb_xceiv.%d",
>> +					phy->port);
>> +		pdev = platform_device_register_full(&pdevinfo);
>> +		if (IS_ERR(pdev)) {
>> +			pr_err("%s: Failed to register device %s : %ld\n",
>> +				__func__,  phy_id, PTR_ERR(pdev));
>> +			kfree(phy_id);
>>  			continue;
>>  		}
>>  
>> @@ -653,26 +670,15 @@ int usbhs_init_phys(struct usbhs_phy_data *phy, int num_phys)
>>  
>>  		/* Do we need RESET regulator ? */
>>  		if (gpio_is_valid(phy->reset_gpio)) {
>> -
>> -			rail_name = kmalloc(13, GFP_KERNEL);
>> -			if (!rail_name)
>> -				return -ENOMEM;
>> -
>> -			scnprintf(rail_name, 13, "hsusb%d_reset", phy->port);
>> -
>> +			scnprintf(rail_name, MAX_STR,
>> +					"hsusb%d_reset", phy->port);
>>  			usbhs_add_regulator(rail_name, phy_id, "reset",
>>  						phy->reset_gpio, 1);
>>  		}
>>  
>>  		/* Do we need VCC regulator ? */
>>  		if (gpio_is_valid(phy->vcc_gpio)) {
>> -
>> -			rail_name = kmalloc(13, GFP_KERNEL);
>> -			if (!rail_name)
>> -				return -ENOMEM;
>> -
>> -			scnprintf(rail_name, 13, "hsusb%d_vcc", phy->port);
>> -
>> +			scnprintf(rail_name, MAX_STR, "hsusb%d_vcc", phy->port);
>>  			usbhs_add_regulator(rail_name, phy_id, "vcc",
>>  					phy->vcc_gpio, phy->vcc_polarity);
>>  		}
>> -- 
>> 1.7.4.1
>>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/