| 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
| ||
|
Message-ID: <48042F7F.8030608@hartkopp.net>
Date: Tue, 15 Apr 2008 06:30:55 +0200
From: Oliver Hartkopp <oliver@...tkopp.net>
To: Greg KH <greg@...ah.com>
CC: linux-usb@...r.kernel.org, netdev@...r.kernel.org,
Alan Cox <alan@...rguk.ukuu.org.uk>,
Filip Aben <f.aben@...ion.com>,
Paulius Zaleckas <paulius.zaleckas@...tonika.lt>,
ajb@...eresystems.co.uk
Subject: Re: [RFC] Patch to option HSO driver to the kernel
Just some obvious things to reduce the source code hopping for the
review and to reduce the source code size.
Regards,
Oliver
> +
> +#define DRIVER_VERSION "1.2"
> +#define MOD_AUTHOR "Option Wireless"
> +#define MOD_DESCRIPTION "USB High Speed Option driver"
> +#define MOD_LICENSE "GPL"
> +
>
Please move the MODULE_OWNER(), MODULE_LICENSE(), etc. macros from the
end of this file directly to this point.
> +/*****************************************************************************/
> +/* Prototypes */
> +/*****************************************************************************/
> +/* Network interface functions */
> +static int hso_net_open(struct net_device *net);
> +static int hso_net_close(struct net_device *net);
> +static int hso_net_start_xmit(struct sk_buff *skb, struct net_device *net);
> +static int hso_net_ioctl(struct net_device *net, struct ifreq *rq, int cmd);
> +static struct net_device_stats *hso_net_get_stats(struct net_device *net);
> +static void hso_net_tx_timeout(struct net_device *net);
> +static void hso_net_set_multicast(struct net_device *net);
> +static void read_bulk_callback(struct urb *urb);
> +static void packetizeRx(struct hso_net *odev, unsigned char *ip_pkt,
> + unsigned int count, unsigned char is_eop);
> +static void write_bulk_callback(struct urb *urb);
> +/* Serial driver functions */
> +static int hso_serial_tiocmset(struct tty_struct *tty, struct file *file,
> + unsigned int set, unsigned int clear);
> +static void ctrl_callback(struct urb *urb);
> +static void put_rxbuf_data(struct urb *urb, struct hso_serial *serial);
> +static void hso_std_serial_read_bulk_callback(struct urb *urb);
> +static void hso_std_serial_write_bulk_callback(struct urb *urb);
> +static void _hso_serial_set_termios(struct tty_struct *tty,
> + struct ktermios *old);
> +static void hso_kick_transmit(struct hso_serial *serial);
> +/* Base driver functions */
> +static int hso_probe(struct usb_interface *interface,
> + const struct usb_device_id *id);
> +static void hso_disconnect(struct usb_interface *interface);
> +/* Helper functions */
> +static int hso_mux_submit_intr_urb(struct hso_shared_int *mux_int,
> + struct usb_device *usb, gfp_t gfp);
> +static void hso_net_init(struct net_device *net);
> +static void set_ethernet_addr(struct hso_net *odev);
> +static struct hso_serial *get_serial_by_index(unsigned index);
> +static struct hso_serial *get_serial_by_shared_int_and_type(
> + struct hso_shared_int *shared_int,
> + int mux);
> +static int get_free_serial_index(void);
> +static void set_serial_by_index(unsigned index, struct hso_serial *serial);
> +static int remove_net_device(struct hso_device *hso_dev);
> +static int add_net_device(struct hso_device *hso_dev);
> +static void log_usb_status(int status, const char *function);
> +static struct usb_endpoint_descriptor *hso_get_ep(struct usb_interface *intf,
> + int type, int dir);
> +static int hso_get_mux_ports(struct usb_interface *intf, unsigned char *ports);
> +static void hso_free_interface(struct usb_interface *intf);
> +static int hso_start_serial_device(struct hso_device *hso_dev);
> +static int hso_stop_serial_device(struct hso_device *hso_dev);
> +static int hso_start_net_device(struct hso_device *hso_dev);
> +static void hso_free_shared_int(struct hso_shared_int *shared_int);
> +static int hso_stop_net_device(struct hso_device *hso_dev);
> +static void hso_serial_ref_free(struct kref *ref);
> +static int hso_suspend(struct usb_interface *iface, pm_message_t message);
> +static int hso_resume(struct usb_interface *iface);
> +static void async_get_intf(struct work_struct *data);
> +static void async_put_intf(struct work_struct *data);
> +static int hso_put_activity(struct hso_device *hso_dev);
> +static int hso_get_activity(struct hso_device *hso_dev);
> +
>
This is a real forward declaration hell. Please try to reorder the
functions in the code to make this as obsolte as possible.
> +/*****************************************************************************/
> +/* Helping functions */
> +/*****************************************************************************/
> +
> +/* convert a character representing a hex value to a number */
> +static unsigned char hex2dec(unsigned char digit)
> +{
> +
> + if ((digit >= '0') && (digit <= '9'))
> + return (digit - '0');
> + /* Map all characters to 0-15 */
> + if ((digit >= 'a') && (digit <= 'z'))
> + return (digit - 'a' + 10) % 16;
> + if ((digit >= 'A') && (digit <= 'Z'))
> + return (digit - 'A' + 10) % 16;
> + return 16;
> +}
> +
>
Shouldn't this already be somewhere else in the kernel?
> +
> +/* module parameters */
> +static int debug;
> +static int procfs = 1;
> +static int tty_major;
> +static int disable_net;
> +static int enable_autopm;
> +
>
please move the module_param() stuff and MODULE_PARM_DESC() from the end
of the file right to this place.
> +static int hso_proc_port_info(char *buf, char **start, off_t offset, int count,
> + int *eof, void *data)
> +{
> + int len = 0;
> + struct hso_device *hso_dev = (struct hso_device *)data;
> + char *port_name = NULL;
> +
> + D1("count: %d", count);
>
This debug macro looks like it is from the very beginning of the
implementation. Remove it.
> +
> +static void hso_serial_throttle(struct tty_struct *tty)
> +{
> + D1(" ");
> +}
> +
> +static void hso_serial_unthrottle(struct tty_struct *tty)
> +{
> + D1(" ");
> +}
> +
> +static void hso_serial_break(struct tty_struct *tty, int break_state)
> +{
> + D1(" ");
> +}
> +
> +static int hso_serial_read_proc(char *page, char **start, off_t off, int count,
> + int *eof, void *data)
> +{
> + return 0;
> +}
> +
>
???
> +static void hso_serial_hangup(struct tty_struct *tty)
> +{
> + D1("hang up");
> +}
> +
>
>
Should all these functions be removed (and therefor not set in
hso_serial_ops) ??
> +
> +/* Base driver functions */
> +
> +/* operations setup of the serial interface */
> +static struct tty_operations hso_serial_ops = {
> + .open = hso_serial_open,
> + .close = hso_serial_close,
> + .write = hso_serial_write,
> + .write_room = hso_serial_write_room,
> + .ioctl = hso_serial_ioctl,
> + .set_termios = hso_serial_set_termios,
> + .throttle = hso_serial_throttle,
> + .unthrottle = hso_serial_unthrottle,
> + .break_ctl = hso_serial_break,
> + .chars_in_buffer = hso_serial_chars_in_buffer,
> + .read_proc = hso_serial_read_proc,
> + .hangup = hso_serial_hangup,
> + .tiocmget = hso_serial_tiocmget,
> + .tiocmset = hso_serial_tiocmset,
> +};
> +
>
--
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