Re: [PATCH v2 1/7] PCI/IOV: Return u16 from pci_sriov_get_totalvfs()

[David Laight <david.laight.linux@gmail.com>]

On Wed, 24 Jun 2026 23:59:56 +0900
"Alexandre Courbot" <acourbot@nvidia.com> wrote:

> On Wed Jun 24, 2026 at 10:39 PM JST, David Laight wrote:
> > On Wed, 24 Jun 2026 21:40:52 +0900
> > "Alexandre Courbot" <acourbot@nvidia.com> wrote:
> >  
> >> On Tue Jun 23, 2026 at 4:43 AM JST, Zhi Wang wrote:  
> >> > pci_sriov_get_totalvfs() reports a VF count, not an errno-style
> >> > status. It returns 0 when SR-IOV is unavailable or the device is not a
> >> > PF, and otherwise returns the PF's driver_max_VFs value.
> >> >
> >> > driver_max_VFs is stored as a u16 in struct pci_sriov. It is derived
> >> > from the SR-IOV TotalVFs field or from a driver-provided limit, so the
> >> > implementation cannot return a negative value.
> >> >
> >> > Change the declaration, CONFIG_PCI_IOV stub, and implementation to
> >> > return u16. Update callers to store the result in u16 variables, remove
> >> > obsolete negative-value checks, and use unsigned format specifiers where
> >> > needed.
> >> >
> >> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> >> > Cc: linux-pci@vger.kernel.org
> >> > Signed-off-by: Zhi Wang <zhiw@nvidia.com>    
> >> 
> >> Suggested-by: Alexandre Courbot <acourbot@nvidia.com>
> >> Link: https://lore.kernel.org/all/DETDILPA1GFY.27WND0TEC5352@nvidia.com/
> >>   
> >> > ---
> >> >  drivers/crypto/hisilicon/qm.c                   | 8 +++++---
> >> >  drivers/crypto/intel/qat/qat_common/adf_sriov.c | 6 +++---
> >> >  drivers/gpu/drm/xe/xe_sriov_pf.c                | 6 ++----
> >> >  drivers/misc/genwqe/card_base.c                 | 6 ++----
> >> >  drivers/net/ethernet/cavium/thunder/nic_main.c  | 2 +-
> >> >  drivers/net/ethernet/emulex/benet/be_main.c     | 3 ++-
> >> >  drivers/net/ethernet/mellanox/mlx5/core/sriov.c | 3 ++-
> >> >  drivers/net/ethernet/sfc/ef10_sriov.c           | 2 +-    
> >> 
> >> I believe that you can avoid converting all these drivers in this patch.
> >> The implicit `u16 -> int` conversion done by C should result in the
> >> expected behavior, and it will be fewer Acked-by to collect.  
> >
> > The generated code is also likely to be slightly better if the function
> > return value is a 32bit value.
> >
> > Similarly you don't really want to do any kind of maths on local variables
> > that aren't 32bit (or 64bit on 64bit builds).
> >
> > The fact that the domain of a value fits in 16 bits doesn't mean that
> > it is better to use u16 - it is usually worse.
> > Pretty much the only place u16 should be used is to reduce the size
> > of structures.
> >
> > So it is probably correct to change the return type to unsigned int and
> > remove the error return checks, but nothing else.  
> 
> For C, I agree that unsigned int is the safest type.
> 
> Rust otoh does not do implicit integer promotion, and making it return a
> `u16` carries useful range information. I wonder if we could have a
> private `__pci_sriov_get_totalvfs` that returns a `u16`, make
> `pci_sriov_get_totalvfs` promote it to an `unsigned int` and return it,
> while the Rust bindings would invoke `__pci_sriov_get_totalvfs` so they
> can expose a `u16`?

That is getting silly, something will end up doing the masking to police
the 16bit value - it isn't just the C rules, the ABI pass/return u16 in
in 32bit registers (even on x86) so one side (or both) has to mask the
value.

Your rust code has the same problem. Add 1 to a u16 variable and code has
to be added to mask the result to 16 bits.
You won't see it on x86 because it has 16bit alu operations, about the
only other cpu eoth them is m68k (I'm not sure about s390 - it is based
on a very old instructions set).

	David