* [PATCH net-next] bonding: Use do_div to divide 64 bit numbers
@ 2014-01-23 18:47 Zoltan Kiss
2014-01-23 20:44 ` Nikolay Aleksandrov
2014-01-23 22:25 ` Sergei Shtylyov
0 siblings, 2 replies; 7+ messages in thread
From: Zoltan Kiss @ 2014-01-23 18:47 UTC (permalink / raw)
To: Jay Vosburgh, Veaceslav Falico, Andy Gospodarek, netdev,
linux-kernel, Nikolay Aleksandrov
Cc: Zoltan Kiss
Nikolay Aleksandrov's recent bonding option API changes (25a9b54a and e4994612)
introduced u64 as the type of downdelay and updelay. On 32 bit the division and
modulo operations cause compile errors:
ERROR: "__udivdi3" [drivers/net/bonding/bonding.ko] undefined!
ERROR: "__umoddi3" [drivers/net/bonding/bonding.ko] undefined!
This patch use the do_div macro, which guaranteed to do the right thing.
Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com>
---
drivers/net/bonding/bond_options.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 4cee04a..4f94907 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -18,6 +18,7 @@
#include <linux/rcupdate.h>
#include <linux/ctype.h>
#include <linux/inet.h>
+#include <asm/div64.h>
#include "bonding.h"
static struct bond_opt_value bond_mode_tbl[] = {
@@ -727,19 +728,20 @@ int bond_option_miimon_set(struct bonding *bond, struct bond_opt_value *newval)
int bond_option_updelay_set(struct bonding *bond, struct bond_opt_value *newval)
{
+ u64 quotient = newval->value;
+ u64 remainder = do_div(quotient, bond->params.miimon);
if (!bond->params.miimon) {
pr_err("%s: Unable to set up delay as MII monitoring is disabled\n",
bond->dev->name);
return -EPERM;
}
- if ((newval->value % bond->params.miimon) != 0) {
+ if (remainder != 0) {
pr_warn("%s: Warning: up delay (%llu) is not a multiple of miimon (%d), updelay rounded to %llu ms\n",
bond->dev->name, newval->value,
bond->params.miimon,
- (newval->value / bond->params.miimon) *
- bond->params.miimon);
+ quotient * bond->params.miimon);
}
- bond->params.updelay = newval->value / bond->params.miimon;
+ bond->params.updelay = quotient;
pr_info("%s: Setting up delay to %d.\n",
bond->dev->name,
bond->params.updelay * bond->params.miimon);
@@ -750,19 +752,20 @@ int bond_option_updelay_set(struct bonding *bond, struct bond_opt_value *newval)
int bond_option_downdelay_set(struct bonding *bond,
struct bond_opt_value *newval)
{
+ u64 quotient = newval->value;
+ u64 remainder = do_div(quotient, bond->params.miimon);
if (!bond->params.miimon) {
pr_err("%s: Unable to set down delay as MII monitoring is disabled\n",
bond->dev->name);
return -EPERM;
}
- if ((newval->value % bond->params.miimon) != 0) {
+ if (remainder != 0) {
pr_warn("%s: Warning: down delay (%llu) is not a multiple of miimon (%d), delay rounded to %llu ms\n",
bond->dev->name, newval->value,
bond->params.miimon,
- (newval->value / bond->params.miimon) *
- bond->params.miimon);
+ quotient * bond->params.miimon);
}
- bond->params.downdelay = newval->value / bond->params.miimon;
+ bond->params.downdelay = quotient;
pr_info("%s: Setting down delay to %d.\n",
bond->dev->name,
bond->params.downdelay * bond->params.miimon);
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH net-next] bonding: Use do_div to divide 64 bit numbers
2014-01-23 18:47 [PATCH net-next] bonding: Use do_div to divide 64 bit numbers Zoltan Kiss
@ 2014-01-23 20:44 ` Nikolay Aleksandrov
2014-01-23 21:18 ` Nikolay Aleksandrov
2014-01-23 22:25 ` Sergei Shtylyov
1 sibling, 1 reply; 7+ messages in thread
From: Nikolay Aleksandrov @ 2014-01-23 20:44 UTC (permalink / raw)
To: Zoltan Kiss, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek,
netdev, linux-kernel
On 01/23/2014 07:47 PM, Zoltan Kiss wrote:
> Nikolay Aleksandrov's recent bonding option API changes (25a9b54a and e4994612)
> introduced u64 as the type of downdelay and updelay. On 32 bit the division and
> modulo operations cause compile errors:
>
> ERROR: "__udivdi3" [drivers/net/bonding/bonding.ko] undefined!
> ERROR: "__umoddi3" [drivers/net/bonding/bonding.ko] undefined!
>
> This patch use the do_div macro, which guaranteed to do the right thing.
>
> Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com>
> ---
> drivers/net/bonding/bond_options.c | 19 +++++++++++--------
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
> index 4cee04a..4f94907 100644
> --- a/drivers/net/bonding/bond_options.c
> +++ b/drivers/net/bonding/bond_options.c
> @@ -18,6 +18,7 @@
> #include <linux/rcupdate.h>
> #include <linux/ctype.h>
> #include <linux/inet.h>
> +#include <asm/div64.h>
> #include "bonding.h"
>
> static struct bond_opt_value bond_mode_tbl[] = {
> @@ -727,19 +728,20 @@ int bond_option_miimon_set(struct bonding *bond, struct bond_opt_value *newval)
>
> int bond_option_updelay_set(struct bonding *bond, struct bond_opt_value *newval)
> {
> + u64 quotient = newval->value;
> + u64 remainder = do_div(quotient, bond->params.miimon);
Hi Zoltan,
Thanks for fixing this, a few comments though:
bond->params.miimon can be 0 here that's why there's a check afterwards,
also please separate the local variable definitions from the body with a
new line.
The same applies for downdelay.
Nik
> if (!bond->params.miimon) {
> pr_err("%s: Unable to set up delay as MII monitoring is disabled\n",
> bond->dev->name);
> return -EPERM;
> }
> - if ((newval->value % bond->params.miimon) != 0) {
> + if (remainder != 0) {
> pr_warn("%s: Warning: up delay (%llu) is not a multiple of miimon (%d), updelay rounded to %llu ms\n",
> bond->dev->name, newval->value,
> bond->params.miimon,
> - (newval->value / bond->params.miimon) *
> - bond->params.miimon);
> + quotient * bond->params.miimon);
> }
> - bond->params.updelay = newval->value / bond->params.miimon;
> + bond->params.updelay = quotient;
> pr_info("%s: Setting up delay to %d.\n",
> bond->dev->name,
> bond->params.updelay * bond->params.miimon);
> @@ -750,19 +752,20 @@ int bond_option_updelay_set(struct bonding *bond, struct bond_opt_value *newval)
> int bond_option_downdelay_set(struct bonding *bond,
> struct bond_opt_value *newval)
> {
> + u64 quotient = newval->value;
> + u64 remainder = do_div(quotient, bond->params.miimon);
> if (!bond->params.miimon) {
> pr_err("%s: Unable to set down delay as MII monitoring is disabled\n",
> bond->dev->name);
> return -EPERM;
> }
> - if ((newval->value % bond->params.miimon) != 0) {
> + if (remainder != 0) {
> pr_warn("%s: Warning: down delay (%llu) is not a multiple of miimon (%d), delay rounded to %llu ms\n",
> bond->dev->name, newval->value,
> bond->params.miimon,
> - (newval->value / bond->params.miimon) *
> - bond->params.miimon);
> + quotient * bond->params.miimon);
> }
> - bond->params.downdelay = newval->value / bond->params.miimon;
> + bond->params.downdelay = quotient;
> pr_info("%s: Setting down delay to %d.\n",
> bond->dev->name,
> bond->params.downdelay * bond->params.miimon);
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH net-next] bonding: Use do_div to divide 64 bit numbers
2014-01-23 20:44 ` Nikolay Aleksandrov
@ 2014-01-23 21:18 ` Nikolay Aleksandrov
2014-01-23 22:43 ` [PATCH net-next] bonding: fix u64 division Nikolay Aleksandrov
2014-01-24 18:27 ` [PATCH net-next] bonding: Use do_div to divide 64 bit numbers Zoltan Kiss
0 siblings, 2 replies; 7+ messages in thread
From: Nikolay Aleksandrov @ 2014-01-23 21:18 UTC (permalink / raw)
To: Zoltan Kiss, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek,
netdev, linux-kernel
On 01/23/2014 09:44 PM, Nikolay Aleksandrov wrote:
> On 01/23/2014 07:47 PM, Zoltan Kiss wrote:
>> Nikolay Aleksandrov's recent bonding option API changes (25a9b54a and e4994612)
>> introduced u64 as the type of downdelay and updelay. On 32 bit the division and
>> modulo operations cause compile errors:
>>
>> ERROR: "__udivdi3" [drivers/net/bonding/bonding.ko] undefined!
>> ERROR: "__umoddi3" [drivers/net/bonding/bonding.ko] undefined!
>>
>> This patch use the do_div macro, which guaranteed to do the right thing.
>>
>> Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com>
>> ---
>> drivers/net/bonding/bond_options.c | 19 +++++++++++--------
>> 1 file changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
>> index 4cee04a..4f94907 100644
>> --- a/drivers/net/bonding/bond_options.c
>> +++ b/drivers/net/bonding/bond_options.c
>> @@ -18,6 +18,7 @@
>> #include <linux/rcupdate.h>
>> #include <linux/ctype.h>
>> #include <linux/inet.h>
>> +#include <asm/div64.h>
>> #include "bonding.h"
>>
>> static struct bond_opt_value bond_mode_tbl[] = {
>> @@ -727,19 +728,20 @@ int bond_option_miimon_set(struct bonding *bond, struct bond_opt_value *newval)
>>
>> int bond_option_updelay_set(struct bonding *bond, struct bond_opt_value *newval)
>> {
>> + u64 quotient = newval->value;
>> + u64 remainder = do_div(quotient, bond->params.miimon);
> Hi Zoltan,
> Thanks for fixing this, a few comments though:
> bond->params.miimon can be 0 here that's why there's a check afterwards,
> also please separate the local variable definitions from the body with a
> new line.
> The same applies for downdelay.
>
> Nik
In fact since we don't need the u64 and newval->value is limited to
INT_MAX, can't we simply cast it to (int) and avoid the do_div entirely ?
>> if (!bond->params.miimon) {
>> pr_err("%s: Unable to set up delay as MII monitoring is disabled\n",
>> bond->dev->name);
>> return -EPERM;
>> }
>> - if ((newval->value % bond->params.miimon) != 0) {
>> + if (remainder != 0) {
>> pr_warn("%s: Warning: up delay (%llu) is not a multiple of miimon (%d), updelay rounded to %llu ms\n",
>> bond->dev->name, newval->value,
>> bond->params.miimon,
>> - (newval->value / bond->params.miimon) *
>> - bond->params.miimon);
>> + quotient * bond->params.miimon);
>> }
>> - bond->params.updelay = newval->value / bond->params.miimon;
>> + bond->params.updelay = quotient;
>> pr_info("%s: Setting up delay to %d.\n",
>> bond->dev->name,
>> bond->params.updelay * bond->params.miimon);
>> @@ -750,19 +752,20 @@ int bond_option_updelay_set(struct bonding *bond, struct bond_opt_value *newval)
>> int bond_option_downdelay_set(struct bonding *bond,
>> struct bond_opt_value *newval)
>> {
>> + u64 quotient = newval->value;
>> + u64 remainder = do_div(quotient, bond->params.miimon);
>> if (!bond->params.miimon) {
>> pr_err("%s: Unable to set down delay as MII monitoring is disabled\n",
>> bond->dev->name);
>> return -EPERM;
>> }
>> - if ((newval->value % bond->params.miimon) != 0) {
>> + if (remainder != 0) {
>> pr_warn("%s: Warning: down delay (%llu) is not a multiple of miimon (%d), delay rounded to %llu ms\n",
>> bond->dev->name, newval->value,
>> bond->params.miimon,
>> - (newval->value / bond->params.miimon) *
>> - bond->params.miimon);
>> + quotient * bond->params.miimon);
>> }
>> - bond->params.downdelay = newval->value / bond->params.miimon;
>> + bond->params.downdelay = quotient;
>> pr_info("%s: Setting down delay to %d.\n",
>> bond->dev->name,
>> bond->params.downdelay * bond->params.miimon);
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH net-next] bonding: fix u64 division
2014-01-23 21:18 ` Nikolay Aleksandrov
@ 2014-01-23 22:43 ` Nikolay Aleksandrov
2014-01-24 0:12 ` David Miller
2014-01-24 18:27 ` [PATCH net-next] bonding: Use do_div to divide 64 bit numbers Zoltan Kiss
1 sibling, 1 reply; 7+ messages in thread
From: Nikolay Aleksandrov @ 2014-01-23 22:43 UTC (permalink / raw)
To: netdev; +Cc: davem, zoltan.kiss, andy, vfalico, fubar, nikolay
After the option conversion downdelay and updelay divide a u64
and on a 32 bit this causes the following errors:
ERROR: "__udivdi3" [drivers/net/bonding/bonding.ko] undefined!
ERROR: "__umoddi3" [drivers/net/bonding/bonding.ko] undefined!
Fix it by using a normal int instead because newval->value is capped
at INT_MAX by the way the option is defined.
Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
This is what I meant, but it's only compile-tested, I can't
test it until tomorrow.
drivers/net/bonding/bond_options.c | 24 ++++++++++++++----------
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 4cee04a..11cb943 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -727,19 +727,21 @@ int bond_option_miimon_set(struct bonding *bond, struct bond_opt_value *newval)
int bond_option_updelay_set(struct bonding *bond, struct bond_opt_value *newval)
{
+ int value = newval->value;
+
if (!bond->params.miimon) {
pr_err("%s: Unable to set up delay as MII monitoring is disabled\n",
bond->dev->name);
return -EPERM;
}
- if ((newval->value % bond->params.miimon) != 0) {
- pr_warn("%s: Warning: up delay (%llu) is not a multiple of miimon (%d), updelay rounded to %llu ms\n",
- bond->dev->name, newval->value,
+ if ((value % bond->params.miimon) != 0) {
+ pr_warn("%s: Warning: up delay (%d) is not a multiple of miimon (%d), updelay rounded to %d ms\n",
+ bond->dev->name, value,
bond->params.miimon,
- (newval->value / bond->params.miimon) *
+ (value / bond->params.miimon) *
bond->params.miimon);
}
- bond->params.updelay = newval->value / bond->params.miimon;
+ bond->params.updelay = value / bond->params.miimon;
pr_info("%s: Setting up delay to %d.\n",
bond->dev->name,
bond->params.updelay * bond->params.miimon);
@@ -750,19 +752,21 @@ int bond_option_updelay_set(struct bonding *bond, struct bond_opt_value *newval)
int bond_option_downdelay_set(struct bonding *bond,
struct bond_opt_value *newval)
{
+ int value = newval->value;
+
if (!bond->params.miimon) {
pr_err("%s: Unable to set down delay as MII monitoring is disabled\n",
bond->dev->name);
return -EPERM;
}
- if ((newval->value % bond->params.miimon) != 0) {
- pr_warn("%s: Warning: down delay (%llu) is not a multiple of miimon (%d), delay rounded to %llu ms\n",
- bond->dev->name, newval->value,
+ if ((value % bond->params.miimon) != 0) {
+ pr_warn("%s: Warning: down delay (%d) is not a multiple of miimon (%d), delay rounded to %d ms\n",
+ bond->dev->name, value,
bond->params.miimon,
- (newval->value / bond->params.miimon) *
+ (value / bond->params.miimon) *
bond->params.miimon);
}
- bond->params.downdelay = newval->value / bond->params.miimon;
+ bond->params.downdelay = value / bond->params.miimon;
pr_info("%s: Setting down delay to %d.\n",
bond->dev->name,
bond->params.downdelay * bond->params.miimon);
--
1.8.4.2
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH net-next] bonding: fix u64 division
2014-01-23 22:43 ` [PATCH net-next] bonding: fix u64 division Nikolay Aleksandrov
@ 2014-01-24 0:12 ` David Miller
0 siblings, 0 replies; 7+ messages in thread
From: David Miller @ 2014-01-24 0:12 UTC (permalink / raw)
To: nikolay; +Cc: netdev, zoltan.kiss, andy, vfalico, fubar
From: Nikolay Aleksandrov <nikolay@redhat.com>
Date: Thu, 23 Jan 2014 23:43:43 +0100
> After the option conversion downdelay and updelay divide a u64
> and on a 32 bit this causes the following errors:
> ERROR: "__udivdi3" [drivers/net/bonding/bonding.ko] undefined!
> ERROR: "__umoddi3" [drivers/net/bonding/bonding.ko] undefined!
>
> Fix it by using a normal int instead because newval->value is capped
> at INT_MAX by the way the option is defined.
>
> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
To fix the build I'm applying this, thanks.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] bonding: Use do_div to divide 64 bit numbers
2014-01-23 21:18 ` Nikolay Aleksandrov
2014-01-23 22:43 ` [PATCH net-next] bonding: fix u64 division Nikolay Aleksandrov
@ 2014-01-24 18:27 ` Zoltan Kiss
1 sibling, 0 replies; 7+ messages in thread
From: Zoltan Kiss @ 2014-01-24 18:27 UTC (permalink / raw)
To: Nikolay Aleksandrov, Jay Vosburgh, Veaceslav Falico,
Andy Gospodarek, netdev, linux-kernel
On 23/01/14 21:18, Nikolay Aleksandrov wrote:
>> Hi Zoltan,
>> Thanks for fixing this, a few comments though:
>> bond->params.miimon can be 0 here that's why there's a check afterwards,
>> also please separate the local variable definitions from the body with a
>> new line.
>> The same applies for downdelay.
>>
>> Nik
> In fact since we don't need the u64 and newval->value is limited to
> INT_MAX, can't we simply cast it to (int) and avoid the do_div entirely ?
Yep, that's even better, thanks for fixing it!
Zoli
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] bonding: Use do_div to divide 64 bit numbers
2014-01-23 18:47 [PATCH net-next] bonding: Use do_div to divide 64 bit numbers Zoltan Kiss
2014-01-23 20:44 ` Nikolay Aleksandrov
@ 2014-01-23 22:25 ` Sergei Shtylyov
1 sibling, 0 replies; 7+ messages in thread
From: Sergei Shtylyov @ 2014-01-23 22:25 UTC (permalink / raw)
To: Zoltan Kiss, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek,
netdev, linux-kernel, Nikolay Aleksandrov
Hello.
On 23-01-2014 22:47, Zoltan Kiss wrote:
> Nikolay Aleksandrov's recent bonding option API changes (25a9b54a and e4994612)
You should also specify those commits' summary lines.
> introduced u64 as the type of downdelay and updelay. On 32 bit the division and
> modulo operations cause compile errors:
> ERROR: "__udivdi3" [drivers/net/bonding/bonding.ko] undefined!
> ERROR: "__umoddi3" [drivers/net/bonding/bonding.ko] undefined!
> This patch use the do_div macro, which guaranteed to do the right thing.
> Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com>
> ---
> drivers/net/bonding/bond_options.c | 19 +++++++++++--------
> 1 file changed, 11 insertions(+), 8 deletions(-)
> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
> index 4cee04a..4f94907 100644
> --- a/drivers/net/bonding/bond_options.c
> +++ b/drivers/net/bonding/bond_options.c
[...]
> @@ -727,19 +728,20 @@ int bond_option_miimon_set(struct bonding *bond, struct bond_opt_value *newval)
>
> int bond_option_updelay_set(struct bonding *bond, struct bond_opt_value *newval)
> {
> + u64 quotient = newval->value;
> + u64 remainder = do_div(quotient, bond->params.miimon);
Empty line after declarations wouldn't hurt here.
> @@ -750,19 +752,20 @@ int bond_option_updelay_set(struct bonding *bond, struct bond_opt_value *newval)
> int bond_option_downdelay_set(struct bonding *bond,
> struct bond_opt_value *newval)
> {
> + u64 quotient = newval->value;
> + u64 remainder = do_div(quotient, bond->params.miimon);
And here.
WBR, Sergei
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-01-24 18:27 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-01-23 18:47 [PATCH net-next] bonding: Use do_div to divide 64 bit numbers Zoltan Kiss
2014-01-23 20:44 ` Nikolay Aleksandrov
2014-01-23 21:18 ` Nikolay Aleksandrov
2014-01-23 22:43 ` [PATCH net-next] bonding: fix u64 division Nikolay Aleksandrov
2014-01-24 0:12 ` David Miller
2014-01-24 18:27 ` [PATCH net-next] bonding: Use do_div to divide 64 bit numbers Zoltan Kiss
2014-01-23 22:25 ` Sergei Shtylyov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).