Skip to contentSkip to conversation

fixup newly added jsm driver

PATCH In review Christoph Hellwig 3 participants 5 messages Mar 29, 2005 – Mar 30, 2005

lore
Share & tools
lore
#0 lore raw

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);
 }
#1 lore raw
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.

#2 lore raw
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

#3 lore raw
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.

#4 lore raw
In reply to Andrew Morton
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.