Linux-PCI Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] PCI: check also dynamic IDs for duplicate in new_id_store()
@ 2020-10-26  3:57 Zhenzhong Duan
  2020-10-27  7:52 ` Christoph Hellwig
  2020-11-13 22:47 ` Bjorn Helgaas
  0 siblings, 2 replies; 5+ messages in thread
From: Zhenzhong Duan @ 2020-10-26  3:57 UTC (permalink / raw)
  To: linux-kernel, linux-pci; +Cc: bhelgaas, hch, Zhenzhong Duan

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.

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.

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] PCI: check also dynamic IDs for duplicate in new_id_store()
  2020-10-26  3:57 [PATCH v2] PCI: check also dynamic IDs for duplicate in new_id_store() Zhenzhong Duan
@ 2020-10-27  7:52 ` Christoph Hellwig
  2020-11-10 12:36   ` Zhenzhong Duan
  2020-11-13 22:47 ` Bjorn Helgaas
  1 sibling, 1 reply; 5+ messages in thread
From: Christoph Hellwig @ 2020-10-27  7:52 UTC (permalink / raw)
  To: Zhenzhong Duan; +Cc: linux-kernel, linux-pci, bhelgaas, hch

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.
> 
> 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.
> 
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@gmail.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] PCI: check also dynamic IDs for duplicate in new_id_store()
  2020-10-27  7:52 ` Christoph Hellwig
@ 2020-11-10 12:36   ` Zhenzhong Duan
  0 siblings, 0 replies; 5+ messages in thread
From: Zhenzhong Duan @ 2020-11-10 12:36 UTC (permalink / raw)
  To: Christoph Hellwig, Bjorn Helgaas; +Cc: linux-kernel, linux-pci

Hi Bjorn,

This patch got reviewed-by, could you kindly check if it can be
upstreamed? Thanks very much.

Zhenzhong

On Tue, Oct 27, 2020 at 3:52 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> 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.
> >
> > 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.
> >
> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@gmail.com>
>
> Looks good,
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] PCI: check also dynamic IDs for duplicate in new_id_store()
  2020-10-26  3:57 [PATCH v2] PCI: check also dynamic IDs for duplicate in new_id_store() Zhenzhong Duan
  2020-10-27  7:52 ` Christoph Hellwig
@ 2020-11-13 22:47 ` Bjorn Helgaas
  2020-11-16  7:08   ` Zhenzhong Duan
  1 sibling, 1 reply; 5+ messages in thread
From: Bjorn Helgaas @ 2020-11-13 22:47 UTC (permalink / raw)
  To: Zhenzhong Duan
  Cc: linux-kernel, linux-pci, bhelgaas, hch, Alex Williamson, Cornelia Huck

[+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
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] PCI: check also dynamic IDs for duplicate in new_id_store()
  2020-11-13 22:47 ` Bjorn Helgaas
@ 2020-11-16  7:08   ` Zhenzhong Duan
  0 siblings, 0 replies; 5+ messages in thread
From: Zhenzhong Duan @ 2020-11-16  7:08 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: linux-kernel, linux-pci, Bjorn Helgaas, Christoph Hellwig,
	Alex Williamson, Cornelia Huck

Hi Bjorn,

On Sat, Nov 14, 2020 at 6:47 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+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.

Current behavior doesn't cause user-visible broken behavior, only not
user friendly. One has to remove an ID at least twice to ensure it's
really removed if he doesn't know how many times it has been added
before.

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

Yes, that's better, will do, thanks for your suggestions.

Zhenzhong

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-26  3:57 [PATCH v2] PCI: check also dynamic IDs for duplicate in new_id_store() Zhenzhong Duan
2020-10-27  7:52 ` Christoph Hellwig
2020-11-10 12:36   ` Zhenzhong Duan
2020-11-13 22:47 ` Bjorn Helgaas
2020-11-16  7:08   ` Zhenzhong Duan

Linux-PCI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pci linux-pci/ https://lore.kernel.org/linux-pci \
		linux-pci@vger.kernel.org
	public-inbox-index linux-pci

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pci


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git