lkml.org 
[lkml]   [2004]   [Nov]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2.6.10-rc1 2/5] driver-model: bus_recan_devices() locking fix
     Hello Dmitry,

    On Thu, Nov 04, 2004 at 11:12:58AM -0800, Dmitry Torokhov wrote:
    > Hmm, I do not like that the patch now fiddles with bus's rwsem before
    > incrementing bus's refcount.

    Yes, I agree with you.

    > I think just iterating through device list right the bus_rescan_devices
    > will be good enough. I sent the patch together with other 3, did it get
    > lost?

    Or maybe just adding a @write_lock argument to bus_for_each_*()
    functions or create a new function named __bus_for_each_*() which
    takes additional @write_lock argument.

    However, I don't really think the get_bus/put_bus() calls are
    necessary there. Unless the caller already has a reference to the
    bus, we're in trouble anyway. get_bus() cannot synchronize with the
    last put_bus(), we need outer synchronization to protect that
    (individual bus drivers are expected to perform the synchronization).

    One thing that bothers me is all the "if (kobject_get(x))" codes.
    Currently, it doesn't seem to do anything other than checking if x is
    NULL or not and incrementing the reference count. i.e. the return
    value only represents if x is NULL or not. I find the codes
    misleading. It seems like above statement can synchronize get() and
    the last put() which isn't true. I don't how how the using xchg in
    kref went, but with preemtion turned on, I don't see how it's gonna
    change the situation.

    Well, I should write the following in another mail but I'll just
    continue here.

    Another problem I've encountered regarding kobject refcounting is
    that currently each sysfs attr carries its owner and gets the owning
    module when the attr is accessed. However, there are attributes which
    are provided not by the module which implements whatever the kobject
    represents but by upper-level module or the kernel builtins. So, it's
    possible to unload a module while it's kobject is still alive by
    holding on to such attributes, and, when the attribute is closed,
    panic occurs, of course (release callback is unloaded already).

    Modules which device_driver kobjects are okay because unload_sem is
    used to prevent the module from unloading before it is released;
    however, bus_type, class and class_device kobjects have this problem.
    I found detach_state attr and all common USB attributes. Also, all
    class_device attributes, including simple_class dev attribute, which
    are defined by the class it belongs to suffer this problem (allocated
    by individual drivers but owned by the class).

    We can do one of

    1. add unload_sem to all driver-model structures.
    -> I believe this defeats the purpose of try_module_get()
    stuff. We wouldn't know when the module will unload.

    2. get the owner field correct for all attributes.
    -> This will be a chore. We'll need to allocate separate
    attr structures for each kobject for all the generic attrs.

    3. remove the owner field from the attribute structure and add an
    owner to kobject and make sysfs ops to hold the owner of the
    respective kobject.
    -> I think this is the best and most consistent solution. As
    we already assume that module which implements a child kobject
    should depend upon the module which implements its parent, all
    attr module reference counting will be correct by using the
    kobject's owner.

    So, I'm currently working on solution #3.

    IMHO, we really need some kind of documentation describing the
    synchronization, reference counting and behavior assumptions in the
    driver-model. It just seems a bit too subtle and fragile as it stands
    now.

    Thanks.

    --
    tejun

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

    \
     
     \ /
      Last update: 2005-03-22 14:07    [W:0.025 / U:29.812 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site