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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Sat, 15 Sep 2007 20:56:19 -0400
From:	Jeff Garzik <jeff@...zik.org>
To:	Michael Wu <flamingice@...rmilk.net>,
	David Miller <davem@...emloft.net>
CC:	"John W. Linville" <linville@...driver.com>,
	netdev@...r.kernel.org, linux-wireless@...r.kernel.org
Subject: Re: Please pull 'adm8211' branch of wireless-2.6

Michael Wu wrote:
> On Saturday 15 September 2007 17:32, Jeff Garzik wrote:
>> Review summary:  many minor issues, only one major one:  irq handler loop
>>
> CCing me would help.

Sorry.  I just hit 'reply to all'...  apparently you were not CC'd on 
the submission.


>>> +	if (flags & IFF_PROMISC)
>>> +		dev->flags |= IEEE80211_HW_RX_INCLUDES_FCS;
>>> +	else
>>> +		dev->flags &= ~IEEE80211_HW_RX_INCLUDES_FCS;
>> why does promisc dictate inclusion of FCS?
>>
> Because that's the way the hardware works.

Why not always include it, regardless of promisc?


>>> +	} while (count++ < 20);
>> NAK -- talk about back to the past.
>>
>> It's bogus to loop in the interrupt handler, then loop again in both RX
>> and TX paths.  You are getting close to reinventing the wheel here.
>>
>> Use NAPI rather than doing such a loop.
>>
> This is old interrupt handler code from Jouni's original driver. I never 
> bothered to replace it with the simpler designs used in newer drivers I've 
> worked on.
> 
> Also - mac80211 drivers have no access to NAPI.

Not true as of net-2.6.24.



>>> +/* CSR (Host Control and Status Registers) */
>>> +struct adm8211_csr {
>> [snip]
>>> +} __attribute__ ((packed));
>> attributed(packed) is unneccesary here, and its use results in
>> sub-optimal code
>>
> How? Doesn't this just turn into a bunch of offsets?

It depends on how its used in the code.


>> enums are preferred.  they do not disappear at the cpp stage, and confer
>> type information.
>>
> I'd rather not.

Elaboration?

Remember this code is not just for you, but for all people working with 
the code.  It makes debugging (and sometimes tracebacks) much more 
readable, since it hasn't been eaten by cpp.

Thanks,

	Jeff


-
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

Powered by blists - more mailing lists