stuff-up in pcmcia/cardbus stuff

stuff-up in pcmcia/cardbus stuff

[Paul Mackerras]

Recent 2.5 kernels will crash with a null pointer dereference on my
powerbook (PowerPC laptop) when I try to suspend.  I tracked it down
to cardbus_suspend() in drivers/pcmcia/pci-socket.c calling
pcmcia_suspend_socket() with a NULL argument.  It turns out that
socket->pcmcia_socket is never set in the current code.

This changeset seems to be the culprit:

ChangeSet 1.1017 2003/02/15 12:42:51 linux@brodo.de
  [PATCH] pcmcia: use device_class->add_device/remove_device
  
  This patch removes the {un}register_ss_entry/pcmcia_{un}register_socket
  calls, and replaces them with generic driver-model-compatible functions.
  
  Also, update the CodingStyle of these cs.c functions to what's recommended
  in the Linux kernel.

Paul.

[PATCH] Re: stuff-up in pcmcia/cardbus stuff

[Dominik Brodowski]

On Tue, Feb 18, 2003 at 12:50:52PM +1100, Paul Mackerras wrote:
> Recent 2.5 kernels will crash with a null pointer dereference on my
> powerbook (PowerPC laptop) when I try to suspend.  I tracked it down
> to cardbus_suspend() in drivers/pcmcia/pci-socket.c calling
> pcmcia_suspend_socket() with a NULL argument.  It turns out that
> socket->pcmcia_socket is never set in the current code.

Indeed. socket->pcmcia_socket (old) == socket->cls_d.s_info[0] (new)
Could you please check whether this patch helps?

	Dominik

diff -ruN linux-original/drivers/pcmcia/pci_socket.c linux/drivers/pcmcia/pci_socket.c
--- linux-original/drivers/pcmcia/pci_socket.c	2003-02-18 09:08:00.000000000 +0100
+++ linux/drivers/pcmcia/pci_socket.c	2003-02-18 09:12:02.000000000 +0100
@@ -230,14 +230,16 @@
 static int cardbus_suspend (struct pci_dev *dev, u32 state)
 {
 	pci_socket_t *socket = pci_get_drvdata(dev);
-	pcmcia_suspend_socket (socket->pcmcia_socket);
+	if (socket && socket->cls_d.s_info[0])
+		pcmcia_suspend_socket (socket->cls_d.s_info[0]);
 	return 0;
 }
 
 static int cardbus_resume (struct pci_dev *dev)
 {
 	pci_socket_t *socket = pci_get_drvdata(dev);
-	pcmcia_resume_socket (socket->pcmcia_socket);
+	if (socket && socket->cls_d.s_info[0])
+		pcmcia_resume_socket (socket->cls_d.s_info[0]);
 	return 0;
 }
 
diff -ruN linux-original/drivers/pcmcia/pci_socket.h linux/drivers/pcmcia/pci_socket.h
--- linux-original/drivers/pcmcia/pci_socket.h	2003-02-18 09:08:00.000000000 +0100
+++ linux/drivers/pcmcia/pci_socket.h	2003-02-18 09:10:29.000000000 +0100
@@ -20,7 +20,6 @@
 	socket_cap_t cap;
 	spinlock_t event_lock;
 	unsigned int events;
-	struct socket_info_t *pcmcia_socket;
 	struct work_struct tq_task;
 	struct timer_list poll_timer;
 

Re: [PATCH] Re: stuff-up in pcmcia/cardbus stuff

[Jeff Garzik]

Dominik Brodowski wrote:
> Indeed. socket->pcmcia_socket (old) == socket->cls_d.s_info[0] (new)

If this is true...

> @@ -230,14 +230,16 @@
>  static int cardbus_suspend (struct pci_dev *dev, u32 state)
>  {
>  	pci_socket_t *socket = pci_get_drvdata(dev);
> -	pcmcia_suspend_socket (socket->pcmcia_socket);
> +	if (socket && socket->cls_d.s_info[0])
> +		pcmcia_suspend_socket (socket->cls_d.s_info[0]);
>  	return 0;
>  }
>  
>  static int cardbus_resume (struct pci_dev *dev)
>  {
>  	pci_socket_t *socket = pci_get_drvdata(dev);
> -	pcmcia_resume_socket (socket->pcmcia_socket);
> +	if (socket && socket->cls_d.s_info[0])
> +		pcmcia_resume_socket (socket->cls_d.s_info[0]);
>  	return 0;
>  }


1) ...why do you bother checking for NULL?  Isn't NULL indicative of a 
BUG(), instead?

2) why are multiple s_info records allocated, when you hardcode use of 
record #0 ?

	Jeff

Re: [PATCH] Re: stuff-up in pcmcia/cardbus stuff

[Dominik Brodowski]

On Tue, Feb 18, 2003 at 04:23:45AM -0500, Jeff Garzik wrote:
> Dominik Brodowski wrote:
> >Indeed. socket->pcmcia_socket (old) == socket->cls_d.s_info[0] (new)
> 
> If this is true...
> 
> >@@ -230,14 +230,16 @@
> > static int cardbus_suspend (struct pci_dev *dev, u32 state)
> > {
> > 	pci_socket_t *socket = pci_get_drvdata(dev);
> >-	pcmcia_suspend_socket (socket->pcmcia_socket);
> >+	if (socket && socket->cls_d.s_info[0])
> >+		pcmcia_suspend_socket (socket->cls_d.s_info[0]);
> > 	return 0;
> > }
> > 
> > static int cardbus_resume (struct pci_dev *dev)
> > {
> > 	pci_socket_t *socket = pci_get_drvdata(dev);
> >-	pcmcia_resume_socket (socket->pcmcia_socket);
> >+	if (socket && socket->cls_d.s_info[0])
> >+		pcmcia_resume_socket (socket->cls_d.s_info[0]);
> > 	return 0;
> > }
> 
> 
> 1) ...why do you bother checking for NULL?  Isn't NULL indicative of a 
> BUG(), instead?

Well, it's only a safeguard against suspending / resuming combined with 
probing or removing the device. Else it's a BUG indeed...

> 2) why are multiple s_info records allocated, when you hardcode use of 
> record #0 ?
Only one s_info is actually allocated (in cs.c::pcmcia_register_socket) as
only one pcmcia/cardbus socket is attached to one pci_dev for yenta-style
devices. There are up to four pcmcia sockets to one pci_dev for i82092
devices, though. And so s_info[3] might be perfectly valid within the
i82092 driver.

	Dominik

[PATCH, updated] Re: stuff-up in pcmcia/cardbus stuff

[Dominik Brodowski]

On Tue, Feb 18, 2003 at 04:23:45AM -0500, Jeff Garzik wrote:
> Dominik Brodowski wrote:
> >Indeed. socket->pcmcia_socket (old) == socket->cls_d.s_info[0] (new)
> 
> If this is true...
<snip>
> 
> 2) why are multiple s_info records allocated, when you hardcode use of 
> record #0 ?

Indeed, the allocation of MAX_SOCK_PER_DEV s_info[] pointers is pointless.

	Dominik

diff -ruN linux-original/drivers/pcmcia/cs.c linux-pcmcia/drivers/pcmcia/cs.c
--- linux-original/drivers/pcmcia/cs.c	2003-02-18 09:08:00.000000000 +0100
+++ linux-pcmcia/drivers/pcmcia/cs.c	2003-02-18 23:10:56.000000000 +0100
@@ -330,11 +330,12 @@
 		return -ENOMEM;
 	memset(s_info, 0, cls_d->nsock * sizeof(socket_info_t));
 
+	cls_d->s_info = s_info;
+
 	/* socket initialization */
 	for (i = 0; i < cls_d->nsock; i++) {
 		socket_info_t *s = &s_info[i];
 
-		cls_d->s_info[i] = s;
 		s->ss_entry = cls_d->ops;
 		s->sock = i;
 
diff -ruN linux-original/drivers/pcmcia/pci_socket.c linux-pcmcia/drivers/pcmcia/pci_socket.c
--- linux-original/drivers/pcmcia/pci_socket.c	2003-02-18 22:54:34.000000000 +0100
+++ linux-pcmcia/drivers/pcmcia/pci_socket.c	2003-02-18 23:10:10.000000000 +0100
@@ -230,14 +230,16 @@
 static int cardbus_suspend (struct pci_dev *dev, u32 state)
 {
 	pci_socket_t *socket = pci_get_drvdata(dev);
-	pcmcia_suspend_socket (socket->pcmcia_socket);
+	if (socket && socket->cls_d.s_info)
+		pcmcia_suspend_socket (socket->cls_d.s_info);
 	return 0;
 }
 
 static int cardbus_resume (struct pci_dev *dev)
 {
 	pci_socket_t *socket = pci_get_drvdata(dev);
-	pcmcia_resume_socket (socket->pcmcia_socket);
+	if (socket && socket->cls_d.s_info)
+		pcmcia_resume_socket (socket->cls_d.s_info);
 	return 0;
 }
 
diff -ruN linux-original/drivers/pcmcia/pci_socket.h linux-pcmcia/drivers/pcmcia/pci_socket.h
--- linux-original/drivers/pcmcia/pci_socket.h	2003-02-18 22:54:34.000000000 +0100
+++ linux-pcmcia/drivers/pcmcia/pci_socket.h	2003-02-18 23:10:42.000000000 +0100
@@ -20,7 +20,6 @@
 	socket_cap_t cap;
 	spinlock_t event_lock;
 	unsigned int events;
-	struct socket_info_t *pcmcia_socket;
 	struct work_struct tq_task;
 	struct timer_list poll_timer;
 
diff -ruN linux-original/include/pcmcia/ss.h linux-pcmcia/include/pcmcia/ss.h
--- linux-original/include/pcmcia/ss.h	2003-02-18 09:08:02.000000000 +0100
+++ linux-pcmcia/include/pcmcia/ss.h	2003-02-18 23:09:55.000000000 +0100
@@ -144,12 +144,10 @@
  *  Calls to set up low-level "Socket Services" drivers
  */
 
-#define MAX_SOCKETS_PER_DEV 8
-
 struct pcmcia_socket_class_data {
 	unsigned int nsock;			/* number of sockets */
 	struct pccard_operations *ops;		/* see above */
-	void *s_info[MAX_SOCKETS_PER_DEV];	/* socket_info_t */
+	void *s_info;				/* socket_info_t */
 	unsigned int use_bus_pm;
 };