Fri, Mar 07, 2025 at 12:53:05AM +0100, [email protected] 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 <[email protected]> >> Suggested-by: Jakub Kicinski <[email protected]> >> Suggested-by: Jiri Pirko <[email protected]> >> Reviewed-by: Aleksandr Loktionov <[email protected]> >> Signed-off-by: Przemek Kitszel <[email protected]> >> --- > >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 <[email protected]> >> CC: Grzegorz Nitka <[email protected]> >> CC: Michal Schmidt <[email protected]> >> CC: Sergey Temerkhanov <[email protected]> >> --- >> 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; >
