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:	Sun, 18 Oct 2009 18:59:21 +0200
From:	Ivo van Doorn <ivdoorn@...il.com>
To:	Bartlomiej Zolnierkiewicz <bzolnier@...il.com>
Cc:	John Linville <linville@...driver.com>,
	linux-wireless@...r.kernel.org, users@...x00.serialmonkey.com,
	Alban Browaeys <prahal@...oo.com>,
	Benoit PAPILLAULT <benoit.papillault@...e.fr>,
	Felix Fietkau <nbd@...nwrt.org>,
	Luis Correia <luis.f.correia@...il.com>,
	Mattias Nissler <mattias.nissler@....de>,
	Mark Asselstine <asselsm@...il.com>,
	Xose Vazquez Perez <xose.vazquez@...il.com>,
	"linux-kernel" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] rt2x00: Implement support for rt2800pci

> I also used the opportunity to take a closer look at this driver and
> it seems that it needlessly adds around 2 KLOC to kernel by duplicating
> the common content of rt2800usb.h to rt2800pci.h instead of moving it
> to the shared header (like it is done in the staging crap drivers):
> 
> $ wc -l drivers/net/wireless/rt2x00/rt2800usb.h drivers/net/wireless/rt2x00/rt2800pci.h
>   1951 drivers/net/wireless/rt2x00/rt2800usb.h
>   1960 drivers/net/wireless/rt2x00/rt2800pci.h
>   3911 total
> 
> $ diff -u drivers/net/wireless/rt2x00/rt2800usb.h drivers/net/wireless/rt2x00/rt2800pci.h|diffstat
>  rt2800pci.h |  213 +++++++++++++++++++++++++++++++-----------------------------
>   1 file changed, 111 insertions(+), 102 deletions(-)

Creating rt2800.h with common register defines has been on the todo list for some time already,
it will likely happen in the future, but until I get around to update my debugfs scripts the seperate
rt2800pci.h and rt2800usb.h files make debugging and register analysis a bit easier.

> Similarly it looks like most of the code between rt2800usb.c and rt2800pci.c
> could also be shared (up to another 2 KLOC saved) by adding abstraction layer
> for accessing chipset registers over different buses (again like it is done
> in staging crap drivers):
> 
> $ wc -l drivers/net/wireless/rt2x00/rt2800usb.c drivers/net/wireless/rt2x00/rt2800pci.c
>   3077 drivers/net/wireless/rt2x00/rt2800usb.c
>   3323 drivers/net/wireless/rt2x00/rt2800pci.c
>   6400 total
> 
> $ diff -u drivers/net/wireless/rt2x00/rt2800usb.c drivers/net/wireless/rt2x00/rt2800pci.c|diffstat
>  rt2800pci.c | 2190 +++++++++++++++++++++++++++++++++---------------------------
>  1 file changed, 1218 insertions(+), 972 deletions(-)

I don't agree on this, for starters the whole "abstraction layer as done in the staging driver, really
obfuscated the code in multiple areas, so whatever abstraction layer will be needed for rt2x00 it
should be done much cleaner without the ugly defines and crap like that. So unless there is a _clean_
and very _readable_ solution for such abstraction layer I might consider it.

Secondly, you can't merge them until both drivers would correctly for all users/chipsets, because only
then you can see what registers are initialized equaly, and where potential pitfalls are between the
2 busses.

> All in all, the total amount of the kernel code needed for implementing
> rt2800pci functionality should 1-2 KLOC instead of the current 5 KLOC.

Ivo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists