diff mbox series

[8/9] HID: i2c-hid: Do panel follower work on the system_wq

Message ID 20230523122802.8.I962bb462ede779005341c49320740ed95810021d@changeid (mailing list archive)
State New, archived
Headers show
Series drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together | expand

Commit Message

Doug Anderson May 23, 2023, 7:28 p.m. UTC 
Turning on an i2c-hid device can be a slow process. This is why
i2c-hid devices use PROBE_PREFER_ASYNCHRONOUS. Unfortunately, when
we're a panel follower the i2c-hid power up sequence now blocks the
power on of the panel. Let's fix that by scheduling the work on the
system_wq.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/hid/i2c-hid/i2c-hid-core.c | 42 +++++++++++++++++++++++++++---
 1 file changed, 38 insertions(+), 4 deletions(-)

Comments

kernel test robot May 24, 2023, 1:36 a.m. UTC | #1 
Hi Douglas,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on hid/for-next drm-misc/drm-misc-next linus/master v6.4-rc3 next-20230523]
[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/Douglas-Anderson/dt-bindings-HID-i2c-hid-Add-panel-property-to-i2c-hid-backed-panels/20230524-034323
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20230523122802.8.I962bb462ede779005341c49320740ed95810021d%40changeid
patch subject: [PATCH 8/9] HID: i2c-hid: Do panel follower work on the system_wq
config: m68k-allyesconfig
compiler: m68k-linux-gcc (GCC) 12.1.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/intel-lab-lkp/linux/commit/97c5984c98b7721d6c5299d8542c612e5c3240d3
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Douglas-Anderson/dt-bindings-HID-i2c-hid-Add-panel-property-to-i2c-hid-backed-panels/20230524-034323
        git checkout 97c5984c98b7721d6c5299d8542c612e5c3240d3
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/hid/i2c-hid/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202305240926.8pjzTMVj-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/hid/i2c-hid/i2c-hid-core.c:1013:5: warning: no previous prototype for 'i2c_hid_core_initial_power_up' [-Wmissing-prototypes]
    1013 | int i2c_hid_core_initial_power_up(struct i2c_hid *ihid)
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/hid/i2c-hid/i2c-hid-core.c:1067:6: warning: no previous prototype for 'ihid_core_panel_prepare_work' [-Wmissing-prototypes]
    1067 | void ihid_core_panel_prepare_work(struct work_struct *work)
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/hid/i2c-hid/i2c-hid-core.c:1093:5: warning: no previous prototype for 'i2c_hid_core_panel_prepared' [-Wmissing-prototypes]
    1093 | int i2c_hid_core_panel_prepared(struct drm_panel_follower *follower)
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/hid/i2c-hid/i2c-hid-core.c:1107:5: warning: no previous prototype for 'i2c_hid_core_panel_unpreparing' [-Wmissing-prototypes]
    1107 | int i2c_hid_core_panel_unpreparing(struct drm_panel_follower *follower)
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/ihid_core_panel_prepare_work +1067 drivers/hid/i2c-hid/i2c-hid-core.c

  1000	
  1001	/**
  1002	 * i2c_hid_core_initial_power_up() - First time power up of the i2c-hid device.
  1003	 * @ihid: The ihid object created during probe.
  1004	 *
  1005	 * This function is called at probe time.
  1006	 *
  1007	 * The initial power on is where we do some basic validation that the device
  1008	 * exists, where we fetch the HID descriptor, and where we create the actual
  1009	 * HID devices.
  1010	 *
  1011	 * Return: 0 or error code.
  1012	 */
> 1013	int i2c_hid_core_initial_power_up(struct i2c_hid *ihid)
  1014	{
  1015		struct i2c_client *client = ihid->client;
  1016		struct hid_device *hid = ihid->hid;
  1017		int ret;
  1018	
  1019		ret = i2c_hid_core_power_up(ihid);
  1020		if (ret)
  1021			return ret;
  1022	
  1023		/* Make sure there is something at this address */
  1024		ret = i2c_smbus_read_byte(client);
  1025		if (ret < 0) {
  1026			i2c_hid_dbg(ihid, "nothing at this address: %d\n", ret);
  1027			ret = -ENXIO;
  1028			goto err;
  1029		}
  1030	
  1031		ret = i2c_hid_fetch_hid_descriptor(ihid);
  1032		if (ret < 0) {
  1033			dev_err(&client->dev,
  1034				"Failed to fetch the HID Descriptor\n");
  1035			goto err;
  1036		}
  1037	
  1038		enable_irq(client->irq);
  1039	
  1040		hid->version = le16_to_cpu(ihid->hdesc.bcdVersion);
  1041		hid->vendor = le16_to_cpu(ihid->hdesc.wVendorID);
  1042		hid->product = le16_to_cpu(ihid->hdesc.wProductID);
  1043	
  1044		hid->initial_quirks |= i2c_hid_get_dmi_quirks(hid->vendor,
  1045							      hid->product);
  1046	
  1047		snprintf(hid->name, sizeof(hid->name), "%s %04X:%04X",
  1048			 client->name, (u16)hid->vendor, (u16)hid->product);
  1049		strscpy(hid->phys, dev_name(&client->dev), sizeof(hid->phys));
  1050	
  1051		ihid->quirks = i2c_hid_lookup_quirk(hid->vendor, hid->product);
  1052	
  1053		ret = hid_add_device(hid);
  1054		if (ret) {
  1055			if (ret != -ENODEV)
  1056				hid_err(client, "can't add hid device: %d\n", ret);
  1057			goto err;
  1058		}
  1059	
  1060		return 0;
  1061	
  1062	err:
  1063		i2c_hid_core_power_down(ihid);
  1064		return ret;
  1065	}
  1066	
> 1067	void ihid_core_panel_prepare_work(struct work_struct *work)
  1068	{
  1069		struct i2c_hid *ihid = container_of(work, struct i2c_hid,
  1070						    panel_follower_prepare_work);
  1071		struct hid_device *hid = ihid->hid;
  1072		int ret;
  1073	
  1074		/*
  1075		 * hid->version is set on the first power up. If it's still zero then
  1076		 * this is the first power on so we should perform initial power up
  1077		 * steps.
  1078		 */
  1079		if (!hid->version)
  1080			ret = i2c_hid_core_initial_power_up(ihid);
  1081		else
  1082			ret = i2c_hid_core_resume(ihid);
  1083	
  1084		if (ret)
  1085			dev_warn(&ihid->client->dev, "Power on failed: %d\n", ret);
  1086		else
  1087			WRITE_ONCE(ihid->prepare_work_finished, true);
  1088	
  1089		/* Match with i2c_hid_core_panel_unpreparing() */
  1090		smp_wmb();
  1091	}
  1092
kernel test robot May 24, 2023, 6:56 p.m. UTC | #2 
Hi Douglas,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on hid/for-next drm-misc/drm-misc-next linus/master v6.4-rc3 next-20230524]
[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/Douglas-Anderson/dt-bindings-HID-i2c-hid-Add-panel-property-to-i2c-hid-backed-panels/20230524-034323
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20230523122802.8.I962bb462ede779005341c49320740ed95810021d%40changeid
patch subject: [PATCH 8/9] HID: i2c-hid: Do panel follower work on the system_wq
config: i386-randconfig-s001
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/97c5984c98b7721d6c5299d8542c612e5c3240d3
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Douglas-Anderson/dt-bindings-HID-i2c-hid-Add-panel-property-to-i2c-hid-backed-panels/20230524-034323
        git checkout 97c5984c98b7721d6c5299d8542c612e5c3240d3
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 olddefconfig
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash drivers/hid/i2c-hid/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202305250203.PEElnTad-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
   drivers/hid/i2c-hid/i2c-hid-core.c:1013:5: sparse: sparse: symbol 'i2c_hid_core_initial_power_up' was not declared. Should it be static?
>> drivers/hid/i2c-hid/i2c-hid-core.c:1067:6: sparse: sparse: symbol 'ihid_core_panel_prepare_work' was not declared. Should it be static?
   drivers/hid/i2c-hid/i2c-hid-core.c:1093:5: sparse: sparse: symbol 'i2c_hid_core_panel_prepared' was not declared. Should it be static?
   drivers/hid/i2c-hid/i2c-hid-core.c:1107:5: sparse: sparse: symbol 'i2c_hid_core_panel_unpreparing' was not declared. Should it be static?
diff mbox series

Patch

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index f1bb89377e8d..800f0dc6f6cf 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -110,7 +110,9 @@  struct i2c_hid {
 
 	struct i2chid_ops	*ops;
 	struct drm_panel_follower panel_follower;
+	struct work_struct	panel_follower_prepare_work;
 	bool			is_panel_follower;
+	bool			prepare_work_finished;
 };
 
 static const struct i2c_hid_quirks {
@@ -1062,10 +1064,12 @@  int i2c_hid_core_initial_power_up(struct i2c_hid *ihid)
 	return ret;
 }
 
-int i2c_hid_core_panel_prepared(struct drm_panel_follower *follower)
+void ihid_core_panel_prepare_work(struct work_struct *work)
 {
-	struct i2c_hid *ihid = container_of(follower, struct i2c_hid, panel_follower);
+	struct i2c_hid *ihid = container_of(work, struct i2c_hid,
+					    panel_follower_prepare_work);
 	struct hid_device *hid = ihid->hid;
+	int ret;
 
 	/*
 	 * hid->version is set on the first power up. If it's still zero then
@@ -1073,15 +1077,44 @@  int i2c_hid_core_panel_prepared(struct drm_panel_follower *follower)
 	 * steps.
 	 */
 	if (!hid->version)
-		return i2c_hid_core_initial_power_up(ihid);
+		ret = i2c_hid_core_initial_power_up(ihid);
+	else
+		ret = i2c_hid_core_resume(ihid);
 
-	return i2c_hid_core_resume(ihid);
+	if (ret)
+		dev_warn(&ihid->client->dev, "Power on failed: %d\n", ret);
+	else
+		WRITE_ONCE(ihid->prepare_work_finished, true);
+
+	/* Match with i2c_hid_core_panel_unpreparing() */
+	smp_wmb();
+}
+
+int i2c_hid_core_panel_prepared(struct drm_panel_follower *follower)
+{
+	struct i2c_hid *ihid = container_of(follower, struct i2c_hid, panel_follower);
+
+	/*
+	 * Powering on a touchscreen can be a slow process. Queue the work to
+	 * the system workqueue so we don't block the panel's power up.
+	 */
+	WRITE_ONCE(ihid->prepare_work_finished, false);
+	schedule_work(&ihid->panel_follower_prepare_work);
+
+	return 0;
 }
 
 int i2c_hid_core_panel_unpreparing(struct drm_panel_follower *follower)
 {
 	struct i2c_hid *ihid = container_of(follower, struct i2c_hid, panel_follower);
 
+	cancel_work_sync(&ihid->panel_follower_prepare_work);
+
+	/* Match with ihid_core_panel_prepare_work() */
+	smp_rmb();
+	if (!READ_ONCE(ihid->prepare_work_finished))
+		return 0;
+
 	return i2c_hid_core_suspend(ihid);
 }
 
@@ -1124,6 +1157,7 @@  int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
 
 	init_waitqueue_head(&ihid->wait);
 	mutex_init(&ihid->reset_lock);
+	INIT_WORK(&ihid->panel_follower_prepare_work, ihid_core_panel_prepare_work);
 
 	/* we need to allocate the command buffer without knowing the maximum
 	 * size of the reports. Let's use HID_MIN_BUFFER_SIZE, then we do the