linux-kernel - Re: [PATCH v2 2/3] Staging: ipack: added support for the TEWS TPCI-200 carrier board

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: <20120507082007.GA30608@becoht-mvanga>
Date:	Mon, 7 May 2012 10:20:07 +0200
From:	Manohar Vanga <manohar.vanga@...n.ch>
To:	Samuel Iglesias Gonsalvez <siglesias@...lia.com>
CC:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	<devel@...verdev.osuosl.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/3] Staging: ipack: added support for the TEWS
 TPCI-200 carrier board

Hey Samuel,

Just a couple of quick comments on this patch :)

> +TPCI-200
> +--------
> +
> +* It receives the name of the mezzanine plugged in each slot by SYSFS.
> +  No autodetection supported yet, because the mezzanine driver could not be
> +  loaded at the time that the tpci200 driver loads.
> +
> +* It has a linked list with the tpci200 devices it is managing. Get rid of it
> +  and use driver_for_each_device() instead.
> +
>  Ipack
>  -----
>  
> @@ -20,4 +30,3 @@ Ipack
>    remove_device() to notify the carrier driver, or the opposite with the call to
>    the ipack_driver_ops' remove() function could be improved.
>  
> -

Is this whitespace change required?

> +#include <linux/module.h>
> +#include "tpci200.h"
> +
> +#define MODULE_NAME "tpci200"

Here you can just use the KBUILD_MODNAME variable

> +#define PFX MODULE_NAME ": "

You can also add this before all your includes:
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

> +static int tpci200_install(struct tpci200_board *tpci200)
> +{
> +	int res = 0;
> +
> +	tpci200->slots = kzalloc(TPCI200_NB_SLOT * sizeof(struct tpci200_slot), GFP_KERNEL);

Did you run checkpatch.pl on the patches? Are you ignoring the >80 char
recommendation? In that case ignore this :)

> +static struct pci_device_id tpci200_idtable[2]; /* last must be zero */
> +
> +static struct pci_driver tpci200_pci_drv = {
> +	.name = "tpci200",
> +	.id_table = tpci200_idtable,
> +	.probe = tpci200_pciprobe,
> +	.remove = __devexit_p(tpci200_pci_remove),
> +};
> +
> +static int __init tpci200_drvr_init_module(void)
> +{
> +	tpci200_idtable[0].vendor = TPCI200_VENDOR_ID;
> +	tpci200_idtable[0].device = TPCI200_DEVICE_ID;
> +	tpci200_idtable[0].subvendor = TPCI200_SUBVENDOR_ID;
> +	tpci200_idtable[0].subdevice = TPCI200_SUBDEVICE_ID;
> +	return pci_register_driver(&tpci200_pci_drv);
> +}

Can't tpci200_idtable be statically declared instead of inside the init function?

> +static void __exit tpci200_drvr_exit_module(void)
> +{
> +	struct tpci200_board *tpci200;
> +	struct list_head *element, *next;
> +
> +	list_for_each_safe(element, next, &tpci200_list) {
> +		tpci200 = list_entry(element, struct tpci200_board, list);
> +		__tpci200_pci_remove(tpci200);
> +	}

You can use list_for_each_entry_safe instead of list_for_each_safe + list_entry.
I think you've used this everywhere so this would apply to the whole patch :)

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