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, 8 Feb 2017 19:45:31 +0100
From:   Richard Leitner <dev@...l1n.net>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        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, dev@...l1n.net
Subject: Re: [PATCH v4] usb: misc: add USB251xB/xBi Hi-Speed Hub Controller
 Driver

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:
>>>>> From: Richard Leitner <dev@...l1n.net>

>>>>> +#define DRIVER_NAME "usb251xb"
>>>>> +#define DRIVER_DESC "Microchip USB 2.0 Hi-Speed Hub Controller"
>>>>> +#define DRIVER_VERSION "1.0"
>>>>
>>>> Is it my MUA, or all above indentations are broken?
>>>
>>> What do you mean?
>>
>> Should the strings be aligned, like the following?
>> #define DRIVER_NAME     "usb251xb"
>> #define DRIVER_DESC     "Microchip USB .."
>> #define DRIVER_VERSION	"1.0"
>
> Yep, tab vs. space indentation.

Ok, will do that for v5.

>>>> 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?

>>>>> +		/* 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?

The captital letter and period looks fine. I'll apply that for v5.

>>>
>>>>> +#else /* CONFIG_OF */
>>>>> +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 :-)

Thanks & regards,
Richard L