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:	Wed, 23 Apr 2014 11:04:47 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Michalis Pappas <mpappas@...tmail.fm>
Cc:	Greg KH <gregkh@...uxfoundation.org>, devel@...verdev.osuosl.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 0/3] staging: gdm72xx: Minor cleanup

On Wed, Apr 23, 2014 at 08:39:06AM +0800, Michalis Pappas wrote:
> After all patches have been applied, the only remaining issue on the
> TODO list is to conform to the coding standards.  The remaining issues
> reported by checkpatch.pl are probably pedantic, so if agreed, that
> task can be removed from the list too.

So I did a:
for i in $(find drivers/staging/gdm72xx/ -name \*.c) ; do ./scripts/checkpatch.pl --strict -f $i 2>&1 ; done | tee err-list

I didn't look through all the issues but the ones I looked at seemed
valid.  The "no space after a cast" rule is good because cast precedence
used to be a common source of bugs.

The code in extract_qos_list() was badly mangled to get around the 80
character limit and checkpatch.pl finds that.  That function is very
ugly.  Just reverse the if statements instead of jamming the code up
								against
								the
								far
								right
								side of
								the
								screen.

		if (!qcb->csr[i].enabled)
			continue;
		if (qcb->csr[i].qos_buf_count >= qcb->qos_limit_size)
			continue;
		if (list_empty(&qcb->qos_list[i]))
			continue;

Also why is returning a u32?  Kernel style is int.  But this doesn't
return errors and the callers don't check so it should be void.  This
driver suffers from prefering u32 over int in many places.

This was my only static checker warning (harmless):
drivers/staging/gdm72xx/gdm_wimax.c:780 gdm_wimax_transmit_pkt()
	warn: we tested 'len' before and it was 'true'

In gdm_usb_send(), the "BUG_ON(len > TX_BUF_SIZE - padding - 1);" should
be proper error handling.

I didn't really do a proper review of this code when I saw all the
checkpatch.pl complaints still.

regards,
dan carpenter

--
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