lkml.org 
[lkml]   [2011]   [Nov]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v3 2/3] hvc_init(): Enforce one-time initialization.
    From
    Date
    On Tue, 2011-11-08 at 13:45 -0800, Miche Baker-Harvey wrote:
    > hvc_init() must only be called once, and no thread should continue with hvc_alloc()
    > until after initialization is complete. The original code does not enforce either
    > of these requirements. A new mutex limits entry to hvc_init() to a single thread,
    > and blocks all later comers until it has completed.
    >
    > This patch fixes multiple crash symptoms.

    Hi Miche,

    A few nit-picky comments below ..

    > @@ -84,6 +85,10 @@ static LIST_HEAD(hvc_structs);
    > * list traversal.
    > */
    > static DEFINE_SPINLOCK(hvc_structs_lock);
    > +/*
    > + * only one task does allocation at a time.
    > + */
    > +static DEFINE_MUTEX(hvc_ports_mutex);

    The comment is wrong, isn't it? Only one task does _init_ at a time.
    Once the driver is initialised allocs can run concurrently.

    So shouldn't it be called hvc_init_mutex ?

    > @@ -825,11 +830,15 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
    > int i;
    >
    > /* We wait until a driver actually comes along */
    > + mutex_lock(&hvc_ports_mutex);
    > if (!hvc_driver) {
    > int err = hvc_init();
    > - if (err)
    > + if (err) {
    > + mutex_unlock(&hvc_ports_mutex);
    > return ERR_PTR(err);
    > + }
    > }
    > + mutex_unlock(&hvc_ports_mutex);
    >
    > hp = kzalloc(ALIGN(sizeof(*hp), sizeof(long)) + outbuf_size,
    > GFP_KERNEL);

    It'd be cleaner I think to do all the locking in hvc_init(). That's safe
    as long as you recheck !hvc_driver in hvc_init() with the lock held.

    cheers

    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2011-11-09 08:27    [W:0.037 / U:30.384 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site