netdev - Re: Please pull 'adm8211' branch of wireless-2.6

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]

Message-ID: <46EC7F33.2050206@garzik.org>
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