* Re: [PATCH] mspro_block: Fix an error code in mspro_block_issue_req()
2019-05-10 11:24 [PATCH] mspro_block: Fix an error code in mspro_block_issue_req() Dan Carpenter
@ 2019-05-10 13:11 ` walter harms
2019-05-10 13:23 ` Dan Carpenter
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: walter harms @ 2019-05-10 13:11 UTC (permalink / raw)
To: kernel-janitors
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);
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] mspro_block: Fix an error code in mspro_block_issue_req()
2019-05-10 11:24 [PATCH] mspro_block: Fix an error code in mspro_block_issue_req() Dan Carpenter
2019-05-10 13:11 ` walter harms
@ 2019-05-10 13:23 ` Dan Carpenter
2019-05-10 15:29 ` walter harms
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2019-05-10 13:23 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] mspro_block: Fix an error code in mspro_block_issue_req()
2019-05-10 11:24 [PATCH] mspro_block: Fix an error code in mspro_block_issue_req() Dan Carpenter
2019-05-10 13:11 ` walter harms
2019-05-10 13:23 ` Dan Carpenter
@ 2019-05-10 15:29 ` walter harms
2019-05-11 9:36 ` Dan Carpenter
2019-05-28 8:52 ` Ulf Hansson
4 siblings, 0 replies; 6+ messages in thread
From: walter harms @ 2019-05-10 15:29 UTC (permalink / raw)
To: kernel-janitors
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;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mspro_block: Fix an error code in mspro_block_issue_req()
2019-05-10 11:24 [PATCH] mspro_block: Fix an error code in mspro_block_issue_req() Dan Carpenter
` (2 preceding siblings ...)
2019-05-10 15:29 ` walter harms
@ 2019-05-11 9:36 ` Dan Carpenter
2019-05-28 8:52 ` Ulf Hansson
4 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2019-05-11 9:36 UTC (permalink / raw)
To: kernel-janitors
return -EINVAL; is more readable (100% unambiguous) than a break
statement.
ergards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] mspro_block: Fix an error code in mspro_block_issue_req()
2019-05-10 11:24 [PATCH] mspro_block: Fix an error code in mspro_block_issue_req() Dan Carpenter
` (3 preceding siblings ...)
2019-05-11 9:36 ` Dan Carpenter
@ 2019-05-28 8:52 ` Ulf Hansson
4 siblings, 0 replies; 6+ messages in thread
From: Ulf Hansson @ 2019-05-28 8:52 UTC (permalink / raw)
To: kernel-janitors
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread