| Message ID | 20230516025521.43352-5-shannon.nelson@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | pds_vdpa driver | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Mon, May 15, 2023 at 07:55:14PM -0700, Shannon Nelson wrote: > The pds_core_logical_qtype enum and IFNAMSIZ are not needed > in the common PDS header, only needed when working with the > adminq, so move them to the adminq header. > > Note: This patch might conflict with pds_vfio patches that are > in review, depending on which patchset gets pulled first. > > Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> > Acked-by: Jason Wang <jasowang@redhat.com> It's a bit weird to add code in one patch then move it in another. Why not start with it in the final location? More importantly, the use of adminq terminology here is a going to be somewhat confusing with the unrelated admin virtqueue just having landed in the virtio spec. Is this terminology coming from some hardware spec? > --- > include/linux/pds/pds_adminq.h | 21 +++++++++++++++++++++ > include/linux/pds/pds_common.h | 21 --------------------- > 2 files changed, 21 insertions(+), 21 deletions(-) > > diff --git a/include/linux/pds/pds_adminq.h b/include/linux/pds/pds_adminq.h > index 98a60ce87b92..61b0a8634e1a 100644 > --- a/include/linux/pds/pds_adminq.h > +++ b/include/linux/pds/pds_adminq.h > @@ -222,6 +222,27 @@ enum pds_core_lif_type { > PDS_CORE_LIF_TYPE_DEFAULT = 0, > }; > > +#define PDS_CORE_IFNAMSIZ 16 > + > +/** > + * enum pds_core_logical_qtype - Logical Queue Types > + * @PDS_CORE_QTYPE_ADMINQ: Administrative Queue > + * @PDS_CORE_QTYPE_NOTIFYQ: Notify Queue > + * @PDS_CORE_QTYPE_RXQ: Receive Queue > + * @PDS_CORE_QTYPE_TXQ: Transmit Queue > + * @PDS_CORE_QTYPE_EQ: Event Queue > + * @PDS_CORE_QTYPE_MAX: Max queue type supported > + */ > +enum pds_core_logical_qtype { > + PDS_CORE_QTYPE_ADMINQ = 0, > + PDS_CORE_QTYPE_NOTIFYQ = 1, > + PDS_CORE_QTYPE_RXQ = 2, > + PDS_CORE_QTYPE_TXQ = 3, > + PDS_CORE_QTYPE_EQ = 4, > + > + PDS_CORE_QTYPE_MAX = 16 /* don't change - used in struct size */ > +}; > + > /** > * union pds_core_lif_config - LIF configuration > * @state: LIF state (enum pds_core_lif_state) > diff --git a/include/linux/pds/pds_common.h b/include/linux/pds/pds_common.h > index 2a0d1669cfd0..435c8e8161c2 100644 > --- a/include/linux/pds/pds_common.h > +++ b/include/linux/pds/pds_common.h > @@ -41,27 +41,6 @@ enum pds_core_vif_types { > > #define PDS_VDPA_DEV_NAME PDS_CORE_DRV_NAME "." PDS_DEV_TYPE_VDPA_STR > > -#define PDS_CORE_IFNAMSIZ 16 > - > -/** > - * enum pds_core_logical_qtype - Logical Queue Types > - * @PDS_CORE_QTYPE_ADMINQ: Administrative Queue > - * @PDS_CORE_QTYPE_NOTIFYQ: Notify Queue > - * @PDS_CORE_QTYPE_RXQ: Receive Queue > - * @PDS_CORE_QTYPE_TXQ: Transmit Queue > - * @PDS_CORE_QTYPE_EQ: Event Queue > - * @PDS_CORE_QTYPE_MAX: Max queue type supported > - */ > -enum pds_core_logical_qtype { > - PDS_CORE_QTYPE_ADMINQ = 0, > - PDS_CORE_QTYPE_NOTIFYQ = 1, > - PDS_CORE_QTYPE_RXQ = 2, > - PDS_CORE_QTYPE_TXQ = 3, > - PDS_CORE_QTYPE_EQ = 4, > - > - PDS_CORE_QTYPE_MAX = 16 /* don't change - used in struct size */ > -}; > - > int pdsc_register_notify(struct notifier_block *nb); > void pdsc_unregister_notify(struct notifier_block *nb); > void *pdsc_get_pf_struct(struct pci_dev *vf_pdev); > -- > 2.17.1
On 5/15/23 11:12 PM, Michael S. Tsirkin wrote: > On Mon, May 15, 2023 at 07:55:14PM -0700, Shannon Nelson wrote: >> The pds_core_logical_qtype enum and IFNAMSIZ are not needed >> in the common PDS header, only needed when working with the >> adminq, so move them to the adminq header. >> >> Note: This patch might conflict with pds_vfio patches that are >> in review, depending on which patchset gets pulled first. >> >> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> >> Acked-by: Jason Wang <jasowang@redhat.com> > > It's a bit weird to add code in one patch then move it > in another. Why not start with it in the final location? Yes, and I usually try to catch those things before they go out :-). In this case the chunk we're moving was added in the pds_core patchset a few weeks ago, and since then we have realized it would be better to be localized to the adminq header. Perhaps this would have been clearer if this patch came first? > > More importantly, the use of adminq terminology here > is a going to be somewhat confusing with the unrelated > admin virtqueue just having landed in the virtio spec. > Is this terminology coming from some hardware spec? This firmware adminq through the PCI interface is similar to how several other network drivers have an adminq for more complex conversations with the FW. Yes, I can see how having both this device adminq and the virtio admin VQ in one place could be confusing, and we'll need to be sure to be clear in the difference. This is part of why we put most of the adminq cmd code in its own cmd.c file, away from the virtio/vdpa handling in vdpa_dev.c. Thanks, sln > >> --- >> include/linux/pds/pds_adminq.h | 21 +++++++++++++++++++++ >> include/linux/pds/pds_common.h | 21 --------------------- >> 2 files changed, 21 insertions(+), 21 deletions(-) >> >> diff --git a/include/linux/pds/pds_adminq.h b/include/linux/pds/pds_adminq.h >> index 98a60ce87b92..61b0a8634e1a 100644 >> --- a/include/linux/pds/pds_adminq.h >> +++ b/include/linux/pds/pds_adminq.h >> @@ -222,6 +222,27 @@ enum pds_core_lif_type { >> PDS_CORE_LIF_TYPE_DEFAULT = 0, >> }; >> >> +#define PDS_CORE_IFNAMSIZ 16 >> + >> +/** >> + * enum pds_core_logical_qtype - Logical Queue Types >> + * @PDS_CORE_QTYPE_ADMINQ: Administrative Queue >> + * @PDS_CORE_QTYPE_NOTIFYQ: Notify Queue >> + * @PDS_CORE_QTYPE_RXQ: Receive Queue >> + * @PDS_CORE_QTYPE_TXQ: Transmit Queue >> + * @PDS_CORE_QTYPE_EQ: Event Queue >> + * @PDS_CORE_QTYPE_MAX: Max queue type supported >> + */ >> +enum pds_core_logical_qtype { >> + PDS_CORE_QTYPE_ADMINQ = 0, >> + PDS_CORE_QTYPE_NOTIFYQ = 1, >> + PDS_CORE_QTYPE_RXQ = 2, >> + PDS_CORE_QTYPE_TXQ = 3, >> + PDS_CORE_QTYPE_EQ = 4, >> + >> + PDS_CORE_QTYPE_MAX = 16 /* don't change - used in struct size */ >> +}; >> + >> /** >> * union pds_core_lif_config - LIF configuration >> * @state: LIF state (enum pds_core_lif_state) >> diff --git a/include/linux/pds/pds_common.h b/include/linux/pds/pds_common.h >> index 2a0d1669cfd0..435c8e8161c2 100644 >> --- a/include/linux/pds/pds_common.h >> +++ b/include/linux/pds/pds_common.h >> @@ -41,27 +41,6 @@ enum pds_core_vif_types { >> >> #define PDS_VDPA_DEV_NAME PDS_CORE_DRV_NAME "." PDS_DEV_TYPE_VDPA_STR >> >> -#define PDS_CORE_IFNAMSIZ 16 >> - >> -/** >> - * enum pds_core_logical_qtype - Logical Queue Types >> - * @PDS_CORE_QTYPE_ADMINQ: Administrative Queue >> - * @PDS_CORE_QTYPE_NOTIFYQ: Notify Queue >> - * @PDS_CORE_QTYPE_RXQ: Receive Queue >> - * @PDS_CORE_QTYPE_TXQ: Transmit Queue >> - * @PDS_CORE_QTYPE_EQ: Event Queue >> - * @PDS_CORE_QTYPE_MAX: Max queue type supported >> - */ >> -enum pds_core_logical_qtype { >> - PDS_CORE_QTYPE_ADMINQ = 0, >> - PDS_CORE_QTYPE_NOTIFYQ = 1, >> - PDS_CORE_QTYPE_RXQ = 2, >> - PDS_CORE_QTYPE_TXQ = 3, >> - PDS_CORE_QTYPE_EQ = 4, >> - >> - PDS_CORE_QTYPE_MAX = 16 /* don't change - used in struct size */ >> -}; >> - >> int pdsc_register_notify(struct notifier_block *nb); >> void pdsc_unregister_notify(struct notifier_block *nb); >> void *pdsc_get_pf_struct(struct pci_dev *vf_pdev); >> -- >> 2.17.1 >
diff --git a/include/linux/pds/pds_adminq.h b/include/linux/pds/pds_adminq.h index 98a60ce87b92..61b0a8634e1a 100644 --- a/include/linux/pds/pds_adminq.h +++ b/include/linux/pds/pds_adminq.h @@ -222,6 +222,27 @@ enum pds_core_lif_type { PDS_CORE_LIF_TYPE_DEFAULT = 0, }; +#define PDS_CORE_IFNAMSIZ 16 + +/** + * enum pds_core_logical_qtype - Logical Queue Types + * @PDS_CORE_QTYPE_ADMINQ: Administrative Queue + * @PDS_CORE_QTYPE_NOTIFYQ: Notify Queue + * @PDS_CORE_QTYPE_RXQ: Receive Queue + * @PDS_CORE_QTYPE_TXQ: Transmit Queue + * @PDS_CORE_QTYPE_EQ: Event Queue + * @PDS_CORE_QTYPE_MAX: Max queue type supported + */ +enum pds_core_logical_qtype { + PDS_CORE_QTYPE_ADMINQ = 0, + PDS_CORE_QTYPE_NOTIFYQ = 1, + PDS_CORE_QTYPE_RXQ = 2, + PDS_CORE_QTYPE_TXQ = 3, + PDS_CORE_QTYPE_EQ = 4, + + PDS_CORE_QTYPE_MAX = 16 /* don't change - used in struct size */ +}; + /** * union pds_core_lif_config - LIF configuration * @state: LIF state (enum pds_core_lif_state) diff --git a/include/linux/pds/pds_common.h b/include/linux/pds/pds_common.h index 2a0d1669cfd0..435c8e8161c2 100644 --- a/include/linux/pds/pds_common.h +++ b/include/linux/pds/pds_common.h @@ -41,27 +41,6 @@ enum pds_core_vif_types { #define PDS_VDPA_DEV_NAME PDS_CORE_DRV_NAME "." PDS_DEV_TYPE_VDPA_STR -#define PDS_CORE_IFNAMSIZ 16 - -/** - * enum pds_core_logical_qtype - Logical Queue Types - * @PDS_CORE_QTYPE_ADMINQ: Administrative Queue - * @PDS_CORE_QTYPE_NOTIFYQ: Notify Queue - * @PDS_CORE_QTYPE_RXQ: Receive Queue - * @PDS_CORE_QTYPE_TXQ: Transmit Queue - * @PDS_CORE_QTYPE_EQ: Event Queue - * @PDS_CORE_QTYPE_MAX: Max queue type supported - */ -enum pds_core_logical_qtype { - PDS_CORE_QTYPE_ADMINQ = 0, - PDS_CORE_QTYPE_NOTIFYQ = 1, - PDS_CORE_QTYPE_RXQ = 2, - PDS_CORE_QTYPE_TXQ = 3, - PDS_CORE_QTYPE_EQ = 4, - - PDS_CORE_QTYPE_MAX = 16 /* don't change - used in struct size */ -}; - int pdsc_register_notify(struct notifier_block *nb); void pdsc_unregister_notify(struct notifier_block *nb); void *pdsc_get_pf_struct(struct pci_dev *vf_pdev);
The pds_core_logical_qtype enum and IFNAMSIZ are not needed in the common PDS header, only needed when working with the adminq, so move them to the adminq header. Note: This patch might conflict with pds_vfio patches that are in review, depending on which patchset gets pulled first. Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> Acked-by: Jason Wang <jasowang@redhat.com> --- include/linux/pds/pds_adminq.h | 21 +++++++++++++++++++++ include/linux/pds/pds_common.h | 21 --------------------- 2 files changed, 21 insertions(+), 21 deletions(-)