CONFIG_BLK_DEV_BSG=n

CONFIG_BLK_DEV_BSG=n

[Medve Emilian-EMMEDVE1]

Hello,


I'm trying to get powerpc to build without block device support
(CONFIG_BLOCK=3Dn). I'm getting the following errors:

  CC      arch/powerpc/kernel/setup_32.o
In file included from include/linux/blkdev.h:17,
                 from include/linux/ide.h:13,
                 from arch/powerpc/kernel/setup_32.c:13:
include/linux/bsg.h:67: warning: 'struct request_queue' declared inside
parameter list
include/linux/bsg.h:67: warning: its scope is only this definition or
declaration, which is probably not what you want
include/linux/bsg.h:71: warning: 'struct request_queue' declared inside
parameter list
In file included from arch/powerpc/kernel/setup_32.c:13:
include/linux/ide.h:857: error: field 'wrq' has incomplete type

  CC      arch/powerpc/kernel/ppc_ksyms.o
In file included from include/linux/blkdev.h:17,
                 from include/linux/ide.h:13,
                 from arch/powerpc/kernel/ppc_ksyms.c:15:
include/linux/bsg.h:67: warning: 'struct request_queue' declared inside
parameter list
include/linux/bsg.h:67: warning: its scope is only this definition or
declaration, which is probably not what you want
include/linux/bsg.h:71: warning: 'struct request_queue' declared inside
parameter list
In file included from arch/powerpc/kernel/ppc_ksyms.c:15:
include/linux/ide.h:857: error: field 'wrq' has incomplete type

I fixed the errors with a small patch in the powerpc code only and I'm
comfortable with that. The matter I wanted your input on is the warnings
from bsg.h coming from this are of the file:

	...
#ifdef __KERNEL__

#if defined(CONFIG_BLK_DEV_BSG)
struct bsg_class_device {
        struct class_device *class_dev;
        struct device *dev;
        int minor;
        struct request_queue *queue;
};

extern int bsg_register_queue(struct request_queue *, struct device *,
const char *);
extern void bsg_unregister_queue(struct request_queue *);
#else
static inline int bsg_register_queue(struct request_queue * rq, struct
device *dev, const char *name
)
{
        return 0;
}
static inline void bsg_unregister_queue(struct request_queue *rq)
{
}
#endif

#endif /* __KERNEL__ */
	...

I noticed that the '#else' branch was last updated by James (a4ee0df8)
in order to address some other warnings in scsi_sysfs.c, for example, in
the next piece of code:

	...
        error =3D bsg_register_queue(rq, &sdev->sdev_gendev, NULL);

        if (error)
                sdev_printk(KERN_INFO, sdev,
                            "Failed to register bsg queue, =
errno=3D%d\n",
error);

        /* we're treating error on bsg register as non-fatal, so pretend
         * nothing went wrong */
        error =3D 0;
	...

The quick fix to those warnings is to add a declaration of struct
request_queue in bsg.h something looking like this:

	...
#ifdef __KERNEL__

struct request_queue; <- This is the addition

#if defined(CONFIG_BLK_DEV_BSG)
struct bsg_class_device {
        struct class_device *class_dev;
	...

However, I was wondering if there isn't a cleaner way of doing it. For
example, from the comments in scsi_sysfs.c it looks like it would be
possible not to call bsg_register_queue() at all when
CONFIG_BLK_DEV_BSG=3Dn and get rid of the '#else' branch in bsg.h as I
don't think bsg_register_queue() and bsg_unregister_queue() should be
called when CONFIG_BLK_DEV_BSG=3Dn.

Which solution would you be more comfortable with?


Thanks,
Emil.


This e-mail, and any associated attachments have been classified as:
--------------------------------------------------------------------
[ ] Public
[x] Freescale Semiconductor Internal Use Only
[ ] Freescale Semiconductor Confidential Proprietary

Re: CONFIG_BLK_DEV_BSG=n

[James Bottomley]

On Fri, 2007-09-14 at 12:50 -0700, Medve Emilian-EMMEDVE1 wrote:
> Which solution would you be more comfortable with?

The one which is currently in -mm is this one:

http://git.kernel.org/?p=linux/kernel/git/jejb/scsi-misc-2.6.git;a=commit;h=49892223f7d3a2333ef9e6cbdd526676e1fc517a

James

Re: CONFIG_BLK_DEV_BSG=n

[David Howells]

James Bottomley <James.Bottomley@SteelEye.com> wrote:

> > Which solution would you be more comfortable with?
> 
> The one which is currently in -mm is this one:
> 
> http://git.kernel.org/?p=linux/kernel/git/jejb/scsi-misc-2.6.git;a=commit;h=49892223f7d3a2333ef9e6cbdd526676e1fc517a

In my opinion, this is the wrong fix.  There shouldn't be anything in the
kernel using stuff from bsg.h if CONFIG_BLOCK=n, so there should be an error if
anything tries to.  The correct fix is to exclude the non-userspace-visible
contents of bsg.h with #ifdef CONFIG_BLOCK, not to declare things that we've
tried to make sure specifically aren't declared.

David
---

[PATCH] VFS: Make BSG declarations dependent on CONFIG_BLOCK

From: David Howells <dhowells@redhat.com>

Make BSG function declarations dependent on CONFIG_BLOCK as they are not
compilable if the block layer is compiled out.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 include/linux/bsg.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/include/linux/bsg.h b/include/linux/bsg.h
index 60e377b..28f5d44 100644
--- a/include/linux/bsg.h
+++ b/include/linux/bsg.h
@@ -52,6 +52,7 @@ struct sg_io_v4 {
 };
 
 #ifdef __KERNEL__
+#ifdef CONFIG_BLOCK
 
 #if defined(CONFIG_BLK_DEV_BSG)
 struct bsg_class_device {
@@ -73,6 +74,7 @@ static inline void bsg_unregister_queue(struct request_queue *rq)
 }
 #endif
 
+#endif /* CONFIG_BLOCK */
 #endif /* __KERNEL__ */
 
 #endif

Re: CONFIG_BLK_DEV_BSG=n

[Jens Axboe]

On Mon, Sep 17 2007, David Howells wrote:
> 
> James Bottomley <James.Bottomley@SteelEye.com> wrote:
> 
> > > Which solution would you be more comfortable with?
> > 
> > The one which is currently in -mm is this one:
> > 
> > http://git.kernel.org/?p=linux/kernel/git/jejb/scsi-misc-2.6.git;a=commit;h=49892223f7d3a2333ef9e6cbdd526676e1fc517a
> 
> In my opinion, this is the wrong fix.  There shouldn't be anything in
> the kernel using stuff from bsg.h if CONFIG_BLOCK=n, so there should
> be an error if anything tries to.  The correct fix is to exclude the
> non-userspace-visible contents of bsg.h with #ifdef CONFIG_BLOCK, not
> to declare things that we've tried to make sure specifically aren't
> declared.

I agree, I'll pass your fix on.

-- 
Jens Axboe