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

This is a discussion on [PATCH] pcmcia: Fix broken abuse of dev->driver_data - Kernel ; From: Alan Cox 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 ...

+ Reply to Thread
Results 1 to 15 of 15

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

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

    From: Alan Cox

    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
    ---

    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);
    --
    discombobulated echidna
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    > > +
    >
    > 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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    > *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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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);
    + }
    }
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    > 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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    Resending the corrected patch.

    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

    ---

    --- 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--;


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



    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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);

    }
    }



    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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/l...o/linux-pcmcia



    --
    Komuro
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    Dear Komuro,

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

    Thanks,
    Dominik

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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


    drivers/pcmcia/ds.c

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

    Signed-off-by: Komuro

    ---


    --- 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);
    }

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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!

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread