Re: [bug report] staging: mt7621-mmc: MIPS: ralink: add sdhci for mt7620a SoC
On Fri, Apr 06, 2018 at 10:09:54AM +0300, Dan Carpenter wrote: > It feels sort of rude talking about Greg like a machine, but to me > having a very predictable maintainer is a fantastic thing. I try to be > machine-like when I am reviewing patches because it removes a lot of the > questions about "am I being too mean to someone?". Doesn't bother me at all! :) greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [bug report] staging: mt7621-mmc: MIPS: ralink: add sdhci for mt7620a SoC
On Thu, Apr 05, 2018 at 06:39:47PM +0200, Christian Lütke-Stetzkamp wrote: > > Hi Dan, > > thanks for the report, I have created patches for all of these and > will send them, once my previous series got merged, because I based > them on that status. I would just be optimistic and send them now. Greg applies things in first come first serve basis. He's likely to apply your series, right? The worst that can happen is that there is a conflict and Greg asks to rebase your patches. Greg doesn't invest any time into figuring out why your patch doesn't apply so it's not a huge burden. It feels sort of rude talking about Greg like a machine, but to me having a very predictable maintainer is a fantastic thing. I try to be machine-like when I am reviewing patches because it removes a lot of the questions about "am I being too mean to someone?". regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [bug report] staging: mt7621-mmc: MIPS: ralink: add sdhci for mt7620a SoC
On Thu, Apr 05, 2018 at 02:56:54PM +0300, Dan Carpenter wrote: > Hello John Crispin, > > The patch 8b634a9c7620: "staging: mt7621-mmc: MIPS: ralink: add sdhci > for mt7620a SoC" from Mar 15, 2018, leads to the following static > checker warning: > > drivers/staging/mt7621-mmc/sd.c:2790 msdc_drv_probe() > warn: curly braces intended? Hi Dan, in my understanding of the code around that place, these lines are only indented by accident. The card detect low/high selection and the activation of software poll should happen even when the SDIO(_EXT) interrupt is not enabled, because they are not related. The indentation will be fixed, once the patch series I sent yesterday in version 2 gets merged. Hopefully this answer is helpful, Christian ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [bug report] staging: mt7621-mmc: MIPS: ralink: add sdhci for mt7620a SoC
On Thu, Apr 05, 2018 at 03:13:10PM +0300, Dan Carpenter wrote: > [ I just decided to forward you guys all the Smatch warnings. -dan ] > > Hello John Crispin, > > The patch 8b634a9c7620: "staging: mt7621-mmc: MIPS: ralink: add sdhci > for mt7620a SoC" from Mar 15, 2018, leads to the following static > checker warning: > > drivers/staging/mt7621-mmc/sd.c:951 msdc_command_start() > warn: we tested 'opcode == 3' before and it was 'false' > > drivers/staging/mt7621-mmc/sd.c:2961 msdc_drv_suspend() > warn: variable dereferenced before check 'mmc' (see line 2959) > > drivers/staging/mt7621-mmc/sd.c:2976 msdc_drv_resume() > warn: variable dereferenced before check 'mmc' (see line 2972) > > drivers/staging/mt7621-mmc/dbg.c:270 msdc_debug_proc_write() > warn: copy_to/from_user() returns a positive value > > drivers/staging/mt7621-mmc/dbg.c:339 msdc_debug_proc_init() > warn: proc file '"msdc_debug"' is world writable > > drivers/staging/mt7621-mmc/dbg.c:341 msdc_debug_proc_init() > warn: 'de' isn't an ERR_PTR Hi Dan, thanks for the report, I have created patches for all of these and will send them, once my previous series got merged, because I based them on that status. Christian ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [bug report] staging: mt7621-mmc: MIPS: ralink: add sdhci for mt7620a SoC
Hi Dan,
I explained why I think this should not be merged, i do not plan to fix
any issues, please send all maintenance requests towards NeilBrown.
And .. it was merged it with an out dated defunct mail addr of mine.
John
On 05/04/18 14:13, Dan Carpenter wrote:
[ I just decided to forward you guys all the Smatch warnings. -dan ]
Hello John Crispin,
The patch 8b634a9c7620: "staging: mt7621-mmc: MIPS: ralink: add sdhci
for mt7620a SoC" from Mar 15, 2018, leads to the following static
checker warning:
drivers/staging/mt7621-mmc/sd.c:951 msdc_command_start()
warn: we tested 'opcode == 3' before and it was 'false'
drivers/staging/mt7621-mmc/sd.c
931 static unsigned int msdc_command_start(struct msdc_host *host,
932struct mmc_command *cmd,
933int tune, /*
not used */
934unsigned long timeout)
935 {
936 u32 base = host->base;
937 u32 opcode = cmd->opcode;
938 u32 rawcmd;
939 u32 wints = MSDC_INT_CMDRDY | MSDC_INT_RSPCRCERR |
MSDC_INT_CMDTMO |
940 MSDC_INT_ACMDRDY | MSDC_INT_ACMDCRCERR |
MSDC_INT_ACMDTMO |
941 MSDC_INT_ACMD19_DONE;
942
943 u32 resp;
944 unsigned long tmo;
945
946 /* Protocol layer does not provide response type, but our hardware
needs
947 * to know exact type, not just size!
948 */
949 if (opcode == MMC_SEND_OP_COND || opcode == SD_APP_OP_COND)
950 resp = RESP_R3;
951 else if (opcode == MMC_SET_RELATIVE_ADDR || opcode ==
SD_SEND_RELATIVE_ADDR)
^^
MMC_SET_RELATIVE_ADDR and SD_SEND_RELATIVE_ADDR are both 3 so this
is redundant.
952 resp = (mmc_cmd_type(cmd) == MMC_CMD_BCR) ? RESP_R6 : RESP_R1;
953 else if (opcode == MMC_FAST_IO)
954 resp = RESP_R4;
955 else if (opcode == MMC_GO_IRQ_STATE)
956 resp = RESP_R5;
957 else if (opcode == MMC_SELECT_CARD)
958 resp = (cmd->arg != 0) ? RESP_R1B : RESP_NONE;
959 else if (opcode == SD_IO_RW_DIRECT || opcode == SD_IO_RW_EXTENDED)
960 resp = RESP_R1; /* SDIO workaround. */
961 else if (opcode == SD_SEND_IF_COND && (mmc_cmd_type(cmd) ==
MMC_CMD_BCR))
962 resp = RESP_R1;
963 else {
964 switch (mmc_resp_type(cmd)) {
drivers/staging/mt7621-mmc/sd.c:2961 msdc_drv_suspend()
warn: variable dereferenced before check 'mmc' (see line 2959)
drivers/staging/mt7621-mmc/sd.c:2976 msdc_drv_resume()
warn: variable dereferenced before check 'mmc' (see line 2972)
drivers/staging/mt7621-mmc/sd.c
2953 /* Fix me: Power Flow */
2954 #ifdef CONFIG_PM
2955 static int msdc_drv_suspend(struct platform_device *pdev, pm_message_t
state)
2956 {
2957 int ret = 0;
2958 struct mmc_host *mmc = platform_get_drvdata(pdev);
2959 struct msdc_host *host = mmc_priv(mmc);
^
Dereference
2960
2961 if (mmc && state.event == PM_EVENT_SUSPEND && (host->hw->flags &
MSDC_SYS_SUSPEND)) { /* will set for card */
^^^
Check
2962 msdc_pm(state, (void*)host);
2963 }
2964
2965 return ret;
2966 }
2967
2968 static int msdc_drv_resume(struct platform_device *pdev)
2969 {
2970 int ret = 0;
2971 struct mmc_host *mmc = platform_get_drvdata(pdev);
2972 struct msdc_host *host = mmc_priv(mmc);
Dereference
2973 struct pm_message state;
2974
2975 state.event = PM_EVENT_RESUME;
2976 if (mmc && (host->hw->flags & MSDC_SYS_SUSPEND)) {/* will set for
card */
^^^
Check
2977 msdc_pm(state, (void*)host);
2978 }
2979
2980 /* This mean WIFI not controller by PM */
2981
2982 return ret;
2983 }
drivers/staging/mt7621-mmc/dbg.c:270 msdc_debug_proc_write()
warn: copy_to/from_user() returns a positive value
drivers/staging/mt7621-mmc/dbg.c
257 static ssize_t msdc_debug_proc_write(struct file *file,
258 const char __user *buf, size_t count, loff_t
*data)
259 {
260 int ret;
261
262 int cmd, p1, p2;
263 int id, zone;
264 int mode, size;
265
266 if (count == 0)return -1;
267 if(count > 255)count = 255;
268
269 ret = copy_from_user(cmd_buf, buf, count);
270 if (ret < 0)return -1;
This should be:
if (copy_from_user(cmd_buf, buf, count))
return -EFAULT;
271
272
Re: [bug report] staging: mt7621-mmc: MIPS: ralink: add sdhci for mt7620a SoC
Hi Dan,
I explained why I think this should not be merged, i do not plan to fix
any issues, please send all maintenance requests towards NeilBrown.
And .. it was merged it with an out dated defunct mail addr of mine.
John
On 05/04/18 13:56, Dan Carpenter wrote:
Hello John Crispin,
The patch 8b634a9c7620: "staging: mt7621-mmc: MIPS: ralink: add sdhci
for mt7620a SoC" from Mar 15, 2018, leads to the following static
checker warning:
drivers/staging/mt7621-mmc/sd.c:2790 msdc_drv_probe()
warn: curly braces intended?
drivers/staging/mt7621-mmc/sd.c
2777 /* For sd card: MSDC_SYS_SUSPEND | MSDC_WP_PIN_EN | MSDC_CD_PIN_EN
| MSDC_REMOVABLE | MSDC_HIGHSPEED,
2778 For sdio : MSDC_EXT_SDIO_IRQ | MSDC_HIGHSPEED */
2779 if (hw->flags & MSDC_HIGHSPEED) {
2780 mmc->caps = MMC_CAP_MMC_HIGHSPEED | MMC_CAP_SD_HIGHSPEED;
2781 }
2782 if (hw->data_pins == 4) { /* current data_pins are all 4*/
2783 mmc->caps |= MMC_CAP_4_BIT_DATA;
2784 } else if (hw->data_pins == 8) {
2785 mmc->caps |= MMC_CAP_8_BIT_DATA;
2786 }
2787 if ((hw->flags & MSDC_SDIO_IRQ) || (hw->flags & MSDC_EXT_SDIO_IRQ))
Are curly braces intended for this if statement?
2788 mmc->caps |= MMC_CAP_SDIO_IRQ; /* yes for sdio */
2789
2790 cd_active_low = !of_property_read_bool(pdev->dev.of_node,
"mediatek,cd-high");
2791 mtk_sw_poll = of_property_read_bool(pdev->dev.of_node,
"mediatek,cd-poll");
2792
2793 if (mtk_sw_poll)
2794 mmc->caps |= MMC_CAP_NEEDS_POLL;
because the indenting seems to say that the braces should reach up to
here.
2795
2796 /* MMC core transfer sizes tunable parameters */
2797 #if LINUX_VERSION_CODE > KERNEL_VERSION(3,10,0)
2798 mmc->max_segs = MAX_HW_SGMTS;
2799 #else
2800 mmc->max_hw_segs = MAX_HW_SGMTS;
2801 mmc->max_phys_segs = MAX_PHY_SGMTS;
2802 #endif
regards,
dan carpenter
___
Linux-mediatek mailing list
linux-media...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
