* [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;
as well as URLs for NNTP newsgroup(s).