| 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
| ||
|
Date: Sat, 10 Feb 2007 12:05:29 -0200
From: Marcelo Tosatti <marcelo@...ck.org>
To: Arnd Bergmann <arnd@...db.de>
Cc: Marcelo Tosatti <marcelo@...ck.org>,
netdev <netdev@...r.kernel.org>, Jeff Garzik <jgarzik@...ox.com>,
"John W. Linville" <linville@...hat.com>,
Dan Williams <dcbw@...hat.com>,
"Luis R. Rodriguez" <mcgrof@...il.com>,
Arnaldo Carvalho de Melo <acme@...stprotocols.net>
Subject: Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
On Sat, Jan 27, 2007 at 02:53:07AM +0100, Arnd Bergmann wrote:
> > +/** Command Processing States and Options */
> > +#define HostCmd_STATE_IDLE 0x0000
> > +#define HostCmd_STATE_IN_USE_BY_HOST 0x0001
> > +#define HostCmd_STATE_IN_USE_BY_MINIPORT 0x0002
> > +#define HostCmd_STATE_FINISHED 0x000f
>
> No SilLYcAps please
Done.
> Most of these are unused as well, I guess it would be good
> to go through the whole list and see what can be removed.
Done.
> > +/**
> > + * @brief This function reads of the packet into the upload buff,
> > + * wake up the main thread and initialise the Rx callack.
> > + *
> > + * @param urb pointer to struct urb
> > + * @return N/A
> > + */
> > +static void if_usb_receive(struct urb *urb)
> > +{
>
> This function is a little long, there is a switch() statement in it
> that can probably be converted into one function per case.
Moved processing of CMD_TYPE_REQUEST and CMD_TYPE_DATA to inline
functions.
> > +/**
> > + * @brief Given a usb_card_rec return its wlan_private
> > + * @param card pointer to a usb_card_rec
> > + * @return pointer to wlan_private
> > + */
> > +wlan_private *libertas_sbi_get_priv(void *card)
> > +{
> > + struct usb_card_rec *cardp = (struct usb_card_rec *)card;
> > + return (wlan_private *)cardp->priv;
> > +}
>
> Don't do explicit casts of void* pointers.
Fixed.
> > + data = *((int *)(wrq->u.name + SUBCMD_OFFSET));
>
> You use this weird line in many places, it would be good to make it
> a helper function.
Converted to a macro.
> > +static u8 Is_Command_Allowed_In_PS(u16 Command)
> > +{
> > + int count = sizeof(Commands_Allowed_In_PS)
> > + / sizeof(Commands_Allowed_In_PS[0]);
> > + int i;
> > +
> > + for (i = 0; i < count; i++) {
> > + if (Command == cpu_to_le16(Commands_Allowed_In_PS[i]))
> > + return 1;
> > + }
> > +
> > + return 0;
> > +}
>
> In places where you use variables that are strictly little-endian,
> it would be nice to use the __le32/__le16 types instead of u32/u16.
>
> > +/**
> > + * @brief This function handles the command response
> > + *
> > + * @param priv A pointer to wlan_private structure
> > + * @return WLAN_STATUS_SUCCESS or WLAN_STATUS_FAILURE
> > + */
> > +int libertas_process_rx_command(wlan_private * priv)
> > +{
>
> This function is incredibly long, to the point where it gets
> unreadable.
Moved the switch() to an inline function, that should make it much more
readable.
> > +static struct dentry *libertas_dir = NULL;
> > +static const int big_buffer_len = 4096;
> > +static char big_buffer[4096];
> > +static DECLARE_MUTEX(big_buffer_sem);
>
> This seems to provide a generalized interface for buffering debugfs
> files. Have you considered using the existing simple_attribute
> and seq_file interfaces instead?
>
> Even if they don't work for this, I would consider it cleaner to
> use get_zeroed_page/free_page instead of the global buffer here.
Borrowed the idea from bcm43xx, but yeah, I agree that using
get_zeroed_page/free_page is cleaner and safer.
Done.
> > +static struct file_operations libertas_devinfo_fops = {
> > + .owner = THIS_MODULE,
> > + .open = open_file_generic,
> > + .write = write_file_dummy,
> > + .read = libertas_dev_info,
> > +};
> > +
> > +static struct file_operations libertas_getscantable_fops = {
> > + .owner = THIS_MODULE,
> > + .open = open_file_generic,
> > + .write = write_file_dummy,
> > + .read = libertas_getscantable,
> > +};
> > +
> > +static struct file_operations libertas_sleepparams_fops = {
> > + .owner = THIS_MODULE,
> > + .open = open_file_generic,
> > + .write = libertas_sleepparams_write,
> > + .read = libertas_sleepparams_read,
> > +};
>
> I would guess you can better express this with some array, like
>
> #define FOPS(read, write) { \
> .owner = THIS_MODULE, \
> .open = open_file_generic, \
> .read = (read), \
> .write = (write), \
> }
>
> struct libertas_debugfs_files {
> char *name;
> int perm;
> struct file_operations fops;
> } debugfs_files = {
> { "devinfo", 0444, FOPS(libertas_dev_info, NULL), },
> { "getscantable", 0444, FOPS(libertas_getscantable, NULL), },
> { "sleepparams", 0644, FOPS(libertas_sleepparams_read,
> libertas_sleepparams_write), },
> ...
> };
>
> > +void libertas_debugfs_init_one(wlan_private *priv, struct net_device *dev)
> > +{
> > + if (!libertas_dir)
> > + goto exit;
> > +
> > + priv->debugfs_dir = debugfs_create_dir(dev->name, libertas_dir);
> > + if (!priv->debugfs_dir)
> > + goto exit;
> > +
> > + priv->debugfs_devinfo = debugfs_create_file("info", 0444,
> > + priv->debugfs_dir,
> > + priv,
> > + &libertas_devinfo_fops);
>
> And then here do a loop over that array.
Done.
-
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
Powered by blists - more mailing lists