linux-kernel - Re: [PATCH v4] USB device driver of Topcliff PCH

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]

Message-id: <op.vjjeuztd7p4s8u@pikus>
Date:	Fri, 24 Sep 2010 14:41:13 +0200
From:	Michał Nazarewicz <m.nazarewicz@...sung.com>
To:	Maurus Cuelenaere <mcuelenaere@...il.com>,
	Masayuki Ohtake <masa-korg@....okisemi.com>
Cc:	Toshiharu Okada <okada533@....okisemi.com>,
	Takahiro Shimizu <shimizu394@....okisemi.com>,
	Tomoya Morinaga <morinaga526@....okisemi.com>,
	MeeGo <meego-dev@...go.com>, LKML <linux-kernel@...r.kernel.org>,
	linux-usb <linux-usb@...r.kernel.org>,
	"Wang, Qi" <qi.wang@...el.com>,
	"Wang, Yong Y" <yong.y.wang@...el.com>,
	Andrew <andrew.chih.howe.khor@...el.com>,
	Intel OTC <joel.clark@...el.com>,
	"Foster, Margie" <margie.foster@...el.com>
Subject: Re: [PATCH v4] USB device driver of Topcliff PCH

On Fri, 24 Sep 2010 14:12:21 +0200, Masayuki Ohtake <masa-korg@....okisemi.com> wrote:
> Hi Maurus and Michal.
>
> Thank you for your comments.
>
> This patch was modified.
>   Add MODULE_AUTHOR()
>   Modify struct pch_udc_stp_dma_desc.
>   Delete union pch_udc_setup_data.
>   Modify as only one "unlock"
>   Modify while loop.
>   Modify loop count.
>   Refactored function
>    -pch_udc_free_dma_chain()
>    -pch_udc_create_dma_chain()
>  and so on

As Maurus commented before, this is not a proper commit message.  Describe
what the patch does.  All other comments should go after the "---" marker.


> diff --git a/drivers/usb/gadget/pch_udc.c b/drivers/usb/gadget/pch_udc.c
> +static inline void pch_udc_writel(struct pch_udc_dev *dev,
> +				    unsigned long val, unsigned long reg)
> +{
> +	iowrite32(val, dev->base_addr + reg);
> +}

Empty line please.

> +static inline void pch_udc_bit_set(struct pch_udc_dev *dev,
> +				     unsigned long reg,
> +				     unsigned long bitmask)
> +{
> +	pch_udc_writel((dev), pch_udc_readl((dev), (reg)) | (bitmask), (reg));
> +}
> +static inline void pch_udc_bit_clr(struct pch_udc_dev *dev,
> +				     unsigned long reg,
> +				     unsigned long bitmask)
> +{
> +	pch_udc_writel((dev), pch_udc_readl((dev), (reg)) & ~(bitmask), (reg));
> +}

Definitely too many parenthesis here.  It's no longer a macro so parens around
arguments are not needed and only clutter the code.

> +
> +static inline u32 pch_udc_ep_readl(struct pch_udc_ep *ep, unsigned long reg)
> +{
> +	return ioread32(ep->dev->base_addr + ep->offset_addr + reg);
> +}
> +
> +static inline void pch_udc_ep_writel(struct pch_udc_ep *ep,
> +				    unsigned long val, unsigned long reg)
> +{
> +	iowrite32(val, ep->dev->base_addr + ep->offset_addr + reg);
> +}

Again, empty line please.

> +static inline void pch_udc_ep_bit_set(struct pch_udc_ep *ep,
> +				     unsigned long reg,
> +				     unsigned long bitmask)
> +{
> +	pch_udc_ep_writel((ep), pch_udc_ep_readl((ep), (reg)) | (bitmask),
> +			  (reg));
> +}
> +static inline void pch_udc_ep_bit_clr(struct pch_udc_ep *ep,
> +				     unsigned long reg,
> +				     unsigned long bitmask)
> +{
> +	pch_udc_ep_writel((ep), pch_udc_ep_readl((ep), (reg)) & ~(bitmask),
> +			  (reg));
> +}

And again, too many parenthesis.

> +static void process_zlp(struct pch_udc_ep *ep, struct pch_udc_request *req)
> +{
> +	struct pch_udc_dev	*dev = ep->dev;
> +
> +	dev_dbg(&dev->pdev->dev, "%s: enter  ep%d%s", __func__, ep->num,
> +		(ep->in ? "in" : "out"));
> +	/* IN zlp's are handled by hardware */
> +	complete_req(ep, req, 0);
> +
> +	/* if set_config or set_intf is waiting for ack by zlp
> +	 * then set CSR_DONE
> +	 */
> +	if (dev->set_cfg_not_acked) {
> +		dev_dbg(&dev->pdev->dev, "%s: process_zlp: csr done", __func__);
> +		pch_udc_set_csr_done(dev);
> +		dev->set_cfg_not_acked = 0;
> +	}
> +	/* setup command is ACK'ed now by zlp */
> +	if ((!dev->stall) && (dev->waiting_zlp_ack)) {

Useless parenthesis.

> +		/* clear NAK by writing CNAK in EP0_IN */
> +		pch_udc_ep_clear_nak(&(dev->ep[UDC_EP0IN_IDX]));
> +		dev->waiting_zlp_ack = 0;
> +	}
> +}


> +/**
> + * pch_udc_alloc_request() - This function allocates request structure.
> + *				It iscalled by gadget driver

"is called" -- typo.

> + * @usbep:	Reference to the USB endpoint structure
> + * @gfp:	Flag to be used while allocating memory
> + * Returns
> + *	NULL:			Failure
> + *	Allocated address:	Success
> + */


Other then those trivial mistakes, I haven't noticed any obvious
problems so I guess "Acked-by" me.

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--
--
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/