Re: [PATCH 1/2] PCI: Add support for VF Resizable Bar extended cap

[Michał Winiarski]

On 16.12.2021 01:21, Krzysztof Wilczyński wrote:
> Hi Michał,
>
> [...]
> > +static int pci_memory_decoding(struct pci_dev *dev)
> > +{
> > +	u16 cmd;
> > +
> > +	pci_read_config_word(dev, PCI_COMMAND, &cmd);
> > +	if (cmd & PCI_COMMAND_MEMORY)
> > +		return -EBUSY;
> > +
> > +	return 0;
> > +}
> > +
> > +#ifdef CONFIG_PCI_IOV
> > +static int pci_vf_memory_decoding(struct pci_dev *dev)
> > +{
> > +	u16 cmd;
> > +
> > +	pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_CTRL, &cmd);
> > +	if (cmd & PCI_SRIOV_CTRL_MSE)
> > +		return -EBUSY;
> > +
> > +	return 0;
> > +}
> > +#endif
> > +
> >   int pci_resize_resource(struct pci_dev *dev, int resno, int size)
> >   {
> >   	struct resource *res = dev->resource + resno;
> >   	struct pci_host_bridge *host;
> >   	int old, ret;
> >   	u32 sizes;
> > -	u16 cmd;
> >   
> >   	/* Check if we must preserve the firmware's resource assignment */
> >   	host = pci_find_host_bridge(dev->bus);
> > @@ -424,9 +447,14 @@ int pci_resize_resource(struct pci_dev *dev, int resno, int size)
> >   	if (!(res->flags & IORESOURCE_UNSET))
> >   		return -EBUSY;
> >   
> > -	pci_read_config_word(dev, PCI_COMMAND, &cmd);
> > -	if (cmd & PCI_COMMAND_MEMORY)
> > -		return -EBUSY;
> > +#ifdef CONFIG_PCI_IOV
> > +	if (resno >= PCI_IOV_RESOURCES)
> > +		ret = pci_vf_memory_decoding(dev);
> > +	else
> > +#endif
> > +	ret = pci_memory_decoding(dev);
> > +	if (ret)
> > +		return ret;
>
> The above else works, however, it does trip our static analysis tools, and
> lack of the indentation makes it slightly confusing to read, at least at
> the first glance.  Thus, I wonder whether it would be possible to combine
> the pci_vf_memory_decoding() and pci_memory_decoding() somehow neatly into
> a private helper that takes a boolean to indicate whether it's a VF or not.
> Then, we could drop the if-statement, since some of the logic could live
> within the helper.
>
> What do you think?

Sure - implementing a helper in a way that Christian suggested (the 
helper still takes resno though) should also take care of that.

Thanks
-Michał

> 	Krzysztof