#
bf15bb38 |
|
06-Jun-2023 |
Michal Schmidt <mschmidt@redhat.com> |
ice: make writes to /dev/gnssX synchronous The current ice driver's GNSS write implementation buffers writes and works through them asynchronously in a kthread. That's bad because: - The GNSS write_raw operation is supposed to be synchronous[1][2]. - There is no upper bound on the number of pending writes. Userspace can submit writes much faster than the driver can process, consuming unlimited amounts of kernel memory. A patch that's currently on review[3] ("[v3,net] ice: Write all GNSS buffers instead of first one") would add one more problem: - The possibility of waiting for a very long time to flush the write work when doing rmmod, softlockups. To fix these issues, simplify the implementation: Drop the buffering, the write_work, and make the writes synchronous. I tested this with gpsd and ubxtool. [1] https://events19.linuxfoundation.org/wp-content/uploads/2017/12/The-GNSS-Subsystem-Johan-Hovold-Hovold-Consulting-AB.pdf "User interface" slide. [2] A comment in drivers/gnss/core.c:gnss_write(): /* Ignoring O_NONBLOCK, write_raw() is synchronous. */ [3] https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20230217120541.16745-1-karol.kolacinski@intel.com/ Fixes: d6b98c8d242a ("ice: add write functionality for GNSS TTY") Signed-off-by: Michal Schmidt <mschmidt@redhat.com> Reviewed-by: Simon Horman <simon.horman@corigine.com> Tested-by: Sunitha Mekala <sunithax.d.mekala@intel.com> (A Contingent worker at Intel) Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com> Signed-off-by: David S. Miller <davem@davemloft.net>
|
#
0ec636e5 |
|
12-Apr-2023 |
Michal Schmidt <mschmidt@redhat.com> |
ice: increase the GNSS data polling interval to 20 ms Double the GNSS data polling interval from 10 ms to 20 ms. According to Karol Kolacinski from the Intel team, they have been planning to make this change. Signed-off-by: Michal Schmidt <mschmidt@redhat.com> Reviewed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> Reviewed-by: Simon Horman <simon.horman@corigine.com> Tested-by: Sunitha Mekala <sunithax.d.mekala@intel.com> (A Contingent worker at Intel) Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
|
#
2f8fdcb0 |
|
12-Apr-2023 |
Michal Schmidt <mschmidt@redhat.com> |
ice: do not busy-wait to read GNSS data The ice-gnss-<dev_name> kernel thread, which reads data from the u-blox GNSS module, keep a CPU core almost 100% busy. The main reason is that it busy-waits for data to become available. A simple improvement would be to replace the "mdelay(10);" in ice_gnss_read() with sleeping. A better fix is to not do any waiting directly in the function and just requeue this delayed work as needed. The advantage is that canceling the work from ice_gnss_exit() becomes immediate, rather than taking up to ~2.5 seconds (ICE_MAX_UBX_READ_TRIES * 10 ms). This lowers the CPU usage of the ice-gnss-<dev_name> thread on my system from ~90 % to ~8 %. I am not sure if the larger 0.1 s pause after inserting data into the gnss subsystem is really necessary, but I'm keeping that as it was. Of course, ideally the driver would not have to poll at all, but I don't know if the E810 can watch for GNSS data availability over the i2c bus by itself and notify the driver. Signed-off-by: Michal Schmidt <mschmidt@redhat.com> Reviewed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> Reviewed-by: Simon Horman <simon.horman@corigine.com> Tested-by: Sunitha Mekala <sunithax.d.mekala@intel.com> (A Contingent worker at Intel) Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
|
#
cf871006 |
|
24-Feb-2023 |
Jacob Keller <jacob.e.keller@intel.com> |
ice: remove unnecessary CONFIG_ICE_GNSS CONFIG_ICE_GNSS was added by commit c7ef8221ca7d ("ice: use GNSS subsystem instead of TTY") as a way to allow the ice driver to optionally support GNSS features without forcing a dependency on CONFIG_GNSS. The original implementation of that commit at [1] used IS_REACHABLE. This was rejected by Olek at [2] with the suggested implementation of CONFIG_ICE_GNSS. Eventually after merging, Linus reported a .config which had CONFIG_ICE_GNSS = y when both GNSS = n and ICE = n. This confused him and he felt that the config option was not useful, and commented about it at [3]. CONFIG_ICE_GNSS is defined to y whenever GNSS = ICE. This results in it being set in cases where both options are not enabled. The goal of CONFIG_ICE_GNSS is to ensure that the GNSS support in the ice driver is enabled when GNSS is enabled. The complaint from Olek about the original IS_REACHABLE was due to the required IS_REACHABLE checks throughout the ice driver code and the fact that ice_gnss.c was compiled regardless of GNSS support. This can be fixed in the Makefile by using ice-$(CONFIG_GNSS) += ice_gnss.o In this case, if GNSS = m and ICE = y, we can result in some confusing behavior where GNSS support is not enabled because its not built in. See [4]. To disallow this, have CONFIG_ICE depend on GNSS || GNSS = n. This ensures that we cannot enable CONFIG_ICE as builtin while GNSS is a module. Drop CONFIG_ICE_GNSS, and replace the IS_ENABLED checks for it with checks for GNSS. Update the Makefile to add the ice_gnss.o object based on CONFIG_GNSS. This works to ensure that GNSS support can optionally be enabled, doesn't have an unnnecessary extra config option, and has Kbuild enforce the dependency such that you can't accidentally enable GNSS as a module and ICE as a builtin. [1] https://lore.kernel.org/intel-wired-lan/20221019095603.44825-1-arkadiusz.kubalewski@intel.com/ [2] https://lore.kernel.org/intel-wired-lan/20221028165706.96849-1-alexandr.lobakin@intel.com/ [3] https://lore.kernel.org/all/CAHk-=wi_410KZqHwF-WL5U7QYxnpHHHNP-3xL=g_y89XnKc-uw@mail.gmail.com/ [4] https://lore.kernel.org/netdev/20230223161309.0e439c5f@kernel.org/ Reported-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> Fixes: c7ef8221ca7d ("ice: use GNSS subsystem instead of TTY") Cc: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> Cc: Alexander Lobakin <alexandr.lobakin@intel.com> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Anthony Nguyen <anthony.l.nguyen@intel.com> Acked-by: Jakub Kicinski <kuba@kernel.org> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com> Signed-off-by: David S. Miller <davem@davemloft.net>
|
#
c7ef8221 |
|
18-Jan-2023 |
Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> |
ice: use GNSS subsystem instead of TTY Previously support for GNSS was implemented as a TTY driver, it allowed to access GNSS receiver on /dev/ttyGNSS_<bus><func>. Use generic GNSS subsystem API instead of implementing own TTY driver. The receiver is accessible on /dev/gnss<id>. In case of multiple receivers in the OS, correct device can be found by enumerating either: - /sys/class/net/<eth port>/device/gnss/ - /sys/class/gnss/gnss<id>/device/ Using GNSS subsystem is superior to implementing own TTY driver, as the GNSS subsystem was designed solely for this purpose. It also implements TTY driver but in a common and defined way. From user perspective, there is no difference in communicating with a device, except new path to the device shall be used. The device will provide same information to the userspace as the old one, and can be used in the same way, i.e.: old # gpsmon /dev/ttyGNSS_2100_0 new # gpsmon /dev/gnss0 There is no other impact on userspace tools. User expecting onboard GNSS receiver support is required to enable CONFIG_GNSS=y/m in kernel config. Reviewed-by: Alexander Lobakin <alexandr.lobakin@intel.com> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> Signed-off-by: Michal Michalik <michal.michalik@intel.com> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> Tested-by: Gurucharan G <gurucharanx.g@intel.com> (A Contingent worker at Intel) Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Signed-off-by: David S. Miller <davem@davemloft.net>
|
#
d6b98c8d |
|
24-Jun-2022 |
Karol Kolacinski <karol.kolacinski@intel.com> |
ice: add write functionality for GNSS TTY Add the possibility to write raw bytes to the GNSS module through the first TTY device. This allows user to configure the module. Create a second read-only TTY device. Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker at Intel) Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
|
#
43113ff7 |
|
01-Mar-2022 |
Karol Kolacinski <karol.kolacinski@intel.com> |
ice: add TTY for GNSS module for E810T device Add a new ice_gnss.c file for holding the basic GNSS module functions. If the device supports GNSS module, call the new ice_gnss_init and ice_gnss_release functions where appropriate. Implement basic functionality for reading the data from GNSS module using TTY device. Add I2C read AQ command. It is now required for controlling the external physical connectors via external I2C port expander on E810-T adapters. Future changes will introduce write functionality. Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> Signed-off-by: Sudhansu Sekhar Mishra <sudhansu.mishra@intel.com> Tested-by: Sunitha Mekala <sunithax.d.mekala@intel.com> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com> Signed-off-by: David S. Miller <davem@davemloft.net>
|