Message ID | 20250306211159.3697-2-przemyslaw.kitszel@intel.com (mailing list archive) |
---|---|
State | Awaiting Upstream |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | [iwl-next] ice: use DSN instead of PCI BDF for ice_adapter index | expand |
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@intel.com> > Suggested-by: Jakub Kicinski <kuba@kernel.org> > Suggested-by: Jiri Pirko <jiri@resnulli.us> > Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com> > Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.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. > CC: Karol Kolacinski <karol.kolacinski@intel.com> > CC: Grzegorz Nitka <grzegorz.nitka@intel.com> > CC: Michal Schmidt <mschmidt@redhat.com> > CC: Sergey Temerkhanov <sergey.temerkhanov@intel.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? > unsigned long index = ice_adapter_index(pdev); > struct ice_adapter *adapter;
Thu, Mar 06, 2025 at 10:11:46PM +0100, przemyslaw.kitszel@intel.com 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@intel.com> >Suggested-by: Jakub Kicinski <kuba@kernel.org> >Suggested-by: Jiri Pirko <jiri@resnulli.us> >Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com> >Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> From my perspective, this is a bug fix, makes sense to me to send to -net tree. >--- >CC: Karol Kolacinski <karol.kolacinski@intel.com> >CC: Grzegorz Nitka <grzegorz.nitka@intel.com> >CC: Michal Schmidt <mschmidt@redhat.com> >CC: Sergey Temerkhanov <sergey.temerkhanov@intel.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); How do you ensure there is no xarray index collision then you cut the number like this? > } > > 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) > { > unsigned long index = ice_adapter_index(pdev); > struct ice_adapter *adapter; >-- >2.46.0 >
Fri, Mar 07, 2025 at 12:53:05AM +0100, jacob.e.keller@intel.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@intel.com> >> Suggested-by: Jakub Kicinski <kuba@kernel.org> >> Suggested-by: Jiri Pirko <jiri@resnulli.us> >> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com> >> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.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@intel.com> >> CC: Grzegorz Nitka <grzegorz.nitka@intel.com> >> CC: Michal Schmidt <mschmidt@redhat.com> >> CC: Sergey Temerkhanov <sergey.temerkhanov@intel.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; >
Comments inline -----Original Message----- From: Jiri Pirko <jiri@resnulli.us> Sent: Friday, March 7, 2025 1:39 PM To: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com> Cc: intel-wired-lan@lists.osuosl.org; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; netdev@vger.kernel.org; Keller, Jacob E <jacob.e.keller@intel.com>; Jakub Kicinski <kuba@kernel.org>; Loktionov, Aleksandr <aleksandr.loktionov@intel.com>; Kolacinski, Karol <karol.kolacinski@intel.com>; Nitka, Grzegorz <grzegorz.nitka@intel.com>; Schmidt, Michal <mschmidt@redhat.com>; Temerkhanov, Sergey <sergey.temerkhanov@intel.com> Subject: Re: [PATCH iwl-next] ice: use DSN instead of PCI BDF for ice_adapter index Thu, Mar 06, 2025 at 10:11:46PM +0100, przemyslaw.kitszel@intel.com 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). It might be worth checking behavior of different hypervisors/VMMs with multifunction PCI devices passthrough. QEMU, for example, has options to set the BDFs explicitly. > >Suggested-by: Jacob Keller <jacob.e.keller@intel.com> >Suggested-by: Jakub Kicinski <kuba@kernel.org> >Suggested-by: Jiri Pirko <jiri@resnulli.us> >Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com> >Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> >-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); >How do you ensure there is no xarray index collision then you cut the number like this? It is also probably necessary to check if all devices supported by the driver have DSN capability enabled. Regards, Sergey
On 3/7/2025 4:40 AM, Jiri Pirko wrote: > Fri, Mar 07, 2025 at 12:53:05AM +0100, jacob.e.keller@intel.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@intel.com> >>> Suggested-by: Jakub Kicinski <kuba@kernel.org> >>> Suggested-by: Jiri Pirko <jiri@resnulli.us> >>> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com> >>> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.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. > I agree that use of the serial number is preferred over BDF for the reasons described in this thread. But I also know that sometimes the DSN is not available, or is not set properly during pre-production and early testing. This could cause issues for early development. These issues can likely be worked around and should not impact what we do for properly functioning boards. I just want to make it clear on the record, since it is likely that we would see this if using an old or badly flashed board, or if we use this same scheme on a future hardware (or even just a spin of the ice hardware). In those cases, developers might have breaking functionality like multiple adapters being tied to the same adapter structure. I *don't* want those to be reported as issues with this code, as they are really issues with the flash data. Perhaps we could have some sort of warning message to go "this doesn't look right" when the DSN capability doesn't exist or when the DSN is 0. In the end, the places where this is likely to fail are also the places where hopefully the developers are smart enough to know what is going on.
> Subject: Re: [PATCH iwl-next] ice: use DSN instead of PCI BDF for ice_adapter index regarding -net vs -next, no one have complained that this bug hurts >> + return (unsigned long)pci_get_dsn(pdev); > >> How do you ensure there is no xarray index collision then you cut the number like this? The reduction occurs only on "32b" systems, which are unlikely to have this device. And any mixing of the upper and lower 4B part still could collide. > > It is also probably necessary to check if all devices supported by the driver have DSN capability enabled. I will double check on the SoC you have in mind. > > Regards, > Sergey
>>> -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. It would indeed, but to do so, one have to constify at least a few other pci_* functions, I didn't even got to the bottom. While I appreciate the added value of typechecks, I would like to focus on different work, especially that there are contributors that seems to be focused on the hunt of such cases :) > > >> >>> unsigned long index = ice_adapter_index(pdev); >>> struct ice_adapter *adapter; >> >
Mon, Mar 10, 2025 at 09:40:16AM +0100, przemyslaw.kitszel@intel.com wrote: >> Subject: Re: [PATCH iwl-next] ice: use DSN instead of PCI BDF for ice_adapter index > >regarding -net vs -next, no one have complained that this bug hurts Wait, so we are now waiting for someone to hit the bug and complain, before we do fix? Does not make any sense to me. > >> > + return (unsigned long)pci_get_dsn(pdev); >> >> > How do you ensure there is no xarray index collision then you cut the number like this? > >The reduction occurs only on "32b" systems, which are unlikely to have >this device. And any mixing of the upper and lower 4B part still could >collide. Passtrough to 32 bit qemu machine? Even how unlikely is that, you are risking a user to hit a bug for newly introduced code without good reason. Why? > >> >> It is also probably necessary to check if all devices supported by the driver have DSN capability enabled. > >I will double check on the SoC you have in mind. > >> >> Regards, >> Sergey >
On 3/10/25 13:23, Jiri Pirko wrote: > Mon, Mar 10, 2025 at 09:40:16AM +0100, przemyslaw.kitszel@intel.com wrote: >>> Subject: Re: [PATCH iwl-next] ice: use DSN instead of PCI BDF for ice_adapter index >> >> regarding -net vs -next, no one have complained that this bug hurts > > Wait, so we are now waiting for someone to hit the bug and complain, > before we do fix? Does not make any sense to me. no one is waiting for a fix, but it could affect users with weird NVM images, so -next seems reasonable > > >> >>>> + return (unsigned long)pci_get_dsn(pdev); >>> >>>> How do you ensure there is no xarray index collision then you cut the number like this? >> >> The reduction occurs only on "32b" systems, which are unlikely to have >> this device. And any mixing of the upper and lower 4B part still could >> collide. > > Passtrough to 32 bit qemu machine? Even how unlikely is that, you are > risking a user to hit a bug for newly introduced code without good > reason. Why? I will combine the two, by simple xor > > >> >>> >>> It is also probably necessary to check if all devices supported by the driver have DSN capability enabled. >> >> I will double check on the SoC you have in mind. IMO an NVM issue, will handle this offline
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); } 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) { unsigned long index = ice_adapter_index(pdev); struct ice_adapter *adapter;