* [net-next PATCH] amt: Use BIT macros instead of open codes
@ 2022-04-30 13:56 Juhee Kang
2022-05-01 9:50 ` Taehee Yoo
2022-05-02 10:11 ` Paolo Abeni
0 siblings, 2 replies; 6+ messages in thread
From: Juhee Kang @ 2022-04-30 13:56 UTC (permalink / raw)
To: ap420073, davem, kuba, pabeni, netdev
Replace open code related to bit operation with BIT macros, which kernel
provided. This patch provides no functional change.
Signed-off-by: Juhee Kang <claudiajkang@gmail.com>
---
drivers/net/amt.c | 2 +-
include/net/amt.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/amt.c b/drivers/net/amt.c
index 10455c9b9da0..76c1969a03f5 100644
--- a/drivers/net/amt.c
+++ b/drivers/net/amt.c
@@ -959,7 +959,7 @@ static void amt_req_work(struct work_struct *work)
amt_update_gw_status(amt, AMT_STATUS_SENT_REQUEST, true);
spin_lock_bh(&amt->lock);
out:
- exp = min_t(u32, (1 * (1 << amt->req_cnt)), AMT_MAX_REQ_TIMEOUT);
+ exp = min_t(u32, (1 * BIT(amt->req_cnt)), AMT_MAX_REQ_TIMEOUT);
mod_delayed_work(amt_wq, &amt->req_wq, msecs_to_jiffies(exp * 1000));
spin_unlock_bh(&amt->lock);
}
diff --git a/include/net/amt.h b/include/net/amt.h
index 7a4db8b903ee..d2fd76b0a424 100644
--- a/include/net/amt.h
+++ b/include/net/amt.h
@@ -354,7 +354,7 @@ struct amt_dev {
#define AMT_MAX_GROUP 32
#define AMT_MAX_SOURCE 128
#define AMT_HSIZE_SHIFT 8
-#define AMT_HSIZE (1 << AMT_HSIZE_SHIFT)
+#define AMT_HSIZE BIT(AMT_HSIZE_SHIFT)
#define AMT_DISCOVERY_TIMEOUT 5000
#define AMT_INIT_REQ_TIMEOUT 1
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [net-next PATCH] amt: Use BIT macros instead of open codes
2022-04-30 13:56 [net-next PATCH] amt: Use BIT macros instead of open codes Juhee Kang
@ 2022-05-01 9:50 ` Taehee Yoo
2022-05-02 10:11 ` Paolo Abeni
1 sibling, 0 replies; 6+ messages in thread
From: Taehee Yoo @ 2022-05-01 9:50 UTC (permalink / raw)
To: Juhee Kang, davem, kuba, pabeni, netdev
On 4/30/22 22:56, Juhee Kang wrote:
Hi Juhee,
Thanks a lot for this work!
> Replace open code related to bit operation with BIT macros, which kernel
> provided. This patch provides no functional change.
>
Reviewed-by: Taehee Yoo <ap420073@gmail.com>
Tested-by: Taehee Yoo <ap420073@gmail.com>
Thanks,
Taehee Yoo
> Signed-off-by: Juhee Kang <claudiajkang@gmail.com>
> ---
> drivers/net/amt.c | 2 +-
> include/net/amt.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index 10455c9b9da0..76c1969a03f5 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c
> @@ -959,7 +959,7 @@ static void amt_req_work(struct work_struct *work)
> amt_update_gw_status(amt, AMT_STATUS_SENT_REQUEST, true);
> spin_lock_bh(&amt->lock);
> out:
> - exp = min_t(u32, (1 * (1 << amt->req_cnt)), AMT_MAX_REQ_TIMEOUT);
> + exp = min_t(u32, (1 * BIT(amt->req_cnt)), AMT_MAX_REQ_TIMEOUT);
> mod_delayed_work(amt_wq, &amt->req_wq, msecs_to_jiffies(exp * 1000));
> spin_unlock_bh(&amt->lock);
> }
> diff --git a/include/net/amt.h b/include/net/amt.h
> index 7a4db8b903ee..d2fd76b0a424 100644
> --- a/include/net/amt.h
> +++ b/include/net/amt.h
> @@ -354,7 +354,7 @@ struct amt_dev {
> #define AMT_MAX_GROUP 32
> #define AMT_MAX_SOURCE 128
> #define AMT_HSIZE_SHIFT 8
> -#define AMT_HSIZE (1 << AMT_HSIZE_SHIFT)
> +#define AMT_HSIZE BIT(AMT_HSIZE_SHIFT)
>
> #define AMT_DISCOVERY_TIMEOUT 5000
> #define AMT_INIT_REQ_TIMEOUT 1
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [net-next PATCH] amt: Use BIT macros instead of open codes
2022-04-30 13:56 [net-next PATCH] amt: Use BIT macros instead of open codes Juhee Kang
2022-05-01 9:50 ` Taehee Yoo
@ 2022-05-02 10:11 ` Paolo Abeni
2022-05-02 17:19 ` Joe Perches
1 sibling, 1 reply; 6+ messages in thread
From: Paolo Abeni @ 2022-05-02 10:11 UTC (permalink / raw)
To: Juhee Kang, ap420073, davem, kuba, netdev
Hello,
On Sat, 2022-04-30 at 13:56 +0000, Juhee Kang wrote:
> Replace open code related to bit operation with BIT macros, which kernel
> provided. This patch provides no functional change.
>
> Signed-off-by: Juhee Kang <claudiajkang@gmail.com>
> ---
> drivers/net/amt.c | 2 +-
> include/net/amt.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index 10455c9b9da0..76c1969a03f5 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c
> @@ -959,7 +959,7 @@ static void amt_req_work(struct work_struct *work)
> amt_update_gw_status(amt, AMT_STATUS_SENT_REQUEST, true);
> spin_lock_bh(&amt->lock);
> out:
> - exp = min_t(u32, (1 * (1 << amt->req_cnt)), AMT_MAX_REQ_TIMEOUT);
> + exp = min_t(u32, (1 * BIT(amt->req_cnt)), AMT_MAX_REQ_TIMEOUT);
> mod_delayed_work(amt_wq, &amt->req_wq, msecs_to_jiffies(exp * 1000));
> spin_unlock_bh(&amt->lock);
> }
> diff --git a/include/net/amt.h b/include/net/amt.h
> index 7a4db8b903ee..d2fd76b0a424 100644
> --- a/include/net/amt.h
> +++ b/include/net/amt.h
> @@ -354,7 +354,7 @@ struct amt_dev {
> #define AMT_MAX_GROUP 32
> #define AMT_MAX_SOURCE 128
> #define AMT_HSIZE_SHIFT 8
> -#define AMT_HSIZE (1 << AMT_HSIZE_SHIFT)
> +#define AMT_HSIZE BIT(AMT_HSIZE_SHIFT)
>
> #define AMT_DISCOVERY_TIMEOUT 5000
> #define AMT_INIT_REQ_TIMEOUT 1
Even if the 2 replaced statements use shift operations, they do not
look like bit manipulation: the first one is an exponential timeout,
the 2nd one is an (hash) size. I think using the BIT() macro here will
be confusing.
Cheers,
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [net-next PATCH] amt: Use BIT macros instead of open codes
2022-05-02 10:11 ` Paolo Abeni
@ 2022-05-02 17:19 ` Joe Perches
2022-05-03 9:28 ` Taehee Yoo
0 siblings, 1 reply; 6+ messages in thread
From: Joe Perches @ 2022-05-02 17:19 UTC (permalink / raw)
To: Paolo Abeni, Juhee Kang, ap420073, davem, kuba, netdev
On Mon, 2022-05-02 at 12:11 +0200, Paolo Abeni wrote:
> On Sat, 2022-04-30 at 13:56 +0000, Juhee Kang wrote:
> > Replace open code related to bit operation with BIT macros, which kernel
> > provided. This patch provides no functional change.
[]
> > diff --git a/drivers/net/amt.c b/drivers/net/amt.c
[]
> > @@ -959,7 +959,7 @@ static void amt_req_work(struct work_struct *work)
> > amt_update_gw_status(amt, AMT_STATUS_SENT_REQUEST, true);
> > spin_lock_bh(&amt->lock);
> > out:
> > - exp = min_t(u32, (1 * (1 << amt->req_cnt)), AMT_MAX_REQ_TIMEOUT);
> > + exp = min_t(u32, (1 * BIT(amt->req_cnt)), AMT_MAX_REQ_TIMEOUT);
> > mod_delayed_work(amt_wq, &amt->req_wq, msecs_to_jiffies(exp * 1000));
> > spin_unlock_bh(&amt->lock);
> > }
> > diff --git a/include/net/amt.h b/include/net/amt.h
[]
> > @@ -354,7 +354,7 @@ struct amt_dev {
> > #define AMT_MAX_GROUP 32
> > #define AMT_MAX_SOURCE 128
> > #define AMT_HSIZE_SHIFT 8
> > -#define AMT_HSIZE (1 << AMT_HSIZE_SHIFT)
> > +#define AMT_HSIZE BIT(AMT_HSIZE_SHIFT)
> >
> > #define AMT_DISCOVERY_TIMEOUT 5000
> > #define AMT_INIT_REQ_TIMEOUT 1
>
> Even if the 2 replaced statements use shift operations, they do not
> look like bit manipulation: the first one is an exponential timeout,
> the 2nd one is an (hash) size. I think using the BIT() macro here will
> be confusing.
I agree.
I also believe one of the uses of amt->req_cnt is error prone.
drivers/net/amt.c:946: if (amt->req_cnt++ > AMT_MAX_REQ_COUNT) {
Combining a test and post increment is not a great style IMO.
Is this really the intended behavior?
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [net-next PATCH] amt: Use BIT macros instead of open codes
2022-05-02 17:19 ` Joe Perches
@ 2022-05-03 9:28 ` Taehee Yoo
2022-05-03 11:33 ` Juhee Kang
0 siblings, 1 reply; 6+ messages in thread
From: Taehee Yoo @ 2022-05-03 9:28 UTC (permalink / raw)
To: Joe Perches, Paolo Abeni, Juhee Kang, davem, kuba, netdev
2022. 5. 3. 오전 2:19에 Joe Perches 이(가) 쓴 글:
> On Mon, 2022-05-02 at 12:11 +0200, Paolo Abeni wrote:
Hi Paolo and Joe,
Thanks a lot for the reviews!
>> On Sat, 2022-04-30 at 13:56 +0000, Juhee Kang wrote:
>>> Replace open code related to bit operation with BIT macros, which
kernel
>>> provided. This patch provides no functional change.
> []
>>> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> []
>>> @@ -959,7 +959,7 @@ static void amt_req_work(struct work_struct *work)
>>> amt_update_gw_status(amt, AMT_STATUS_SENT_REQUEST, true);
>>> spin_lock_bh(&amt->lock);
>>> out:
>>> - exp = min_t(u32, (1 * (1 << amt->req_cnt)), AMT_MAX_REQ_TIMEOUT);
>>> + exp = min_t(u32, (1 * BIT(amt->req_cnt)), AMT_MAX_REQ_TIMEOUT);
>>> mod_delayed_work(amt_wq, &amt->req_wq, msecs_to_jiffies(exp *
1000));
>>> spin_unlock_bh(&amt->lock);
>>> }
>>> diff --git a/include/net/amt.h b/include/net/amt.h
> []
>>> @@ -354,7 +354,7 @@ struct amt_dev {
>>> #define AMT_MAX_GROUP 32
>>> #define AMT_MAX_SOURCE 128
>>> #define AMT_HSIZE_SHIFT 8
>>> -#define AMT_HSIZE (1 << AMT_HSIZE_SHIFT)
>>> +#define AMT_HSIZE BIT(AMT_HSIZE_SHIFT)
>>>
>>> #define AMT_DISCOVERY_TIMEOUT 5000
>>> #define AMT_INIT_REQ_TIMEOUT 1
>>
>> Even if the 2 replaced statements use shift operations, they do not
>> look like bit manipulation: the first one is an exponential timeout,
>> the 2nd one is an (hash) size. I think using the BIT() macro here will
>> be confusing.
>
> I agree.
>
> I also believe one of the uses of amt->req_cnt is error prone.
>
> drivers/net/amt.c:946: if (amt->req_cnt++ > AMT_MAX_REQ_COUNT) {
>
> Combining a test and post increment is not a great style IMO.
> Is this really the intended behavior?
I agree that it would be better to avoid that style.
I will send a patch for that after some bugfix.
Thanks a lot,
Taehee Yoo
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [net-next PATCH] amt: Use BIT macros instead of open codes
2022-05-03 9:28 ` Taehee Yoo
@ 2022-05-03 11:33 ` Juhee Kang
0 siblings, 0 replies; 6+ messages in thread
From: Juhee Kang @ 2022-05-03 11:33 UTC (permalink / raw)
To: Taehee Yoo; +Cc: Joe Perches, Paolo Abeni, davem, Jakub Kicinski, Networking
Hello Paolo and Joe,
Thanks for the reviews!
On Tue, May 3, 2022 at 6:28 PM Taehee Yoo <ap420073@gmail.com> wrote:
>
> 2022. 5. 3. 오전 2:19에 Joe Perches 이(가) 쓴 글:
> > On Mon, 2022-05-02 at 12:11 +0200, Paolo Abeni wrote:
>
> Hi Paolo and Joe,
> Thanks a lot for the reviews!
>
> >> On Sat, 2022-04-30 at 13:56 +0000, Juhee Kang wrote:
> >>> Replace open code related to bit operation with BIT macros, which
> kernel
> >>> provided. This patch provides no functional change.
> > []
> >>> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> > []
> >>> @@ -959,7 +959,7 @@ static void amt_req_work(struct work_struct *work)
> >>> amt_update_gw_status(amt, AMT_STATUS_SENT_REQUEST, true);
> >>> spin_lock_bh(&amt->lock);
> >>> out:
> >>> - exp = min_t(u32, (1 * (1 << amt->req_cnt)), AMT_MAX_REQ_TIMEOUT);
> >>> + exp = min_t(u32, (1 * BIT(amt->req_cnt)), AMT_MAX_REQ_TIMEOUT);
> >>> mod_delayed_work(amt_wq, &amt->req_wq, msecs_to_jiffies(exp *
> 1000));
> >>> spin_unlock_bh(&amt->lock);
> >>> }
> >>> diff --git a/include/net/amt.h b/include/net/amt.h
> > []
> >>> @@ -354,7 +354,7 @@ struct amt_dev {
> >>> #define AMT_MAX_GROUP 32
> >>> #define AMT_MAX_SOURCE 128
> >>> #define AMT_HSIZE_SHIFT 8
> >>> -#define AMT_HSIZE (1 << AMT_HSIZE_SHIFT)
> >>> +#define AMT_HSIZE BIT(AMT_HSIZE_SHIFT)
> >>>
> >>> #define AMT_DISCOVERY_TIMEOUT 5000
> >>> #define AMT_INIT_REQ_TIMEOUT 1
> >>
> >> Even if the 2 replaced statements use shift operations, they do not
> >> look like bit manipulation: the first one is an exponential timeout,
> >> the 2nd one is an (hash) size. I think using the BIT() macro here will
> >> be confusing.
> >
> > I agree.
> >
> > I also believe one of the uses of amt->req_cnt is error prone.
> >
> > drivers/net/amt.c:946: if (amt->req_cnt++ > AMT_MAX_REQ_COUNT) {
> >
> > Combining a test and post increment is not a great style IMO.
> > Is this really the intended behavior?
>
> I agree that it would be better to avoid that style.
> I will send a patch for that after some bugfix.
>
> Thanks a lot,
> Taehee Yoo
>
> >
> >
--
Best regards,
Juhee Kang
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-05-03 11:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-30 13:56 [net-next PATCH] amt: Use BIT macros instead of open codes Juhee Kang
2022-05-01 9:50 ` Taehee Yoo
2022-05-02 10:11 ` Paolo Abeni
2022-05-02 17:19 ` Joe Perches
2022-05-03 9:28 ` Taehee Yoo
2022-05-03 11:33 ` Juhee Kang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.