| Message ID | 1520274579-30492-1-git-send-email-fkan@apm.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Mon, Mar 5, 2018 at 10:29 AM, Feng Kan <fkan@apm.com> wrote: > When bridge and its endpoint is enumerated the devices are added to the > dpm list. Afterward, the bridge defers probe when IOMMU is not ready. > This causes the bridge to be moved to the end of the dpm list when > deferred probe kicks in. The order of the dpm list for bridge and > endpoint is reversed. > > Add reordering code to move the bridge and its children and consumers to > the end of the pm list so the order for suspend and resume is not altered. > The code also move device and its children and consumers to the tail of > device_kset list if it is registered. > > Signed-off-by: Feng Kan <fkan@apm.com> > Signed-off-by: Toan Le <toanle@apm.com> > --- > V3: > 1. additional code comment changes > V2: > 1. change patch title from "move device and its children..." > 2. move define based on Bjorn's comment > 3. rename function name and comment content > > drivers/base/base.h | 3 +++ > drivers/base/core.c | 20 ++++++++++++++++++++ > drivers/base/dd.c | 4 +--- > 3 files changed, 24 insertions(+), 3 deletions(-) > > diff --git a/drivers/base/base.h b/drivers/base/base.h > index d800de6..a75c302 100644 > --- a/drivers/base/base.h > +++ b/drivers/base/base.h > @@ -161,3 +161,6 @@ static inline void module_remove_driver(struct device_driver *drv) { } > extern void device_links_no_driver(struct device *dev); > extern bool device_links_busy(struct device *dev); > extern void device_links_unbind_consumers(struct device *dev); > + > +/* device pm support */ > +void device_pm_move_to_tail(struct device *dev); > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 110230d..3b37906 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -148,6 +148,26 @@ static int device_reorder_to_tail(struct device *dev, void *not_used) > } > > /** > + * device_pm_move_to_tail - Move set of devices to the end of device lists > + * @dev: current device pointer > + * > + * This is a device_reorder_to_tail() wrapper taking the requisite locks. > + * > + * It moves the device along with all of its children and all of its consumers > + * to the ends of the device_kset and dpm_list, recursively. > + */ > +void device_pm_move_to_tail(struct device *dev) > +{ > + int idx; > + > + idx = device_links_read_lock(); > + device_pm_lock(); > + device_reorder_to_tail(dev, NULL); > + device_pm_unlock(); > + device_links_read_unlock(idx); > +} > + > +/** > * device_link_add - Create a link between two devices. > * @consumer: Consumer end of the link. > * @supplier: Supplier end of the link. > diff --git a/drivers/base/dd.c b/drivers/base/dd.c > index 2c964f5..96fab29 100644 > --- a/drivers/base/dd.c > +++ b/drivers/base/dd.c > @@ -123,9 +123,7 @@ static void deferred_probe_work_func(struct work_struct *work) > * the list is a good order for suspend but deferred > * probe makes that very unsafe. > */ > - device_pm_lock(); > - device_pm_move_last(dev); > - device_pm_unlock(); > + device_pm_move_to_tail(dev); > > dev_dbg(dev, "Retrying from deferred list\n"); > if (initcall_debug && !initcalls_done) > -- > 1.8.3.1 > Hi, just wanted to check if there is anything else I need to change on this. Thanks.
On Thu, Mar 15, 2018 at 11:41:33AM -0700, Feng Kan wrote: > On Mon, Mar 5, 2018 at 10:29 AM, Feng Kan <fkan@apm.com> wrote: > > When bridge and its endpoint is enumerated the devices are added to the > > dpm list. Afterward, the bridge defers probe when IOMMU is not ready. > > This causes the bridge to be moved to the end of the dpm list when > > deferred probe kicks in. The order of the dpm list for bridge and > > endpoint is reversed. > > > > Add reordering code to move the bridge and its children and consumers to > > the end of the pm list so the order for suspend and resume is not altered. > > The code also move device and its children and consumers to the tail of > > device_kset list if it is registered. > > > > Signed-off-by: Feng Kan <fkan@apm.com> > > Signed-off-by: Toan Le <toanle@apm.com> > > ... > > > Hi, just wanted to check if there is anything else I need to change on > this. Thanks. I'm assuming Rafael will handle this.
On Fri, Mar 30, 2018 at 3:50 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: > On Thu, Mar 15, 2018 at 11:41:33AM -0700, Feng Kan wrote: >> On Mon, Mar 5, 2018 at 10:29 AM, Feng Kan <fkan@apm.com> wrote: >> > When bridge and its endpoint is enumerated the devices are added to the >> > dpm list. Afterward, the bridge defers probe when IOMMU is not ready. >> > This causes the bridge to be moved to the end of the dpm list when >> > deferred probe kicks in. The order of the dpm list for bridge and >> > endpoint is reversed. >> > >> > Add reordering code to move the bridge and its children and consumers to >> > the end of the pm list so the order for suspend and resume is not altered. >> > The code also move device and its children and consumers to the tail of >> > device_kset list if it is registered. >> > >> > Signed-off-by: Feng Kan <fkan@apm.com> >> > Signed-off-by: Toan Le <toanle@apm.com> >> > ... >> > >> Hi, just wanted to check if there is anything else I need to change on >> this. Thanks. > > I'm assuming Rafael will handle this. Rafael, sorry to ping again. Do you have a position on this patch yet?
On Wednesday, April 4, 2018 11:10:41 PM CEST Feng Kan wrote: > On Fri, Mar 30, 2018 at 3:50 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Thu, Mar 15, 2018 at 11:41:33AM -0700, Feng Kan wrote: > >> On Mon, Mar 5, 2018 at 10:29 AM, Feng Kan <fkan@apm.com> wrote: > >> > When bridge and its endpoint is enumerated the devices are added to the > >> > dpm list. Afterward, the bridge defers probe when IOMMU is not ready. > >> > This causes the bridge to be moved to the end of the dpm list when > >> > deferred probe kicks in. The order of the dpm list for bridge and > >> > endpoint is reversed. > >> > > >> > Add reordering code to move the bridge and its children and consumers to > >> > the end of the pm list so the order for suspend and resume is not altered. > >> > The code also move device and its children and consumers to the tail of > >> > device_kset list if it is registered. > >> > > >> > Signed-off-by: Feng Kan <fkan@apm.com> > >> > Signed-off-by: Toan Le <toanle@apm.com> > >> > ... > >> > > >> Hi, just wanted to check if there is anything else I need to change on > >> this. Thanks. > > > > I'm assuming Rafael will handle this. > Rafael, sorry to ping again. Do you have a position on this patch yet? Sorry, I thought that Greg might want to send a comment here. I'm basically fine with the approach. I only have two minor remarks to the kernedoc comment of the new function, but let me reply directly to the patch message.
On Monday, March 5, 2018 7:29:39 PM CEST Feng Kan wrote: > When bridge and its endpoint is enumerated the devices are added to the > dpm list. Afterward, the bridge defers probe when IOMMU is not ready. > This causes the bridge to be moved to the end of the dpm list when > deferred probe kicks in. The order of the dpm list for bridge and > endpoint is reversed. > > Add reordering code to move the bridge and its children and consumers to > the end of the pm list so the order for suspend and resume is not altered. > The code also move device and its children and consumers to the tail of > device_kset list if it is registered. > > Signed-off-by: Feng Kan <fkan@apm.com> > Signed-off-by: Toan Le <toanle@apm.com> > --- > V3: > 1. additional code comment changes > V2: > 1. change patch title from "move device and its children..." > 2. move define based on Bjorn's comment > 3. rename function name and comment content > > drivers/base/base.h | 3 +++ > drivers/base/core.c | 20 ++++++++++++++++++++ > drivers/base/dd.c | 4 +--- > 3 files changed, 24 insertions(+), 3 deletions(-) > > diff --git a/drivers/base/base.h b/drivers/base/base.h > index d800de6..a75c302 100644 > --- a/drivers/base/base.h > +++ b/drivers/base/base.h > @@ -161,3 +161,6 @@ static inline void module_remove_driver(struct device_driver *drv) { } > extern void device_links_no_driver(struct device *dev); > extern bool device_links_busy(struct device *dev); > extern void device_links_unbind_consumers(struct device *dev); > + > +/* device pm support */ > +void device_pm_move_to_tail(struct device *dev); > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 110230d..3b37906 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -148,6 +148,26 @@ static int device_reorder_to_tail(struct device *dev, void *not_used) > } > > /** > + * device_pm_move_to_tail - Move set of devices to the end of device lists > + * @dev: current device pointer What does "current" mean here, exactly? I guess it would be better to say "Device to move" or similar here. > + * > + * This is a device_reorder_to_tail() wrapper taking the requisite locks. > + * > + * It moves the device along with all of its children and all of its consumers "The device" is @dev, so it's better to use @dev here, like "It moves @dev along with ..." Thanks, Rafael
diff --git a/drivers/base/base.h b/drivers/base/base.h index d800de6..a75c302 100644 --- a/drivers/base/base.h +++ b/drivers/base/base.h @@ -161,3 +161,6 @@ static inline void module_remove_driver(struct device_driver *drv) { } extern void device_links_no_driver(struct device *dev); extern bool device_links_busy(struct device *dev); extern void device_links_unbind_consumers(struct device *dev); + +/* device pm support */ +void device_pm_move_to_tail(struct device *dev); diff --git a/drivers/base/core.c b/drivers/base/core.c index 110230d..3b37906 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -148,6 +148,26 @@ static int device_reorder_to_tail(struct device *dev, void *not_used) } /** + * device_pm_move_to_tail - Move set of devices to the end of device lists + * @dev: current device pointer + * + * This is a device_reorder_to_tail() wrapper taking the requisite locks. + * + * It moves the device along with all of its children and all of its consumers + * to the ends of the device_kset and dpm_list, recursively. + */ +void device_pm_move_to_tail(struct device *dev) +{ + int idx; + + idx = device_links_read_lock(); + device_pm_lock(); + device_reorder_to_tail(dev, NULL); + device_pm_unlock(); + device_links_read_unlock(idx); +} + +/** * device_link_add - Create a link between two devices. * @consumer: Consumer end of the link. * @supplier: Supplier end of the link. diff --git a/drivers/base/dd.c b/drivers/base/dd.c index 2c964f5..96fab29 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -123,9 +123,7 @@ static void deferred_probe_work_func(struct work_struct *work) * the list is a good order for suspend but deferred * probe makes that very unsafe. */ - device_pm_lock(); - device_pm_move_last(dev); - device_pm_unlock(); + device_pm_move_to_tail(dev); dev_dbg(dev, "Retrying from deferred list\n"); if (initcall_debug && !initcalls_done)