pcmcia: 64bit fixes, anonymous cards, other bugs

These patches update the PCMCIA layer to work on 64bit machines and also to support anonymous memory cards (a feature that was broken in the conversion from the old pcmcia_cs model to the pccard drivers).

In addition they fix various bugs found during the work, and provide an alternative rather less hideously convoluted resource manager that gets used when the only PCMCIA present is via PCI bridges. In that situation the core pci layer can do the job perfectly well for us and just needs a bit of wrapping.

Alan

---

Alan Cox (5):
pcmcia: correct types
pcmcia cis: on an out of range CIS read return 0xff, don't just warn
pcmcia: Fix requery
pcmcia: handle anonymous cards by generating a fake CIS
pcmcia: add a new resource manager for non ISA systems

 drivers/pcmcia/Kconfig       |   12 ++-
 drivers/pcmcia/Makefile      |    1 
 drivers/pcmcia/cistpl.c      |   31 ++++++--
 drivers/pcmcia/cs_internal.h |    6 +
 drivers/pcmcia/ds.c          |    3 +
 drivers/pcmcia/rsrc_mgr.c    |    5 +
 drivers/pcmcia/rsrc_pci.c    |  172 ++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 216 insertions(+), 14 deletions(-)
 create mode 100644 drivers/pcmcia/rsrc_pci.c

--
"Writing in comic sans is like talking earnestly whilst you have hiccups"

We should be using resource_size_t and unsigned types correctly, otherwise we sign extend the flags on a 64bit box, which is not what we want.

---
 drivers/pcmcia/cs_internal.h |    6 +++---
 drivers/pcmcia/rsrc_mgr.c    |    5 +++--
 2 files changed, 6 insertions(+), 5 deletions(-)

index 7f1953f..e86cd6b 100644
@@ -80,9 +80,9 @@ struct pccard_resource_ops {
  * Stuff internal to module "pcmcia_rsrc":
  */
 extern int static_init(struct pcmcia_socket *s);
-extern struct resource *pcmcia_make_resource(unsigned long start,
-					unsigned long end,
-					int flags, const char *name);
+extern struct resource *pcmcia_make_resource(resource_size_t start,
+					resource_size_t end,
+					unsigned long flags, const char *name);
 
 /*
  * Stuff internal to module "pcmcia_core":

index aa628ed..df2cb70 100644
@@ -30,8 +30,9 @@ int static_init(struct pcmcia_socket *s)
 	return 0;
 }
 
-struct resource *pcmcia_make_resource(unsigned long start, unsigned long end,
-				int flags, const char *name)
+struct resource *pcmcia_make_resource(resource_size_t start,
+					resource_size_t end,
+					unsigned long flags, const char *name)
 {
 	struct resource *res = kzalloc(sizeof(*res), GFP_KERNEL);
 

The current code displays warnings but then proceeds to try and reference the data through the PCMCIA window. Instead return 0xff. This prevents bogus CIS data sending us off into hyperspace.

---
 drivers/pcmcia/cistpl.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

index 884a984..4ff725c 100644
@@ -168,9 +168,12 @@ int pcmcia_read_cis_mem(struct pcmcia_socket *s, int attr, u_int addr,
 	} else {
 		u_int inc = 1, card_offset, flags;
 
-		if (addr > CISTPL_MAX_CIS_SIZE)
+		if (addr > CISTPL_MAX_CIS_SIZE) {
 			dev_dbg(&s->dev,
 				"attempt to read CIS mem at addr %#x", addr);
+			memset(ptr, 0xff, len);
+			return -1;
+		}
 
 		flags = MAP_ACTIVE | ((cis_width) ? MAP_16BIT : 0);
 		if (attr) {

The requery logic goes off and attempts to read the CIS of empty slots. In most cases this happens not to do any harm - but not all!

Add the missing check and also a WARN() to catch any other offenders.

---
 drivers/pcmcia/cistpl.c |    2 +-
 drivers/pcmcia/ds.c     |    3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

index 4ff725c..8b3b492 100644
@@ -1386,7 +1386,7 @@ int pccard_validate_cis(struct pcmcia_socket *s, unsigned int *info)
 	if (!s)
 		return -EINVAL;
 
-	if (s->functions) {
+	if (s->functions || !(s->state & SOCKET_PRESENT)) {
 		WARN_ON(1);
 		return -EINVAL;
 	}

index 757119b..d3baf0b 100644
@@ -667,6 +667,9 @@ static void pcmcia_requery(struct pcmcia_socket *s)
 {
 	int has_pfc;
 
+	if (!(s->state & SOCKET_PRESENT))
+		return;
+
 	if (s->functions == 0) {
 		pcmcia_card_add(s);
 		return;

The core pcmcia code blows up all over the place if it allowed a card without a valid CIS. We need to allow such cards as the CIS stuff is not on the older flash, ROM and SRAM cards. We give it a suitably blank fake CIS instead.

In order to minimise the risk of misidentifying junk and feeding it to the wrong thing we only fix up apparently anonymous cards if the driver for them has been enabled.

---
 drivers/pcmcia/cistpl.c |   24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

index 8b3b492..64d0515 100644
@@ -1451,10 +1451,26 @@ int pccard_validate_cis(struct pcmcia_socket *s, unsigned int *info)
 done:
 	/* invalidate CIS cache on failure */
 	if (!dev_ok || !ident_ok || !count) {
-		mutex_lock(&s->ops_mutex);
-		destroy_cis_cache(s);
-		mutex_unlock(&s->ops_mutex);
-		ret = -EIO;
+#if defined(CONFIG_MTD_PCMCIA_ANONYMOUS)
+		/* Set up as an anonymous card. If we don't have anonymous
+		   memory support then just error the card as there is no
+		   point trying to second guess.
+
+		   Note: some cards have just a device entry, it may be
+		   worth extending support to cover these in future */
+		if (!dev_ok || !ident_ok) {
+			dev_info(&s->dev, "no CIS, assuming an anonymous memory card.\n");
+			pcmcia_replace_cis(s, "\xFF", 1);
+			count = 1;
+			ret = 0;
+		} else
+#endif
+		{
+			mutex_lock(&s->ops_mutex);
+			destroy_cis_cache(s);
+			mutex_unlock(&s->ops_mutex);
+			ret = -EIO;
+		}
 	}
 
 	if (info)

Unfortunately, this patch does not work well with all of the callers of pccard_validate_cis(). While it helps for ds.c:pcmcia_card_add() and does not matter for cistpl.c:pccard_show_cis(), it breaks the callback in rsrc_nonstatic.c:readable():

There, we test whether iomem resources actually work -- and we test this by reading the CIS. This patch means that non-working resources are assumed to work -- and the valid CIS is replaced with the fake CIS in this case.

Therefore, I'd suggest to move the override to the one place where it is needed -- to ds.c:pcmcia_card_add(). A patch which implements this is below; it fixes my test setup (which needs rsrc_nonstatic.c).

Alan, could you verify this patch helps with the use case you had in mind when writing this patch? I inted to apply this patch to the PCMCIA tree only after such testing.

Best,
Dominik

--------------------------------8<---------------------------------
pcmcia: do not break rsrc_nonstatic when handling anonymous cards

Patch 1c6c9b1d9d25 caused a regression for rsrc_nonstatic: It relies on pccard_validate_cis() to determine whether an iomem resource can be used for PCMCIA cards. This override, however, lead invalid iomem resources to be accepted -- and lead to a fake CIS being used instead of the original CIS.

To fix this issue, move the override for anonymous cards to the one place where it is needed -- when adding a PCMCIA device.

index 64d0515..d444415 100644
@@ -1451,26 +1451,16 @@ int pccard_validate_cis(struct pcmcia_socket *s, unsigned int *info)
 done:
 	/* invalidate CIS cache on failure */
 	if (!dev_ok || !ident_ok || !count) {
-#if defined(CONFIG_MTD_PCMCIA_ANONYMOUS)
-		/* Set up as an anonymous card. If we don't have anonymous
-		   memory support then just error the card as there is no
-		   point trying to second guess.
-
-		   Note: some cards have just a device entry, it may be
-		   worth extending support to cover these in future */
-		if (!dev_ok || !ident_ok) {
-			dev_info(&s->dev, "no CIS, assuming an anonymous memory card.\n");
-			pcmcia_replace_cis(s, "\xFF", 1);
-			count = 1;
-			ret = 0;
-		} else
-#endif
-		{
-			mutex_lock(&s->ops_mutex);
-			destroy_cis_cache(s);
-			mutex_unlock(&s->ops_mutex);
+		mutex_lock(&s->ops_mutex);
+		destroy_cis_cache(s);
+		mutex_unlock(&s->ops_mutex);
+		/* We differentiate between dev_ok, ident_ok and count
+		   failures to allow for an override for anonymous cards
+		   in ds.c */
+		if (!dev_ok || !ident_ok)
 			ret = -EIO;
-		}
+		else
+			ret = -EFAULT;
 	}
 
 	if (info)

index d3baf0b..e1498a0 100644
@@ -634,8 +634,24 @@ static int pcmcia_card_add(struct pcmcia_socket *s)
 
 	ret = pccard_validate_cis(s, &no_chains);
 	if (ret || !no_chains) {
-		dev_dbg(&s->dev, "invalid CIS or invalid resources\n");
-		return -ENODEV;
+#if defined(CONFIG_MTD_PCMCIA_ANONYMOUS)
+		/* Set up as an anonymous card. If we don't have anonymous
+		   memory support then just error the card as there is no
+		   point trying to second guess.
+
+		   Note: some cards have just a device entry, it may be
+		   worth extending support to cover these in future */
+		if (ret == -EIO) {
+			dev_info(&s->dev, "no CIS, assuming an anonymous memory card.\n");
+			pcmcia_replace_cis(s, "\xFF", 1);
+			no_chains = 1;
+			ret = 0;
+		} else
+#endif
+		{
+			dev_dbg(&s->dev, "invalid CIS or invalid resources\n");
+			return -ENODEV;
+		}
 	}
 
 	if (!pccard_read_tuple(s, BIND_FN_ALL, CISTPL_LONGLINK_MFC, &mfc))

I'm not sure it's the right way to do readable() but we seem to be stuck with that anyway.

The change looks good to me, and I will try and test it soon but it may take some time due to various other things going on in life. Can you submit it for 4.2 anyway and give it a bit of time for any screaming then push to stable ? The number of people using anonymous cards is pretty small these days so I think it's better to fix the regression you have and worry about anonymous cards as a secondary thing.

Alan

Indeed - any replacement would need much testing at least.

Have added it to my tree, and will do as you suggest.

Thanks,

Dominik

On a pure PCI platform we don't actually need all the complexity of the rsrc_nonstatic manager, in fact we can just work directly with the pci allocators and avoid all the complexity (and code bloat).

---
 drivers/pcmcia/Kconfig    |   12 ++-
 drivers/pcmcia/Makefile   |    1 
 drivers/pcmcia/rsrc_pci.c |  172 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 182 insertions(+), 3 deletions(-)
 create mode 100644 drivers/pcmcia/rsrc_pci.c

index b0ce7cd..2bd21a6 100644
@@ -69,7 +69,8 @@ config YENTA
 	tristate "CardBus yenta-compatible bridge support"
 	depends on PCI
 	select CARDBUS if !EXPERT
-	select PCCARD_NONSTATIC if PCMCIA != n
+	select PCCARD_NONSTATIC if PCMCIA != n && ISA
+	select PCCARD_PCI if PCMCIA !=n && !ISA
 	---help---
 	  This option enables support for CardBus host bridges.  Virtually
 	  all modern PCMCIA bridges are CardBus compatible.  A "bridge" is

@@ -109,7 +110,8 @@ config YENTA_TOSHIBA
 config PD6729
 	tristate "Cirrus PD6729 compatible bridge support"
 	depends on PCMCIA && PCI
-	select PCCARD_NONSTATIC
+	select PCCARD_NONSTATIC if PCMCIA != n && ISA
+	select PCCARD_PCI if PCMCIA !=n && !ISA
 	help
 	  This provides support for the Cirrus PD6729 PCI-to-PCMCIA bridge
 	  device, found in some older laptops and PCMCIA card readers.

@@ -117,7 +119,8 @@ config PD6729
 config I82092
 	tristate "i82092 compatible bridge support"
 	depends on PCMCIA && PCI
-	select PCCARD_NONSTATIC
+	select PCCARD_NONSTATIC if PCMCIA != n && ISA
+	select PCCARD_PCI if PCMCIA !=n && !ISA
 	help
 	  This provides support for the Intel I82092AA PCI-to-PCMCIA bridge device,
 	  found in some older laptops and more commonly in evaluation boards for the

@@ -289,6 +292,9 @@ config ELECTRA_CF
 	  Say Y here to support the CompactFlash controller on the
 	  PA Semi Electra eval board.
 
+config PCCARD_PCI
+	bool
+
 config PCCARD_NONSTATIC
 	bool
 

index 27e94b3..f1a7ca0 100644
@@ -12,6 +12,7 @@ obj-$(CONFIG_PCMCIA)				+= pcmcia.o
 pcmcia_rsrc-y					+= rsrc_mgr.o
 pcmcia_rsrc-$(CONFIG_PCCARD_NONSTATIC)		+= rsrc_nonstatic.o
 pcmcia_rsrc-$(CONFIG_PCCARD_IODYN)		+= rsrc_iodyn.o
+pcmcia_rsrc-$(CONFIG_PCCARD_PCI)		+= rsrc_pci.o
 obj-$(CONFIG_PCCARD)				+= pcmcia_rsrc.o
 
 

Alan,

I know you're re-working this thing by now, but still:

Can we be certain that BIOS, ACPI etc. properly report all io resources which must not be utilized by other devices? Does this really depend on ISA being set in Kconfig? Should this also be enabled for CardBus bridges on the root PCI bus? And: could doing a check for X86 like in rsrc_nonstatic.c ( avoid anything < 0x100 for ioports ) help to avoid some of the possible fallout?

Best,

Dominik

It's on the todo list to finish debugging

No you can't. However there appears to be a convention that for mmio the windows are aligned on largish boundaries and vendors only hide hardware so that it's next to existing resources on an alignment such that it won't get allocated.

I've no idea if it's in a spec anywhere or that's just a "Hey it works in Windows" bit of history.

Probably yes.

Added to the TODO list for it.

Alan