[PATCH] Staging: rts5208: sd: Remove redundant code

[PATCH] Staging: rts5208: sd: Remove redundant code

[Shivani Bhardwaj]

Change the order of statements and remove extra code.

Signed-off-by: Shivani Bhardwaj <shivanib134@gmail.com>
---
 drivers/staging/rts5208/sd.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/rts5208/sd.c b/drivers/staging/rts5208/sd.c
index d6c4982..4269be9 100644
--- a/drivers/staging/rts5208/sd.c
+++ b/drivers/staging/rts5208/sd.c
@@ -2707,17 +2707,14 @@ RTY_SD_RST:
 		return STATUS_FAIL;
 	}
 
+	CLR_SD_HCXC(sd_card);
+
 	if (hi_cap_flow) {
 		if (rsp[1] & 0x40)
 			SET_SD_HCXC(sd_card);
-		else
-			CLR_SD_HCXC(sd_card);
-
-		support_1v8 = false;
-	} else {
-		CLR_SD_HCXC(sd_card);
-		support_1v8 = false;
 	}
+
+	support_1v8 = false;
 	dev_dbg(rtsx_dev(chip), "support_1v8 = %d\n", support_1v8);
 
 	if (support_1v8) {
-- 
2.1.0

Re: [Outreachy kernel] [PATCH] Staging: rts5208: sd: Remove redundant code

[Greg KH]

On Wed, Oct 14, 2015 at 04:30:10AM +0530, Shivani Bhardwaj wrote:
> Change the order of statements and remove extra code.
> 
> Signed-off-by: Shivani Bhardwaj <shivanib134@gmail.com>
> ---
>  drivers/staging/rts5208/sd.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/rts5208/sd.c b/drivers/staging/rts5208/sd.c
> index d6c4982..4269be9 100644
> --- a/drivers/staging/rts5208/sd.c
> +++ b/drivers/staging/rts5208/sd.c
> @@ -2707,17 +2707,14 @@ RTY_SD_RST:
>  		return STATUS_FAIL;
>  	}
>  
> +	CLR_SD_HCXC(sd_card);
> +
>  	if (hi_cap_flow) {
>  		if (rsp[1] & 0x40)
>  			SET_SD_HCXC(sd_card);
> -		else
> -			CLR_SD_HCXC(sd_card);
> -
> -		support_1v8 = false;
> -	} else {
> -		CLR_SD_HCXC(sd_card);
> -		support_1v8 = false;
>  	}
> +
> +	support_1v8 = false;
>  	dev_dbg(rtsx_dev(chip), "support_1v8 = %d\n", support_1v8);
>  
>  	if (support_1v8) {

Are you sure you didn't change the logic of what is happening here?

Why are you making this change?  What prompted it?

thanks,

greg k-h

Re: [Outreachy kernel] [PATCH] Staging: rts5208: sd: Remove redundant code

[Shivani Bhardwaj]

On Wed, Oct 14, 2015 at 4:44 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Oct 14, 2015 at 04:30:10AM +0530, Shivani Bhardwaj wrote:
>> Change the order of statements and remove extra code.
>>
>> Signed-off-by: Shivani Bhardwaj <shivanib134@gmail.com>
>> ---
>>  drivers/staging/rts5208/sd.c | 11 ++++-------
>>  1 file changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/staging/rts5208/sd.c b/drivers/staging/rts5208/sd.c
>> index d6c4982..4269be9 100644
>> --- a/drivers/staging/rts5208/sd.c
>> +++ b/drivers/staging/rts5208/sd.c
>> @@ -2707,17 +2707,14 @@ RTY_SD_RST:
>>               return STATUS_FAIL;
>>       }
>>
>> +     CLR_SD_HCXC(sd_card);
>> +
>>       if (hi_cap_flow) {
>>               if (rsp[1] & 0x40)
>>                       SET_SD_HCXC(sd_card);
>> -             else
>> -                     CLR_SD_HCXC(sd_card);
>> -
>> -             support_1v8 = false;
>> -     } else {
>> -             CLR_SD_HCXC(sd_card);
>> -             support_1v8 = false;
>>       }
>> +
>> +     support_1v8 = false;
>>       dev_dbg(rtsx_dev(chip), "support_1v8 = %d\n", support_1v8);
>>
>>       if (support_1v8) {
>
> Are you sure you didn't change the logic of what is happening here?
>
> Why are you making this change?  What prompted it?
>
> thanks,
>
> greg k-h

Logic remains fine, I think. support_1v8 statement was common to if
and else so it is taken out. Also, it can be noted that programmer
wants to set sd_card value only if hi_cap_flow is not null and (rsp[1]
& 0x40) is true. For rest of the cases, programmer wants to clear the
sd_card.
I think I'm sure about the changes made.

I came across this code while running notnull.cocci against this.
However, after I checked the code, cocci script was not very apt, I
saw this, took references from
http://lxr.free-electrons.com/source/drivers/staging/rts5208/rtsx_chip.h#L421
about the macros and deleted the extra code by changing order.
I've explained the logic above, if you still find any discrepancies,
please tell me.

Thank you.

Re: [Outreachy kernel] [PATCH] Staging: rts5208: sd: Remove redundant code

[Greg KH]

On Wed, Oct 14, 2015 at 04:54:45AM +0530, Shivani Bhardwaj wrote:
> On Wed, Oct 14, 2015 at 4:44 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Wed, Oct 14, 2015 at 04:30:10AM +0530, Shivani Bhardwaj wrote:
> >> Change the order of statements and remove extra code.
> >>
> >> Signed-off-by: Shivani Bhardwaj <shivanib134@gmail.com>
> >> ---
> >>  drivers/staging/rts5208/sd.c | 11 ++++-------
> >>  1 file changed, 4 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/staging/rts5208/sd.c b/drivers/staging/rts5208/sd.c
> >> index d6c4982..4269be9 100644
> >> --- a/drivers/staging/rts5208/sd.c
> >> +++ b/drivers/staging/rts5208/sd.c
> >> @@ -2707,17 +2707,14 @@ RTY_SD_RST:
> >>               return STATUS_FAIL;
> >>       }
> >>
> >> +     CLR_SD_HCXC(sd_card);
> >> +
> >>       if (hi_cap_flow) {
> >>               if (rsp[1] & 0x40)
> >>                       SET_SD_HCXC(sd_card);
> >> -             else
> >> -                     CLR_SD_HCXC(sd_card);
> >> -
> >> -             support_1v8 = false;
> >> -     } else {
> >> -             CLR_SD_HCXC(sd_card);
> >> -             support_1v8 = false;
> >>       }
> >> +
> >> +     support_1v8 = false;
> >>       dev_dbg(rtsx_dev(chip), "support_1v8 = %d\n", support_1v8);
> >>
> >>       if (support_1v8) {
> >
> > Are you sure you didn't change the logic of what is happening here?
> >
> > Why are you making this change?  What prompted it?
> >
> > thanks,
> >
> > greg k-h
> 
> Logic remains fine, I think. support_1v8 statement was common to if
> and else so it is taken out. Also, it can be noted that programmer
> wants to set sd_card value only if hi_cap_flow is not null and (rsp[1]
> & 0x40) is true. For rest of the cases, programmer wants to clear the
> sd_card.
> I think I'm sure about the changes made.

But now you always call CLR_SD_HCXC() where before there was one code
path that did not make that call.  Are you sure it is safe to call
CLR_SD_HCXC() before SET_SD_HCXC()?

thanks,

greg k-h

Re: [Outreachy kernel] [PATCH] Staging: rts5208: sd: Remove redundant code

[Shivani Bhardwaj]

On Wed, Oct 14, 2015 at 5:09 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Oct 14, 2015 at 04:54:45AM +0530, Shivani Bhardwaj wrote:
>> On Wed, Oct 14, 2015 at 4:44 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> > On Wed, Oct 14, 2015 at 04:30:10AM +0530, Shivani Bhardwaj wrote:
>> >> Change the order of statements and remove extra code.
>> >>
>> >> Signed-off-by: Shivani Bhardwaj <shivanib134@gmail.com>
>> >> ---
>> >>  drivers/staging/rts5208/sd.c | 11 ++++-------
>> >>  1 file changed, 4 insertions(+), 7 deletions(-)
>> >>
>> >> diff --git a/drivers/staging/rts5208/sd.c b/drivers/staging/rts5208/sd.c
>> >> index d6c4982..4269be9 100644
>> >> --- a/drivers/staging/rts5208/sd.c
>> >> +++ b/drivers/staging/rts5208/sd.c
>> >> @@ -2707,17 +2707,14 @@ RTY_SD_RST:
>> >>               return STATUS_FAIL;
>> >>       }
>> >>
>> >> +     CLR_SD_HCXC(sd_card);
>> >> +
>> >>       if (hi_cap_flow) {
>> >>               if (rsp[1] & 0x40)
>> >>                       SET_SD_HCXC(sd_card);
>> >> -             else
>> >> -                     CLR_SD_HCXC(sd_card);
>> >> -
>> >> -             support_1v8 = false;
>> >> -     } else {
>> >> -             CLR_SD_HCXC(sd_card);
>> >> -             support_1v8 = false;
>> >>       }
>> >> +
>> >> +     support_1v8 = false;
>> >>       dev_dbg(rtsx_dev(chip), "support_1v8 = %d\n", support_1v8);
>> >>
>> >>       if (support_1v8) {
>> >
>> > Are you sure you didn't change the logic of what is happening here?
>> >
>> > Why are you making this change?  What prompted it?
>> >
>> > thanks,
>> >
>> > greg k-h
>>
>> Logic remains fine, I think. support_1v8 statement was common to if
>> and else so it is taken out. Also, it can be noted that programmer
>> wants to set sd_card value only if hi_cap_flow is not null and (rsp[1]
>> & 0x40) is true. For rest of the cases, programmer wants to clear the
>> sd_card.
>> I think I'm sure about the changes made.
>
> But now you always call CLR_SD_HCXC() where before there was one code
> path that did not make that call.  Are you sure it is safe to call
> CLR_SD_HCXC() before SET_SD_HCXC()?
>
> thanks,
>
> greg k-h

I don't think it should be a problem because that is what we
ultimately want in our code - to clear the memory card subject to
constraints that one of the two if conditions is wrong. Now, if both
the if statements, that is,
if (hi_cap_flow) and if (rsp[1] & 0x40)

turn out to be true, they set the cleared sd_card again.

That is what we wanted. If both the if statements are true - set the
sd_card else clear it up. :)

Re: [Outreachy kernel] [PATCH] Staging: rts5208: sd: Remove redundant code

[Shivani Bhardwaj]

On Wed, Oct 14, 2015 at 5:15 AM, Shivani Bhardwaj <shivanib134@gmail.com> wrote:
> On Wed, Oct 14, 2015 at 5:09 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> On Wed, Oct 14, 2015 at 04:54:45AM +0530, Shivani Bhardwaj wrote:
>>> On Wed, Oct 14, 2015 at 4:44 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>>> > On Wed, Oct 14, 2015 at 04:30:10AM +0530, Shivani Bhardwaj wrote:
>>> >> Change the order of statements and remove extra code.
>>> >>
>>> >> Signed-off-by: Shivani Bhardwaj <shivanib134@gmail.com>
>>> >> ---
>>> >>  drivers/staging/rts5208/sd.c | 11 ++++-------
>>> >>  1 file changed, 4 insertions(+), 7 deletions(-)
>>> >>
>>> >> diff --git a/drivers/staging/rts5208/sd.c b/drivers/staging/rts5208/sd.c
>>> >> index d6c4982..4269be9 100644
>>> >> --- a/drivers/staging/rts5208/sd.c
>>> >> +++ b/drivers/staging/rts5208/sd.c
>>> >> @@ -2707,17 +2707,14 @@ RTY_SD_RST:
>>> >>               return STATUS_FAIL;
>>> >>       }
>>> >>
>>> >> +     CLR_SD_HCXC(sd_card);
>>> >> +
>>> >>       if (hi_cap_flow) {
>>> >>               if (rsp[1] & 0x40)
>>> >>                       SET_SD_HCXC(sd_card);
>>> >> -             else
>>> >> -                     CLR_SD_HCXC(sd_card);
>>> >> -
>>> >> -             support_1v8 = false;
>>> >> -     } else {
>>> >> -             CLR_SD_HCXC(sd_card);
>>> >> -             support_1v8 = false;
>>> >>       }
>>> >> +
>>> >> +     support_1v8 = false;
>>> >>       dev_dbg(rtsx_dev(chip), "support_1v8 = %d\n", support_1v8);
>>> >>
>>> >>       if (support_1v8) {
>>> >
>>> > Are you sure you didn't change the logic of what is happening here?
>>> >
>>> > Why are you making this change?  What prompted it?
>>> >
>>> > thanks,
>>> >
>>> > greg k-h
>>>
>>> Logic remains fine, I think. support_1v8 statement was common to if
>>> and else so it is taken out. Also, it can be noted that programmer
>>> wants to set sd_card value only if hi_cap_flow is not null and (rsp[1]
>>> & 0x40) is true. For rest of the cases, programmer wants to clear the
>>> sd_card.
>>> I think I'm sure about the changes made.
>>
>> But now you always call CLR_SD_HCXC() where before there was one code
>> path that did not make that call.  Are you sure it is safe to call
>> CLR_SD_HCXC() before SET_SD_HCXC()?
>>
>> thanks,
>>
>> greg k-h
>
> I don't think it should be a problem because that is what we
> ultimately want in our code - to clear the memory card subject to
> constraints that one of the two if conditions is wrong. Now, if both
> the if statements, that is,
> if (hi_cap_flow) and if (rsp[1] & 0x40)
>
> turn out to be true, they set the cleared sd_card again.
>
> That is what we wanted. If both the if statements are true - set the
> sd_card else clear it up. :)

Also, a plus point is that even if the value is to be set (I do not
know the inside mechanism of how it will happen) , there would not be
any scope of errors (considering some bits are to be written/
logically operated) because sd_card would be empty. :)

Re: [Outreachy kernel] [PATCH] Staging: rts5208: sd: Remove redundant code

[Arnd Bergmann]

On Wednesday 14 October 2015 05:15:12 Shivani Bhardwaj wrote:
> On Wed, Oct 14, 2015 at 5:09 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Wed, Oct 14, 2015 at 04:54:45AM +0530, Shivani Bhardwaj wrote:
> >> On Wed, Oct 14, 2015 at 4:44 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> >> > On Wed, Oct 14, 2015 at 04:30:10AM +0530, Shivani Bhardwaj wrote:
> >> >> Change the order of statements and remove extra code.
> >> >> +     CLR_SD_HCXC(sd_card);
> >> >> +
> >> >>       if (hi_cap_flow) {
> >> >>               if (rsp[1] & 0x40)
> >> >>                       SET_SD_HCXC(sd_card);
> >> >> -             else
> >> >> -                     CLR_SD_HCXC(sd_card);
> >> >> -
> >> >> -             support_1v8 = false;
> >> >> -     } else {
> >> >> -             CLR_SD_HCXC(sd_card);
> >> >> -             support_1v8 = false;
> >> >>       }
> >> >> +
> >> >> +     support_1v8 = false;
> >> >>       dev_dbg(rtsx_dev(chip), "support_1v8 = %d\n", support_1v8);
> >> >>
> >> >>       if (support_1v8) {
> >> >
> >> > Are you sure you didn't change the logic of what is happening here?
> >> >
> >> > Why are you making this change?  What prompted it?
...

> I don't think it should be a problem because that is what we
> ultimately want in our code - to clear the memory card subject to
> constraints that one of the two if conditions is wrong. Now, if both
> the if statements, that is,
> if (hi_cap_flow) and if (rsp[1] & 0x40)
> 
> turn out to be true, they set the cleared sd_card again.
> 
> That is what we wanted. If both the if statements are true - set the
> sd_card else clear it up. 

I guess this would be easier to verify if it were not obscured by the
SET_SD_HCXC/CLR_SD_HCXC macros ;-)

Your change looks like a correct transformation, and it would be logical
to remove the support_1v8 variable next, as it can never be true.

However, I wonder whether that is just a bug in the driver, as UHS-1
cards are required to support 1.8v operation, and there is clearly
code to handle it here. Maybe one of them should actually say
"support_1v8 = true".

	Arnd

Re: [Outreachy kernel] [PATCH] Staging: rts5208: sd: Remove redundant code

[Shivani Bhardwaj]

On Wed, Oct 14, 2015 at 2:54 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 14 October 2015 05:15:12 Shivani Bhardwaj wrote:
>> On Wed, Oct 14, 2015 at 5:09 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> > On Wed, Oct 14, 2015 at 04:54:45AM +0530, Shivani Bhardwaj wrote:
>> >> On Wed, Oct 14, 2015 at 4:44 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> >> > On Wed, Oct 14, 2015 at 04:30:10AM +0530, Shivani Bhardwaj wrote:
>> >> >> Change the order of statements and remove extra code.
>> >> >> +     CLR_SD_HCXC(sd_card);
>> >> >> +
>> >> >>       if (hi_cap_flow) {
>> >> >>               if (rsp[1] & 0x40)
>> >> >>                       SET_SD_HCXC(sd_card);
>> >> >> -             else
>> >> >> -                     CLR_SD_HCXC(sd_card);
>> >> >> -
>> >> >> -             support_1v8 = false;
>> >> >> -     } else {
>> >> >> -             CLR_SD_HCXC(sd_card);
>> >> >> -             support_1v8 = false;
>> >> >>       }
>> >> >> +
>> >> >> +     support_1v8 = false;
>> >> >>       dev_dbg(rtsx_dev(chip), "support_1v8 = %d\n", support_1v8);
>> >> >>
>> >> >>       if (support_1v8) {
>> >> >
>> >> > Are you sure you didn't change the logic of what is happening here?
>> >> >
>> >> > Why are you making this change?  What prompted it?
> ...
>
>> I don't think it should be a problem because that is what we
>> ultimately want in our code - to clear the memory card subject to
>> constraints that one of the two if conditions is wrong. Now, if both
>> the if statements, that is,
>> if (hi_cap_flow) and if (rsp[1] & 0x40)
>>
>> turn out to be true, they set the cleared sd_card again.
>>
>> That is what we wanted. If both the if statements are true - set the
>> sd_card else clear it up.
>
> I guess this would be easier to verify if it were not obscured by the
> SET_SD_HCXC/CLR_SD_HCXC macros ;-)
>
> Your change looks like a correct transformation, and it would be logical
> to remove the support_1v8 variable next, as it can never be true.
>
> However, I wonder whether that is just a bug in the driver, as UHS-1
> cards are required to support 1.8v operation, and there is clearly
> code to handle it here. Maybe one of them should actually say
> "support_1v8 = true".
>
>         Arnd

I do not exactly understand the role of variables here. But, as far as
I can guess (after having checked the secure digital)  support_1v8
should be true for hi_cap_flow=false. That is, if capacity flow is
less, we can allow voltage to be 1.8V otherwise it should be 3.3V.
However, I really do not understand what the array rsp is for. So, I'm
not sure what its status should be.
Please correct me if I am wrong.
Thanks

Re: [Outreachy kernel] [PATCH] Staging: rts5208: sd: Remove redundant code

[Arnd Bergmann]

On Wednesday 14 October 2015 15:46:09 Shivani Bhardwaj wrote:
> On Wed, Oct 14, 2015 at 2:54 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Wednesday 14 October 2015 05:15:12 Shivani Bhardwaj wrote:
> >> On Wed, Oct 14, 2015 at 5:09 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> >> > On Wed, Oct 14, 2015 at 04:54:45AM +0530, Shivani Bhardwaj wrote:
> >> I don't think it should be a problem because that is what we
> >> ultimately want in our code - to clear the memory card subject to
> >> constraints that one of the two if conditions is wrong. Now, if both
> >> the if statements, that is,
> >> if (hi_cap_flow) and if (rsp[1] & 0x40)
> >>
> >> turn out to be true, they set the cleared sd_card again.
> >>
> >> That is what we wanted. If both the if statements are true - set the
> >> sd_card else clear it up.
> >
> > I guess this would be easier to verify if it were not obscured by the
> > SET_SD_HCXC/CLR_SD_HCXC macros 
> >
> > Your change looks like a correct transformation, and it would be logical
> > to remove the support_1v8 variable next, as it can never be true.
> >
> > However, I wonder whether that is just a bug in the driver, as UHS-1
> > cards are required to support 1.8v operation, and there is clearly
> > code to handle it here. Maybe one of them should actually say
> > "support_1v8 = true".
> >
> >         Arnd
> 
> I do not exactly understand the role of variables here. But, as far as
> I can guess (after having checked the secure digital)  support_1v8
> should be true for hi_cap_flow=false.

Yes, that seems like a good guess, but it's impossible to know if that
is correct without having a look at some datasheet or specification.

> That is, if capacity flow is
> less, we can allow voltage to be 1.8V otherwise it should be 3.3V.
> However, I really do not understand what the array rsp is for. So, I'm
> not sure what its status should be.
> Please correct me if I am wrong.

'rsp' here is the response from this command:

         retval = sd_send_cmd_get_rsp(chip, SD_APP_OP_COND, voltage,
                                      SD_RSP_TYPE_R3, rsp, 5);


This is a standard SD card command (ACMD41) that is documented
in the SD card "Physical Layer Simplified Specification" version 4.10.

I have not looked any further, but the answer is probably in there.

	Arnd