[PATCH v2] staging: unisys: uislib: uisqueue.c: rewrite of do_locked_client_insert

[PATCH v2] staging: unisys: uislib: uisqueue.c: rewrite of do_locked_client_insert

[Sudip Mukherjee]

From: Sudip Mukherjee <sudip@vectorindia.org>

removed unused variables
fixed sparse warning of context imbalance in 'do_locked_client_insert'
                         different lock contexts for basic block

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---

v1 of the patch of the patch just fixed the sparse warning.
On suggestion of Dan Carpenter v2 is the total rewrite of the function.
Two of the function arguments (interruptHandle,channelId) are also not used. Wanted to remove them as well , 
but then thought maybe the original author have planned for some use of those variables.

 drivers/staging/unisys/uislib/uisqueue.c | 37 ++++++++------------------------
 1 file changed, 9 insertions(+), 28 deletions(-)

diff --git a/drivers/staging/unisys/uislib/uisqueue.c b/drivers/staging/unisys/uislib/uisqueue.c
index 63dbe7c..acfa46d 100644
--- a/drivers/staging/unisys/uislib/uisqueue.c
+++ b/drivers/staging/unisys/uislib/uisqueue.c
@@ -78,39 +78,20 @@ do_locked_client_insert(struct uisqueue_info *queueinfo,
 			u64 interruptHandle, u8 *channelId)
 {
 	unsigned long flags;
-	unsigned char queueWasEmpty;
-	unsigned int locked = 0;
-	unsigned int acquired = 0;
 	u8 rc = 0;
 
 	spin_lock_irqsave(lock, flags);
-	locked = 1;
-
 	if (!ULTRA_CHANNEL_CLIENT_ACQUIRE_OS(queueinfo->chan, channelId, NULL))
-		goto Away;
-
-	acquired = 1;
-
-	queueWasEmpty = visor_signalqueue_empty(queueinfo->chan, whichqueue);
-	if (!visor_signal_insert(queueinfo->chan, whichqueue, pSignal))
-		goto Away;
-	ULTRA_CHANNEL_CLIENT_RELEASE_OS(queueinfo->chan, channelId, NULL);
-	acquired = 0;
-
-	queueinfo->packets_sent++;
-
-	rc = 1;
-Away:
-	if (acquired) {
-		ULTRA_CHANNEL_CLIENT_RELEASE_OS(queueinfo->chan, channelId,
-						NULL);
-		acquired = 0;
-	}
-	if (locked) {
-		spin_unlock_irqrestore((spinlock_t *) lock, flags);
-		locked = 0;
+		goto unlock;
+	visor_signalqueue_empty(queueinfo->chan, whichqueue);
+	/*visor_signal_insert() only return 0 or 1 */
+	if (visor_signal_insert(queueinfo->chan, whichqueue, pSignal) == 1) {
+		queueinfo->packets_sent++;
+		rc = 1;
 	}
-
+	ULTRA_CHANNEL_CLIENT_RELEASE_OS(queueinfo->chan, channelId, NULL);
+unlock:
+	spin_unlock_irqrestore((spinlock_t *)lock, flags);
 	return rc;
 }
 
-- 
1.8.1.2

Re: [PATCH v2] staging: unisys: uislib: uisqueue.c: rewrite of do_locked_client_insert

[Greg Kroah-Hartman]

On Tue, Sep 02, 2014 at 11:46:35PM +0530, Sudip Mukherjee wrote:
> From: Sudip Mukherjee <sudip@vectorindia.org>
> 
> removed unused variables
> fixed sparse warning of context imbalance in 'do_locked_client_insert'
>                          different lock contexts for basic block
> 
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
> 
> v1 of the patch of the patch just fixed the sparse warning.
> On suggestion of Dan Carpenter v2 is the total rewrite of the function.
> Two of the function arguments (interruptHandle,channelId) are also not used. Wanted to remove them as well , 
> but then thought maybe the original author have planned for some use of those variables.
> 
>  drivers/staging/unisys/uislib/uisqueue.c | 37 ++++++++------------------------
>  1 file changed, 9 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/staging/unisys/uislib/uisqueue.c b/drivers/staging/unisys/uislib/uisqueue.c
> index 63dbe7c..acfa46d 100644
> --- a/drivers/staging/unisys/uislib/uisqueue.c
> +++ b/drivers/staging/unisys/uislib/uisqueue.c
> @@ -78,39 +78,20 @@ do_locked_client_insert(struct uisqueue_info *queueinfo,
>  			u64 interruptHandle, u8 *channelId)
>  {
>  	unsigned long flags;
> -	unsigned char queueWasEmpty;
> -	unsigned int locked = 0;
> -	unsigned int acquired = 0;
>  	u8 rc = 0;
>  
>  	spin_lock_irqsave(lock, flags);
> -	locked = 1;
> -
>  	if (!ULTRA_CHANNEL_CLIENT_ACQUIRE_OS(queueinfo->chan, channelId, NULL))
> -		goto Away;
> -
> -	acquired = 1;
> -
> -	queueWasEmpty = visor_signalqueue_empty(queueinfo->chan, whichqueue);
> -	if (!visor_signal_insert(queueinfo->chan, whichqueue, pSignal))
> -		goto Away;
> -	ULTRA_CHANNEL_CLIENT_RELEASE_OS(queueinfo->chan, channelId, NULL);
> -	acquired = 0;
> -
> -	queueinfo->packets_sent++;
> -
> -	rc = 1;
> -Away:
> -	if (acquired) {
> -		ULTRA_CHANNEL_CLIENT_RELEASE_OS(queueinfo->chan, channelId,
> -						NULL);
> -		acquired = 0;
> -	}
> -	if (locked) {
> -		spin_unlock_irqrestore((spinlock_t *) lock, flags);
> -		locked = 0;
> +		goto unlock;
> +	visor_signalqueue_empty(queueinfo->chan, whichqueue);
> +	/*visor_signal_insert() only return 0 or 1 */

Odd comment style (hint, you need a ' ' at the beginning...)

And why comment this at all?

thanks,

greg k-h

Re: [PATCH v2] staging: unisys: uislib: uisqueue.c: rewrite of do_locked_client_insert

[Sudip Mukherjee]

On Tue, Sep 02, 2014 at 11:24:02AM -0700, Greg Kroah-Hartman wrote:
> On Tue, Sep 02, 2014 at 11:46:35PM +0530, Sudip Mukherjee wrote:
> > From: Sudip Mukherjee <sudip@vectorindia.org>
> > 
> > removed unused variables
> > fixed sparse warning of context imbalance in 'do_locked_client_insert'
> >                          different lock contexts for basic block
> > 
> > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> > ---
> > 
> > v1 of the patch of the patch just fixed the sparse warning.
> > On suggestion of Dan Carpenter v2 is the total rewrite of the function.
> > Two of the function arguments (interruptHandle,channelId) are also not used. Wanted to remove them as well , 
> > but then thought maybe the original author have planned for some use of those variables.
> > 
> >  drivers/staging/unisys/uislib/uisqueue.c | 37 ++++++++------------------------
> >  1 file changed, 9 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/staging/unisys/uislib/uisqueue.c b/drivers/staging/unisys/uislib/uisqueue.c
> > index 63dbe7c..acfa46d 100644
> > --- a/drivers/staging/unisys/uislib/uisqueue.c
> > +++ b/drivers/staging/unisys/uislib/uisqueue.c
> > @@ -78,39 +78,20 @@ do_locked_client_insert(struct uisqueue_info *queueinfo,
> >  			u64 interruptHandle, u8 *channelId)
> >  {
> >  	unsigned long flags;
> > -	unsigned char queueWasEmpty;
> > -	unsigned int locked = 0;
> > -	unsigned int acquired = 0;
> >  	u8 rc = 0;
> >  
> >  	spin_lock_irqsave(lock, flags);
> > -	locked = 1;
> > -
> >  	if (!ULTRA_CHANNEL_CLIENT_ACQUIRE_OS(queueinfo->chan, channelId, NULL))
> > -		goto Away;
> > -
> > -	acquired = 1;
> > -
> > -	queueWasEmpty = visor_signalqueue_empty(queueinfo->chan, whichqueue);
> > -	if (!visor_signal_insert(queueinfo->chan, whichqueue, pSignal))
> > -		goto Away;
> > -	ULTRA_CHANNEL_CLIENT_RELEASE_OS(queueinfo->chan, channelId, NULL);
> > -	acquired = 0;
> > -
> > -	queueinfo->packets_sent++;
> > -
> > -	rc = 1;
> > -Away:
> > -	if (acquired) {
> > -		ULTRA_CHANNEL_CLIENT_RELEASE_OS(queueinfo->chan, channelId,
> > -						NULL);
> > -		acquired = 0;
> > -	}
> > -	if (locked) {
> > -		spin_unlock_irqrestore((spinlock_t *) lock, flags);
> > -		locked = 0;
> > +		goto unlock;
> > +	visor_signalqueue_empty(queueinfo->chan, whichqueue);
> > +	/*visor_signal_insert() only return 0 or 1 */
> 
> Odd comment style (hint, you need a ' ' at the beginning...)
> 
> And why comment this at all?
> 
> thanks,
> 
> greg k-h

comment is actually not required. thought it will be easier to understant the code if the return values can be see.
I will delete the comment and send it again.

thanks
sudip

Re: [PATCH v2] staging: unisys: uislib: uisqueue.c: rewrite of do_locked_client_insert

[Dan Carpenter]

On Tue, Sep 02, 2014 at 11:46:35PM +0530, Sudip Mukherjee wrote:
> From: Sudip Mukherjee <sudip@vectorindia.org>

I really would prefer if you just figured out your email settings so
this isn't needed.  The From: header is mostly used for people
forwarding patches from other people.  We have allowed people to use
the From header like this if they can't get their corporate email
configured properly but I try to discorage it.  If everyone starts using
>From headers like this then it becomes a pain to deal with.

> 
> removed unused variables
> fixed sparse warning of context imbalance in 'do_locked_client_insert'
>                          different lock contexts for basic block
> 
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
> 

This patch is much better and more interesting, but I still want some
more changes.

> v1 of the patch of the patch just fixed the sparse warning.
> On suggestion of Dan Carpenter v2 is the total rewrite of the function.
> Two of the function arguments (interruptHandle,channelId) are also not used. Wanted to remove them as well , 
> but then thought maybe the original author have planned for some use of those variables.

In the kernel we don't put code in until we are ready to use it.  Don't
worry about future changes.  But on the other hand, don't remove the
parameters in this patch because that is doing too many changes in one
patch.  It would have to be done in a follow on patch if you decide to
do it.

> -	if (locked) {
> -		spin_unlock_irqrestore((spinlock_t *) lock, flags);
> -		locked = 0;
> +		goto unlock;
> +	visor_signalqueue_empty(queueinfo->chan, whichqueue);

Just remove this function.  But mention it in the changelog in case
there are side effects.

> +	/*visor_signal_insert() only return 0 or 1 */

Don't put obvious comments like this.  A normal reader will assume that
this function is boolean based on how it is used.

> +	if (visor_signal_insert(queueinfo->chan, whichqueue, pSignal) == 1) {

Don't put the == 1.  In terms of English, 1 really is intended as
"success" and not the number one.  Also don't test for == true or
== false.

	if (foo) {
	if (foo == true) {

These two statement *mean* the same thing in terms of English, but the
first one is simpler and less wordy.

regards,
dan carpenter

Re: [PATCH v2] staging: unisys: uislib: uisqueue.c: rewrite of do_locked_client_insert

[Sudip Mukherjee]

On Wed, Sep 03, 2014 at 11:40:38AM +0300, Dan Carpenter wrote:
> On Tue, Sep 02, 2014 at 11:46:35PM +0530, Sudip Mukherjee wrote:
> > From: Sudip Mukherjee <sudip@vectorindia.org>
> 
> I really would prefer if you just figured out your email settings so
> this isn't needed.  The From: header is mostly used for people
> forwarding patches from other people.  We have allowed people to use
> the From header like this if they can't get their corporate email
> configured properly but I try to discorage it.  If everyone starts using
> From headers like this then it becomes a pain to deal with.
> 
I will configure the corporate mail. I am the server admin , so there should
not be any problem in settings. :)

> > 
> > removed unused variables
> > fixed sparse warning of context imbalance in 'do_locked_client_insert'
> >                          different lock contexts for basic block
> > 
> > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> > ---
> > 
> 
> This patch is much better and more interesting, but I still want some
> more changes.
> 
I have already sent v3 of the patch just before your mail , based on 
what greg k-h has suggested about the commnent. Please discard that.

> > v1 of the patch of the patch just fixed the sparse warning.
> > On suggestion of Dan Carpenter v2 is the total rewrite of the function.
> > Two of the function arguments (interruptHandle,channelId) are also not used. Wanted to remove them as well , 
> > but then thought maybe the original author have planned for some use of those variables.
> 
> In the kernel we don't put code in until we are ready to use it.  Don't
> worry about future changes.  But on the other hand, don't remove the
> parameters in this patch because that is doing too many changes in one
> patch.  It would have to be done in a follow on patch if you decide to
> do it.
> 
> > -	if (locked) {
> > -		spin_unlock_irqrestore((spinlock_t *) lock, flags);
> > -		locked = 0;
> > +		goto unlock;
> > +	visor_signalqueue_empty(queueinfo->chan, whichqueue);
> 
> Just remove this function.  But mention it in the changelog in case
> there are side effects.
> 
> > +	/*visor_signal_insert() only return 0 or 1 */
> 
> Don't put obvious comments like this.  A normal reader will assume that
> this function is boolean based on how it is used.
> 
> > +	if (visor_signal_insert(queueinfo->chan, whichqueue, pSignal) == 1) {
> 
> Don't put the == 1.  In terms of English, 1 really is intended as
> "success" and not the number one.  Also don't test for == true or
> == false.
> 
> 	if (foo) {
> 	if (foo == true) {
> 
> These two statement *mean* the same thing in terms of English, but the
> first one is simpler and less wordy.
> 
> regards,
> dan carpenter

thanks
sudip

Re: [PATCH v2] staging: unisys: uislib: uisqueue.c: rewrite of do_locked_client_insert

[Dan Carpenter]

On Wed, Sep 03, 2014 at 02:29:44PM +0530, Sudip Mukherjee wrote:
> > This patch is much better and more interesting, but I still want some
> > more changes.
> > 
> I have already sent v3 of the patch just before your mail , based on 
> what greg k-h has suggested about the commnent. Please discard that.

Greg has a million emails in his inbox so you need to reply to the patch
itself if you want him to discard it.  If it's in a different thread he
can't keep track.

regards,
dan carpenter

Re: [PATCH v2] staging: unisys: uislib: uisqueue.c: rewrite of do_locked_client_insert

[Sudip Mukherjee]

On Wed, Sep 03, 2014 at 02:29:44PM +0530, Sudip Mukherjee wrote:
> On Wed, Sep 03, 2014 at 11:40:38AM +0300, Dan Carpenter wrote:
> > On Tue, Sep 02, 2014 at 11:46:35PM +0530, Sudip Mukherjee wrote:
> > > From: Sudip Mukherjee <sudip@vectorindia.org>
> > 
> > I really would prefer if you just figured out your email settings so
> > this isn't needed.  The From: header is mostly used for people
> > forwarding patches from other people.  We have allowed people to use
> > the From header like this if they can't get their corporate email
> > configured properly but I try to discorage it.  If everyone starts using
> > From headers like this then it becomes a pain to deal with.
> > 
> I will configure the corporate mail. I am the server admin , so there should
> not be any problem in settings. :)
> 
v4 of the patch was sent from the corporate mail. The settings were done.
But the problem is coming in a different area. I have given strict DMARC check 
for the corporate mail server. DMARC = domain based message authentication.
So the mail i sent reached all the list subscriber from a different server than 
our designated server , and as a result it has been marked as spam in many places.
I have already received a few complaints regarding that.
Is there any other way that i send the patch from my personal account , 
and use my corporate mail in Signed-off-by ... 

thanks
sudip

> > > 
> > > removed unused variables
> > > fixed sparse warning of context imbalance in 'do_locked_client_insert'
> > >                          different lock contexts for basic block
> > > 
> > > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> > > ---
> > > 
> > 
> > This patch is much better and more interesting, but I still want some
> > more changes.
> > 
> I have already sent v3 of the patch just before your mail , based on 
> what greg k-h has suggested about the commnent. Please discard that.
> 
> > > v1 of the patch of the patch just fixed the sparse warning.
> > > On suggestion of Dan Carpenter v2 is the total rewrite of the function.
> > > Two of the function arguments (interruptHandle,channelId) are also not used. Wanted to remove them as well , 
> > > but then thought maybe the original author have planned for some use of those variables.
> > 
> > In the kernel we don't put code in until we are ready to use it.  Don't
> > worry about future changes.  But on the other hand, don't remove the
> > parameters in this patch because that is doing too many changes in one
> > patch.  It would have to be done in a follow on patch if you decide to
> > do it.
> > 
> > > -	if (locked) {
> > > -		spin_unlock_irqrestore((spinlock_t *) lock, flags);
> > > -		locked = 0;
> > > +		goto unlock;
> > > +	visor_signalqueue_empty(queueinfo->chan, whichqueue);
> > 
> > Just remove this function.  But mention it in the changelog in case
> > there are side effects.
> > 
> > > +	/*visor_signal_insert() only return 0 or 1 */
> > 
> > Don't put obvious comments like this.  A normal reader will assume that
> > this function is boolean based on how it is used.
> > 
> > > +	if (visor_signal_insert(queueinfo->chan, whichqueue, pSignal) == 1) {
> > 
> > Don't put the == 1.  In terms of English, 1 really is intended as
> > "success" and not the number one.  Also don't test for == true or
> > == false.
> > 
> > 	if (foo) {
> > 	if (foo == true) {
> > 
> > These two statement *mean* the same thing in terms of English, but the
> > first one is simpler and less wordy.
> > 
> > regards,
> > dan carpenter
> 
> thanks
> sudip

Re: [PATCH v2] staging: unisys: uislib: uisqueue.c: rewrite of do_locked_client_insert

[Dan Carpenter]

On Wed, Sep 03, 2014 at 08:13:25PM +0530, Sudip Mukherjee wrote:
> On Wed, Sep 03, 2014 at 02:29:44PM +0530, Sudip Mukherjee wrote:
> > On Wed, Sep 03, 2014 at 11:40:38AM +0300, Dan Carpenter wrote:
> > > On Tue, Sep 02, 2014 at 11:46:35PM +0530, Sudip Mukherjee wrote:
> > > > From: Sudip Mukherjee <sudip@vectorindia.org>
> > > 
> > > I really would prefer if you just figured out your email settings so
> > > this isn't needed.  The From: header is mostly used for people
> > > forwarding patches from other people.  We have allowed people to use
> > > the From header like this if they can't get their corporate email
> > > configured properly but I try to discorage it.  If everyone starts using
> > > From headers like this then it becomes a pain to deal with.
> > > 
> > I will configure the corporate mail. I am the server admin , so there should
> > not be any problem in settings. :)
> > 
> v4 of the patch was sent from the corporate mail. The settings were done.
> But the problem is coming in a different area. I have given strict DMARC check 
> for the corporate mail server. DMARC = domain based message authentication.
> So the mail i sent reached all the list subscriber from a different server than 
> our designated server , and as a result it has been marked as spam in many places.
> I have already received a few complaints regarding that.
> Is there any other way that i send the patch from my personal account , 
> and use my corporate mail in Signed-off-by ... 

I guess it's ok by me.  It's up to Greg.

regards,
dan carpenter

Re: [PATCH v2] staging: unisys: uislib: uisqueue.c: rewrite of do_locked_client_insert

[Sudip Mukherjee]

On Wed, Sep 03, 2014 at 06:41:30PM +0300, Dan Carpenter wrote:
> On Wed, Sep 03, 2014 at 08:13:25PM +0530, Sudip Mukherjee wrote:
> > On Wed, Sep 03, 2014 at 02:29:44PM +0530, Sudip Mukherjee wrote:
> > > On Wed, Sep 03, 2014 at 11:40:38AM +0300, Dan Carpenter wrote:
> > > > On Tue, Sep 02, 2014 at 11:46:35PM +0530, Sudip Mukherjee wrote:
> > > > > From: Sudip Mukherjee <sudip@vectorindia.org>
> > > > 
> > > > I really would prefer if you just figured out your email settings so
> > > > this isn't needed.  The From: header is mostly used for people
> > > > forwarding patches from other people.  We have allowed people to use
> > > > the From header like this if they can't get their corporate email
> > > > configured properly but I try to discorage it.  If everyone starts using
> > > > From headers like this then it becomes a pain to deal with.
> > > > 
> > > I will configure the corporate mail. I am the server admin , so there should
> > > not be any problem in settings. :)
> > > 
> > v4 of the patch was sent from the corporate mail. The settings were done.
> > But the problem is coming in a different area. I have given strict DMARC check 
> > for the corporate mail server. DMARC = domain based message authentication.
> > So the mail i sent reached all the list subscriber from a different server than 
> > our designated server , and as a result it has been marked as spam in many places.
> > I have already received a few complaints regarding that.
> > Is there any other way that i send the patch from my personal account , 
> > and use my corporate mail in Signed-off-by ... 
> 
> I guess it's ok by me.  It's up to Greg.
> 

Greh K-H has already accepted some of my patches and some are still pending :( , where from was my personal email and
Signed-off-by as my corporate email. So i guess its ok for him also. .

thanks
sudip


> regards,
> dan carpenter
>