| Message ID | 1534229416-13254-21-git-send-email-ajay.kathat@microchip.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | staging: wilc1000: avoid use of static and global variable | expand |
On 14.08.2018 09:50, Ajay Singh wrote: > Cleanup patch to avoid line over 80 chars issue reported by > checkpatch.pl script. > > Signed-off-by: Ajay Singh <ajay.kathat@microchip.com> > --- > drivers/staging/wilc1000/wilc_wlan.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/staging/wilc1000/wilc_wlan.c b/drivers/staging/wilc1000/wilc_wlan.c > index 041c9dd..f0743d9 100644 > --- a/drivers/staging/wilc1000/wilc_wlan.c > +++ b/drivers/staging/wilc1000/wilc_wlan.c > @@ -137,6 +137,11 @@ static inline int add_tcp_pending_ack(struct wilc_vif *vif, u32 ack, > return 0; > } > > +static inline void clear_tcp_session_txq(struct wilc_vif *vif, int index) > +{ > + vif->ack_filter.pending_acks_info[index].txqe = NULL; > +} > + This seems useless to me... > static inline void tcp_process(struct net_device *dev, struct txq_entry_t *tqe) > { > void *buffer = tqe->buffer; > @@ -670,7 +675,7 @@ int wilc_wlan_handle_txq(struct net_device *dev, u32 *txq_count) > tqe->tx_complete_func(tqe->priv, tqe->status); > if (tqe->tcp_pending_ack_idx != NOT_TCP_ACK && > tqe->tcp_pending_ack_idx < MAX_PENDING_ACKS) > - vif->ack_filter.pending_acks_info[tqe->tcp_pending_ack_idx].txqe = NULL; > + clear_tcp_session_txq(vif, tqe->tcp_pending_ack_idx); > kfree(tqe); > } while (--entries); > >
On Thu, 23 Aug 2018 11:12:08 +0300 Claudiu Beznea <Claudiu.Beznea@microchip.com> wrote: > On 14.08.2018 09:50, Ajay Singh wrote: > > Cleanup patch to avoid line over 80 chars issue reported by > > checkpatch.pl script. > > > > Signed-off-by: Ajay Singh <ajay.kathat@microchip.com> > > --- > > drivers/staging/wilc1000/wilc_wlan.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/staging/wilc1000/wilc_wlan.c > > b/drivers/staging/wilc1000/wilc_wlan.c index 041c9dd..f0743d9 100644 > > --- a/drivers/staging/wilc1000/wilc_wlan.c > > +++ b/drivers/staging/wilc1000/wilc_wlan.c > > @@ -137,6 +137,11 @@ static inline int add_tcp_pending_ack(struct > > wilc_vif *vif, u32 ack, return 0; > > } > > > > +static inline void clear_tcp_session_txq(struct wilc_vif *vif, int > > index) +{ > > + vif->ack_filter.pending_acks_info[index].txqe = NULL; > > +} > > + > > This seems useless to me... Sorry, this point is not fully clear to me. Did you mean setting of 'NULL' to 'pending_acks_info[index].txqe' is not required? > > > static inline void tcp_process(struct net_device *dev, struct > > txq_entry_t *tqe) { > > void *buffer = tqe->buffer; > > @@ -670,7 +675,7 @@ int wilc_wlan_handle_txq(struct net_device > > *dev, u32 *txq_count) tqe->tx_complete_func(tqe->priv, tqe->status); > > if (tqe->tcp_pending_ack_idx != NOT_TCP_ACK && > > tqe->tcp_pending_ack_idx < MAX_PENDING_ACKS) > > - > > vif->ack_filter.pending_acks_info[tqe->tcp_pending_ack_idx].txqe = > > NULL; > > + clear_tcp_session_txq(vif, > > tqe->tcp_pending_ack_idx); kfree(tqe); > > } while (--entries); > > > >
On 23.08.2018 13:33, Ajay Singh wrote: > On Thu, 23 Aug 2018 11:12:08 +0300 > Claudiu Beznea <Claudiu.Beznea@microchip.com> wrote: > >> On 14.08.2018 09:50, Ajay Singh wrote: >>> Cleanup patch to avoid line over 80 chars issue reported by >>> checkpatch.pl script. >>> >>> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com> >>> --- >>> drivers/staging/wilc1000/wilc_wlan.c | 7 ++++++- >>> 1 file changed, 6 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/staging/wilc1000/wilc_wlan.c >>> b/drivers/staging/wilc1000/wilc_wlan.c index 041c9dd..f0743d9 100644 >>> --- a/drivers/staging/wilc1000/wilc_wlan.c >>> +++ b/drivers/staging/wilc1000/wilc_wlan.c >>> @@ -137,6 +137,11 @@ static inline int add_tcp_pending_ack(struct >>> wilc_vif *vif, u32 ack, return 0; >>> } >>> >>> +static inline void clear_tcp_session_txq(struct wilc_vif *vif, int >>> index) +{ >>> + vif->ack_filter.pending_acks_info[index].txqe = NULL; >>> +} >>> + >> >> This seems useless to me... > > Sorry, this point is not fully clear to me. > > Did you mean setting of 'NULL' to 'pending_acks_info[index].txqe' is > not required? > No, having a new function that sets a variable just to avoid line over 80 warning. > >> >>> static inline void tcp_process(struct net_device *dev, struct >>> txq_entry_t *tqe) { >>> void *buffer = tqe->buffer; >>> @@ -670,7 +675,7 @@ int wilc_wlan_handle_txq(struct net_device >>> *dev, u32 *txq_count) tqe->tx_complete_func(tqe->priv, tqe->status); >>> if (tqe->tcp_pending_ack_idx != NOT_TCP_ACK && >>> tqe->tcp_pending_ack_idx < MAX_PENDING_ACKS) >>> - >>> vif->ack_filter.pending_acks_info[tqe->tcp_pending_ack_idx].txqe = >>> NULL; >>> + clear_tcp_session_txq(vif, >>> tqe->tcp_pending_ack_idx); kfree(tqe); >>> } while (--entries); >>> >>> > >
Hi Claudiu, On Fri, 24 Aug 2018 12:31:28 +0300 Claudiu Beznea <Claudiu.Beznea@microchip.com> wrote: > On 23.08.2018 13:33, Ajay Singh wrote: > > On Thu, 23 Aug 2018 11:12:08 +0300 > > Claudiu Beznea <Claudiu.Beznea@microchip.com> wrote: > > > >> On 14.08.2018 09:50, Ajay Singh wrote: > >>> Cleanup patch to avoid line over 80 chars issue reported by > >>> checkpatch.pl script. > >>> > >>> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com> > >>> --- > >>> drivers/staging/wilc1000/wilc_wlan.c | 7 ++++++- > >>> 1 file changed, 6 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/staging/wilc1000/wilc_wlan.c > >>> b/drivers/staging/wilc1000/wilc_wlan.c index 041c9dd..f0743d9 > >>> 100644 --- a/drivers/staging/wilc1000/wilc_wlan.c > >>> +++ b/drivers/staging/wilc1000/wilc_wlan.c > >>> @@ -137,6 +137,11 @@ static inline int add_tcp_pending_ack(struct > >>> wilc_vif *vif, u32 ack, return 0; > >>> } > >>> > >>> +static inline void clear_tcp_session_txq(struct wilc_vif *vif, > >>> int index) +{ > >>> + vif->ack_filter.pending_acks_info[index].txqe = NULL; > >>> +} > >>> + > >> > >> This seems useless to me... > > > > Sorry, this point is not fully clear to me. > > > > Did you mean setting of 'NULL' to 'pending_acks_info[index].txqe' is > > not required? > > > > No, having a new function that sets a variable just to avoid line > over 80 warning. Okay got it. How about using a temp variable to hold the value of 'tqe->tcp_pending_ack_idx'. It can also help to overcome the checkpatch warning. Regards, Ajay
On Mon, Aug 27, 2018 at 10:54:38AM +0530, Ajay Singh wrote: > Hi Claudiu, > > On Fri, 24 Aug 2018 12:31:28 +0300 > Claudiu Beznea <Claudiu.Beznea@microchip.com> wrote: > > > On 23.08.2018 13:33, Ajay Singh wrote: > > > On Thu, 23 Aug 2018 11:12:08 +0300 > > > Claudiu Beznea <Claudiu.Beznea@microchip.com> wrote: > > > > > >> On 14.08.2018 09:50, Ajay Singh wrote: > > >>> Cleanup patch to avoid line over 80 chars issue reported by > > >>> checkpatch.pl script. > > >>> > > >>> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com> > > >>> --- > > >>> drivers/staging/wilc1000/wilc_wlan.c | 7 ++++++- > > >>> 1 file changed, 6 insertions(+), 1 deletion(-) > > >>> > > >>> diff --git a/drivers/staging/wilc1000/wilc_wlan.c > > >>> b/drivers/staging/wilc1000/wilc_wlan.c index 041c9dd..f0743d9 > > >>> 100644 --- a/drivers/staging/wilc1000/wilc_wlan.c > > >>> +++ b/drivers/staging/wilc1000/wilc_wlan.c > > >>> @@ -137,6 +137,11 @@ static inline int add_tcp_pending_ack(struct > > >>> wilc_vif *vif, u32 ack, return 0; > > >>> } > > >>> > > >>> +static inline void clear_tcp_session_txq(struct wilc_vif *vif, > > >>> int index) +{ > > >>> + vif->ack_filter.pending_acks_info[index].txqe = NULL; > > >>> +} > > >>> + > > >> > > >> This seems useless to me... > > > > > > Sorry, this point is not fully clear to me. > > > > > > Did you mean setting of 'NULL' to 'pending_acks_info[index].txqe' is > > > not required? > > > > > > > No, having a new function that sets a variable just to avoid line > > over 80 warning. > > Okay got it. > How about using a temp variable to hold the value of > 'tqe->tcp_pending_ack_idx'. It can also help to overcome the > checkpatch warning. Just ignore the checkpatch warning... Don't add a temporary variable just to please checkpatch... It's good to fix checkpatch warnings if it makes the code cleaner, but I hate when people do: tmp = some_long_variable_name; some_other_long_variable = tmp; The tmp variable is only used one time and a simple statement becomes two statements and it breaks grep. You could consider renaming some of the variables. I think the "_info" doesn't add anything, because obviously it's information. Maybe "tcp_pending_ack_index" could just become "->ack_idx". vif->ack_filter.pending_ack[tqe->ack_idx].txqe = NULL; It's still very searchable. If you grep for "ack_idx" then the pending and TCP are right there so no information is lost. regards, dan carpenter
Hi Dan, On Mon, 27 Aug 2018 15:00:50 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote: > On Mon, Aug 27, 2018 at 10:54:38AM +0530, Ajay Singh wrote: > > Hi Claudiu, > > > > On Fri, 24 Aug 2018 12:31:28 +0300 > > Claudiu Beznea <Claudiu.Beznea@microchip.com> wrote: > > > > > On 23.08.2018 13:33, Ajay Singh wrote: > > > > On Thu, 23 Aug 2018 11:12:08 +0300 > > > > Claudiu Beznea <Claudiu.Beznea@microchip.com> wrote: > > > > > > > >> On 14.08.2018 09:50, Ajay Singh wrote: > > > >>> Cleanup patch to avoid line over 80 chars issue reported by > > > >>> checkpatch.pl script. > > > >>> > > > >>> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com> > > > >>> --- > > > >>> drivers/staging/wilc1000/wilc_wlan.c | 7 ++++++- > > > >>> 1 file changed, 6 insertions(+), 1 deletion(-) > > > >>> > > > >>> diff --git a/drivers/staging/wilc1000/wilc_wlan.c > > > >>> b/drivers/staging/wilc1000/wilc_wlan.c index 041c9dd..f0743d9 > > > >>> 100644 --- a/drivers/staging/wilc1000/wilc_wlan.c > > > >>> +++ b/drivers/staging/wilc1000/wilc_wlan.c > > > >>> @@ -137,6 +137,11 @@ static inline int > > > >>> add_tcp_pending_ack(struct wilc_vif *vif, u32 ack, return 0; > > > >>> } > > > >>> > > > >>> +static inline void clear_tcp_session_txq(struct wilc_vif > > > >>> *vif, int index) +{ > > > >>> + vif->ack_filter.pending_acks_info[index].txqe = NULL; > > > >>> +} > > > >>> + > > > >> > > > >> This seems useless to me... > > > > > > > > Sorry, this point is not fully clear to me. > > > > > > > > Did you mean setting of 'NULL' to > > > > 'pending_acks_info[index].txqe' is not required? > > > > > > > > > > No, having a new function that sets a variable just to avoid line > > > over 80 warning. > > > > Okay got it. > > How about using a temp variable to hold the value of > > 'tqe->tcp_pending_ack_idx'. It can also help to overcome the > > checkpatch warning. > > Just ignore the checkpatch warning... Don't add a temporary variable > just to please checkpatch... It's good to fix checkpatch warnings if > it makes the code cleaner, but I hate when people do: > > tmp = some_long_variable_name; > some_other_long_variable = tmp; > > The tmp variable is only used one time and a simple statement becomes > two statements and it breaks grep. > > You could consider renaming some of the variables. I think the > "_info" doesn't add anything, because obviously it's information. > Maybe "tcp_pending_ack_index" could just become "->ack_idx". Thanks for providing the inputs. I will include the changes by using shorter name for 'tcp_pending_ack_index' in next version. Regards, Ajay
diff --git a/drivers/staging/wilc1000/wilc_wlan.c b/drivers/staging/wilc1000/wilc_wlan.c index 041c9dd..f0743d9 100644 --- a/drivers/staging/wilc1000/wilc_wlan.c +++ b/drivers/staging/wilc1000/wilc_wlan.c @@ -137,6 +137,11 @@ static inline int add_tcp_pending_ack(struct wilc_vif *vif, u32 ack, return 0; } +static inline void clear_tcp_session_txq(struct wilc_vif *vif, int index) +{ + vif->ack_filter.pending_acks_info[index].txqe = NULL; +} + static inline void tcp_process(struct net_device *dev, struct txq_entry_t *tqe) { void *buffer = tqe->buffer; @@ -670,7 +675,7 @@ int wilc_wlan_handle_txq(struct net_device *dev, u32 *txq_count) tqe->tx_complete_func(tqe->priv, tqe->status); if (tqe->tcp_pending_ack_idx != NOT_TCP_ACK && tqe->tcp_pending_ack_idx < MAX_PENDING_ACKS) - vif->ack_filter.pending_acks_info[tqe->tcp_pending_ack_idx].txqe = NULL; + clear_tcp_session_txq(vif, tqe->tcp_pending_ack_idx); kfree(tqe); } while (--entries);
Cleanup patch to avoid line over 80 chars issue reported by checkpatch.pl script. Signed-off-by: Ajay Singh <ajay.kathat@microchip.com> --- drivers/staging/wilc1000/wilc_wlan.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)