Re: [PATCH v2] PCI: check also dynamic IDs for duplicate in new_id_store()

[Bjorn Helgaas <helgaas@kernel.org>]

[+cc Alex, Cornelia in case VFIO cares about new_id/remove_id
semantics]

On Mon, Oct 26, 2020 at 11:57:10AM +0800, Zhenzhong Duan wrote:
> When a device ID data is writen to /sys/bus/pci/drivers/.../new_id,
> only static ID table is checked for duplicate and multiple dynamic ID
> entries of same kind are allowed to exist in a dynamic linked list.

This doesn't quite say what the problem is.

I see that currently new_id_store() uses pci_match_id() to see if the
new device ID is in the static id_table, so adding the same ID twice
adds multiple entries to the dynids list.  That does seem wrong, and I
think we should fix it.

But I would like to clarify this commit log so we know whether the
current behavior causes user-visible broken behavior.  The dynids list
is mostly used by pci_match_device(), and it looks like duplicate
entries shouldn't cause it a problem.

I guess remove_id_store() will only remove one of the duplicate
entries, so if we add an ID several times, we would have to remove it
the same number of times before it's completely gone.

> Fix it by calling pci_match_device() which checks both dynamic and static
> IDs.
> 
> After fix, it shows below result which is expected.
> 
> echo "1af4:1000" > /sys/bus/pci/drivers/vfio-pci/new_id
> echo "1af4:1000" > /sys/bus/pci/drivers/vfio-pci/new_id
> -bash: echo: write error: File exists
> 
> Drop the static specifier and add a prototype to avoid build error.

I don't get this part.  You added a prototype in include/linux/pci.h,
which means you expect callers outside drivers/pci.  But there aren't
any.

In fact, you're only adding a call in the same file where
pci_match_device() is defined.  The usual way to resolve that is to
move the pci_match_device() definition before the call, so no forward
declaration is needed and the function can remain static.

I think pci_match_id() and pci_match_device() should both be moved so
they remain together.  It would be nice if the move itself were a
no-op patch separate from the one that changes new_id_store().

> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@gmail.com>
> ---
> v2: revert the export of pci_match_device() per Christoph
>     combind PATCH1 and PATCH2 into one.
> 
>  drivers/pci/pci-driver.c | 4 ++--
>  include/linux/pci.h      | 2 ++
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 8b587fc..cdc7d13 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -125,7 +125,7 @@ static ssize_t new_id_store(struct device_driver *driver, const char *buf,
>  		pdev->subsystem_device = subdevice;
>  		pdev->class = class;
>  
> -		if (pci_match_id(pdrv->id_table, pdev))
> +		if (pci_match_device(pdrv, pdev))
>  			retval = -EEXIST;
>  
>  		kfree(pdev);
> @@ -250,7 +250,7 @@ const struct pci_device_id *pci_match_id(const struct pci_device_id *ids,
>   * system is in its list of supported devices.  Returns the matching
>   * pci_device_id structure or %NULL if there is no match.
>   */
> -static const struct pci_device_id *pci_match_device(struct pci_driver *drv,
> +const struct pci_device_id *pci_match_device(struct pci_driver *drv,
>  						    struct pci_dev *dev)
>  {
>  	struct pci_dynid *dynid;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 22207a7..ec57312 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1409,6 +1409,8 @@ int pci_add_dynid(struct pci_driver *drv,
>  		  unsigned long driver_data);
>  const struct pci_device_id *pci_match_id(const struct pci_device_id *ids,
>  					 struct pci_dev *dev);
> +const struct pci_device_id *pci_match_device(struct pci_driver *drv,
> +					     struct pci_dev *dev);
>  int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max,
>  		    int pass);
>  
> -- 
> 1.8.3.1
>