| Message ID | 20240820090239.17771-1-xuiagnh@gmail.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | platform/x86: system76: Reducing redundant conditional judgments in system76_add() | expand |
On Tue, 20 Aug 2024, Xi Huang wrote: > In case of an error, goto jumps to the “error” label, > where the if (data->has_open_ec) check is redundant in most cases. > Since the conditions for most goto statements have already > been satisfied by if (data->has_open_ec),the code has been modified to > improve execution speed. And why would the error rollback path need to be improved in execution speed? In any case, this change is going to below the noise level when it comes to a measurable impact. I'm sorry but I don't think this change is useful.
Hi Xi,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on v6.11-rc4 next-20240820]
[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#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Xi-Huang/platform-x86-system76-Reducing-redundant-conditional-judgments-in-system76_add/20240820-170328
base: linus/master
patch link: https://lore.kernel.org/r/20240820090239.17771-1-xuiagnh%40gmail.com
patch subject: [PATCH] platform/x86: system76: Reducing redundant conditional judgments in system76_add()
config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20240821/202408210354.eRwpaGm7-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240821/202408210354.eRwpaGm7-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408210354.eRwpaGm7-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/platform/x86/system76_acpi.c: In function 'system76_add':
>> drivers/platform/x86/system76_acpi.c:759:12: warning: suggest explicit braces to avoid ambiguous 'else' [-Wdangling-else]
759 | if (err)
| ^
vim +/else +759 drivers/platform/x86/system76_acpi.c
fd13c8622a5ad4f Jeremy Soller 2019-10-09 673
fd13c8622a5ad4f Jeremy Soller 2019-10-09 674 // Add a System76 ACPI device
fd13c8622a5ad4f Jeremy Soller 2019-10-09 675 static int system76_add(struct acpi_device *acpi_dev)
fd13c8622a5ad4f Jeremy Soller 2019-10-09 676 {
fd13c8622a5ad4f Jeremy Soller 2019-10-09 677 struct system76_data *data;
fd13c8622a5ad4f Jeremy Soller 2019-10-09 678 int err;
fd13c8622a5ad4f Jeremy Soller 2019-10-09 679
fd13c8622a5ad4f Jeremy Soller 2019-10-09 680 data = devm_kzalloc(&acpi_dev->dev, sizeof(*data), GFP_KERNEL);
fd13c8622a5ad4f Jeremy Soller 2019-10-09 681 if (!data)
fd13c8622a5ad4f Jeremy Soller 2019-10-09 682 return -ENOMEM;
fd13c8622a5ad4f Jeremy Soller 2019-10-09 683 acpi_dev->driver_data = data;
fd13c8622a5ad4f Jeremy Soller 2019-10-09 684 data->acpi_dev = acpi_dev;
fd13c8622a5ad4f Jeremy Soller 2019-10-09 685
c4499272566d677 Tim Crawford 2021-12-22 686 // Some models do not run open EC firmware. Check for an ACPI method
c4499272566d677 Tim Crawford 2021-12-22 687 // that only exists on open EC to guard functionality specific to it.
c4499272566d677 Tim Crawford 2021-12-22 688 data->has_open_ec = acpi_has_method(acpi_device_handle(data->acpi_dev), "NFAN");
c4499272566d677 Tim Crawford 2021-12-22 689
fd13c8622a5ad4f Jeremy Soller 2019-10-09 690 err = system76_get(data, "INIT");
fd13c8622a5ad4f Jeremy Soller 2019-10-09 691 if (err)
fd13c8622a5ad4f Jeremy Soller 2019-10-09 692 return err;
fd13c8622a5ad4f Jeremy Soller 2019-10-09 693 data->ap_led.name = "system76_acpi::airplane";
fd13c8622a5ad4f Jeremy Soller 2019-10-09 694 data->ap_led.flags = LED_CORE_SUSPENDRESUME;
fd13c8622a5ad4f Jeremy Soller 2019-10-09 695 data->ap_led.brightness_get = ap_led_get;
5b36398dc846a52 Nick Shipp 2020-07-09 696 data->ap_led.brightness_set_blocking = ap_led_set;
fd13c8622a5ad4f Jeremy Soller 2019-10-09 697 data->ap_led.max_brightness = 1;
fd13c8622a5ad4f Jeremy Soller 2019-10-09 698 data->ap_led.default_trigger = "rfkill-none";
fd13c8622a5ad4f Jeremy Soller 2019-10-09 699 err = devm_led_classdev_register(&acpi_dev->dev, &data->ap_led);
fd13c8622a5ad4f Jeremy Soller 2019-10-09 700 if (err)
fd13c8622a5ad4f Jeremy Soller 2019-10-09 701 return err;
fd13c8622a5ad4f Jeremy Soller 2019-10-09 702
fd13c8622a5ad4f Jeremy Soller 2019-10-09 703 data->kb_led.name = "system76_acpi::kbd_backlight";
fd13c8622a5ad4f Jeremy Soller 2019-10-09 704 data->kb_led.flags = LED_BRIGHT_HW_CHANGED | LED_CORE_SUSPENDRESUME;
fd13c8622a5ad4f Jeremy Soller 2019-10-09 705 data->kb_led.brightness_get = kb_led_get;
5b36398dc846a52 Nick Shipp 2020-07-09 706 data->kb_led.brightness_set_blocking = kb_led_set;
5d36931f0fe5166 Tim Crawford 2023-07-19 707 if (acpi_has_method(acpi_device_handle(data->acpi_dev), "GKBK")) {
5d36931f0fe5166 Tim Crawford 2023-07-19 708 // Use the new ACPI methods
5d36931f0fe5166 Tim Crawford 2023-07-19 709 data->kbled_type = system76_get(data, "GKBK");
5d36931f0fe5166 Tim Crawford 2023-07-19 710
5d36931f0fe5166 Tim Crawford 2023-07-19 711 switch (data->kbled_type) {
5d36931f0fe5166 Tim Crawford 2023-07-19 712 case KBLED_NONE:
5d36931f0fe5166 Tim Crawford 2023-07-19 713 // Nothing to do: Device will not be registered.
5d36931f0fe5166 Tim Crawford 2023-07-19 714 break;
5d36931f0fe5166 Tim Crawford 2023-07-19 715 case KBLED_WHITE:
5d36931f0fe5166 Tim Crawford 2023-07-19 716 data->kb_led.max_brightness = 255;
5d36931f0fe5166 Tim Crawford 2023-07-19 717 data->kb_toggle_brightness = 72;
5d36931f0fe5166 Tim Crawford 2023-07-19 718 break;
5d36931f0fe5166 Tim Crawford 2023-07-19 719 case KBLED_RGB:
5d36931f0fe5166 Tim Crawford 2023-07-19 720 data->kb_led.max_brightness = 255;
5d36931f0fe5166 Tim Crawford 2023-07-19 721 data->kb_led.groups = system76_kb_led_color_groups;
5d36931f0fe5166 Tim Crawford 2023-07-19 722 data->kb_toggle_brightness = 72;
5d36931f0fe5166 Tim Crawford 2023-07-19 723 data->kb_color = 0xffffff;
5d36931f0fe5166 Tim Crawford 2023-07-19 724 system76_set(data, "SKBC", data->kb_color);
5d36931f0fe5166 Tim Crawford 2023-07-19 725 break;
5d36931f0fe5166 Tim Crawford 2023-07-19 726 }
5d36931f0fe5166 Tim Crawford 2023-07-19 727 } else {
5d36931f0fe5166 Tim Crawford 2023-07-19 728 // Use the old ACPI methods
fd13c8622a5ad4f Jeremy Soller 2019-10-09 729 if (acpi_has_method(acpi_device_handle(data->acpi_dev), "SKBC")) {
5d36931f0fe5166 Tim Crawford 2023-07-19 730 data->kbled_type = KBLED_RGB;
fd13c8622a5ad4f Jeremy Soller 2019-10-09 731 data->kb_led.max_brightness = 255;
603a7dd08f881e1 Tim Crawford 2021-10-06 732 data->kb_led.groups = system76_kb_led_color_groups;
fd13c8622a5ad4f Jeremy Soller 2019-10-09 733 data->kb_toggle_brightness = 72;
fd13c8622a5ad4f Jeremy Soller 2019-10-09 734 data->kb_color = 0xffffff;
fd13c8622a5ad4f Jeremy Soller 2019-10-09 735 system76_set(data, "SKBC", data->kb_color);
fd13c8622a5ad4f Jeremy Soller 2019-10-09 736 } else {
5d36931f0fe5166 Tim Crawford 2023-07-19 737 data->kbled_type = KBLED_WHITE;
fd13c8622a5ad4f Jeremy Soller 2019-10-09 738 data->kb_led.max_brightness = 5;
fd13c8622a5ad4f Jeremy Soller 2019-10-09 739 }
5d36931f0fe5166 Tim Crawford 2023-07-19 740 }
5d36931f0fe5166 Tim Crawford 2023-07-19 741
5d36931f0fe5166 Tim Crawford 2023-07-19 742 if (data->kbled_type != KBLED_NONE) {
fd13c8622a5ad4f Jeremy Soller 2019-10-09 743 err = devm_led_classdev_register(&acpi_dev->dev, &data->kb_led);
fd13c8622a5ad4f Jeremy Soller 2019-10-09 744 if (err)
fd13c8622a5ad4f Jeremy Soller 2019-10-09 745 return err;
5d36931f0fe5166 Tim Crawford 2023-07-19 746 }
fd13c8622a5ad4f Jeremy Soller 2019-10-09 747
0de30fc684b3883 Jeremy Soller 2021-10-06 748 data->input = devm_input_allocate_device(&acpi_dev->dev);
0de30fc684b3883 Jeremy Soller 2021-10-06 749 if (!data->input)
0de30fc684b3883 Jeremy Soller 2021-10-06 750 return -ENOMEM;
0de30fc684b3883 Jeremy Soller 2021-10-06 751
0de30fc684b3883 Jeremy Soller 2021-10-06 752 data->input->name = "System76 ACPI Hotkeys";
0de30fc684b3883 Jeremy Soller 2021-10-06 753 data->input->phys = "system76_acpi/input0";
0de30fc684b3883 Jeremy Soller 2021-10-06 754 data->input->id.bustype = BUS_HOST;
0de30fc684b3883 Jeremy Soller 2021-10-06 755 data->input->dev.parent = &acpi_dev->dev;
0de30fc684b3883 Jeremy Soller 2021-10-06 756 input_set_capability(data->input, EV_KEY, KEY_SCREENLOCK);
0de30fc684b3883 Jeremy Soller 2021-10-06 757
0de30fc684b3883 Jeremy Soller 2021-10-06 758 err = input_register_device(data->input);
0de30fc684b3883 Jeremy Soller 2021-10-06 @759 if (err)
634a2087bfdcba7 Xi Huang 2024-08-20 760 if (data->has_open_ec)
634a2087bfdcba7 Xi Huang 2024-08-20 761 goto free_error;
634a2087bfdcba7 Xi Huang 2024-08-20 762 else
634a2087bfdcba7 Xi Huang 2024-08-20 763 return err;
0de30fc684b3883 Jeremy Soller 2021-10-06 764
c4499272566d677 Tim Crawford 2021-12-22 765 if (data->has_open_ec) {
95563d45b5da9cd Jeremy Soller 2021-10-06 766 err = system76_get_object(data, "NFAN", &data->nfan);
95563d45b5da9cd Jeremy Soller 2021-10-06 767 if (err)
634a2087bfdcba7 Xi Huang 2024-08-20 768 goto free_error;
95563d45b5da9cd Jeremy Soller 2021-10-06 769
95563d45b5da9cd Jeremy Soller 2021-10-06 770 err = system76_get_object(data, "NTMP", &data->ntmp);
95563d45b5da9cd Jeremy Soller 2021-10-06 771 if (err)
634a2087bfdcba7 Xi Huang 2024-08-20 772 goto free_error;
95563d45b5da9cd Jeremy Soller 2021-10-06 773
95563d45b5da9cd Jeremy Soller 2021-10-06 774 data->therm = devm_hwmon_device_register_with_info(&acpi_dev->dev,
95563d45b5da9cd Jeremy Soller 2021-10-06 775 "system76_acpi", data, &thermal_chip_info, NULL);
95563d45b5da9cd Jeremy Soller 2021-10-06 776 err = PTR_ERR_OR_ZERO(data->therm);
95563d45b5da9cd Jeremy Soller 2021-10-06 777 if (err)
634a2087bfdcba7 Xi Huang 2024-08-20 778 goto free_error;
95563d45b5da9cd Jeremy Soller 2021-10-06 779
76f7eba3e0a248a Tim Crawford 2021-10-06 780 system76_battery_init();
c4499272566d677 Tim Crawford 2021-12-22 781 }
76f7eba3e0a248a Tim Crawford 2021-10-06 782
fd13c8622a5ad4f Jeremy Soller 2019-10-09 783 return 0;
95563d45b5da9cd Jeremy Soller 2021-10-06 784
634a2087bfdcba7 Xi Huang 2024-08-20 785 free_error:
95563d45b5da9cd Jeremy Soller 2021-10-06 786 kfree(data->ntmp);
95563d45b5da9cd Jeremy Soller 2021-10-06 787 kfree(data->nfan);
95563d45b5da9cd Jeremy Soller 2021-10-06 788 return err;
fd13c8622a5ad4f Jeremy Soller 2019-10-09 789 }
fd13c8622a5ad4f Jeremy Soller 2019-10-09 790
Hi Xi, kernel test robot noticed the following build warnings: [auto build test WARNING on linus/master] [also build test WARNING on v6.11-rc4 next-20240820] [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#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Xi-Huang/platform-x86-system76-Reducing-redundant-conditional-judgments-in-system76_add/20240820-170328 base: linus/master patch link: https://lore.kernel.org/r/20240820090239.17771-1-xuiagnh%40gmail.com patch subject: [PATCH] platform/x86: system76: Reducing redundant conditional judgments in system76_add() config: x86_64-buildonly-randconfig-001-20240820 (https://download.01.org/0day-ci/archive/20240821/202408210506.TtQpGzMk-lkp@intel.com/config) compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240821/202408210506.TtQpGzMk-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202408210506.TtQpGzMk-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/platform/x86/system76_acpi.c:762:3: warning: add explicit braces to avoid dangling else [-Wdangling-else] 762 | else | ^ 1 warning generated. vim +762 drivers/platform/x86/system76_acpi.c 673 674 // Add a System76 ACPI device 675 static int system76_add(struct acpi_device *acpi_dev) 676 { 677 struct system76_data *data; 678 int err; 679 680 data = devm_kzalloc(&acpi_dev->dev, sizeof(*data), GFP_KERNEL); 681 if (!data) 682 return -ENOMEM; 683 acpi_dev->driver_data = data; 684 data->acpi_dev = acpi_dev; 685 686 // Some models do not run open EC firmware. Check for an ACPI method 687 // that only exists on open EC to guard functionality specific to it. 688 data->has_open_ec = acpi_has_method(acpi_device_handle(data->acpi_dev), "NFAN"); 689 690 err = system76_get(data, "INIT"); 691 if (err) 692 return err; 693 data->ap_led.name = "system76_acpi::airplane"; 694 data->ap_led.flags = LED_CORE_SUSPENDRESUME; 695 data->ap_led.brightness_get = ap_led_get; 696 data->ap_led.brightness_set_blocking = ap_led_set; 697 data->ap_led.max_brightness = 1; 698 data->ap_led.default_trigger = "rfkill-none"; 699 err = devm_led_classdev_register(&acpi_dev->dev, &data->ap_led); 700 if (err) 701 return err; 702 703 data->kb_led.name = "system76_acpi::kbd_backlight"; 704 data->kb_led.flags = LED_BRIGHT_HW_CHANGED | LED_CORE_SUSPENDRESUME; 705 data->kb_led.brightness_get = kb_led_get; 706 data->kb_led.brightness_set_blocking = kb_led_set; 707 if (acpi_has_method(acpi_device_handle(data->acpi_dev), "GKBK")) { 708 // Use the new ACPI methods 709 data->kbled_type = system76_get(data, "GKBK"); 710 711 switch (data->kbled_type) { 712 case KBLED_NONE: 713 // Nothing to do: Device will not be registered. 714 break; 715 case KBLED_WHITE: 716 data->kb_led.max_brightness = 255; 717 data->kb_toggle_brightness = 72; 718 break; 719 case KBLED_RGB: 720 data->kb_led.max_brightness = 255; 721 data->kb_led.groups = system76_kb_led_color_groups; 722 data->kb_toggle_brightness = 72; 723 data->kb_color = 0xffffff; 724 system76_set(data, "SKBC", data->kb_color); 725 break; 726 } 727 } else { 728 // Use the old ACPI methods 729 if (acpi_has_method(acpi_device_handle(data->acpi_dev), "SKBC")) { 730 data->kbled_type = KBLED_RGB; 731 data->kb_led.max_brightness = 255; 732 data->kb_led.groups = system76_kb_led_color_groups; 733 data->kb_toggle_brightness = 72; 734 data->kb_color = 0xffffff; 735 system76_set(data, "SKBC", data->kb_color); 736 } else { 737 data->kbled_type = KBLED_WHITE; 738 data->kb_led.max_brightness = 5; 739 } 740 } 741 742 if (data->kbled_type != KBLED_NONE) { 743 err = devm_led_classdev_register(&acpi_dev->dev, &data->kb_led); 744 if (err) 745 return err; 746 } 747 748 data->input = devm_input_allocate_device(&acpi_dev->dev); 749 if (!data->input) 750 return -ENOMEM; 751 752 data->input->name = "System76 ACPI Hotkeys"; 753 data->input->phys = "system76_acpi/input0"; 754 data->input->id.bustype = BUS_HOST; 755 data->input->dev.parent = &acpi_dev->dev; 756 input_set_capability(data->input, EV_KEY, KEY_SCREENLOCK); 757 758 err = input_register_device(data->input); 759 if (err) 760 if (data->has_open_ec) 761 goto free_error; > 762 else 763 return err; 764 765 if (data->has_open_ec) { 766 err = system76_get_object(data, "NFAN", &data->nfan); 767 if (err) 768 goto free_error; 769 770 err = system76_get_object(data, "NTMP", &data->ntmp); 771 if (err) 772 goto free_error; 773 774 data->therm = devm_hwmon_device_register_with_info(&acpi_dev->dev, 775 "system76_acpi", data, &thermal_chip_info, NULL); 776 err = PTR_ERR_OR_ZERO(data->therm); 777 if (err) 778 goto free_error; 779 780 system76_battery_init(); 781 } 782 783 return 0; 784 785 free_error: 786 kfree(data->ntmp); 787 kfree(data->nfan); 788 return err; 789 } 790
diff --git a/drivers/platform/x86/system76_acpi.c b/drivers/platform/x86/system76_acpi.c index 3da753b3d..05b4bf18f 100644 --- a/drivers/platform/x86/system76_acpi.c +++ b/drivers/platform/x86/system76_acpi.c @@ -757,33 +757,34 @@ static int system76_add(struct acpi_device *acpi_dev) err = input_register_device(data->input); if (err) - goto error; + if (data->has_open_ec) + goto free_error; + else + return err; if (data->has_open_ec) { err = system76_get_object(data, "NFAN", &data->nfan); if (err) - goto error; + goto free_error; err = system76_get_object(data, "NTMP", &data->ntmp); if (err) - goto error; + goto free_error; data->therm = devm_hwmon_device_register_with_info(&acpi_dev->dev, "system76_acpi", data, &thermal_chip_info, NULL); err = PTR_ERR_OR_ZERO(data->therm); if (err) - goto error; + goto free_error; system76_battery_init(); } return 0; -error: - if (data->has_open_ec) { - kfree(data->ntmp); - kfree(data->nfan); - } +free_error: + kfree(data->ntmp); + kfree(data->nfan); return err; }
In case of an error, goto jumps to the “error” label, where the if (data->has_open_ec) check is redundant in most cases. Since the conditions for most goto statements have already been satisfied by if (data->has_open_ec),the code has been modified to improve execution speed. Signed-off-by: Xi Huang <xuiagnh@gmail.com> --- drivers/platform/x86/system76_acpi.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)