| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 14 Apr 2023 17:29:10 +0000
From: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
To: mschmidt <mschmidt@...hat.com>,
"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"Brandeburg, Jesse" <jesse.brandeburg@...el.com>,
"Kolacinski, Karol" <karol.kolacinski@...el.com>,
"Nguyen, Anthony L" <anthony.l.nguyen@...el.com>,
Simon Horman <simon.horman@...igine.com>,
"Michalik, Michal" <michal.michalik@...el.com>,
poros <poros@...hat.com>, Andrew Lunn <andrew@...n.ch>
Subject: RE: [PATCH net-next v2 1/6] ice: do not busy-wait to read GNSS data
>From: Michal Schmidt <mschmidt@...hat.com>
>Sent: Wednesday, April 12, 2023 10:19 AM
>
>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@...hat.com>
>---
> drivers/net/ethernet/intel/ice/ice_gnss.c | 42 ++++++++++-------------
> drivers/net/ethernet/intel/ice/ice_gnss.h | 3 +-
> 2 files changed, 20 insertions(+), 25 deletions(-)
>
>diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c
>b/drivers/net/ethernet/intel/ice/ice_gnss.c
>index 8dec748bb53a..2ea8a2b11bcd 100644
>--- a/drivers/net/ethernet/intel/ice/ice_gnss.c
>+++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
>@@ -117,6 +117,7 @@ static void ice_gnss_read(struct kthread_work *work)
> {
> struct gnss_serial *gnss = container_of(work, struct gnss_serial,
> read_work.work);
>+ unsigned long delay = ICE_GNSS_POLL_DATA_DELAY_TIME;
> unsigned int i, bytes_read, data_len, count;
> struct ice_aqc_link_topo_addr link_topo;
> struct ice_pf *pf;
>@@ -136,11 +137,6 @@ static void ice_gnss_read(struct kthread_work *work)
> return;
>
> hw = &pf->hw;
>- buf = (char *)get_zeroed_page(GFP_KERNEL);
>- if (!buf) {
>- err = -ENOMEM;
>- goto exit;
>- }
>
> memset(&link_topo, 0, sizeof(struct ice_aqc_link_topo_addr));
> link_topo.topo_params.index = ICE_E810T_GNSS_I2C_BUS;
>@@ -151,25 +147,24 @@ static void ice_gnss_read(struct kthread_work *work)
> i2c_params = ICE_GNSS_UBX_DATA_LEN_WIDTH |
> ICE_AQC_I2C_USE_REPEATED_START;
>
>- /* Read data length in a loop, when it's not 0 the data is ready */
>- for (i = 0; i < ICE_MAX_UBX_READ_TRIES; i++) {
>- err = ice_aq_read_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
>- cpu_to_le16(ICE_GNSS_UBX_DATA_LEN_H),
>- i2c_params, (u8 *)&data_len_b, NULL);
>- if (err)
>- goto exit_buf;
>+ err = ice_aq_read_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
>+ cpu_to_le16(ICE_GNSS_UBX_DATA_LEN_H),
>+ i2c_params, (u8 *)&data_len_b, NULL);
>+ if (err)
>+ goto requeue;
>
>- data_len = be16_to_cpu(data_len_b);
>- if (data_len != 0 && data_len != U16_MAX)
>- break;
>+ data_len = be16_to_cpu(data_len_b);
>+ if (data_len == 0 || data_len == U16_MAX)
>+ goto requeue;
>
>- mdelay(10);
>- }
>+ /* The u-blox has data_len bytes for us to read */
>
> data_len = min_t(typeof(data_len), data_len, PAGE_SIZE);
>- if (!data_len) {
>+
>+ buf = (char *)get_zeroed_page(GFP_KERNEL);
>+ if (!buf) {
> err = -ENOMEM;
>- goto exit_buf;
>+ goto requeue;
> }
>
> /* Read received data */
>@@ -183,7 +178,7 @@ static void ice_gnss_read(struct kthread_work *work)
> cpu_to_le16(ICE_GNSS_UBX_EMPTY_DATA),
> bytes_read, &buf[i], NULL);
> if (err)
>- goto exit_buf;
>+ goto free_buf;
> }
>
> count = gnss_insert_raw(pf->gnss_dev, buf, i);
>@@ -191,10 +186,11 @@ static void ice_gnss_read(struct kthread_work *work)
> dev_warn(ice_pf_to_dev(pf),
> "gnss_insert_raw ret=%d size=%d\n",
> count, i);
>-exit_buf:
>+ delay = ICE_GNSS_TIMER_DELAY_TIME;
>+free_buf:
> free_page((unsigned long)buf);
>- kthread_queue_delayed_work(gnss->kworker, &gnss->read_work,
>- ICE_GNSS_TIMER_DELAY_TIME);
>+requeue:
>+ kthread_queue_delayed_work(gnss->kworker, &gnss->read_work, delay);
> exit:
> if (err)
> dev_dbg(ice_pf_to_dev(pf), "GNSS failed to read err=%d\n",
>err);
>diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.h
>b/drivers/net/ethernet/intel/ice/ice_gnss.h
>index 4d49e5b0b4b8..640df7411373 100644
>--- a/drivers/net/ethernet/intel/ice/ice_gnss.h
>+++ b/drivers/net/ethernet/intel/ice/ice_gnss.h
>@@ -5,6 +5,7 @@
> #define _ICE_GNSS_H_
>
> #define ICE_E810T_GNSS_I2C_BUS 0x2
>+#define ICE_GNSS_POLL_DATA_DELAY_TIME (HZ / 100) /* poll every 10 ms */
> #define ICE_GNSS_TIMER_DELAY_TIME (HZ / 10) /* 0.1 second per message */
> #define ICE_GNSS_TTY_WRITE_BUF 250
> #define ICE_MAX_I2C_DATA_SIZE
> FIELD_MAX(ICE_AQC_I2C_DATA_SIZE_M)
>@@ -20,8 +21,6 @@
> * passed as I2C addr parameter.
> */
> #define ICE_GNSS_UBX_WRITE_BYTES (ICE_MAX_I2C_WRITE_BYTES + 1)
>-#define ICE_MAX_UBX_READ_TRIES 255
>-#define ICE_MAX_UBX_ACK_READ_TRIES 4095
>
> struct gnss_write_buf {
> struct list_head queue;
>--
>2.39.2
Looks good, thank you Michal!
Reviewed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
Powered by blists - more mailing lists