[PATCH 2/3] mmc: core: remove the invalid message in mmc_select_timing

[Ulf Hansson]

On 28 April 2016 at 16:58, Dong Aisheng <dongas86 at 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 at 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 at 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