netdev - Re: [PATCH v2 3/6] pds_core: add new fwctl auxiliary

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4852e263-dc4d-46bb-9fb5-cd78d7f8c553@amd.com>
Date: Wed, 5 Mar 2025 17:48:28 -0800
From: "Nelson, Shannon" <shannon.nelson@....com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: jgg@...dia.com, andrew.gospodarek@...adcom.com,
 aron.silverton@...cle.com, dan.j.williams@...el.com, daniel.vetter@...ll.ch,
 dave.jiang@...el.com, dsahern@...nel.org, gregkh@...uxfoundation.org,
 hch@...radead.org, itayavr@...dia.com, jiri@...dia.com, kuba@...nel.org,
 lbloch@...dia.com, leonro@...dia.com, linux-cxl@...r.kernel.org,
 linux-rdma@...r.kernel.org, netdev@...r.kernel.org, saeedm@...dia.com,
 brett.creeley@....com
Subject: Re: [PATCH v2 3/6] pds_core: add new fwctl auxiliary_device

On 3/4/2025 12:03 AM, Jonathan Cameron wrote:
> On Fri, 28 Feb 2025 17:35:51 -0800
> Shannon Nelson <shannon.nelson@....com> wrote:
> 
>> Add support for a new fwctl-based auxiliary_device for creating a
>> channel for fwctl support into the AMD/Pensando DSC.
>>
>> Signed-off-by: Shannon Nelson <shannon.nelson@....com>
> Hi Shannon,
> 
> A few really minor comments inline from a fresh read through.
> 
> Thanks,
> 
> Jonathan
> 
>> ---
>>   drivers/net/ethernet/amd/pds_core/auxbus.c |  3 +--
>>   drivers/net/ethernet/amd/pds_core/core.c   |  7 +++++++
>>   drivers/net/ethernet/amd/pds_core/core.h   |  1 +
>>   drivers/net/ethernet/amd/pds_core/main.c   | 11 +++++++++++
>>   include/linux/pds/pds_common.h             |  2 ++
>>   5 files changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/amd/pds_core/auxbus.c b/drivers/net/ethernet/amd/pds_core/auxbus.c
>> index db950a9c9d30..ac6f76c161f2 100644
>> --- a/drivers/net/ethernet/amd/pds_core/auxbus.c
>> +++ b/drivers/net/ethernet/amd/pds_core/auxbus.c
>> @@ -225,8 +225,7 @@ int pdsc_auxbus_dev_add(struct pdsc *cf, struct pdsc *pf,
>>        }
>>
>>        /* Verify that the type is supported and enabled.  It is not
>> -      * an error if there is no auxbus device support for this
>> -      * VF, it just means something else needs to happen with it.
>> +      * an error if there is no auxbus device support.
> 
> Comment feels a bit general. Is this no auxbus support for this device
> or none at all in the kernel?

Either one of no CONFIG_AUXILIARY_BUS or no fwctl service from the DSC. 
I'll make that more clear.


> 
>>         */
>>        vt_support = !!le16_to_cpu(pf->dev_ident.vif_types[vt]);
>>        if (!(vt_support &&
> 
>> diff --git a/drivers/net/ethernet/amd/pds_core/main.c b/drivers/net/ethernet/amd/pds_core/main.c
>> index a3a68889137b..41575c7a148d 100644
>> --- a/drivers/net/ethernet/amd/pds_core/main.c
>> +++ b/drivers/net/ethernet/amd/pds_core/main.c
>> @@ -265,6 +265,10 @@ static int pdsc_init_pf(struct pdsc *pdsc)
>>
>>        mutex_unlock(&pdsc->config_lock);
>>
>> +     err = pdsc_auxbus_dev_add(pdsc, pdsc, PDS_DEV_TYPE_FWCTL, &pdsc->padev);
>> +     if (err)
>> +             goto err_out_stop;
>> +
>>        dl = priv_to_devlink(pdsc);
>>        devl_lock(dl);
>>        err = devl_params_register(dl, pdsc_dl_params,
>> @@ -297,6 +301,7 @@ static int pdsc_init_pf(struct pdsc *pdsc)
>>        devlink_params_unregister(dl, pdsc_dl_params,
>>                                  ARRAY_SIZE(pdsc_dl_params));
>>   err_out_stop:
>> +     pdsc_auxbus_dev_del(pdsc, pdsc, &pdsc->padev);
> 
> This doesn't smell right (by which I mean I had to go look at the
> implementation to be sure it wasn't a bug) In my ideal
> world that should be obvious on a more local basis.
> I'd expect a new label here. pdsc_auxbus_dev_add() should be and is
> side effect free if it fails. That is it should not make sense
> to call pdsc_auxbus_dev_del() if it fails.
> 
> It isn't a bug today as that becomes a noop due to
> &pdsc->padev being NULL but that is a detail I shouldn't
> ideally need to know when reading this code.
> 
> I'd put err_out_stop label here and rename previous
> one to err_out_auxbus_del + replace the existing
> goto err_out_stop;
> with
> goto err_out_auxbus_del;

Yes, I can break that out to be more clear.

Thanks,
sln

> 
>>        pdsc_stop(pdsc);
>>   err_out_teardown:
>>        pdsc_teardown(pdsc, PDSC_TEARDOWN_REMOVING);
>> @@ -427,6 +432,7 @@ static void pdsc_remove(struct pci_dev *pdev)
>>                 * shut themselves down.
>>                 */
>>                pdsc_sriov_configure(pdev, 0);
>> +             pdsc_auxbus_dev_del(pdsc, pdsc, &pdsc->padev);
>>
>>                timer_shutdown_sync(&pdsc->wdtimer);
>>                if (pdsc->wq)
>> @@ -485,6 +491,8 @@ static void pdsc_reset_prepare(struct pci_dev *pdev)
>>                if (!IS_ERR(pf))
>>                        pdsc_auxbus_dev_del(pdsc, pf,
>>                                            &pf->vfs[pdsc->vf_id].padev);
>> +     } else {
>> +             pdsc_auxbus_dev_del(pdsc, pdsc, &pdsc->padev);
>>        }
>>
>>        pdsc_unmap_bars(pdsc);
>> @@ -531,6 +539,9 @@ static void pdsc_reset_done(struct pci_dev *pdev)
>>                if (!IS_ERR(pf))
>>                        pdsc_auxbus_dev_add(pdsc, pf, PDS_DEV_TYPE_VDPA,
>>                                            &pf->vfs[pdsc->vf_id].padev);
>> +     } else {
>> +             pdsc_auxbus_dev_add(pdsc, pdsc, PDS_DEV_TYPE_FWCTL,
>> +                                 &pdsc->padev);
>>        }
>>   }
>