linux-kernel - Re: [PATCH v3] EHCI bus glue for on-chip PMC MSP USB controller.

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] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 9 Feb 2011 09:20:37 -0800
From:	Greg KH <greg@...ah.com>
To:	Anoop P A <anoop.pa@...il.com>
Cc:	gregkh@...e.de, dbrownell@...rs.sourceforge.net, ust@...x.de,
	pkondeti@...eaurora.org, stern@...land.harvard.edu, gadiyar@...com,
	alek.du@...el.com, jacob.jun.pan@...el.com,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-mips@...ux-mips.org, ralf@...ux-mips.org
Subject: Re: [PATCH v3] EHCI bus glue for on-chip PMC MSP USB controller.

On Wed, Feb 09, 2011 at 07:42:33PM +0530, Anoop P A wrote:
> > > +#ifdef CONFIG_MSP_HAS_DUAL_USB
> > > +	gpio_direction_output(MSP_PIN_USB1_HOST_DEV, 1);
> > > +#endif
> > 
> > Please don't put #defines in .c files.
> You mean #ifdef ???

Yes, sorry.

> > > +static int ehci_msp_suspend(struct device *dev)
> > > +{
> > > +	struct usb_hcd *hcd = dev_get_drvdata(dev);
> > > +	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
> > > +	unsigned long flags;
> > > +	int rc;
> > > +
> > > +	return 0;
> > > +	rc = 0;
> > > +
> > > +	if (time_before(jiffies, ehci->next_statechange))
> > > +		msleep(10);
> > 
> > Short sleep, why?
> I am not very sure. Person who originally wrote this driver is
> unreachable.Any potential issues??

Yes, suspend/resume time delays are not nice for some systems.  I would
verify that this really is necessary, and, as you are going to be the one
maintaining and responsible for the code, it would be good for you to
figure out exactly what it is doing, and why.


> > > +static int ehci_msp_resume(struct device *dev)
> > > +{
> > > +	struct usb_hcd *hcd = dev_get_drvdata(dev);
> > > +	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
> > > +
> > > +
> > > +	/* maybe restore FLADJ */
> > 
> > Don't you know?
> Not really :)

Heh, you should.

> > > +/* may be called without controller electrically present */
> > > +/* may be called with controller, bus, and devices active */
> > > +
> > 
> > What may be called?
> may be usb_hcd_msp_remove :)

Then put it in the comment block for that function, not above it, with
an extra space between it, that just causes confusion.

thanks,

greg k-h
--
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/