| 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
| ||
|
Message-ID: <20250701102409.GA118736@unreal>
Date: Tue, 1 Jul 2025 13:24:09 +0300
From: Leon Romanovsky <leon@...nel.org>
To: Abhijit Gangurde <abhijit.gangurde@....com>
Cc: shannon.nelson@....com, brett.creeley@....com, davem@...emloft.net,
edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
corbet@....net, jgg@...pe.ca, andrew+netdev@...n.ch,
allen.hubbe@....com, nikhil.agarwal@....com,
linux-rdma@...r.kernel.org, netdev@...r.kernel.org,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
Andrew Boyer <andrew.boyer@....com>
Subject: Re: [PATCH v3 09/14] RDMA/ionic: Create device queues to support
admin operations
On Tue, Jun 24, 2025 at 05:43:10PM +0530, Abhijit Gangurde wrote:
> Setup RDMA admin queues using device command exposed over
> auxiliary device and manage these queues using ida.
>
> Co-developed-by: Andrew Boyer <andrew.boyer@....com>
> Signed-off-by: Andrew Boyer <andrew.boyer@....com>
> Co-developed-by: Allen Hubbe <allen.hubbe@....com>
> Signed-off-by: Allen Hubbe <allen.hubbe@....com>
> Signed-off-by: Abhijit Gangurde <abhijit.gangurde@....com>
> ---
> v2->v3
> - Fixed lockdep warning
> - Used IDA for resource id allocation
> - Removed rw locks around xarrays
>
> drivers/infiniband/hw/ionic/ionic_admin.c | 1169 +++++++++++++++++
> .../infiniband/hw/ionic/ionic_controlpath.c | 184 +++
> drivers/infiniband/hw/ionic/ionic_fw.h | 164 +++
> drivers/infiniband/hw/ionic/ionic_ibdev.c | 56 +
> drivers/infiniband/hw/ionic/ionic_ibdev.h | 225 ++++
> drivers/infiniband/hw/ionic/ionic_pgtbl.c | 113 ++
> drivers/infiniband/hw/ionic/ionic_queue.c | 52 +
> drivers/infiniband/hw/ionic/ionic_queue.h | 234 ++++
> drivers/infiniband/hw/ionic/ionic_res.h | 154 +++
> 9 files changed, 2351 insertions(+)
> create mode 100644 drivers/infiniband/hw/ionic/ionic_admin.c
> create mode 100644 drivers/infiniband/hw/ionic/ionic_controlpath.c
> create mode 100644 drivers/infiniband/hw/ionic/ionic_fw.h
> create mode 100644 drivers/infiniband/hw/ionic/ionic_pgtbl.c
> create mode 100644 drivers/infiniband/hw/ionic/ionic_queue.c
> create mode 100644 drivers/infiniband/hw/ionic/ionic_queue.h
> create mode 100644 drivers/infiniband/hw/ionic/ionic_res.h
<...>
> +static void ionic_admin_timedout(struct ionic_aq *aq)
> +{
> + struct ionic_cq *cq = &aq->vcq->cq[0];
> + struct ionic_ibdev *dev = aq->dev;
> + unsigned long irqflags;
> + u16 pos;
> +
> + spin_lock_irqsave(&aq->lock, irqflags);
> + if (ionic_queue_empty(&aq->q))
> + goto out;
> +
> + /* Reset ALL adminq if any one times out */
> + if (aq->admin_state < IONIC_ADMIN_KILLED)
> + queue_work(ionic_evt_workq, &dev->reset_work);
> +
> + ibdev_err(&dev->ibdev, "admin command timed out, aq %d\n", aq->aqid);
> +
> + ibdev_warn(&dev->ibdev, "admin timeout was set for %ums\n",
> + (u32)jiffies_to_msecs(IONIC_ADMIN_TIMEOUT));
> + ibdev_warn(&dev->ibdev, "admin inactivity for %ums\n",
> + (u32)jiffies_to_msecs(jiffies - aq->stamp));
> +
> + ibdev_warn(&dev->ibdev, "admin commands outstanding %u\n",
> + ionic_queue_length(&aq->q));
> + ibdev_warn(&dev->ibdev, "%s more commands pending\n",
> + list_empty(&aq->wr_post) ? "no" : "some");
> +
> + pos = cq->q.prod;
> +
> + ibdev_warn(&dev->ibdev, "admin cq pos %u (next to complete)\n", pos);
> + print_hex_dump(KERN_WARNING, "cqe ", DUMP_PREFIX_OFFSET, 16, 1,
> + ionic_queue_at(&cq->q, pos),
> + BIT(cq->q.stride_log2), true);
> +
> + pos = (pos - 1) & cq->q.mask;
> +
> + ibdev_warn(&dev->ibdev, "admin cq pos %u (last completed)\n", pos);
> + print_hex_dump(KERN_WARNING, "cqe ", DUMP_PREFIX_OFFSET, 16, 1,
> + ionic_queue_at(&cq->q, pos),
> + BIT(cq->q.stride_log2), true);
> +
> + pos = aq->q.cons;
> +
> + ibdev_warn(&dev->ibdev, "admin pos %u (next to complete)\n", pos);
> + print_hex_dump(KERN_WARNING, "cmd ", DUMP_PREFIX_OFFSET, 16, 1,
> + ionic_queue_at(&aq->q, pos),
> + BIT(aq->q.stride_log2), true);
> +
> + pos = (aq->q.prod - 1) & aq->q.mask;
> + if (pos == aq->q.cons)
> + goto out;
> +
> + ibdev_warn(&dev->ibdev, "admin pos %u (last posted)\n", pos);
> + print_hex_dump(KERN_WARNING, "cmd ", DUMP_PREFIX_OFFSET, 16, 1,
> + ionic_queue_at(&aq->q, pos),
> + BIT(aq->q.stride_log2), true);
> +
> +out:
> + spin_unlock_irqrestore(&aq->lock, irqflags);
> +}
Please reduce number of debug prints. You are supposed to send driver
that works and not for the debug session.
> +
> +static void ionic_admin_reset_dwork(struct ionic_ibdev *dev)
> +{
> + if (atomic_read(&dev->admin_state) >= IONIC_ADMIN_KILLED)
> + return;
<...>
> + if (aq->admin_state >= IONIC_ADMIN_KILLED)
> + return;
<...>
> + ibdev_dbg(&dev->ibdev, "poll admin cq %u prod %u\n",
> + cq->cqid, cq->q.prod);
> + print_hex_dump_debug("cqe ", DUMP_PREFIX_OFFSET, 16, 1,
> + qcqe, BIT(cq->q.stride_log2), true);
We have restrack to print CQE and other objects, please use it.
> + *cqe = qcqe;
> +
> + return true;
> +}
> +
> +static void ionic_admin_poll_locked(struct ionic_aq *aq)
> +{
> + struct ionic_cq *cq = &aq->vcq->cq[0];
> + struct ionic_admin_wr *wr, *wr_next;
> + struct ionic_ibdev *dev = aq->dev;
> + u32 wr_strides, avlbl_strides;
> + struct ionic_v1_cqe *cqe;
> + u32 qtf, qid;
> + u16 old_prod;
> + u8 type;
> +
> + lockdep_assert_held(&aq->lock);
> +
> + if (aq->admin_state >= IONIC_ADMIN_KILLED) {
IONIC_ADMIN_KILLED is the last, there is no ">" option.
> + list_for_each_entry_safe(wr, wr_next, &aq->wr_prod, aq_ent) {
> + INIT_LIST_HEAD(&wr->aq_ent);
> + aq->q_wr[wr->status].wr = NULL;
> + wr->status = aq->admin_state;
> + complete_all(&wr->work);
> + }
> + INIT_LIST_HEAD(&aq->wr_prod);
<...>
> + if (do_reset)
> + /* Reset device on a timeout */
> + ionic_admin_timedout(bad_aq);
I wonder why RDMA driver resets device and not the one who owns PCI.
> + else if (do_reschedule)
> + /* Try to poll again later */
> + ionic_admin_reset_dwork(dev);
> +}
<...>
> + vcq = kzalloc(sizeof(*vcq), GFP_KERNEL);
> + if (!vcq) {
> + rc = -ENOMEM;
> + goto err_alloc;
> + }
> +
> + vcq->ibcq.device = &dev->ibdev;
> + vcq->ibcq.uobject = NULL;
1. There is no need in explicit NULL here, vcq was allocated with kzalloc()
2. Maybe rdma_zalloc_drv_obj() should be used here.
> + vcq->ibcq.comp_handler = ionic_rdma_admincq_comp;
> + vcq->ibcq.event_handler = ionic_rdma_admincq_event;
> + vcq->ibcq.cq_context = NULL;
> + atomic_set(&vcq->ibcq.usecnt, 0);
<...>
> + aq->admin_state = IONIC_ADMIN_KILLED;
<...>
> + old_state = atomic_cmpxchg(&dev->admin_state, IONIC_ADMIN_ACTIVE,
> + IONIC_ADMIN_PAUSED);
> + if (old_state != IONIC_ADMIN_ACTIVE)
In all these places you are mixing enum_admin_state and atomic_t for
same values, but different variable. Please chose or atomic_t or enum.
> + return;
> +
> + /* Pause all the AQs */
> + local_irq_save(irqflags);
> + for (i = 0; i < dev->lif_cfg.aq_count; i++) {
> + struct ionic_aq *aq = dev->aq_vec[i];
> +
> + spin_lock(&aq->lock);
> + /* pause rdma admin queues to reset device */
> + if (aq->admin_state == IONIC_ADMIN_ACTIVE)
> + aq->admin_state = IONIC_ADMIN_PAUSED;
> + spin_unlock(&aq->lock);
> + }
> + local_irq_restore(irqflags);
> +
> + rc = ionic_rdma_reset_devcmd(dev);
> + if (unlikely(rc)) {
> + ibdev_err(&dev->ibdev, "failed to reset rdma %d\n", rc);
> + ionic_request_rdma_reset(dev->lif_cfg.lif);
> + }
> +
> + ionic_kill_ibdev(dev, fatal_path);
> +}
<...>
> +static void ionic_cq_event(struct ionic_ibdev *dev, u32 cqid, u8 code)
> +{
> + struct ib_event ibev;
> + struct ionic_cq *cq;
> +
> + rcu_read_lock();
> + cq = xa_load(&dev->cq_tbl, cqid);
> + if (cq)
> + kref_get(&cq->cq_kref);
> + rcu_read_unlock();
What and how does this RCU protect?
> +
> + if (!cq) {
Is it possible?
> + ibdev_dbg(&dev->ibdev,
> + "missing cqid %#x code %u\n", cqid, code);
> + return;
> + }
<...>
> module_init(ionic_mod_init);
> diff --git a/drivers/infiniband/hw/ionic/ionic_ibdev.h b/drivers/infiniband/hw/ionic/ionic_ibdev.h
> index e13adff390d7..e7563c0429fc 100644
> --- a/drivers/infiniband/hw/ionic/ionic_ibdev.h
> +++ b/drivers/infiniband/hw/ionic/ionic_ibdev.h
> @@ -4,18 +4,243 @@
> #ifndef _IONIC_IBDEV_H_
> #define _IONIC_IBDEV_H_
>
> +#include <rdma/ib_umem.h>
> #include <rdma/ib_verbs.h>
> +
> #include <ionic_api.h>
> +#include <ionic_regs.h>
> +
> +#include "ionic_fw.h"
> +#include "ionic_queue.h"
> +#include "ionic_res.h"
>
> #include "ionic_lif_cfg.h"
>
> +#define DRIVER_NAME "ionic_rdma"
It is KBUILD_MODNAME, please use it.
> +#define DRIVER_SHORTNAME "ionr"
> +
> #define IONIC_MIN_RDMA_VERSION 0
> #define IONIC_MAX_RDMA_VERSION 2
Nothing from the above is applicable to upstream code.
Thanks
Powered by blists - more mailing lists