| Message ID | 20220319063800.28791-1-xiam0nd.tong@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | cw1200: remove an unneeded NULL check on list iterator | expand |
> On 19. Mar 2022, at 07:38, Xiaomeng Tong <xiam0nd.tong@gmail.com> wrote: > > The list iterator 'item' is always non-NULL so it doesn't need to be > checked. Thus just remove the unnecessary NULL check. Also remove the > unnecessary initializer because the list iterator is always initialized. > > Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com> > --- > drivers/net/wireless/st/cw1200/queue.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/wireless/st/cw1200/queue.c b/drivers/net/wireless/st/cw1200/queue.c > index 12952b1c29df..05392598e273 100644 > --- a/drivers/net/wireless/st/cw1200/queue.c > +++ b/drivers/net/wireless/st/cw1200/queue.c > @@ -90,7 +90,7 @@ static void __cw1200_queue_gc(struct cw1200_queue *queue, > bool unlock) > { > struct cw1200_queue_stats *stats = queue->stats; > - struct cw1200_queue_item *item = NULL, *tmp; > + struct cw1200_queue_item *item, *tmp; > bool wakeup_stats = false; > > list_for_each_entry_safe(item, tmp, &queue->queue, head) { > @@ -117,7 +117,7 @@ static void __cw1200_queue_gc(struct cw1200_queue *queue, > queue->overfull = false; > if (unlock) > __cw1200_queue_unlock(queue); > - } else if (item) { > + } else { I don't think this is fixing anything here. You are basically just removing a check that was always true. I'm pretty sure that this check is here to check if either the list is empty or no element was found. If I'm not wrong, some time ago, lists where not circular but actually pointed to NULL (or the head was NULL) so this check made sense but doesn't anymore. The appropriate fix would be only setting 'item' when a break is hit and keep the original check. > unsigned long tmo = item->queue_timestamp + queue->ttl; > mod_timer(&queue->gc, tmo); > cw1200_pm_stay_awake(&stats->priv->pm_state, > -- > 2.17.1 > > I've made those changes already and I'm in the process of upstreaming them in an organized way, so maybe it would make sense to synchronize, so we don't post duplicate patches. Jakob
On Sun, 20 Mar 2022 01:47:26 +0100, Jakob Koschel <jakobkoschel@gmail.com> wrote: > I don't think this is fixing anything here. You are basically just removing > a check that was always true. Yes. > > I'm pretty sure that this check is here to check if either the list is empty or no > element was found. If I'm not wrong, some time ago, lists where not circular but > actually pointed to NULL (or the head was NULL) so this check made sense but doesn't > anymore. > > The appropriate fix would be only setting 'item' when a break is hit and keep > the original check. You are right if that is the author's original intention. I will fix it in PATCH v2. > > > unsigned long tmo = item->queue_timestamp + queue->ttl; > > mod_timer(&queue->gc, tmo); > > cw1200_pm_stay_awake(&stats->priv->pm_state, > > -- > > 2.17.1 > > > > > > I've made those changes already and I'm in the process of upstreaming them in an organized > way, so maybe it would make sense to synchronize, so we don't post duplicate patches. Ok, I will cc you when sending related patches to avoid duplication. I hope you can do the same, thank you. Here are the 9 patches I have sent, so you don't have to reinvent th wheel: https://lore.kernel.org/all/20220319102222.3079-1-xiam0nd.tong@gmail.com/ https://lore.kernel.org/all/20220319073143.30184-1-xiam0nd.tong@gmail.com/ https://lore.kernel.org/all/20220319063800.28791-1-xiam0nd.tong@gmail.com/ https://lore.kernel.org/all/20220319053742.27443-1-xiam0nd.tong@gmail.com/ https://lore.kernel.org/all/20220319052350.26535-1-xiam0nd.tong@gmail.com/ https://lore.kernel.org/all/20220319044416.24242-1-xiam0nd.tong@gmail.com/ https://lore.kernel.org/all/20220319043606.23292-1-xiam0nd.tong@gmail.com/ https://lore.kernel.org/all/20220319042657.21835-1-xiam0nd.tong@gmail.com/ https://lore.kernel.org/all/20220316075153.3708-1-xiam0nd.tong@gmail.com/ -- Xiaomeng Tong
diff --git a/drivers/net/wireless/st/cw1200/queue.c b/drivers/net/wireless/st/cw1200/queue.c index 12952b1c29df..05392598e273 100644 --- a/drivers/net/wireless/st/cw1200/queue.c +++ b/drivers/net/wireless/st/cw1200/queue.c @@ -90,7 +90,7 @@ static void __cw1200_queue_gc(struct cw1200_queue *queue, bool unlock) { struct cw1200_queue_stats *stats = queue->stats; - struct cw1200_queue_item *item = NULL, *tmp; + struct cw1200_queue_item *item, *tmp; bool wakeup_stats = false; list_for_each_entry_safe(item, tmp, &queue->queue, head) { @@ -117,7 +117,7 @@ static void __cw1200_queue_gc(struct cw1200_queue *queue, queue->overfull = false; if (unlock) __cw1200_queue_unlock(queue); - } else if (item) { + } else { unsigned long tmo = item->queue_timestamp + queue->ttl; mod_timer(&queue->gc, tmo); cw1200_pm_stay_awake(&stats->priv->pm_state,
The list iterator 'item' is always non-NULL so it doesn't need to be checked. Thus just remove the unnecessary NULL check. Also remove the unnecessary initializer because the list iterator is always initialized. Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com> --- drivers/net/wireless/st/cw1200/queue.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)