* [PATCH 0/5] ACPI: scan: Fixes and cleanups related to dependencies list handling @ 2021-06-16 14:21 Rafael J. Wysocki 2021-06-16 14:21 ` [PATCH 1/5] ACPI: scan: Rearrange acpi_dev_get_first_consumer_dev_cb() Rafael J. Wysocki ` (4 more replies) 0 siblings, 5 replies; 24+ messages in thread From: Rafael J. Wysocki @ 2021-06-16 14:21 UTC (permalink / raw) To: Linux ACPI; +Cc: LKML, Hans de Goede Hi, The following patches address a few issues and do a few cleanups related to the handling of the _DEP dependencies list. Please refer to the patch changelogs for details. Thanks! ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/5] ACPI: scan: Rearrange acpi_dev_get_first_consumer_dev_cb() 2021-06-16 14:21 [PATCH 0/5] ACPI: scan: Fixes and cleanups related to dependencies list handling Rafael J. Wysocki @ 2021-06-16 14:21 ` Rafael J. Wysocki 2021-06-16 14:36 ` Hans de Goede 2021-06-16 14:22 ` [PATCH 2/5] ACPI: scan: Make acpi_walk_dep_device_list() Rafael J. Wysocki ` (3 subsequent siblings) 4 siblings, 1 reply; 24+ messages in thread From: Rafael J. Wysocki @ 2021-06-16 14:21 UTC (permalink / raw) To: Linux ACPI; +Cc: LKML, Hans de Goede From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Make acpi_dev_get_first_consumer_dev_cb() a bit more straightforward and rewrite the comment in it. No functional impact. Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> --- drivers/acpi/scan.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) Index: linux-pm/drivers/acpi/scan.c =================================================================== --- linux-pm.orig/drivers/acpi/scan.c +++ linux-pm/drivers/acpi/scan.c @@ -2107,13 +2107,12 @@ static int acpi_dev_get_first_consumer_d struct acpi_device *adev; adev = acpi_bus_get_acpi_device(dep->consumer); - if (!adev) - /* If we don't find an adev then we want to continue parsing */ - return 0; - - *(struct acpi_device **)data = adev; - - return 1; + if (adev) { + *(struct acpi_device **)data = adev; + return 1; + } + /* Continue parsing if the device object is not present. */ + return 0; } static int acpi_scan_clear_dep(struct acpi_dep_data *dep, void *data) ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/5] ACPI: scan: Rearrange acpi_dev_get_first_consumer_dev_cb() 2021-06-16 14:21 ` [PATCH 1/5] ACPI: scan: Rearrange acpi_dev_get_first_consumer_dev_cb() Rafael J. Wysocki @ 2021-06-16 14:36 ` Hans de Goede 0 siblings, 0 replies; 24+ messages in thread From: Hans de Goede @ 2021-06-16 14:36 UTC (permalink / raw) To: Rafael J. Wysocki, Linux ACPI; +Cc: LKML Hi, On 6/16/21 4:21 PM, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Make acpi_dev_get_first_consumer_dev_cb() a bit more straightforward > and rewrite the comment in it. > > No functional impact. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Thanks, patch looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > --- > drivers/acpi/scan.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > Index: linux-pm/drivers/acpi/scan.c > =================================================================== > --- linux-pm.orig/drivers/acpi/scan.c > +++ linux-pm/drivers/acpi/scan.c > @@ -2107,13 +2107,12 @@ static int acpi_dev_get_first_consumer_d > struct acpi_device *adev; > > adev = acpi_bus_get_acpi_device(dep->consumer); > - if (!adev) > - /* If we don't find an adev then we want to continue parsing */ > - return 0; > - > - *(struct acpi_device **)data = adev; > - > - return 1; > + if (adev) { > + *(struct acpi_device **)data = adev; > + return 1; > + } > + /* Continue parsing if the device object is not present. */ > + return 0; > } > > static int acpi_scan_clear_dep(struct acpi_dep_data *dep, void *data) > > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 2/5] ACPI: scan: Make acpi_walk_dep_device_list() 2021-06-16 14:21 [PATCH 0/5] ACPI: scan: Fixes and cleanups related to dependencies list handling Rafael J. Wysocki 2021-06-16 14:21 ` [PATCH 1/5] ACPI: scan: Rearrange acpi_dev_get_first_consumer_dev_cb() Rafael J. Wysocki @ 2021-06-16 14:22 ` Rafael J. Wysocki 2021-06-16 14:41 ` Hans de Goede 2021-06-16 14:23 ` [PATCH 3/5] ACPI: scan: Fix device object rescan in acpi_scan_clear_dep() Rafael J. Wysocki ` (2 subsequent siblings) 4 siblings, 1 reply; 24+ messages in thread From: Rafael J. Wysocki @ 2021-06-16 14:22 UTC (permalink / raw) To: Linux ACPI; +Cc: LKML, Hans de Goede From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Because acpi_walk_dep_device_list() is only called by the code in the file in which it is defined, make it static, drop the export of it and drop its header from acpi.h. Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> --- drivers/acpi/scan.c | 7 +++---- include/linux/acpi.h | 3 --- 2 files changed, 3 insertions(+), 7 deletions(-) Index: linux-pm/drivers/acpi/scan.c =================================================================== --- linux-pm.orig/drivers/acpi/scan.c +++ linux-pm/drivers/acpi/scan.c @@ -2145,9 +2145,9 @@ static int acpi_scan_clear_dep(struct ac * negative value is returned by the callback then the loop is broken and that * value is returned as the final error. */ -int acpi_walk_dep_device_list(acpi_handle handle, - int (*callback)(struct acpi_dep_data *, void *), - void *data) +static int acpi_walk_dep_device_list(acpi_handle handle, + int (*callback)(struct acpi_dep_data *, void *), + void *data) { struct acpi_dep_data *dep, *tmp; int ret = 0; @@ -2164,7 +2164,6 @@ int acpi_walk_dep_device_list(acpi_handl return ret > 0 ? 0 : ret; } -EXPORT_SYMBOL_GPL(acpi_walk_dep_device_list); /** * acpi_dev_clear_dependencies - Inform consumers that the device is now active Index: linux-pm/include/linux/acpi.h =================================================================== --- linux-pm.orig/include/linux/acpi.h +++ linux-pm/include/linux/acpi.h @@ -668,9 +668,6 @@ extern bool acpi_driver_match_device(str const struct device_driver *drv); int acpi_device_uevent_modalias(struct device *, struct kobj_uevent_env *); int acpi_device_modalias(struct device *, char *, int); -int acpi_walk_dep_device_list(acpi_handle handle, - int (*callback)(struct acpi_dep_data *, void *), - void *data); struct platform_device *acpi_create_platform_device(struct acpi_device *, struct property_entry *); ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/5] ACPI: scan: Make acpi_walk_dep_device_list() 2021-06-16 14:22 ` [PATCH 2/5] ACPI: scan: Make acpi_walk_dep_device_list() Rafael J. Wysocki @ 2021-06-16 14:41 ` Hans de Goede 2021-06-16 15:11 ` Rafael J. Wysocki 0 siblings, 1 reply; 24+ messages in thread From: Hans de Goede @ 2021-06-16 14:41 UTC (permalink / raw) To: Rafael J. Wysocki, Linux ACPI; +Cc: LKML Hi, On 6/16/21 4:22 PM, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Because acpi_walk_dep_device_list() is only called by the code in the > file in which it is defined, make it static, drop the export of it > and drop its header from acpi.h. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Actually, acpi_walk_dep_device_list() was split out as a helper function used to implement acpi_dev_clear_dependencies() because it will be used outside of drivers/acpi. Specifically it will be used in the new intel_skl_int3472 driver: https://patchwork.kernel.org/project/platform-driver-x86/patch/20210603224007.120560-6-djrscally@gmail.com/ Which I plan to merge into pdx86/for-next today, I've just merged your linux-pm/acpi-scan PULL-req which exports acpi_walk_dep_device_list() as preparation for this. Regards, Hans > --- > drivers/acpi/scan.c | 7 +++---- > include/linux/acpi.h | 3 --- > 2 files changed, 3 insertions(+), 7 deletions(-) > > Index: linux-pm/drivers/acpi/scan.c > =================================================================== > --- linux-pm.orig/drivers/acpi/scan.c > +++ linux-pm/drivers/acpi/scan.c > @@ -2145,9 +2145,9 @@ static int acpi_scan_clear_dep(struct ac > * negative value is returned by the callback then the loop is broken and that > * value is returned as the final error. > */ > -int acpi_walk_dep_device_list(acpi_handle handle, > - int (*callback)(struct acpi_dep_data *, void *), > - void *data) > +static int acpi_walk_dep_device_list(acpi_handle handle, > + int (*callback)(struct acpi_dep_data *, void *), > + void *data) > { > struct acpi_dep_data *dep, *tmp; > int ret = 0; > @@ -2164,7 +2164,6 @@ int acpi_walk_dep_device_list(acpi_handl > > return ret > 0 ? 0 : ret; > } > -EXPORT_SYMBOL_GPL(acpi_walk_dep_device_list); > > /** > * acpi_dev_clear_dependencies - Inform consumers that the device is now active > Index: linux-pm/include/linux/acpi.h > =================================================================== > --- linux-pm.orig/include/linux/acpi.h > +++ linux-pm/include/linux/acpi.h > @@ -668,9 +668,6 @@ extern bool acpi_driver_match_device(str > const struct device_driver *drv); > int acpi_device_uevent_modalias(struct device *, struct kobj_uevent_env *); > int acpi_device_modalias(struct device *, char *, int); > -int acpi_walk_dep_device_list(acpi_handle handle, > - int (*callback)(struct acpi_dep_data *, void *), > - void *data); > > struct platform_device *acpi_create_platform_device(struct acpi_device *, > struct property_entry *); > > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/5] ACPI: scan: Make acpi_walk_dep_device_list() 2021-06-16 14:41 ` Hans de Goede @ 2021-06-16 15:11 ` Rafael J. Wysocki 2021-06-16 15:25 ` Rafael J. Wysocki 2021-06-16 15:28 ` Hans de Goede 0 siblings, 2 replies; 24+ messages in thread From: Rafael J. Wysocki @ 2021-06-16 15:11 UTC (permalink / raw) To: Hans de Goede; +Cc: Rafael J. Wysocki, Linux ACPI, LKML On Wed, Jun 16, 2021 at 4:41 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 6/16/21 4:22 PM, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Because acpi_walk_dep_device_list() is only called by the code in the > > file in which it is defined, make it static, drop the export of it > > and drop its header from acpi.h. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Actually, acpi_walk_dep_device_list() was split out as a > helper function used to implement acpi_dev_clear_dependencies() > because it will be used outside of drivers/acpi. Not exactly. > Specifically it will be used in the new intel_skl_int3472 driver: > https://patchwork.kernel.org/project/platform-driver-x86/patch/20210603224007.120560-6-djrscally@gmail.com/ That driver will use acpi_dev_get_first_consumer_dev() which is based on acpi_walk_dep_device_list(), but still defined in drivers/acpi/scan.c. > Which I plan to merge into pdx86/for-next today, I've just merged > your linux-pm/acpi-scan PULL-req which exports acpi_walk_dep_device_list() > as preparation for this. No, the acpi_walk_dep_device_list() is a leftover there AFAICS. If it needs to be exported in the future, that still can be done. ATM the export isn't necessary. Thanks! ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/5] ACPI: scan: Make acpi_walk_dep_device_list() 2021-06-16 15:11 ` Rafael J. Wysocki @ 2021-06-16 15:25 ` Rafael J. Wysocki 2021-06-16 15:28 ` Hans de Goede 1 sibling, 0 replies; 24+ messages in thread From: Rafael J. Wysocki @ 2021-06-16 15:25 UTC (permalink / raw) To: Hans de Goede; +Cc: Rafael J. Wysocki, Linux ACPI, LKML On Wed, Jun 16, 2021 at 5:11 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Wed, Jun 16, 2021 at 4:41 PM Hans de Goede <hdegoede@redhat.com> wrote: > > > > Hi, > > > > On 6/16/21 4:22 PM, Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > Because acpi_walk_dep_device_list() is only called by the code in the > > > file in which it is defined, make it static, drop the export of it > > > and drop its header from acpi.h. > > > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Actually, acpi_walk_dep_device_list() was split out as a > > helper function used to implement acpi_dev_clear_dependencies() > > because it will be used outside of drivers/acpi. > > Not exactly. > > > Specifically it will be used in the new intel_skl_int3472 driver: > > https://patchwork.kernel.org/project/platform-driver-x86/patch/20210603224007.120560-6-djrscally@gmail.com/ > > That driver will use acpi_dev_get_first_consumer_dev() which is based > on acpi_walk_dep_device_list(), but still defined in > drivers/acpi/scan.c. > > > Which I plan to merge into pdx86/for-next today, I've just merged > > your linux-pm/acpi-scan PULL-req which exports acpi_walk_dep_device_list() > > as preparation for this. > > No, the acpi_walk_dep_device_list() is a leftover there AFAICS. I mean the export of it. > If it needs to be exported in the future, that still can be done. ATM > the export isn't necessary. > > Thanks! ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/5] ACPI: scan: Make acpi_walk_dep_device_list() 2021-06-16 15:11 ` Rafael J. Wysocki 2021-06-16 15:25 ` Rafael J. Wysocki @ 2021-06-16 15:28 ` Hans de Goede 1 sibling, 0 replies; 24+ messages in thread From: Hans de Goede @ 2021-06-16 15:28 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: Rafael J. Wysocki, Linux ACPI, LKML Hi, On 6/16/21 5:11 PM, Rafael J. Wysocki wrote: > On Wed, Jun 16, 2021 at 4:41 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi, >> >> On 6/16/21 4:22 PM, Rafael J. Wysocki wrote: >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>> >>> Because acpi_walk_dep_device_list() is only called by the code in the >>> file in which it is defined, make it static, drop the export of it >>> and drop its header from acpi.h. >>> >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> >> Actually, acpi_walk_dep_device_list() was split out as a >> helper function used to implement acpi_dev_clear_dependencies() >> because it will be used outside of drivers/acpi. > > Not exactly. > >> Specifically it will be used in the new intel_skl_int3472 driver: >> https://patchwork.kernel.org/project/platform-driver-x86/patch/20210603224007.120560-6-djrscally@gmail.com/ > > That driver will use acpi_dev_get_first_consumer_dev() which is based > on acpi_walk_dep_device_list(), but still defined in > drivers/acpi/scan.c. > >> Which I plan to merge into pdx86/for-next today, I've just merged >> your linux-pm/acpi-scan PULL-req which exports acpi_walk_dep_device_list() >> as preparation for this. > > No, the acpi_walk_dep_device_list() is a leftover there AFAICS. You are right, my bad. So with this resolved, this patch is fine too: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 3/5] ACPI: scan: Fix device object rescan in acpi_scan_clear_dep() 2021-06-16 14:21 [PATCH 0/5] ACPI: scan: Fixes and cleanups related to dependencies list handling Rafael J. Wysocki 2021-06-16 14:21 ` [PATCH 1/5] ACPI: scan: Rearrange acpi_dev_get_first_consumer_dev_cb() Rafael J. Wysocki 2021-06-16 14:22 ` [PATCH 2/5] ACPI: scan: Make acpi_walk_dep_device_list() Rafael J. Wysocki @ 2021-06-16 14:23 ` Rafael J. Wysocki 2021-06-16 14:48 ` Hans de Goede 2021-06-16 14:24 ` [PATCH 4/5] ACPI: scan: Reorganize acpi_device_add() Rafael J. Wysocki 2021-06-16 14:25 ` [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies Rafael J. Wysocki 4 siblings, 1 reply; 24+ messages in thread From: Rafael J. Wysocki @ 2021-06-16 14:23 UTC (permalink / raw) To: Linux ACPI; +Cc: LKML, Hans de Goede From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> In general, acpi_bus_attach() can only be run safely under acpi_scan_lock, but that lock cannot be acquired under acpi_dep_list_lock, so make acpi_scan_clear_dep() schedule deferred execution of acpi_bus_attach() under acpi_scan_lock instead of calling it directly. This also fixes a possible race between acpi_scan_clear_dep() and device removal that might cause a device object that went away to be accessed, because acpi_scan_clear_dep() is changed to acquire a reference on the consumer device object. Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> --- drivers/acpi/scan.c | 50 +++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 45 insertions(+), 5 deletions(-) Index: linux-pm/drivers/acpi/scan.c =================================================================== --- linux-pm.orig/drivers/acpi/scan.c +++ linux-pm/drivers/acpi/scan.c @@ -2115,16 +2115,56 @@ static int acpi_dev_get_first_consumer_d return 0; } -static int acpi_scan_clear_dep(struct acpi_dep_data *dep, void *data) -{ +struct acpi_scan_clear_dep_work { + struct work_struct work; struct acpi_device *adev; +}; + +static void acpi_scan_clear_dep_fn(struct work_struct *work) +{ + struct acpi_scan_clear_dep_work *cdw; + + cdw = container_of(work, struct acpi_scan_clear_dep_work, work); - acpi_bus_get_device(dep->consumer, &adev); + acpi_scan_lock_acquire(); + acpi_bus_attach(cdw->adev, true); + acpi_scan_lock_release(); + + acpi_dev_put(cdw->adev); + kfree(cdw); +} + +static bool acpi_scan_clear_dep_queue(struct acpi_device *adev) +{ + struct acpi_scan_clear_dep_work *cdw; + + if (adev->dep_unmet) + return false; + + cdw = kmalloc(sizeof(*cdw), GFP_KERNEL); + if (!cdw) + return false; + + cdw->adev = adev; + INIT_WORK(&cdw->work, acpi_scan_clear_dep_fn); + /* + * Since the work function may block on the lock until the entire + * initial enumeration of devices is complete, put it into the unbound + * workqueue. + */ + queue_work(system_unbound_wq, &cdw->work); + + return true; +} + +static int acpi_scan_clear_dep(struct acpi_dep_data *dep, void *data) +{ + struct acpi_device *adev = acpi_bus_get_acpi_device(dep->consumer); if (adev) { adev->dep_unmet--; - if (!adev->dep_unmet) - acpi_bus_attach(adev, true); + if (!acpi_scan_clear_dep_queue(adev)) + acpi_dev_put(adev); } list_del(&dep->node); ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 3/5] ACPI: scan: Fix device object rescan in acpi_scan_clear_dep() 2021-06-16 14:23 ` [PATCH 3/5] ACPI: scan: Fix device object rescan in acpi_scan_clear_dep() Rafael J. Wysocki @ 2021-06-16 14:48 ` Hans de Goede 2021-06-16 15:12 ` Rafael J. Wysocki 0 siblings, 1 reply; 24+ messages in thread From: Hans de Goede @ 2021-06-16 14:48 UTC (permalink / raw) To: Rafael J. Wysocki, Linux ACPI; +Cc: LKML Hi, On 6/16/21 4:23 PM, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > In general, acpi_bus_attach() can only be run safely under > acpi_scan_lock, but that lock cannot be acquired under > acpi_dep_list_lock, so make acpi_scan_clear_dep() schedule deferred > execution of acpi_bus_attach() under acpi_scan_lock instead of > calling it directly. > > This also fixes a possible race between acpi_scan_clear_dep() and > device removal that might cause a device object that went away to > be accessed, because acpi_scan_clear_dep() is changed to acquire > a reference on the consumer device object. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/acpi/scan.c | 50 +++++++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 45 insertions(+), 5 deletions(-) > > Index: linux-pm/drivers/acpi/scan.c > =================================================================== > --- linux-pm.orig/drivers/acpi/scan.c > +++ linux-pm/drivers/acpi/scan.c > @@ -2115,16 +2115,56 @@ static int acpi_dev_get_first_consumer_d > return 0; > } > > -static int acpi_scan_clear_dep(struct acpi_dep_data *dep, void *data) > -{ > +struct acpi_scan_clear_dep_work { > + struct work_struct work; > struct acpi_device *adev; > +}; > + > +static void acpi_scan_clear_dep_fn(struct work_struct *work) > +{ > + struct acpi_scan_clear_dep_work *cdw; > + > + cdw = container_of(work, struct acpi_scan_clear_dep_work, work); > > - acpi_bus_get_device(dep->consumer, &adev); > + acpi_scan_lock_acquire(); > + acpi_bus_attach(cdw->adev, true); > + acpi_scan_lock_release(); > + > + acpi_dev_put(cdw->adev); > + kfree(cdw); > +} > + > +static bool acpi_scan_clear_dep_queue(struct acpi_device *adev) > +{ > + struct acpi_scan_clear_dep_work *cdw; > + > + if (adev->dep_unmet) > + return false; > + > + cdw = kmalloc(sizeof(*cdw), GFP_KERNEL); > + if (!cdw) > + return false; > + > + cdw->adev = adev; > + INIT_WORK(&cdw->work, acpi_scan_clear_dep_fn); > + /* > + * Since the work function may block on the lock until the entire > + * initial enumeration of devices is complete, put it into the unbound > + * workqueue. > + */ > + queue_work(system_unbound_wq, &cdw->work); Hmm, I'm a bit worried about this. Even with the system_unbound_wq some code may expect at least some progress being made with processing works during the initial enumeration. OTOH this does run pretty early on. Still I wonder if it would not be better to create + use our own workqueue for this ? I guess we can always do this if we run into issues later... With that said / otherwise the patch looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > + > + return true; > +} > + > +static int acpi_scan_clear_dep(struct acpi_dep_data *dep, void *data) > +{ > + struct acpi_device *adev = acpi_bus_get_acpi_device(dep->consumer); > > if (adev) { > adev->dep_unmet--; > - if (!adev->dep_unmet) > - acpi_bus_attach(adev, true); > + if (!acpi_scan_clear_dep_queue(adev)) > + acpi_dev_put(adev); > } > > list_del(&dep->node); > > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 3/5] ACPI: scan: Fix device object rescan in acpi_scan_clear_dep() 2021-06-16 14:48 ` Hans de Goede @ 2021-06-16 15:12 ` Rafael J. Wysocki 0 siblings, 0 replies; 24+ messages in thread From: Rafael J. Wysocki @ 2021-06-16 15:12 UTC (permalink / raw) To: Hans de Goede; +Cc: Rafael J. Wysocki, Linux ACPI, LKML On Wed, Jun 16, 2021 at 4:48 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 6/16/21 4:23 PM, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > In general, acpi_bus_attach() can only be run safely under > > acpi_scan_lock, but that lock cannot be acquired under > > acpi_dep_list_lock, so make acpi_scan_clear_dep() schedule deferred > > execution of acpi_bus_attach() under acpi_scan_lock instead of > > calling it directly. > > > > This also fixes a possible race between acpi_scan_clear_dep() and > > device removal that might cause a device object that went away to > > be accessed, because acpi_scan_clear_dep() is changed to acquire > > a reference on the consumer device object. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > drivers/acpi/scan.c | 50 +++++++++++++++++++++++++++++++++++++++++++++----- > > 1 file changed, 45 insertions(+), 5 deletions(-) > > > > Index: linux-pm/drivers/acpi/scan.c > > =================================================================== > > --- linux-pm.orig/drivers/acpi/scan.c > > +++ linux-pm/drivers/acpi/scan.c > > @@ -2115,16 +2115,56 @@ static int acpi_dev_get_first_consumer_d > > return 0; > > } > > > > -static int acpi_scan_clear_dep(struct acpi_dep_data *dep, void *data) > > -{ > > +struct acpi_scan_clear_dep_work { > > + struct work_struct work; > > struct acpi_device *adev; > > +}; > > + > > +static void acpi_scan_clear_dep_fn(struct work_struct *work) > > +{ > > + struct acpi_scan_clear_dep_work *cdw; > > + > > + cdw = container_of(work, struct acpi_scan_clear_dep_work, work); > > > > - acpi_bus_get_device(dep->consumer, &adev); > > + acpi_scan_lock_acquire(); > > + acpi_bus_attach(cdw->adev, true); > > + acpi_scan_lock_release(); > > + > > + acpi_dev_put(cdw->adev); > > + kfree(cdw); > > +} > > + > > +static bool acpi_scan_clear_dep_queue(struct acpi_device *adev) > > +{ > > + struct acpi_scan_clear_dep_work *cdw; > > + > > + if (adev->dep_unmet) > > + return false; > > + > > + cdw = kmalloc(sizeof(*cdw), GFP_KERNEL); > > + if (!cdw) > > + return false; > > + > > + cdw->adev = adev; > > + INIT_WORK(&cdw->work, acpi_scan_clear_dep_fn); > > + /* > > + * Since the work function may block on the lock until the entire > > + * initial enumeration of devices is complete, put it into the unbound > > + * workqueue. > > + */ > > + queue_work(system_unbound_wq, &cdw->work); > > Hmm, I'm a bit worried about this. Even with the system_unbound_wq > some code may expect at least some progress being made with processing > works during the initial enumeration. OTOH this does run pretty early on. > > Still I wonder if it would not be better to create + use our own workqueue > for this ? > > I guess we can always do this if we run into issues later... Exactly my thought. > With that said / otherwise the patch looks good to me: > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> Thanks! ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 4/5] ACPI: scan: Reorganize acpi_device_add() 2021-06-16 14:21 [PATCH 0/5] ACPI: scan: Fixes and cleanups related to dependencies list handling Rafael J. Wysocki ` (2 preceding siblings ...) 2021-06-16 14:23 ` [PATCH 3/5] ACPI: scan: Fix device object rescan in acpi_scan_clear_dep() Rafael J. Wysocki @ 2021-06-16 14:24 ` Rafael J. Wysocki 2021-06-16 14:49 ` Hans de Goede 2021-06-16 14:25 ` [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies Rafael J. Wysocki 4 siblings, 1 reply; 24+ messages in thread From: Rafael J. Wysocki @ 2021-06-16 14:24 UTC (permalink / raw) To: Linux ACPI; +Cc: LKML, Hans de Goede From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Move the invocation of acpi_attach_data() in acpi_device_add() into a separate function. No intentional functional impact. Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> --- drivers/acpi/scan.c | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) Index: linux-pm/drivers/acpi/scan.c =================================================================== --- linux-pm.orig/drivers/acpi/scan.c +++ linux-pm/drivers/acpi/scan.c @@ -640,23 +640,32 @@ static int acpi_device_set_name(struct a return 0; } +static int acpi_tie_acpi_dev(struct acpi_device *adev) +{ + acpi_handle handle = adev->handle; + acpi_status status; + + if (!handle) + return 0; + + status = acpi_attach_data(handle, acpi_scan_drop_device, adev); + if (ACPI_FAILURE(status)) { + acpi_handle_err(handle, "Unable to attach device data\n"); + return -ENODEV; + } + + return 0; +} + int acpi_device_add(struct acpi_device *device, void (*release)(struct device *)) { struct acpi_device_bus_id *acpi_device_bus_id; int result; - if (device->handle) { - acpi_status status; - - status = acpi_attach_data(device->handle, acpi_scan_drop_device, - device); - if (ACPI_FAILURE(status)) { - acpi_handle_err(device->handle, - "Unable to attach device data\n"); - return -ENODEV; - } - } + result = acpi_tie_acpi_dev(device); + if (result) + return result; /* * Linkage ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/5] ACPI: scan: Reorganize acpi_device_add() 2021-06-16 14:24 ` [PATCH 4/5] ACPI: scan: Reorganize acpi_device_add() Rafael J. Wysocki @ 2021-06-16 14:49 ` Hans de Goede 0 siblings, 0 replies; 24+ messages in thread From: Hans de Goede @ 2021-06-16 14:49 UTC (permalink / raw) To: Rafael J. Wysocki, Linux ACPI; +Cc: LKML Hi, On 6/16/21 4:24 PM, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Move the invocation of acpi_attach_data() in acpi_device_add() > into a separate function. > > No intentional functional impact. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Thanks, patch looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > --- > drivers/acpi/scan.c | 31 ++++++++++++++++++++----------- > 1 file changed, 20 insertions(+), 11 deletions(-) > > Index: linux-pm/drivers/acpi/scan.c > =================================================================== > --- linux-pm.orig/drivers/acpi/scan.c > +++ linux-pm/drivers/acpi/scan.c > @@ -640,23 +640,32 @@ static int acpi_device_set_name(struct a > return 0; > } > > +static int acpi_tie_acpi_dev(struct acpi_device *adev) > +{ > + acpi_handle handle = adev->handle; > + acpi_status status; > + > + if (!handle) > + return 0; > + > + status = acpi_attach_data(handle, acpi_scan_drop_device, adev); > + if (ACPI_FAILURE(status)) { > + acpi_handle_err(handle, "Unable to attach device data\n"); > + return -ENODEV; > + } > + > + return 0; > +} > + > int acpi_device_add(struct acpi_device *device, > void (*release)(struct device *)) > { > struct acpi_device_bus_id *acpi_device_bus_id; > int result; > > - if (device->handle) { > - acpi_status status; > - > - status = acpi_attach_data(device->handle, acpi_scan_drop_device, > - device); > - if (ACPI_FAILURE(status)) { > - acpi_handle_err(device->handle, > - "Unable to attach device data\n"); > - return -ENODEV; > - } > - } > + result = acpi_tie_acpi_dev(device); > + if (result) > + return result; > > /* > * Linkage > > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies 2021-06-16 14:21 [PATCH 0/5] ACPI: scan: Fixes and cleanups related to dependencies list handling Rafael J. Wysocki ` (3 preceding siblings ...) 2021-06-16 14:24 ` [PATCH 4/5] ACPI: scan: Reorganize acpi_device_add() Rafael J. Wysocki @ 2021-06-16 14:25 ` Rafael J. Wysocki 2021-06-16 14:55 ` Hans de Goede ` (4 more replies) 4 siblings, 5 replies; 24+ messages in thread From: Rafael J. Wysocki @ 2021-06-16 14:25 UTC (permalink / raw) To: Linux ACPI; +Cc: LKML, Hans de Goede From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> If acpi_add_single_object() runs concurrently with respect to acpi_scan_clear_dep() which deletes a dependencies list entry where the device being added is the consumer, the device's dep_unmet counter may not be updated to reflect that change. Namely, if the dependencies list entry is deleted right after calling acpi_scan_dep_init() and before calling acpi_device_add(), acpi_scan_clear_dep() will not find the device object corresponding to the consumer device ACPI handle and it will not update its dep_unmet counter to reflect the deletion of the list entry. Consequently, the dep_unmet counter of the device will never become zero going forward which may prevent it from being completely enumerated. To address this problem, modify acpi_add_single_object() to run acpi_tie_acpi_dev(), to attach the ACPI device object created by it to the corresponding ACPI namespace node, under acpi_dep_list_lock along with acpi_scan_dep_init() whenever the latter is called. Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> --- drivers/acpi/scan.c | 46 +++++++++++++++++++++++++++++++++------------- 1 file changed, 33 insertions(+), 13 deletions(-) Index: linux-pm/drivers/acpi/scan.c =================================================================== --- linux-pm.orig/drivers/acpi/scan.c +++ linux-pm/drivers/acpi/scan.c @@ -657,16 +657,12 @@ static int acpi_tie_acpi_dev(struct acpi return 0; } -int acpi_device_add(struct acpi_device *device, - void (*release)(struct device *)) +int __acpi_device_add(struct acpi_device *device, + void (*release)(struct device *)) { struct acpi_device_bus_id *acpi_device_bus_id; int result; - result = acpi_tie_acpi_dev(device); - if (result) - return result; - /* * Linkage * ------- @@ -755,6 +751,17 @@ err_unlock: return result; } +int acpi_device_add(struct acpi_device *adev, void (*release)(struct device *)) +{ + int ret; + + ret = acpi_tie_acpi_dev(adev); + if (ret) + return ret; + + return __acpi_device_add(adev, release); +} + /* -------------------------------------------------------------------------- Device Enumeration -------------------------------------------------------------------------- */ @@ -1681,14 +1688,10 @@ static void acpi_scan_dep_init(struct ac { struct acpi_dep_data *dep; - mutex_lock(&acpi_dep_list_lock); - list_for_each_entry(dep, &acpi_dep_list, node) { if (dep->consumer == adev->handle) adev->dep_unmet++; } - - mutex_unlock(&acpi_dep_list_lock); } void acpi_device_add_finalize(struct acpi_device *device) @@ -1707,6 +1710,7 @@ static int acpi_add_single_object(struct acpi_handle handle, int type, bool dep_init) { struct acpi_device *device; + bool release_dep_lock = false; int result; device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL); @@ -1720,16 +1724,32 @@ static int acpi_add_single_object(struct * this must be done before the get power-/wakeup_dev-flags calls. */ if (type == ACPI_BUS_TYPE_DEVICE || type == ACPI_BUS_TYPE_PROCESSOR) { - if (dep_init) + if (dep_init) { + mutex_lock(&acpi_dep_list_lock); + /* + * Hold the lock until the acpi_tie_acpi_dev() call + * below to prevent concurrent acpi_scan_clear_dep() + * from deleting a dependency list entry without + * updating dep_unmet for the device. + */ + release_dep_lock = true; acpi_scan_dep_init(device); - + } acpi_scan_init_status(device); } acpi_bus_get_power_flags(device); acpi_bus_get_wakeup_device_flags(device); - result = acpi_device_add(device, acpi_device_release); + result = acpi_tie_acpi_dev(device); + + if (release_dep_lock) + mutex_unlock(&acpi_dep_list_lock); + + if (result) + return result; + + result = __acpi_device_add(device, acpi_device_release); if (result) { acpi_device_release(&device->dev); return result; ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies 2021-06-16 14:25 ` [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies Rafael J. Wysocki @ 2021-06-16 14:55 ` Hans de Goede 2021-06-16 15:19 ` Rafael J. Wysocki 2021-06-16 18:00 ` [PATCH v2 " Rafael J. Wysocki ` (3 subsequent siblings) 4 siblings, 1 reply; 24+ messages in thread From: Hans de Goede @ 2021-06-16 14:55 UTC (permalink / raw) To: Rafael J. Wysocki, Linux ACPI; +Cc: LKML Hi, On 6/16/21 4:25 PM, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > If acpi_add_single_object() runs concurrently with respect to > acpi_scan_clear_dep() which deletes a dependencies list entry where > the device being added is the consumer, the device's dep_unmet > counter may not be updated to reflect that change. > > Namely, if the dependencies list entry is deleted right after > calling acpi_scan_dep_init() and before calling acpi_device_add(), > acpi_scan_clear_dep() will not find the device object corresponding > to the consumer device ACPI handle and it will not update its > dep_unmet counter to reflect the deletion of the list entry. > Consequently, the dep_unmet counter of the device will never > become zero going forward which may prevent it from being > completely enumerated. > > To address this problem, modify acpi_add_single_object() to run > acpi_tie_acpi_dev(), to attach the ACPI device object created by it > to the corresponding ACPI namespace node, under acpi_dep_list_lock > along with acpi_scan_dep_init() whenever the latter is called. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/acpi/scan.c | 46 +++++++++++++++++++++++++++++++++------------- > 1 file changed, 33 insertions(+), 13 deletions(-) > > Index: linux-pm/drivers/acpi/scan.c > =================================================================== > --- linux-pm.orig/drivers/acpi/scan.c > +++ linux-pm/drivers/acpi/scan.c > @@ -657,16 +657,12 @@ static int acpi_tie_acpi_dev(struct acpi > return 0; > } > > -int acpi_device_add(struct acpi_device *device, > - void (*release)(struct device *)) > +int __acpi_device_add(struct acpi_device *device, > + void (*release)(struct device *)) > { > struct acpi_device_bus_id *acpi_device_bus_id; > int result; > > - result = acpi_tie_acpi_dev(device); > - if (result) > - return result; > - > /* > * Linkage > * ------- > @@ -755,6 +751,17 @@ err_unlock: > return result; > } > > +int acpi_device_add(struct acpi_device *adev, void (*release)(struct device *)) > +{ > + int ret; > + > + ret = acpi_tie_acpi_dev(adev); > + if (ret) > + return ret; > + > + return __acpi_device_add(adev, release); > +} > + > /* -------------------------------------------------------------------------- > Device Enumeration > -------------------------------------------------------------------------- */ > @@ -1681,14 +1688,10 @@ static void acpi_scan_dep_init(struct ac > { > struct acpi_dep_data *dep; > > - mutex_lock(&acpi_dep_list_lock); > - > list_for_each_entry(dep, &acpi_dep_list, node) { > if (dep->consumer == adev->handle) > adev->dep_unmet++; > } > - > - mutex_unlock(&acpi_dep_list_lock); > } > > void acpi_device_add_finalize(struct acpi_device *device) > @@ -1707,6 +1710,7 @@ static int acpi_add_single_object(struct > acpi_handle handle, int type, bool dep_init) > { > struct acpi_device *device; > + bool release_dep_lock = false; > int result; > > device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL); > @@ -1720,16 +1724,32 @@ static int acpi_add_single_object(struct > * this must be done before the get power-/wakeup_dev-flags calls. > */ > if (type == ACPI_BUS_TYPE_DEVICE || type == ACPI_BUS_TYPE_PROCESSOR) { > - if (dep_init) > + if (dep_init) { > + mutex_lock(&acpi_dep_list_lock); > + /* > + * Hold the lock until the acpi_tie_acpi_dev() call > + * below to prevent concurrent acpi_scan_clear_dep() > + * from deleting a dependency list entry without > + * updating dep_unmet for the device. > + */ > + release_dep_lock = true; > acpi_scan_dep_init(device); > - > + } > acpi_scan_init_status(device); > } > > acpi_bus_get_power_flags(device); > acpi_bus_get_wakeup_device_flags(device); > > - result = acpi_device_add(device, acpi_device_release); > + result = acpi_tie_acpi_dev(device); > + > + if (release_dep_lock) > + mutex_unlock(&acpi_dep_list_lock); > + > + if (result) AFAICT you are missing a "acpi_device_release(&device->dev);" call in the error-exit path here, causing a mem-leak. Otherwise this looks good, with the above fixed this is: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > + return result; > + > + result = __acpi_device_add(device, acpi_device_release); > if (result) { > acpi_device_release(&device->dev); > return result; > > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies 2021-06-16 14:55 ` Hans de Goede @ 2021-06-16 15:19 ` Rafael J. Wysocki 0 siblings, 0 replies; 24+ messages in thread From: Rafael J. Wysocki @ 2021-06-16 15:19 UTC (permalink / raw) To: Hans de Goede; +Cc: Rafael J. Wysocki, Linux ACPI, LKML On Wed, Jun 16, 2021 at 4:55 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 6/16/21 4:25 PM, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > If acpi_add_single_object() runs concurrently with respect to > > acpi_scan_clear_dep() which deletes a dependencies list entry where > > the device being added is the consumer, the device's dep_unmet > > counter may not be updated to reflect that change. > > > > Namely, if the dependencies list entry is deleted right after > > calling acpi_scan_dep_init() and before calling acpi_device_add(), > > acpi_scan_clear_dep() will not find the device object corresponding > > to the consumer device ACPI handle and it will not update its > > dep_unmet counter to reflect the deletion of the list entry. > > Consequently, the dep_unmet counter of the device will never > > become zero going forward which may prevent it from being > > completely enumerated. > > > > To address this problem, modify acpi_add_single_object() to run > > acpi_tie_acpi_dev(), to attach the ACPI device object created by it > > to the corresponding ACPI namespace node, under acpi_dep_list_lock > > along with acpi_scan_dep_init() whenever the latter is called. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > drivers/acpi/scan.c | 46 +++++++++++++++++++++++++++++++++------------- > > 1 file changed, 33 insertions(+), 13 deletions(-) > > > > Index: linux-pm/drivers/acpi/scan.c > > =================================================================== > > --- linux-pm.orig/drivers/acpi/scan.c > > +++ linux-pm/drivers/acpi/scan.c > > @@ -657,16 +657,12 @@ static int acpi_tie_acpi_dev(struct acpi > > return 0; > > } > > > > -int acpi_device_add(struct acpi_device *device, > > - void (*release)(struct device *)) > > +int __acpi_device_add(struct acpi_device *device, > > + void (*release)(struct device *)) > > { > > struct acpi_device_bus_id *acpi_device_bus_id; > > int result; > > > > - result = acpi_tie_acpi_dev(device); > > - if (result) > > - return result; > > - > > /* > > * Linkage > > * ------- > > @@ -755,6 +751,17 @@ err_unlock: > > return result; > > } > > > > +int acpi_device_add(struct acpi_device *adev, void (*release)(struct device *)) > > +{ > > + int ret; > > + > > + ret = acpi_tie_acpi_dev(adev); > > + if (ret) > > + return ret; > > + > > + return __acpi_device_add(adev, release); > > +} > > + > > /* -------------------------------------------------------------------------- > > Device Enumeration > > -------------------------------------------------------------------------- */ > > @@ -1681,14 +1688,10 @@ static void acpi_scan_dep_init(struct ac > > { > > struct acpi_dep_data *dep; > > > > - mutex_lock(&acpi_dep_list_lock); > > - > > list_for_each_entry(dep, &acpi_dep_list, node) { > > if (dep->consumer == adev->handle) > > adev->dep_unmet++; > > } > > - > > - mutex_unlock(&acpi_dep_list_lock); > > } > > > > void acpi_device_add_finalize(struct acpi_device *device) > > @@ -1707,6 +1710,7 @@ static int acpi_add_single_object(struct > > acpi_handle handle, int type, bool dep_init) > > { > > struct acpi_device *device; > > + bool release_dep_lock = false; > > int result; > > > > device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL); > > @@ -1720,16 +1724,32 @@ static int acpi_add_single_object(struct > > * this must be done before the get power-/wakeup_dev-flags calls. > > */ > > if (type == ACPI_BUS_TYPE_DEVICE || type == ACPI_BUS_TYPE_PROCESSOR) { > > - if (dep_init) > > + if (dep_init) { > > + mutex_lock(&acpi_dep_list_lock); > > + /* > > + * Hold the lock until the acpi_tie_acpi_dev() call > > + * below to prevent concurrent acpi_scan_clear_dep() > > + * from deleting a dependency list entry without > > + * updating dep_unmet for the device. > > + */ > > + release_dep_lock = true; > > acpi_scan_dep_init(device); > > - > > + } > > acpi_scan_init_status(device); > > } > > > > acpi_bus_get_power_flags(device); > > acpi_bus_get_wakeup_device_flags(device); > > > > - result = acpi_device_add(device, acpi_device_release); > > + result = acpi_tie_acpi_dev(device); > > + > > + if (release_dep_lock) > > + mutex_unlock(&acpi_dep_list_lock); > > + > > + if (result) > > AFAICT you are missing a "acpi_device_release(&device->dev);" > call in the error-exit path here, causing a mem-leak. Indeed. I'll send a v2 of this patch alone to fix this issue. > Otherwise this looks good, with the above fixed this is: > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> Thanks! ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 5/5] ACPI: scan: Fix race related to dropping dependencies 2021-06-16 14:25 ` [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies Rafael J. Wysocki 2021-06-16 14:55 ` Hans de Goede @ 2021-06-16 18:00 ` Rafael J. Wysocki 2021-06-16 18:04 ` Hans de Goede 2021-06-17 0:25 ` kernel test robot ` (2 subsequent siblings) 4 siblings, 1 reply; 24+ messages in thread From: Rafael J. Wysocki @ 2021-06-16 18:00 UTC (permalink / raw) To: Linux ACPI; +Cc: LKML, Hans de Goede From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> If acpi_add_single_object() runs concurrently with respect to acpi_scan_clear_dep() which deletes a dependencies list entry where the device being added is the consumer, the device's dep_unmet counter may not be updated to reflect that change. Namely, if the dependencies list entry is deleted right after calling acpi_scan_dep_init() and before calling acpi_device_add(), acpi_scan_clear_dep() will not find the device object corresponding to the consumer device ACPI handle and it will not update its dep_unmet counter to reflect the deletion of the list entry. Consequently, the dep_unmet counter of the device will never become zero going forward which may prevent it from being completely enumerated. To address this problem, modify acpi_add_single_object() to run acpi_tie_acpi_dev(), to attach the ACPI device object created by it to the corresponding ACPI namespace node, under acpi_dep_list_lock along with acpi_scan_dep_init() whenever the latter is called. Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Reviewed-by: Hans de Goede <hdegoede@redhat.com> --- -> v2: * Fix memory leak spotted by Hans. * Add the R-by tag from Hans. --- drivers/acpi/scan.c | 50 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 32 insertions(+), 18 deletions(-) Index: linux-pm/drivers/acpi/scan.c =================================================================== --- linux-pm.orig/drivers/acpi/scan.c +++ linux-pm/drivers/acpi/scan.c @@ -657,16 +652,12 @@ static int acpi_tie_acpi_dev(struct acpi return 0; } -int acpi_device_add(struct acpi_device *device, - void (*release)(struct device *)) +int __acpi_device_add(struct acpi_device *device, + void (*release)(struct device *)) { struct acpi_device_bus_id *acpi_device_bus_id; int result; - result = acpi_tie_acpi_dev(device); - if (result) - return result; - /* * Linkage * ------- @@ -755,6 +746,17 @@ err_unlock: return result; } +int acpi_device_add(struct acpi_device *adev, void (*release)(struct device *)) +{ + int ret; + + ret = acpi_tie_acpi_dev(adev); + if (ret) + return ret; + + return __acpi_device_add(adev, release); +} + /* -------------------------------------------------------------------------- Device Enumeration -------------------------------------------------------------------------- */ @@ -1681,14 +1683,10 @@ static void acpi_scan_dep_init(struct ac { struct acpi_dep_data *dep; - mutex_lock(&acpi_dep_list_lock); - list_for_each_entry(dep, &acpi_dep_list, node) { if (dep->consumer == adev->handle) adev->dep_unmet++; } - - mutex_unlock(&acpi_dep_list_lock); } void acpi_device_add_finalize(struct acpi_device *device) @@ -1707,6 +1705,7 @@ static int acpi_add_single_object(struct acpi_handle handle, int type, bool dep_init) { struct acpi_device *device; + bool release_dep_lock = false; int result; device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL); @@ -1720,16 +1719,31 @@ static int acpi_add_single_object(struct * this must be done before the get power-/wakeup_dev-flags calls. */ if (type == ACPI_BUS_TYPE_DEVICE || type == ACPI_BUS_TYPE_PROCESSOR) { - if (dep_init) + if (dep_init) { + mutex_lock(&acpi_dep_list_lock); + /* + * Hold the lock until the acpi_tie_acpi_dev() call + * below to prevent concurrent acpi_scan_clear_dep() + * from deleting a dependency list entry without + * updating dep_unmet for the device. + */ + release_dep_lock = true; acpi_scan_dep_init(device); - + } acpi_scan_init_status(device); } acpi_bus_get_power_flags(device); acpi_bus_get_wakeup_device_flags(device); - result = acpi_device_add(device, acpi_device_release); + result = acpi_tie_acpi_dev(device); + + if (release_dep_lock) + mutex_unlock(&acpi_dep_list_lock); + + if (!result) + result = __acpi_device_add(device, acpi_device_release); + if (result) { acpi_device_release(&device->dev); return result; ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 5/5] ACPI: scan: Fix race related to dropping dependencies 2021-06-16 18:00 ` [PATCH v2 " Rafael J. Wysocki @ 2021-06-16 18:04 ` Hans de Goede 0 siblings, 0 replies; 24+ messages in thread From: Hans de Goede @ 2021-06-16 18:04 UTC (permalink / raw) To: Rafael J. Wysocki, Linux ACPI; +Cc: LKML Hi, On 6/16/21 8:00 PM, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > If acpi_add_single_object() runs concurrently with respect to > acpi_scan_clear_dep() which deletes a dependencies list entry where > the device being added is the consumer, the device's dep_unmet > counter may not be updated to reflect that change. > > Namely, if the dependencies list entry is deleted right after > calling acpi_scan_dep_init() and before calling acpi_device_add(), > acpi_scan_clear_dep() will not find the device object corresponding > to the consumer device ACPI handle and it will not update its > dep_unmet counter to reflect the deletion of the list entry. > Consequently, the dep_unmet counter of the device will never > become zero going forward which may prevent it from being > completely enumerated. > > To address this problem, modify acpi_add_single_object() to run > acpi_tie_acpi_dev(), to attach the ACPI device object created by it > to the corresponding ACPI namespace node, under acpi_dep_list_lock > along with acpi_scan_dep_init() whenever the latter is called. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Reviewed-by: Hans de Goede <hdegoede@redhat.com> FWIW this looks good to me now. Regards, Hans > --- > > -> v2: > * Fix memory leak spotted by Hans. > * Add the R-by tag from Hans. > > --- > drivers/acpi/scan.c | 50 ++++++++++++++++++++++++++++++++------------------ > 1 file changed, 32 insertions(+), 18 deletions(-) > > Index: linux-pm/drivers/acpi/scan.c > =================================================================== > --- linux-pm.orig/drivers/acpi/scan.c > +++ linux-pm/drivers/acpi/scan.c > @@ -657,16 +652,12 @@ static int acpi_tie_acpi_dev(struct acpi > return 0; > } > > -int acpi_device_add(struct acpi_device *device, > - void (*release)(struct device *)) > +int __acpi_device_add(struct acpi_device *device, > + void (*release)(struct device *)) > { > struct acpi_device_bus_id *acpi_device_bus_id; > int result; > > - result = acpi_tie_acpi_dev(device); > - if (result) > - return result; > - > /* > * Linkage > * ------- > @@ -755,6 +746,17 @@ err_unlock: > return result; > } > > +int acpi_device_add(struct acpi_device *adev, void (*release)(struct device *)) > +{ > + int ret; > + > + ret = acpi_tie_acpi_dev(adev); > + if (ret) > + return ret; > + > + return __acpi_device_add(adev, release); > +} > + > /* -------------------------------------------------------------------------- > Device Enumeration > -------------------------------------------------------------------------- */ > @@ -1681,14 +1683,10 @@ static void acpi_scan_dep_init(struct ac > { > struct acpi_dep_data *dep; > > - mutex_lock(&acpi_dep_list_lock); > - > list_for_each_entry(dep, &acpi_dep_list, node) { > if (dep->consumer == adev->handle) > adev->dep_unmet++; > } > - > - mutex_unlock(&acpi_dep_list_lock); > } > > void acpi_device_add_finalize(struct acpi_device *device) > @@ -1707,6 +1705,7 @@ static int acpi_add_single_object(struct > acpi_handle handle, int type, bool dep_init) > { > struct acpi_device *device; > + bool release_dep_lock = false; > int result; > > device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL); > @@ -1720,16 +1719,31 @@ static int acpi_add_single_object(struct > * this must be done before the get power-/wakeup_dev-flags calls. > */ > if (type == ACPI_BUS_TYPE_DEVICE || type == ACPI_BUS_TYPE_PROCESSOR) { > - if (dep_init) > + if (dep_init) { > + mutex_lock(&acpi_dep_list_lock); > + /* > + * Hold the lock until the acpi_tie_acpi_dev() call > + * below to prevent concurrent acpi_scan_clear_dep() > + * from deleting a dependency list entry without > + * updating dep_unmet for the device. > + */ > + release_dep_lock = true; > acpi_scan_dep_init(device); > - > + } > acpi_scan_init_status(device); > } > > acpi_bus_get_power_flags(device); > acpi_bus_get_wakeup_device_flags(device); > > - result = acpi_device_add(device, acpi_device_release); > + result = acpi_tie_acpi_dev(device); > + > + if (release_dep_lock) > + mutex_unlock(&acpi_dep_list_lock); > + > + if (!result) > + result = __acpi_device_add(device, acpi_device_release); > + > if (result) { > acpi_device_release(&device->dev); > return result; > > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies 2021-06-16 14:25 ` [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies Rafael J. Wysocki @ 2021-06-17 0:25 ` kernel test robot 2021-06-16 18:00 ` [PATCH v2 " Rafael J. Wysocki ` (3 subsequent siblings) 4 siblings, 0 replies; 24+ messages in thread From: kernel test robot @ 2021-06-17 0:25 UTC (permalink / raw) To: Rafael J. Wysocki, Linux ACPI Cc: kbuild-all, clang-built-linux, LKML, Hans de Goede [-- Attachment #1: Type: text/plain, Size: 5204 bytes --] Hi "Rafael, I love your patch! Perhaps something to improve: [auto build test WARNING on pm/linux-next] [also build test WARNING on next-20210616] [cannot apply to linux/master linus/master v5.13-rc6] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Rafael-J-Wysocki/ACPI-scan-Fixes-and-cleanups-related-to-dependencies-list-handling/20210617-013528 base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next config: x86_64-randconfig-b001-20210615 (attached as .config) compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 64720f57bea6a6bf033feef4a5751ab9c0c3b401) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install x86_64 cross compiling tool for clang build # apt-get install binutils-x86-64-linux-gnu # https://github.com/0day-ci/linux/commit/6369a007980e42b3ba7bbfb9833146f1867c790b git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Rafael-J-Wysocki/ACPI-scan-Fixes-and-cleanups-related-to-dependencies-list-handling/20210617-013528 git checkout 6369a007980e42b3ba7bbfb9833146f1867c790b # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): >> drivers/acpi/scan.c:660:5: warning: no previous prototype for function '__acpi_device_add' [-Wmissing-prototypes] int __acpi_device_add(struct acpi_device *device, ^ drivers/acpi/scan.c:660:1: note: declare 'static' if the function is not intended to be used outside of this translation unit int __acpi_device_add(struct acpi_device *device, ^ static 1 warning generated. vim +/__acpi_device_add +660 drivers/acpi/scan.c 659 > 660 int __acpi_device_add(struct acpi_device *device, 661 void (*release)(struct device *)) 662 { 663 struct acpi_device_bus_id *acpi_device_bus_id; 664 int result; 665 666 /* 667 * Linkage 668 * ------- 669 * Link this device to its parent and siblings. 670 */ 671 INIT_LIST_HEAD(&device->children); 672 INIT_LIST_HEAD(&device->node); 673 INIT_LIST_HEAD(&device->wakeup_list); 674 INIT_LIST_HEAD(&device->physical_node_list); 675 INIT_LIST_HEAD(&device->del_list); 676 mutex_init(&device->physical_node_lock); 677 678 mutex_lock(&acpi_device_lock); 679 680 acpi_device_bus_id = acpi_device_bus_id_match(acpi_device_hid(device)); 681 if (acpi_device_bus_id) { 682 result = acpi_device_set_name(device, acpi_device_bus_id); 683 if (result) 684 goto err_unlock; 685 } else { 686 acpi_device_bus_id = kzalloc(sizeof(*acpi_device_bus_id), 687 GFP_KERNEL); 688 if (!acpi_device_bus_id) { 689 result = -ENOMEM; 690 goto err_unlock; 691 } 692 acpi_device_bus_id->bus_id = 693 kstrdup_const(acpi_device_hid(device), GFP_KERNEL); 694 if (!acpi_device_bus_id->bus_id) { 695 kfree(acpi_device_bus_id); 696 result = -ENOMEM; 697 goto err_unlock; 698 } 699 700 ida_init(&acpi_device_bus_id->instance_ida); 701 702 result = acpi_device_set_name(device, acpi_device_bus_id); 703 if (result) { 704 kfree_const(acpi_device_bus_id->bus_id); 705 kfree(acpi_device_bus_id); 706 goto err_unlock; 707 } 708 709 list_add_tail(&acpi_device_bus_id->node, &acpi_bus_id_list); 710 } 711 712 if (device->parent) 713 list_add_tail(&device->node, &device->parent->children); 714 715 if (device->wakeup.flags.valid) 716 list_add_tail(&device->wakeup_list, &acpi_wakeup_device_list); 717 718 mutex_unlock(&acpi_device_lock); 719 720 if (device->parent) 721 device->dev.parent = &device->parent->dev; 722 723 device->dev.bus = &acpi_bus_type; 724 device->dev.release = release; 725 result = device_add(&device->dev); 726 if (result) { 727 dev_err(&device->dev, "Error registering device\n"); 728 goto err; 729 } 730 731 result = acpi_device_setup_files(device); 732 if (result) 733 pr_err("Error creating sysfs interface for device %s\n", 734 dev_name(&device->dev)); 735 736 return 0; 737 738 err: 739 mutex_lock(&acpi_device_lock); 740 741 if (device->parent) 742 list_del(&device->node); 743 744 list_del(&device->wakeup_list); 745 746 err_unlock: 747 mutex_unlock(&acpi_device_lock); 748 749 acpi_detach_data(device->handle, acpi_scan_drop_device); 750 751 return result; 752 } 753 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 30253 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies @ 2021-06-17 0:25 ` kernel test robot 0 siblings, 0 replies; 24+ messages in thread From: kernel test robot @ 2021-06-17 0:25 UTC (permalink / raw) To: kbuild-all [-- Attachment #1: Type: text/plain, Size: 5349 bytes --] Hi "Rafael, I love your patch! Perhaps something to improve: [auto build test WARNING on pm/linux-next] [also build test WARNING on next-20210616] [cannot apply to linux/master linus/master v5.13-rc6] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Rafael-J-Wysocki/ACPI-scan-Fixes-and-cleanups-related-to-dependencies-list-handling/20210617-013528 base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next config: x86_64-randconfig-b001-20210615 (attached as .config) compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 64720f57bea6a6bf033feef4a5751ab9c0c3b401) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install x86_64 cross compiling tool for clang build # apt-get install binutils-x86-64-linux-gnu # https://github.com/0day-ci/linux/commit/6369a007980e42b3ba7bbfb9833146f1867c790b git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Rafael-J-Wysocki/ACPI-scan-Fixes-and-cleanups-related-to-dependencies-list-handling/20210617-013528 git checkout 6369a007980e42b3ba7bbfb9833146f1867c790b # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): >> drivers/acpi/scan.c:660:5: warning: no previous prototype for function '__acpi_device_add' [-Wmissing-prototypes] int __acpi_device_add(struct acpi_device *device, ^ drivers/acpi/scan.c:660:1: note: declare 'static' if the function is not intended to be used outside of this translation unit int __acpi_device_add(struct acpi_device *device, ^ static 1 warning generated. vim +/__acpi_device_add +660 drivers/acpi/scan.c 659 > 660 int __acpi_device_add(struct acpi_device *device, 661 void (*release)(struct device *)) 662 { 663 struct acpi_device_bus_id *acpi_device_bus_id; 664 int result; 665 666 /* 667 * Linkage 668 * ------- 669 * Link this device to its parent and siblings. 670 */ 671 INIT_LIST_HEAD(&device->children); 672 INIT_LIST_HEAD(&device->node); 673 INIT_LIST_HEAD(&device->wakeup_list); 674 INIT_LIST_HEAD(&device->physical_node_list); 675 INIT_LIST_HEAD(&device->del_list); 676 mutex_init(&device->physical_node_lock); 677 678 mutex_lock(&acpi_device_lock); 679 680 acpi_device_bus_id = acpi_device_bus_id_match(acpi_device_hid(device)); 681 if (acpi_device_bus_id) { 682 result = acpi_device_set_name(device, acpi_device_bus_id); 683 if (result) 684 goto err_unlock; 685 } else { 686 acpi_device_bus_id = kzalloc(sizeof(*acpi_device_bus_id), 687 GFP_KERNEL); 688 if (!acpi_device_bus_id) { 689 result = -ENOMEM; 690 goto err_unlock; 691 } 692 acpi_device_bus_id->bus_id = 693 kstrdup_const(acpi_device_hid(device), GFP_KERNEL); 694 if (!acpi_device_bus_id->bus_id) { 695 kfree(acpi_device_bus_id); 696 result = -ENOMEM; 697 goto err_unlock; 698 } 699 700 ida_init(&acpi_device_bus_id->instance_ida); 701 702 result = acpi_device_set_name(device, acpi_device_bus_id); 703 if (result) { 704 kfree_const(acpi_device_bus_id->bus_id); 705 kfree(acpi_device_bus_id); 706 goto err_unlock; 707 } 708 709 list_add_tail(&acpi_device_bus_id->node, &acpi_bus_id_list); 710 } 711 712 if (device->parent) 713 list_add_tail(&device->node, &device->parent->children); 714 715 if (device->wakeup.flags.valid) 716 list_add_tail(&device->wakeup_list, &acpi_wakeup_device_list); 717 718 mutex_unlock(&acpi_device_lock); 719 720 if (device->parent) 721 device->dev.parent = &device->parent->dev; 722 723 device->dev.bus = &acpi_bus_type; 724 device->dev.release = release; 725 result = device_add(&device->dev); 726 if (result) { 727 dev_err(&device->dev, "Error registering device\n"); 728 goto err; 729 } 730 731 result = acpi_device_setup_files(device); 732 if (result) 733 pr_err("Error creating sysfs interface for device %s\n", 734 dev_name(&device->dev)); 735 736 return 0; 737 738 err: 739 mutex_lock(&acpi_device_lock); 740 741 if (device->parent) 742 list_del(&device->node); 743 744 list_del(&device->wakeup_list); 745 746 err_unlock: 747 mutex_unlock(&acpi_device_lock); 748 749 acpi_detach_data(device->handle, acpi_scan_drop_device); 750 751 return result; 752 } 753 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org [-- Attachment #2: config.gz --] [-- Type: application/gzip, Size: 30253 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies 2021-06-16 14:25 ` [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies Rafael J. Wysocki @ 2021-06-17 0:41 ` kernel test robot 2021-06-16 18:00 ` [PATCH v2 " Rafael J. Wysocki ` (3 subsequent siblings) 4 siblings, 0 replies; 24+ messages in thread From: kernel test robot @ 2021-06-17 0:41 UTC (permalink / raw) To: Rafael J. Wysocki, Linux ACPI; +Cc: kbuild-all, LKML, Hans de Goede [-- Attachment #1: Type: text/plain, Size: 1756 bytes --] Hi "Rafael, I love your patch! Perhaps something to improve: [auto build test WARNING on pm/linux-next] [also build test WARNING on next-20210616] [cannot apply to linux/master linus/master v5.13-rc6] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Rafael-J-Wysocki/ACPI-scan-Fixes-and-cleanups-related-to-dependencies-list-handling/20210617-013528 base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next config: i386-randconfig-s001-20210615 (attached as .config) compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 reproduce: # apt-get install sparse # sparse version: v0.6.3-341-g8af24329-dirty # https://github.com/0day-ci/linux/commit/6369a007980e42b3ba7bbfb9833146f1867c790b git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Rafael-J-Wysocki/ACPI-scan-Fixes-and-cleanups-related-to-dependencies-list-handling/20210617-013528 git checkout 6369a007980e42b3ba7bbfb9833146f1867c790b # save the attached .config to linux build tree make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' W=1 ARCH=i386 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> sparse warnings: (new ones prefixed by >>) >> drivers/acpi/scan.c:660:5: sparse: sparse: symbol '__acpi_device_add' was not declared. Should it be static? Please review and possibly fold the followup patch. --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 41825 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies @ 2021-06-17 0:41 ` kernel test robot 0 siblings, 0 replies; 24+ messages in thread From: kernel test robot @ 2021-06-17 0:41 UTC (permalink / raw) To: kbuild-all [-- Attachment #1: Type: text/plain, Size: 1795 bytes --] Hi "Rafael, I love your patch! Perhaps something to improve: [auto build test WARNING on pm/linux-next] [also build test WARNING on next-20210616] [cannot apply to linux/master linus/master v5.13-rc6] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Rafael-J-Wysocki/ACPI-scan-Fixes-and-cleanups-related-to-dependencies-list-handling/20210617-013528 base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next config: i386-randconfig-s001-20210615 (attached as .config) compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 reproduce: # apt-get install sparse # sparse version: v0.6.3-341-g8af24329-dirty # https://github.com/0day-ci/linux/commit/6369a007980e42b3ba7bbfb9833146f1867c790b git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Rafael-J-Wysocki/ACPI-scan-Fixes-and-cleanups-related-to-dependencies-list-handling/20210617-013528 git checkout 6369a007980e42b3ba7bbfb9833146f1867c790b # save the attached .config to linux build tree make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' W=1 ARCH=i386 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> sparse warnings: (new ones prefixed by >>) >> drivers/acpi/scan.c:660:5: sparse: sparse: symbol '__acpi_device_add' was not declared. Should it be static? Please review and possibly fold the followup patch. --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org [-- Attachment #2: config.gz --] [-- Type: application/gzip, Size: 41825 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* [RFC PATCH] ACPI: scan: __acpi_device_add() can be static 2021-06-16 14:25 ` [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies Rafael J. Wysocki @ 2021-06-17 0:42 ` kernel test robot 2021-06-16 18:00 ` [PATCH v2 " Rafael J. Wysocki ` (3 subsequent siblings) 4 siblings, 0 replies; 24+ messages in thread From: kernel test robot @ 2021-06-17 0:42 UTC (permalink / raw) To: Rafael J. Wysocki, Linux ACPI; +Cc: kbuild-all, LKML, Hans de Goede drivers/acpi/scan.c:660:5: warning: symbol '__acpi_device_add' was not declared. Should it be static? Reported-by: kernel test robot <lkp@intel.com> Signed-off-by: kernel test robot <lkp@intel.com> --- scan.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index be8b149244220..f30ab5a55a9a4 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -657,8 +657,8 @@ static int acpi_tie_acpi_dev(struct acpi_device *adev) return 0; } -int __acpi_device_add(struct acpi_device *device, - void (*release)(struct device *)) +static int __acpi_device_add(struct acpi_device *device, + void (*release)(struct device *)) { struct acpi_device_bus_id *acpi_device_bus_id; int result; ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [RFC PATCH] ACPI: scan: __acpi_device_add() can be static @ 2021-06-17 0:42 ` kernel test robot 0 siblings, 0 replies; 24+ messages in thread From: kernel test robot @ 2021-06-17 0:42 UTC (permalink / raw) To: kbuild-all [-- Attachment #1: Type: text/plain, Size: 796 bytes --] drivers/acpi/scan.c:660:5: warning: symbol '__acpi_device_add' was not declared. Should it be static? Reported-by: kernel test robot <lkp@intel.com> Signed-off-by: kernel test robot <lkp@intel.com> --- scan.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index be8b149244220..f30ab5a55a9a4 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -657,8 +657,8 @@ static int acpi_tie_acpi_dev(struct acpi_device *adev) return 0; } -int __acpi_device_add(struct acpi_device *device, - void (*release)(struct device *)) +static int __acpi_device_add(struct acpi_device *device, + void (*release)(struct device *)) { struct acpi_device_bus_id *acpi_device_bus_id; int result; ^ permalink raw reply related [flat|nested] 24+ messages in thread
end of thread, other threads:[~2021-06-17 0:42 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-16 14:21 [PATCH 0/5] ACPI: scan: Fixes and cleanups related to dependencies list handling Rafael J. Wysocki 2021-06-16 14:21 ` [PATCH 1/5] ACPI: scan: Rearrange acpi_dev_get_first_consumer_dev_cb() Rafael J. Wysocki 2021-06-16 14:36 ` Hans de Goede 2021-06-16 14:22 ` [PATCH 2/5] ACPI: scan: Make acpi_walk_dep_device_list() Rafael J. Wysocki 2021-06-16 14:41 ` Hans de Goede 2021-06-16 15:11 ` Rafael J. Wysocki 2021-06-16 15:25 ` Rafael J. Wysocki 2021-06-16 15:28 ` Hans de Goede 2021-06-16 14:23 ` [PATCH 3/5] ACPI: scan: Fix device object rescan in acpi_scan_clear_dep() Rafael J. Wysocki 2021-06-16 14:48 ` Hans de Goede 2021-06-16 15:12 ` Rafael J. Wysocki 2021-06-16 14:24 ` [PATCH 4/5] ACPI: scan: Reorganize acpi_device_add() Rafael J. Wysocki 2021-06-16 14:49 ` Hans de Goede 2021-06-16 14:25 ` [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies Rafael J. Wysocki 2021-06-16 14:55 ` Hans de Goede 2021-06-16 15:19 ` Rafael J. Wysocki 2021-06-16 18:00 ` [PATCH v2 " Rafael J. Wysocki 2021-06-16 18:04 ` Hans de Goede 2021-06-17 0:25 ` [PATCH " kernel test robot 2021-06-17 0:25 ` kernel test robot 2021-06-17 0:41 ` kernel test robot 2021-06-17 0:41 ` kernel test robot 2021-06-17 0:42 ` [RFC PATCH] ACPI: scan: __acpi_device_add() can be static kernel test robot 2021-06-17 0:42 ` kernel test robot
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.