linux-kernel - Re: [PATCH] staging:rts

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]

Date:	Sun, 9 Oct 2011 16:57:45 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	wwang <wei_wang@...lsil.com.cn>
Cc:	"devel@...uxdriverproject.org" <devel@...uxdriverproject.org>,
	"gregkh@...e.de" <gregkh@...e.de>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] staging:rts_pstor:Fix SDIO issue

What I'm saying is, it's confusing to add another variable which is
only subtly different from retval.  Here is what it looks like after
we apply the patch.

	int reset_pass = 0;

	/* skip 45 lines */

	retval = sd_change_bank_voltage(chip, SD_IO_3V3);
	if (retval != STATUS_SUCCESS) {
		TRACE_RET(chip, STATUS_FAIL);
	}

	/* skip 30 lines */

	if (!reset_pass)
		TRACE_RET(chip, STATUS_FAIL);

The reviewer would have to read through 80 lines of code to find that
we don't care about the return value from sd_change_bank_voltage().
Don't do it that way.

Here is how I would write what it.  I can't actually test this since
I don't have the hardware...

diff --git a/drivers/staging/rts_pstor/sd.c b/drivers/staging/rts_pstor/sd.c
index fb62eaf..46bf0e1 100644
--- a/drivers/staging/rts_pstor/sd.c
+++ b/drivers/staging/rts_pstor/sd.c
@@ -3134,39 +3134,36 @@ int reset_sd_card(struct rtsx_chip *chip)
 
 	if (chip->sd_ctl & RESET_MMC_FIRST) {
 		retval = reset_mmc(chip);
-		if ((retval != STATUS_SUCCESS) && !sd_check_err_code(chip, SD_NO_CARD)) {
+		if (retval != STATUS_SUCCESS) {
+			if (sd_check_err_code(chip, SD_NO_CARD))
+				TRACE_RET(chip, STATUS_FAIL);
+
 			retval = reset_sd(chip);
 			if (retval != STATUS_SUCCESS) {
-				if (CHECK_PID(chip, 0x5209)) {
-					retval = sd_change_bank_voltage(chip, SD_IO_3V3);
-					if (retval != STATUS_SUCCESS) {
-						TRACE_RET(chip, STATUS_FAIL);
-					}
-				}
+				if (CHECK_PID(chip, 0x5209))
+					sd_change_bank_voltage(chip, SD_IO_3V3);
+				TRACE_RET(chip, STATUS_FAIL);
 			}
 		}
 	} else {
 		retval = reset_sd(chip);
 		if (retval != STATUS_SUCCESS) {
-			if (sd_check_err_code(chip, SD_NO_CARD)) {
+			if (sd_check_err_code(chip, SD_NO_CARD))
 				TRACE_RET(chip, STATUS_FAIL);
-			}
 
 			if (CHECK_PID(chip, 0x5209)) {
 				retval = sd_change_bank_voltage(chip, SD_IO_3V3);
-				if (retval != STATUS_SUCCESS) {
+				if (retval != STATUS_SUCCESS)
 					TRACE_RET(chip, STATUS_FAIL);
-				}
 			}
 
-			if (!chip->sd_io) {
-				retval = reset_mmc(chip);
-			}
-		}
-	}
+			if (chip->sd_io)
+				TRACE_RET(chip, STATUS_FAIL);
 
-	if (retval != STATUS_SUCCESS) {
-		TRACE_RET(chip, STATUS_FAIL);
+			retval = reset_mmc(chip);
+			if (retval != STATUS_SUCCESS)
+				TRACE_RET(chip, STATUS_FAIL);
+		}
 	}
 
 	retval = sd_set_clock_divider(chip, SD_CLK_DIVIDE_0);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/