| Message ID | 20180109155445.31999-1-mchandras@suse.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,branch-2.8] netdev-dpdk: replace uint8_t with dpdk_port_t | expand |
> -----Original Message----- > From: Markos Chandras [mailto:mchandras@suse.de] > Sent: Tuesday, January 9, 2018 3:55 PM > To: dev@openvswitch.org > Cc: Kavanagh, Mark B <mark.b.kavanagh@intel.com>; Ilya Maximets > <i.maximets@samsung.com>; Stokes, Ian <ian.stokes@intel.com>; Markos > Chandras <mchandras@suse.de> > Subject: [PATCH branch-2.8] netdev-dpdk: replace uint8_t with dpdk_port_t > > From: Mark Kavanagh <mark.b.kavanagh@intel.com> > > netdev_dpdk_detach() declares a 'port_id' variable, of type uint8_t. > This variable should instead be of type dpdk_port_t. Hi Markos, is there a specific reason you require this patch back ported to 2.8? OVS 2.8 supports DPDK 17.05.2, looking at the prototype of the rte_eth_dev_detach() function in DPDK uint8_t is correct. rte_eth_dev_detach(uint8_t port_id, char *name); I would have thought this change is only required if using DPDK 17.11 which will be part of OVS 2.9. Maybe I've missed something. Thanks Ian > > Fixes: bb37956ac ("netdev-dpdk: Use uint8_t for port_id.") > CC: Ilya Maximets <i.maximets@samsung.com> > Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com> > Acked-by: Ilya Maximets <i.maximets@samsung.com> > Signed-off-by: Ian Stokes <ian.stokes@intel.com> > Signed-off-by: Markos Chandras <mchandras@suse.de> > --- > lib/netdev-dpdk.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index > 41acb5b62..dc96d7ce3 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -2521,7 +2521,7 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int > argc OVS_UNUSED, { > int ret; > char *response; > - uint8_t port_id; > + dpdk_port_t port_id; > char devname[RTE_ETH_NAME_MAX_LEN]; > struct netdev_dpdk *dev; > > -- > 2.15.1
On Tue, Jan 09, 2018 at 03:54:45PM +0000, Markos Chandras wrote: > From: Mark Kavanagh <mark.b.kavanagh@intel.com> > > netdev_dpdk_detach() declares a 'port_id' variable, of type uint8_t. > This variable should instead be of type dpdk_port_t. > > Fixes: bb37956ac ("netdev-dpdk: Use uint8_t for port_id.") > CC: Ilya Maximets <i.maximets@samsung.com> > Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com> > Acked-by: Ilya Maximets <i.maximets@samsung.com> > Signed-off-by: Ian Stokes <ian.stokes@intel.com> > Signed-off-by: Markos Chandras <mchandras@suse.de> Applied to branch-2.8, thanks!
Hi Ian, On 09/01/18 16:16, Stokes, Ian wrote: >> -----Original Message----- >> From: Markos Chandras [mailto:mchandras@suse.de] >> Sent: Tuesday, January 9, 2018 3:55 PM >> To: dev@openvswitch.org >> Cc: Kavanagh, Mark B <mark.b.kavanagh@intel.com>; Ilya Maximets >> <i.maximets@samsung.com>; Stokes, Ian <ian.stokes@intel.com>; Markos >> Chandras <mchandras@suse.de> >> Subject: [PATCH branch-2.8] netdev-dpdk: replace uint8_t with dpdk_port_t >> >> From: Mark Kavanagh <mark.b.kavanagh@intel.com> >> >> netdev_dpdk_detach() declares a 'port_id' variable, of type uint8_t. >> This variable should instead be of type dpdk_port_t. > > Hi Markos, is there a specific reason you require this patch back ported to 2.8? No particular reason I just thought to point that out since in other parts of this file dpdk_port_t is used for the port_id, for example in the netdev_dpdk_process_devargs() and netdev_dpdk_set_config() functions so maybe worth fixing these inconsistencies.
> -----Original Message----- > From: Markos Chandras [mailto:mchandras@suse.de] > Sent: Tuesday, January 9, 2018 4:23 PM > To: Stokes, Ian <ian.stokes@intel.com>; dev@openvswitch.org > Cc: Kavanagh, Mark B <mark.b.kavanagh@intel.com>; Ilya Maximets > <i.maximets@samsung.com> > Subject: Re: [PATCH branch-2.8] netdev-dpdk: replace uint8_t with > dpdk_port_t > > Hi Ian, > > On 09/01/18 16:16, Stokes, Ian wrote: > >> -----Original Message----- > >> From: Markos Chandras [mailto:mchandras@suse.de] > >> Sent: Tuesday, January 9, 2018 3:55 PM > >> To: dev@openvswitch.org > >> Cc: Kavanagh, Mark B <mark.b.kavanagh@intel.com>; Ilya Maximets > >> <i.maximets@samsung.com>; Stokes, Ian <ian.stokes@intel.com>; Markos > >> Chandras <mchandras@suse.de> > >> Subject: [PATCH branch-2.8] netdev-dpdk: replace uint8_t with > >> dpdk_port_t > >> > >> From: Mark Kavanagh <mark.b.kavanagh@intel.com> > >> > >> netdev_dpdk_detach() declares a 'port_id' variable, of type uint8_t. > >> This variable should instead be of type dpdk_port_t. > > > > Hi Markos, is there a specific reason you require this patch back ported > to 2.8? > > No particular reason I just thought to point that out since in other parts > of this file dpdk_port_t is used for the port_id, for example in the > netdev_dpdk_process_devargs() and netdev_dpdk_set_config() functions so > maybe worth fixing these inconsistencies. Sure, I was just concerned was it fixing a compilation issue or such for you. I've seen it's been applied already and I've given it a quick validation check without issue so no worries. Thanks Ian > > -- > markos > > SUSE LINUX GmbH | GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB > 21284 (AG Nürnberg) Maxfeldstr. 5, D-90409, Nürnberg
On Tue, Jan 09, 2018 at 04:43:32PM +0000, Stokes, Ian wrote: > > > > -----Original Message----- > > From: Markos Chandras [mailto:mchandras@suse.de] > > Sent: Tuesday, January 9, 2018 4:23 PM > > To: Stokes, Ian <ian.stokes@intel.com>; dev@openvswitch.org > > Cc: Kavanagh, Mark B <mark.b.kavanagh@intel.com>; Ilya Maximets > > <i.maximets@samsung.com> > > Subject: Re: [PATCH branch-2.8] netdev-dpdk: replace uint8_t with > > dpdk_port_t > > > > Hi Ian, > > > > On 09/01/18 16:16, Stokes, Ian wrote: > > >> -----Original Message----- > > >> From: Markos Chandras [mailto:mchandras@suse.de] > > >> Sent: Tuesday, January 9, 2018 3:55 PM > > >> To: dev@openvswitch.org > > >> Cc: Kavanagh, Mark B <mark.b.kavanagh@intel.com>; Ilya Maximets > > >> <i.maximets@samsung.com>; Stokes, Ian <ian.stokes@intel.com>; Markos > > >> Chandras <mchandras@suse.de> > > >> Subject: [PATCH branch-2.8] netdev-dpdk: replace uint8_t with > > >> dpdk_port_t > > >> > > >> From: Mark Kavanagh <mark.b.kavanagh@intel.com> > > >> > > >> netdev_dpdk_detach() declares a 'port_id' variable, of type uint8_t. > > >> This variable should instead be of type dpdk_port_t. > > > > > > Hi Markos, is there a specific reason you require this patch back ported > > to 2.8? > > > > No particular reason I just thought to point that out since in other parts > > of this file dpdk_port_t is used for the port_id, for example in the > > netdev_dpdk_process_devargs() and netdev_dpdk_set_config() functions so > > maybe worth fixing these inconsistencies. > > Sure, I was just concerned was it fixing a compilation issue or such for you. I've seen it's been applied already and I've given it a quick validation check without issue so no worries. I interpreted your Signed-off-by as a request to apply it, but I think I must have misunderstood a backport of a patch already on master. I won't apply it so quickly next time. I'm learning here too :-)
Hi Ben, On 09/01/18 16:56, Ben Pfaff wrote: >> Sure, I was just concerned was it fixing a compilation issue or such for you. I've seen it's been applied already and I've given it a quick validation check without issue so no worries. > > I interpreted your Signed-off-by as a request to apply it, but I think I > must have misunderstood a backport of a patch already on master. I > won't apply it so quickly next time. I'm learning here too :-) > Oh should I have done something different to make it clear that it was a backport? I simply took the patch from master, modified the Subject line to indicate the branch for the backport and added my SoB line. Might be better to use 'cherry-pick -x' for backports to make it more explicit. I apologize for any confusion this may have caused
> Hi Ben, > > On 09/01/18 16:56, Ben Pfaff wrote: > >> Sure, I was just concerned was it fixing a compilation issue or such > for you. I've seen it's been applied already and I've given it a quick > validation check without issue so no worries. > > > > I interpreted your Signed-off-by as a request to apply it, but I think > > I must have misunderstood a backport of a patch already on master. I > > won't apply it so quickly next time. I'm learning here too :-) No problem, I made the same assumption when I first looked at the patch. > > > > Oh should I have done something different to make it clear that it was a > backport? I simply took the patch from master, modified the Subject line > to indicate the branch for the backport and added my SoB line. Might be > better to use 'cherry-pick -x' for backports to make it more explicit. I > apologize for any confusion this may have caused I think the approach you took is the agreed upon approach (specifying the branch that the patch is targeted at). The confusion lay in the sign offs :). Ian > > -- > markos > > SUSE LINUX GmbH | GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB > 21284 (AG Nürnberg) Maxfeldstr. 5, D-90409, Nürnberg
On Tue, Jan 09, 2018 at 05:02:11PM +0000, Markos Chandras wrote: > Hi Ben, > > On 09/01/18 16:56, Ben Pfaff wrote: > >> Sure, I was just concerned was it fixing a compilation issue or such for you. I've seen it's been applied already and I've given it a quick validation check without issue so no worries. > > > > I interpreted your Signed-off-by as a request to apply it, but I think I > > must have misunderstood a backport of a patch already on master. I > > won't apply it so quickly next time. I'm learning here too :-) > > > > Oh should I have done something different to make it clear that it was a > backport? I simply took the patch from master, modified the Subject line > to indicate the branch for the backport and added my SoB line. Might be > better to use 'cherry-pick -x' for backports to make it more explicit. I > apologize for any confusion this may have caused I think you did OK, I just wasn't paying attention carefully enough.
On Tue, Jan 09, 2018 at 09:27:11AM -0800, Ben Pfaff wrote: > On Tue, Jan 09, 2018 at 05:02:11PM +0000, Markos Chandras wrote: > > Hi Ben, > > > > On 09/01/18 16:56, Ben Pfaff wrote: > > >> Sure, I was just concerned was it fixing a compilation issue or such for you. I've seen it's been applied already and I've given it a quick validation check without issue so no worries. > > > > > > I interpreted your Signed-off-by as a request to apply it, but I think I > > > must have misunderstood a backport of a patch already on master. I > > > won't apply it so quickly next time. I'm learning here too :-) > > > > > > > Oh should I have done something different to make it clear that it was a > > backport? I simply took the patch from master, modified the Subject line > > to indicate the branch for the backport and added my SoB line. Might be > > better to use 'cherry-pick -x' for backports to make it more explicit. I > > apologize for any confusion this may have caused > > I think you did OK, I just wasn't paying attention carefully enough. For what it's worth, one reason I didn't apply more scrutiny is that the patch seemed harmless at worst.
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 41acb5b62..dc96d7ce3 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -2521,7 +2521,7 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int argc OVS_UNUSED, { int ret; char *response; - uint8_t port_id; + dpdk_port_t port_id; char devname[RTE_ETH_NAME_MAX_LEN]; struct netdev_dpdk *dev;