[PATCH] pcmcia: Fix broken abuse of dev->driver_data

[PATCH] pcmcia: Fix broken abuse of dev->driver_data

[Alan Cox]

From: Alan Cox <alan@redhat.com>

PCMCIA abuses dev->private_data in the probe methods. Unfortunately it
continues to abuse it after calling drv->probe() which leads to crashes and
other nasties (such as bogus probes of multifunction devices) giving errors like

pcmcia: registering new device pcmcia0.1
kernel: 0.1: GetNextTuple: No more items

Extract the passed data before calling the driver probe function that way
we don't blow up when the driver reuses dev->private_data as its right.

As its close to the final release just move the hack so it works out,
hopefully someone will be sufficiently embarrassed to produce a nice rework
for 2.6.28.

Signed-off-by: Alan Cox <alan@redhat.com>
---

 drivers/pcmcia/ds.c |   23 ++++++++++++++---------
 1 files changed, 14 insertions(+), 9 deletions(-)


diff --git a/drivers/pcmcia/ds.c b/drivers/pcmcia/ds.c
index 4174d96..853d342 100644
--- a/drivers/pcmcia/ds.c
+++ b/drivers/pcmcia/ds.c
@@ -426,6 +426,18 @@ static int pcmcia_device_probe(struct device * dev)
 	p_dev = to_pcmcia_dev(dev);
 	p_drv = to_pcmcia_drv(dev->driver);
 	s = p_dev->socket;
+	
+	/* The PCMCIA code passes the match data in via dev->driver_data
+	 * which is an ugly hack. Once the driver probe is called it may
+	 * and often will overwrite the match data so we must save it first 
+	 *
+	 * handle pseudo multifunction devices:
+	 * there are at most two pseudo multifunction devices.
+	 * if we're matching against the first, schedule a
+	 * call which will then check whether there are two
+	 * pseudo devices, and if not, add the second one.
+	 */
+	did = p_dev->dev.driver_data;
 
 	ds_dbg(1, "trying to bind %s to %s\n", p_dev->dev.bus_id,
 	       p_drv->drv.name);
@@ -455,21 +467,14 @@ static int pcmcia_device_probe(struct device * dev)
 		goto put_module;
 	}
 
-	/* handle pseudo multifunction devices:
-	 * there are at most two pseudo multifunction devices.
-	 * if we're matching against the first, schedule a
-	 * call which will then check whether there are two
-	 * pseudo devices, and if not, add the second one.
-	 */
-	did = p_dev->dev.driver_data;
 	if (did && (did->match_flags & PCMCIA_DEV_ID_MATCH_DEVICE_NO) &&
 	    (p_dev->socket->device_count == 1) && (p_dev->device_no ==
0)) pcmcia_add_device_later(p_dev->socket, 0);
 
- put_module:
+put_module:
 	if (ret)
 		module_put(p_drv->owner);
- put_dev:
+put_dev:
 	if (ret)
 		put_device(dev);
 	return (ret);
--
<meme>discombobulated echidna</meme>

Re: [PATCH] pcmcia: Fix broken abuse of dev->driver_data

[Dominik Brodowski]

Alan,

(CC: linux-pcmcia added)

Many thanks for finding this bug.

On Mon, Sep 22, 2008 at 03:58:14PM +0100, Alan Cox wrote:
> PCMCIA abuses dev->private_data in the probe methods. Unfortunately it
> continues to abuse it after calling drv->probe() which leads to crashes and
> other nasties (such as bogus probes of multifunction devices) giving errors like

Well, PCMCIA drivers have (struct pcmcia_device *) p_dev->priv available and
use this regularly, so using dev->private_data never has been supported.
Nonetheless, let's add support for using dev->private_data also for PCMCIA
devices and remove p_dev->priv eventually.

> As its close to the final release just move the hack so it works out,
> hopefully someone will be sufficiently embarrassed to produce a nice rework
> for 2.6.28.

Could you remove this from the changelog entry, please?


> @@ -426,6 +426,18 @@ static int pcmcia_device_probe(struct device * dev)
>  	p_dev = to_pcmcia_dev(dev);
>  	p_drv = to_pcmcia_drv(dev->driver);
>  	s = p_dev->socket;
> +	

trailing whitespace

> +	/* The PCMCIA code passes the match data in via dev->driver_data
> +	 * which is an ugly hack. Once the driver probe is called it may
> +	 * and often will overwrite the match data so we must save it first 

trailing whitespace

>  	if (did && (did->match_flags & PCMCIA_DEV_ID_MATCH_DEVICE_NO) &&
>  	    (p_dev->socket->device_count == 1) && (p_dev->device_no ==
> 0)) pcmcia_add_device_later(p_dev->socket, 0);

uh?

>  
> - put_module:
> +put_module:
>  	if (ret)
>  		module_put(p_drv->owner);
> - put_dev:
> +put_dev:

unrelated -- please do not change it this time.

Best and thanks again,

	Dominik

Re: [PATCH] pcmcia: Fix broken abuse of dev->driver_data

[Alan Cox]

> > +	
> 
> trailing whitespace

Nod..

> >  	if (did && (did->match_flags & PCMCIA_DEV_ID_MATCH_DEVICE_NO) &&
> >  	    (p_dev->socket->device_count == 1) && (p_dev->device_no ==
> > 0)) pcmcia_add_device_later(p_dev->socket, 0);
> 
> uh?

Looks like a mailer folded it later, but the patch itself seems just fine.

> >  
> > - put_module:
> > +put_module:
> >  	if (ret)
> >  		module_put(p_drv->owner);
> > - put_dev:
> > +put_dev:
> 
> unrelated -- please do not change it this time.

Not sure you can have it both ways - if you don't want stray whitespace
then fixing the labels to conform to coding style seems to go with it ;)

Alan

Re: [PATCH] pcmcia: Fix broken abuse of dev->driver_data

[Dominik Brodowski]

On Mon, Sep 22, 2008 at 11:12:27PM +0100, Alan Cox wrote:
> Nod..
...
> Looks like a mailer folded it later, but the patch itself seems just fine.

*gnah* already applied upstream, including the comment implying that PCMCIA
driver's making use of driver_data were correct, while they were not. At
least the trailing whitespace didn't get merged.

> > >  
> > > - put_module:
> > > +put_module:
> > >  	if (ret)
> > >  		module_put(p_drv->owner);
> > > - put_dev:
> > > +put_dev:
> > 
> > unrelated -- please do not change it this time.
> 
> Not sure you can have it both ways - if you don't want stray whitespace
> then fixing the labels to conform to coding style seems to go with it ;)

Well, there's a difference, and I know you know that I know that you know ;)
but you should also know that I already tried to apply the patch as it is,
which implies that I do not object too strongly to the removal of these two
non-characters from ds.c ;)

Thanks,
	Dominik

Re: [PATCH] pcmcia: Fix broken abuse of dev->driver_data

[Alan Cox]

> *gnah* already applied upstream, including the comment implying that PCMCIA
> driver's making use of driver_data were correct, while they were not. At
> least the trailing whitespace didn't get merged.

It was correct - dev->private_data for struct device is driver stuff, and
as the same drivers are used for pcmcia and non pcmcia its a bit hard to
argue otherwise.

Anyway tis finally sorted (and boy was it fun to pin down)

Alan

Re: [PATCH] pcmcia: Fix broken abuse of dev->driver_data

[Komuro]

Hi,

>PCMCIA abuses dev->private_data in the probe methods. Unfortunately it
>continues to abuse it after calling drv->probe() which leads to crashes and
>other nasties (such as bogus probes of multifunction devices) giving errors like
>
>pcmcia: registering new device pcmcia0.1
>kernel: 0.1: GetNextTuple: No more items

The pata_pcmcia works now. Thanks!



Best Regards
Komuro

Re: [PATCH git latest] drivers/net: fixing a datarace related to update_stats()

[Komuro]

Hi,

You need two braces...

- 	if ((inw(ioaddr+EL3_STATUS) & 0xe000) == 0x2000)
+ 	if ((inw(ioaddr+EL3_STATUS) & 0xe000) == 0x2000) {
+    	    spin_lock_irqsave(&lp->lock, flags);
 	    update_stats(dev);
+    	    spin_unlock_irqrestore(&lp->lock, flags);
+	}
     }

Re: [PATCH git latest] drivers/net: fixing a datarace related to update_stats()

[Tammy]

> You need two braces...
>
> -       if ((inw(ioaddr+EL3_STATUS) & 0xe000) == 0x2000)
> +       if ((inw(ioaddr+EL3_STATUS) & 0xe000) == 0x2000) {
> +           spin_lock_irqsave(&lp->lock, flags);
>            update_stats(dev);
> +           spin_unlock_irqrestore(&lp->lock, flags);
> +       }
>     }
>


Thanks a lot. I will resubmit the patch then.

Lin

Re: [PATCH git latest] drivers/net: fixing a datarace related to update_stats()

[Lin Tan]

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

Resending the corrected patch.

[-- Attachment #2: update_stats.readytosend --]
[-- Type: text/plain, Size: 1275 bytes --]

Fixing a datarace.

As indicated by the following comment, a lock must be held before calling 
function update_stats(). This rule is followed in some cases, but not in 
others. For example, the lock is held when the function is called in function 
el3_get_stats(), but the lock is NOT held when called in el3_close(). It can 
cause potential data races. 

/*
  ...
  
  Caller must hold the lock for this
*/
static void update_stats(struct net_device *dev)
{
...
}


Signed-off-by: Lin Tan <tammy000@gmail.com>

---

--- a/drivers/net/pcmcia/3c589_cs.c	2008-09-25 11:52:42.000000000 -0500
+++ b/drivers/net/pcmcia/3c589_cs.c	2008-09-26 11:07:04.000000000 -0500
@@ -920,6 +920,7 @@
     struct el3_private *lp = netdev_priv(dev);
     struct pcmcia_device *link = lp->p_dev;
     unsigned int ioaddr = dev->base_addr;
+    unsigned long flags;
     
     DEBUG(1, "%s: shutting down ethercard.\n", dev->name);
 
@@ -946,8 +947,11 @@
 	outw(0x0f00, ioaddr + WN0_IRQ);
         
 	/* Check if the card still exists */
-	if ((inw(ioaddr+EL3_STATUS) & 0xe000) == 0x2000)
+	if ((inw(ioaddr+EL3_STATUS) & 0xe000) == 0x2000) {
+    	    spin_lock_irqsave(&lp->lock, flags);
 	    update_stats(dev);
+    	    spin_unlock_irqrestore(&lp->lock, flags);
+	}
     }
 
     link->open--;

RE: [PATCH 03/19] pcmcia: Whine harder about use of EXCLUSIVE

[Komuro]

duplicate warning message in pcmcia_resource.c ....


>   /* Make sure the fact the request type was overridden is passed back */
>    if (type == IRQF_SHARED && !(req->Attributes & IRQ_TYPE_DYNAMIC_SHARING)) {
>        req->Attributes |= IRQ_TYPE_DYNAMIC_SHARING;
>        printk(KERN_WARNING "pcmcia: request for exclusive IRQ could not be fulfilled.\n");
>        printk(KERN_WARNING "pcmcia: the driver needs updating to supported shared IRQ lines\n");
>     }



>diff --git a/drivers/pcmcia/pcmcia_resource.c b/drivers/pcmcia/pcmcia_resource.c
>index 4884a18..8f1c3d4 100644
>--- a/drivers/pcmcia/pcmcia_resource.c
>+++ b/drivers/pcmcia/pcmcia_resource.c
>@@ -738,8 +738,9 @@ int pcmcia_request_irq(struct pcmcia_device *p_dev, irq_req_t *req)
> 	type = 0;
> 	if (s->functions > 1)		/* All of this ought to be handled higher up */
> 		type = IRQF_SHARED;
>-	if (req->Attributes & IRQ_TYPE_DYNAMIC_SHARING)
>+	else if (req->Attributes & IRQ_TYPE_DYNAMIC_SHARING)
> 		type = IRQF_SHARED;
>+	else printk(KERN_WARNING "pcmcia: Driver needs updating to support IRQ sharing.\n");
> 
> #ifdef CONFIG_PCMCIA_PROBE
 

Best Regards
Komuro

[BUG REPORT!!!] [PATCH 19/49] pcmcia: remove remaining in-kernel pcmcia_get_configuration_info() users

[Komuro]

Dear Dominik

Unfortunately, your path "[PATCH 19/49] pcmcia: remove remaining in-kernel
pcmcia_get_configuration_info() users"
broke the "serial_cs" and other drivers.

You replace "config->BasePort2" to "link->io.BasePort2" at serial_cs. 

But link->io.BasePort2, link->io.NumPorts2(etc) 
does not contain correct value (it is zero).

Please fix this problem (or reveart the patch).


Best Regards
Komuro

Here is your patch.
>--- a/drivers/serial/serial_cs.c
>+++ b/drivers/serial/serial_cs.c
>@@ -488,23 +488,23 @@ static int simple_config_check_notpicky(struct pcmcia_device *p_dev,
> static int simple_config(struct pcmcia_device *link)
> {
> 	struct serial_info *info = link->priv;
>-	config_info_t config;
>-	int i, try;
>+	int i = -ENODEV, try;
> 
> 	/* If the card is already configured, look up the port and irq */
>-	i = pcmcia_get_configuration_info(link, &config);
>-	if ((i == CS_SUCCESS) && (config.Attributes & CONF_VALID_CLIENT)) {
>+	if (link->function_config) {
> 		unsigned int port = 0;
>-		if ((config.BasePort2 != 0) && (config.NumPorts2 == 8)) {
>-			port = config.BasePort2;
>+		if ((link->io.BasePort2 != 0) &&
>+		    (link->io.NumPorts2 == 8)) {
>+			port = link->io.BasePort2;
> 			info->slave = 1;
> 		} else if ((info->manfid == MANFID_OSITECH) &&
>-			   (config.NumPorts1 == 0x40)) {
>-			port = config.BasePort1 + 0x28;
>+			   (link->io.NumPorts1 == 0x40)) {
>+			port = link->io.BasePort1 + 0x28;
> 			info->slave = 1;
> 		}
> 		if (info->slave) {
>-			return setup_serial(link, info, port, config.AssignedIRQ);
>+			return setup_serial(link, info, port,
>+					    link->irq.AssignedIRQ);
 		}
 	}

Re: [BUG REPORT!!!] [PATCH 19/49] pcmcia: remove remaining in-kernel pcmcia_get_configuration_info() users

[Komuro]

Dear Dominik


The reason of this bug is link->io is not copied
to the slave serial_cs device.

My patch below fix this problem.

--- ds.c.orig   2008-11-01 08:59:13.000000000 +0900
+++ ds.c        2008-11-02 17:05:44.000000000 +0900
@@ -668,6 +668,7 @@ struct pcmcia_device * pcmcia_device_add
         list_for_each_entry(tmp_dev, &s->devices_list, socket_device_list)
                 if (p_dev->func == tmp_dev->func) {
                        p_dev->function_config = tmp_dev->function_config;
+                       p_dev->io = tmp_dev->io;
                        kref_get(&p_dev->function_config->ref);
                }



Best Regards
Komuro



> Dear Dominik
> 
> Unfortunately, your path "[PATCH 19/49] pcmcia: remove remaining in-kernel
> pcmcia_get_configuration_info() users"
> broke the "serial_cs" and other drivers.
> 
> You replace "config->BasePort2" to "link->io.BasePort2" at serial_cs. 
> 
> But link->io.BasePort2, link->io.NumPorts2(etc) 
> does not contain correct value (it is zero).
> 
> Please fix this problem (or reveart the patch).
> 
> 
> Best Regards
> Komuro
> 
> Here is your patch.
> >--- a/drivers/serial/serial_cs.c
> >+++ b/drivers/serial/serial_cs.c
> >@@ -488,23 +488,23 @@ static int simple_config_check_notpicky(struct pcmcia_device *p_dev,
> > static int simple_config(struct pcmcia_device *link)
> > {
> > 	struct serial_info *info = link->priv;
> >-	config_info_t config;
> >-	int i, try;
> >+	int i = -ENODEV, try;
> > 
> > 	/* If the card is already configured, look up the port and irq */
> >-	i = pcmcia_get_configuration_info(link, &config);
> >-	if ((i == CS_SUCCESS) && (config.Attributes & CONF_VALID_CLIENT)) {
> >+	if (link->function_config) {
> > 		unsigned int port = 0;
> >-		if ((config.BasePort2 != 0) && (config.NumPorts2 == 8)) {
> >-			port = config.BasePort2;
> >+		if ((link->io.BasePort2 != 0) &&
> >+		    (link->io.NumPorts2 == 8)) {
> >+			port = link->io.BasePort2;
> > 			info->slave = 1;
> > 		} else if ((info->manfid == MANFID_OSITECH) &&
> >-			   (config.NumPorts1 == 0x40)) {
> >-			port = config.BasePort1 + 0x28;
> >+			   (link->io.NumPorts1 == 0x40)) {
> >+			port = link->io.BasePort1 + 0x28;
> > 			info->slave = 1;
> > 		}
> > 		if (info->slave) {
> >-			return setup_serial(link, info, port, config.AssignedIRQ);
> >+			return setup_serial(link, info, port,
> >+					    link->irq.AssignedIRQ);
>  		}
>  	}
> 
> 
> 
> 
> _______________________________________________
> Linux PCMCIA reimplementation list
> http://lists.infradead.org/mailman/listinfo/linux-pcmcia


-- 
Komuro <komurojun-mbn@nifty.com>

Re: [BUG REPORT!!!] [PATCH 19/49] pcmcia: remove remaining in-kernel pcmcia_get_configuration_info() users

[Dominik Brodowski]

Dear Komuro,

thanks for tracking this down. Could I get your Signed-off-by line for the
patch you submitted, please?

Thanks,
	Dominik

Re: [BUG REPORT!!!] [PATCH 19/49] pcmcia: remove remaining in-kernel pcmcia_get_configuration_info() users

[Komuro]

drivers/pcmcia/ds.c

setup "io" and "irq" for pseudo multifunction devices.

Signed-off-by: Komuro <komurojun-mbn@nifty.com>

---


--- linux-2.6.28-rc2/drivers/pcmcia/ds.c.orig	2008-11-01 08:59:13.000000000 +0900
+++ linux-2.6.28-rc2/drivers/pcmcia/ds.c	2008-11-02 18:59:18.000000000 +0900
@@ -668,6 +668,8 @@ struct pcmcia_device * pcmcia_device_add
         list_for_each_entry(tmp_dev, &s->devices_list, socket_device_list)
                 if (p_dev->func == tmp_dev->func) {
 			p_dev->function_config = tmp_dev->function_config;
+			p_dev->io = tmp_dev->io;
+			p_dev->irq = tmp_dev->irq;
 			kref_get(&p_dev->function_config->ref);
 		}
 

Re: [BUG REPORT!!!] [PATCH 19/49] pcmcia: remove remaining in-kernel pcmcia_get_configuration_info() users

[Dominik Brodowski]

On Sun, Nov 02, 2008 at 07:33:24PM +0900, Komuro wrote:
> 
> drivers/pcmcia/ds.c
> 
> setup "io" and "irq" for pseudo multifunction devices.

Applied, thanks!