| Message ID | 29b6e300-37ed-c880-de09-b8102bc4ae16@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
I'm not a C/C++ developer. Well this is very far away from my memory. I changed the following line: + error = netdev_linux_get_ethtool(name, miimon); to + error = netdev_linux_get_ethtool(&name, &miimon); as it looks like we're sending adresses to functions instead of the values. This should work and it passed the "make check". Correct me if I'm wrong please! Dave On 09/25/2016 12:06 PM, David Hill wrote: > From 2748f6325facb7d5d880d620041e2f11767abe0c Mon Sep 17 00:00:00 2001 > From: David Hill <dhill@redhat.com> > Date: Tue, 30 Aug 2016 15:13:31 -0400 > Subject: [PATCH 1/1] netdev-linux.c - Trying to use miimon and on > failure failback to ethtool > > Some network drivers might return true to SIOCGMIIPHY and an error on > SIOCGMIIREG, this patch will > simply fallback to ethtool if this happens and failover should still > happen on an OVS using such nics. > > Signed-off-by: David Hill <dhill@redhat.com> > > --- > lib/netdev-linux.c | 50 > ++++++++++++++++++++++++++++++-------------------- > 1 file changed, 30 insertions(+), 20 deletions(-) > > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c > index efc9527..05aa91f 100644 > --- a/lib/netdev-linux.c > +++ b/lib/netdev-linux.c > @@ -1426,6 +1426,33 @@ netdev_linux_do_miimon(const char *name, int > cmd, const char *cmd_name, > } > > static int > +netdev_linux_get_ethtool(const char *name, bool *miimon) > +{ > + struct mii_ioctl_data data; > + int error; > + > + *miimon = false; > + > + struct ethtool_cmd ecmd; > + VLOG_DBG_RL(&rl, "%s: failed to query MII, falling back to ethtool", > + name); > + > + COVERAGE_INC(netdev_get_ethtool); > + memset(&ecmd, 0, sizeof ecmd); > + error = netdev_linux_do_ethtool(name, &ecmd, ETHTOOL_GLINK, > + "ETHTOOL_GLINK"); > + if (!error) { > + struct ethtool_value eval; > + memcpy(&eval, &ecmd, sizeof eval); > + *miimon = !!eval.data; > + } else { > + VLOG_WARN_RL(&rl, "%s: ethtool link status failed", name); > + } > + > + return error; > +} > + > +static int > netdev_linux_get_miimon(const char *name, bool *miimon) > { > struct mii_ioctl_data data; > @@ -1443,28 +1470,11 @@ netdev_linux_get_miimon(const char *name, bool > *miimon) > if (!error) { > *miimon = !!(data.val_out & BMSR_LSTATUS); > - } else { > - VLOG_WARN_RL(&rl, "%s: failed to query MII", name); > - } > - } else { > - struct ethtool_cmd ecmd; > - > - VLOG_DBG_RL(&rl, "%s: failed to query MII, falling back to > ethtool", > - name); > - > - COVERAGE_INC(netdev_get_ethtool); > - memset(&ecmd, 0, sizeof ecmd); > - error = netdev_linux_do_ethtool(name, &ecmd, ETHTOOL_GLINK, > - "ETHTOOL_GLINK"); > - if (!error) { > - struct ethtool_value eval; > - > - memcpy(&eval, &ecmd, sizeof eval); > - *miimon = !!eval.data; > - } else { > - VLOG_WARN_RL(&rl, "%s: ethtool link status failed", name); > } > } > + if (error) { > + error = netdev_linux_get_ethtool(&name, &miimon); > + } > > return error; > } > -- > 2.7.4 >
Hi David, Thanks for the submission. Since this is a bugfix for a long-running behaviour, I reduced the patch to just the functional changes and applied it to master, branch-2.6 and branch-2.5. Here's the final change, please let me know if it is still missing something: https://github.com/openvswitch/ovs/commit/9120cfc05cd49d4ba1a47eb97e6407e72a5d33f7 I had trouble applying your patch but I was able to work around it. In future please use "git format-patch" to format a patch, and if you can, "git send-email" to send it. These take care of ensuring that the patch arrives correctly to the mailinglist. For bugfixes, the change should also be as small as possible to make it easier to review the core change and less likely to have unintended consequences. Refactoring and cosmetic changes should be separate. Did you ever get confirmation from upstream that the fix was applied to Linux? On 25 September 2016 at 09:18, David Hill <dhill@redhat.com> wrote: > I'm not a C/C++ developer. Well this is very far away from my memory. I > changed the following line: > > + error = netdev_linux_get_ethtool(name, miimon); > to > + error = netdev_linux_get_ethtool(&name, &miimon); > > as it looks like we're sending adresses to functions instead of the values. > This should work and it > passed the "make check". > > Correct me if I'm wrong please! > > Dave > > > On 09/25/2016 12:06 PM, David Hill wrote: >> >> From 2748f6325facb7d5d880d620041e2f11767abe0c Mon Sep 17 00:00:00 2001 >> From: David Hill <dhill@redhat.com> >> Date: Tue, 30 Aug 2016 15:13:31 -0400 >> Subject: [PATCH 1/1] netdev-linux.c - Trying to use miimon and on failure >> failback to ethtool >> >> Some network drivers might return true to SIOCGMIIPHY and an error on >> SIOCGMIIREG, this patch will >> simply fallback to ethtool if this happens and failover should still >> happen on an OVS using such nics. >> >> Signed-off-by: David Hill <dhill@redhat.com> >> >> --- >> lib/netdev-linux.c | 50 >> ++++++++++++++++++++++++++++++-------------------- >> 1 file changed, 30 insertions(+), 20 deletions(-) >> >> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c >> index efc9527..05aa91f 100644 >> --- a/lib/netdev-linux.c >> +++ b/lib/netdev-linux.c >> @@ -1426,6 +1426,33 @@ netdev_linux_do_miimon(const char *name, int cmd, >> const char *cmd_name, >> } >> >> static int >> +netdev_linux_get_ethtool(const char *name, bool *miimon) >> +{ >> + struct mii_ioctl_data data; >> + int error; >> + >> + *miimon = false; >> + >> + struct ethtool_cmd ecmd; >> + VLOG_DBG_RL(&rl, "%s: failed to query MII, falling back to ethtool", >> + name); >> + >> + COVERAGE_INC(netdev_get_ethtool); >> + memset(&ecmd, 0, sizeof ecmd); >> + error = netdev_linux_do_ethtool(name, &ecmd, ETHTOOL_GLINK, >> + "ETHTOOL_GLINK"); >> + if (!error) { >> + struct ethtool_value eval; >> + memcpy(&eval, &ecmd, sizeof eval); >> + *miimon = !!eval.data; >> + } else { >> + VLOG_WARN_RL(&rl, "%s: ethtool link status failed", name); >> + } >> + >> + return error; >> +} >> + >> +static int >> netdev_linux_get_miimon(const char *name, bool *miimon) >> { >> struct mii_ioctl_data data; >> @@ -1443,28 +1470,11 @@ netdev_linux_get_miimon(const char *name, bool >> *miimon) >> if (!error) { >> *miimon = !!(data.val_out & BMSR_LSTATUS); >> - } else { >> - VLOG_WARN_RL(&rl, "%s: failed to query MII", name); >> - } >> - } else { >> - struct ethtool_cmd ecmd; >> - >> - VLOG_DBG_RL(&rl, "%s: failed to query MII, falling back to >> ethtool", >> - name); >> - >> - COVERAGE_INC(netdev_get_ethtool); >> - memset(&ecmd, 0, sizeof ecmd); >> - error = netdev_linux_do_ethtool(name, &ecmd, ETHTOOL_GLINK, >> - "ETHTOOL_GLINK"); >> - if (!error) { >> - struct ethtool_value eval; >> - >> - memcpy(&eval, &ecmd, sizeof eval); >> - *miimon = !!eval.data; >> - } else { >> - VLOG_WARN_RL(&rl, "%s: ethtool link status failed", name); >> } >> } >> + if (error) { >> + error = netdev_linux_get_ethtool(&name, &miimon); >> + } >> >> return error; >> } >> -- >> 2.7.4 >> > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index efc9527..05aa91f 100644 --- a/lib/netdev-linux.c +++ b/lib/netdev-linux.c @@ -1426,6 +1426,33 @@ netdev_linux_do_miimon(const char *name, int cmd, const char *cmd_name, } static int +netdev_linux_get_ethtool(const char *name, bool *miimon) +{ + struct mii_ioctl_data data; + int error; + + *miimon = false; + + struct ethtool_cmd ecmd; + VLOG_DBG_RL(&rl, "%s: failed to query MII, falling back to ethtool", + name); + + COVERAGE_INC(netdev_get_ethtool); + memset(&ecmd, 0, sizeof ecmd); + error = netdev_linux_do_ethtool(name, &ecmd, ETHTOOL_GLINK, + "ETHTOOL_GLINK"); + if (!error) { + struct ethtool_value eval; + memcpy(&eval, &ecmd, sizeof eval); + *miimon = !!eval.data; + } else { + VLOG_WARN_RL(&rl, "%s: ethtool link status failed", name); + } + + return error; +} + +static int netdev_linux_get_miimon(const char *name, bool *miimon) {