[PATCH] Hotplug support for several PSX controlers

[PATCH] Hotplug support for several PSX controlers

[Eric Piel]

[-- Attachment #1: Type: text/plain, Size: 1039 bytes --]

Hello,

Since 2.6.9, several PSX controlers in the same time can be supported. 
However, because of a bug, if not all the PSX controlers are pluged in 
then nothing works. Typically, you load gamecon with options saying that 
you have two PSX adapter ports and then you plug and unplug has many 
controllers has you want. There is a bug which prevent keypress to be 
detected when not all the controllers connected.

The problem was that when a port didn't have a controler pluged the 
packet length to receive was read as very big, leading to a kind of 
buffer overflow. This patch checks the packet length and if it is bigger 
than the theoritical possible it considers that there is no controller 
pluged on this port.

It probably works on a vanilla 2.6.10-rc3 but I highly recommand to use 
the Vojtech's tree which contains an important fix about PSX DDR (cf 
http://marc.theaimsgroup.com/?l=linux-kernel&m=110118014804716&w=2).

I've heard that Linus wants 2.6.10 ready for Christmas, this patch 
should definitetly helps ;-)

Eric

[-- Attachment #2: gamecon-psx-hotplug-clamp-length.patch --]
[-- Type: text/x-patch, Size: 1549 bytes --]

--- linux-2.6.10.orig/drivers/input/joystick/gamecon.c	2004-10-20 21:25:06.000000000 +0200
+++ linux-2.6.10-rc3/drivers/input/joystick/gamecon.c	2004-12-12 14:58:35.000000000 +0100
@@ -241,7 +241,7 @@ static void gc_multi_read_packet(struct 
 #define GC_PSX_SELECT	0x02		/* Pin 3 */
 
 #define GC_PSX_ID(x)	((x) >> 4)	/* High nibble is device type */
-#define GC_PSX_LEN(x)	((x) & 0xf)	/* Low nibble is length in words */
+#define GC_PSX_LEN(x)	(((x) & 0xf) << 1)	/* Low nibble is length in words */
 
 static int gc_psx_delay = GC_PSX_DELAY;
 module_param_named(psx_delay, gc_psx_delay, uint, 0);
@@ -259,7 +259,7 @@ static short gc_psx_ddr_btn[] = { BTN_0,
  * the psx pad.
  */
 
-static void gc_psx_command(struct gc *gc, int b, unsigned char data[GC_PSX_LENGTH])
+static void gc_psx_command(struct gc *gc, int b, unsigned char data[5])
 {
 	int i, j, cmd, read;
 	for (i = 0; i < 5; i++)
@@ -302,10 +302,12 @@ static void gc_psx_read_packet(struct gc
 
 	for (i =0; i < 5; i++)								/* Find the longest pad */
 		if((gc_status_bit[i] & (gc->pads[GC_PSX] | gc->pads[GC_DDR])) &&
-			       	(GC_PSX_LEN(id[i]) > max_len))
+			       	(GC_PSX_LEN(id[i]) > max_len) &&
+				/* Too long length is a sign that no joystick is present */
+			       	(GC_PSX_LEN(id[i]) <= GC_PSX_LENGTH))			
 			max_len = GC_PSX_LEN(id[i]);
 
-	for (i = 0; i < max_len * 2; i++) {						/* Read in all the data */
+	for (i = 0; i < max_len; i++) {						/* Read in all the data */
 		gc_psx_command(gc, 0, data2);
 		for (j = 0; j < 5; j++)
 			data[j][i] = data2[j];

Re: [PATCH] Hotplug support for several PSX controlers

[Peter Nelson]

Eric Piel wrote:

> Since 2.6.9, several PSX controlers in the same time can be supported. 
> However, because of a bug, if not all the PSX controlers are pluged in 
> then nothing works. Typically, you load gamecon with options saying 
> that you have two PSX adapter ports and then you plug and unplug has 
> many controllers has you want. There is a bug which prevent keypress 
> to be detected when not all the controllers connected.

As I added to the documentation "hot swapping should work (but is not 
recomended)."  This might make it a bit more likely to work, but still 
"not recomended."

> The problem was that when a port didn't have a controler pluged the 
> packet length to receive was read as very big, leading to a kind of 
> buffer overflow. This patch checks the packet length and if it is 
> bigger than the theoritical possible it considers that there is no 
> controller pluged on this port.

This seems like a reasonable explination when ports float if 
unconnected.  Your patch does almost the right thing.  First 
gc_psx_command should take a data[5] argument, that was a logic error on 
my part.  Second, you compare the calculated length to PSX_LENGTH, which 
is just saying we read in bytes.  It should check <= 6, which is the 
longest string of packets possible (buttons, buttons, right, right, 
left, left, see 
<http://www.gamesx.com/controldata/psxcont/psxcont.htm>).  Changing to 
compare to 6 makes the patch look good to me.

> It probably works on a vanilla 2.6.10-rc3 but I highly recommand to 
> use the Vojtech's tree which contains an important fix about PSX DDR 
> (cf http://marc.theaimsgroup.com/?l=linux-kernel&m=110118014804716&w=2).

Vojtech already accepted my almost-identical patch when I noticed this 
in September.  See
http://marc.theaimsgroup.com/?l=linux-kernel&m=109571247127456&w=4

> I've heard that Linus wants 2.6.10 ready for Christmas, this patch 
> should definitetly helps ;-)

I'm all for both my previous patch and this one making it into 2.6.10 =)

-Peter

Re: [PATCH] Hotplug support for several PSX controlers

[Eric Piel]

Peter Nelson wrote:
 > As I added to the documentation "hot swapping should work (but is not
 > recomended)."  This might make it a bit more likely to work, but still
 > "not recomended."
Yes, I had read it somewhere, I've tried and it works :-)


 > This seems like a reasonable explination when ports float if
 > unconnected.  Your patch does almost the right thing.  First
 > gc_psx_command should take a data[5] argument, that was a logic error on
 > my part.  Second, you compare the calculated length to PSX_LENGTH, which
 > is just saying we read in bytes.  It should check <= 6, which is the
 > longest string of packets possible (buttons, buttons, right, right,
 > left, left, see
 > <http://www.gamesx.com/controldata/psxcont/psxcont.htm>).  Changing to
 > compare to 6 makes the patch look good to me.
I didn't know about the official spec. So changing PSX_LENGTH to 6 makes 
everything even more correct, good.


 >> It probably works on a vanilla 2.6.10-rc3 but I highly recommand to
 >> use the Vojtech's tree which contains an important fix about PSX DDR
 >> (cf http://marc.theaimsgroup.com/?l=linux-kernel&m=110118014804716&w=2).
 >
 >
 > Vojtech already accepted my almost-identical patch when I noticed this
 > in September.  See
 > http://marc.theaimsgroup.com/?l=linux-kernel&m=109571247127456&w=4
Sorry not mentioning your patch, it is actually the one I use. When 
searching for the reference I mistook with the post of Decklin, sorry.


 >> I've heard that Linus wants 2.6.10 ready for Christmas, this patch
 >> should definitetly helps ;-)
 > I'm all for both my previous patch and this one making it into 2.6.10 =)
Is there anyone in particular to tell about those patches? Or is the 
normal way that Vojtech inserts the patches in his tree and Linus pulls 
it when he feels it's stable enough?

Eric

Re: [PATCH] Hotplug support for several PSX controlers

[Peter Nelson]

Eric Piel wrote:

> > This seems like a reasonable explination when ports float if
> > unconnected.  Your patch does almost the right thing.  First
> > gc_psx_command should take a data[5] argument, that was a logic 
> error on
> > my part.  Second, you compare the calculated length to PSX_LENGTH, 
> which
> > is just saying we read in bytes.  It should check <= 6, which is the
> > longest string of packets possible (buttons, buttons, right, right,
> > left, left, see
> > <http://www.gamesx.com/controldata/psxcont/psxcont.htm>).  Changing to
> > compare to 6 makes the patch look good to me.
> I didn't know about the official spec. So changing PSX_LENGTH to 6 
> makes everything even more correct, good.

Sorry I confused you, but the source didn't use the variables as it 
should.  GC_PSX_LENGTH should be 8 meaning we read down 8 bits (1 byte) 
at a time.  I'll create new variable called GC_PSX_BYTES set to 6 
meaning we can pull down up to 6 bytes in a row for a single read.

> >> I've heard that Linus wants 2.6.10 ready for Christmas, this patch
> >> should definitetly helps ;-)
> > I'm all for both my previous patch and this one making it into 
> 2.6.10 =)
> Is there anyone in particular to tell about those patches? Or is the 
> normal way that Vojtech inserts the patches in his tree and Linus 
> pulls it when he feels it's stable enough?

 From what I see Vojtech accepts them and then Linus eventually pulls 
from Vojtech's tree.  Here is my modified patch.  Please test it to 
double check it works.

-Peter

---
From: Eric Piel <Eric.Piel@tremplin-utc.net>

Fixes hotplug support for PSX controllers and some mis-sized arrays.

Signed-off-by: Peter Nelson <rufus-kernel@hackish.org>
---
===== gamecon.c 1.19 vs edited =====
--- 1.19/drivers/input/joystick/gamecon.c	2004-12-13 20:04:39 -05:00
+++ edited/gamecon.c	2004-12-13 20:18:02 -05:00
@@ -228,6 +228,7 @@
 
 #define GC_PSX_DELAY	25		/* 25 usec */
 #define GC_PSX_LENGTH	8		/* talk to the controller in bytes */
+#define GC_PSX_BYTES	6		/* the maximum number of bytes to read off the controller */
 
 #define GC_PSX_MOUSE	1		/* Mouse */
 #define GC_PSX_NEGCON	2		/* NegCon */
@@ -241,7 +242,7 @@
 #define GC_PSX_SELECT	0x02		/* Pin 3 */
 
 #define GC_PSX_ID(x)	((x) >> 4)	/* High nibble is device type */
-#define GC_PSX_LEN(x)	((x) & 0xf)	/* Low nibble is length in words */
+#define GC_PSX_LEN(x)	(((x) & 0xf) << 1)	/* Low nibble is length in bytes/2 */
 
 static int gc_psx_delay = GC_PSX_DELAY;
 module_param_named(psx_delay, gc_psx_delay, uint, 0);
@@ -259,13 +260,13 @@
  * the psx pad.
  */
 
-static void gc_psx_command(struct gc *gc, int b, unsigned char data[GC_PSX_LENGTH])
+static void gc_psx_command(struct gc *gc, int b, unsigned char data[5])
 {
 	int i, j, cmd, read;
 	for (i = 0; i < 5; i++)
 		data[i] = 0;
 
-	for (i = 0; i < 8; i++, b >>= 1) {
+	for (i = 0; i < GC_PSX_LENGTH; i++, b >>= 1) {
 		cmd = (b & 1) ? GC_PSX_COMMAND : 0;
 		parport_write_data(gc->pd->port, cmd | GC_PSX_POWER);
 		udelay(gc_psx_delay);
@@ -283,7 +284,7 @@
  * device identifier code.
  */
 
-static void gc_psx_read_packet(struct gc *gc, unsigned char data[5][GC_PSX_LENGTH], unsigned char id[5])
+static void gc_psx_read_packet(struct gc *gc, unsigned char data[5][GC_PSX_BYTES], unsigned char id[5])
 {
 	int i, j, max_len = 0;
 	unsigned long flags;
@@ -302,10 +303,11 @@
 
 	for (i =0; i < 5; i++)								/* Find the longest pad */
 		if((gc_status_bit[i] & (gc->pads[GC_PSX] | gc->pads[GC_DDR]))
-		   && (GC_PSX_LEN(id[i]) > max_len))
+		   && (GC_PSX_LEN(id[i]) > max_len)
+		   && (GC_PSX_LEN(id[i]) <= GC_PSX_BYTES))
 			max_len = GC_PSX_LEN(id[i]);
 
-	for (i = 0; i < max_len * 2; i++) {						/* Read in all the data */
+	for (i = 0; i < max_len; i++) {							/* Read in all the data */
 		gc_psx_command(gc, 0, data2);
 		for (j = 0; j < 5; j++)
 			data[j][i] = data2[j];
@@ -330,7 +332,7 @@
 	struct gc *gc = (void *) private;
 	struct input_dev *dev = gc->dev;
 	unsigned char data[GC_MAX_LENGTH];
-	unsigned char data_psx[5][GC_PSX_LENGTH];
+	unsigned char data_psx[5][GC_PSX_BYTES];
 	int i, j, s;
 
 /*

Re: [PATCH] Hotplug support for several PSX controlers

[Eric Piel]

Peter Nelson wrote:
> Eric Piel wrote:
:
:
>> Is there anyone in particular to tell about those patches? Or is the 
>> normal way that Vojtech inserts the patches in his tree and Linus 
>> pulls it when he feels it's stable enough?
> 
> 
>  From what I see Vojtech accepts them and then Linus eventually pulls 
> from Vojtech's tree.  Here is my modified patch.  Please test it to 
> double check it works.
Ok, I've tried it completly yesterday evening and I confirm it works 
perfectly. There is a minor correction to do concerning the comment of 
GC_PSX_LENGTH: "bytes" need to be changed to "bits" .

Eric



> 
> ---
> From: Eric Piel <Eric.Piel@tremplin-utc.net>
> 
> Fixes hotplug support for PSX controllers and some mis-sized arrays.
> 
> Signed-off-by: Peter Nelson <rufus-kernel@hackish.org>
> ---
> ===== gamecon.c 1.19 vs edited =====
> --- 1.19/drivers/input/joystick/gamecon.c    2004-12-13 20:04:39 -05:00
> +++ edited/gamecon.c    2004-12-13 20:18:02 -05:00
> @@ -228,6 +228,7 @@
> 
> #define GC_PSX_DELAY    25        /* 25 usec */
> #define GC_PSX_LENGTH    8        /* talk to the controller in bytes */
> +#define GC_PSX_BYTES    6        /* the maximum number of bytes to read 
> off the controller */
> 
> #define GC_PSX_MOUSE    1        /* Mouse */
> #define GC_PSX_NEGCON    2        /* NegCon */
> @@ -241,7 +242,7 @@
> #define GC_PSX_SELECT    0x02        /* Pin 3 */
> 
> #define GC_PSX_ID(x)    ((x) >> 4)    /* High nibble is device type */
> -#define GC_PSX_LEN(x)    ((x) & 0xf)    /* Low nibble is length in 
> words */
> +#define GC_PSX_LEN(x)    (((x) & 0xf) << 1)    /* Low nibble is length 
> in bytes/2 */
> 
> static int gc_psx_delay = GC_PSX_DELAY;
> module_param_named(psx_delay, gc_psx_delay, uint, 0);
> @@ -259,13 +260,13 @@
>  * the psx pad.
>  */
> 
> -static void gc_psx_command(struct gc *gc, int b, unsigned char 
> data[GC_PSX_LENGTH])
> +static void gc_psx_command(struct gc *gc, int b, unsigned char data[5])
> {
>     int i, j, cmd, read;
>     for (i = 0; i < 5; i++)
>         data[i] = 0;
> 
> -    for (i = 0; i < 8; i++, b >>= 1) {
> +    for (i = 0; i < GC_PSX_LENGTH; i++, b >>= 1) {
>         cmd = (b & 1) ? GC_PSX_COMMAND : 0;
>         parport_write_data(gc->pd->port, cmd | GC_PSX_POWER);
>         udelay(gc_psx_delay);
> @@ -283,7 +284,7 @@
>  * device identifier code.
>  */
> 
> -static void gc_psx_read_packet(struct gc *gc, unsigned char 
> data[5][GC_PSX_LENGTH], unsigned char id[5])
> +static void gc_psx_read_packet(struct gc *gc, unsigned char 
> data[5][GC_PSX_BYTES], unsigned char id[5])
> {
>     int i, j, max_len = 0;
>     unsigned long flags;
> @@ -302,10 +303,11 @@
> 
>     for (i =0; i < 5; i++)                                /* Find the 
> longest pad */
>         if((gc_status_bit[i] & (gc->pads[GC_PSX] | gc->pads[GC_DDR]))
> -           && (GC_PSX_LEN(id[i]) > max_len))
> +           && (GC_PSX_LEN(id[i]) > max_len)
> +           && (GC_PSX_LEN(id[i]) <= GC_PSX_BYTES))
>             max_len = GC_PSX_LEN(id[i]);
> 
> -    for (i = 0; i < max_len * 2; i++) {                        /* Read 
> in all the data */
> +    for (i = 0; i < max_len; i++) {                            /* Read 
> in all the data */
>         gc_psx_command(gc, 0, data2);
>         for (j = 0; j < 5; j++)
>             data[j][i] = data2[j];
> @@ -330,7 +332,7 @@
>     struct gc *gc = (void *) private;
>     struct input_dev *dev = gc->dev;
>     unsigned char data[GC_MAX_LENGTH];
> -    unsigned char data_psx[5][GC_PSX_LENGTH];
> +    unsigned char data_psx[5][GC_PSX_BYTES];
>     int i, j, s;
> 
> /*
>