lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:	Thu, 30 May 2013 10:56:29 +0800
From:	Yuan-Hsin Chen <yuanlmm@...il.com>
To:	Felipe Balbi <balbi@...com>
Cc:	Yuan-Hsin Chen <yhchen@...aday-tech.com>,
	Greg KH <gregkh@...uxfoundation.org>,
	USB list <linux-usb@...r.kernel.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] usb: gadget: add Faraday fotg210_udc driver

Hi,

On Thu, May 30, 2013 at 7:55 AM, Felipe Balbi <balbi@...com> wrote:
> Hi,
>
> On Wed, May 29, 2013 at 06:31:50PM +0000, Yuan-Hsin Chen wrote:
>> Faraday fotg210 udc driver supports only Bulk transfer so far.
>> fotg210 could be configured as an USB2.0 peripheral.
>>
>> This driver is tested with mass storage gadget driver on Faraday
>> EVB a369.
>>
>> Signed-off-by: Yuan-Hsin Chen <yhchen@...aday-tech.com>
>
> Run through checkpatch.pl --strict and fix all errors, warnings and all
> checks which make sense.
>
> WARNING: please write a paragraph that describes the config symbol fully
> #81: FILE: drivers/usb/gadget/Kconfig:196:
> +config USB_FOTG210_UDC
>
> CHECK: braces {} should be used on all arms of this statement
> #183: FILE: drivers/usb/gadget/fotg210-udc.c:76:
> +       if (ep->epnum) {
> [...]
> +       } else
> [...]
>
> WARNING: line over 80 characters
> #190: FILE: drivers/usb/gadget/fotg210-udc.c:83:
> +static void fotg210_fifo_ep_mapping(struct fotg210_ep *ep, u32 epnum, u32 dir_in)
>
> WARNING: line over 80 characters
> #237: FILE: drivers/usb/gadget/fotg210-udc.c:130:
> +static void fotg210_set_mps(struct fotg210_ep *ep, u32 epnum, u32 mps, u32 dir_in)
>
> WARNING: line over 80 characters
> #241: FILE: drivers/usb/gadget/fotg210-udc.c:134:
> +       u32 offset = dir_in ? FOTG210_INEPMPSR(epnum) : FOTG210_OUTEPMPSR(epnum);
>
> WARNING: line over 80 characters
> #349: FILE: drivers/usb/gadget/fotg210-udc.c:242:
> +static void fotg210_ep_free_request(struct usb_ep *_ep, struct usb_request *_req)
>
> WARNING: Avoid CamelCase: <DMATFNR_ACC_Fn>
> #372: FILE: drivers/usb/gadget/fotg210-udc.c:265:
> +               value |= DMATFNR_ACC_Fn(ep->epnum - 1);
>
> WARNING: line over 80 characters
> #418: FILE: drivers/usb/gadget/fotg210-udc.c:311:
> +               value = ioread32(ep->fotg210->reg + FOTG210_FIBCR(ep->epnum - 1));
>
> WARNING: line over 80 characters
> #420: FILE: drivers/usb/gadget/fotg210-udc.c:313:
> +               iowrite32(value, ep->fotg210->reg + FOTG210_FIBCR(ep->epnum - 1));
>
> WARNING: line over 80 characters
> #441: FILE: drivers/usb/gadget/fotg210-udc.c:334:
> +                       length = ioread32(ep->fotg210->reg + FOTG210_FIBCR(ep->epnum - 1));
>
> WARNING: Avoid CamelCase: <FIBCR_BCFx>
> #442: FILE: drivers/usb/gadget/fotg210-udc.c:335:
> +                       length &= FIBCR_BCFx;
>
> WARNING: Prefer netdev_dbg(netdev, ... then dev_dbg(dev, ... then pr_debug(...  to printk(KERN_DEBUG ...
> #456: FILE: drivers/usb/gadget/fotg210-udc.c:349:
> +               printk(KERN_DEBUG "dma_mapping_error\n");
>
> CHECK: Alignment should match open parenthesis
> #461: FILE: drivers/usb/gadget/fotg210-udc.c:354:
> +       dma_sync_single_for_device(NULL, d, length,
> +                       ep->dir_in ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
>
> WARNING: line over 80 characters
> #476: FILE: drivers/usb/gadget/fotg210-udc.c:369:
> +static void fotg210_ep0_queue(struct fotg210_ep *ep, struct fotg210_request *req)
>
> CHECK: braces {} should be used on all arms of this statement
> #483: FILE: drivers/usb/gadget/fotg210-udc.c:376:
> +               if (req->req.length) {
> [...]
> +               } else
> [...]
>
> WARNING: Prefer netdev_dbg(netdev, ... then dev_dbg(dev, ... then pr_debug(...  to printk(KERN_DEBUG ...
> #486: FILE: drivers/usb/gadget/fotg210-udc.c:379:
> +                       printk(KERN_DEBUG "%s : req->req.length = 0x%x\n",
>
> CHECK: Alignment should match open parenthesis
> #487: FILE: drivers/usb/gadget/fotg210-udc.c:380:
> +                       printk(KERN_DEBUG "%s : req->req.length = 0x%x\n",
> +                               __func__, req->req.length);
>
> CHECK: braces {} should be used on all arms of this statement
> #492: FILE: drivers/usb/gadget/fotg210-udc.c:385:
> +               if (!req->req.length)
> [...]
> +               else {
> [...]
>
> WARNING: line over 80 characters
> #495: FILE: drivers/usb/gadget/fotg210-udc.c:388:
> +                       u32 value = ioread32(ep->fotg210->reg + FOTG210_DMISGR0);
>
> WARNING: Prefer netdev_dbg(netdev, ... then dev_dbg(dev, ... then pr_debug(...  to printk(KERN_DEBUG ...
> #680: FILE: drivers/usb/gadget/fotg210-udc.c:573:
> +               printk(KERN_DEBUG "    0x%x\n", data);
>
> WARNING: Prefer netdev_dbg(netdev, ... then dev_dbg(dev, ... then pr_debug(...  to printk(KERN_DEBUG ...
> #691: FILE: drivers/usb/gadget/fotg210-udc.c:584:
> +               printk(KERN_DEBUG "    0x%x\n", data);
>
> WARNING: Prefer netdev_dbg(netdev, ... then dev_dbg(dev, ... then pr_debug(...  to printk(KERN_DEBUG ...
> #696: FILE: drivers/usb/gadget/fotg210-udc.c:589:
> +               printk(KERN_DEBUG "    0x%x\n", data);
>
> WARNING: Prefer netdev_dbg(netdev, ... then dev_dbg(dev, ... then pr_debug(...  to printk(KERN_DEBUG ...
> #702: FILE: drivers/usb/gadget/fotg210-udc.c:595:
> +               printk(KERN_DEBUG "    0x%x\n", data);
>
> WARNING: Prefer netdev_dbg(netdev, ... then dev_dbg(dev, ... then pr_debug(...  to printk(KERN_DEBUG ...
> #741: FILE: drivers/usb/gadget/fotg210-udc.c:634:
> +       printk(KERN_DEBUG "request error!!\n");
>
> WARNING: line over 80 characters
> #744: FILE: drivers/usb/gadget/fotg210-udc.c:637:
> +static void fotg210_set_address(struct fotg210_udc *fotg210, struct usb_ctrlrequest *ctrl)
>
> WARNING: Avoid CamelCase: <ctrl->wValue>
> #746: FILE: drivers/usb/gadget/fotg210-udc.c:639:
> +       if (ctrl->wValue >= 0x0100)
>
> CHECK: braces {} should be used on all arms of this statement
> #746: FILE: drivers/usb/gadget/fotg210-udc.c:639:
> +       if (ctrl->wValue >= 0x0100)
> [...]
> +       else {
> [...]
>
> WARNING: line over 80 characters
> #754: FILE: drivers/usb/gadget/fotg210-udc.c:647:
> +static void fotg210_set_feature(struct fotg210_udc *fotg210, struct usb_ctrlrequest *ctrl)
>
> WARNING: Avoid CamelCase: <ctrl->bRequestType>
> #756: FILE: drivers/usb/gadget/fotg210-udc.c:649:
> +       switch (ctrl->bRequestType & USB_RECIP_MASK) {
>
> WARNING: Avoid CamelCase: <ctrl->wIndex>
> #765: FILE: drivers/usb/gadget/fotg210-udc.c:658:
> +               epnum = le16_to_cpu(ctrl->wIndex) & USB_ENDPOINT_NUMBER_MASK;
>
> WARNING: line over 80 characters
> #779: FILE: drivers/usb/gadget/fotg210-udc.c:672:
> +static void fotg210_clear_feature(struct fotg210_udc *fotg210, struct usb_ctrlrequest *ctrl)
>
> WARNING: line over 80 characters
> #821: FILE: drivers/usb/gadget/fotg210-udc.c:714:
> +static void fotg210_get_status(struct fotg210_udc *fotg210, struct usb_ctrlrequest *ctrl)
>
> WARNING: line over 80 characters
> #836: FILE: drivers/usb/gadget/fotg210-udc.c:729:
> +                               fotg210_is_epnstall(fotg210->ep[epnum]) << USB_ENDPOINT_HALT;
>
> WARNING: line over 80 characters
> #854: FILE: drivers/usb/gadget/fotg210-udc.c:747:
> +static int fotg210_setup_packet(struct fotg210_udc *fotg210, struct usb_ctrlrequest *ctrl)
>
> CHECK: braces {} should be used on all arms of this statement
> #870: FILE: drivers/usb/gadget/fotg210-udc.c:763:
> +       if ((ctrl->bRequestType & USB_TYPE_MASK) == USB_TYPE_STANDARD) {
> [...]
> +       } else
> [...]
>
> WARNING: Avoid CamelCase: <ctrl->bRequest>
> #871: FILE: drivers/usb/gadget/fotg210-udc.c:764:
> +               switch (ctrl->bRequest) {
>
> CHECK: braces {} should be used on all arms of this statement
> #902: FILE: drivers/usb/gadget/fotg210-udc.c:795:
> +       if (!list_empty(&ep->queue) && !ep->dir_in) {
> [...]
> +       } else
> [...]
>
> CHECK: braces {} should be used on all arms of this statement
> #921: FILE: drivers/usb/gadget/fotg210-udc.c:814:
> +       if ((!list_empty(&ep->queue)) && (ep->dir_in)) {
> [...]
> +       } else
> [...]
>
> WARNING: Prefer netdev_info(netdev, ... then dev_info(dev, ... then pr_info(...  to printk(KERN_INFO ...
> #989: FILE: drivers/usb/gadget/fotg210-udc.c:882:
> +                       printk(KERN_INFO"fotg210 udc reset\n");
>
> WARNING: Prefer netdev_info(netdev, ... then dev_info(dev, ... then pr_info(...  to printk(KERN_INFO ...
> #995: FILE: drivers/usb/gadget/fotg210-udc.c:888:
> +                       printk(KERN_INFO"fotg210 udc suspend\n");
>
> WARNING: Prefer netdev_info(netdev, ... then dev_info(dev, ... then pr_info(...  to printk(KERN_INFO ...
> #1001: FILE: drivers/usb/gadget/fotg210-udc.c:894:
> +                       printk(KERN_INFO"fotg210 udc resume\n");
>
> WARNING: Prefer netdev_info(netdev, ... then dev_info(dev, ... then pr_info(...  to printk(KERN_INFO ...
> #1007: FILE: drivers/usb/gadget/fotg210-udc.c:900:
> +                       printk(KERN_INFO"fotg210 iso sequence error\n");
>
> WARNING: Prefer netdev_info(netdev, ... then dev_info(dev, ... then pr_info(...  to printk(KERN_INFO ...
> #1013: FILE: drivers/usb/gadget/fotg210-udc.c:906:
> +                       printk(KERN_INFO"fotg210 iso sequence abort\n");
>
> WARNING: Prefer netdev_info(netdev, ... then dev_info(dev, ... then pr_info(...  to printk(KERN_INFO ...
> #1020: FILE: drivers/usb/gadget/fotg210-udc.c:913:
> +                       printk(KERN_INFO"fotg210 transferred 0 byte\n");
>
> WARNING: Prefer netdev_info(netdev, ... then dev_info(dev, ... then pr_info(...  to printk(KERN_INFO ...
> #1027: FILE: drivers/usb/gadget/fotg210-udc.c:920:
> +                       printk(KERN_INFO"fotg210 received 0 byte\n");
>
> WARNING: Prefer netdev_info(netdev, ... then dev_info(dev, ... then pr_info(...  to printk(KERN_INFO ...
> #1047: FILE: drivers/usb/gadget/fotg210-udc.c:940:
> +                       printk(KERN_INFO "fotg210 CX command abort\n");
>
> WARNING: Prefer netdev_info(netdev, ... then dev_info(dev, ... then pr_info(...  to printk(KERN_INFO ...
> #1051: FILE: drivers/usb/gadget/fotg210-udc.c:944:
> +                       printk(KERN_INFO "fotg210 ep0 setup\n");
>
> WARNING: line over 80 characters
> #1054: FILE: drivers/usb/gadget/fotg210-udc.c:947:
> +                               if (fotg210->driver->setup(&fotg210->gadget, &ctrl) < 0)
>
> WARNING: Prefer netdev_info(netdev, ... then dev_info(dev, ... then pr_info(...  to printk(KERN_INFO ...
> #1060: FILE: drivers/usb/gadget/fotg210-udc.c:953:
> +                       printk(KERN_INFO "fotg210 cmd end\n");
>
> WARNING: Prefer netdev_info(netdev, ... then dev_info(dev, ... then pr_info(...  to printk(KERN_INFO ...
> #1070: FILE: drivers/usb/gadget/fotg210-udc.c:963:
> +                       printk(KERN_INFO "fotg210 ep0 fail\n");
>
> CHECK: Alignment should match open parenthesis
> #1129: FILE: drivers/usb/gadget/fotg210-udc.c:1022:
> +       iowrite32(GMIR_MHC_INT | GMIR_MOTG_INT | GMIR_INT_POLARITY,
> +               fotg210->reg + FOTG210_GMIR);
>
> WARNING: space prohibited before semicolon
> #1237: FILE: drivers/usb/gadget/fotg210-udc.c:1130:
> +       for (i = 0; i < FOTG210_MAX_NUM_EP ; i++) {
>
> CHECK: Alignment should match open parenthesis
> #1243: FILE: drivers/usb/gadget/fotg210-udc.c:1136:
> +                       list_add_tail(&fotg210->ep[i]->ep.ep_list,
> +                                    &fotg210->gadget.ep_list);
>
> WARNING: kfree(NULL) is safe this check is probably not required
> #1291: FILE: drivers/usb/gadget/fotg210-udc.c:1184:
> +       if (fotg210)
> +               kfree(fotg210);
>
> CHECK: No space is necessary after a cast
> #1298: FILE: drivers/usb/gadget/fotg210-udc.c:1191:
> +       .driver         = {
> +               .name = (char *) udc_name,
>
> WARNING: line over 80 characters
> #1477: FILE: drivers/usb/gadget/fotg210.h:162:
> +#define EPMAP_FIFONO(ep, dir)  ((((ep) - 1) << ((ep) - 1) * 8) << ((dir) ? 0 : 4))
>
> WARNING: line over 80 characters
> #1478: FILE: drivers/usb/gadget/fotg210.h:163:
> +#define EPMAP_FIFONOMSK(ep, dir)       ((3 << ((ep) - 1) * 8) << ((dir) ? 0 : 4))
>
> CHECK: spinlock_t definition without comment
> #1549: FILE: drivers/usb/gadget/fotg210.h:234:
> +       spinlock_t              lock;
>
> total: 0 errors, 45 warnings, 13 checks, 1472 lines checked
>
> Your patch has style problems, please review.
>
> If any of these errors are false positives, please report
> them to the maintainer, see CHECKPATCH in MAINTAINERS.
>

Sorry about my negligence. I'll fix and resend it later.
Thanks for your time.

Yuan-Hsin

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

Powered by blists - more mailing lists