linux-kernel - Re: [PATCH 2/2] rt2x00: Implement support for rt2800pci

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: <200910181859.22413.IvDoorn@gmail.com>
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/