[PATCH] scsi: make scsi_devinfo infrastructure optional

[PATCH] scsi: make scsi_devinfo infrastructure optional

[Bartlomiej Zolnierkiewicz]

Add SCSI_QUIRKS config option (default y and dependent on EMBEDDED
config option) to allow disabling of scsi_devinfo infrastructure.

The output code size savings are ~14k for CONFIG_SCSI_QUIRKS=n
(as measured on x86-32):

CONFIG_SCSI_QUIRKS=y:
   text    data     bss     dec     hex filename
  47342    4368     628   52338    cc72 drivers/scsi/scsi_mod.o
   3897    2776     260    6933    1b15 drivers/scsi/scsi_devinfo.o

CONFIG_SCSI_QUIRKS=n:
   text    data     bss     dec     hex filename
  42968    1592     368   44928    af80 drivers/scsi/scsi_mod.o

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
Using CONFIG_SCSI_QUIRKS=n it was possible to shrink the binary
code size of the minimal SCSI core configuration by 14% (as measured
on x86-32 few kernel versions ago)..

 drivers/scsi/Kconfig     |    9 +++++++++
 drivers/scsi/Makefile    |    3 ++-
 drivers/scsi/scsi_priv.h |   33 +++++++++++++++++++++++++++++++++
 3 files changed, 44 insertions(+), 1 deletion(-)

Index: b/drivers/scsi/Kconfig
===================================================================
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -47,6 +47,15 @@ config SCSI_TGT
 	  If you want to use SCSI target mode drivers enable this option.
 	  If you choose M, the module will be called scsi_tgt.
 
+config SCSI_QUIRKS
+	bool "SCSI quirks support" if EMBEDDED
+	default y
+	help
+	  This option enables support for devices that require settings
+	  that differ from the default.
+
+	  If unsure say Y.
+
 config SCSI_NETLINK
 	bool
 	default	n
Index: b/drivers/scsi/Makefile
===================================================================
--- a/drivers/scsi/Makefile
+++ b/drivers/scsi/Makefile
@@ -160,7 +160,8 @@ obj-$(CONFIG_SCSI_WAIT_SCAN)	+= scsi_wai
 scsi_mod-y			+= scsi.o hosts.o scsi_ioctl.o constants.o \
 				   scsicam.o scsi_error.o scsi_lib.o
 scsi_mod-$(CONFIG_SCSI_DMA)	+= scsi_lib_dma.o
-scsi_mod-y			+= scsi_scan.o scsi_sysfs.o scsi_devinfo.o
+scsi_mod-y			+= scsi_scan.o scsi_sysfs.o
+scsi_mod-$(CONFIG_SCSI_QUIRKS)	+= scsi_devinfo.o
 scsi_mod-$(CONFIG_SCSI_NETLINK)	+= scsi_netlink.o
 scsi_mod-$(CONFIG_SYSCTL)	+= scsi_sysctl.o
 scsi_mod-$(CONFIG_SCSI_PROC_FS)	+= scsi_proc.o
Index: b/drivers/scsi/scsi_priv.h
===================================================================
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -47,6 +47,7 @@ enum {
 	SCSI_DEVINFO_SPI,
 };
 
+#ifdef CONFIG_SCSI_QUIRKS
 extern int scsi_get_device_flags(struct scsi_device *sdev,
 				 const unsigned char *vendor,
 				 const unsigned char *model);
@@ -61,6 +62,38 @@ extern int scsi_dev_info_remove_list(int
 
 extern int __init scsi_init_devinfo(void);
 extern void scsi_exit_devinfo(void);
+#else
+static inline int scsi_get_device_flags(struct scsi_device *sdev,
+					const unsigned char *vendor,
+					const unsigned char *model)
+{
+	return 0;
+}
+static inline int scsi_get_device_flags_keyed(struct scsi_device *sdev,
+					      const unsigned char *vendor,
+					      const unsigned char *model,
+					      int key)
+{
+	return 0;
+}
+static inline int scsi_dev_info_list_add_keyed(int compatible, char *vendor,
+					       char *model, char *strflags,
+					       int flags, int key)
+{
+	return 0;
+}
+static inline int scsi_dev_info_add_list(int key, const char *name)
+{
+	return 0;
+}
+static inline int scsi_dev_info_remove_list(int key)
+{
+	return 0;
+}
+
+static inline int scsi_init_devinfo(void) { return 0; }
+static inline void scsi_exit_devinfo(void) { }
+#endif
 
 /* scsi_error.c */
 extern enum blk_eh_timer_return scsi_times_out(struct request *req);

Re: [PATCH] scsi: make scsi_devinfo infrastructure optional

[James Bottomley]

On Wed, 2011-02-09 at 15:15 +0100, Bartlomiej Zolnierkiewicz wrote:
> Add SCSI_QUIRKS config option (default y and dependent on EMBEDDED
> config option) to allow disabling of scsi_devinfo infrastructure.
> 
> The output code size savings are ~14k for CONFIG_SCSI_QUIRKS=n
> (as measured on x86-32):

I don't understand the point of this patch ... without the quirks SCSI
will do the wrong thing on a whole bunch of stuff.  The savings look to
be tiny ... since the SCSI module is habitually a lot larger than your
figures suggest.

James

Re: [PATCH] scsi: make scsi_devinfo infrastructure optional

[Bartlomiej Zolnierkiewicz]

On Wed, Feb 9, 2011 at 7:00 PM, James Bottomley <James.Bottomley@suse.de> wrote:
> On Wed, 2011-02-09 at 15:15 +0100, Bartlomiej Zolnierkiewicz wrote:
>> Add SCSI_QUIRKS config option (default y and dependent on EMBEDDED
>> config option) to allow disabling of scsi_devinfo infrastructure.
>>
>> The output code size savings are ~14k for CONFIG_SCSI_QUIRKS=n
>> (as measured on x86-32):
>
> I don't understand the point of this patch ... without the quirks SCSI
> will do the wrong thing on a whole bunch of stuff.  The savings look to
> be tiny ... since the SCSI module is habitually a lot larger than your
> figures suggest.

The patch was originally done for embedded ATA-only setups.

Thanks,
Bartlomiej

Re: [PATCH] scsi: make scsi_devinfo infrastructure optional

[James Bottomley]

On Thu, 2011-02-10 at 10:18 +0100, Bartlomiej Zolnierkiewicz wrote:
> On Wed, Feb 9, 2011 at 7:00 PM, James Bottomley <James.Bottomley@suse.de> wrote:
> > On Wed, 2011-02-09 at 15:15 +0100, Bartlomiej Zolnierkiewicz wrote:
> >> Add SCSI_QUIRKS config option (default y and dependent on EMBEDDED
> >> config option) to allow disabling of scsi_devinfo infrastructure.
> >>
> >> The output code size savings are ~14k for CONFIG_SCSI_QUIRKS=n
> >> (as measured on x86-32):
> >
> > I don't understand the point of this patch ... without the quirks SCSI
> > will do the wrong thing on a whole bunch of stuff.  The savings look to
> > be tiny ... since the SCSI module is habitually a lot larger than your
> > figures suggest.
> 
> The patch was originally done for embedded ATA-only setups.

Well, if it's for ATA only then the better course would be extracting
libata from scsi.  It's also a bit misleading to do sizings on x86,
because that doesn't imply embedded to me.  Aren't there still ATAPI
devices that require the quirks?

Most embedded setups include some form of USB ... again, the pluggable
CD/DVD use the quirks table.

Given the potential for disaster even on embedded systems, I don't
really think something like this is a good idea.

James

Re: [PATCH] scsi: make scsi_devinfo infrastructure optional

[Bartlomiej Zolnierkiewicz]

On Thu, Feb 10, 2011 at 4:07 PM, James Bottomley
<James.Bottomley@suse.de> wrote:
> On Thu, 2011-02-10 at 10:18 +0100, Bartlomiej Zolnierkiewicz wrote:
>> On Wed, Feb 9, 2011 at 7:00 PM, James Bottomley <James.Bottomley@suse.de> wrote:
>> > On Wed, 2011-02-09 at 15:15 +0100, Bartlomiej Zolnierkiewicz wrote:
>> >> Add SCSI_QUIRKS config option (default y and dependent on EMBEDDED
>> >> config option) to allow disabling of scsi_devinfo infrastructure.
>> >>
>> >> The output code size savings are ~14k for CONFIG_SCSI_QUIRKS=n
>> >> (as measured on x86-32):
>> >
>> > I don't understand the point of this patch ... without the quirks SCSI
>> > will do the wrong thing on a whole bunch of stuff.  The savings look to
>> > be tiny ... since the SCSI module is habitually a lot larger than your
>> > figures suggest.
>>
>> The patch was originally done for embedded ATA-only setups.
>
> Well, if it's for ATA only then the better course would be extracting
> libata from scsi.  It's also a bit misleading to do sizings on x86,
> because that doesn't imply embedded to me.  Aren't there still ATAPI
> devices that require the quirks?

According to my knowledge all ATAPI quirks are handled locally in libata & sr.

> Most embedded setups include some form of USB ... again, the pluggable
> CD/DVD use the quirks table.

This was done long time ago specifically for embedded 486-like
embedded system w/o USB support and only using flash storage but
indeed this is not a common case.

> Given the potential for disaster even on embedded systems, I don't
> really think something like this is a good idea.

Well, I don't insist on applying it upstream as it is, it is more to
show the direction where the possible room for improvements is in case
of older/embedded systems and reducing memory/code size usage.  [
There were some concerns about it during recent proposal to use more
generalized code for support of some rare Intel-like PATA chipsets
(which seem to cost ~20k as measured on x86-64 in terms of additional
memory/code requirements, though most such systems are x86-32 only so
the incurred cost is probably smaller) .]

BTW with some effort we can do on-demand quirk table loading if it
ever grows too big in the future.

Thanks,
Bartlomiej

Re: [PATCH] scsi: make scsi_devinfo infrastructure optional

[James Bottomley]

On Fri, 2011-02-11 at 10:52 +0100, Bartlomiej Zolnierkiewicz wrote:
> On Thu, Feb 10, 2011 at 4:07 PM, James Bottomley
> <James.Bottomley@suse.de> wrote:
> > On Thu, 2011-02-10 at 10:18 +0100, Bartlomiej Zolnierkiewicz wrote:
> >> On Wed, Feb 9, 2011 at 7:00 PM, James Bottomley <James.Bottomley@suse.de> wrote:
> >> > On Wed, 2011-02-09 at 15:15 +0100, Bartlomiej Zolnierkiewicz wrote:
> >> >> Add SCSI_QUIRKS config option (default y and dependent on EMBEDDED
> >> >> config option) to allow disabling of scsi_devinfo infrastructure.
> >> >>
> >> >> The output code size savings are ~14k for CONFIG_SCSI_QUIRKS=n
> >> >> (as measured on x86-32):
> >> >
> >> > I don't understand the point of this patch ... without the quirks SCSI
> >> > will do the wrong thing on a whole bunch of stuff.  The savings look to
> >> > be tiny ... since the SCSI module is habitually a lot larger than your
> >> > figures suggest.
> >>
> >> The patch was originally done for embedded ATA-only setups.
> >
> > Well, if it's for ATA only then the better course would be extracting
> > libata from scsi.  It's also a bit misleading to do sizings on x86,
> > because that doesn't imply embedded to me.  Aren't there still ATAPI
> > devices that require the quirks?
> 
> According to my knowledge all ATAPI quirks are handled locally in libata & sr.

Then look again: libata handles the transport quirks and sr handles the
capability quirks.  SCSI handles the protocol quirks.

> > Most embedded setups include some form of USB ... again, the pluggable
> > CD/DVD use the quirks table.
> 
> This was done long time ago specifically for embedded 486-like
> embedded system w/o USB support and only using flash storage but
> indeed this is not a common case.
> 
> > Given the potential for disaster even on embedded systems, I don't
> > really think something like this is a good idea.
> 
> Well, I don't insist on applying it upstream as it is, it is more to
> show the direction where the possible room for improvements is in case
> of older/embedded systems and reducing memory/code size usage.  [
> There were some concerns about it during recent proposal to use more
> generalized code for support of some rare Intel-like PATA chipsets
> (which seem to cost ~20k as measured on x86-64 in terms of additional
> memory/code requirements, though most such systems are x86-32 only so
> the incurred cost is probably smaller) .]
> 
> BTW with some effort we can do on-demand quirk table loading if it
> ever grows too big in the future.

But we already have that, if you look.  Part of what you're removing is
the proc interface that allows on demand loading.  If you just want the
ability to compile out the built in table and load everything from boot,
that's about a two line patch.

James

Re: [PATCH] scsi: make scsi_devinfo infrastructure optional

[Bartlomiej Zolnierkiewicz]

James Bottomley wrote:

> On Fri, 2011-02-11 at 10:52 +0100, Bartlomiej Zolnierkiewicz wrote:
> > On Thu, Feb 10, 2011 at 4:07 PM, James Bottomley
> > <James.Bottomley@suse.de> wrote:
> > > On Thu, 2011-02-10 at 10:18 +0100, Bartlomiej Zolnierkiewicz wrote:
> > >> On Wed, Feb 9, 2011 at 7:00 PM, James Bottomley <James.Bottomley@suse.de> wrote:
> > >> > On Wed, 2011-02-09 at 15:15 +0100, Bartlomiej Zolnierkiewicz wrote:
> > >> >> Add SCSI_QUIRKS config option (default y and dependent on EMBEDDED
> > >> >> config option) to allow disabling of scsi_devinfo infrastructure.
> > >> >>
> > >> >> The output code size savings are ~14k for CONFIG_SCSI_QUIRKS=n
> > >> >> (as measured on x86-32):
> > >> >
> > >> > I don't understand the point of this patch ... without the quirks SCSI
> > >> > will do the wrong thing on a whole bunch of stuff.  The savings look to
> > >> > be tiny ... since the SCSI module is habitually a lot larger than your
> > >> > figures suggest.
> > >>
> > >> The patch was originally done for embedded ATA-only setups.
> > >
> > > Well, if it's for ATA only then the better course would be extracting
> > > libata from scsi.  It's also a bit misleading to do sizings on x86,
> > > because that doesn't imply embedded to me.  Aren't there still ATAPI
> > > devices that require the quirks?
> > 
> > According to my knowledge all ATAPI quirks are handled locally in libata & sr.
> 
> Then look again: libata handles the transport quirks and sr handles the
> capability quirks.  SCSI handles the protocol quirks.

I'm aware of this.  What I meant was that I don't know of any real-world 
ATAPI hardware needing the protocol quirks handling in SCSI.

> > > Most embedded setups include some form of USB ... again, the pluggable
> > > CD/DVD use the quirks table.
> > 
> > This was done long time ago specifically for embedded 486-like
> > embedded system w/o USB support and only using flash storage but
> > indeed this is not a common case.
> > 
> > > Given the potential for disaster even on embedded systems, I don't
> > > really think something like this is a good idea.
> > 
> > Well, I don't insist on applying it upstream as it is, it is more to
> > show the direction where the possible room for improvements is in case
> > of older/embedded systems and reducing memory/code size usage.  [
> > There were some concerns about it during recent proposal to use more
> > generalized code for support of some rare Intel-like PATA chipsets
> > (which seem to cost ~20k as measured on x86-64 in terms of additional
> > memory/code requirements, though most such systems are x86-32 only so
> > the incurred cost is probably smaller) .]
> > 
> > BTW with some effort we can do on-demand quirk table loading if it
> > ever grows too big in the future.
> 
> But we already have that, if you look.  Part of what you're removing is
> the proc interface that allows on demand loading.  If you just want the
> ability to compile out the built in table and load everything from boot,
> that's about a two line patch.

Indeed!  Though code savings are smaller using this method..

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Subject: [PATCH] scsi: make scsi_devinfo table content optional

Add CONFIG_SCSI_QUIRKS config option (default y and dependent
on EMBEDDED config option) to allow disabling of the default
scsi_devinfo table content.

The output code size savings are ~7.5k for CONFIG_SCSI_QUIRKS=n
(as measured on x86-64):

CONFIG_SCSI_QUIRKS=y:
   text    data     bss     dec     hex filename
   5965    5520     288   11773    2dfd drivers/scsi/scsi_devinfo.o

CONFIG_SCSI_QUIRKS=n:
   text    data     bss     dec     hex filename
   3740      48     288    4076     fec drivers/scsi/scsi_devinfo.o

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/scsi/Kconfig        |    9 +++++++++
 drivers/scsi/scsi_devinfo.c |    2 ++
 2 files changed, 11 insertions(+)

Index: b/drivers/scsi/Kconfig
===================================================================
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -47,6 +47,15 @@ config SCSI_TGT
 	  If you want to use SCSI target mode drivers enable this option.
 	  If you choose M, the module will be called scsi_tgt.
 
+config SCSI_QUIRKS
+	bool "SCSI quirks list" if EMBEDDED
+	default y
+	help
+	  This option enables deprecated in-kernel list of devices
+	  that require settings that differ from the default.
+
+	  If unsure say Y.
+
 config SCSI_NETLINK
 	bool
 	default	n
Index: b/drivers/scsi/scsi_devinfo.c
===================================================================
--- a/drivers/scsi/scsi_devinfo.c
+++ b/drivers/scsi/scsi_devinfo.c
@@ -53,6 +53,7 @@ static struct {
 	char *revision;	/* revision known to be bad, unused */
 	unsigned flags;
 } scsi_static_device_list[] __initdata = {
+#ifdef CONFIG_SCSI_QUIRKS
 	/*
 	 * The following devices are known not to tolerate a lun != 0 scan
 	 * for one reason or another. Some will respond to all luns,
@@ -251,6 +252,7 @@ static struct {
 	{"XYRATEX", "RS", "*", BLIST_SPARSELUN | BLIST_LARGELUN},
 	{"Zzyzx", "RocketStor 500S", NULL, BLIST_SPARSELUN},
 	{"Zzyzx", "RocketStor 2000", NULL, BLIST_SPARSELUN},
+#endif
 	{ NULL, NULL, NULL, 0 },
 };
 

Re: [PATCH] scsi: make scsi_devinfo infrastructure optional

[Marcin Slusarz]

On Sat, Feb 12, 2011 at 01:16:50PM +0100, Bartlomiej Zolnierkiewicz wrote:
> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Subject: [PATCH] scsi: make scsi_devinfo table content optional
> 
> Add CONFIG_SCSI_QUIRKS config option (default y and dependent
> on EMBEDDED config option) to allow disabling of the default
> scsi_devinfo table content.

Note: all uses of CONFIG_EMBEDDED were recently moved to CONFIG_EXPERT.
(see commit 6a108a14fa356ef607be308b68337939e56ea94e)
This one should probably use it too.

> 
> The output code size savings are ~7.5k for CONFIG_SCSI_QUIRKS=n
> (as measured on x86-64):
> 
> CONFIG_SCSI_QUIRKS=y:
>    text    data     bss     dec     hex filename
>    5965    5520     288   11773    2dfd drivers/scsi/scsi_devinfo.o
> 
> CONFIG_SCSI_QUIRKS=n:
>    text    data     bss     dec     hex filename
>    3740      48     288    4076     fec drivers/scsi/scsi_devinfo.o
> 
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> ---
>  drivers/scsi/Kconfig        |    9 +++++++++
>  drivers/scsi/scsi_devinfo.c |    2 ++
>  2 files changed, 11 insertions(+)
> 
> Index: b/drivers/scsi/Kconfig
> ===================================================================
> --- a/drivers/scsi/Kconfig
> +++ b/drivers/scsi/Kconfig
> @@ -47,6 +47,15 @@ config SCSI_TGT
>  	  If you want to use SCSI target mode drivers enable this option.
>  	  If you choose M, the module will be called scsi_tgt.
>  
> +config SCSI_QUIRKS
> +	bool "SCSI quirks list" if EMBEDDED
> +	default y
> +	help
> +	  This option enables deprecated in-kernel list of devices
> +	  that require settings that differ from the default.
> +
> +	  If unsure say Y.
> +
>  config SCSI_NETLINK
>  	bool
>  	default	n
> Index: b/drivers/scsi/scsi_devinfo.c
> ===================================================================
> --- a/drivers/scsi/scsi_devinfo.c
> +++ b/drivers/scsi/scsi_devinfo.c
> @@ -53,6 +53,7 @@ static struct {
>  	char *revision;	/* revision known to be bad, unused */
>  	unsigned flags;
>  } scsi_static_device_list[] __initdata = {
> +#ifdef CONFIG_SCSI_QUIRKS
>  	/*
>  	 * The following devices are known not to tolerate a lun != 0 scan
>  	 * for one reason or another. Some will respond to all luns,
> @@ -251,6 +252,7 @@ static struct {
>  	{"XYRATEX", "RS", "*", BLIST_SPARSELUN | BLIST_LARGELUN},
>  	{"Zzyzx", "RocketStor 500S", NULL, BLIST_SPARSELUN},
>  	{"Zzyzx", "RocketStor 2000", NULL, BLIST_SPARSELUN},
> +#endif
>  	{ NULL, NULL, NULL, 0 },
>  };
>  
> --