[PATCH] rts5208: add a missing check for the status of command sending

public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed

* [PATCH] rts5208: add a missing check for the status of command sending
@ 2018-12-20  7:57 Kangjie Lu
  2018-12-20  9:09 ` Dan Carpenter
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Kangjie Lu @ 2018-12-20  7:57 UTC (permalink / raw)
  To: kjlu
  Cc: pakki001, Greg Kroah-Hartman, Dan Carpenter, Kees Cook,
	Arnd Bergmann, Aymen Qader, devel, linux-kernel

ms_send_cmd() may fail. The fix checks the return value of it, and if it
fails, returns the error "STATUS_FAIL" upstream.

Signed-off-by: Kangjie Lu <kjlu@umn.edu>
---
 drivers/staging/rts5208/ms.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/rts5208/ms.c b/drivers/staging/rts5208/ms.c
index f53adf15c685..5a9e562465e9 100644
--- a/drivers/staging/rts5208/ms.c
+++ b/drivers/staging/rts5208/ms.c
@@ -2731,7 +2731,9 @@ static int mspro_rw_multi_sector(struct scsi_cmnd *srb,
 		}
 
 		if (val & MS_INT_BREQ)
-			ms_send_cmd(chip, PRO_STOP, WAIT_INT);
+			retval = ms_send_cmd(chip, PRO_STOP, WAIT_INT);
+			if (retval != STATUS_SUCCESS)
+				return STATUS_FAIL;
 
 		if (val & (MS_CRC16_ERR | MS_RDY_TIMEOUT)) {
 			dev_dbg(rtsx_dev(chip), "MSPro CRC error, tune clock!\n");
-- 
2.17.2 (Apple Git-113)


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] rts5208: add a missing check for the status of command sending
  2018-12-20  7:57 [PATCH] rts5208: add a missing check for the status of command sending Kangjie Lu
@ 2018-12-20  9:09 ` Dan Carpenter
  2018-12-20  9:27 ` Dan Carpenter
  2018-12-20 17:31 ` kbuild test robot
  2 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2018-12-20  9:09 UTC (permalink / raw)
  To: Kangjie Lu
  Cc: pakki001, Greg Kroah-Hartman, Kees Cook, Arnd Bergmann,
	Aymen Qader, devel, linux-kernel

On Thu, Dec 20, 2018 at 01:57:01AM -0600, Kangjie Lu wrote:
> ms_send_cmd() may fail. The fix checks the return value of it, and if it
> fails, returns the error "STATUS_FAIL" upstream.
> 
> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
> ---
>  drivers/staging/rts5208/ms.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/rts5208/ms.c b/drivers/staging/rts5208/ms.c
> index f53adf15c685..5a9e562465e9 100644
> --- a/drivers/staging/rts5208/ms.c
> +++ b/drivers/staging/rts5208/ms.c
> @@ -2731,7 +2731,9 @@ static int mspro_rw_multi_sector(struct scsi_cmnd *srb,
>  		}
>  
>  		if (val & MS_INT_BREQ)

You would need to add a curly brace here.


> -			ms_send_cmd(chip, PRO_STOP, WAIT_INT);
> +			retval = ms_send_cmd(chip, PRO_STOP, WAIT_INT);
> +			if (retval != STATUS_SUCCESS)
> +				return STATUS_FAIL;

You can't overwrite "retval".  We already had an error code save there
but now you're changing to STATUS_SUCCESS.

Anyway, I think the original error handling is probably correct and we
should leave it as-is.  We're already trying to handle an error
situation by resetting stuff.  Then if we get another error while we
take these extreme measures to recover, we shouldn't give up.  We should
just keep on trying to reset instead of returning early.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] rts5208: add a missing check for the status of command sending
  2018-12-20  7:57 [PATCH] rts5208: add a missing check for the status of command sending Kangjie Lu
  2018-12-20  9:09 ` Dan Carpenter
@ 2018-12-20  9:27 ` Dan Carpenter
  2018-12-20 17:31 ` kbuild test robot
  2 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2018-12-20  9:27 UTC (permalink / raw)
  To: Kangjie Lu
  Cc: devel, Kees Cook, Arnd Bergmann, Greg Kroah-Hartman, linux-kernel,
	pakki001, Aymen Qader

I think maybe this is the first kernel patch I have recieved from you.

When you're adding error handling there are a couple ways to go wrong
and this is what I look at when I review error handling patches:
1) The error handling is not required.
2) The error handling is not complete.

I have messed up on both of these in my own patches because sometimes
the code is complicated to understand.  Sometimes there isn't any way
to recover from errors.  For example, we sometimes deliberately assume
that the PCI bus is working because if it's not the real fix is to buy
new hardware.

Another thing that helps is to try write about the real world impact
about the patch in the changelog.  Try ask, why has that bug not been
found before?  Always take a look at how the function is called and the
wider context.

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] rts5208: add a missing check for the status of command sending
  2018-12-20  7:57 [PATCH] rts5208: add a missing check for the status of command sending Kangjie Lu
  2018-12-20  9:09 ` Dan Carpenter
  2018-12-20  9:27 ` Dan Carpenter
@ 2018-12-20 17:31 ` kbuild test robot
  2 siblings, 0 replies; 4+ messages in thread
From: kbuild test robot @ 2018-12-20 17:31 UTC (permalink / raw)
  To: Kangjie Lu
  Cc: kbuild-all, kjlu, devel, Kees Cook, Arnd Bergmann,
	Greg Kroah-Hartman, linux-kernel, pakki001, Aymen Qader,
	Dan Carpenter

[-- Attachment #1: Type: text/plain, Size: 11421 bytes --]

Hi Kangjie,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on staging/staging-testing]
[also build test WARNING on v4.20-rc7 next-20181220]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Kangjie-Lu/rts5208-add-a-missing-check-for-the-status-of-command-sending/20181221-004836
config: i386-randconfig-x005-201850 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   drivers/staging//rts5208/ms.c: In function 'mspro_rw_multi_sector':
>> drivers/staging//rts5208/ms.c:2722:3: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
      if (val & MS_INT_BREQ)
      ^~
   drivers/staging//rts5208/ms.c:2724:4: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if'
       if (retval != STATUS_SUCCESS)
       ^~

vim +/if +2722 drivers/staging//rts5208/ms.c

fa590c222f Micky Ching     2013-11-12  2605  
fa590c222f Micky Ching     2013-11-12  2606  static int mspro_rw_multi_sector(struct scsi_cmnd *srb,
fa590c222f Micky Ching     2013-11-12  2607  				 struct rtsx_chip *chip, u32 start_sector,
fa590c222f Micky Ching     2013-11-12  2608  				 u16 sector_cnt)
fa590c222f Micky Ching     2013-11-12  2609  {
d1af6476e6 Dilek Uzulmez   2016-02-23  2610  	struct ms_info *ms_card = &chip->ms_card;
11201769d1 Quentin Lambert 2015-03-04  2611  	bool mode_2k = false;
11201769d1 Quentin Lambert 2015-03-04  2612  	int retval;
fa590c222f Micky Ching     2013-11-12  2613  	u16 count;
fa590c222f Micky Ching     2013-11-12  2614  	u8 val, trans_mode, rw_tpc, rw_cmd;
fa590c222f Micky Ching     2013-11-12  2615  
fa590c222f Micky Ching     2013-11-12  2616  	ms_set_err_code(chip, MS_NO_ERROR);
fa590c222f Micky Ching     2013-11-12  2617  
fa590c222f Micky Ching     2013-11-12  2618  	ms_card->cleanup_counter = 0;
fa590c222f Micky Ching     2013-11-12  2619  
fa590c222f Micky Ching     2013-11-12  2620  	if (CHK_MSHG(ms_card)) {
fa590c222f Micky Ching     2013-11-12  2621  		if ((start_sector % 4) || (sector_cnt % 4)) {
fa590c222f Micky Ching     2013-11-12  2622  			if (srb->sc_data_direction == DMA_FROM_DEVICE) {
fa590c222f Micky Ching     2013-11-12  2623  				rw_tpc = PRO_READ_LONG_DATA;
fa590c222f Micky Ching     2013-11-12  2624  				rw_cmd = PRO_READ_DATA;
fa590c222f Micky Ching     2013-11-12  2625  			} else {
fa590c222f Micky Ching     2013-11-12  2626  				rw_tpc = PRO_WRITE_LONG_DATA;
fa590c222f Micky Ching     2013-11-12  2627  				rw_cmd = PRO_WRITE_DATA;
fa590c222f Micky Ching     2013-11-12  2628  			}
fa590c222f Micky Ching     2013-11-12  2629  		} else {
fa590c222f Micky Ching     2013-11-12  2630  			if (srb->sc_data_direction == DMA_FROM_DEVICE) {
fa590c222f Micky Ching     2013-11-12  2631  				rw_tpc = PRO_READ_QUAD_DATA;
fa590c222f Micky Ching     2013-11-12  2632  				rw_cmd = PRO_READ_2K_DATA;
fa590c222f Micky Ching     2013-11-12  2633  			} else {
fa590c222f Micky Ching     2013-11-12  2634  				rw_tpc = PRO_WRITE_QUAD_DATA;
fa590c222f Micky Ching     2013-11-12  2635  				rw_cmd = PRO_WRITE_2K_DATA;
fa590c222f Micky Ching     2013-11-12  2636  			}
11201769d1 Quentin Lambert 2015-03-04  2637  			mode_2k = true;
fa590c222f Micky Ching     2013-11-12  2638  		}
fa590c222f Micky Ching     2013-11-12  2639  	} else {
fa590c222f Micky Ching     2013-11-12  2640  		if (srb->sc_data_direction == DMA_FROM_DEVICE) {
fa590c222f Micky Ching     2013-11-12  2641  			rw_tpc = PRO_READ_LONG_DATA;
fa590c222f Micky Ching     2013-11-12  2642  			rw_cmd = PRO_READ_DATA;
fa590c222f Micky Ching     2013-11-12  2643  		} else {
fa590c222f Micky Ching     2013-11-12  2644  			rw_tpc = PRO_WRITE_LONG_DATA;
fa590c222f Micky Ching     2013-11-12  2645  			rw_cmd = PRO_WRITE_DATA;
fa590c222f Micky Ching     2013-11-12  2646  		}
fa590c222f Micky Ching     2013-11-12  2647  	}
fa590c222f Micky Ching     2013-11-12  2648  
fa590c222f Micky Ching     2013-11-12  2649  	retval = ms_switch_clock(chip);
9f902b495b Aymen Qader     2018-09-20  2650  	if (retval != STATUS_SUCCESS)
031366ea65 Joe Perches     2015-03-25  2651  		return STATUS_FAIL;
fa590c222f Micky Ching     2013-11-12  2652  
fa590c222f Micky Ching     2013-11-12  2653  	if (srb->sc_data_direction == DMA_FROM_DEVICE)
fa590c222f Micky Ching     2013-11-12  2654  		trans_mode = MS_TM_AUTO_READ;
fa590c222f Micky Ching     2013-11-12  2655  	else
fa590c222f Micky Ching     2013-11-12  2656  		trans_mode = MS_TM_AUTO_WRITE;
fa590c222f Micky Ching     2013-11-12  2657  
8ee775f92c Joe Perches     2015-03-25  2658  	retval = rtsx_read_register(chip, MS_TRANS_CFG, &val);
9f902b495b Aymen Qader     2018-09-20  2659  	if (retval)
8ee775f92c Joe Perches     2015-03-25  2660  		return retval;
fa590c222f Micky Ching     2013-11-12  2661  
fa590c222f Micky Ching     2013-11-12  2662  	if (ms_card->seq_mode) {
25ccf0b0a7 Wayne Porter    2016-10-11  2663  		if ((ms_card->pre_dir != srb->sc_data_direction) ||
25ccf0b0a7 Wayne Porter    2016-10-11  2664  		    ((ms_card->pre_sec_addr + ms_card->pre_sec_cnt) !=
25ccf0b0a7 Wayne Porter    2016-10-11  2665  		     start_sector) ||
25ccf0b0a7 Wayne Porter    2016-10-11  2666  		    (mode_2k && (ms_card->seq_mode & MODE_512_SEQ)) ||
25ccf0b0a7 Wayne Porter    2016-10-11  2667  		    (!mode_2k && (ms_card->seq_mode & MODE_2K_SEQ)) ||
25ccf0b0a7 Wayne Porter    2016-10-11  2668  		    !(val & MS_INT_BREQ) ||
25ccf0b0a7 Wayne Porter    2016-10-11  2669  		    ((ms_card->total_sec_cnt + sector_cnt) > 0xFE00)) {
fa590c222f Micky Ching     2013-11-12  2670  			ms_card->seq_mode = 0;
fa590c222f Micky Ching     2013-11-12  2671  			ms_card->total_sec_cnt = 0;
fa590c222f Micky Ching     2013-11-12  2672  			if (val & MS_INT_BREQ) {
fa590c222f Micky Ching     2013-11-12  2673  				retval = ms_send_cmd(chip, PRO_STOP, WAIT_INT);
9f902b495b Aymen Qader     2018-09-20  2674  				if (retval != STATUS_SUCCESS)
031366ea65 Joe Perches     2015-03-25  2675  					return STATUS_FAIL;
fa590c222f Micky Ching     2013-11-12  2676  
25ccf0b0a7 Wayne Porter    2016-10-11  2677  				rtsx_write_register(chip, RBCTL, RB_FLUSH,
25ccf0b0a7 Wayne Porter    2016-10-11  2678  						    RB_FLUSH);
fa590c222f Micky Ching     2013-11-12  2679  			}
fa590c222f Micky Ching     2013-11-12  2680  		}
fa590c222f Micky Ching     2013-11-12  2681  	}
fa590c222f Micky Ching     2013-11-12  2682  
fa590c222f Micky Ching     2013-11-12  2683  	if (!ms_card->seq_mode) {
fa590c222f Micky Ching     2013-11-12  2684  		ms_card->total_sec_cnt = 0;
fa590c222f Micky Ching     2013-11-12  2685  		if (sector_cnt >= SEQ_START_CRITERIA) {
fa590c222f Micky Ching     2013-11-12  2686  			if ((ms_card->capacity - start_sector) > 0xFE00)
fa590c222f Micky Ching     2013-11-12  2687  				count = 0xFE00;
fa590c222f Micky Ching     2013-11-12  2688  			else
fa590c222f Micky Ching     2013-11-12  2689  				count = (u16)(ms_card->capacity - start_sector);
fa590c222f Micky Ching     2013-11-12  2690  
fa590c222f Micky Ching     2013-11-12  2691  			if (count > sector_cnt) {
fa590c222f Micky Ching     2013-11-12  2692  				if (mode_2k)
b0ef3ed48e Jiayi Ye        2014-10-20  2693  					ms_card->seq_mode = MODE_2K_SEQ;
fa590c222f Micky Ching     2013-11-12  2694  				else
b0ef3ed48e Jiayi Ye        2014-10-20  2695  					ms_card->seq_mode = MODE_512_SEQ;
fa590c222f Micky Ching     2013-11-12  2696  			}
fa590c222f Micky Ching     2013-11-12  2697  		} else {
fa590c222f Micky Ching     2013-11-12  2698  			count = sector_cnt;
fa590c222f Micky Ching     2013-11-12  2699  		}
fa590c222f Micky Ching     2013-11-12  2700  		retval = mspro_set_rw_cmd(chip, start_sector, count, rw_cmd);
fa590c222f Micky Ching     2013-11-12  2701  		if (retval != STATUS_SUCCESS) {
fa590c222f Micky Ching     2013-11-12  2702  			ms_card->seq_mode = 0;
031366ea65 Joe Perches     2015-03-25  2703  			return STATUS_FAIL;
fa590c222f Micky Ching     2013-11-12  2704  		}
fa590c222f Micky Ching     2013-11-12  2705  	}
fa590c222f Micky Ching     2013-11-12  2706  
fa590c222f Micky Ching     2013-11-12  2707  	retval = ms_transfer_data(chip, trans_mode, rw_tpc, sector_cnt,
fa590c222f Micky Ching     2013-11-12  2708  				  WAIT_INT, mode_2k, scsi_sg_count(srb),
fa590c222f Micky Ching     2013-11-12  2709  				  scsi_sglist(srb), scsi_bufflen(srb));
fa590c222f Micky Ching     2013-11-12  2710  	if (retval != STATUS_SUCCESS) {
fa590c222f Micky Ching     2013-11-12  2711  		ms_card->seq_mode = 0;
fa590c222f Micky Ching     2013-11-12  2712  		rtsx_read_register(chip, MS_TRANS_CFG, &val);
fa590c222f Micky Ching     2013-11-12  2713  		rtsx_clear_ms_error(chip);
fa590c222f Micky Ching     2013-11-12  2714  
fa590c222f Micky Ching     2013-11-12  2715  		if (detect_card_cd(chip, MS_CARD) != STATUS_SUCCESS) {
fa590c222f Micky Ching     2013-11-12  2716  			chip->rw_need_retry = 0;
2d77259135 Gaurav Pathak   2017-07-20  2717  			dev_dbg(rtsx_dev(chip), "No card exist, exit %s\n",
2d77259135 Gaurav Pathak   2017-07-20  2718  				__func__);
031366ea65 Joe Perches     2015-03-25  2719  			return STATUS_FAIL;
fa590c222f Micky Ching     2013-11-12  2720  		}
fa590c222f Micky Ching     2013-11-12  2721  
fa590c222f Micky Ching     2013-11-12 @2722  		if (val & MS_INT_BREQ)
e7cf02e967 Kangjie Lu      2018-12-20  2723  			retval = ms_send_cmd(chip, PRO_STOP, WAIT_INT);
e7cf02e967 Kangjie Lu      2018-12-20  2724  			if (retval != STATUS_SUCCESS)
e7cf02e967 Kangjie Lu      2018-12-20  2725  				return STATUS_FAIL;
fa590c222f Micky Ching     2013-11-12  2726  
fa590c222f Micky Ching     2013-11-12  2727  		if (val & (MS_CRC16_ERR | MS_RDY_TIMEOUT)) {
bf6c0d110e Fabio Falzoi    2014-07-30  2728  			dev_dbg(rtsx_dev(chip), "MSPro CRC error, tune clock!\n");
fa590c222f Micky Ching     2013-11-12  2729  			chip->rw_need_retry = 1;
fa590c222f Micky Ching     2013-11-12  2730  			ms_auto_tune_clock(chip);
fa590c222f Micky Ching     2013-11-12  2731  		}
fa590c222f Micky Ching     2013-11-12  2732  
031366ea65 Joe Perches     2015-03-25  2733  		return retval;
fa590c222f Micky Ching     2013-11-12  2734  	}
fa590c222f Micky Ching     2013-11-12  2735  
fa590c222f Micky Ching     2013-11-12  2736  	if (ms_card->seq_mode) {
fa590c222f Micky Ching     2013-11-12  2737  		ms_card->pre_sec_addr = start_sector;
fa590c222f Micky Ching     2013-11-12  2738  		ms_card->pre_sec_cnt = sector_cnt;
fa590c222f Micky Ching     2013-11-12  2739  		ms_card->pre_dir = srb->sc_data_direction;
fa590c222f Micky Ching     2013-11-12  2740  		ms_card->total_sec_cnt += sector_cnt;
fa590c222f Micky Ching     2013-11-12  2741  	}
fa590c222f Micky Ching     2013-11-12  2742  
fa590c222f Micky Ching     2013-11-12  2743  	return STATUS_SUCCESS;
fa590c222f Micky Ching     2013-11-12  2744  }
fa590c222f Micky Ching     2013-11-12  2745  

:::::: The code at line 2722 was first introduced by commit
:::::: fa590c222fbaa428edb2ce2194638906cea1400a staging: rts5208: add support for rts5208 and rts5288

:::::: TO: Micky Ching <micky_ching@realsil.com.cn>
:::::: CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30279 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2018-12-20 17:32 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-12-20  7:57 [PATCH] rts5208: add a missing check for the status of command sending Kangjie Lu
2018-12-20  9:09 ` Dan Carpenter
2018-12-20  9:27 ` Dan Carpenter
2018-12-20 17:31 ` kbuild test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox