* [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 related [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, other threads:[~2020-11-16 7:10 UTC | newest] 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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).