linux-kernel - Re: [PATCH 2/3] staging: greybus: loopback.c: do insertion in O(n) instead of O(n lg n)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2f90c2c7-6419-c4d1-0688-7cb84963281c@nexus-software.ie>
Date:   Tue, 23 Oct 2018 23:30:01 +0100
From:   Bryan O'Donoghue <pure.logic@...us-software.ie>
To:     Rasmus Villemoes <linux@...musvillemoes.dk>,
        Johan Hovold <johan@...nel.org>, Alex Elder <elder@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     greybus-dev@...ts.linaro.org, devel@...verdev.osuosl.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] staging: greybus: loopback.c: do insertion in O(n)
 instead of O(n lg n)

On 23/10/2018 17:20, Rasmus Villemoes wrote:
> On 2018-10-11 01:03, Bryan O'Donoghue wrote:
>> On 05/10/2018 15:28, Rasmus Villemoes wrote:
>>> Signed-off-by: Rasmus Villemoes<linux@...musvillemoes.dk>
>>> ---
>>> I have no idea if the performance matters (it probably doesn't). Feel
>>> free to ignore this and the followup cleanup.
>>
>> What's the problem you're fixing here ?
>>
>> Is it tested ?
> 
> I got curious why one would want to keep a linked list sorted in the
> first place (it can't be for doing binary searches). But it seems that
> gb_loopback_device::list is unused, along with the list_op_async. Given
> that the below compiles, doesn't that prove that the code is
> dead/unused, or what am I missing?
> 
> diff --git a/drivers/staging/greybus/loopback.c
> b/drivers/staging/greybus/loopback.c
> index 7080294f705c..e4d42c1dc284 100644
> --- a/drivers/staging/greybus/loopback.c
> +++ b/drivers/staging/greybus/loopback.c
> @@ -47,8 +47,6 @@ struct gb_loopback_device {
> 
>   	/* We need to take a lock in atomic context */
>   	spinlock_t lock;
> -	struct list_head list;
> -	struct list_head list_op_async;
>   	wait_queue_head_t wq;
>   };
> 
> @@ -68,7 +66,6 @@ struct gb_loopback {
>   	struct kfifo kfifo_lat;
>   	struct mutex mutex;
>   	struct task_struct *task;
> -	struct list_head entry;
>   	struct device *dev;
>   	wait_queue_head_t wq;
>   	wait_queue_head_t wq_completion;
> @@ -987,37 +984,6 @@ static const struct file_operations
> gb_loopback_debugfs_latency_ops = {
>   	.release	= single_release,
>   };
> 
> -static int gb_loopback_bus_id_compare(void *priv, struct list_head *lha,
> -				      struct list_head *lhb)
> -{
> -	struct gb_loopback *a = list_entry(lha, struct gb_loopback, entry);
> -	struct gb_loopback *b = list_entry(lhb, struct gb_loopback, entry);
> -	struct gb_connection *ca = a->connection;
> -	struct gb_connection *cb = b->connection;
> -
> -	if (ca->bundle->intf->interface_id < cb->bundle->intf->interface_id)
> -		return -1;
> -	if (cb->bundle->intf->interface_id < ca->bundle->intf->interface_id)
> -		return 1;
> -	if (ca->bundle->id < cb->bundle->id)
> -		return -1;
> -	if (cb->bundle->id < ca->bundle->id)
> -		return 1;
> -	if (ca->intf_cport_id < cb->intf_cport_id)
> -		return -1;
> -	else if (cb->intf_cport_id < ca->intf_cport_id)
> -		return 1;
> -
> -	return 0;
> -}
> -
> -static void gb_loopback_insert_id(struct gb_loopback *gb)
> -{
> -	/* perform an insertion sort */
> -	list_add_tail(&gb->entry, &gb_dev.list);
> -	list_sort(NULL, &gb_dev.list, gb_loopback_bus_id_compare);
> -}
> -
>   #define DEBUGFS_NAMELEN 32
> 
>   static int gb_loopback_probe(struct gb_bundle *bundle,
> @@ -1113,7 +1079,6 @@ static int gb_loopback_probe(struct gb_bundle *bundle,
>   	}
> 
>   	spin_lock_irqsave(&gb_dev.lock, flags);
> -	gb_loopback_insert_id(gb);
>   	gb_dev.count++;
>   	spin_unlock_irqrestore(&gb_dev.lock, flags);
> 
> @@ -1169,7 +1134,6 @@ static void gb_loopback_disconnect(struct
> gb_bundle *bundle)
> 
>   	spin_lock_irqsave(&gb_dev.lock, flags);
>   	gb_dev.count--;
> -	list_del(&gb->entry);
>   	spin_unlock_irqrestore(&gb_dev.lock, flags);
> 
>   	device_unregister(gb->dev);
> @@ -1196,8 +1160,6 @@ static int loopback_init(void)
>   {
>   	int retval;
> 
> -	INIT_LIST_HEAD(&gb_dev.list);
> -	INIT_LIST_HEAD(&gb_dev.list_op_async);
>   	spin_lock_init(&gb_dev.lock);
>   	gb_dev.root = debugfs_create_dir("gb_loopback", NULL);
> 

Looks like dead code alright.

Reviewed-by: Bryan O'Donoghue <pure.logic@...us-software.ie>