netdev - Re: [PATCH RFC] wimax/i2400m: fixup completion handling for resetting a device

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: <5515910C.8000107@cogentembedded.com>
Date:	Fri, 27 Mar 2015 20:19:08 +0300
From:	Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
To:	Nicholas Mc Guire <der.herr@...r.at>
CC:	Nicholas Mc Guire <hofrat@...dl.org>,
	Inaky Perez-Gonzalez <inaky.perez-gonzalez@...el.com>,
	linux-wimax@...el.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC] wimax/i2400m: fixup completion handling for resetting
 a device

Hello.

On 03/20/2015 10:47 AM, Nicholas Mc Guire wrote:

    Sorry for late reply, I'm pretty busy these days.

>>> wait_for_completion_timeout return 0 (timeout) or >=1 (completion) so the check
>>> for > 0 in the else branch is always true and can be dropped. The comment seems
>>> misleading as it is always going to pass the result up.

>>> The sync of the completion access with __i2400m_dev_reset_handle (which checks
>>> for   if (i2400m->reset_ctx)   could race if i2400m_reset() returns negative so
>>> the resetting of i2400m->reset_ctx == NULL is moved to the out: path.

>>> As wait_for_completion_timeout returns unsigned long not int, an appropriately
>>> named variable of type unsigned long is added and assignments fixed up.

>>     Don't try to do several things in one patch.

> normaly yes - this was marked as RFC and if I had split it up into
> 3 patches it would be hard to see how it fits together without
> actually applying them.

    You could summarize your intent in the cover letter (PATCH #0).

> The intent was to get feedback notably on moving i2400m->reset_ctx == NULL
> and if dropping the (I think missleading) comment about negative return is ok

> Should this be in seperate patches even as RFC ?

    I think the RFC patches should still conform to all the usual patch rules. 
How would we understand whether you intent to split the patch up later, if you 
didn't even write about it anywhere?

[...]

WBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html