[PATCH] Drivers: hv: vmbus: One function call less in create_gpadl_header() after error detection

[PATCH] Drivers: hv: vmbus: One function call less in create_gpadl_header() after error detection

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 26 Dec 2023 20:00:24 +0100

The kfree() function was called in two cases by
the create_gpadl_header() function during error handling
even if the passed variable contained a null pointer.
This issue was detected by using the Coccinelle software.

Thus use another label.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/hv/channel.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 56f7e06c673e..4d1bbda895d8 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -336,7 +336,7 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
 			  sizeof(struct gpa_range) + pfncount * sizeof(u64);
 		msgheader =  kzalloc(msgsize, GFP_KERNEL);
 		if (!msgheader)
-			goto nomem;
+			goto free_body;

 		INIT_LIST_HEAD(&msgheader->submsglist);
 		msgheader->msgsize = msgsize;
@@ -417,7 +417,7 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
 			  sizeof(struct gpa_range) + pagecount * sizeof(u64);
 		msgheader = kzalloc(msgsize, GFP_KERNEL);
 		if (msgheader == NULL)
-			goto nomem;
+			goto free_body;

 		INIT_LIST_HEAD(&msgheader->submsglist);
 		msgheader->msgsize = msgsize;
@@ -439,6 +439,7 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
 	return 0;
 nomem:
 	kfree(msgheader);
+free_body:
 	kfree(msgbody);
 	return -ENOMEM;
 }
--
2.43.0

RE: [PATCH] Drivers: hv: vmbus: One function call less in create_gpadl_header() after error detection

[Michael Kelley]

From: Markus Elfring <Markus.Elfring@web.de> Sent: Tuesday, December 26, 2023 11:09 AM
> 
> The kfree() function was called in two cases by
> the create_gpadl_header() function during error handling
> even if the passed variable contained a null pointer.
> This issue was detected by using the Coccinelle software.
> 
> Thus use another label.

Interestingly, there's a third case in this function where
"goto nomem" is done, and in this case, msgbody is NULL.
Does Coccinelle not complain about that case as well?

As I'm sure you know, the code is correct as is, because kfree()
checks for a NULL argument.  So this is really an exercise in
making Coccinelle happy.  To me, the additional label is
incremental complexity for someone to deal with when
reading the code at some time in the future.  So I'd vote for
leaving the code as is.  But it's not a big deal either way.  I
can see you've been cleaning up a lot of Coccinelle-reported
issues across the kernel, most of which result in code
simplifications.  If leaving this unchanged causes you problems,
then I won't object (though perhaps that 3rd "goto nomem"
should be dealt with as well for consistency).

Michael

> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/hv/channel.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index 56f7e06c673e..4d1bbda895d8 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -336,7 +336,7 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
>  			  sizeof(struct gpa_range) + pfncount * sizeof(u64);
>  		msgheader =  kzalloc(msgsize, GFP_KERNEL);
>  		if (!msgheader)
> -			goto nomem;
> +			goto free_body;
> 
>  		INIT_LIST_HEAD(&msgheader->submsglist);
>  		msgheader->msgsize = msgsize;
> @@ -417,7 +417,7 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
>  			  sizeof(struct gpa_range) + pagecount * sizeof(u64);
>  		msgheader = kzalloc(msgsize, GFP_KERNEL);
>  		if (msgheader == NULL)
> -			goto nomem;
> +			goto free_body;
> 
>  		INIT_LIST_HEAD(&msgheader->submsglist);
>  		msgheader->msgsize = msgsize;
> @@ -439,6 +439,7 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
>  	return 0;
>  nomem:
>  	kfree(msgheader);
> +free_body:
>  	kfree(msgbody);
>  	return -ENOMEM;
>  }
> --
> 2.43.0
> 

Re: Drivers: hv: vmbus: One function call less in create_gpadl_header() after error detection

[Markus Elfring]

>> The kfree() function was called in two cases by
>> the create_gpadl_header() function during error handling
>> even if the passed variable contained a null pointer.
>> This issue was detected by using the Coccinelle software.
>>
>> Thus use another label.
>
> Interestingly, there's a third case in this function where
> "goto nomem" is done, and in this case, msgbody is NULL.
> Does Coccinelle not complain about that case as well?
>
> As I'm sure you know, the code is correct as is, because kfree()
> checks for a NULL argument.  So this is really an exercise in
> making Coccinelle happy.  To me, the additional label is
> incremental complexity for someone to deal with when
> reading the code at some time in the future.  So I'd vote for
> leaving the code as is.  But it's not a big deal either way.  I
> can see you've been cleaning up a lot of Coccinelle-reported
> issues across the kernel, most of which result in code
> simplifications.  If leaving this unchanged causes you problems,
> then I won't object (though perhaps that 3rd "goto nomem"
> should be dealt with as well for consistency).

How do you think about the clarification approach
“Reconsidering kfree() calls for null pointers (with SmPL)”?
https://lore.kernel.org/cocci/6cbcf640-55e5-2f11-4a09-716fe681c0d2@web.de/
https://sympa.inria.fr/sympa/arc/cocci/2023-03/msg00096.html

Regards,
Markus

Re: [PATCH] Drivers: hv: vmbus: One function call less in create_gpadl_header() after error detection

[Dan Carpenter]

On Wed, Jan 10, 2024 at 05:41:00AM +0000, Michael Kelley wrote:
> From: Markus Elfring <Markus.Elfring@web.de> Sent: Tuesday, December 26, 2023 11:09 AM
> > 
> > The kfree() function was called in two cases by
> > the create_gpadl_header() function during error handling
> > even if the passed variable contained a null pointer.
> > This issue was detected by using the Coccinelle software.
> > 
> > Thus use another label.
> 
> Interestingly, there's a third case in this function where
> "goto nomem" is done, and in this case, msgbody is NULL.
> Does Coccinelle not complain about that case as well?
> 
> As I'm sure you know, the code is correct as is, because kfree()
> checks for a NULL argument.  So this is really an exercise in
> making Coccinelle happy.

Coccinelle is a kind of tool to search code.  Markus has created his own
search.  It's not a part of the standard Coccinelle scripts in
scripts/coccinelle/ or a CodingStyle issue or anything.  It's just a
matter of Markus prefering one style over another.

regards,
dan carpenter

RE: Drivers: hv: vmbus: One function call less in create_gpadl_header() after error detection

[Michael Kelley]

From: Markus Elfring <Markus.Elfring@web.de> Sent: Wednesday, January 10, 2024 2:58 AM
> 
> >> The kfree() function was called in two cases by
> >> the create_gpadl_header() function during error handling
> >> even if the passed variable contained a null pointer.
> >> This issue was detected by using the Coccinelle software.
> >>
> >> Thus use another label.
> >
> > Interestingly, there's a third case in this function where
> > "goto nomem" is done, and in this case, msgbody is NULL.
> > Does Coccinelle not complain about that case as well?
> >
> > As I'm sure you know, the code is correct as is, because kfree()
> > checks for a NULL argument.  So this is really an exercise in
> > making Coccinelle happy.  To me, the additional label is
> > incremental complexity for someone to deal with when
> > reading the code at some time in the future.  So I'd vote for
> > leaving the code as is.  But it's not a big deal either way.  I
> > can see you've been cleaning up a lot of Coccinelle-reported
> > issues across the kernel, most of which result in code
> > simplifications.  If leaving this unchanged causes you problems,
> > then I won't object (though perhaps that 3rd "goto nomem"
> > should be dealt with as well for consistency).
> 
> How do you think about the clarification approach
> “Reconsidering kfree() calls for null pointers (with SmPL)”?
> https://lore.kernel.org/cocci/6cbcf640-55e5-2f11-4a09-716fe681c0d2@web.de/
> https://sympa.inria.fr/sympa/arc/cocci/2023-03/msg00096.html
> 

It occurred to me overnight that the existing error handling
in create_gpadl_header() is unnecessarily complicated.  Here's
an approach that I think would fix what you have flagged, and
would reduce complexity instead of increasing it.  Thoughts?

Michael

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 56f7e06c673e..44b1d5c8dfed 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -336,7 +336,7 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
 			  sizeof(struct gpa_range) + pfncount * sizeof(u64);
 		msgheader =  kzalloc(msgsize, GFP_KERNEL);
 		if (!msgheader)
-			goto nomem;
+			return -ENOMEM;
 
 		INIT_LIST_HEAD(&msgheader->submsglist);
 		msgheader->msgsize = msgsize;
@@ -386,8 +386,8 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
 					list_del(&pos->msglistentry);
 					kfree(pos);
 				}
-
-				goto nomem;
+				kfree(msgheader);
+				return -ENOMEM;
 			}
 
 			msgbody->msgsize = msgsize;
@@ -416,8 +416,8 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
 			  sizeof(struct vmbus_channel_gpadl_header) +
 			  sizeof(struct gpa_range) + pagecount * sizeof(u64);
 		msgheader = kzalloc(msgsize, GFP_KERNEL);
-		if (msgheader == NULL)
-			goto nomem;
+		if (!msgheader)
+			return -ENOMEM;
 
 		INIT_LIST_HEAD(&msgheader->submsglist);
 		msgheader->msgsize = msgsize;
@@ -437,10 +437,6 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
 	}
 
 	return 0;
-nomem:
-	kfree(msgheader);
-	kfree(msgbody);
-	return -ENOMEM;
 }
 
 /*

Re: Drivers: hv: vmbus: One function call less in create_gpadl_header() after error detection

[Markus Elfring]

> It occurred to me overnight that the existing error handling
> in create_gpadl_header() is unnecessarily complicated.  Here's
> an approach that I think would fix what you have flagged, and
> would reduce complexity instead of increasing it.  Thoughts?

I find this development view interesting.


> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index 56f7e06c673e..44b1d5c8dfed 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -336,7 +336,7 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
>  			  sizeof(struct gpa_range) + pfncount * sizeof(u64);
>  		msgheader =  kzalloc(msgsize, GFP_KERNEL);
>  		if (!msgheader)
> -			goto nomem;
> +			return -ENOMEM;
>
>  		INIT_LIST_HEAD(&msgheader->submsglist);
>  		msgheader->msgsize = msgsize;
> @@ -386,8 +386,8 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
>  					list_del(&pos->msglistentry);
>  					kfree(pos);
>  				}
> -
> -				goto nomem;
> +				kfree(msgheader);
> +				return -ENOMEM;
>  			}
>
>  			msgbody->msgsize = msgsize;
> @@ -416,8 +416,8 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
>  			  sizeof(struct vmbus_channel_gpadl_header) +
>  			  sizeof(struct gpa_range) + pagecount * sizeof(u64);
>  		msgheader = kzalloc(msgsize, GFP_KERNEL);
> -		if (msgheader == NULL)
> -			goto nomem;
> +		if (!msgheader)
> +			return -ENOMEM;
>
>  		INIT_LIST_HEAD(&msgheader->submsglist);
>  		msgheader->msgsize = msgsize;
> @@ -437,10 +437,6 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
>  	}
>
>  	return 0;
> -nomem:
> -	kfree(msgheader);
> -	kfree(msgbody);
> -	return -ENOMEM;
>  }
>
>  /*
>

Should up to two memory areas still be released after a data processing failure?

Regards,
Markus

RE: Drivers: hv: vmbus: One function call less in create_gpadl_header() after error detection

[Michael Kelley]

From: Markus Elfring <Markus.Elfring@web.de> Sent: Wednesday, January 10, 2024 9:08 AM
> 
> > It occurred to me overnight that the existing error handling
> > in create_gpadl_header() is unnecessarily complicated.  Here's
> > an approach that I think would fix what you have flagged, and
> > would reduce complexity instead of increasing it.  Thoughts?
> 
> I find this development view interesting.
> 
> 
> > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> > index 56f7e06c673e..44b1d5c8dfed 100644
> > --- a/drivers/hv/channel.c
> > +++ b/drivers/hv/channel.c
> > @@ -336,7 +336,7 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
> >  			  sizeof(struct gpa_range) + pfncount * sizeof(u64);
> >  		msgheader =  kzalloc(msgsize, GFP_KERNEL);
> >  		if (!msgheader)
> > -			goto nomem;
> > +			return -ENOMEM;
> >
> >  		INIT_LIST_HEAD(&msgheader->submsglist);
> >  		msgheader->msgsize = msgsize;
> > @@ -386,8 +386,8 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
> >  					list_del(&pos->msglistentry);
> >  					kfree(pos);
> >  				}
> > -
> > -				goto nomem;
> > +				kfree(msgheader);
> > +				return -ENOMEM;
> >  			}
> >
> >  			msgbody->msgsize = msgsize;
> > @@ -416,8 +416,8 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
> >  			  sizeof(struct vmbus_channel_gpadl_header) +
> >  			  sizeof(struct gpa_range) + pagecount * sizeof(u64);
> >  		msgheader = kzalloc(msgsize, GFP_KERNEL);
> > -		if (msgheader == NULL)
> > -			goto nomem;
> > +		if (!msgheader)
> > +			return -ENOMEM;
> >
> >  		INIT_LIST_HEAD(&msgheader->submsglist);
> >  		msgheader->msgsize = msgsize;
> > @@ -437,10 +437,6 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
> >  	}
> >
> >  	return 0;
> > -nomem:
> > -	kfree(msgheader);
> > -	kfree(msgbody);
> > -	return -ENOMEM;
> >  }
> >
> >  /*
> >
> 
> Should up to two memory areas still be released after a data processing
> failure?

The function create_gpadl_header() is responsible only for allocating
the memory and filling in various fields.  It doesn't do any processing of
the data and doesn't generate any errors other than memory allocation
failures.  If create_gpadl_header() succeeds, it returns a pointer to the
allocated memory via the msginfo parameter, and the caller becomes
responsible for free'ing the memory.

The only caller is __vmbus_establish_gpadl(), which *does* free the
memory after communicating with Hyper-V.   Processing errors may
occur when communicating with Hyper-V, but in a quick review,
__vmbus_establish_gpadl() seems to handle those errors and to
correctly free the memory.

Michael

Re: Drivers: hv: vmbus: One function call less in create_gpadl_header() after error detection

[Dan Carpenter]

The second half of the if statement is basically duplicated.  It doesn't
need to be treated as a special case.  We could do something like below.
I deliberately didn't delete the tabs.  Also I haven't tested it.

regards,
dan carpenter

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 56f7e06c673e..2ba65f9ad3f1 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -328,9 +328,9 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
 		  sizeof(struct gpa_range);
 	pfncount = pfnsize / sizeof(u64);
 
-	if (pagecount > pfncount) {
-		/* we need a gpadl body */
-		/* fill in the header */
+	if (pagecount < pfncount)
+		pfncount = pagecount;
+
 		msgsize = sizeof(struct vmbus_channel_msginfo) +
 			  sizeof(struct vmbus_channel_gpadl_header) +
 			  sizeof(struct gpa_range) + pfncount * sizeof(u64);
@@ -410,31 +410,6 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
 			pfnsum += pfncurr;
 			pfnleft -= pfncurr;
 		}
-	} else {
-		/* everything fits in a header */
-		msgsize = sizeof(struct vmbus_channel_msginfo) +
-			  sizeof(struct vmbus_channel_gpadl_header) +
-			  sizeof(struct gpa_range) + pagecount * sizeof(u64);
-		msgheader = kzalloc(msgsize, GFP_KERNEL);
-		if (msgheader == NULL)
-			goto nomem;
-
-		INIT_LIST_HEAD(&msgheader->submsglist);
-		msgheader->msgsize = msgsize;
-
-		gpadl_header = (struct vmbus_channel_gpadl_header *)
-			msgheader->msg;
-		gpadl_header->rangecount = 1;
-		gpadl_header->range_buflen = sizeof(struct gpa_range) +
-					 pagecount * sizeof(u64);
-		gpadl_header->range[0].byte_offset = 0;
-		gpadl_header->range[0].byte_count = hv_gpadl_size(type, size);
-		for (i = 0; i < pagecount; i++)
-			gpadl_header->range[0].pfn_array[i] = hv_gpadl_hvpfn(
-				type, kbuffer, size, send_offset, i);
-
-		*msginfo = msgheader;
-	}
 
 	return 0;
 nomem:

RE: Drivers: hv: vmbus: One function call less in create_gpadl_header() after error detection

[Michael Kelley]

From: Dan Carpenter <dan.carpenter@linaro.org> Sent: Wednesday, January 10, 2024 10:38 AM
> 
> The second half of the if statement is basically duplicated.  It doesn't
> need to be treated as a special case.  We could do something like below.
> I deliberately didn't delete the tabs.  Also I haven't tested it.

Indeed!  I looked at the history, and this function has been
structured with the duplication since sometime in 2010, which
pre-dates my involvement by several years.  I don't know of
any reason why the duplication is needed, and agree it could
be eliminated.

Assuming Markus is OK with my proposal on the handling of
memory allocation failures, a single patch could simplify this
function quite a bit.

Dan -- do you want to create and submit the patch?  I'll test the
code on Hyper-V.  Or I can create, test, and submit the patch with
a "Suggested-by: Dan Carpenter".

Michael

> 
> regards,
> dan carpenter
> 
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index 56f7e06c673e..2ba65f9ad3f1 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -328,9 +328,9 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
>  		  sizeof(struct gpa_range);
>  	pfncount = pfnsize / sizeof(u64);
> 
> -	if (pagecount > pfncount) {
> -		/* we need a gpadl body */
> -		/* fill in the header */
> +	if (pagecount < pfncount)
> +		pfncount = pagecount;
> +
>  		msgsize = sizeof(struct vmbus_channel_msginfo) +
>  			  sizeof(struct vmbus_channel_gpadl_header) +
>  			  sizeof(struct gpa_range) + pfncount * sizeof(u64);
> @@ -410,31 +410,6 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
>  			pfnsum += pfncurr;
>  			pfnleft -= pfncurr;
>  		}
> -	} else {
> -		/* everything fits in a header */
> -		msgsize = sizeof(struct vmbus_channel_msginfo) +
> -			  sizeof(struct vmbus_channel_gpadl_header) +
> -			  sizeof(struct gpa_range) + pagecount * sizeof(u64);
> -		msgheader = kzalloc(msgsize, GFP_KERNEL);
> -		if (msgheader == NULL)
> -			goto nomem;
> -
> -		INIT_LIST_HEAD(&msgheader->submsglist);
> -		msgheader->msgsize = msgsize;
> -
> -		gpadl_header = (struct vmbus_channel_gpadl_header *)
> -			msgheader->msg;
> -		gpadl_header->rangecount = 1;
> -		gpadl_header->range_buflen = sizeof(struct gpa_range) +
> -					 pagecount * sizeof(u64);
> -		gpadl_header->range[0].byte_offset = 0;
> -		gpadl_header->range[0].byte_count = hv_gpadl_size(type, size);
> -		for (i = 0; i < pagecount; i++)
> -			gpadl_header->range[0].pfn_array[i] = hv_gpadl_hvpfn(
> -				type, kbuffer, size, send_offset, i);
> -
> -		*msginfo = msgheader;
> -	}
> 
>  	return 0;
>  nomem:

Re: Drivers: hv: vmbus: One function call less in create_gpadl_header() after error detection

[Dan Carpenter]

On Wed, Jan 10, 2024 at 10:17:17PM +0000, Michael Kelley wrote:
> From: Dan Carpenter <dan.carpenter@linaro.org> Sent: Wednesday, January 10, 2024 10:38 AM
> > 
> > The second half of the if statement is basically duplicated.  It doesn't
> > need to be treated as a special case.  We could do something like below.
> > I deliberately didn't delete the tabs.  Also I haven't tested it.
> 
> Indeed!  I looked at the history, and this function has been
> structured with the duplication since sometime in 2010, which
> pre-dates my involvement by several years.  I don't know of
> any reason why the duplication is needed, and agree it could
> be eliminated.
> 
> Assuming Markus is OK with my proposal on the handling of
> memory allocation failures, a single patch could simplify this
> function quite a bit.
> 
> Dan -- do you want to create and submit the patch?  I'll test the
> code on Hyper-V.  Or I can create, test, and submit the patch with
> a "Suggested-by: Dan Carpenter".

I messed up the if statement the first couple times I tried to think
about it so I don't trust myself here.  Could you give me the
Suggested-by tag?

regards,
dan carpenter

RE: Drivers: hv: vmbus: One function call less in create_gpadl_header() after error detection

[Michael Kelley]

From: Dan Carpenter <dan.carpenter@linaro.org> Sent: Wednesday, January 10, 2024 11:05 PM
> 
> On Wed, Jan 10, 2024 at 10:17:17PM +0000, Michael Kelley wrote:
> > From: Dan Carpenter <dan.carpenter@linaro.org> Sent: Wednesday, January
> 10, 2024 10:38 AM
> > >
> > > The second half of the if statement is basically duplicated.  It doesn't
> > > need to be treated as a special case.  We could do something like below.
> > > I deliberately didn't delete the tabs.  Also I haven't tested it.
> >
> > Indeed!  I looked at the history, and this function has been
> > structured with the duplication since sometime in 2010, which
> > pre-dates my involvement by several years.  I don't know of
> > any reason why the duplication is needed, and agree it could
> > be eliminated.
> >
> > Assuming Markus is OK with my proposal on the handling of
> > memory allocation failures, a single patch could simplify this
> > function quite a bit.
> >
> > Dan -- do you want to create and submit the patch?  I'll test the
> > code on Hyper-V.  Or I can create, test, and submit the patch with
> > a "Suggested-by: Dan Carpenter".
> 
> I messed up the if statement the first couple times I tried to think
> about it so I don't trust myself here.  Could you give me the
> Suggested-by tag?
> 

Will do.

Michael