next-2008-0306 scsi/libsas build failure

next-2008-0306 scsi/libsas build failure

[Randy Dunlap]

drivers/built-in.o: In function `sas_request_addr':
(.text+0x598fb): undefined reference to `request_firmware'
drivers/built-in.o: In function `sas_request_addr':
(.text+0x5998e): undefined reference to `release_firmware'
make[1]: *** [.tmp_vmlinux1] Error 1
make: *** [sub-make] Error 2

drivers/scsi/libsas/sas_scsi_host.c calls request_firmware() and
release_firmware() without checking to see if CONFIG_FW_LOADER=y.

---
~Randy

Re: next-2008-0306 scsi/libsas build failure

[Greg KH]

On Thu, Mar 06, 2008 at 09:09:04AM -0800, Randy Dunlap wrote:
> drivers/built-in.o: In function `sas_request_addr':
> (.text+0x598fb): undefined reference to `request_firmware'
> drivers/built-in.o: In function `sas_request_addr':
> (.text+0x5998e): undefined reference to `release_firmware'
> make[1]: *** [.tmp_vmlinux1] Error 1
> make: *** [sub-make] Error 2
> 
> drivers/scsi/libsas/sas_scsi_host.c calls request_firmware() and
> release_firmware() without checking to see if CONFIG_FW_LOADER=y.

Sounds like a bug in the firmware code, it should provide stubs if this
config option is not present, right?

thanks,

greg k-h

Re: next-2008-0306 scsi/libsas build failure

[Randy Dunlap]

Greg KH wrote:
> On Thu, Mar 06, 2008 at 09:09:04AM -0800, Randy Dunlap wrote:
>> drivers/built-in.o: In function `sas_request_addr':
>> (.text+0x598fb): undefined reference to `request_firmware'
>> drivers/built-in.o: In function `sas_request_addr':
>> (.text+0x5998e): undefined reference to `release_firmware'
>> make[1]: *** [.tmp_vmlinux1] Error 1
>> make: *** [sub-make] Error 2
>>
>> drivers/scsi/libsas/sas_scsi_host.c calls request_firmware() and
>> release_firmware() without checking to see if CONFIG_FW_LOADER=y.
> 
> Sounds like a bug in the firmware code, it should provide stubs if this
> config option is not present, right?

That would be one (good) solution.

I'll wait to see if James or Darrick have other ideas...

-- 
~Randy

Re: next-2008-0306 scsi/libsas build failure

[Darrick J. Wong]

>> Sounds like a bug in the firmware code, it should provide stubs if this
>> config option is not present, right?
>
> That would be one (good) solution.
>
> I'll wait to see if James or Darrick have other ideas...

sas_request_addr should only be called in the (possibly) exceptional
case that a SAS controller forgot its address and wants a sysadmin to
provide one.  However, it's not critical to the operation of libsas, so
in this particular case we can just return an error code if
CONFIG_FW_LOADER=n.

--D

[PATCH] libsas: Check that the firmware loader is present in sas_request_addr

[Darrick J. Wong]

Return an error code in sas_request_addr if the fw loader isn't
configured.

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
---

 drivers/scsi/libsas/sas_scsi_host.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index 601ec5b..e44be7a 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -1085,6 +1085,7 @@ static void sas_parse_addr(u8 *sas_addr, const char *p)
 
 int sas_request_addr(struct Scsi_Host *shost, u8 *addr)
 {
+#ifdef CONFIG_FW_LOADER
 	int res;
 	const struct firmware *fw;
 
@@ -1102,6 +1103,9 @@ int sas_request_addr(struct Scsi_Host *shost, u8 *addr)
 out:
 	release_firmware(fw);
 	return res;
+#else
+	return -ENOENT;
+#endif
 }
 EXPORT_SYMBOL_GPL(sas_request_addr);
 

Re: [PATCH] libsas: Check that the firmware loader is present in sas_request_addr

[James Bottomley]

On Thu, 2008-03-06 at 13:02 -0800, Darrick J. Wong wrote:
> Return an error code in sas_request_addr if the fw loader isn't
> configured.
> 
> Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
> ---
> 
>  drivers/scsi/libsas/sas_scsi_host.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
> index 601ec5b..e44be7a 100644
> --- a/drivers/scsi/libsas/sas_scsi_host.c
> +++ b/drivers/scsi/libsas/sas_scsi_host.c
> @@ -1085,6 +1085,7 @@ static void sas_parse_addr(u8 *sas_addr, const char *p)
>  
>  int sas_request_addr(struct Scsi_Host *shost, u8 *addr)
>  {
> +#ifdef CONFIG_FW_LOADER
>  	int res;
>  	const struct firmware *fw;
>  
> @@ -1102,6 +1103,9 @@ int sas_request_addr(struct Scsi_Host *shost, u8 *addr)
>  out:
>  	release_firmware(fw);
>  	return res;
> +#else
> +	return -ENOENT;
> +#endif
>  }
>  EXPORT_SYMBOL_GPL(sas_request_addr);

We could do it that way ... I suspect what Greg was asking for was more
like this:

James

---

diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index 33d8f20..4d10c73 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -10,7 +10,10 @@ struct firmware {
 	size_t size;
 	u8 *data;
 };
+
 struct device;
+
+#if defined(CONFIG_FW_LOADER) || defined(CONFIG_FW_LOADER_MODULE)
 int request_firmware(const struct firmware **fw, const char *name,
 		     struct device *device);
 int request_firmware_nowait(
@@ -19,4 +22,24 @@ int request_firmware_nowait(
 	void (*cont)(const struct firmware *fw, void *context));
 
 void release_firmware(const struct firmware *fw);
+#else
+static inline int request_firmware(const struct firmware **fw,
+				   const char *name,
+				   struct device *device)
+{
+	return -EINVAL;
+}
+static inline int request_firmware_nowait(
+	struct module *module, int uevent,
+	const char *name, struct device *device, void *context,
+	void (*cont)(const struct firmware *fw, void *context))
+{
+	return -EINVAL;
+}
+
+static inline void release_firmware(const struct firmware *fw)
+{
+}
+#endif
+
 #endif

Re: [PATCH] libsas: Check that the firmware loader is present in sas_request_addr

[Randy Dunlap]

James Bottomley wrote:
> On Thu, 2008-03-06 at 13:02 -0800, Darrick J. Wong wrote:
>> Return an error code in sas_request_addr if the fw loader isn't
>> configured.
>>
>> Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
>> ---
>>
>>  drivers/scsi/libsas/sas_scsi_host.c |    4 ++++
>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
>> index 601ec5b..e44be7a 100644
>> --- a/drivers/scsi/libsas/sas_scsi_host.c
>> +++ b/drivers/scsi/libsas/sas_scsi_host.c
>> @@ -1085,6 +1085,7 @@ static void sas_parse_addr(u8 *sas_addr, const char *p)
>>  
>>  int sas_request_addr(struct Scsi_Host *shost, u8 *addr)
>>  {
>> +#ifdef CONFIG_FW_LOADER
>>  	int res;
>>  	const struct firmware *fw;
>>  
>> @@ -1102,6 +1103,9 @@ int sas_request_addr(struct Scsi_Host *shost, u8 *addr)
>>  out:
>>  	release_firmware(fw);
>>  	return res;
>> +#else
>> +	return -ENOENT;
>> +#endif
>>  }
>>  EXPORT_SYMBOL_GPL(sas_request_addr);
> 
> We could do it that way ... I suspect what Greg was asking for was more
> like this:

Yes, it seems that this is needed.  Lots of drivers use
request_firmware().  Oooooh, and they select FW_LOADER.  :(
That's how they "get around" this problem.

> James
> 
> ---
> 
> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> index 33d8f20..4d10c73 100644
> --- a/include/linux/firmware.h
> +++ b/include/linux/firmware.h
> @@ -10,7 +10,10 @@ struct firmware {
>  	size_t size;
>  	u8 *data;
>  };
> +
>  struct device;
> +
> +#if defined(CONFIG_FW_LOADER) || defined(CONFIG_FW_LOADER_MODULE)
>  int request_firmware(const struct firmware **fw, const char *name,
>  		     struct device *device);
>  int request_firmware_nowait(
> @@ -19,4 +22,24 @@ int request_firmware_nowait(
>  	void (*cont)(const struct firmware *fw, void *context));
>  
>  void release_firmware(const struct firmware *fw);
> +#else
> +static inline int request_firmware(const struct firmware **fw,
> +				   const char *name,
> +				   struct device *device)
> +{
> +	return -EINVAL;
> +}
> +static inline int request_firmware_nowait(
> +	struct module *module, int uevent,
> +	const char *name, struct device *device, void *context,
> +	void (*cont)(const struct firmware *fw, void *context))
> +{
> +	return -EINVAL;
> +}
> +
> +static inline void release_firmware(const struct firmware *fw)
> +{
> +}
> +#endif
> +
>  #endif
> 
> 


-- 
~Randy

Re: [PATCH] libsas: Check that the firmware loader is present in sas_request_addr

[Randy Dunlap]

On Thu, 06 Mar 2008 15:17:13 -0600 James Bottomley wrote:

> We could do it that way ... I suspect what Greg was asking for was more
> like this:
> 
> James

Builds cleanly for CONFIG_FW_LOADER=n.
I'm having trouble setting CONFIG_FW_LOADER=m for test building it.
Anyway,
Acked-by: Randy Dunlap <randy.dunlap@oracle.com>

> ---
> 
> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> index 33d8f20..4d10c73 100644
> --- a/include/linux/firmware.h
> +++ b/include/linux/firmware.h
> @@ -10,7 +10,10 @@ struct firmware {
>  	size_t size;
>  	u8 *data;
>  };
> +
>  struct device;
> +
> +#if defined(CONFIG_FW_LOADER) || defined(CONFIG_FW_LOADER_MODULE)
>  int request_firmware(const struct firmware **fw, const char *name,
>  		     struct device *device);
>  int request_firmware_nowait(
> @@ -19,4 +22,24 @@ int request_firmware_nowait(
>  	void (*cont)(const struct firmware *fw, void *context));
>  
>  void release_firmware(const struct firmware *fw);
> +#else
> +static inline int request_firmware(const struct firmware **fw,
> +				   const char *name,
> +				   struct device *device)
> +{
> +	return -EINVAL;
> +}
> +static inline int request_firmware_nowait(
> +	struct module *module, int uevent,
> +	const char *name, struct device *device, void *context,
> +	void (*cont)(const struct firmware *fw, void *context))
> +{
> +	return -EINVAL;
> +}
> +
> +static inline void release_firmware(const struct firmware *fw)
> +{
> +}
> +#endif
> +
>  #endif

---
~Randy

Re: [PATCH] libsas: Check that the firmware loader is present in sas_request_addr

[James Bottomley]

On Thu, 2008-03-06 at 14:04 -0800, Randy Dunlap wrote:
> On Thu, 06 Mar 2008 15:17:13 -0600 James Bottomley wrote:
> 
> > We could do it that way ... I suspect what Greg was asking for was more
> > like this:
> > 
> > James
> 
> Builds cleanly for CONFIG_FW_LOADER=n.
> I'm having trouble setting CONFIG_FW_LOADER=m for test building it.

Ironically, I can, and have, tested that config ... I just couldn't
build with CONFIG_FW_LOADER=n.

I think we've covered all the test cases between us.

> Anyway,
> Acked-by: Randy Dunlap <randy.dunlap@oracle.com>

Thanks.

James

Re: [PATCH] libsas: Check that the firmware loader is present in sas_request_addr

[Greg KH]

On Thu, Mar 06, 2008 at 04:16:21PM -0600, James Bottomley wrote:
> On Thu, 2008-03-06 at 14:04 -0800, Randy Dunlap wrote:
> > On Thu, 06 Mar 2008 15:17:13 -0600 James Bottomley wrote:
> > 
> > > We could do it that way ... I suspect what Greg was asking for was more
> > > like this:
> > > 
> > > James
> > 
> > Builds cleanly for CONFIG_FW_LOADER=n.
> > I'm having trouble setting CONFIG_FW_LOADER=m for test building it.
> 
> Ironically, I can, and have, tested that config ... I just couldn't
> build with CONFIG_FW_LOADER=n.
> 
> I think we've covered all the test cases between us.
> 
> > Anyway,
> > Acked-by: Randy Dunlap <randy.dunlap@oracle.com>
> 
> Thanks.

Yes, this is what I was thinking was needed.

Care to resend it with a signed-off-by?

thanks,

greg k-h

[PATCH] firmware: provide stubs for the FW_LOADER=n case

[James Bottomley]

libsas has a case where it uses the firmware loader to provide services,
but doesn't want to select it all the time.  This currently causes a
compile failure in libsas if FW_LOADER=n.  Fix this by providing error
stubs for the firmware loader API in the FW_LOADER=n case.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

---

On Thu, 2008-03-06 at 20:54 -0800, Greg KH wrote:
> Yes, this is what I was thinking was needed.
> 
> Care to resend it with a signed-off-by?
> 
> thanks,

Sure, attached.

James

diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index 33d8f20..4d10c73 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -10,7 +10,10 @@ struct firmware {
 	size_t size;
 	u8 *data;
 };
+
 struct device;
+
+#if defined(CONFIG_FW_LOADER) || defined(CONFIG_FW_LOADER_MODULE)
 int request_firmware(const struct firmware **fw, const char *name,
 		     struct device *device);
 int request_firmware_nowait(
@@ -19,4 +22,24 @@ int request_firmware_nowait(
 	void (*cont)(const struct firmware *fw, void *context));
 
 void release_firmware(const struct firmware *fw);
+#else
+static inline int request_firmware(const struct firmware **fw,
+				   const char *name,
+				   struct device *device)
+{
+	return -EINVAL;
+}
+static inline int request_firmware_nowait(
+	struct module *module, int uevent,
+	const char *name, struct device *device, void *context,
+	void (*cont)(const struct firmware *fw, void *context))
+{
+	return -EINVAL;
+}
+
+static inline void release_firmware(const struct firmware *fw)
+{
+}
+#endif
+
 #endif

patch firmware-provide-stubs-for-the-fw_loader-n-case.patch added to gregkh-2.6 tree

[gregkh]

This is a note to let you know that I've just added the patch titled

     Subject: firmware: provide stubs for the FW_LOADER=n case

to my gregkh-2.6 tree.  Its filename is

     firmware-provide-stubs-for-the-fw_loader-n-case.patch

This tree can be found at 
    http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/


>From James.Bottomley@HansenPartnership.com  Fri Mar  7 08:30:11 2008
From: James Bottomley <James.Bottomley@HansenPartnership.com>
Date: Fri, 07 Mar 2008 08:57:54 -0600
Subject: firmware: provide stubs for the FW_LOADER=n case
To: Greg KH <greg@kroah.com>
Cc: Randy Dunlap <randy.dunlap@oracle.com>, "Darrick J. Wong" <djwong@us.ibm.com>, linux-next@vger.kernel.org, scsi <linux-scsi@vger.kernel.org>
Message-ID: <1204901874.2889.3.camel@localhost.localdomain>


libsas has a case where it uses the firmware loader to provide services,
but doesn't want to select it all the time.  This currently causes a
compile failure in libsas if FW_LOADER=n.  Fix this by providing error
stubs for the firmware loader API in the FW_LOADER=n case.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
Cc: Randy Dunlap <randy.dunlap@oracle.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 include/linux/firmware.h |   23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -10,7 +10,10 @@ struct firmware {
 	size_t size;
 	u8 *data;
 };
+
 struct device;
+
+#if defined(CONFIG_FW_LOADER) || defined(CONFIG_FW_LOADER_MODULE)
 int request_firmware(const struct firmware **fw, const char *name,
 		     struct device *device);
 int request_firmware_nowait(
@@ -19,4 +22,24 @@ int request_firmware_nowait(
 	void (*cont)(const struct firmware *fw, void *context));
 
 void release_firmware(const struct firmware *fw);
+#else
+static inline int request_firmware(const struct firmware **fw,
+				   const char *name,
+				   struct device *device)
+{
+	return -EINVAL;
+}
+static inline int request_firmware_nowait(
+	struct module *module, int uevent,
+	const char *name, struct device *device, void *context,
+	void (*cont)(const struct firmware *fw, void *context))
+{
+	return -EINVAL;
+}
+
+static inline void release_firmware(const struct firmware *fw)
+{
+}
+#endif
+
 #endif


Patches currently in gregkh-2.6 which might be from James.Bottomley@HansenPartnership.com are

pci/pci-remove-parisc-consumer-of-the-pci-global_list.patch
usb/usb-enable-usb-persist-by-default.patch
driver-core/firmware-provide-stubs-for-the-fw_loader-n-case.patch