Skip to content

Commit

Permalink
ice: make writes to /dev/gnssX synchronous
Browse files Browse the repository at this point in the history
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: d6b98c8 ("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>
  • Loading branch information
michich authored and davem330 committed Jun 7, 2023
1 parent d636fc5 commit bf15bb3
Show file tree
Hide file tree
Showing 4 changed files with 6 additions and 72 deletions.
2 changes: 1 addition & 1 deletion drivers/net/ethernet/intel/ice/ice_common.c
Original file line number Diff line number Diff line change
Expand Up @@ -5160,7 +5160,7 @@ ice_aq_read_i2c(struct ice_hw *hw, struct ice_aqc_link_topo_addr topo_addr,
*/
int
ice_aq_write_i2c(struct ice_hw *hw, struct ice_aqc_link_topo_addr topo_addr,
u16 bus_addr, __le16 addr, u8 params, u8 *data,
u16 bus_addr, __le16 addr, u8 params, const u8 *data,
struct ice_sq_cd *cd)
{
struct ice_aq_desc desc = { 0 };
Expand Down
2 changes: 1 addition & 1 deletion drivers/net/ethernet/intel/ice/ice_common.h
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ ice_aq_read_i2c(struct ice_hw *hw, struct ice_aqc_link_topo_addr topo_addr,
struct ice_sq_cd *cd);
int
ice_aq_write_i2c(struct ice_hw *hw, struct ice_aqc_link_topo_addr topo_addr,
u16 bus_addr, __le16 addr, u8 params, u8 *data,
u16 bus_addr, __le16 addr, u8 params, const u8 *data,
struct ice_sq_cd *cd);
bool ice_fw_supports_report_dflt_cfg(struct ice_hw *hw);
#endif /* _ICE_COMMON_H_ */
64 changes: 4 additions & 60 deletions drivers/net/ethernet/intel/ice/ice_gnss.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -72,39 +72,7 @@ ice_gnss_do_write(struct ice_pf *pf, unsigned char *buf, unsigned int size)
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;
}

/**
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}

Expand All @@ -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)
Expand All @@ -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 = {
Expand Down Expand Up @@ -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;
Expand Down
10 changes: 0 additions & 10 deletions drivers/net/ethernet/intel/ice/ice_gnss.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,26 +22,16 @@
*/
#define ICE_GNSS_UBX_WRITE_BYTES (ICE_MAX_I2C_WRITE_BYTES + 1)

struct gnss_write_buf {
struct list_head queue;
unsigned int size;
unsigned char *buf;
};

/**
* struct gnss_serial - data used to initialize GNSS TTY port
* @back: back pointer to PF
* @kworker: kwork thread for handling periodic work
* @read_work: read_work function for handling GNSS reads
* @write_work: write_work function for handling GNSS writes
* @queue: write buffers queue
*/
struct gnss_serial {
struct ice_pf *back;
struct kthread_worker *kworker;
struct kthread_delayed_work read_work;
struct kthread_work write_work;
struct list_head queue;
};

#if IS_ENABLED(CONFIG_GNSS)
Expand Down

0 comments on commit bf15bb3

Please sign in to comment.