Re: [PATCH net-next] bonding: Use do_div to divide 64 bit numbers

[Nikolay Aleksandrov <nikolay@redhat.com>]

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);
>