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

[Zhenzhong Duan]

Hi Bjorn,

On Sat, Nov 14, 2020 at 6:47 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> 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