| Message ID | 20190510112440.GA22858@mwanda (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mspro_block: Fix an error code in mspro_block_issue_req() | expand |
Am 10.05.2019 13:24, schrieb Dan Carpenter: > We accidentally changed the error code from -EAGAIN to 1 when we did the > blk-mq conversion. > > Maybe a contributing factor to this mistake is that it wasn't obvious > that the "while (chunk) {" condition is always true. I have cleaned > that up as well. > > Fixes: d0be12274dad ("mspro_block: convert to blk-mq") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > drivers/memstick/core/mspro_block.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/drivers/memstick/core/mspro_block.c b/drivers/memstick/core/mspro_block.c > index aba50ec98b4d..9fc22c755dbf 100644 > --- a/drivers/memstick/core/mspro_block.c > +++ b/drivers/memstick/core/mspro_block.c > @@ -694,13 +694,13 @@ static void h_mspro_block_setup_cmd(struct memstick_dev *card, u64 offset, > > /*** Data transfer ***/ > > -static int mspro_block_issue_req(struct memstick_dev *card, bool chunk) > +static int mspro_block_issue_req(struct memstick_dev *card) > { > struct mspro_block_data *msb = memstick_get_drvdata(card); > u64 t_off; > unsigned int count; > > - while (chunk) { > + while (true) { > msb->current_page = 0; > msb->current_seg = 0; > msb->seg_count = blk_rq_map_sg(msb->block_req->q, > @@ -709,6 +709,7 @@ static int mspro_block_issue_req(struct memstick_dev *card, bool chunk) > > if (!msb->seg_count) { > unsigned int bytes = blk_rq_cur_bytes(msb->block_req); > + bool chunk; > > chunk = blk_update_request(msb->block_req, > BLK_STS_RESOURCE, > @@ -718,7 +719,7 @@ static int mspro_block_issue_req(struct memstick_dev *card, bool chunk) > __blk_mq_end_request(msb->block_req, > BLK_STS_RESOURCE); > msb->block_req = NULL; > - break; > + return -EAGAIN; > } > > t_off = blk_rq_pos(msb->block_req); > @@ -735,8 +736,6 @@ static int mspro_block_issue_req(struct memstick_dev *card, bool chunk) > memstick_new_req(card->host); > return 0; > } > - > - return 1; > } > If i see this correcly everybody leaving the loop making the function return something. I do not know how would do that but IMHO is better to be defensive and end the function with a propper return code (perhaps 0). re, wh > static int mspro_block_complete_req(struct memstick_dev *card, int error) > @@ -779,7 +778,7 @@ static int mspro_block_complete_req(struct memstick_dev *card, int error) > chunk = blk_update_request(msb->block_req, > errno_to_blk_status(error), t_len); > if (chunk) { > - error = mspro_block_issue_req(card, chunk); > + error = mspro_block_issue_req(card); > if (!error) > goto out; > } else { > @@ -849,7 +848,7 @@ static blk_status_t mspro_queue_rq(struct blk_mq_hw_ctx *hctx, > msb->block_req = bd->rq; > blk_mq_start_request(bd->rq); > > - if (mspro_block_issue_req(card, true)) > + if (mspro_block_issue_req(card)) > msb->block_req = NULL; > > spin_unlock_irq(&msb->q_lock);
On Fri, May 10, 2019 at 03:11:45PM +0200, walter harms wrote: > If i see this correcly everybody leaving the loop making the function return something. > I do not know how would do that but IMHO is better to be defensive and end the function > with a propper return code (perhaps 0). > It's a forever loop. If we add another break statement without adding at return then GCC will complain. I feel like maybe you're saying that you don't like forever loops? That becomes a pretty complicated position to take... regards, dan carpenter
Am 10.05.2019 15:23, schrieb Dan Carpenter: > On Fri, May 10, 2019 at 03:11:45PM +0200, walter harms wrote: >> If i see this correcly everybody leaving the loop making the function return something. >> I do not know how would do that but IMHO is better to be defensive and end the function >> with a propper return code (perhaps 0). >> > > It's a forever loop. If we add another break statement without adding > at return then GCC will complain. > > I feel like maybe you're saying that you don't like forever loops? That > becomes a pretty complicated position to take... > No, not really, but after several years of programming i have learned that forever() is never forever and a break statement is easily added (in future). Also it also looks strange to have a function returning int but the last thing is not returning something. just my 2 cents, re, wh ps: you could easily do memstick_new_req(card->host); break; } return 0;
return -EINVAL; is more readable (100% unambiguous) than a break statement. ergards, dan carpenter
On Fri, 10 May 2019 at 13:25, Dan Carpenter <dan.carpenter@oracle.com> wrote: > > We accidentally changed the error code from -EAGAIN to 1 when we did the > blk-mq conversion. > > Maybe a contributing factor to this mistake is that it wasn't obvious > that the "while (chunk) {" condition is always true. I have cleaned > that up as well. > > Fixes: d0be12274dad ("mspro_block: convert to blk-mq") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Applied for fixes and by adding a stable tag, thanks! Kind regards Uffe > --- > drivers/memstick/core/mspro_block.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/drivers/memstick/core/mspro_block.c b/drivers/memstick/core/mspro_block.c > index aba50ec98b4d..9fc22c755dbf 100644 > --- a/drivers/memstick/core/mspro_block.c > +++ b/drivers/memstick/core/mspro_block.c > @@ -694,13 +694,13 @@ static void h_mspro_block_setup_cmd(struct memstick_dev *card, u64 offset, > > /*** Data transfer ***/ > > -static int mspro_block_issue_req(struct memstick_dev *card, bool chunk) > +static int mspro_block_issue_req(struct memstick_dev *card) > { > struct mspro_block_data *msb = memstick_get_drvdata(card); > u64 t_off; > unsigned int count; > > - while (chunk) { > + while (true) { > msb->current_page = 0; > msb->current_seg = 0; > msb->seg_count = blk_rq_map_sg(msb->block_req->q, > @@ -709,6 +709,7 @@ static int mspro_block_issue_req(struct memstick_dev *card, bool chunk) > > if (!msb->seg_count) { > unsigned int bytes = blk_rq_cur_bytes(msb->block_req); > + bool chunk; > > chunk = blk_update_request(msb->block_req, > BLK_STS_RESOURCE, > @@ -718,7 +719,7 @@ static int mspro_block_issue_req(struct memstick_dev *card, bool chunk) > __blk_mq_end_request(msb->block_req, > BLK_STS_RESOURCE); > msb->block_req = NULL; > - break; > + return -EAGAIN; > } > > t_off = blk_rq_pos(msb->block_req); > @@ -735,8 +736,6 @@ static int mspro_block_issue_req(struct memstick_dev *card, bool chunk) > memstick_new_req(card->host); > return 0; > } > - > - return 1; > } > > static int mspro_block_complete_req(struct memstick_dev *card, int error) > @@ -779,7 +778,7 @@ static int mspro_block_complete_req(struct memstick_dev *card, int error) > chunk = blk_update_request(msb->block_req, > errno_to_blk_status(error), t_len); > if (chunk) { > - error = mspro_block_issue_req(card, chunk); > + error = mspro_block_issue_req(card); > if (!error) > goto out; > } else { > @@ -849,7 +848,7 @@ static blk_status_t mspro_queue_rq(struct blk_mq_hw_ctx *hctx, > msb->block_req = bd->rq; > blk_mq_start_request(bd->rq); > > - if (mspro_block_issue_req(card, true)) > + if (mspro_block_issue_req(card)) > msb->block_req = NULL; > > spin_unlock_irq(&msb->q_lock); > -- > 2.18.0 >
diff --git a/drivers/memstick/core/mspro_block.c b/drivers/memstick/core/mspro_block.c index aba50ec98b4d..9fc22c755dbf 100644 --- a/drivers/memstick/core/mspro_block.c +++ b/drivers/memstick/core/mspro_block.c @@ -694,13 +694,13 @@ static void h_mspro_block_setup_cmd(struct memstick_dev *card, u64 offset, /*** Data transfer ***/ -static int mspro_block_issue_req(struct memstick_dev *card, bool chunk) +static int mspro_block_issue_req(struct memstick_dev *card) { struct mspro_block_data *msb = memstick_get_drvdata(card); u64 t_off; unsigned int count; - while (chunk) { + while (true) { msb->current_page = 0; msb->current_seg = 0; msb->seg_count = blk_rq_map_sg(msb->block_req->q, @@ -709,6 +709,7 @@ static int mspro_block_issue_req(struct memstick_dev *card, bool chunk) if (!msb->seg_count) { unsigned int bytes = blk_rq_cur_bytes(msb->block_req); + bool chunk; chunk = blk_update_request(msb->block_req, BLK_STS_RESOURCE, @@ -718,7 +719,7 @@ static int mspro_block_issue_req(struct memstick_dev *card, bool chunk) __blk_mq_end_request(msb->block_req, BLK_STS_RESOURCE); msb->block_req = NULL; - break; + return -EAGAIN; } t_off = blk_rq_pos(msb->block_req); @@ -735,8 +736,6 @@ static int mspro_block_issue_req(struct memstick_dev *card, bool chunk) memstick_new_req(card->host); return 0; } - - return 1; } static int mspro_block_complete_req(struct memstick_dev *card, int error) @@ -779,7 +778,7 @@ static int mspro_block_complete_req(struct memstick_dev *card, int error) chunk = blk_update_request(msb->block_req, errno_to_blk_status(error), t_len); if (chunk) { - error = mspro_block_issue_req(card, chunk); + error = mspro_block_issue_req(card); if (!error) goto out; } else { @@ -849,7 +848,7 @@ static blk_status_t mspro_queue_rq(struct blk_mq_hw_ctx *hctx, msb->block_req = bd->rq; blk_mq_start_request(bd->rq); - if (mspro_block_issue_req(card, true)) + if (mspro_block_issue_req(card)) msb->block_req = NULL; spin_unlock_irq(&msb->q_lock);
We accidentally changed the error code from -EAGAIN to 1 when we did the blk-mq conversion. Maybe a contributing factor to this mistake is that it wasn't obvious that the "while (chunk) {" condition is always true. I have cleaned that up as well. Fixes: d0be12274dad ("mspro_block: convert to blk-mq") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- drivers/memstick/core/mspro_block.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)