On Mon, Feb 6, 2017 at 10:54 AM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
> On 6 February 2017 at 10:07, Arnd Bergmann <arnd@xxxxxxxx> wrote:
>> On Fri, Feb 3, 2017 at 1:57 PM, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
>>>>> if (mmc_card_removed(card))
>>>>> req->rq_flags |= RQF_QUIET;
>>>>> - while (ret)
>>>>> - ret = blk_end_request(req, -EIO,
>>>>> - blk_rq_cur_bytes(req));
>>>>> + while (blk_end_request(req, -EIO, blk_rq_cur_bytes(req)))
>>>>> + {
>>>>> + }
>>>>
>>>> These brackets isn't needed, so am going to change this before applying.
>>>>
>>>> while (blk_end_request(req, -EIO, blk_rq_cur_bytes(req)));
>>>
>>> Aha you know your C syntax better than me. Looks like something that
>>> checkpatch should catch actually, but I ran this through checkpatch and
>>> it didn't complain.
>>
>> IIRC gcc-7 will warn about Ulf's version, as it is a bit misleading and can
>> lead to subtle bugs like
>>
>> int ret = 0;
>> while (!ret);
>> ret = do_something();
>>
>> which would pass a casual review but is actually an endless loop.
>>
>> Arnd
>
> Okay, seems reasonable. However mine version is already being used at
> some places in the kernel.
>
> Linus version, was actually complained by checkpatch. Perhaps me an
> Linus ran different versions of check patch.
I might have missed it. It complained when I had:
while (cond)
{
};
about the semicolon in the end... maybe there was another complaint
too.
> So, then which version is preferred here?
>
> Perhaps something like this is the best?
>
> while (1) {
> ret = do_something();
> if (!ret)
> break;
> }
Actually in this case the whole while() thing looks a bit bogus:
it is just hammering the block layer to end the request.
I guess since there is no way to return any unfinished status
upward.
True, I preserved that strange semantic, but what should we
really do? Timeout and complain in dmesg?
It just looks dangerous.
Anyways let's address any issues with follow-up patches...
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
