linux-kernel - Re: [PATCH 05/31] usb: usbssp: Added first part of initialization sequence.

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]

Message-ID: <20180712090924.GA8255@kroah.com>
Date:   Thu, 12 Jul 2018 11:09:24 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Pawel Laszczak <pawell@...ence.com>
Cc:     "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        Felipe Balbi <balbi@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Lukasz Tyrala <ltyrala@...ence.com>,
        Alan Douglas <adouglas@...ence.com>
Subject: Re: [PATCH 05/31] usb: usbssp: Added first part of initialization
 sequence.

On Thu, Jul 12, 2018 at 09:03:30AM +0000, Pawel Laszczak wrote:
> > > +/* USB 2.0 hardware LMP capability*/
> > > +#define USBSSP_HLC			(1 << 19)
> > > +#define USBSSP_BLC			(1 << 20)
> > 
> > Again, BIT() please.
> > 
> > > +int usbssp_handshake(void __iomem *ptr, u32 mask, u32 done, int usec)
> > > +{
> > > +	u32	result;
> > 
> > Some places you use tabs for the variable declarations, and some you do
> > not.  Pick a single style and stick to it please.
> > 
> > > +
> > > +	do {
> > > +		result = readl(ptr);
> > > +		if (result == ~(u32)0)	/* card removed */
> > > +			return -ENODEV;
> > > +		result &= mask;
> > > +		if (result == done)
> > > +			return 0;
> > > +		udelay(1);
> > > +		usec--;
> > > +	} while (usec > 0);
> > > +	return -ETIMEDOUT;
> > 
> > We don't have a built-in kernel function to do this type of thing already?
> > That's sad.  Oh well...
> > 
> > > +int usbssp_init(struct usbssp_udc *usbssp_data) {
> > > +	int retval = 0;
> > > +
> > > +	usbssp_dbg_trace(usbssp_data, trace_usbssp_dbg_init,
> > "usbssp_init");
> > > +
> > > +	spin_lock_init(&usbssp_data->lock);
> > > +	spin_lock_init(&usbssp_data->irq_thread_lock);
> > > +
> > > +	//TODO: memory initialization
> > > +	//retval = usbssp_mem_init(usbssp_data, GFP_KERNEL);
> > > +
> > > +	usbssp_dbg_trace(usbssp_data, trace_usbssp_dbg_init,
> > > +			"Finished usbssp_init");
> > 
> > When your trace functions do nothing but say "entered a function", and
> > "exited a function", why even have them?  ftrace can provide that for you
> > already, no need to overload that on the tracing framework, right?
> 
> Do you suggest to use only: 
> 	trace_usbssp_dbg_init("Finished usbssp_init"); 
> instead: 
> 	usbssp_dbg(usbssp_data, "%pV\n", "Finished usbssp_init");
> 	trace_usbssp_dbg_init("Finished usbssp_init");
> ?
> 
> I'm simple re-used the code from XHCI driver. It's really redundant, 
> but I don't know the intention of author 😊.  

Why are any of those lines needed?  Doesn't ftrace work properly for
you?

And yeah, if xhci has this it should be removed from there as well.

thanks,

greg k-h