From patchwork Tue May 2 15:09:09 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jens Axboe X-Patchwork-Id: 9708233 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 997E26021C for ; Tue, 2 May 2017 15:09:15 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 843622847F for ; Tue, 2 May 2017 15:09:15 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 75A062848D; Tue, 2 May 2017 15:09:15 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.4 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID, RCVD_IN_DNSWL_HI, RCVD_IN_SORBS_SPAM autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 78BCF2847F for ; Tue, 2 May 2017 15:09:14 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751112AbdEBPJN (ORCPT ); Tue, 2 May 2017 11:09:13 -0400 Received: from mail-it0-f52.google.com ([209.85.214.52]:38820 "EHLO mail-it0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750763AbdEBPJM (ORCPT ); Tue, 2 May 2017 11:09:12 -0400 Received: by mail-it0-f52.google.com with SMTP id e65so15462901ita.1 for ; Tue, 02 May 2017 08:09:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel-dk.20150623.gappssmtp.com; s=20150623; h=subject:to:references:cc:from:message-id:date:user-agent :mime-version:in-reply-to:content-transfer-encoding; bh=3lEBlaU5pMXj56JRxg/o44PafxCorGUe32G3O/0Ff1E=; b=K+mKmkW4cgJXBvgkzXXYrhxvQNPp59+7rKSPnMVa+PKpUVVSfcScL4F6XBSdijFnFj P9OqqEtBNpk7ZmtEuZLFH++d273FSWro0VIuRhO2jUdw5C7/xA4emXKfC9SF4YslZVx3 /W8XaFrGCNCjaGsgnka8lX75SUkw2brIQ/qC5Kon1dryhJv7TBGnbHViMxAxUMOTo42a 16vhZYpftXwxgzPZVQ1qfAIz/+HCH3W08lUbgTboDcYdLGiFwSAcylj8V7SeYHXseho4 QrudKpzpObWnI8d2paUJVD18q1m/Q6/Fb8pk2YRQAE7PrfwlF1se7gBFjVY4dkNyCYP6 k4Rg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=3lEBlaU5pMXj56JRxg/o44PafxCorGUe32G3O/0Ff1E=; b=Uaq+RJ5jv/WXEJ2ouEjJH6UcUrMG1+f6D9q98nlFqxdIkaAGSW2yATDZj1qiFISu4M 1L8NyporbvBgysxstTqS+Gg3/bkabgTb2bSt7+Li9sh+GZu9K9WgwaOx5qkArYUdRH// H5Rb0R4TdlOkGHtroXaCy5BYYHEuEqCedEOWCswucEQ4cu8Bg/O7bqwKsNH+ZdU9s8B5 +TMWPrA9ZwySleoBPNGz4PIFWtiP4+WcUtXfArE+fH3k6E6oBygI76Vu2t+/nAHwQOnK RHim/7fLuwo3vUk+6pqm8UrLrAl3cbXDYbp08ywkia+5+9KtJeewE0Wd6obLjg9PM7pi rffg== X-Gm-Message-State: AN3rC/7HeRVkV8A3/7aBItxtrpLLVGy9KNU9gA5hLGuZbP5S9AqC4E14 29b2B2Cxks0d4Q== X-Received: by 10.36.53.12 with SMTP id k12mr3866703ita.15.1493737751294; Tue, 02 May 2017 08:09:11 -0700 (PDT) Received: from [192.168.1.154] ([216.160.245.98]) by smtp.gmail.com with ESMTPSA id w134sm8156897iod.14.2017.05.02.08.09.10 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 02 May 2017 08:09:10 -0700 (PDT) Subject: Re: [PATCH 4/6] mtip32xx: convert internal command issue to block IO path To: Christoph Hellwig References: <1493398473-29872-1-git-send-email-axboe@fb.com> <1493398473-29872-5-git-send-email-axboe@fb.com> <20170502072810.GA11582@lst.de> Cc: linux-block@vger.kernel.org, ming.lei@redhat.com From: Jens Axboe Message-ID: Date: Tue, 2 May 2017 09:09:09 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: Sender: linux-block-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-block@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 05/02/2017 08:47 AM, Jens Axboe wrote: > On 05/02/2017 07:53 AM, Jens Axboe wrote: >> On 05/02/2017 01:28 AM, Christoph Hellwig wrote: >>> Looks fine for now: >>> >>> Reviewed-by: Christoph Hellwig >>> >>> But rather sooner than later we need to make this path at least go >>> through the normal end_request processing. Without that we're just >>> bound to run into problems like we had with the tag changes again >>> when the driver is using the block layer APIs incompletely. >> >> I completely agree, I'll see if I can find some time to do that >> soon. > > Seems like a fine thing to do over morning coffee. How about something > like the below? It's on top of the previous series. Basically this kills > the private completion handler and data, which were basically redundant > as they did nothing but spew a warning in case of an error. > > It'd be nice to unify the INTERNAL and regular commands, but we don't > have to do that to use the block layer APIs. If someone else wants to do > that, be my guest. > > It compiles and loads fine on my setup, did some basic testing it that > went fine. Would be nice if someone else could look it over as well, to > verify that I didn't drop any of the 0-vs-error setting. I think it's > fine, we just need to ensure that cmd->status is always set to the > appropriate error, then we propagate that through the mtip32xx softirq > handler to the block layer. Looks like I was missing one error conversion, on the internal request path for taskfile TFE. Changed mtip_complete_command() to take a status argument, and fixup that case too. diff --git a/drivers/block/mtip32xx/mtip32xx.c b/drivers/block/mtip32xx/mtip32xx.c index 96fe6500e941..63a64123b3cb 100644 --- a/drivers/block/mtip32xx/mtip32xx.c +++ b/drivers/block/mtip32xx/mtip32xx.c @@ -205,66 +205,12 @@ static struct mtip_cmd *mtip_get_int_command(struct driver_data *dd) return blk_mq_rq_to_pdu(rq); } -static void mtip_put_int_command(struct driver_data *dd, struct mtip_cmd *cmd) -{ - blk_put_request(blk_mq_rq_from_pdu(cmd)); -} - -/* - * Once we add support for one hctx per mtip group, this will change a bit - */ -static struct request *mtip_rq_from_tag(struct driver_data *dd, - unsigned int tag) -{ - struct blk_mq_hw_ctx *hctx = dd->queue->queue_hw_ctx[0]; - - return blk_mq_tag_to_rq(hctx->tags, tag); -} - static struct mtip_cmd *mtip_cmd_from_tag(struct driver_data *dd, unsigned int tag) { - struct request *rq = mtip_rq_from_tag(dd, tag); - - return blk_mq_rq_to_pdu(rq); -} - -/* - * IO completion function. - * - * This completion function is called by the driver ISR when a - * command that was issued by the kernel completes. It first calls the - * asynchronous completion function which normally calls back into the block - * layer passing the asynchronous callback data, then unmaps the - * scatter list associated with the completed command, and finally - * clears the allocated bit associated with the completed command. - * - * @port Pointer to the port data structure. - * @tag Tag of the command. - * @data Pointer to driver_data. - * @status Completion status. - * - * return value - * None - */ -static void mtip_async_complete(struct mtip_port *port, - int tag, struct mtip_cmd *cmd, int status) -{ - struct driver_data *dd = port->dd; - struct request *rq; - - if (unlikely(!dd) || unlikely(!port)) - return; - - if (unlikely(status == PORT_IRQ_TF_ERR)) { - dev_warn(&port->dd->pdev->dev, - "Command tag %d failed due to TFE\n", tag); - } - - rq = mtip_rq_from_tag(dd, tag); + struct blk_mq_hw_ctx *hctx = dd->queue->queue_hw_ctx[0]; - cmd->status = status; - blk_mq_complete_request(rq); + return blk_mq_rq_to_pdu(blk_mq_tag_to_rq(hctx->tags, tag)); } /* @@ -581,38 +527,19 @@ static void print_tags(struct driver_data *dd, "%d command(s) %s: tagmap [%s]", cnt, msg, tagmap); } -/* - * Internal command completion callback function. - * - * This function is normally called by the driver ISR when an internal - * command completed. This function signals the command completion by - * calling complete(). - * - * @port Pointer to the port data structure. - * @tag Tag of the command that has completed. - * @data Pointer to a completion structure. - * @status Completion status. - * - * return value - * None - */ -static void mtip_completion(struct mtip_port *port, - int tag, struct mtip_cmd *command, int status) -{ - struct completion *waiting = command->comp_data; - if (unlikely(status == PORT_IRQ_TF_ERR)) - dev_warn(&port->dd->pdev->dev, - "Internal command %d completed with TFE\n", tag); - - command->comp_func = NULL; - command->comp_data = NULL; - complete(waiting); -} - static int mtip_read_log_page(struct mtip_port *port, u8 page, u16 *buffer, dma_addr_t buffer_dma, unsigned int sectors); static int mtip_get_smart_attr(struct mtip_port *port, unsigned int id, struct smart_attr *attrib); + +static void mtip_complete_command(struct mtip_cmd *cmd, int status) +{ + struct request *req = blk_mq_rq_from_pdu(cmd); + + cmd->status = status; + blk_mq_complete_request(req); +} + /* * Handle an error. * @@ -641,11 +568,7 @@ static void mtip_handle_tfe(struct driver_data *dd) if (test_bit(MTIP_PF_IC_ACTIVE_BIT, &port->flags)) { cmd = mtip_cmd_from_tag(dd, MTIP_TAG_INTERNAL); dbg_printk(MTIP_DRV_NAME " TFE for the internal command\n"); - - if (cmd->comp_data && cmd->comp_func) { - cmd->comp_func(port, MTIP_TAG_INTERNAL, - cmd, PORT_IRQ_TF_ERR); - } + mtip_complete_command(cmd, -EIO); return; } @@ -672,19 +595,9 @@ static void mtip_handle_tfe(struct driver_data *dd) continue; cmd = mtip_cmd_from_tag(dd, tag); - if (likely(cmd->comp_func)) { - set_bit(tag, tagaccum); - cmd_cnt++; - cmd->comp_func(port, tag, cmd, 0); - } else { - dev_err(&port->dd->pdev->dev, - "Missing completion func for tag %d", - tag); - if (mtip_check_surprise_removal(dd->pdev)) { - /* don't proceed further */ - return; - } - } + mtip_complete_command(cmd, 0); + set_bit(tag, tagaccum); + cmd_cnt++; } } @@ -754,10 +667,7 @@ static void mtip_handle_tfe(struct driver_data *dd) tag, fail_reason != NULL ? fail_reason : "unknown"); - if (cmd->comp_func) { - cmd->comp_func(port, tag, - cmd, -ENODATA); - } + mtip_complete_command(cmd, -ENODATA); continue; } } @@ -780,12 +690,7 @@ static void mtip_handle_tfe(struct driver_data *dd) dev_warn(&port->dd->pdev->dev, "retiring tag %d\n", tag); - if (cmd->comp_func) - cmd->comp_func(port, tag, cmd, PORT_IRQ_TF_ERR); - else - dev_warn(&port->dd->pdev->dev, - "Bad completion for tag %d\n", - tag); + mtip_complete_command(cmd, -EIO); } } print_tags(dd, "reissued (TFE)", tagaccum, cmd_cnt); @@ -818,18 +723,7 @@ static inline void mtip_workq_sdbfx(struct mtip_port *port, int group, continue; command = mtip_cmd_from_tag(dd, tag); - if (likely(command->comp_func)) - command->comp_func(port, tag, command, 0); - else { - dev_dbg(&dd->pdev->dev, - "Null completion for tag %d", - tag); - - if (mtip_check_surprise_removal( - dd->pdev)) { - return; - } - } + mtip_complete_command(command, 0); } completed >>= 1; } @@ -850,13 +744,8 @@ static inline void mtip_process_legacy(struct driver_data *dd, u32 port_stat) if (test_bit(MTIP_PF_IC_ACTIVE_BIT, &port->flags) && (cmd != NULL) && !(readl(port->cmd_issue[MTIP_TAG_INTERNAL]) & (1 << MTIP_TAG_INTERNAL))) { - if (cmd->comp_func) { - cmd->comp_func(port, MTIP_TAG_INTERNAL, cmd, 0); - return; - } + mtip_complete_command(cmd, 0); } - - return; } /* @@ -864,7 +753,6 @@ static inline void mtip_process_legacy(struct driver_data *dd, u32 port_stat) */ static inline void mtip_process_errors(struct driver_data *dd, u32 port_stat) { - if (unlikely(port_stat & PORT_IRQ_CONNECT)) { dev_warn(&dd->pdev->dev, "Clearing PxSERR.DIAG.x\n"); @@ -991,8 +879,7 @@ static irqreturn_t mtip_irq_handler(int irq, void *instance) static void mtip_issue_non_ncq_command(struct mtip_port *port, int tag) { - writel(1 << MTIP_TAG_BIT(tag), - port->cmd_issue[MTIP_TAG_INDEX(tag)]); + writel(1 << MTIP_TAG_BIT(tag), port->cmd_issue[MTIP_TAG_INDEX(tag)]); } static bool mtip_pause_ncq(struct mtip_port *port, @@ -1121,7 +1008,6 @@ static int mtip_exec_internal_command(struct mtip_port *port, u32 opts, unsigned long timeout) { - DECLARE_COMPLETION_ONSTACK(wait); struct mtip_cmd *int_cmd; struct driver_data *dd = port->dd; struct request *rq; @@ -1146,7 +1032,7 @@ static int mtip_exec_internal_command(struct mtip_port *port, return -EFAULT; } rq = blk_mq_rq_from_pdu(int_cmd); - rq->end_io_data = &icmd; + rq->special = &icmd; set_bit(MTIP_PF_IC_ACTIVE_BIT, &port->flags); @@ -1159,17 +1045,13 @@ static int mtip_exec_internal_command(struct mtip_port *port, /* wait for io to complete if non atomic */ if (mtip_quiesce_io(port, MTIP_QUIESCE_IO_TIMEOUT_MS) < 0) { dev_warn(&dd->pdev->dev, "Failed to quiesce IO\n"); - mtip_put_int_command(dd, int_cmd); + blk_mq_free_request(rq); clear_bit(MTIP_PF_IC_ACTIVE_BIT, &port->flags); wake_up_interruptible(&port->svc_wait); return -EBUSY; } } - /* Set the completion function and data for the command. */ - int_cmd->comp_data = &wait; - int_cmd->comp_func = mtip_completion; - /* Copy the command to the command table */ memcpy(int_cmd->command, fis, fis_len*4); @@ -1177,11 +1059,9 @@ static int mtip_exec_internal_command(struct mtip_port *port, rq->timeout = timeout; /* insert request and run queue */ - blk_execute_rq_nowait(rq->q, NULL, rq, true, NULL); + blk_execute_rq(rq->q, NULL, rq, true); - wait_for_completion(&wait); rv = int_cmd->status; - if (rv < 0) { if (rv == -ERESTARTSYS) { /* interrupted */ dev_err(&dd->pdev->dev, @@ -1223,7 +1103,7 @@ static int mtip_exec_internal_command(struct mtip_port *port, } exec_ic_exit: /* Clear the allocated and active bits for the internal command. */ - mtip_put_int_command(dd, int_cmd); + blk_mq_free_request(rq); clear_bit(MTIP_PF_IC_ACTIVE_BIT, &port->flags); if (rv >= 0 && mtip_pause_ncq(port, fis)) { /* NCQ paused */ @@ -2378,12 +2258,6 @@ static void mtip_hw_submit_io(struct driver_data *dd, struct request *rq, (nents << 16) | 5 | AHCI_CMD_PREFETCH); command->command_header->byte_count = 0; - /* - * Set the completion function and data for the command - * within this layer. - */ - command->comp_data = dd; - command->comp_func = mtip_async_complete; command->direction = dma_dir; /* @@ -3761,15 +3635,13 @@ static int mtip_issue_reserved_cmd(struct blk_mq_hw_ctx *hctx, struct request *rq) { struct driver_data *dd = hctx->queue->queuedata; - struct mtip_int_cmd *icmd = rq->end_io_data; + struct mtip_int_cmd *icmd = rq->special; struct mtip_cmd *cmd = blk_mq_rq_to_pdu(rq); struct mtip_cmd_sg *command_sg; if (mtip_commands_active(dd->port)) return BLK_MQ_RQ_QUEUE_BUSY; - rq->end_io_data = NULL; - /* Populate the SG list */ cmd->command_header->opts = __force_bit2int cpu_to_le32(icmd->opts | icmd->fis_len); @@ -3857,9 +3729,7 @@ static enum blk_eh_timer_return mtip_cmd_timeout(struct request *req, struct mtip_cmd *cmd = blk_mq_rq_to_pdu(req); cmd->status = -ETIME; - if (cmd->comp_func) - cmd->comp_func(dd->port, MTIP_TAG_INTERNAL, cmd, -ETIME); - goto exit_handler; + return BLK_EH_HANDLED; } if (test_bit(req->tag, dd->port->cmds_to_issue)) @@ -4087,21 +3957,10 @@ static int mtip_block_initialize(struct driver_data *dd) static void mtip_no_dev_cleanup(struct request *rq, void *data, bool reserv) { - struct driver_data *dd = (struct driver_data *)data; - struct mtip_cmd *cmd; - - if (likely(!reserv)) { - cmd = blk_mq_rq_to_pdu(rq); - cmd->status = -ENODEV; - blk_mq_complete_request(rq); - } else if (test_bit(MTIP_PF_IC_ACTIVE_BIT, &dd->port->flags)) { + struct mtip_cmd *cmd = blk_mq_rq_to_pdu(rq); - cmd = mtip_cmd_from_tag(dd, MTIP_TAG_INTERNAL); - cmd->status = -ENODEV; - if (cmd->comp_func) - cmd->comp_func(dd->port, MTIP_TAG_INTERNAL, - cmd, -ENODEV); - } + cmd->status = -ENODEV; + blk_mq_complete_request(rq); } /* diff --git a/drivers/block/mtip32xx/mtip32xx.h b/drivers/block/mtip32xx/mtip32xx.h index 57b41528a824..37b8e3e0bb78 100644 --- a/drivers/block/mtip32xx/mtip32xx.h +++ b/drivers/block/mtip32xx/mtip32xx.h @@ -333,16 +333,6 @@ struct mtip_cmd { dma_addr_t command_dma; /* corresponding physical address */ - void *comp_data; /* data passed to completion function comp_func() */ - /* - * Completion function called by the ISR upon completion of - * a command. - */ - void (*comp_func)(struct mtip_port *port, - int tag, - struct mtip_cmd *cmd, - int status); - int scatter_ents; /* Number of scatter list entries used */ int unaligned; /* command is unaligned on 4k boundary */