diff options
| author | Michal Schmidt <mschmidt@redhat.com> | 2023-06-06 10:12:53 -0700 |
|---|---|---|
| committer | David S. Miller <davem@davemloft.net> | 2023-06-07 10:51:58 +0100 |
| commit | bf15bb38ec7f4ff522da5c20e1673dbda7159938 (patch) | |
| tree | e012fb729354d7f343b9f277938f4ccd7381c33c /drivers/net/ethernet/intel/ice/ice_gnss.c | |
| parent | d636fc5dd692c8f4e00ae6e0359c0eceeb5d9bdb (diff) | |
| download | lwn-bf15bb38ec7f4ff522da5c20e1673dbda7159938.tar.gz lwn-bf15bb38ec7f4ff522da5c20e1673dbda7159938.zip | |
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>
Diffstat (limited to 'drivers/net/ethernet/intel/ice/ice_gnss.c')
| -rw-r--r-- | drivers/net/ethernet/intel/ice/ice_gnss.c | 64 |
1 files changed, 4 insertions, 60 deletions
diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c index 2ea8a2b11bcd..bd0ed155e11b 100644 --- a/drivers/net/ethernet/intel/ice/ice_gnss.c +++ b/drivers/net/ethernet/intel/ice/ice_gnss.c @@ -16,8 +16,8 @@ * * number of bytes written - success * * negative - error code */ -static unsigned int -ice_gnss_do_write(struct ice_pf *pf, unsigned char *buf, unsigned int size) +static int +ice_gnss_do_write(struct ice_pf *pf, const unsigned char *buf, unsigned int size) { struct ice_aqc_link_topo_addr link_topo; struct ice_hw *hw = &pf->hw; @@ -72,39 +72,7 @@ err_out: dev_err(ice_pf_to_dev(pf), "GNSS failed to write, offset=%u, size=%u, err=%d\n", offset, size, err); - return offset; -} - -/** - * ice_gnss_write_pending - Write all pending data to internal GNSS - * @work: GNSS write work structure - */ -static void ice_gnss_write_pending(struct kthread_work *work) -{ - struct gnss_serial *gnss = container_of(work, struct gnss_serial, - write_work); - struct ice_pf *pf = gnss->back; - - if (!pf) - return; - - if (!test_bit(ICE_FLAG_GNSS, pf->flags)) - return; - - if (!list_empty(&gnss->queue)) { - struct gnss_write_buf *write_buf = NULL; - unsigned int bytes; - - write_buf = list_first_entry(&gnss->queue, - struct gnss_write_buf, queue); - - bytes = ice_gnss_do_write(pf, write_buf->buf, write_buf->size); - dev_dbg(ice_pf_to_dev(pf), "%u bytes written to GNSS\n", bytes); - - list_del(&write_buf->queue); - kfree(write_buf->buf); - kfree(write_buf); - } + return err; } /** @@ -220,8 +188,6 @@ static struct gnss_serial *ice_gnss_struct_init(struct ice_pf *pf) pf->gnss_serial = gnss; kthread_init_delayed_work(&gnss->read_work, ice_gnss_read); - INIT_LIST_HEAD(&gnss->queue); - kthread_init_work(&gnss->write_work, ice_gnss_write_pending); kworker = kthread_create_worker(0, "ice-gnss-%s", dev_name(dev)); if (IS_ERR(kworker)) { kfree(gnss); @@ -281,7 +247,6 @@ static void ice_gnss_close(struct gnss_device *gdev) if (!gnss) return; - kthread_cancel_work_sync(&gnss->write_work); kthread_cancel_delayed_work_sync(&gnss->read_work); } @@ -300,10 +265,7 @@ ice_gnss_write(struct gnss_device *gdev, const unsigned char *buf, size_t count) { struct ice_pf *pf = gnss_get_drvdata(gdev); - struct gnss_write_buf *write_buf; struct gnss_serial *gnss; - unsigned char *cmd_buf; - int err = count; /* We cannot write a single byte using our I2C implementation. */ if (count <= 1 || count > ICE_GNSS_TTY_WRITE_BUF) @@ -319,24 +281,7 @@ ice_gnss_write(struct gnss_device *gdev, const unsigned char *buf, if (!gnss) return -ENODEV; - cmd_buf = kcalloc(count, sizeof(*buf), GFP_KERNEL); - if (!cmd_buf) - return -ENOMEM; - - memcpy(cmd_buf, buf, count); - write_buf = kzalloc(sizeof(*write_buf), GFP_KERNEL); - if (!write_buf) { - kfree(cmd_buf); - return -ENOMEM; - } - - write_buf->buf = cmd_buf; - write_buf->size = count; - INIT_LIST_HEAD(&write_buf->queue); - list_add_tail(&write_buf->queue, &gnss->queue); - kthread_queue_work(gnss->kworker, &gnss->write_work); - - return err; + return ice_gnss_do_write(pf, buf, count); } static const struct gnss_operations ice_gnss_ops = { @@ -432,7 +377,6 @@ void ice_gnss_exit(struct ice_pf *pf) if (pf->gnss_serial) { struct gnss_serial *gnss = pf->gnss_serial; - kthread_cancel_work_sync(&gnss->write_work); kthread_cancel_delayed_work_sync(&gnss->read_work); kthread_destroy_worker(gnss->kworker); gnss->kworker = NULL; |