Re: [PATCH v6 2/3] PCI: Add parameter nr_devfns to pci_add_dma_alias

[James Sewart <jamessewart@arista.com>]

> On 10 Dec 2019, at 22:37, Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> [+cc Joerg]
> 
> On Tue, Dec 03, 2019 at 03:43:53PM +0000, James Sewart wrote:
>> pci_add_dma_alias can now be used to create a dma alias for a range of
>> devfns.
>> 
>> Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
>> Signed-off-by: James Sewart <jamessewart@arista.com>
>> ---
>> drivers/pci/pci.c    | 22 +++++++++++++++++-----
>> drivers/pci/quirks.c | 14 +++++++-------
>> include/linux/pci.h  |  2 +-
>> 3 files changed, 25 insertions(+), 13 deletions(-)
> 
> Heads up Joerg: I also updated drivers/iommu/amd_iommu.c (this is the
> one reported by the kbuild test robot) and removed the printk there
> that prints the same thing as the one in pci_add_dma_alias(), and I
> updated a PCI quirk that was merged after this patch was posted.
> 
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index d3c83248f3ce..dbb01aceafda 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -5857,7 +5857,8 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
>> /**
>>  * pci_add_dma_alias - Add a DMA devfn alias for a device
>>  * @dev: the PCI device for which alias is added
>> - * @devfn: alias slot and function
>> + * @devfn_from: alias slot and function
>> + * @nr_devfns: Number of subsequent devfns to alias
>>  *
>>  * This helper encodes an 8-bit devfn as a bit number in dma_alias_mask
>>  * which is used to program permissible bus-devfn source addresses for DMA
>> @@ -5873,8 +5874,13 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
>>  * cannot be left as a userspace activity).  DMA aliases should therefore
>>  * be configured via quirks, such as the PCI fixup header quirk.
>>  */
>> -void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
>> +void pci_add_dma_alias(struct pci_dev *dev, u8 devfn_from, unsigned nr_devfns)
>> {
>> +	int devfn_to;
>> +
>> +	nr_devfns = min(nr_devfns, (unsigned)MAX_NR_DEVFNS);
>> +	devfn_to = devfn_from + nr_devfns - 1;
> 
> I made this look like:
> 
> +       devfn_to = min(devfn_from + nr_devfns - 1,
> +                      (unsigned) MAX_NR_DEVFNS - 1);
> 
> so devfn_from=0xf0, nr_devfns=0x20 doesn't cause devfn_to to wrap
> around.
> 
> I did keep Logan's reviewed-by, so let me know if I broke something.

I think nr_devfns still needs updating as it is used for bitmap_set. 
Although thinking about it now we should limit the number to alias to be 
maximum (MAX_NR_DEVFNS - devfn_from), so that we don’t set past the end of 
the bitmap:

 nr_devfns = min(nr_devfns, (unsigned) MAX_NR_DEVFNS - devfn_from);

I think with this change we wont need to clip devfn_to.

> 
>> 	if (!dev->dma_alias_mask)
>> 		dev->dma_alias_mask = bitmap_zalloc(MAX_NR_DEVFNS, GFP_KERNEL);
>> 	if (!dev->dma_alias_mask) {
>> @@ -5882,9 +5888,15 @@ void pci_add_dma_alias(struct pci_dev *dev, u8 devfn)
>> 		return;
>> 	}
>> 
>> -	set_bit(devfn, dev->dma_alias_mask);
>> -	pci_info(dev, "Enabling fixed DMA alias to %02x.%d\n",
>> -		 PCI_SLOT(devfn), PCI_FUNC(devfn));
>> +	bitmap_set(dev->dma_alias_mask, devfn_from, nr_devfns);
>> +
>> +	if (nr_devfns == 1)
>> +		pci_info(dev, "Enabling fixed DMA alias to %02x.%d\n",
>> +				PCI_SLOT(devfn_from), PCI_FUNC(devfn_from));
>> +	else if(nr_devfns > 1)
>> +		pci_info(dev, "Enabling fixed DMA alias for devfn range from %02x.%d to %02x.%d\n",
>> +				PCI_SLOT(devfn_from), PCI_FUNC(devfn_from),
>> +				PCI_SLOT(devfn_to), PCI_FUNC(devfn_to));
>> }