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

[Gary Guo]

On Wed Jun 24, 2026 at 8:38 PM BST, David Laight wrote:
> On Wed, 24 Jun 2026 23:59:56 +0900
> "Alexandre Courbot" <acourbot@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
>> >> > Cc: linux-pci@xxxxxxxxxxxxxxx
>> >> > Signed-off-by: Zhi Wang <zhiw@xxxxxxxxxx>    
>> >> 
>> >> Suggested-by: Alexandre Courbot <acourbot@xxxxxxxxxx>
>> >> Link: https://lore.kernel.org/all/DETDILPA1GFY.27WND0TEC5352@xxxxxxxxxx/
>> >>   
>> >> > ---
>> >> >  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`?

There're no value in having two versions with just slightly different promotion
behaviour. Rust side can still use u16 by casting from unsigned int without
having to expose both.


> 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).

There's no implicit integer casting (extending or truncating) in Rust so usually
the setter and getters for the same property would be of the same type.
pci_sriov_set_totalvfs takes u16 so I expect the pci_sriov_get_totalvfs
API binding in Rust returning u16 too. That said, this can be handled by Rust
bindings as mentioned above.

This is not hot path and just have a handful of users anyway and I wouldn't
worry about a single masking/sign-extension instruction.

Best,
Gary