On 22.08.2023 03:29, Stewart Hildebrand wrote:
> @@ -291,12 +291,9 @@ static void msi_set_enable(struct pci_dev *dev, int
> enable)
> static void msix_set_enable(struct pci_dev *dev, int enable)
> {
> int pos;
> - u16 control, seg = dev->seg;
> - u8 bus = dev->bus;
> - u8 slot = PCI_SLOT(dev->devfn);
> - u8 func = PCI_FUNC(dev->devfn);
> + u16 control;
As you touch such places, it would be nice to also switch to uint16_t at
the same time. (Similarly when you touch "pos" declarations anyway,
converting them to unsigned int when they're wrongly plain int would also
be nice.)
> @@ -318,17 +315,12 @@ static bool msi_set_mask_bit(struct irq_desc *desc,
> bool host, bool guest)
> {
> struct msi_desc *entry = desc->msi_desc;
> struct pci_dev *pdev;
> - u16 seg, control;
> - u8 bus, slot, func;
> + u16 control;
> bool flag = host || guest, maskall;
>
> ASSERT(spin_is_locked(&desc->lock));
> BUG_ON(!entry || !entry->dev);
> pdev = entry->dev;
> - seg = pdev->seg;
> - bus = pdev->bus;
> - slot = PCI_SLOT(pdev->devfn);
> - func = PCI_FUNC(pdev->devfn);
> switch ( entry->msi_attrib.type )
> {
> case PCI_CAP_ID_MSI:
You don't further alter the function, so this looks like an unrelated
(but still desirable for at least Misra) change.
> @@ -685,8 +674,8 @@ static u64 read_pci_mem_bar(u16 seg, u8 bus, u8 slot, u8
> func, u8 bir, int vf)
> {
> struct pci_dev *pdev = pci_get_pdev(NULL,
> PCI_SBDF(seg, bus, slot, func));
With this, ...
> - unsigned int pos = pci_find_ext_capability(seg, bus,
> - PCI_DEVFN(slot, func),
> + unsigned int pos = pci_find_ext_capability(PCI_SBDF(seg, bus, slot,
> + func),
> PCI_EXT_CAP_ID_SRIOV);
... please use pdev->sbdf here. Albeit I guess some further re-arrangement
is wanted here, to eliminate all of those PCI_SBDF() (and then also dealing
with the otherwise necessary NULL check). IOW perhaps fine the way you have
it, and then to be subject to a follow-on change.
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -193,11 +193,10 @@ int pci_mmcfg_read(unsigned int seg, unsigned int bus,
> unsigned int devfn, int reg, int len, u32 *value);
> int pci_mmcfg_write(unsigned int seg, unsigned int bus,
> unsigned int devfn, int reg, int len, u32 value);
> -int pci_find_cap_offset(u16 seg, u8 bus, u8 dev, u8 func, u8 cap);
> -int pci_find_next_cap(u16 seg, u8 bus, unsigned int devfn, u8 pos, int cap);
> -int pci_find_ext_capability(int seg, int bus, int devfn, int cap);
> -int pci_find_next_ext_capability(int seg, int bus, int devfn, int start,
> - int cap);
> +int pci_find_cap_offset(pci_sbdf_t sbdf, u8 cap);
> +int pci_find_next_cap(pci_sbdf_t sbdf, u8 pos, int cap);
> +int pci_find_ext_capability(pci_sbdf_t sbdf, int cap);
> +int pci_find_next_ext_capability(pci_sbdf_t sbdf, int start, int cap);
The remaining types want converting, too: Neither fixed-width nor plain int
are appropriate here. (It's a few too many type adjustments to make, for me
to offer doing so while committing.)
Jan
