linux-kernel - Re: [PATCH v4] usb: misc: add USB251xB/xBi Hi-Speed Hub Controller Driver

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, 08 Feb 2017 21:20:44 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Richard Leitner <dev@...l1n.net>,
        Richard Leitner <richard.leitner@...data.com>,
        Greg KH <gregkh@...uxfoundation.org>
Cc:     linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, robh+dt@...nel.org,
        mark.rutland@....com, stern@...land.harvard.edu
Subject: Re: [PATCH v4] usb: misc: add USB251xB/xBi Hi-Speed Hub Controller
 Driver

On Wed, 2017-02-08 at 19:45 +0100, Richard Leitner wrote:
> On 02/08/2017 05:40 PM, Andy Shevchenko wrote:
> > On Wed, 2017-02-08 at 16:17 +0100, Richard Leitner wrote:
> > > On 02/08/2017 02:59 PM, Greg KH wrote:
> > > > On Wed, Feb 08, 2017 at 03:21:08PM +0200, Andy Shevchenko wrote:
> > > > > On Wed, 2017-02-08 at 09:52 +0100, Richard Leitner wrote:

> > > > > Above doesn't make much sense. Why not to use
> > > > > 
> > > > > > BIT(bit)
> > > > > 
> > > > > and
> > > > > 
> > > > > & ~BIT(bit)
> > > > > 
> > > > > in place?
> > > > 
> > > > I thought we already had functions to do this for you.  Don't
> > > > write
> > > > new
> > > > ones "by hand" either wya.
> > > 
> > > Which functions do you mean? I only found set_bit() and
> > > clear_bit()
> > > from
> > > atomic_ops. But those operate on "unsigned long" variables. From
> > > the
> > > documentation:
> > > 	Native atomic bit operations are defined to operate
> > > 	on objects aligned to the size of an "unsigned long"
> > > 	C data type, and are least of that size.
> > 
> > __set_bit(), __clear_bit() -- non-atomic variants, but you are
> > right,
> > that (unsigned long) exactly the point I didn't propose them.
> 
> So the preferred solution is the BIT() stuff?

I think BIT() macro in place. Otherwise you'll need a temporary unsigned
long variable. If you already have one, then __*_bit() would work.

> +		/* the first data byte transferred tells
> > > > > > the
> > > > > > hub how
> > > > > > many data
> > > > > > +		 * bytes will follow (byte count)
> > > > > > +		 */
> > > > > 
> > > > > I'm not sure this is good formatted comment for USB subsystem.
> > > > 
> > > > Looks fine to me, why do you think it is incorrect?
> > 
> > I would do like
> > 
> > /*
> >  * The multi-line
> >  * comment.
> >  */
> > 
> > Capital letter, period at the end, first empty line (unlike in net
> > subsystem).
> 
> So what's the preferred format? Empty line at the beginning or not?

Both fine to me. It's not a real code anyway.

> > > > > > +static int usb251xb_get_ofdata(struct usb251xb *hub,
> > > > > > +			       struct usb251xb_data *data)
> > > > > > +{
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +#endif /* CONFIG_OF */
> > > > > 
> > > > > I don't think it's a good idea to have those ugly #ifdef.
> > > > 
> > > > How can it be removed?
> > 
> > __maybe_unused for function, device_property_*() instead of
> > of_property_*() calls.
> > 
> > Something like that. But if you are insisting this is *only* OF
> > available hardware or we don't care, I'll not object.
> 
> In usb3503.c and usb4604.c we have that #ifdef CONFIG_OF too. IMHO
> those 
> drivers should use the same solution here. But you guys are the ones 
> with tons of kernel coding experience, so just say how it should be
> done :-)

I'll agree with whatever Greg wants here.

-- 
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Intel Finland Oy