| Message ID | 1379611214-1089-1-git-send-email-grundler@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 19 September 2013 19:20, Grant Grundler <grundler@chromium.org> wrote: > struct mmc_queue defines issue_fn as an indirect function call. > issue_fn field only gets set to mmc_blk_issue_rq and only gets > invoked immediately after calling blk_fetch_request(). > Don't bother with indirect function call - it's pointless and just > obfuscates the code. > > Signed-off-by: Grant Grundler <grundler@chromium.org> Acked-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > drivers/mmc/card/block.c | 1 - > drivers/mmc/card/queue.c | 2 +- > drivers/mmc/card/queue.h | 1 - > 3 files changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c > index 1a3163f..b2cdd10 100644 > --- a/drivers/mmc/card/block.c > +++ b/drivers/mmc/card/block.c > @@ -2072,7 +2072,6 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card, > if (ret) > goto err_putdisk; > > - md->queue.issue_fn = mmc_blk_issue_rq; > md->queue.data = md; > > md->disk->major = MMC_BLOCK_MAJOR; > diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c > index fa9632e..6f9adc5 100644 > --- a/drivers/mmc/card/queue.c > +++ b/drivers/mmc/card/queue.c > @@ -67,7 +67,7 @@ static int mmc_queue_thread(void *d) > if (req || mq->mqrq_prev->req) { > set_current_state(TASK_RUNNING); > cmd_flags = req ? req->cmd_flags : 0; > - mq->issue_fn(mq, req); > + mmc_blk_issue_rq(mq, req); > if (mq->flags & MMC_QUEUE_NEW_REQUEST) { > mq->flags &= ~MMC_QUEUE_NEW_REQUEST; > continue; /* fetch again */ > diff --git a/drivers/mmc/card/queue.h b/drivers/mmc/card/queue.h > index 5752d50..35380015 100644 > --- a/drivers/mmc/card/queue.h > +++ b/drivers/mmc/card/queue.h > @@ -51,7 +51,6 @@ struct mmc_queue { > #define MMC_QUEUE_SUSPENDED (1 << 0) > #define MMC_QUEUE_NEW_REQUEST (1 << 1) > > - int (*issue_fn)(struct mmc_queue *, struct request *); > void *data; > struct request_queue *queue; > struct mmc_queue_req mqrq[2]; > -- > 1.8.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- 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
Hi, On Fri, Sep 20 2013, Ulf Hansson wrote: > On 19 September 2013 19:20, Grant Grundler <grundler@chromium.org> wrote: >> struct mmc_queue defines issue_fn as an indirect function call. >> issue_fn field only gets set to mmc_blk_issue_rq and only gets >> invoked immediately after calling blk_fetch_request(). >> Don't bother with indirect function call - it's pointless and just >> obfuscates the code. >> >> Signed-off-by: Grant Grundler <grundler@chromium.org> > > Acked-by: Ulf Hansson <ulf.hansson@linaro.org> Thanks, pushed to mmc-next for 3.13. - Chris.
Hi, On Wed, Sep 25 2013, Chris Ball wrote: > Hi, > > On Fri, Sep 20 2013, Ulf Hansson wrote: >> On 19 September 2013 19:20, Grant Grundler <grundler@chromium.org> wrote: >>> struct mmc_queue defines issue_fn as an indirect function call. >>> issue_fn field only gets set to mmc_blk_issue_rq and only gets >>> invoked immediately after calling blk_fetch_request(). >>> Don't bother with indirect function call - it's pointless and just >>> obfuscates the code. >>> >>> Signed-off-by: Grant Grundler <grundler@chromium.org> >> >> Acked-by: Ulf Hansson <ulf.hansson@linaro.org> > > Thanks, pushed to mmc-next for 3.13. Have dropped this, it's breaking my build: /home/cjb/git/mmc/drivers/mmc/card/block.c:1955:12: warning: ‘mmc_blk_issue_rq’ defined but not used [-Wunused-function] /home/cjb/git/mmc/drivers/mmc/card/queue.c: In function ‘mmc_queue_thread’: /home/cjb/git/mmc/drivers/mmc/card/queue.c:70:4: error: implicit declaration of function ‘mmc_blk_issue_rq’ [-Werror=implicit-function-declaration] Grant, please could you take a look and resubmit? - Chris.
On Wed, Sep 25, 2013 at 7:37 PM, Chris Ball <cjb@laptop.org> wrote: > Hi, > > On Wed, Sep 25 2013, Chris Ball wrote: >> Hi, >> >> On Fri, Sep 20 2013, Ulf Hansson wrote: >>> On 19 September 2013 19:20, Grant Grundler <grundler@chromium.org> wrote: >>>> struct mmc_queue defines issue_fn as an indirect function call. >>>> issue_fn field only gets set to mmc_blk_issue_rq and only gets >>>> invoked immediately after calling blk_fetch_request(). >>>> Don't bother with indirect function call - it's pointless and just >>>> obfuscates the code. >>>> >>>> Signed-off-by: Grant Grundler <grundler@chromium.org> >>> >>> Acked-by: Ulf Hansson <ulf.hansson@linaro.org> >> >> Thanks, pushed to mmc-next for 3.13. > > Have dropped this, it's breaking my build: > > /home/cjb/git/mmc/drivers/mmc/card/block.c:1955:12: warning: ‘mmc_blk_issue_rq’ defined but not used [-Wunused-function] The function is declared static. :( Let me respin to remove the static and add a function prototype to a header file. I just got lucky when I built this in an earlier tree. sorry about that... grant > /home/cjb/git/mmc/drivers/mmc/card/queue.c: In function ‘mmc_queue_thread’: > /home/cjb/git/mmc/drivers/mmc/card/queue.c:70:4: error: implicit declaration of function ‘mmc_blk_issue_rq’ [-Werror=implicit-function-declaration] > > Grant, please could you take a look and resubmit? > > - Chris. > -- > Chris Ball <cjb@laptop.org> <http://printf.net/> -- 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, Sep 26, 2013 at 2:56 PM, Grant Grundler <grundler@chromium.org> wrote: > On Wed, Sep 25, 2013 at 7:37 PM, Chris Ball <cjb@laptop.org> wrote: >> Hi, >> >> On Wed, Sep 25 2013, Chris Ball wrote: >>> Hi, >>> >>> On Fri, Sep 20 2013, Ulf Hansson wrote: >>>> On 19 September 2013 19:20, Grant Grundler <grundler@chromium.org> wrote: >>>>> struct mmc_queue defines issue_fn as an indirect function call. >>>>> issue_fn field only gets set to mmc_blk_issue_rq and only gets >>>>> invoked immediately after calling blk_fetch_request(). >>>>> Don't bother with indirect function call - it's pointless and just >>>>> obfuscates the code. >>>>> >>>>> Signed-off-by: Grant Grundler <grundler@chromium.org> >>>> >>>> Acked-by: Ulf Hansson <ulf.hansson@linaro.org> >>> >>> Thanks, pushed to mmc-next for 3.13. >> >> Have dropped this, it's breaking my build: >> >> /home/cjb/git/mmc/drivers/mmc/card/block.c:1955:12: warning: ‘mmc_blk_issue_rq’ defined but not used [-Wunused-function] > > The function is declared static. :( Let me respin to remove the > static and add a function prototype to a header file. block.o and queue.o are linked together into one .ko all the time: obj-$(CONFIG_MMC_BLOCK) += mmc_block.o mmc_block-objs := block.o queue.o Two ways to handle this: I can 1) add a local function prototype of mmc_blk_issue_rq() to queue.c 2) move mmc_init_queue() and mmc_queue_thread() from queue.c to block.c (2) actually makes sense since both functions are block IO specific. Thoughts? Preference? Other ideas? thanks, grant ps. It's more obvious now that the return value from mmc_blk_issue_rq() is getting ignored. *sigh* -- 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 27 September 2013 00:16, Grant Grundler <grundler@chromium.org> wrote: > On Thu, Sep 26, 2013 at 2:56 PM, Grant Grundler <grundler@chromium.org> wrote: >> On Wed, Sep 25, 2013 at 7:37 PM, Chris Ball <cjb@laptop.org> wrote: >>> Hi, >>> >>> On Wed, Sep 25 2013, Chris Ball wrote: >>>> Hi, >>>> >>>> On Fri, Sep 20 2013, Ulf Hansson wrote: >>>>> On 19 September 2013 19:20, Grant Grundler <grundler@chromium.org> wrote: >>>>>> struct mmc_queue defines issue_fn as an indirect function call. >>>>>> issue_fn field only gets set to mmc_blk_issue_rq and only gets >>>>>> invoked immediately after calling blk_fetch_request(). >>>>>> Don't bother with indirect function call - it's pointless and just >>>>>> obfuscates the code. >>>>>> >>>>>> Signed-off-by: Grant Grundler <grundler@chromium.org> >>>>> >>>>> Acked-by: Ulf Hansson <ulf.hansson@linaro.org> >>>> >>>> Thanks, pushed to mmc-next for 3.13. >>> >>> Have dropped this, it's breaking my build: >>> >>> /home/cjb/git/mmc/drivers/mmc/card/block.c:1955:12: warning: ‘mmc_blk_issue_rq’ defined but not used [-Wunused-function] >> >> The function is declared static. :( Let me respin to remove the >> static and add a function prototype to a header file. > > block.o and queue.o are linked together into one .ko all the time: > obj-$(CONFIG_MMC_BLOCK) += mmc_block.o > mmc_block-objs := block.o queue.o > > Two ways to handle this: I can > 1) add a local function prototype of mmc_blk_issue_rq() to queue.c > 2) move mmc_init_queue() and mmc_queue_thread() from queue.c to block.c > > (2) actually makes sense since both functions are block IO specific. > > Thoughts? Preference? Other ideas? Hi Grant, Generally I am in favour of cleaning up messy code. But in this case it now seems a bit overworked. How about actually leaving it as is? Kind regards Ulf Hansson > > thanks, > grant > > ps. It's more obvious now that the return value from > mmc_blk_issue_rq() is getting ignored. *sigh* -- 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/card/block.c b/drivers/mmc/card/block.c index 1a3163f..b2cdd10 100644 --- a/drivers/mmc/card/block.c +++ b/drivers/mmc/card/block.c @@ -2072,7 +2072,6 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card, if (ret) goto err_putdisk; - md->queue.issue_fn = mmc_blk_issue_rq; md->queue.data = md; md->disk->major = MMC_BLOCK_MAJOR; diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c index fa9632e..6f9adc5 100644 --- a/drivers/mmc/card/queue.c +++ b/drivers/mmc/card/queue.c @@ -67,7 +67,7 @@ static int mmc_queue_thread(void *d) if (req || mq->mqrq_prev->req) { set_current_state(TASK_RUNNING); cmd_flags = req ? req->cmd_flags : 0; - mq->issue_fn(mq, req); + mmc_blk_issue_rq(mq, req); if (mq->flags & MMC_QUEUE_NEW_REQUEST) { mq->flags &= ~MMC_QUEUE_NEW_REQUEST; continue; /* fetch again */ diff --git a/drivers/mmc/card/queue.h b/drivers/mmc/card/queue.h index 5752d50..35380015 100644 --- a/drivers/mmc/card/queue.h +++ b/drivers/mmc/card/queue.h @@ -51,7 +51,6 @@ struct mmc_queue { #define MMC_QUEUE_SUSPENDED (1 << 0) #define MMC_QUEUE_NEW_REQUEST (1 << 1) - int (*issue_fn)(struct mmc_queue *, struct request *); void *data; struct request_queue *queue; struct mmc_queue_req mqrq[2];
struct mmc_queue defines issue_fn as an indirect function call. issue_fn field only gets set to mmc_blk_issue_rq and only gets invoked immediately after calling blk_fetch_request(). Don't bother with indirect function call - it's pointless and just obfuscates the code. Signed-off-by: Grant Grundler <grundler@chromium.org> --- drivers/mmc/card/block.c | 1 - drivers/mmc/card/queue.c | 2 +- drivers/mmc/card/queue.h | 1 - 3 files changed, 1 insertion(+), 3 deletions(-)