| 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: <6fb7e2cf-e26d-4af5-84e4-2c56c184a1df@amd.com>
Date: Mon, 16 Sep 2024 13:55:55 -0500
From: Wei Huang <wei.huang2@....com>
To: Alejandro Lucero Palau <alucerop@....com>, linux-pci@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
netdev@...r.kernel.org
Cc: Jonathan.Cameron@...wei.com, helgaas@...nel.org, corbet@....net,
davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, alex.williamson@...hat.com, gospo@...adcom.com,
michael.chan@...adcom.com, ajit.khaparde@...adcom.com,
somnath.kotur@...adcom.com, andrew.gospodarek@...adcom.com,
manoj.panicker2@....com, Eric.VanTassell@....com, vadim.fedorenko@...ux.dev,
horms@...nel.org, bagasdotme@...il.com, bhelgaas@...gle.com,
lukas@...ner.de, paul.e.luse@...el.com, jing2.liu@...el.com
Subject: Re: [PATCH V4 11/12] bnxt_en: Add TPH support in BNXT driver
On 9/11/24 10:37 AM, Alejandro Lucero Palau wrote:
>
> On 8/22/24 21:41, Wei Huang wrote:
>> From: Manoj Panicker <manoj.panicker2@....com>
>>
>> Implement TPH support in Broadcom BNXT device driver. The driver uses
>> TPH functions to retrieve and configure the device's Steering Tags when
>> its interrupt affinity is being changed.
>>
>> Co-developed-by: Wei Huang <wei.huang2@....com>
>> Signed-off-by: Wei Huang <wei.huang2@....com>
>> Signed-off-by: Manoj Panicker <manoj.panicker2@....com>
>> Reviewed-by: Ajit Khaparde <ajit.khaparde@...adcom.com>
>> Reviewed-by: Somnath Kotur <somnath.kotur@...adcom.com>
>> Reviewed-by: Andy Gospodarek <andrew.gospodarek@...adcom.com>
>> ---
>> drivers/net/ethernet/broadcom/bnxt/bnxt.c | 78 +++++++++++++++++++++++
>> drivers/net/ethernet/broadcom/bnxt/bnxt.h | 4 ++
>> 2 files changed, 82 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> index ffa74c26ee53..5903cd36b54d 100644
>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> @@ -55,6 +55,7 @@
>> #include <net/page_pool/helpers.h>
>> #include <linux/align.h>
>> #include <net/netdev_queues.h>
>> +#include <linux/pci-tph.h>
>>
>> #include "bnxt_hsi.h"
>> #include "bnxt.h"
>> @@ -10821,6 +10822,58 @@ int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init)
>> return 0;
>> }
>>
>> +static void __bnxt_irq_affinity_notify(struct irq_affinity_notify *notify,
>> + const cpumask_t *mask)
>> +{
>> + struct bnxt_irq *irq;
>> + u16 tag;
>> +
>> + irq = container_of(notify, struct bnxt_irq, affinity_notify);
>> + cpumask_copy(irq->cpu_mask, mask);
>> +
>> + if (pcie_tph_get_cpu_st(irq->bp->pdev, TPH_MEM_TYPE_VM,
>> + cpumask_first(irq->cpu_mask), &tag))
>
>
> I understand just one cpu from the mask has to be used, but I wonder if
> some check should be done for ensuring the mask is not mad.
>
> This is control path and the related queue is going to be restarted, so
> maybe a sanity check for ensuring all the cpus in the mask are from the
> same CCX complex?
I don't think this is always true and we shouldn't warn when this
happens. There is only one ST can be supported, so the driver need to
make a good judgement on which ST to be used. But no matter what, ST is
just a hint - it shouldn't cause any correctness issues in HW, even when
it is not the optimal target CPU. So warning is unnecessary.
>
> That would be an iteration checking the tag is the same one for all of
> them. If not, at least a warning stating the tag/CCX/cpu used.
>
>
>> + return;
>> +
>> + if (pcie_tph_set_st_entry(irq->bp->pdev, irq->msix_nr, tag))
>> + return;
>> +
>> + if (netif_running(irq->bp->dev)) {
>> + rtnl_lock();
>> + bnxt_close_nic(irq->bp, false, false);
>> + bnxt_open_nic(irq->bp, false, false);
>> + rtnl_unlock();
>> + }
>> +}
>> +
>> +static void __bnxt_irq_affinity_release(struct kref __always_unused *ref)
>> +{
>> +}
>> +
>> +static void bnxt_release_irq_notifier(struct bnxt_irq *irq)
>> +{
>> + irq_set_affinity_notifier(irq->vector, NULL);
>> +}
>> +
>> +static void bnxt_register_irq_notifier(struct bnxt *bp, struct bnxt_irq *irq)
>> +{
>> + struct irq_affinity_notify *notify;
>> +
>> + /* Nothing to do if TPH is not enabled */
>> + if (!pcie_tph_enabled(bp->pdev))
>> + return;
>> +
>> + irq->bp = bp;
>> +
>> + /* Register IRQ affinility notifier */
>> + notify = &irq->affinity_notify;
>> + notify->irq = irq->vector;
>> + notify->notify = __bnxt_irq_affinity_notify;
>> + notify->release = __bnxt_irq_affinity_release;
>> +
>> + irq_set_affinity_notifier(irq->vector, notify);
>> +}
>> +
>> static void bnxt_free_irq(struct bnxt *bp)
>> {
>> struct bnxt_irq *irq;
>> @@ -10843,11 +10896,17 @@ static void bnxt_free_irq(struct bnxt *bp)
>> free_cpumask_var(irq->cpu_mask);
>> irq->have_cpumask = 0;
>> }
>> +
>> + bnxt_release_irq_notifier(irq);
>> +
>> free_irq(irq->vector, bp->bnapi[i]);
>> }
>>
>> irq->requested = 0;
>> }
>> +
>> + /* Disable TPH support */
>> + pcie_disable_tph(bp->pdev);
>> }
>>
>> static int bnxt_request_irq(struct bnxt *bp)
>> @@ -10870,6 +10929,13 @@ static int bnxt_request_irq(struct bnxt *bp)
>> if (!(bp->flags & BNXT_FLAG_USING_MSIX))
>> flags = IRQF_SHARED;
>>
>> + /* Enable TPH support as part of IRQ request */
>> + if (pcie_tph_modes(bp->pdev) & PCI_TPH_CAP_INT_VEC) {
>> + rc = pcie_enable_tph(bp->pdev, PCI_TPH_CAP_INT_VEC);
>> + if (rc)
>> + netdev_warn(bp->dev, "failed enabling TPH support\n");
>> + }
>> +
>> for (i = 0, j = 0; i < bp->cp_nr_rings; i++) {
>> int map_idx = bnxt_cp_num_to_irq_num(bp, i);
>> struct bnxt_irq *irq = &bp->irq_tbl[map_idx];
>> @@ -10893,8 +10959,10 @@ static int bnxt_request_irq(struct bnxt *bp)
>>
>> if (zalloc_cpumask_var(&irq->cpu_mask, GFP_KERNEL)) {
>> int numa_node = dev_to_node(&bp->pdev->dev);
>> + u16 tag;
>>
>> irq->have_cpumask = 1;
>> + irq->msix_nr = map_idx;
>> cpumask_set_cpu(cpumask_local_spread(i, numa_node),
>> irq->cpu_mask);
>> rc = irq_set_affinity_hint(irq->vector, irq->cpu_mask);
>> @@ -10904,6 +10972,16 @@ static int bnxt_request_irq(struct bnxt *bp)
>> irq->vector);
>> break;
>> }
>> +
>> + bnxt_register_irq_notifier(bp, irq);
>> +
>> + /* Init ST table entry */
>> + if (pcie_tph_get_cpu_st(irq->bp->pdev, TPH_MEM_TYPE_VM,
>> + cpumask_first(irq->cpu_mask),
>> + &tag))
>> + break;
>> +
>> + pcie_tph_set_st_entry(irq->bp->pdev, irq->msix_nr, tag);
>> }
>> }
>> return rc;
>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
>> index 6bbdc718c3a7..ae1abcc1bddf 100644
>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
>> @@ -1224,6 +1224,10 @@ struct bnxt_irq {
>> u8 have_cpumask:1;
>> char name[IFNAMSIZ + 2];
>> cpumask_var_t cpu_mask;
>> +
>> + struct bnxt *bp;
>> + int msix_nr;
>> + struct irq_affinity_notify affinity_notify;
>> };
>>
>> #define HWRM_RING_ALLOC_TX 0x1
Powered by blists - more mailing lists