| Message ID | 1461171092-30631-2-git-send-email-aisheng.dong@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 20 April 2016 at 18:51, Dong Aisheng <aisheng.dong@nxp.com> wrote: > mmc_select_hs200() and mmc_select_hs() will keep the timing > as before if switch fails. So it's meaningless to print the > failed switched mode outside based on the current host timing. > > Furthermore, the original print is wrong, it should be: > pr_warn("%s: switch to %s failed\n", > mmc_hostname(card->host), > mmc_card_hs(card) ? "high-speed" : > (mmc_card_hs200(card) ? "hs200" : "")); > > Since we already have error message in mmc_select_hs200(), > simply remove it outside. > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > --- > drivers/mmc/core/mmc.c | 10 +--------- > 1 file changed, 1 insertion(+), 9 deletions(-) > > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c > index f6683a9..55c8201 100644 > --- a/drivers/mmc/core/mmc.c > +++ b/drivers/mmc/core/mmc.c > @@ -1321,21 +1321,13 @@ static int mmc_select_timing(struct mmc_card *card) > if (err && err != -EBADMSG) > return err; > > - if (err) { > - pr_warn("%s: switch to %s failed\n", > - mmc_card_hs(card) ? "high-speed" : > - (mmc_card_hs200(card) ? "hs200" : ""), > - mmc_hostname(card->host)); > - err = 0; > - } > - No, this is not correct. First, in patch1/3, you change to return an error code immediately when __mmc_set_signal_voltage() fails, instead of using the "goto err". Thus there will be no error printed for this case any more. Second, mmc_select_hs() may fail and there is no print done within that function which is why above print is also needed. On the other hand I agree with you, it doesn't seems necessary to print two messages when mmc_select_hs200() fails. One should be enough. Can we change this so *only* mmc_select_timing() will deal with the print at all errors? > bus_speed: > /* > * Set the bus speed to the selected bus timing. > * If timing is not selected, backward compatible is the default. > */ > mmc_set_bus_speed(card); > - return err; > + return 0; > } > > /* > -- > 1.9.1 > Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 28, 2016 at 11:24:07AM +0200, Ulf Hansson wrote: > On 20 April 2016 at 18:51, Dong Aisheng <aisheng.dong@nxp.com> wrote: > > mmc_select_hs200() and mmc_select_hs() will keep the timing > > as before if switch fails. So it's meaningless to print the > > failed switched mode outside based on the current host timing. > > > > Furthermore, the original print is wrong, it should be: > > pr_warn("%s: switch to %s failed\n", > > mmc_hostname(card->host), > > mmc_card_hs(card) ? "high-speed" : > > (mmc_card_hs200(card) ? "hs200" : "")); > > > > Since we already have error message in mmc_select_hs200(), > > simply remove it outside. > > > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > > --- > > drivers/mmc/core/mmc.c | 10 +--------- > > 1 file changed, 1 insertion(+), 9 deletions(-) > > > > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c > > index f6683a9..55c8201 100644 > > --- a/drivers/mmc/core/mmc.c > > +++ b/drivers/mmc/core/mmc.c > > @@ -1321,21 +1321,13 @@ static int mmc_select_timing(struct mmc_card *card) > > if (err && err != -EBADMSG) > > return err; > > > > - if (err) { > > - pr_warn("%s: switch to %s failed\n", > > - mmc_card_hs(card) ? "high-speed" : > > - (mmc_card_hs200(card) ? "hs200" : ""), > > - mmc_hostname(card->host)); > > - err = 0; > > - } > > - > > No, this is not correct. > > First, in patch1/3, you change to return an error code immediately > when __mmc_set_signal_voltage() fails, instead of using the "goto > err". Thus there will be no error printed for this case any more. > I noticed this issue when i made up this patch. Why i simply return is because i want to avoid write the following code: static int mmc_select_hs200(struct mmc_card *card) { ... err = voltage_switch(); /* If fails try again during next card power cycle */ if (err) goto err; .... /* swith HS TIMING */ err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING, val, card->ext_csd.generic_cmd6_time, true, send_status, true); if (err) goto err2; .......... err2: if (err) { /* fall back to the old signal voltage, if fails report error */ if (__mmc_set_signal_voltage(host, old_signal_voltage)) err = -EIO; } err: if (err) pr_err("%s: %s failed, error %d\n", mmc_hostname(card->host), __func__, err); return err; } Because if first signal switch fails, we don't need fall back to original voltage. But HS timing switch needs. goto err makes the code a bit ugly. So i choose to avoid it and simply return since it's not so important and most host driver may already implement their debug IO switch debug info. Do you think if we can accept it? > Second, mmc_select_hs() may fail and there is no print done within > that function which is why above print is also needed. > The same as above, i think this print is not so important. And __mmc_switch already has a err warn. And the most important reason is the original print outside is meaningless because it intends to print out the speed mode it wants to switch, however,it actually prints out the restored original speed mode. > On the other hand I agree with you, it doesn't seems necessary to > print two messages when mmc_select_hs200() fails. One should be > enough. Can we change this so *only* mmc_select_timing() will deal > with the print at all errors? > Yes, i agree with your idea. I also thought about it before, but it seems after patch 3, it may not be so necessary. Can you help check it? > > bus_speed: > > /* > > * Set the bus speed to the selected bus timing. > > * If timing is not selected, backward compatible is the default. > > */ > > mmc_set_bus_speed(card); > > - return err; > > + return 0; > > } > > > > /* > > -- > > 1.9.1 > > > > Kind regards > Uffe Regards Dong Aisheng -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 28 April 2016 at 16:58, Dong Aisheng <dongas86@gmail.com> wrote: > On Thu, Apr 28, 2016 at 11:24:07AM +0200, Ulf Hansson wrote: >> On 20 April 2016 at 18:51, Dong Aisheng <aisheng.dong@nxp.com> wrote: >> > mmc_select_hs200() and mmc_select_hs() will keep the timing >> > as before if switch fails. So it's meaningless to print the >> > failed switched mode outside based on the current host timing. >> > >> > Furthermore, the original print is wrong, it should be: >> > pr_warn("%s: switch to %s failed\n", >> > mmc_hostname(card->host), >> > mmc_card_hs(card) ? "high-speed" : >> > (mmc_card_hs200(card) ? "hs200" : "")); >> > >> > Since we already have error message in mmc_select_hs200(), >> > simply remove it outside. >> > >> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> >> > --- >> > drivers/mmc/core/mmc.c | 10 +--------- >> > 1 file changed, 1 insertion(+), 9 deletions(-) >> > >> > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c >> > index f6683a9..55c8201 100644 >> > --- a/drivers/mmc/core/mmc.c >> > +++ b/drivers/mmc/core/mmc.c >> > @@ -1321,21 +1321,13 @@ static int mmc_select_timing(struct mmc_card *card) >> > if (err && err != -EBADMSG) >> > return err; >> > >> > - if (err) { >> > - pr_warn("%s: switch to %s failed\n", >> > - mmc_card_hs(card) ? "high-speed" : >> > - (mmc_card_hs200(card) ? "hs200" : ""), >> > - mmc_hostname(card->host)); >> > - err = 0; >> > - } >> > - >> >> No, this is not correct. >> >> First, in patch1/3, you change to return an error code immediately >> when __mmc_set_signal_voltage() fails, instead of using the "goto >> err". Thus there will be no error printed for this case any more. >> > > I noticed this issue when i made up this patch. > Why i simply return is because i want to avoid write the following code: > static int mmc_select_hs200(struct mmc_card *card) > { > ... > err = voltage_switch(); > /* If fails try again during next card power cycle */ > if (err) > goto err; > .... > /* swith HS TIMING */ > err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, > EXT_CSD_HS_TIMING, val, > card->ext_csd.generic_cmd6_time, > true, send_status, true); > if (err) > goto err2; > .......... > > err2: > if (err) { > /* fall back to the old signal voltage, if fails report error */ > if (__mmc_set_signal_voltage(host, old_signal_voltage)) > err = -EIO; > } > > err: > if (err) > pr_err("%s: %s failed, error %d\n", mmc_hostname(card->host), > __func__, err); > > return err; > } > > Because if first signal switch fails, we don't need fall back to original > voltage. But HS timing switch needs. > > goto err makes the code a bit ugly. > So i choose to avoid it and simply return since it's not so important > and most host driver may already implement their debug IO switch debug info. > > Do you think if we can accept it? Okay. > >> Second, mmc_select_hs() may fail and there is no print done within >> that function which is why above print is also needed. >> > > The same as above, i think this print is not so important. > And __mmc_switch already has a err warn. > > And the most important reason is the original print outside is > meaningless because it intends to print out the speed mode it > wants to switch, however,it actually prints out the restored > original speed mode. > >> On the other hand I agree with you, it doesn't seems necessary to >> print two messages when mmc_select_hs200() fails. One should be >> enough. Can we change this so *only* mmc_select_timing() will deal >> with the print at all errors? >> > > Yes, i agree with your idea. > I also thought about it before, but it seems after patch 3, > it may not be so necessary. > Can you help check it? > >> > bus_speed: >> > /* >> > * Set the bus speed to the selected bus timing. >> > * If timing is not selected, backward compatible is the default. >> > */ >> > mmc_set_bus_speed(card); >> > - return err; >> > + return 0; >> > } >> > >> > /* >> > -- >> > 1.9.1 >> > >> >> Kind regards >> Uffe > > Regards > Dong Aisheng I decided to apply this as is for next. Thanks! Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index f6683a9..55c8201 100644 --- a/drivers/mmc/core/mmc.c +++ b/drivers/mmc/core/mmc.c @@ -1321,21 +1321,13 @@ static int mmc_select_timing(struct mmc_card *card) if (err && err != -EBADMSG) return err; - if (err) { - pr_warn("%s: switch to %s failed\n", - mmc_card_hs(card) ? "high-speed" : - (mmc_card_hs200(card) ? "hs200" : ""), - mmc_hostname(card->host)); - err = 0; - } - bus_speed: /* * Set the bus speed to the selected bus timing. * If timing is not selected, backward compatible is the default. */ mmc_set_bus_speed(card); - return err; + return 0; } /*
mmc_select_hs200() and mmc_select_hs() will keep the timing as before if switch fails. So it's meaningless to print the failed switched mode outside based on the current host timing. Furthermore, the original print is wrong, it should be: pr_warn("%s: switch to %s failed\n", mmc_hostname(card->host), mmc_card_hs(card) ? "high-speed" : (mmc_card_hs200(card) ? "hs200" : "")); Since we already have error message in mmc_select_hs200(), simply remove it outside. Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> --- drivers/mmc/core/mmc.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-)