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]

Date:   Thu, 12 Jul 2018 09:03:30 +0000
From:   Pawel Laszczak <pawell@...ence.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
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.

> > +/* 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 😊.  
 
> > +/*
> > + * gadget-if.c file is part of gadget.c file and implements interface
> > + * for gadget driver
> > + */
> > +#include "gadget-if.c"
> 
> Ugh, I know USB hcd drivers love to include .c files in the middle of them, but
> do we have to continue that crazy practice in newer drivers?
> Is it really necessary?

It's not necessary I wanted to leave this as two separate file, because gadget.c 
Is based on xhci.c and gadget-if.c define interface between gadget.c and 
USB gadget core driver. I didn't want to combine these two files. 

I will add some additional function declaration to gadget.h and remove it 
from gadget.c file. 

 
> > +	/*
> > +	 * Check the compiler generated sizes of structures that must be laid
> > +	 * out in specific ways for hardware access.
> > +	 */
> > +	BUILD_BUG_ON(sizeof(struct usbssp_doorbell_array) != 2*32/8);
> > +	BUILD_BUG_ON(sizeof(struct usbssp_slot_ctx) != 8*32/8);
> > +	BUILD_BUG_ON(sizeof(struct usbssp_ep_ctx) != 8*32/8);
> > +	/* usbssp_device has eight fields, and also
> > +	 * embeds one usbssp_slot_ctx and 31 usbssp_ep_ctx
> > +	 */
> > +	BUILD_BUG_ON(sizeof(struct usbssp_stream_ctx) != 4*32/8);
> > +	BUILD_BUG_ON(sizeof(union usbssp_trb) != 4*32/8);
> > +	BUILD_BUG_ON(sizeof(struct usbssp_erst_entry) != 4*32/8);
> > +	BUILD_BUG_ON(sizeof(struct usbssp_cap_regs) != 8*32/8);
> > +	BUILD_BUG_ON(sizeof(struct usbssp_intr_reg) != 8*32/8);
> > +	/* usbssp_run_regs has eight fields and embeds 128
> usbssp_intr_regs */
> > +	BUILD_BUG_ON(sizeof(struct usbssp_run_regs) != (8+8*128)*32/8);
> 
> I love hard-coded numbers as much as the next person, but really?  Is this
> necessary now that you have the code up and working properly?

Code is still being developed,  and is still tested. It's better to leave this code at 
this moment.  I will remove it in the feature. 

thanks,
Pawel Laszczak