Prateek Sood <[email protected]> 于2019年5月14日周二 下午7:00写道： > > On 5/14/19 4:26 PM, Mukesh Ojha wrote: > > ++ > > > > On 5/4/2019 8:17 PM, Muchun Song wrote: > >> Benjamin Herrenschmidt <[email protected]> 于2019年5月2日周四 下午2:25写道： > >> > >>>>> The basic idea yes, the whole bool *locked is horrid though. > >>>>> Wouldn't it > >>>>> work to have a get_device_parent_locked that always returns with > >>>>> the mutex held, > >>>>> or just move the mutex to the caller or something simpler like this > >>>>> ? > >>>>> > >>>> Greg and Rafael, do you have any suggestions for this? Or you also > >>>> agree with Ben? > >>> Ping guys ? This is worth fixing... > >> I also agree with you. But Greg and Rafael seem to be high latency right > >> now. > >> > >> From your suggestions, I think introduce get_device_parent_locked() may > >> easy > >> to fix. So, do you agree with the fix of the following code snippet > >> (You can also > >> view attachments)? > >> > >> I introduce a new function named get_device_parent_locked_if_glue_dir() > >> which > >> always returns with the mutex held only when we live in glue dir. We > >> should call > >> unlock_if_glue_dir() to release the mutex. The > >> get_device_parent_locked_if_glue_dir() > >> and unlock_if_glue_dir() should be called in pairs. > >> > >> --- > >> drivers/base/core.c | 44 ++++++++++++++++++++++++++++++++++++-------- > >> 1 file changed, 36 insertions(+), 8 deletions(-) > >> > >> diff --git a/drivers/base/core.c b/drivers/base/core.c > >> index 4aeaa0c92bda..5112755c43fa 100644 > >> --- a/drivers/base/core.c > >> +++ b/drivers/base/core.c > >> @@ -1739,8 +1739,9 @@ class_dir_create_and_add(struct class *class, > >> struct kobject *parent_kobj) > >> static DEFINE_MUTEX(gdp_mutex); > >> -static struct kobject *get_device_parent(struct device *dev, > >> - struct device *parent) > >> +static struct kobject *__get_device_parent(struct device *dev, > >> + struct device *parent, > >> + bool lock) > >> { > >> if (dev->class) { > >> struct kobject *kobj = NULL; > >> @@ -1779,14 +1780,16 @@ static struct kobject > >> *get_device_parent(struct device *dev, > >> } > >> spin_unlock(&dev->class->p->glue_dirs.list_lock); > >> if (kobj) { > >> - mutex_unlock(&gdp_mutex); > >> + if (!lock) > >> + mutex_unlock(&gdp_mutex); > >> return kobj; > >> } > >> /* or create a new class-directory at the parent device */ > >> k = class_dir_create_and_add(dev->class, parent_kobj); > >> /* do not emit an uevent for this simple "glue" directory */ > >> - mutex_unlock(&gdp_mutex); > >> + if (!lock) > >> + mutex_unlock(&gdp_mutex); > >> return k; > >> } > >> @@ -1799,6 +1802,19 @@ static struct kobject *get_device_parent(struct > >> device *dev, > >> return NULL; > >> } > >> +static inline struct kobject *get_device_parent(struct device *dev, > >> + struct device *parent) > >> +{ > >> + return __get_device_parent(dev, parent, false); > >> +} > >> + > >> +static inline struct kobject * > >> +get_device_parent_locked_if_glue_dir(struct device *dev, > >> + struct device *parent) > >> +{ > >> + return __get_device_parent(dev, parent, true); > >> +} > >> + > >> static inline bool live_in_glue_dir(struct kobject *kobj, > >> struct device *dev) > >> { > >> @@ -1831,6 +1847,16 @@ static void cleanup_glue_dir(struct device > >> *dev, struct kobject *glue_dir) > >> mutex_unlock(&gdp_mutex); > >> } > >> +static inline void unlock_if_glue_dir(struct device *dev, > >> + struct kobject *glue_dir) > >> +{ > >> + /* see if we live in a "glue" directory */ > >> + if (!live_in_glue_dir(glue_dir, dev)) > >> + return; > >> + > >> + mutex_unlock(&gdp_mutex); > >> +} > >> + > >> static int device_add_class_symlinks(struct device *dev) > >> { > >> struct device_node *of_node = dev_of_node(dev); > >> @@ -2040,7 +2066,7 @@ int device_add(struct device *dev) > >> pr_debug("device: '%s': %s\n", dev_name(dev), __func__); > >> parent = get_device(dev->parent); > >> - kobj = get_device_parent(dev, parent); > >> + kobj = get_device_parent_locked_if_glue_dir(dev, parent); > >> if (IS_ERR(kobj)) { > >> error = PTR_ERR(kobj); > >> goto parent_error; > >> @@ -2055,10 +2081,12 @@ int device_add(struct device *dev) > >> /* first, register with generic layer. */ > >> /* we require the name to be set before, and pass NULL */ > >> error = kobject_add(&dev->kobj, dev->kobj.parent, NULL); > >> - if (error) { > >> - glue_dir = get_glue_dir(dev); > >> + > >> + glue_dir = get_glue_dir(dev); > >> + unlock_if_glue_dir(dev, glue_dir); > >> + > >> + if (error) > >> goto Error; > >> - } > >> /* notify platform of device entry */ > >> error = device_platform_notify(dev, KOBJ_ADD); > >> -- > > This change has been done in device_add(). AFAICT, locked > version of get_device_parent should be used in device_move() > also. >
Yeah, I agree with you. I will send the v2 patch later to fix it also. Thanks. Yours, Muchun
