On 28 April 2016 at 16:58, Dong Aisheng <dongas86@xxxxxxxxx> 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@xxxxxxx> 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@xxxxxxx>
>> > ---
>> > 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
