netdev - Re: [MeeGo-Dev][PATCH] Topcliff: Update PCH

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-next>] [day] [month] [year] [list]

Message-ID: <4C629FE5.6000204@pengutronix.de>
Date:	Wed, 11 Aug 2010 15:04:37 +0200
From:	Marc Kleine-Budde <mkl@...gutronix.de>
To:	Masayuki Ohtak <masa-korg@....okisemi.com>
CC:	meego-dev@...go.com, Wolfgang Grandegger <wg@...ndegger.com>,
	socketcan-core@...ts.berlios.de, netdev@...r.kernel.org,
	andrew.chih.howe.khor@...el.com, gregkh@...e.de,
	arjan@...ux.intel.com, qi.wang@...el.com, yong.y.wang@...el.com
Subject: Re: [MeeGo-Dev][PATCH] Topcliff: Update PCH_CAN driver to 2.6.35

Masayuki Ohtak wrote:
> CAN driver of Topcliff PCH
> 
> Topcliff PCH is the platform controller hub that is going to be used in
> Intel's upcoming general embedded platform. All IO peripherals in
> Topcliff PCH are actually devices sitting on AMBA bus. 
> Topcliff PCH has CAN I/F. This driver enables CAN function.

Thanks for your contribution. Some remarks in addition to Wolfgang's.

- Try to send patches directly with git send-email
- Rebase your tree to net-next-2.6.
- don't use global variables
- don't use that "int handle", e.g.:

> static int pch_can_msg_tx(int handle, struct pch_can_msg *msg,
> 			  struct net_device *ndev)
> {
> 	u32 id1 = 0;
> 	u32 id2 = 0;
> 	u32 data_a1 = 0;
> 	u32 data_a2 = 0;
> 	u32 data_b1 = 0;
> 	u32 data_b2 = 0;
> 	u32 tx_disable_counter = 0;
> 	u32 buffer_status = 0;
> 	u32 tx_buffer_avail = 0;
> 	u32 status;
> 	u32 i;
> 	u32 counter;
> 	enum pch_can_run_mode run_mode;
> 	int retval = 0;
> 	u32 if1_creq;
> 
> 	if ((handle == 0) || (msg == NULL)) {
> 		dev_err(&ndev->dev, "%s -> Invalid Parameter.\n", __func__);
> 		retval = -EPERM;
> 	}
> 
> 	else {
> 		/* Obatining the remap address for access. */
> 		struct can_hw *can = (struct can_hw *) handle;
> 

use a proper struct. There are numerous drawbacks, no type safety it's
not 64 safe, bad style,...

- there are several checks for handle against 0 that make no real sense

- clean up you error paths:

if (error) {
	/* error handling */
} else {
	/* do real work */
}

This leads to a big indention level in the interesting code path, making
it very hard to read. Please rework your code this way:

if (error) {
	/* error handling */
	return error;
	/* or */
	goto out_handle_error;
}

/* do real work */

- get rid of the intermediate struct pch_can_msg:
  Your data path is:
  struct can_frame -> struct pch_can_msg -> registers
  write from struct can_frame into registers directly

- what's the purpose of "p_can_os->can_callback", call the function
  directly from the interrupt handler

- implement NAPI

- get rid of "1 << BIT_SHIFT_SIX" and friend,
  use "1 << 6" or "BIT(6)" if you like defines

- use defines to set bits in struct can_frame can_id

cheers, Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


Download attachment "signature.asc" of type "application/pgp-signature" (261 bytes)