On Fri, May 29, 2020 at 04:03:02PM +0800, Jason Wang wrote:> +static void vp_vdpa_set_vq_ready(struct vdpa_device *vdpa, > + u16 qid, bool ready) > +{ > + struct vp_vdpa *vp_vdpa = vdpa_to_vp(vdpa); > + > + vp_iowrite16(qid, &vp_vdpa->common->queue_select); > + vp_iowrite16(ready, &vp_vdpa->common->queue_enable); > +} > +Looks like this needs to check and just skip the write if ready == 0, right? Of course vdpa core then insists on calling vp_vdpa_get_vq_ready which will warn. Maybe just drop the check from core, move it to drivers which need it? ...> +static const struct pci_device_id vp_vdpa_id_table[] = { > + { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, PCI_ANY_ID) }, > + { 0 } > +};This looks like it'll create a mess with either virtio pci or vdpa being loaded at random. Maybe just don't specify any IDs for now. Down the road we could get a distinct vendor ID or a range of device IDs for this.> +MODULE_DEVICE_TABLE(pci, vp_vdpa_id_table); > + > +static struct pci_driver vp_vdpa_driver = { > + .name = "vp-vdpa", > + .id_table = vp_vdpa_id_table, > + .probe = vp_vdpa_probe, > + .remove = vp_vdpa_remove, > +}; > + > +module_pci_driver(vp_vdpa_driver); > + > +MODULE_AUTHOR("Jason Wang <jasowang at redhat.com>"); > +MODULE_DESCRIPTION("vp-vdpa"); > +MODULE_LICENSE("GPL"); > +MODULE_VERSION("1"); > -- > 2.20.1
On 2020/6/2 ??1:08, Michael S. Tsirkin wrote:> On Fri, May 29, 2020 at 04:03:02PM +0800, Jason Wang wrote: >> +static void vp_vdpa_set_vq_ready(struct vdpa_device *vdpa, >> + u16 qid, bool ready) >> +{ >> + struct vp_vdpa *vp_vdpa = vdpa_to_vp(vdpa); >> + >> + vp_iowrite16(qid, &vp_vdpa->common->queue_select); >> + vp_iowrite16(ready, &vp_vdpa->common->queue_enable); >> +} >> + > Looks like this needs to check and just skip the write if > ready == 0, right? Of course vdpa core then insists on calling > vp_vdpa_get_vq_ready which will warn. Maybe just drop the > check from core, move it to drivers which need it? > > ...That may work, but it may cause inconsistent semantic for set_vq_ready if we leave it to the driver.> > >> +static const struct pci_device_id vp_vdpa_id_table[] = { >> + { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, PCI_ANY_ID) }, >> + { 0 } >> +}; > This looks like it'll create a mess with either virtio pci > or vdpa being loaded at random. Maybe just don't specify > any IDs for now. Down the road we could get a > distinct vendor ID or a range of device IDs for this.Right, will do. Thanks> >> +MODULE_DEVICE_TABLE(pci, vp_vdpa_id_table); >> + >> +static struct pci_driver vp_vdpa_driver = { >> + .name = "vp-vdpa", >> + .id_table = vp_vdpa_id_table, >> + .probe = vp_vdpa_probe, >> + .remove = vp_vdpa_remove, >> +}; >> + >> +module_pci_driver(vp_vdpa_driver); >> + >> +MODULE_AUTHOR("Jason Wang <jasowang at redhat.com>"); >> +MODULE_DESCRIPTION("vp-vdpa"); >> +MODULE_LICENSE("GPL"); >> +MODULE_VERSION("1"); >> -- >> 2.20.1
On 2020/6/2 ??3:08, Jason Wang wrote:>> >>> +static const struct pci_device_id vp_vdpa_id_table[] = { >>> +??? { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, PCI_ANY_ID) }, >>> +??? { 0 } >>> +}; >> This looks like it'll create a mess with either virtio pci >> or vdpa being loaded at random. Maybe just don't specify >> any IDs for now. Down the road we could get a >> distinct vendor ID or a range of device IDs for this. > > > Right, will do. > > ThanksRethink about this. If we don't specify any ID, the binding won't work. How about using a dedicated subsystem vendor id for this? Thanks