driver model and sysfs support for PCMCIA (1/3)

Hi,

This patch updates the pcmcia subsystem to utilize the driver model and sysfs. It cooperates peacefully with pcmcia-cs and does not break any drivers or require any API changes. It has been tested and is stable on my boxes.

The following features have been added to the pcmcia subsystem:
1.) "struct device" is created and removed
2.) pcmcia drivers are bound to the driver model device 3.) sysfs entries are created for identification information 4.) hotplug events are generated
5.) class devices are linked to physical pcmcia devices 6.) class devices are linked to pcmcia drivers

These changes are necessary for pcmcia to function properly on modern hotplug/sysfs systems. I would appreciate any comments or suggestions.

Thanks,
Adam

Applies against 2.6.7:

diff -ur linux/drivers/pcmcia/cs.c linux-pcmcia/drivers/pcmcia/cs.c
@@ -1155,6 +1155,22 @@
 
 #endif
 
+/*=====================================================================
+
+    Return the driver model device associated with a card..
+
+======================================================================*/
+
+struct device *pcmcia_lookup_device(client_handle_t handle)
+{
+	if (CHECK_HANDLE(handle))
+		return NULL;
+
+	return handle->device;
+}
+
+EXPORT_SYMBOL(pcmcia_lookup_device);
+
 /*======================================================================
 
     Get the current socket state bits.  We don't support the latched

@@ -2141,6 +2157,7 @@
 EXPORT_SYMBOL(pcmcia_suspend_card);
 EXPORT_SYMBOL(pcmcia_validate_cis);
 EXPORT_SYMBOL(pcmcia_write_memory);
+EXPORT_SYMBOL(read_tuple);
 
 EXPORT_SYMBOL(dead_socket);
 EXPORT_SYMBOL(MTDHelperEntry);

diff -ur linux/drivers/pcmcia/cs_internal.h linux-pcmcia/drivers/pcmcia/cs_internal.h
@@ -33,6 +33,7 @@
 typedef struct client_t {
     u_short		client_magic;
     struct pcmcia_socket *Socket;
+    struct device      *device;
     u_char		Function;
     dev_info_t		dev_info;
     u_int		Attributes;

diff -ur linux/include/pcmcia/cs.h linux-pcmcia/include/pcmcia/cs.h
@@ -318,6 +318,7 @@
 /* Special stuff for binding drivers to sockets */
 typedef struct bind_req_t {
     struct pcmcia_socket	*Socket;
+    struct pcmcia_device	*device;
     u_char	Function;
     dev_info_t	*dev_info;
 } bind_req_t;

@@ -452,6 +453,7 @@
 int pcmcia_set_event_mask(client_handle_t handle, eventmask_t *mask);
 int pcmcia_report_error(client_handle_t handle, error_info_t *err);
 struct pci_bus *pcmcia_lookup_bus(client_handle_t handle);
+struct device *pcmcia_lookup_device(client_handle_t handle);
 
 /* rsrc_mgr.c */
 int pcmcia_adjust_resource_info(client_handle_t handle, adjust_t *adj);

diff -ur linux/include/pcmcia/ds.h linux-pcmcia/include/pcmcia/ds.h
@@ -151,6 +151,21 @@
 	struct device_driver	drv;
 };
 
+struct pcmcia_device {
+	struct device dev;
+	struct pcmcia_bus_socket *socket;
+	struct list_head device_list;
+	int function;
+	int id_mask;
+	cistpl_vers_1_t vers_1;
+	cistpl_manfid_t manfid;
+};
+
+#define	to_pcmcia_device(n) container_of(n, struct pcmcia_device, dev)
+
+#define DEVICE_HAS_VERSION_INFO		0x0001
+#define DEVICE_HAS_MANF_INFO		0x0002
+
 /* driver registration */
 int pcmcia_register_driver(struct pcmcia_driver *driver);
 void pcmcia_unregister_driver(struct pcmcia_driver *driver);

Works nicely here with SMC 2632W-v2 wireless card (atmel-cs) and

02:0f.0 CardBus bridge: Texas Instruments PCI4451 PC card Cardbus Controller

Dmitry

Thank you, I'll add this to my collection of PCMCIA device model patches. I have quite a selection. 8)

I still have rather a number of patches for the pcmcia resource and socket driver side of things, and I'm hesitant to put anything other than bug fixes in before these are all in. This is something which will only proceed slowly - hopefully it should be complete by 2.6.9. This is especially slow because the only kernels which appear to be well tested for PCMCIA are Linus' main releases and I don't want to push too many changes in at one time.

Also, there remain races in the PCMCIA network drivers in their unload code - they free everything and _then_ unregister from the network layer. This should be fixed before we even think of adding device model support... I thought I'd already sent this patch, but it seems it never made it into the kernel.

Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

Hi Adam,

First of all I'd like to say that I'm glad you're taking interest in the PCMCIA core, and that your experience and patch-writing skills will surely be a valuable addition in the joint effort of transforming the PCMCIA subsystem to "kernel standards", e.g. hotplug, driver model and sysfs.

Second, you might not know about the linux-pcmcia list
http://lists.infradead.org/mailman/listinfo/linux-pcmcia
yet -- it's where most PCMCIA-core-in-2.{6,7} stuff is discussed, and it is quite low-traffic. Please CC this list on PCMCIA-core-in-2.{6,7} matters in future.

Third, about your patches:
- I like many ideas in your patches -- large parts of them, though, are "double work" as similar things have already been submitted (by me) to Russell on the linux-pcmcia mailing list. What's missing in my current patches [proof-of-concepts do exist and had been announced both on lkml and on said linux-pcmcia list, though] is the exporting of product and manufactor ID and "vers_1" strings, because that needs better resource handling.
- the resource_ready handling is "racy", at least. Resources can disappear again.
- what I don't like in your patches is that they add an aditional "layer" and thus additional complexity on top of PCMCIA. It already has a multitude of structs with different lifetime rules. Your additions don't make it easier to simplify this complexity. That's why my patchsets [*] try to reduce the complexity first, add struct pcmcia_device next, and reduce complexity by merging other stuff into struct pcmcia_device in the third step. I'd need to re-check whether the step (1) you're leaving out does _not_ cause lifetime headaches and races in strange circumstances [and I don't mean PCMCIA net drivers here, as they're in a comparably good shape.]

Dominik

[*] these patchsets have been announced in these messages:
1) http://lists.infradead.org/pipermail/linux-pcmcia/2004-May/000877.html
2) http://lists.infradead.org/pipermail/linux-pcmcia/2004-May/000878.html
3) http://lists.infradead.org/pipermail/linux-pcmcia/2004-May/000880.html
4) http://lists.infradead.org/pipermail/linux-pcmcia/2004-May/000881.html
5) http://lists.infradead.org/pipermail/linux-pcmcia/2004-May/000882.html

Sure, no problem.

Could you provide an example of how resources will disappear again? My patch was designed so that even if they weren't available, nothing bad would happen. Furthermore, it rescans for devices when userspace attempts to bind a driver. Resources would almost certainly be available during that time, and if they weren't, the entire pcmcia stack would be broken, not just sysfs. In other words, it seems that resource handling is a problem for all of pcmcia.

My patch adds new code to ds.c but I don't necessarily see it as a new layer. Because it has very little interaction with pcmcia and its drivers, I think it reduces the risk of introducing new bugs. My goal was not to create a perfect infrastructure for pcmcia and the driver model. It was to add minimal support for a much needed feature while introducing as few potential bugs as possible to a stable kernel series. I see 2.7 as the time for rewrites. With that in mind, I consider your patches to be a great solution, but I'm worried about changing internal ds functionality during 2.6.

It was not my intention to create "double work". I was aware of your effort in 2.5 but was unable to find any recent patches. (brodo.de was actually down when I was looking into the issue) I would really like to work out a common solution, perhaps with aspects from both of our efforts. I think that it is important, however, that we focus on getting working driver model support into the kernel quickly. Once that has been achieved, further cleanups could follow. I appreciate your comments and look forward to any additional suggestions or alternate viewpoints.

Thanks,
Adam

Hi Adam,

/etc/pcmcia/config.opts may include

include memory 0xc0000-0xfffff
exclude memory 0xc0000-0xfffff

even though it wouldn't make sense.

Indeed, they _are_ available during that time, else userspace wouldn't know what driver to bind. That's why my approach registers the pcmcia "sysfs" device struct at that place.

It is, but partly because used ioports and iomem are not 100% accounted in /proc/ioports and /proc/iomem. I'm eagerly awaiting the creation of a PNP- and/or ACPI-based resource core "backend", like you proposed at Kernel Summit last year, IIRC, which possibly allows the PCMCIA core on x86{,_64} to "trust" the resources not in the resource database to be available for PCMCIA's use.

However, adding pcmcia devices at the place you suggest causes resource headaches and makes merging my patches in 2.7. much more difficult. So, could we work to a compromise patch where PCMCIA sysfs device structs are only registered at "bind" time [as long as Russell agrees, that is...]?

Also, what do we need the "hotplug" export for? I'd like to avoid backwards compatibility trouble in future, and as users _need_ to run cardmgr hotplug seems to be without usage now.

Dominik

Hmm, ok.

I appreciate the interest. It's currently under development.

I agree that the current resources_ready flag could create potential problems. I've created another patch against the previous three that removes its usage, and relies entirely on DS_BIND_REQUEST. Devices are now removed but never added during hardware events. As for "hotplug", I was just trying to create a complete driver model implementation. I don't expect it to be used much now, but it is an official driver model feature.

Thanks,
Adam

@@ -108,9 +108,6 @@
     struct pcmcia_bus_socket *socket;
 } user_info_t;
 
-static LIST_HEAD(pcmcia_sockets);
-static DECLARE_MUTEX(pcmcia_socket_mutex);
-
 /* Socket state information */
 struct pcmcia_bus_socket {
 	atomic_t		refcount;

@@ -124,7 +121,6 @@
 	struct pcmcia_socket	*parent;
 	struct list_head	devices;
 	struct semaphore	device_mutex;
-	struct list_head	socket_list;
 };
 
 #define DS_SOCKET_PRESENT		0x01

@@ -141,8 +137,6 @@
 
 extern struct proc_dir_entry *proc_pccard;
 
-static int resources_ready;
-
 /*====================================================================*/
 
 /* code which was in cs.c before */

@@ -521,9 +515,6 @@
 	if (!(s->state & DS_SOCKET_PRESENT))
 		return CS_NO_CARD;
 
-	if (!resources_ready && !(s->parent->features & SS_CAP_STATIC_MAP))
-		return CS_NO_CARD;
-
 	down(&s->device_mutex);
 	if (!list_empty(&s->devices)) {
 		ret = -EBUSY;

@@ -639,18 +630,6 @@
 
 #endif /* CONFIG_HOTPLUG */
 
-static void pcmcia_rescan_sockets(void)
-{
-	struct pcmcia_bus_socket *s;
-
-	down(&pcmcia_socket_mutex);
-
-	list_for_each_entry(s, &pcmcia_sockets, socket_list)
-		pcmcia_bus_insert_card(s);
-
-	up(&pcmcia_socket_mutex);
-}
-
 /*======================================================================
 
     These manage a ring buffer of events pending for one user process

@@ -733,7 +712,6 @@
 	
     case CS_EVENT_CARD_INSERTION:
 	s->state |= DS_SOCKET_PRESENT;
-	pcmcia_bus_insert_card(s);
 	handle_event(s, event);
 	break;
 

@@ -1182,12 +1160,6 @@
     switch (cmd) {
     case DS_ADJUST_RESOURCE_INFO:
 	ret = pcmcia_adjust_resource_info(s->handle, &buf.adjust);
-	/*
-	 * We can't read CIS information until user space has given us the
-	 * memory resource locations.  Therefore, we wait until now.
-	 */
-	if ((ret == CS_SUCCESS) && (buf.adjust.Resource == RES_MEMORY_RANGE))
-		resources_ready = 1;
 	break;
     case DS_GET_CARD_SERVICES_INFO:
 	ret = pcmcia_get_card_services_info(&buf.servinfo);

@@ -1258,7 +1230,7 @@
 	break;
     case DS_BIND_REQUEST:
 	if (!capable(CAP_SYS_ADMIN)) return -EPERM;
-	pcmcia_rescan_sockets();
+	pcmcia_bus_insert_card(s);
 	err = bind_request(s, &buf.bind_info);
 	break;
     case DS_GET_DEVICE_INFO:

@@ -1330,10 +1302,6 @@
 	memset(s, 0, sizeof(struct pcmcia_bus_socket));
 	atomic_set(&s->refcount, 1);
 
-	down(&pcmcia_socket_mutex);
-	list_add_tail(&s->socket_list, &pcmcia_sockets);
-	up(&pcmcia_socket_mutex);
-
 	/*
 	 * Ugly. But we want to wait for the socket threads to have started up.
 	 * We really should let the drivers themselves drive some of this..

@@ -1395,10 +1363,7 @@
 
 	pcmcia_deregister_client(socket->pcmcia->handle);
 
-	down(&pcmcia_socket_mutex);
 	pcmcia_bus_remove_card(socket->pcmcia);
-	list_del(&socket->pcmcia->socket_list);
-	up(&pcmcia_socket_mutex);
 
 	socket->pcmcia->state |= DS_SOCKET_DEAD;
 	pcmcia_put_bus_socket(socket->pcmcia);

The problem are the "values" you export by the hotplug call: if your patch is merged "as is", then the values you make available to userspace will need to be supported for a long time, even if this means it'll cause difficulties once pcmcia becomes fully integrated into the driver model.

Dominik