[8/9] HID: i2c-hid: Do panel follower work on the system_wq
Commit Message
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
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
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?
@@ -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