fixup newly added jsm driver
PATCH 1/1 fixup newly added jsm driver
One more prematurely added drivers..
- plug various leaks and use after frees in the remove and initialization failure path (some still left)
- remove useless global list of boards and use pci_set_drvdata instead
- unobsfucate init path by merging functions together
- kill various totally useless state variables
- .. probably more I forgot
Note that the tty part still generates lots of sparse warnings and there's still a totally useless layer of function pointer indirections, but maybe someone else will fix that bit up.
===== drivers/serial/jsm/jsm.h 1.1 vs edited =====
drivers/serial/jsm/jsm.h +0 -38
@@ -93,28 +93,6 @@#define JSM_VERSION "jsm: 1.1-1-INKERNEL"#define JSM_PARTNUM "40002438_A-INKERNEL" -/* -* All the possible states the driver can be while being loaded. -*/ -enum { -DRIVER_INITIALIZED =0 , -DRIVER_READY -}; - -/* -* All the possible states the board can be while booting up. -*/ -enum { -BOARD_FAILED =0 , -BOARD_FOUND , -BOARD_READY -}; - -struct board_id { -u8 *name; -u32 maxports; -}; -struct jsm_board ;struct jsm_channel ;
@@ -149,7 +127,6 @@int boardnum;/* Board number: 0-32 */ int type;/* Type of board */ -char *name;/* Product Name */ u8 rev;/* PCI revision ID */ struct pci_dev *pci_dev;u32 maxports;/* MAX ports this board can handle */
@@ -160,9 +137,6 @@* the interrupt routine from each other. */ -u32 state;/* State of card. */ -wait_queue_head_t state_wait;/* Place to sleep on for state change */ -u32 nasync;/* Number of ports on card */ u32 irq;/* Interrupt request number */
@@ -181,9 +155,6 @@struct jsm_channel *channels[MAXPORTS ];/* array of pointers to our channels. */ char *flipbuf;/* Our flip buffer, alloced if board is found */ -u16 dpatype;/* The board "type", as defined by DPA */ -u16 dpastatus;/* The board "status", as defined by DPA */ -u32 bd_dividend;/* Board/UARTs specific dividend */ struct board_ops *bd_ops;
@@ -412,12 +384,6 @@extern int jsm_debug;extern int jsm_rawreadok; -extern int jsm_driver_state;/* The state of the driver */ -extern char *jsm_driver_state_text[];/* Array of driver state text */ - -extern spinlock_t jsm_board_head_lock; -extern struct list_head jsm_board_head; -/************************************************************************* * * Prototypes for non-static functions used in more than one module
@@ -430,8 +396,5 @@void jsm_input (struct jsm_channel *ch);void jsm_carrier (struct jsm_channel *ch);void jsm_check_queue_flow_control (struct jsm_channel *ch); - -void jsm_create_driver_sysfiles (struct device_driver *); -void jsm_remove_driver_sysfiles (struct device_driver *);#endif
===== drivers/serial/jsm/jsm_driver.c 1.1 vs edited =====
drivers/serial/jsm/jsm_driver.c +95 -253
458 lines not on this page · raw message
===== drivers/serial/jsm/jsm_tty.c 1.1 vs edited =====
drivers/serial/jsm/jsm_tty.c +0 -24
@@ -1016,28 +1016,4 @@ }return data_count; -} - -static ssize_t jsm_driver_version_show (struct device_driver *ddp,char *buf) -{ -return snprintf (buf,PAGE_SIZE ,"%s\n" ,JSM_VERSION ); -} -static DRIVER_ATTR(version, S_IRUSR, jsm_driver_version_show, NULL); - -static ssize_t jsm_driver_state_show (struct device_driver *ddp,char *buf) -{ -return snprintf (buf,PAGE_SIZE ,"%s\n" , jsm_driver_state_text[jsm_driver_state]); -} -static DRIVER_ATTR(state, S_IRUSR, jsm_driver_state_show, NULL); - -void jsm_create_driver_sysfiles (struct device_driver *driverfs) -{ -driver_create_file (driverfs, &driver_attr_version); -driver_create_file (driverfs, &driver_attr_state); -} - -void jsm_remove_driver_sysfiles (struct device_driver *driverfs) -{ -driver_remove_file (driverfs, &driver_attr_version); -driver_remove_file (driverfs, &driver_attr_state); }
Quoted 2 lines · Christoph Hellwig
One more prematurely added drivers..
This driver was first sent out for review a month ago, was upissued twice and generated over seventy linux-kernel emails including some from Russell and some from Greg. It was by no means a "premature" addition.
One could say that it was inadequately reviewed, but how is one to determine that? If the thing has been under discussion for a month and the submitter says "I've addressed all comments" then it's going to get merged.
Quoted 5 lines · Andrew Morton
Quoted 1 line · Christoph Hellwig
One more prematurely added drivers..
Quoted 7 lines
This driver was first sent out for review a month ago, was upissued twice and generated over seventy linux-kernel emails including some from Russell and some from Greg. It was by no means a "premature" addition.
One could say that it was inadequately reviewed, but how is one to determine that? If the thing has been under discussion for a month and the submitter says "I've addressed all comments" then it's going to get merged.
I'd characterize it slightly differently than both of you:
It got a decent review, but from Christoph's list it looks like not all the issues raised during the review got addressed.
FWIW.
Jeff
Quoted 2 lines · Jeff Garzik
It got a decent review, but from Christoph's list it looks like not all the issues raised during the review got addressed.
Exactly. Most reviews take more than one pass.
Quoted 11 lines · Andrew Morton
Quoted 2 lines · Christoph Hellwig
One more prematurely added drivers..
This driver was first sent out for review a month ago, was upissued twice and generated over seventy linux-kernel emails including some from Russell and some from Greg. It was by no means a "premature" addition.
One could say that it was inadequately reviewed, but how is one to determine that? If the thing has been under discussion for a month and the submitter says "I've addressed all comments" then it's going to get merged.
I don't think the submitter should say all issues have been addressed but that should come from a sufficiently trusted reviewer.