[PATCH 0/4] usb: gadget: fotg210-udc: Fixes and cleanup

[PATCH 0/4] usb: gadget: fotg210-udc: Fixes and cleanup

[Christophe JAILLET]

This serie aims to fix 2 issues. (path 2 & 4)

The 2nd patch fixes a memory leak. It uses devm_ function a simplify the
handling of the memory.

The 4th patch fixes a potential invalid pointer dereference.

The 2 other ones, are just clean-ups to remove useless code and add other
uses of devm_ function to simplify code.

I've left the request_irq/free_irq because I'm unsure of potential side
effects if some other resources are freed while an IRQ can still be
triggered. So I've preferred to leave it as-is.

Christophe JAILLET (4):
  usb: gadget: fotg210-udc: Remove a useless
  usb: gadget: fotg210-udc: Fix a memory leak
  usb: gadget: fotg210-udc: Simplify code
  usb: gadget: fotg210-udc: Fix a potential invalid pointer dereference

 drivers/usb/gadget/udc/fotg210-udc.c | 41 ++++++++++++------------------------
 1 file changed, 14 insertions(+), 27 deletions(-)

-- 
2.14.1

[PATCH 1/4] usb: gadget: fotg210-udc: Remove a useless

[Christophe JAILLET]

There is no need to use an intermediate array for these memory allocations,
so, axe it.

While at it, turn a '== NULL' into a shorter '!' when testing memory
allocation failure.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/usb/gadget/udc/fotg210-udc.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/gadget/udc/fotg210-udc.c b/drivers/usb/gadget/udc/fotg210-udc.c
index 53a48f561458..2fea32a153b7 100644
--- a/drivers/usb/gadget/udc/fotg210-udc.c
+++ b/drivers/usb/gadget/udc/fotg210-udc.c
@@ -1078,7 +1078,6 @@ static int fotg210_udc_probe(struct platform_device *pdev)
 {
 	struct resource *res, *ires;
 	struct fotg210_udc *fotg210 = NULL;
-	struct fotg210_ep *_ep[FOTG210_MAX_NUM_EP];
 	int ret = 0;
 	int i;
 
@@ -1102,10 +1101,9 @@ static int fotg210_udc_probe(struct platform_device *pdev)
 		goto err_alloc;
 
 	for (i = 0; i < FOTG210_MAX_NUM_EP; i++) {
-		_ep[i] = kzalloc(sizeof(struct fotg210_ep), GFP_KERNEL);
-		if (_ep[i] == NULL)
+		fotg210->ep[i] = kzalloc(sizeof(struct fotg210_ep), GFP_KERNEL);
+		if (!fotg210->ep[i])
 			goto err_alloc;
-		fotg210->ep[i] = _ep[i];
 	}
 
 	fotg210->reg = ioremap(res->start, resource_size(res));
-- 
2.14.1

[PATCH 2/4] usb: gadget: fotg210-udc: Fix a memory leak

[Christophe JAILLET]

The memory referenced by the 'fotg210->ep[]' array, is never freed.
So there is a memory leak in the error handling path of the probe function
and when the driver is unloaded.
Use 'devm_kzalloc()' to fix these leaks.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/usb/gadget/udc/fotg210-udc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/fotg210-udc.c b/drivers/usb/gadget/udc/fotg210-udc.c
index 2fea32a153b7..a4d46b9759be 100644
--- a/drivers/usb/gadget/udc/fotg210-udc.c
+++ b/drivers/usb/gadget/udc/fotg210-udc.c
@@ -1101,7 +1101,8 @@ static int fotg210_udc_probe(struct platform_device *pdev)
 		goto err_alloc;
 
 	for (i = 0; i < FOTG210_MAX_NUM_EP; i++) {
-		fotg210->ep[i] = kzalloc(sizeof(struct fotg210_ep), GFP_KERNEL);
+		fotg210->ep[i] = devm_kzalloc(&pdev->dev,
+					sizeof(struct fotg210_ep), GFP_KERNEL);
 		if (!fotg210->ep[i])
 			goto err_alloc;
 	}
-- 
2.14.1

[PATCH 3/4] usb: gadget: fotg210-udc: Simplify code

[Christophe JAILLET]

Use 'devm_kzalloc()' and 'devm_ioremap()' to simplify code.

While at it, turn some '== NULL' into shorter '!' when testing memory
allocation failure.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/usb/gadget/udc/fotg210-udc.c | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/gadget/udc/fotg210-udc.c b/drivers/usb/gadget/udc/fotg210-udc.c
index a4d46b9759be..99a18b14c8c2 100644
--- a/drivers/usb/gadget/udc/fotg210-udc.c
+++ b/drivers/usb/gadget/udc/fotg210-udc.c
@@ -1065,11 +1065,8 @@ static int fotg210_udc_remove(struct platform_device *pdev)
 	struct fotg210_udc *fotg210 = platform_get_drvdata(pdev);
 
 	usb_del_gadget_udc(&fotg210->gadget);
-	iounmap(fotg210->reg);
 	free_irq(platform_get_irq(pdev, 0), fotg210);
-
 	fotg210_ep_free_request(&fotg210->ep[0]->ep, fotg210->ep0_req);
-	kfree(fotg210);
 
 	return 0;
 }
@@ -1096,21 +1093,22 @@ static int fotg210_udc_probe(struct platform_device *pdev)
 	ret = -ENOMEM;
 
 	/* initialize udc */
-	fotg210 = kzalloc(sizeof(struct fotg210_udc), GFP_KERNEL);
-	if (fotg210 == NULL)
-		goto err_alloc;
+	fotg210 = devm_kzalloc(&pdev->dev, sizeof(struct fotg210_udc),
+			       GFP_KERNEL);
+	if (!fotg210)
+		return -ENOMEM;
 
 	for (i = 0; i < FOTG210_MAX_NUM_EP; i++) {
 		fotg210->ep[i] = devm_kzalloc(&pdev->dev,
 					sizeof(struct fotg210_ep), GFP_KERNEL);
 		if (!fotg210->ep[i])
-			goto err_alloc;
+			return -ENOMEM;
 	}
 
-	fotg210->reg = ioremap(res->start, resource_size(res));
-	if (fotg210->reg == NULL) {
+	fotg210->reg = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+	if (!fotg210->reg) {
 		pr_err("ioremap error.\n");
-		goto err_map;
+		return -ENOMEM;
 	}
 
 	spin_lock_init(&fotg210->lock);
@@ -1185,13 +1183,6 @@ static int fotg210_udc_probe(struct platform_device *pdev)
 err_req:
 	fotg210_ep_free_request(&fotg210->ep[0]->ep, fotg210->ep0_req);
 
-err_map:
-	if (fotg210->reg)
-		iounmap(fotg210->reg);
-
-err_alloc:
-	kfree(fotg210);
-
 	return ret;
 }
 
-- 
2.14.1

[PATCH 4/4] usb: gadget: fotg210-udc: Fix a potential invalid pointer dereference

[Christophe JAILLET]

We should not call 'fotg210_ep_free_request()' with NULL as a 2nd parameter.
Depending of the layout of 'struct fotg210_request', this could lead to an
un-expected behavior.

So, if 'fotg210_ep_alloc_request()' fails, we should return directly.
This also gives the opportunity to further simplify code.


(In fact, this change should be a no-op, because 'req' is the first field of
'struct fotg210_request'. So passing NULL would result in 'free(NULL)' in
'fotg210_ep_free_request()'. Anyway avoiding the goto is cleaner.)

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/usb/gadget/udc/fotg210-udc.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/gadget/udc/fotg210-udc.c b/drivers/usb/gadget/udc/fotg210-udc.c
index 99a18b14c8c2..03adfcb58548 100644
--- a/drivers/usb/gadget/udc/fotg210-udc.c
+++ b/drivers/usb/gadget/udc/fotg210-udc.c
@@ -1075,8 +1075,7 @@ static int fotg210_udc_probe(struct platform_device *pdev)
 {
 	struct resource *res, *ires;
 	struct fotg210_udc *fotg210 = NULL;
-	int ret = 0;
-	int i;
+	int ret, i;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
@@ -1090,8 +1089,6 @@ static int fotg210_udc_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	ret = -ENOMEM;
-
 	/* initialize udc */
 	fotg210 = devm_kzalloc(&pdev->dev, sizeof(struct fotg210_udc),
 			       GFP_KERNEL);
@@ -1155,8 +1152,8 @@ static int fotg210_udc_probe(struct platform_device *pdev)
 
 	fotg210->ep0_req = fotg210_ep_alloc_request(&fotg210->ep[0]->ep,
 				GFP_KERNEL);
-	if (fotg210->ep0_req == NULL)
-		goto err_req;
+	if (!fotg210->ep0_req)
+		return -ENOMEM;
 
 	fotg210_init(fotg210);
 
-- 
2.14.1

Re: [PATCH 0/4] usb: gadget: fotg210-udc: Fixes and cleanup

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1197 bytes --]


Hi,

Christophe JAILLET <christophe.jaillet@wanadoo.fr> writes:
> This serie aims to fix 2 issues. (path 2 & 4)
>
> The 2nd patch fixes a memory leak. It uses devm_ function a simplify the
> handling of the memory.
>
> The 4th patch fixes a potential invalid pointer dereference.
>
> The 2 other ones, are just clean-ups to remove useless code and add other
> uses of devm_ function to simplify code.
>
> I've left the request_irq/free_irq because I'm unsure of potential side
> effects if some other resources are freed while an IRQ can still be
> triggered. So I've preferred to leave it as-is.
>
> Christophe JAILLET (4):
>   usb: gadget: fotg210-udc: Remove a useless
>   usb: gadget: fotg210-udc: Fix a memory leak
>   usb: gadget: fotg210-udc: Simplify code
>   usb: gadget: fotg210-udc: Fix a potential invalid pointer dereference

you should NEVER make fixes depend on cleanups. It should be the other
way around :-) First fixes, then cleanups. The reason is that fixes can
get accepted during -rc cycle, but cleanups must wait until the next
merge window.

Please fix up your patches, otherwise I'll have to apply the entire
series for v4.17

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 0/4] usb: gadget: fotg210-udc: Fixes and cleanup

[Christophe Jaillet]

Le 12/02/2018 à 09:48, Felipe Balbi a écrit :
> 
> Hi,
> 
> Christophe JAILLET <christophe.jaillet@wanadoo.fr> writes:
>> This serie aims to fix 2 issues. (path 2 & 4)
>>
>> The 2nd patch fixes a memory leak. It uses devm_ function a simplify the
>> handling of the memory.
>>
>> The 4th patch fixes a potential invalid pointer dereference.
>>
>> The 2 other ones, are just clean-ups to remove useless code and add other
>> uses of devm_ function to simplify code.
>>
>> I've left the request_irq/free_irq because I'm unsure of potential side
>> effects if some other resources are freed while an IRQ can still be
>> triggered. So I've preferred to leave it as-is.
>>
>> Christophe JAILLET (4):
>>    usb: gadget: fotg210-udc: Remove a useless
>>    usb: gadget: fotg210-udc: Fix a memory leak
>>    usb: gadget: fotg210-udc: Simplify code
>>    usb: gadget: fotg210-udc: Fix a potential invalid pointer dereference
> 
> you should NEVER make fixes depend on cleanups. It should be the other
> way around :-) First fixes, then cleanups. The reason is that fixes can
> get accepted during -rc cycle, but cleanups must wait until the next
> merge window.
> 
> Please fix up your patches, otherwise I'll have to apply the entire
> series for v4.17
> 

I agree with you. I will be more careful in the future.
However, I will not re-send an updated version. Development on this 
driver does not seem to be very active. So the proposed fix (2/4) and 
cleanups can wait a few more months.
Feel free to update yourself 2/4 (and eventually drop 1/4 completely to 
avoid the time to re-work it) if you think that it worth it.

Best regards,
CJ


---
L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel antivirus Avast.
https://www.avast.com/antivirus

Re: [PATCH 0/4] usb: gadget: fotg210-udc: Fixes and cleanup

[Hans Ulli Kroll]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 950 bytes --]

Hi Christophe

On Mon, 12 Feb 2018, Christophe Jaillet wrote:

> Le 12/02/2018 à 09:48, Felipe Balbi a écrit :
> > 
[..]

> > way around :-) First fixes, then cleanups. The reason is that fixes can
> > get accepted during -rc cycle, but cleanups must wait until the next
> > merge window.
> > 
> > Please fix up your patches, otherwise I'll have to apply the entire
> > series for v4.17
> > 
> 
> I agree with you. I will be more careful in the future.
> However, I will not re-send an updated version. Development on this driver
> does not seem to be very active.

... not quite right.
This driver and the fotg210-hcd is *currently* under my observation, I 
want to add a phy/otg driber on top of these.
Feel free to CC me if you want to resend these patches.

With "not very active" makes me happy, so I don't break not much.
And if anyone ask, I have some devices to test, of cource ;-)

Greetings
Hans Ulli Kroll