| Message ID | 20190719113139.4005262-1-arnd@arndb.de |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] blkdev: always export SECTOR_SHIFT | expand |
> +/* > + * The basic unit of block I/O is a sector. It is used in a number of contexts > + * in Linux (blk, bio, genhd). The size of one sector is 512 = 2**9 > + * bytes. Variables of type sector_t represent an offset or size that is a > + * multiple of 512 bytes. Hence these two constants. > + */ > +#ifndef SECTOR_SHIFT > +#define SECTOR_SHIFT 9 > +#endif > +#ifndef SECTOR_SIZE > +#define SECTOR_SIZE (1 << SECTOR_SHIFT) > +#endif While we're at it we really should drop the ifndefs. Otherwise looks good. In fact given that sector_t is in linux/types.h I wonder if these should just move there.
Hi Arnd, On Fri, Jul 19, 2019 at 8:32 PM Arnd Bergmann <arnd@arndb.de> wrote: > > When CONFIG_BLOCK is disabled, SECTOR_SHIFT is unknown, and this leads > to a failure in the testing infrastructure added from commit c93a0368aaa2 > ("kbuild: do not create wrappers for header-test-y"): I think this should be commit 43c78d88036e ("kbuild: compile-test kernel headers to ensure they are self-contained") Thanks. > > In file included from <built-in>:3: > include/linux/iomap.h:76:48: error: use of undeclared identifier 'SECTOR_SHIFT' > return (iomap->addr + pos - iomap->offset) >> SECTOR_SHIFT; > > If we want to keep build testing all headers, the macro needs to > either be defined, or not used. Move it out of the #ifdef > section to ensure it is visible. > > Fixes: db074436f421 ("iomap: move the direct IO code into a separate file") > Link: https://lore.kernel.org/lkml/20190718125509.775525-1-arnd@arndb.de/T/ > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > The discussion about the build testing is still going on, but I promised > to send this version anyway for reference. I see no other header-test > failures in randconfig builds with this patch. > --- -- Best Regards Masahiro Yamada
On Fri, Jul 19, 2019 at 2:13 PM Christoph Hellwig <hch@lst.de> wrote: > > > +/* > > + * The basic unit of block I/O is a sector. It is used in a number of contexts > > + * in Linux (blk, bio, genhd). The size of one sector is 512 = 2**9 > > + * bytes. Variables of type sector_t represent an offset or size that is a > > + * multiple of 512 bytes. Hence these two constants. > > + */ > > +#ifndef SECTOR_SHIFT > > +#define SECTOR_SHIFT 9 > > +#endif > > +#ifndef SECTOR_SIZE > > +#define SECTOR_SIZE (1 << SECTOR_SHIFT) > > +#endif > > While we're at it we really should drop the ifndefs. Good idea. Needs some more build testing then. > Otherwise looks good. > > In fact given that sector_t is in linux/types.h I wonder if these > should just move there. Less sure about that, we don't really have other constants in that file, just typedefs and a few common structures. ARnd
On Fri, Jul 19, 2019 at 2:48 PM Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > Hi Arnd, > > On Fri, Jul 19, 2019 at 8:32 PM Arnd Bergmann <arnd@arndb.de> wrote: > > > > When CONFIG_BLOCK is disabled, SECTOR_SHIFT is unknown, and this leads > > to a failure in the testing infrastructure added from commit c93a0368aaa2 > > ("kbuild: do not create wrappers for header-test-y"): > > I think this should be > > commit 43c78d88036e ("kbuild: compile-test kernel headers to ensure > they are self-contained") Ok, fixing that in the resend without the #ifndef. Arnd
On Fri, Jul 19, 2019 at 3:14 PM Arnd Bergmann <arnd@arndb.de> wrote: > > On Fri, Jul 19, 2019 at 2:13 PM Christoph Hellwig <hch@lst.de> wrote: > > > > > +/* > > > + * The basic unit of block I/O is a sector. It is used in a number of contexts > > > + * in Linux (blk, bio, genhd). The size of one sector is 512 = 2**9 > > > + * bytes. Variables of type sector_t represent an offset or size that is a > > > + * multiple of 512 bytes. Hence these two constants. > > > + */ > > > +#ifndef SECTOR_SHIFT > > > +#define SECTOR_SHIFT 9 > > > +#endif > > > +#ifndef SECTOR_SIZE > > > +#define SECTOR_SIZE (1 << SECTOR_SHIFT) > > > +#endif > > > > While we're at it we really should drop the ifndefs. > > Good idea. Needs some more build testing then. Did not take long: In file included from block/partitions/msdos.c:24: In file included from block/partitions/check.h:3: include/linux/blkdev.h:15:9: error: 'SECTOR_SIZE' macro redefined [-Werror,-Wmacro-redefined] #define SECTOR_SIZE (1 << SECTOR_SHIFT) ^ include/uapi/linux/msdos_fs.h:14:9: note: previous definition is here #define SECTOR_SIZE 512 /* sector size (bytes) */ ^ This could clearly be fixed as well, but I suspect we're better off not touching it any further than necessary. Arnd
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index 0ec4f975437e..9c22d8bc6bf9 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -5,6 +5,19 @@ #include <linux/sched.h> #include <linux/sched/clock.h> +/* + * The basic unit of block I/O is a sector. It is used in a number of contexts + * in Linux (blk, bio, genhd). The size of one sector is 512 = 2**9 + * bytes. Variables of type sector_t represent an offset or size that is a + * multiple of 512 bytes. Hence these two constants. + */ +#ifndef SECTOR_SHIFT +#define SECTOR_SHIFT 9 +#endif +#ifndef SECTOR_SIZE +#define SECTOR_SIZE (1 << SECTOR_SHIFT) +#endif + #ifdef CONFIG_BLOCK #include <linux/major.h> @@ -889,19 +902,6 @@ static inline struct request_queue *bdev_get_queue(struct block_device *bdev) return bdev->bd_disk->queue; /* this is never NULL */ } -/* - * The basic unit of block I/O is a sector. It is used in a number of contexts - * in Linux (blk, bio, genhd). The size of one sector is 512 = 2**9 - * bytes. Variables of type sector_t represent an offset or size that is a - * multiple of 512 bytes. Hence these two constants. - */ -#ifndef SECTOR_SHIFT -#define SECTOR_SHIFT 9 -#endif -#ifndef SECTOR_SIZE -#define SECTOR_SIZE (1 << SECTOR_SHIFT) -#endif - /* * blk_rq_pos() : the current sector * blk_rq_bytes() : bytes left in the entire request
When CONFIG_BLOCK is disabled, SECTOR_SHIFT is unknown, and this leads to a failure in the testing infrastructure added from commit c93a0368aaa2 ("kbuild: do not create wrappers for header-test-y"): In file included from <built-in>:3: include/linux/iomap.h:76:48: error: use of undeclared identifier 'SECTOR_SHIFT' return (iomap->addr + pos - iomap->offset) >> SECTOR_SHIFT; If we want to keep build testing all headers, the macro needs to either be defined, or not used. Move it out of the #ifdef section to ensure it is visible. Fixes: db074436f421 ("iomap: move the direct IO code into a separate file") Link: https://lore.kernel.org/lkml/20190718125509.775525-1-arnd@arndb.de/T/ Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- The discussion about the build testing is still going on, but I promised to send this version anyway for reference. I see no other header-test failures in randconfig builds with this patch. --- include/linux/blkdev.h | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) -- 2.20.0