| Message ID | 20241215205358.4100142-1-zmw12306@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: gadget: m66592-udc: Add check for clk_enable() | expand |
Hi Greg, On Mon, Dec 16, 2024 at 2:56 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Sun, Dec 15, 2024 at 03:53:58PM -0500, Mingwei Zheng wrote: > > Add check for the return value of clk_enable() to catch the potential > > error. > > > > Fixes: b4822e2317e8 ("usb: gadget: m66592-udc: Convert to use module_platform_driver()") > > Signed-off-by: Mingwei Zheng <zmw12306@gmail.com> > > Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com> > > Why this order of signed-off-by lines? Shouldn't yours be last? Who > wrote this patch? > I listed two names because both of us co-authored this patch. > > --- > > drivers/usb/gadget/udc/m66592-udc.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/gadget/udc/m66592-udc.c b/drivers/usb/gadget/udc/m66592-udc.c > > index a938b2af0944..bf408476a24c 100644 > > --- a/drivers/usb/gadget/udc/m66592-udc.c > > +++ b/drivers/usb/gadget/udc/m66592-udc.c > > @@ -1606,7 +1606,11 @@ static int m66592_probe(struct platform_device *pdev) > > ret = PTR_ERR(m66592->clk); > > goto clean_up2; > > } > > - clk_enable(m66592->clk); > > + ret = clk_enable(m66592->clk); > > + if (ret) { > > + clk_put(m66592->clk); > > + goto clean_up2; > > + } > > How did you find this and how was it tested? > > thanks, > > greg k-h We found it through a static analysis tool. Additionally, we validated the patch's correctness using the built-in tests provided during the compilation process. Please kindly let me know if you need further details or have any questions. Thank you! Best, Mingwei
On Tue, Dec 17, 2024 at 10:06:26PM -0500, Mingwei Zheng wrote: > Hi Greg, > > On Mon, Dec 16, 2024 at 2:56 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > On Sun, Dec 15, 2024 at 03:53:58PM -0500, Mingwei Zheng wrote: > > > Add check for the return value of clk_enable() to catch the potential > > > error. > > > > > > Fixes: b4822e2317e8 ("usb: gadget: m66592-udc: Convert to use module_platform_driver()") > > > Signed-off-by: Mingwei Zheng <zmw12306@gmail.com> > > > Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com> > > > > Why this order of signed-off-by lines? Shouldn't yours be last? Who > > wrote this patch? > > > > I listed two names because both of us co-authored this patch. > > > > --- > > > drivers/usb/gadget/udc/m66592-udc.c | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/usb/gadget/udc/m66592-udc.c b/drivers/usb/gadget/udc/m66592-udc.c > > > index a938b2af0944..bf408476a24c 100644 > > > --- a/drivers/usb/gadget/udc/m66592-udc.c > > > +++ b/drivers/usb/gadget/udc/m66592-udc.c > > > @@ -1606,7 +1606,11 @@ static int m66592_probe(struct platform_device *pdev) > > > ret = PTR_ERR(m66592->clk); > > > goto clean_up2; > > > } > > > - clk_enable(m66592->clk); > > > + ret = clk_enable(m66592->clk); > > > + if (ret) { > > > + clk_put(m66592->clk); > > > + goto clean_up2; > > > + } > > > > How did you find this and how was it tested? > > > > thanks, > > > > greg k-h > > We found it through a static analysis tool. Then you need to properly document that as our documentation says it is required, right? thanks, greg k-h
Hi Greg, On Mon, Dec 23, 2024 at 12:46 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Tue, Dec 17, 2024 at 10:06:26PM -0500, Mingwei Zheng wrote: > > Hi Greg, > > > > On Mon, Dec 16, 2024 at 2:56 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > > On Sun, Dec 15, 2024 at 03:53:58PM -0500, Mingwei Zheng wrote: > > > > Add check for the return value of clk_enable() to catch the potential > > > > error. > > > > > > > > Fixes: b4822e2317e8 ("usb: gadget: m66592-udc: Convert to use module_platform_driver()") > > > > Signed-off-by: Mingwei Zheng <zmw12306@gmail.com> > > > > Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com> > > > > > > Why this order of signed-off-by lines? Shouldn't yours be last? Who > > > wrote this patch? > > > > > > > I listed two names because both of us co-authored this patch. > > > > > > --- > > > > drivers/usb/gadget/udc/m66592-udc.c | 6 +++++- > > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/usb/gadget/udc/m66592-udc.c b/drivers/usb/gadget/udc/m66592-udc.c > > > > index a938b2af0944..bf408476a24c 100644 > > > > --- a/drivers/usb/gadget/udc/m66592-udc.c > > > > +++ b/drivers/usb/gadget/udc/m66592-udc.c > > > > @@ -1606,7 +1606,11 @@ static int m66592_probe(struct platform_device *pdev) > > > > ret = PTR_ERR(m66592->clk); > > > > goto clean_up2; > > > > } > > > > - clk_enable(m66592->clk); > > > > + ret = clk_enable(m66592->clk); > > > > + if (ret) { > > > > + clk_put(m66592->clk); > > > > + goto clean_up2; > > > > + } > > > > > > How did you find this and how was it tested? > > > > > > thanks, > > > > > > greg k-h > > > > We found it through a static analysis tool. > > Then you need to properly document that as our documentation says it is > required, right? > > thanks, > > greg k-h I send a PATCH v2 specifying the tool name in the commit msg for your review. Thank you so much for your suggestion! Best, Mingwei
diff --git a/drivers/usb/gadget/udc/m66592-udc.c b/drivers/usb/gadget/udc/m66592-udc.c index a938b2af0944..bf408476a24c 100644 --- a/drivers/usb/gadget/udc/m66592-udc.c +++ b/drivers/usb/gadget/udc/m66592-udc.c @@ -1606,7 +1606,11 @@ static int m66592_probe(struct platform_device *pdev) ret = PTR_ERR(m66592->clk); goto clean_up2; } - clk_enable(m66592->clk); + ret = clk_enable(m66592->clk); + if (ret) { + clk_put(m66592->clk); + goto clean_up2; + } } INIT_LIST_HEAD(&m66592->gadget.ep_list);
Add check for the return value of clk_enable() to catch the potential error. Fixes: b4822e2317e8 ("usb: gadget: m66592-udc: Convert to use module_platform_driver()") Signed-off-by: Mingwei Zheng <zmw12306@gmail.com> Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com> --- drivers/usb/gadget/udc/m66592-udc.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)