From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753338AbbIPJtp (ORCPT ); Wed, 16 Sep 2015 05:49:45 -0400 Received: from mailout1.w1.samsung.com ([210.118.77.11]:24728 "EHLO mailout1.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751811AbbIPJti (ORCPT ); Wed, 16 Sep 2015 05:49:38 -0400 X-AuditID: cbfec7f4-f79c56d0000012ee-31-55f93b305b68 Message-id: <55F93B26.2000401@samsung.com> Date: Wed, 16 Sep 2015 11:49:26 +0200 From: Robert Baldyga User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.8.0 MIME-version: 1.0 To: Krzysztof Opasiak , balbi@ti.com Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, b.zolnierkie@samsung.com, m.szyprowski@samsung.com, andrzej.p@samsung.com Subject: Re: [PATCH 05/26] usb: gadget: f_ecm: eliminate abuse of ep->driver data References: <1442327229-28320-1-git-send-email-r.baldyga@samsung.com> <1442327229-28320-6-git-send-email-r.baldyga@samsung.com> <55F83D36.7090208@samsung.com> In-reply-to: <55F83D36.7090208@samsung.com> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrJLMWRmVeSWpSXmKPExsVy+t/xK7oG1j9DDSbM4bKY9bKdxWLjjPWs Fgfv11s0L17PZnF74jQ2i8u75rBZLFrWymyx9shddgcOj/1z17B79G1Zxehx/MZ2Jo/Pm+QC WKK4bFJSczLLUov07RK4Mh5tvM9a0CFU8XznedYGxgN8XYycHBICJhLLn21jhbDFJC7cW8/W xcjFISSwlFHi34dPTCAJIYFnjBKr7/qA2LwCWhI3HvxhBrFZBFQl5q0/AlbDJqAjseX7BEYQ W1QgQmL56pOMEPWCEj8m32PpYuTgEBGwlji0zB9kPrPAMkaJJxMWs4PUCAsES+ycfYQVYvEi oMXrzoI1cwpoS8x5fhOsmVlAT+L+RS2QMLOAvMTmNW+ZJzAKzEKyYhZC1SwkVQsYmVcxiqaW JhcUJ6XnGuoVJ+YWl+al6yXn525ihAT4lx2Mi49ZHWIU4GBU4uF1ePkjVIg1say4MvcQowQH s5IIb6TCz1Ah3pTEyqrUovz4otKc1OJDjNIcLErivHN3vQ8REkhPLEnNTk0tSC2CyTJxcEo1 MHKc33Fv5eW7951MRWbb6Oze9p+3PSDoeYhee80MZxeL8h+Lex4v+yjXFTwr68KyvLXXZ+lU yHTybLzN7j3l8nbh29MUyneItLY8keNO4L/2ofd2ggB/3Zv951X+2Ks/vLPsUMuFa33TldZM Olhytt+Cy7Xi+/P5Mx+pnagrv1vK45XIu2B3Q7USS3FGoqEWc1FxIgAJOG8PbAIAAA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 09/15/2015 05:45 PM, Krzysztof Opasiak wrote: > > > On 09/15/2015 04:26 PM, Robert Baldyga wrote: >> Since ep->driver_data is not used for endpoint claiming, neither for >> enabled/disabled state storing, we can reduce number of places where >> we read or modify it's value, as now it has no particular meaning for >> function or framework logic. >> >> In case of f_ecm, ep->driver_data was used only for endpoint claiming >> and marking endpoints as enabled, so we can simplify code by reducing >> it. >> >> Signed-off-by: Robert Baldyga > > ( ... ) > >> >> @@ -820,14 +811,6 @@ fail: >> usb_ep_free_request(ecm->notify, ecm->notify_req); >> } >> >> - /* we might as well release our claims on endpoints */ >> - if (ecm->notify) >> - ecm->notify->driver_data = NULL; >> - if (ecm->port.out_ep) >> - ecm->port.out_ep->driver_data = NULL; >> - if (ecm->port.in_ep) >> - ecm->port.in_ep->driver_data = NULL; >> - >> ERROR(cdev, "%s: can't bind, err %d\n", f->name, status); >> >> return status; >> > > You have done this in almost all functions but personally I'm really > concern about this change. > > By convention function should free all allocated resources when exiting > with non 0 code. Endpoints are some kind of resources, they are > "allocated" using usb_ep_autoconfig() and if you are not going to use > them because error occurred you should free the using > usb_ep_autoconfig_release(). Moreover, you have done this in source sink > function so why not do this in all other? Well, if bind() returns an error code, composite calls usb_ep_autoconfig_reset() anyway, so manual endpoint releasing in error path isn't necessary. We can compare the way it works with devm_* functions family - you don't need to care about resource lifetime as it's managed for you. The function f_sourcesink is a bit different, because it can work with two different endpoint configurations {bulk-in, bulk-out} or {bulk-in, bulk-out, iso-in, iso-out}. When bulk endpoints are obtained successfully it tries to get iso endpoints, but if one if them is not available it releases previously obtained iso and ends with success. In such case it simply starts in bulk-only configuration. Best regards, Robert