On 03/30/2017 12:10 PM, Bart Van Assche wrote:
> On Thu, 2017-03-30 at 09:27 -0600, Jens Axboe wrote:
>> On 03/29/2017 03:32 PM, Bart Van Assche wrote:
>>> Make it possible to check whether or not a block layer queue has
>>> been stopped. Make it possible to start and to run a blk-mq queue
>>> from user space.
>>
>> Looks good, minor nitpicking below.
>>
>>> +static int blk_queue_flags_show(struct seq_file *m, void *v)
>>> +{
>>> + struct request_queue *q = m->private;
>>> + bool sep = false;
>>> + int i;
>>> +
>>> + for (i = 0; i < sizeof(q->queue_flags) * BITS_PER_BYTE; i++) {
>>> + if (!(q->queue_flags & BIT(i)))
>>> + continue;
>>> + if (sep)
>>> + seq_puts(m, " ");
>>> + sep = true;
>>
>> Put that sep = true in the else branch?
>
> I can do that, but that would result in slightly less efficient assembler
> code (additional jump) while stores can be pipelined easily.
It's a sysfs show function, it's not like we care about branching. But we
can leave it as-is, I don't feel that strongly about it at all.
>>> + if (strcmp(op, "run") == 0) {
>>> + blk_mq_run_hw_queues(q, true);
>>> + } else if (strcmp(op, "start") == 0) {
>>> + blk_mq_start_stopped_hw_queues(q, true);
>>> + } else {
>>> + pr_err("%s: unsupported operation %s\n", __func__, op);
>>> + return -EINVAL;
>>> + }
>>
>> You are inconsistent with your braces. I'd prefer no braces for single
>> lines.
>
> Sorry but if braces are used for one side of an if-then-else statement then
> checkpatch wants braces to be used for the other side too, even if that other
> side is only a single line. From the checkpatch source code:
Omar informs me that it's coding style mandated. The block layer generally
does NOT do braces, so I'd prefer to keep it consistent at least. Again, not
really a huge problem, and as I prefaced the original email with, totally
nitpicking.
--
Jens Axboe
