| 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: <23018193-6db4-c0be-05a8-ed68853bd2ff@amd.com>
Date: Thu, 3 Jul 2025 12:29:45 +0530
From: Abhijit Gangurde <abhijit.gangurde@....com>
To: Leon Romanovsky <leon@...nel.org>
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 7/1/25 15:54, Leon Romanovsky wrote:
> 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.
I will keep it minimal.
>
>> +
>> +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.
sure
>
>> + *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.
Will correct this.
>
>> + 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.
RDMA driver is requesting the reset via eth driver which holds the
privilege.
>
>> + 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.
Will remove the explicit NULLs.
>
>> + 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.
admin_state within the admin queues is protected by the spinlock,
hence it is used as enum_admin_state. However device's admin_state
is used as as atomic to avoid reset race of reset.
>
>> + 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?
This is to protect kref_get against free of cq in destroy_cq.
>
>> +
>> + if (!cq) {
> Is it possible?
Possible when HCA goes bad.
>
>> + 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.
sure.
>
>> +#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
I will remove this.
Thanks,
Abhijit.
Powered by blists - more mailing lists