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 [email protected]
[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.
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;
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
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
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;
};