| Message ID | 1438018981-13893-1-git-send-email-grygorii.strashko@ti.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 7/27/2015 1:43 PM, Strashko, Grygorii wrote: > Now device's shutdown sequence is performed in reverse order of their > registration in devices_kset list and this sequence corresponds to the > reverse device's creation order. So, devices_kset data tracks > "parent<-child" device's dependencies only. > > Unfortunately, that's not enough and causes problems in case of > implementing board's specific shutdown procedures. For example [1]: > "DRA7XX_evm uses PCF8575 and one of the PCF output lines feeds to > MMC/SD and this line should be driven high in order for the MMC/SD to > be detected. This line is modelled as regulator and the hsmmc driver > takes care of enabling and disabling it. In the case of 'reboot', > during shutdown path as part of it's cleanup process the hsmmc driver > disables this regulator. This makes MMC boot not functional." > > To handle this issue the .shutdown() callback could be implemented > for PCF8575 device where corresponding GPIO pins will be configured to > states, required for correct warm/cold reset. This can be achieved > only when all .shutdown() callbacks have been called already for all > PCF8575's consumers. But devices_kset is not filled correctly now: > > devices_kset: Device61 4e000000.dmm > devices_kset: Device62 48070000.i2c > devices_kset: Device63 48072000.i2c > devices_kset: Device64 48060000.i2c > devices_kset: Device65 4809c000.mmc > ... > devices_kset: Device102 fixedregulator-sd > ... > devices_kset: Device181 0-0020 // PCF8575 > devices_kset: Device182 gpiochip496 > devices_kset: Device183 0-0021 // PCF8575 > devices_kset: Device184 gpiochip480 > > As can be seen from above .shutdown() callback for PCF8575 will be called > before its consumers, which, in turn means, that any changes of PCF8575 > GPIO's pins will be or unsafe or overwritten later by GPIO's consumers. > The problem can be solved if devices_kset list will be filled not only > according device creation order, but also according device's probing > order to track "supplier<-consumer" dependencies also. > > Hence, as a fix, lets add devices_kset_move_last(), > devices_kset_move_before(), devices_kset_move_after() and call them > from device_move() and also add call of devices_kset_move_last() in > really_probe(). After this change all entries in devices_kset will > be sorted according to device's creation ("parent<-child") and > probing ("supplier<-consumer") order. > > devices_kset after: > devices_kset: Device121 48070000.i2c > devices_kset: Device122 i2c-0 > ... > devices_kset: Device147 regulator.24 > devices_kset: Device148 0-0020 > devices_kset: Device149 gpiochip496 > devices_kset: Device150 0-0021 > devices_kset: Device151 gpiochip480 > devices_kset: Device152 0-0019 > ... > devices_kset: Device372 fixedregulator-sd > devices_kset: Device373 regulator.29 > devices_kset: Device374 4809c000.mmc > devices_kset: Device375 mmc0 > > [1] http://www.spinics.net/lists/linux-mmc/msg29825.html > > Cc: Sekhar Nori <nsekhar@ti.com> > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > > --- > drivers/base/base.h | 1 + > drivers/base/core.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > drivers/base/dd.c | 8 ++++++++ > 3 files changed, 58 insertions(+) > > diff --git a/drivers/base/base.h b/drivers/base/base.h > index fd3347d..8c5eb73 100644 > --- a/drivers/base/base.h > +++ b/drivers/base/base.h > @@ -134,6 +134,7 @@ extern int devres_release_all(struct device *dev); > > /* /sys/devices directory */ > extern struct kset *devices_kset; > +extern void devices_kset_move_last(struct device *dev); > > #if defined(CONFIG_MODULES) && defined(CONFIG_SYSFS) > extern void module_add_driver(struct module *mod, struct device_driver *drv); > diff --git a/drivers/base/core.c b/drivers/base/core.c > index dafae6d..fc5a558 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -534,6 +534,52 @@ static DEVICE_ATTR_RO(dev); > struct kset *devices_kset; > > /** > + * devices_kset_move_before - Move device in the devices_kset's list. > + * @deva: Device to move. > + * @devb: Device @deva should come before. > + */ > +static void devices_kset_move_before(struct device *deva, struct device *devb) > +{ > + if (!devices_kset) > + return; > + pr_debug("devices_kset: Moving %s before %s\n", > + dev_name(deva), dev_name(devb)); > + spin_lock(&devices_kset->list_lock); > + list_move_tail(&deva->kobj.entry, &devb->kobj.entry); > + spin_unlock(&devices_kset->list_lock); > +} > + > +/** > + * devices_kset_move_after - Move device in the devices_kset's list. > + * @deva: Device to move > + * @devb: Device @deva should come after. > + */ > +static void devices_kset_move_after(struct device *deva, struct device *devb) > +{ > + if (!devices_kset) > + return; > + pr_debug("devices_kset: Moving %s after %s\n", > + dev_name(deva), dev_name(devb)); > + spin_lock(&devices_kset->list_lock); > + list_move(&deva->kobj.entry, &devb->kobj.entry); > + spin_unlock(&devices_kset->list_lock); > +} > + > +/** > + * devices_kset_move_last - move the device to the end of devices_kset's list. > + * @dev: device to move > + */ > +void devices_kset_move_last(struct device *dev) > +{ > + if (!devices_kset) > + return; > + pr_debug("devices_kset: Moving %s to end of list\n", dev_name(dev)); > + spin_lock(&devices_kset->list_lock); > + list_move_tail(&dev->kobj.entry, &devices_kset->list); > + spin_unlock(&devices_kset->list_lock); > +} > + > +/** > * device_create_file - create sysfs attribute file for device. > * @dev: device. > * @attr: device attribute descriptor. > @@ -1923,12 +1969,15 @@ int device_move(struct device *dev, struct device *new_parent, > break; > case DPM_ORDER_DEV_AFTER_PARENT: > device_pm_move_after(dev, new_parent); > + devices_kset_move_after(dev, new_parent); > break; > case DPM_ORDER_PARENT_BEFORE_DEV: > device_pm_move_before(new_parent, dev); > + devices_kset_move_before(new_parent, dev); > break; > case DPM_ORDER_DEV_LAST: > device_pm_move_last(dev); > + devices_kset_move_last(dev); > break; > } > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c > index a638bbb..cc2b1d4 100644 > --- a/drivers/base/dd.c > +++ b/drivers/base/dd.c > @@ -304,6 +304,14 @@ static int really_probe(struct device *dev, struct device_driver *drv) > goto probe_failed; > } > > + /* > + * Ensure devices are listed in devices_kset in correct order > + * It's important to move Dev to the end of devices_kset before > + * calling .probe, because it could be recursive and parent Dev > + * should always go first > + */ > + devices_kset_move_last(dev); > + > if (dev->bus->probe) { > ret = dev->bus->probe(dev); > if (ret) > I loaded this change up with an Arm64 booting kernel and verified that some previously device drivers which were previously incorrectly ordered are now properly ordered. Reviewed-by: Rhyland Klein <rklein@nvidia.com> Tested-by: Rhyland Klein <rklein@nvidia.com>
diff --git a/drivers/base/base.h b/drivers/base/base.h index fd3347d..8c5eb73 100644 --- a/drivers/base/base.h +++ b/drivers/base/base.h @@ -134,6 +134,7 @@ extern int devres_release_all(struct device *dev); /* /sys/devices directory */ extern struct kset *devices_kset; +extern void devices_kset_move_last(struct device *dev); #if defined(CONFIG_MODULES) && defined(CONFIG_SYSFS) extern void module_add_driver(struct module *mod, struct device_driver *drv); diff --git a/drivers/base/core.c b/drivers/base/core.c index dafae6d..fc5a558 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -534,6 +534,52 @@ static DEVICE_ATTR_RO(dev); struct kset *devices_kset; /** + * devices_kset_move_before - Move device in the devices_kset's list. + * @deva: Device to move. + * @devb: Device @deva should come before. + */ +static void devices_kset_move_before(struct device *deva, struct device *devb) +{ + if (!devices_kset) + return; + pr_debug("devices_kset: Moving %s before %s\n", + dev_name(deva), dev_name(devb)); + spin_lock(&devices_kset->list_lock); + list_move_tail(&deva->kobj.entry, &devb->kobj.entry); + spin_unlock(&devices_kset->list_lock); +} + +/** + * devices_kset_move_after - Move device in the devices_kset's list. + * @deva: Device to move + * @devb: Device @deva should come after. + */ +static void devices_kset_move_after(struct device *deva, struct device *devb) +{ + if (!devices_kset) + return; + pr_debug("devices_kset: Moving %s after %s\n", + dev_name(deva), dev_name(devb)); + spin_lock(&devices_kset->list_lock); + list_move(&deva->kobj.entry, &devb->kobj.entry); + spin_unlock(&devices_kset->list_lock); +} + +/** + * devices_kset_move_last - move the device to the end of devices_kset's list. + * @dev: device to move + */ +void devices_kset_move_last(struct device *dev) +{ + if (!devices_kset) + return; + pr_debug("devices_kset: Moving %s to end of list\n", dev_name(dev)); + spin_lock(&devices_kset->list_lock); + list_move_tail(&dev->kobj.entry, &devices_kset->list); + spin_unlock(&devices_kset->list_lock); +} + +/** * device_create_file - create sysfs attribute file for device. * @dev: device. * @attr: device attribute descriptor. @@ -1923,12 +1969,15 @@ int device_move(struct device *dev, struct device *new_parent, break; case DPM_ORDER_DEV_AFTER_PARENT: device_pm_move_after(dev, new_parent); + devices_kset_move_after(dev, new_parent); break; case DPM_ORDER_PARENT_BEFORE_DEV: device_pm_move_before(new_parent, dev); + devices_kset_move_before(new_parent, dev); break; case DPM_ORDER_DEV_LAST: device_pm_move_last(dev); + devices_kset_move_last(dev); break; } diff --git a/drivers/base/dd.c b/drivers/base/dd.c index a638bbb..cc2b1d4 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -304,6 +304,14 @@ static int really_probe(struct device *dev, struct device_driver *drv) goto probe_failed; } + /* + * Ensure devices are listed in devices_kset in correct order + * It's important to move Dev to the end of devices_kset before + * calling .probe, because it could be recursive and parent Dev + * should always go first + */ + devices_kset_move_last(dev); + if (dev->bus->probe) { ret = dev->bus->probe(dev); if (ret)
Now device's shutdown sequence is performed in reverse order of their registration in devices_kset list and this sequence corresponds to the reverse device's creation order. So, devices_kset data tracks "parent<-child" device's dependencies only. Unfortunately, that's not enough and causes problems in case of implementing board's specific shutdown procedures. For example [1]: "DRA7XX_evm uses PCF8575 and one of the PCF output lines feeds to MMC/SD and this line should be driven high in order for the MMC/SD to be detected. This line is modelled as regulator and the hsmmc driver takes care of enabling and disabling it. In the case of 'reboot', during shutdown path as part of it's cleanup process the hsmmc driver disables this regulator. This makes MMC boot not functional." To handle this issue the .shutdown() callback could be implemented for PCF8575 device where corresponding GPIO pins will be configured to states, required for correct warm/cold reset. This can be achieved only when all .shutdown() callbacks have been called already for all PCF8575's consumers. But devices_kset is not filled correctly now: devices_kset: Device61 4e000000.dmm devices_kset: Device62 48070000.i2c devices_kset: Device63 48072000.i2c devices_kset: Device64 48060000.i2c devices_kset: Device65 4809c000.mmc ... devices_kset: Device102 fixedregulator-sd ... devices_kset: Device181 0-0020 // PCF8575 devices_kset: Device182 gpiochip496 devices_kset: Device183 0-0021 // PCF8575 devices_kset: Device184 gpiochip480 As can be seen from above .shutdown() callback for PCF8575 will be called before its consumers, which, in turn means, that any changes of PCF8575 GPIO's pins will be or unsafe or overwritten later by GPIO's consumers. The problem can be solved if devices_kset list will be filled not only according device creation order, but also according device's probing order to track "supplier<-consumer" dependencies also. Hence, as a fix, lets add devices_kset_move_last(), devices_kset_move_before(), devices_kset_move_after() and call them from device_move() and also add call of devices_kset_move_last() in really_probe(). After this change all entries in devices_kset will be sorted according to device's creation ("parent<-child") and probing ("supplier<-consumer") order. devices_kset after: devices_kset: Device121 48070000.i2c devices_kset: Device122 i2c-0 ... devices_kset: Device147 regulator.24 devices_kset: Device148 0-0020 devices_kset: Device149 gpiochip496 devices_kset: Device150 0-0021 devices_kset: Device151 gpiochip480 devices_kset: Device152 0-0019 ... devices_kset: Device372 fixedregulator-sd devices_kset: Device373 regulator.29 devices_kset: Device374 4809c000.mmc devices_kset: Device375 mmc0 [1] http://www.spinics.net/lists/linux-mmc/msg29825.html Cc: Sekhar Nori <nsekhar@ti.com> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> --- drivers/base/base.h | 1 + drivers/base/core.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ drivers/base/dd.c | 8 ++++++++ 3 files changed, 58 insertions(+)