netdev - RE: [PATCH] tehuti: driver for Tehuti 10GbE network adapters

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: <20070919104438.B1FF775CAD@mail.tehutinetworks.co.il>
Date:	Wed, 19 Sep 2007 12:44:33 +0200
From:	"Alexander Indenbaum" <baum@...utinetworks.net>
To:	"'Christoph Hellwig'" <hch@...radead.org>
Cc:	<jeff@...zik.org>, <davem@...emloft.org>,
	<akpm@...ux-foundation.org>, <netdev@...r.kernel.org>,
	"'Nadav Shemer'" <nadav@...utinetworks.net>,
	"'Andy Gospodarek'" <andy@...yhouse.net>
Subject: RE: [PATCH] tehuti: driver for Tehuti 10GbE network adapters

Christoph,

Thank you for reviewing the driver and giving us this feedback. Our goal is
the driver which is in full accordance with all Linux kernel style
guidelines. Please note the fist Tehuti driver submission to netdev was by
Andy on Mars 15 and since the driver has been heavily re-factored based on
Jeff's suggestions. I agree with most of your style remarks and we'll
release the patch soon.

Meanwhile let me ask clarification about two of your remarks bellow that I'm
not sure I fully understand.

> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@...radead.org]
> Sent: Tuesday, September 18, 2007 11:47 AM
> To: Andy Gospodarek
> Cc: jeff@...zik.org; davem@...emloft.org; akpm@...ux-foundation.org;
> netdev@...r.kernel.org; baum@...utinetworks.net
> Subject: Re: [PATCH] tehuti: driver for Tehuti 10GbE network adapters
> 
> 
> Some comment from looking at the driver in the -mm tree:
> 
>  - please kill the CPU_CHIP_SWAP macros and use the normal linux
>    cpu_to_le*/le*_to_cpu and verify them using sparse.
>    See Documentation/sparse.txt on how to do that
>  - please include the linux header in the .c file, not the .h
>  - please don't redefine the dma mask constants
>  - please use the firmware loader instead of mebedding a firmware
>    image

Could you give us some pointers to docs and examples of "firmware loader"?
I'm not sure I'm familiar with such mechanism in Linux kernel.

>  - please don't invent your own debugging macros but use
>    dev_dbg and friends
>  - please kill the ENTER/RET macros
>  - please kill BDX_ASSERT
>  - the unregister_netdev directly followed by free_netdev in
>    bdx_remove look buggy, but I'm not entirely sure how to handle
>    multi-port devices properly here.

Putting dual-port issue aside, could you elaborate what is the problem in
your opinion in bdx_remove() implementation? What is wrong with calling
free_netdev() right after unregister_netdev()? Could you provide pointers
for docs and examples to correct PCI network device remove interface
implementation? 

>  - please declare bdx_ethtool_ops on file-scope and kill
>    bdx_ethtool_ops
>  - please don't put assignments into conditionals ala
> 
> 	if ((err = pci_request_regions(pdev, BDX_DRV_NAME)))
> 		goto err_dma;
> 
>    but rather write
> 
> 	err = pci_request_regions(pdev, BDX_DRV_NAME);
> 	if (err)
> 		goto err_dma;

Thank you again for your help,
Alexander Indenbaum

-
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