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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Hi,
Christophe JAILLET <[email protected]> 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
Le 12/02/2018 à 09:48, Felipe Balbi a écrit :
>
> Hi,
>
> Christophe JAILLET <[email protected]> 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
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