lkml.org 
[lkml]   [2012]   [May]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH] w1: Introduce a slave mutex for serializing IO.
    On Fri, 4 May 2012 01:27:06 +0400 Evgeniy Polyakov <zbr@ioremap.net> wrote:

    > On Fri, May 04, 2012 at 07:08:38AM +1000, NeilBrown (neilb@suse.de) wrote:
    > > You can only check the owner on SMP builds, or when debugging is enabled.
    > > So I don't think that approach can work.
    >
    > You can store owner in master device and protect with mutex itself.
    > On non-smp systems it can not be preempted, so can be checked without
    > mutex.
    >

    I tried that - or something a lot like it. Patch below.

    However lockdep didn't like it. There are ordering problems between this
    mutex and and sysfs's s_active.

    When you access battery properies via sysfs, the sysfs lock is taken first,
    then the master->mutex.
    When w1_reconnect_slaves calls through to device_del and sys_addrm_finish,
    the mutex is held while the sysfs lock is wanted.

    So we might need to come up with something more clever.

    I haven't had a chance to look really deeply into this yet. Hopefully when I
    do I'll find something clever and let you know.

    Thanks,
    NeilBrown



    diff --git a/drivers/w1/slaves/w1_bq27000.c b/drivers/w1/slaves/w1_bq27000.c
    index 52ad812..83ebaad 100644
    --- a/drivers/w1/slaves/w1_bq27000.c
    +++ b/drivers/w1/slaves/w1_bq27000.c
    @@ -30,11 +30,14 @@ static int w1_bq27000_read(struct device *dev, unsigned int reg)
    {
    u8 val;
    struct w1_slave *sl = container_of(dev->parent, struct w1_slave, dev);
    + bool own_mutex = (sl->master->mutex_owner == current);

    - mutex_lock(&sl->master->mutex);
    + if (!own_mutex)
    + mutex_lock(&sl->master->mutex);
    w1_write_8(sl->master, HDQ_CMD_READ | reg);
    val = w1_read_8(sl->master);
    - mutex_unlock(&sl->master->mutex);
    + if (!own_mutex)
    + mutex_unlock(&sl->master->mutex);

    return val;
    }
    diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
    index 9761950..97de03d 100644
    --- a/drivers/w1/w1.c
    +++ b/drivers/w1/w1.c
    @@ -616,7 +616,13 @@ static int __w1_attach_slave_device(struct w1_slave *sl)
    dev_dbg(&sl->dev, "%s: registering %s as %p.\n", __func__,
    dev_name(&sl->dev), sl);

    + /* device_register might end up asking the slave to
    + * access the bus, so we must let it know that it
    + * already holds the lock.
    + */
    + sl->master->mutex_owner = current;
    err = device_register(&sl->dev);
    + sl->master->mutex_owner = NULL;
    if (err < 0) {
    dev_err(&sl->dev,
    "Device registration [%s] failed. err=%d\n",
    diff --git a/drivers/w1/w1.h b/drivers/w1/w1.h
    index 4d012ca..ebb157c 100644
    --- a/drivers/w1/w1.h
    +++ b/drivers/w1/w1.h
    @@ -180,6 +180,13 @@ struct w1_master

    struct task_struct *thread;
    struct mutex mutex;
    + /* The mutex_owner owns the mutex and so does not
    + * need to take it again (and doing so would deadlock).
    + * This is important when registering a device while holding
    + * the mutex as the slave might need to access the bus as part
    + * of registration.
    + */
    + struct task_struct *mutex_owner;

    struct device_driver *driver;
    struct device dev;[unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2012-05-09 04:01    [W:0.024 / U:3.380 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site