| 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: <vt6wnwcje727xv4agzhkpe5ympcvhtgg7qbaq4hlvw42roji2r@3kwjm4togc7m>
Date: Fri, 7 Mar 2025 13:40:49 +0100
From: Jiri Pirko <jiri@...nulli.us>
To: Jacob Keller <jacob.e.keller@...el.com>
Cc: Przemek Kitszel <przemyslaw.kitszel@...el.com>,
intel-wired-lan@...ts.osuosl.org, Tony Nguyen <anthony.l.nguyen@...el.com>, netdev@...r.kernel.org,
Jakub Kicinski <kuba@...nel.org>, Aleksandr Loktionov <aleksandr.loktionov@...el.com>,
Karol Kolacinski <karol.kolacinski@...el.com>, Grzegorz Nitka <grzegorz.nitka@...el.com>,
Michal Schmidt <mschmidt@...hat.com>, Sergey Temerkhanov <sergey.temerkhanov@...el.com>
Subject: Re: [PATCH iwl-next] ice: use DSN instead of PCI BDF for ice_adapter
index
Fri, Mar 07, 2025 at 12:53:05AM +0100, jacob.e.keller@...el.com wrote:
>
>
>On 3/6/2025 1:11 PM, Przemek Kitszel wrote:
>> Use Device Serial Number instead of PCI bus/device/function for
>> index of struct ice_adapter.
>> Functions on the same physical device should point to the very same
>> ice_adapter instance.
>>
>> This is not only simplification, but also fixes things up when PF
>> is passed to VM (and thus has a random BDF).
>>
>> Suggested-by: Jacob Keller <jacob.e.keller@...el.com>
>> Suggested-by: Jakub Kicinski <kuba@...nel.org>
>> Suggested-by: Jiri Pirko <jiri@...nulli.us>
>> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@...el.com>
>> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
>> ---
>
>The only caution I have here is that we might run into issues with
>pre-production or poorly flashed boards which don't have DSN properly
>flashed. This shouldn't be an impact outside of early testing or
>mistakes by devs. I think there is a default ID which is almost all 0s
>we could check and log a warning to help prevent confusion in such a case?
>
>A couple systems I've seen have serial numbers like:
>
> serial_number 00-00-00-00-00-00-00-00
> serial_number 00-00-00-00-00-00-00-00
>
>or
>
> serial_number 00-01-00-ff-ff-00-00-00
> serial_number 00-01-00-ff-ff-00-00-00
>
>
>In practice I'm not sure how big a deal breaker this is. Properly
>initialized boards should have unique IDs, and if you update via
>devlink, or any of our standard update tools, it will maintain the ID
>across flash. However, during early development, boards were often
>flashed manually which could lead to such non-unique IDs.
Do we need a workaround for pre-production buggy hw now? Sounds a bit
weird tbh.
>
>> CC: Karol Kolacinski <karol.kolacinski@...el.com>
>> CC: Grzegorz Nitka <grzegorz.nitka@...el.com>
>> CC: Michal Schmidt <mschmidt@...hat.com>
>> CC: Sergey Temerkhanov <sergey.temerkhanov@...el.com>
>> ---
>> drivers/net/ethernet/intel/ice/ice_adapter.h | 4 +--
>> drivers/net/ethernet/intel/ice/ice_adapter.c | 29 +++-----------------
>> 2 files changed, 6 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.h b/drivers/net/ethernet/intel/ice/ice_adapter.h
>> index e233225848b3..1935163bd32f 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_adapter.h
>> +++ b/drivers/net/ethernet/intel/ice/ice_adapter.h
>> @@ -42,7 +42,7 @@ struct ice_adapter {
>> struct ice_port_list ports;
>> };
>>
>> -struct ice_adapter *ice_adapter_get(const struct pci_dev *pdev);
>> -void ice_adapter_put(const struct pci_dev *pdev);
>> +struct ice_adapter *ice_adapter_get(struct pci_dev *pdev);
>> +void ice_adapter_put(struct pci_dev *pdev);
>>
>> #endif /* _ICE_ADAPTER_H */
>> diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.c b/drivers/net/ethernet/intel/ice/ice_adapter.c
>> index 01a08cfd0090..b668339ed0ef 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_adapter.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_adapter.c
>> @@ -1,7 +1,6 @@
>> // SPDX-License-Identifier: GPL-2.0-only
>> // SPDX-FileCopyrightText: Copyright Red Hat
>>
>> -#include <linux/bitfield.h>
>> #include <linux/cleanup.h>
>> #include <linux/mutex.h>
>> #include <linux/pci.h>
>> @@ -14,29 +13,9 @@
>> static DEFINE_XARRAY(ice_adapters);
>> static DEFINE_MUTEX(ice_adapters_mutex);
>>
>> -/* PCI bus number is 8 bits. Slot is 5 bits. Domain can have the rest. */
>> -#define INDEX_FIELD_DOMAIN GENMASK(BITS_PER_LONG - 1, 13)
>> -#define INDEX_FIELD_DEV GENMASK(31, 16)
>> -#define INDEX_FIELD_BUS GENMASK(12, 5)
>> -#define INDEX_FIELD_SLOT GENMASK(4, 0)
>> -
>> -static unsigned long ice_adapter_index(const struct pci_dev *pdev)
>> +static unsigned long ice_adapter_index(struct pci_dev *pdev)
>> {
>> - unsigned int domain = pci_domain_nr(pdev->bus);
>> -
>> - WARN_ON(domain > FIELD_MAX(INDEX_FIELD_DOMAIN));
>> -
>> - switch (pdev->device) {
>> - case ICE_DEV_ID_E825C_BACKPLANE:
>> - case ICE_DEV_ID_E825C_QSFP:
>> - case ICE_DEV_ID_E825C_SFP:
>> - case ICE_DEV_ID_E825C_SGMII:
>> - return FIELD_PREP(INDEX_FIELD_DEV, pdev->device);
>> - default:
>> - return FIELD_PREP(INDEX_FIELD_DOMAIN, domain) |
>> - FIELD_PREP(INDEX_FIELD_BUS, pdev->bus->number) |
>> - FIELD_PREP(INDEX_FIELD_SLOT, PCI_SLOT(pdev->devfn));
>> - }
>> + return (unsigned long)pci_get_dsn(pdev);
>
>Much simpler :D
>
>> }
>>
>> static struct ice_adapter *ice_adapter_new(void)
>> @@ -77,7 +56,7 @@ static void ice_adapter_free(struct ice_adapter *adapter)
>> * Return: Pointer to ice_adapter on success.
>> * ERR_PTR() on error. -ENOMEM is the only possible error.
>> */
>> -struct ice_adapter *ice_adapter_get(const struct pci_dev *pdev)
>> +struct ice_adapter *ice_adapter_get(struct pci_dev *pdev)
>> {
>> unsigned long index = ice_adapter_index(pdev);
>> struct ice_adapter *adapter;
>> @@ -110,7 +89,7 @@ struct ice_adapter *ice_adapter_get(const struct pci_dev *pdev)
>> *
>> * Context: Process, may sleep.
>> */
>> -void ice_adapter_put(const struct pci_dev *pdev)
>> +void ice_adapter_put(struct pci_dev *pdev)
>> {
>
>A bit of a shame that this needs to be non const now.. Could
>pci_get_dsn() be made const? Or does it do something which might modify
>the device somehow?
Would make sense to me to make it const.
>
>> unsigned long index = ice_adapter_index(pdev);
>> struct ice_adapter *adapter;
>
Powered by blists - more mailing lists