All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jens Axboe <axboe@kernel.dk>
To: Muthu Kumar <muthu.lkml@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	linux-kernel@vger.kernel.org, torvalds@linux-foundation.org,
	martin.petersen@oracle.com
Subject: Re: [PATCH] Fix setting bio flags in drivers (sd_dif/floppy).
Date: Thu, 01 Mar 2012 21:25:34 +0100	[thread overview]
Message-ID: <4F4FDB3E.7000405@kernel.dk> (raw)
In-Reply-To: <CAFR8uedyn_dmE1NPk_H8_vb_ohYcScaG2whEa9=aOcSRWDvcxg@mail.gmail.com>

On 2012-03-01 21:23, Muthu Kumar wrote:
>>>                       kunmap_atomic(sdt, KM_USER0);
>>>               }
>>>
>>> -             bio->bi_flags |= BIO_MAPPED_INTEGRITY;
>>> +             bio->bi_flags |= (1 << BIO_MAPPED_INTEGRITY);
>>>       }
>>>
>>>       return 0;
>>
>> urgh.  This isn't the first time.
>>
>> It's too easy for people to make this mistake.  I'm not sure what a
>> good fix would be - I don't think sparse can save us with __bitwise or
>> similar.
>>
>> The approach we took in buffer_head.h with BH_Foo and BUFFER_FNS
>> accessors worked pretty well.
>>
> 
> Does this look good? BTW, I used the non-atomic variants __set/__clear
> to match the behavior of current usage.
> If this looks good, I can send the proper patch as attachment (so no
> line wraps :)

Lets not wrap this in macros, it just makes the code harder to read.
Making the BIO_ flags a bit shift value was a mistake in hindsight, too
easy to get wrong.

If we're going to be changing everything anyway, it'd be better to have
__ variants if we really need the bit shifts, and the non __ variants be
the value. Similar to what is done for request flags.

-- 
Jens Axboe


  reply	other threads:[~2012-03-01 20:25 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-02-29 23:22 [PATCH] Fix setting bio flags in drivers (sd_dif/floppy) Muthu Kumar
2012-03-01  0:08 ` Linus Torvalds
2012-03-01  0:20   ` Andrew Morton
2012-03-01  8:23   ` Jens Axboe
2012-03-01  0:12 ` Andrew Morton
2012-03-01  0:22   ` Martin K. Petersen
2012-03-01 20:23   ` Muthu Kumar
2012-03-01 20:25     ` Jens Axboe [this message]
2012-03-01 21:20       ` Muthu Kumar
2012-03-01 22:03         ` Jens Axboe
2012-03-01 23:27           ` Muthu Kumar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4F4FDB3E.7000405@kernel.dk \
    --to=axboe@kernel.dk \
    --cc=akpm@linux-foundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=martin.petersen@oracle.com \
    --cc=muthu.lkml@gmail.com \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.