| Message ID | 1509715220-31885-9-git-send-email-adrian.hunter@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Nov 3, 2017 at 2:20 PM, Adrian Hunter <adrian.hunter@intel.com> wrote: > Recovery is simpler to understand if it is only used for errors. Create a > separate function for card polling. > > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> This looks good but I can't see why it's not folded into patch 3 already. This error handling is introduced there. Yours, Linus Walleij
On 08/11/17 11:30, Linus Walleij wrote: > On Fri, Nov 3, 2017 at 2:20 PM, Adrian Hunter <adrian.hunter@intel.com> wrote: > >> Recovery is simpler to understand if it is only used for errors. Create a >> separate function for card polling. >> >> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> > > This looks good but I can't see why it's not folded into > patch 3 already. This error handling is introduced there. What are you on about? If we're going to split up the patches (which I argued against - the new code is all new, so it could be read independently from the old mess) then this is a logically distinct step. Polling and error-recovery are conceptually different things and it is important to separate them to make the code easier to understand.
On Thu, Nov 9, 2017 at 8:56 AM, Adrian Hunter <adrian.hunter@intel.com> wrote: > On 08/11/17 11:30, Linus Walleij wrote: >> On Fri, Nov 3, 2017 at 2:20 PM, Adrian Hunter <adrian.hunter@intel.com> wrote: >> >>> Recovery is simpler to understand if it is only used for errors. Create a >>> separate function for card polling. >>> >>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> >> >> This looks good but I can't see why it's not folded into >> patch 3 already. This error handling is introduced there. > > What are you on about? You are attacking your most valuable resource, a reviewer. And I even said the patch looks good. The only thing you attain with this kind of langauge is alienante me and discourage others to review your patch set. You also give your employer a bad name, since you are representing them. > If we're going to split up the patches (which I > argued against - the new code is all new, so it could be read independently > from the old mess) then this is a logically distinct step. Polling and > error-recovery are conceptually different things and it is important to > separate them to make the code easier to understand. I understand it can be tough to deal with review comments and it can make you loose your temper when people (sometimes even the same person!) say contradictory things. But in hindsight, don't you think these 5 last lines of your message had been enough without that first line? Yours, Linus Walleij
On 09/11/17 14:52, Linus Walleij wrote: > On Thu, Nov 9, 2017 at 8:56 AM, Adrian Hunter <adrian.hunter@intel.com> wrote: >> On 08/11/17 11:30, Linus Walleij wrote: >>> On Fri, Nov 3, 2017 at 2:20 PM, Adrian Hunter <adrian.hunter@intel.com> wrote: >>> >>>> Recovery is simpler to understand if it is only used for errors. Create a >>>> separate function for card polling. >>>> >>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> >>> >>> This looks good but I can't see why it's not folded into >>> patch 3 already. This error handling is introduced there. >> >> What are you on about? > > You are attacking your most valuable resource, a reviewer. > > And I even said the patch looks good. > > The only thing you attain with this kind of langauge is alienante > me and discourage others to review your patch set. You also > give your employer a bad name, since you are representing > them. 6 months of being messed around will do that. >> If we're going to split up the patches (which I >> argued against - the new code is all new, so it could be read independently >> from the old mess) then this is a logically distinct step. Polling and >> error-recovery are conceptually different things and it is important to >> separate them to make the code easier to understand. > > I understand it can be tough to deal with review comments > and it can make you loose your temper when people (sometimes > even the same person!) say contradictory things. > > But in hindsight, don't you think these 5 last lines of your message > had been enough without that first line? Very true.
On Thu, Nov 9, 2017 at 2:02 PM, Adrian Hunter <adrian.hunter@intel.com> wrote: > On 09/11/17 14:52, Linus Walleij wrote: >> On Thu, Nov 9, 2017 at 8:56 AM, Adrian Hunter <adrian.hunter@intel.com> wrote: >>> On 08/11/17 11:30, Linus Walleij wrote: >>>> On Fri, Nov 3, 2017 at 2:20 PM, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>> >>>>> Recovery is simpler to understand if it is only used for errors. Create a >>>>> separate function for card polling. >>>>> >>>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> >>>> >>>> This looks good but I can't see why it's not folded into >>>> patch 3 already. This error handling is introduced there. >>> >>> What are you on about? >> >> You are attacking your most valuable resource, a reviewer. >> >> And I even said the patch looks good. >> >> The only thing you attain with this kind of langauge is alienante >> me and discourage others to review your patch set. You also >> give your employer a bad name, since you are representing >> them. > > 6 months of being messed around will do that. Blessed are the meek, for they will inherit the earth. Nobody is after you, this is just hard stuff and too few people care to help out with review etc. With this and with the block layer as well. That makes things slow. It's noone's agenda to slow things down. Yours, Linus Walleij
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index cbb4b35a592d..0c29b1d8d545 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -2094,6 +2094,24 @@ static inline bool mmc_blk_rq_error(struct mmc_blk_request *brq) brq->data.error || brq->cmd.resp[0] & CMD_ERRORS; } +static int mmc_blk_card_busy(struct mmc_card *card, struct request *req) +{ + struct mmc_queue_req *mqrq = req_to_mmc_queue_req(req); + bool gen_err = false; + int err; + + if (mmc_host_is_spi(card->host) || rq_data_dir(req) == READ) + return 0; + + err = card_busy_detect(card, MMC_BLK_TIMEOUT_MS, false, req, &gen_err); + + /* Copy the general error bit so it will be seen later on */ + if (gen_err) + mqrq->brq.stop.resp[0] |= R1_ERROR; + + return err; +} + static inline void mmc_blk_rw_reset_success(struct mmc_queue *mq, struct request *req) { @@ -2150,8 +2168,15 @@ static void mmc_blk_mq_poll_completion(struct mmc_queue *mq, struct request *req) { struct mmc_queue_req *mqrq = req_to_mmc_queue_req(req); + struct mmc_host *host = mq->card->host; - mmc_blk_rw_recovery(mq, req); + if (mmc_blk_rq_error(&mqrq->brq) || + mmc_blk_card_busy(mq->card, req)) { + mmc_blk_rw_recovery(mq, req); + } else { + mmc_blk_rw_reset_success(mq, req); + mmc_retune_release(host); + } mmc_blk_urgent_bkops(mq, mqrq); }
Recovery is simpler to understand if it is only used for errors. Create a separate function for card polling. Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> --- drivers/mmc/core/block.c | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-)