| Message ID | 20220211110423.22733-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/1] ACPI: Switch to use list_entry_is_head() helper | expand |
Hi Andy,
I love your patch! Yet something to improve:
[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on nvdimm/libnvdimm-for-next v5.17-rc3 next-20220211]
[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/Andy-Shevchenko/ACPI-Switch-to-use-list_entry_is_head-helper/20220211-190438
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: ia64-allmodconfig (https://download.01.org/0day-ci/archive/20220212/202202120054.idhiETlD-lkp@intel.com/config)
compiler: ia64-linux-gcc (GCC) 11.2.0
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
# https://github.com/0day-ci/linux/commit/95f7c8c71bb18e505f5399a87cbb192f481c86fe
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Andy-Shevchenko/ACPI-Switch-to-use-list_entry_is_head-helper/20220211-190438
git checkout 95f7c8c71bb18e505f5399a87cbb192f481c86fe
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=ia64 SHELL=/bin/bash drivers/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/acpi/acpi_ipmi.c: In function 'ipmi_cancel_tx_msg':
>> drivers/acpi/acpi_ipmi.c:369:17: error: expected ')' before 'return'
369 | return;
| ^~~~~~
drivers/acpi/acpi_ipmi.c:368:12: note: to match this '('
368 | if (list_entry_is_head(tx_msg, &ipmi->tx_msg_list, head)
| ^
>> drivers/acpi/acpi_ipmi.c:372:1: error: expected expression before '}' token
372 | }
| ^
vim +369 drivers/acpi/acpi_ipmi.c
352
353 static void ipmi_cancel_tx_msg(struct acpi_ipmi_device *ipmi,
354 struct acpi_ipmi_msg *msg)
355 {
356 struct acpi_ipmi_msg *tx_msg, *temp;
357 unsigned long flags;
358
359 spin_lock_irqsave(&ipmi->tx_msg_lock, flags);
360 list_for_each_entry_safe(tx_msg, temp, &ipmi->tx_msg_list, head) {
361 if (msg == tx_msg) {
362 list_del(&tx_msg->head);
363 break;
364 }
365 }
366 spin_unlock_irqrestore(&ipmi->tx_msg_lock, flags);
367
368 if (list_entry_is_head(tx_msg, &ipmi->tx_msg_list, head)
> 369 return;
370
371 acpi_ipmi_msg_put(tx_msg);
> 372 }
373
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Fri, Feb 11, 2022 at 01:04:23PM +0200, Andy Shevchenko wrote: > Since we got list_entry_is_head() helper in the generic header, > we may switch the ACPI modules to use it. This eliminates the > need in additional variable. In some cases it reduces critical > sections as well. Besides the work required in a couple of cases (LKP) there is an ongoing discussion about list loops (and this particular API). Rafael, what do you think is the best course of action here?
On Wed, Mar 2, 2022 at 4:50 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Feb 11, 2022 at 01:04:23PM +0200, Andy Shevchenko wrote: > > Since we got list_entry_is_head() helper in the generic header, > > we may switch the ACPI modules to use it. This eliminates the > > need in additional variable. In some cases it reduces critical > > sections as well. > > Besides the work required in a couple of cases (LKP) there is an > ongoing discussion about list loops (and this particular API). > > Rafael, what do you think is the best course of action here? I think the current approach is to do the opposite of what this patch is attempting to do: avoid using the list iterator outside of the loop.
On Wed, Mar 02, 2022 at 05:36:20PM +0100, Rafael J. Wysocki wrote: > On Wed, Mar 2, 2022 at 4:50 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Feb 11, 2022 at 01:04:23PM +0200, Andy Shevchenko wrote: > > > Since we got list_entry_is_head() helper in the generic header, > > > we may switch the ACPI modules to use it. This eliminates the > > > need in additional variable. In some cases it reduces critical > > > sections as well. > > > > Besides the work required in a couple of cases (LKP) there is an > > ongoing discussion about list loops (and this particular API). > > > > Rafael, what do you think is the best course of action here? > > I think the current approach is to do the opposite of what this patch > is attempting to do: avoid using the list iterator outside of the > loop. OK, let's drop this change.
diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c index a5fe2926bf50..f9e56138f8d1 100644 --- a/drivers/acpi/acpi_ipmi.c +++ b/drivers/acpi/acpi_ipmi.c @@ -354,27 +354,26 @@ static void ipmi_cancel_tx_msg(struct acpi_ipmi_device *ipmi, struct acpi_ipmi_msg *msg) { struct acpi_ipmi_msg *tx_msg, *temp; - bool msg_found = false; unsigned long flags; spin_lock_irqsave(&ipmi->tx_msg_lock, flags); list_for_each_entry_safe(tx_msg, temp, &ipmi->tx_msg_list, head) { if (msg == tx_msg) { - msg_found = true; list_del(&tx_msg->head); break; } } spin_unlock_irqrestore(&ipmi->tx_msg_lock, flags); - if (msg_found) - acpi_ipmi_msg_put(tx_msg); + if (list_entry_is_head(tx_msg, &ipmi->tx_msg_list, head) + return; + + acpi_ipmi_msg_put(tx_msg); } static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data) { struct acpi_ipmi_device *ipmi_device = user_msg_data; - bool msg_found = false; struct acpi_ipmi_msg *tx_msg, *temp; struct device *dev = ipmi_device->dev; unsigned long flags; @@ -389,14 +388,13 @@ static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data) spin_lock_irqsave(&ipmi_device->tx_msg_lock, flags); list_for_each_entry_safe(tx_msg, temp, &ipmi_device->tx_msg_list, head) { if (msg->msgid == tx_msg->tx_msgid) { - msg_found = true; list_del(&tx_msg->head); break; } } spin_unlock_irqrestore(&ipmi_device->tx_msg_lock, flags); - if (!msg_found) { + if (list_entry_is_head(tx_msg, &ipmi_device->tx_msg_list, head)) { dev_warn(dev, "Unexpected response (msg id %ld) is returned.\n", msg->msgid); @@ -483,13 +481,11 @@ static void ipmi_register_bmc(int iface, struct device *dev) static void ipmi_bmc_gone(int iface) { struct acpi_ipmi_device *ipmi_device, *temp; - bool dev_found = false; mutex_lock(&driver_data.ipmi_lock); list_for_each_entry_safe(ipmi_device, temp, &driver_data.ipmi_devices, head) { if (ipmi_device->ipmi_ifnum != iface) { - dev_found = true; __ipmi_dev_kill(ipmi_device); break; } @@ -500,7 +496,7 @@ static void ipmi_bmc_gone(int iface) struct acpi_ipmi_device, head); mutex_unlock(&driver_data.ipmi_lock); - if (dev_found) { + if (!list_entry_is_head(ipmi_device, &driver_data.ipmi_devices, head)) { ipmi_flush_tx_msg(ipmi_device); acpi_ipmi_dev_put(ipmi_device); } diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c index ef104809f27b..ffc0b3ee190b 100644 --- a/drivers/acpi/glue.c +++ b/drivers/acpi/glue.c @@ -61,17 +61,15 @@ EXPORT_SYMBOL_GPL(unregister_acpi_bus_type); static struct acpi_bus_type *acpi_get_bus_type(struct device *dev) { - struct acpi_bus_type *tmp, *ret = NULL; + struct acpi_bus_type *tmp; down_read(&bus_type_sem); list_for_each_entry(tmp, &bus_type_list, list) { - if (tmp->match(dev)) { - ret = tmp; + if (tmp->match(dev)) break; - } } up_read(&bus_type_sem); - return ret; + return list_entry_is_head(tmp, &bus_type_list, list) ? NULL : tmp; } #define FIND_CHILD_MIN_SCORE 1 diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c index e5d7f2bda13f..b31c16e5e42c 100644 --- a/drivers/acpi/nfit/core.c +++ b/drivers/acpi/nfit/core.c @@ -1076,8 +1076,8 @@ static void nfit_mem_init_bdw(struct acpi_nfit_desc *acpi_desc, static int __nfit_mem_init(struct acpi_nfit_desc *acpi_desc, struct acpi_nfit_system_address *spa) { - struct nfit_mem *nfit_mem, *found; struct nfit_memdev *nfit_memdev; + struct nfit_mem *nfit_mem; int type = spa ? nfit_spa_type(spa) : 0; switch (type) { @@ -1106,19 +1106,13 @@ static int __nfit_mem_init(struct acpi_nfit_desc *acpi_desc, continue; if (!spa && nfit_memdev->memdev->range_index) continue; - found = NULL; dcr = nfit_memdev->memdev->region_index; device_handle = nfit_memdev->memdev->device_handle; list_for_each_entry(nfit_mem, &acpi_desc->dimms, list) - if (__to_nfit_memdev(nfit_mem)->device_handle - == device_handle) { - found = nfit_mem; + if (__to_nfit_memdev(nfit_mem)->device_handle == device_handle) break; - } - if (found) - nfit_mem = found; - else { + if (list_entry_is_head(nfit_mem, &acpi_desc->dimms, list)) { nfit_mem = devm_kzalloc(acpi_desc->dev, sizeof(*nfit_mem), GFP_KERNEL); if (!nfit_mem) diff --git a/drivers/acpi/nfit/mce.c b/drivers/acpi/nfit/mce.c index ee8d9973f60b..dbe70ebdfc79 100644 --- a/drivers/acpi/nfit/mce.c +++ b/drivers/acpi/nfit/mce.c @@ -33,7 +33,6 @@ static int nfit_handle_mce(struct notifier_block *nb, unsigned long val, mutex_lock(&acpi_desc_lock); list_for_each_entry(acpi_desc, &acpi_descs, list) { struct device *dev = acpi_desc->dev; - int found_match = 0; mutex_lock(&acpi_desc->init_mutex); list_for_each_entry(nfit_spa, &acpi_desc->spas, list) { @@ -46,7 +45,6 @@ static int nfit_handle_mce(struct notifier_block *nb, unsigned long val, continue; if ((spa->address + spa->length - 1) < mce->addr) continue; - found_match = 1; dev_dbg(dev, "addr in SPA %d (0x%llx, 0x%llx)\n", spa->range_index, spa->address, spa->length); /* @@ -58,7 +56,7 @@ static int nfit_handle_mce(struct notifier_block *nb, unsigned long val, } mutex_unlock(&acpi_desc->init_mutex); - if (!found_match) + if (list_entry_is_head(nfit_spa, &acpi_desc->spas, list)) continue; /* If this fails due to an -ENOMEM, there is little we can do */ diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c index c2d494784425..90ef0629737d 100644 --- a/drivers/acpi/resource.c +++ b/drivers/acpi/resource.c @@ -767,7 +767,7 @@ static int acpi_dev_consumes_res(struct acpi_device *adev, struct resource *res) { struct list_head resource_list; struct resource_entry *rentry; - int ret, found = 0; + int ret; INIT_LIST_HEAD(&resource_list); ret = acpi_dev_get_resources(adev, &resource_list, NULL, NULL); @@ -775,15 +775,12 @@ static int acpi_dev_consumes_res(struct acpi_device *adev, struct resource *res) return 0; list_for_each_entry(rentry, &resource_list, node) { - if (resource_contains(rentry->res, res)) { - found = 1; + if (resource_contains(rentry->res, res)) break; - } - } acpi_dev_free_resource_list(&resource_list); - return found; + return !list_entry_is_head(rentry, &resource_list, node); } static acpi_status acpi_res_consumer_cb(acpi_handle handle, u32 depth, diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c index d5cedffeeff9..9dcebb4421a0 100644 --- a/drivers/acpi/utils.c +++ b/drivers/acpi/utils.c @@ -771,17 +771,14 @@ EXPORT_SYMBOL(acpi_dev_hid_uid_match); bool acpi_dev_found(const char *hid) { struct acpi_device_bus_id *acpi_device_bus_id; - bool found = false; mutex_lock(&acpi_device_lock); list_for_each_entry(acpi_device_bus_id, &acpi_bus_id_list, node) - if (!strcmp(acpi_device_bus_id->bus_id, hid)) { - found = true; + if (!strcmp(acpi_device_bus_id->bus_id, hid)) break; - } mutex_unlock(&acpi_device_lock); - return found; + return !list_entry_is_head(acpi_device_bus_id, &acpi_bus_id_list, node); } EXPORT_SYMBOL(acpi_dev_found);
Since we got list_entry_is_head() helper in the generic header, we may switch the ACPI modules to use it. This eliminates the need in additional variable. In some cases it reduces critical sections as well. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/acpi/acpi_ipmi.c | 16 ++++++---------- drivers/acpi/glue.c | 8 +++----- drivers/acpi/nfit/core.c | 12 +++--------- drivers/acpi/nfit/mce.c | 4 +--- drivers/acpi/resource.c | 9 +++------ drivers/acpi/utils.c | 7 ++----- 6 files changed, 18 insertions(+), 38 deletions(-)