Skip to contentSkip to conversation

SCSI updates for 2.6.25

PATCH In review James Bottomley 7 participants 16 messages Apr 18, 2008 – Apr 28, 2008

Subsystems (5)
Documentation SCSI Kbuild Kconfig +1 more subsystems
lore
Share & tools
lore

James Bottomley

author maintainer
#0 lore raw

PATCH 1/1 SCSI updates for 2.6.25

This is the first piece of our merge for the merge window. I have a bit of other stuff with dependent patches, and I'm sure there will be something else crawling out of the woodwork.

It's the usual grab bag of driver updates and mid-layer changes. There's no new stand out features in this one.

The patch is available here:

master.kernel.org:/pub/scm/linux/kernel/git/jejb/scsi-misc-2.6.git

The short change log is:

Adrian Bunk (5):
qla1280: remove version check
sun3_scsi_vme: add MODULE_LICENSE
st: rename flush_write_buffer()
aic94xx: cleanups
make scsi_end_bidi_request() static

Al Viro (1):
libsas: fix endianness bug in sas_ata

Andi Kleen (1):
Remove random noop unchecked_isa_dma users

Andrew Vasquez (15):
qla2xxx: Update version number to 8.02.01-k1.
qla2xxx: Remove unused member (dma_handle) from srb_t structure.
qla2xxx: Add midlayer target/device reset support.
qla2xxx: Check alternate 'reason' code during GPSC status handling.
qla2xxx: Add Flash Descriptor Table layout support.
qla2xxx: Use PCI-SIG nomenclature for PCIe bandwidth units.
qla2xxx: Cruft cleanup of functions and structures.
qla2xxx: Add hardware trace-logging support.
qla2xxx: Add FC-transport Asynchronous Event Notification support.
qla2xxx: Simplify interrupt handler locking.
qla2xxx: Use an rport's scsi_target_id member consistently throughout driv
qla2xxx: Remove unused and obsolete #define's.
qla2xxx: Add support for host supported speeds FC transport attribute.
qla2xxx: Update copyright banner.
qla2xxx: Update firmware filenames for new ISP parts.

Boaz Harrosh (4):
iscsi: bidi support for iscsi_tcp
iscsi: bidi support at the generic libiscsi level
iscsi: extended cdb support
gdth: remove command accessors

Christof Schmitt (5):
zfcp: Fix error handling for blocked unit for send FCP command
zfcp: Remove zfcp_erp_wait from slave destory handler to fix deadlock
zfcp: Move DBF definitions to private header file
zfcp: Fix handling for boxed port after physical close
zfcp: convert zfcp to use target reset and device reset handler

Darrick J. Wong (2):
aic94xx: Use sas_request_addr() to provide SAS WWN if the adapter lacks on
libsas: Provide a transport-level facility to request SAS addrs

David C Somayajulu (1):
qla4xxx: fix scsi command completion, lun reset and target reset code

Denis Cheng (2):
hosts.c: removed one unneeded shost->active_mode assignment 3w-9xxx, 3w-xxxx: memset not needed in probe

FUJITA Tomonori (40):
bsg: no need to set BSG_F_BLOCK bit in bsg_complete_all_commands
bsg: remove minor in struct bsg_device
bsg: use better helper list functions
bsg: replace kobject_get with blk_get_queue
bsg: takes a ref to struct device in fops->open
tgt: use KMEM_CACHE macro
scsi_debug: remove unnecessary function declarations
scsi_debug: support large non-fake virtual disk
scsi_debug: remove the duplicated code in resp_read and resp_write
scsi_debug: sweep up sdebug_capacity calculation
scsi_debug: remove unnecessary sdebug_store_size
scsi_debug: fix lba and data length calculation bugs
ps3rom: use scsi_build_sense_buffer
stex: use scsi_build_sense_buffer
libata: use scsi_build_sense_buffer
scsi_debug: use scsi_build_sense_buffer
add scsi_build_sense_buffer helper function
scsi_debug: remove unnecessary function declarations
scsi_debug: use list_for_each_entry_safe
scsi_debug: remove unnecessary condition test in devInfoReg
scsi_debug: create new scsi_debug devices at a single place
scsi_debug: remove temporary hack around sscanf for negative values 3w-9xxx: use sg buffer copy helper functions
3w-xxxx: use sg buffer copy helper functions
stex: use sg buffer copy helper functions
aacraid: use sg buffer copy helper functions
ips: use sg buffer copy helper funcitons
simscsi: use sg buffer copy helper funcitons
ps3rom: use sg buffer copy helper funcitons
scsi_debug: use sg buffer copy helper functions
scsi: add wrapper functions for sg buffer copy helper functions
block: add sg buffer copy helper functions
aic79xx: fix IOMMU mapping failure handling
aic7xxx: fix IOMMU mapping failure handling
scsi_debug: use shost_priv macro
scsi_debug: remove unnecessary checking
scsi_debug: remove scsi_debug.h
scsi_debug: stop including drivers/scsi/scsi.h
aacraid: READ_CAPACITY_16 shouldn't trust allocation length in cdb
ips: sg chaining support to the path to non I/O commands

Geert Uytterhoeven (1):
ps3rom: Simplify fill_from_dev_buffer()

Grant Grundler (1):
initio: fix big endian problems for auto request sense

Harihara Kadayam (1):
qla2xxx: Add ISP84XX support.

Harvey Harrison (1):
ch: fix sparse shadowed variable warnings

James Bottomley (9):
ips: remove spurious cpu_to_leX on outX statements
libsas: fix missing inlines in header file
transport_class: BUG if we can't release the attribute container fix barrier failure issue
hptiop: fix header.context usage
wd33c93: fix up cut and paste error
mpt fusion: fix up msi_enable in mpt_suspend
export command allocation and freeing functions independently of the host consolidate command allocation in a single place

James Smart (4):
lpfc 8.2.6 : Update lpfc driver version to 8.2.6 lpfc 8.2.6 : Miscellaneous Fixes
lpfc 8.2.6 : PCI Parity and EEH handling fixes lpfc 8.2.6 : Multiple discovery fixes

Jeff Garzik (2):
gdth: convert to PCI hotplug API
gdth: PCI probe cleanups, prep for PCI hotplug API conversion

Kai Makisara (2):
st: show options currently set in sysfs
st: add option to use SILI in variable block reads

Marcin Slusarz (1):
aacraid, ips: leX_add_cpu conversion

Mark Salyzyn (1):
aacraid: Fix down_interruptible() to check the return value

Martin Peschke (21):
zfcp: fix 31 bit compile warnings
zfcp: fix compiler warning caused by poking inside new semaphore (linux-ne
zfcp: Add docbook comments to debug trace.
zfcp: Cleanup line breaks in debug trace.
zfcp: Shorten excessive names in debug trace.
zfcp: Simplify zfcp_dbf_tag and related functions in debug trace.
zfcp: Simplify usage of hex dump output function for debug trace.
zfcp: Remove obsolete output function from debug trace.
zfcp: Cleanup debug trace view functions.
zfcp: simplify zfcp_dbf_timestamp()
zfcp: Remove obsolete erp_dbf trace
zfcp: Add trace records for recovery actions.
zfcp: Trace all triggers of error recovery activity
zfcp: Add traces for state changes.
zfcp: Add trace records for recovery thread and its queues
zfcp: Register new recovery trace.
zfcp: Introduce printf helper functions for debug trace.
zfcp: Add qtcb dump to hba debug trace
zfcp: Remove qtcb dump to kernel log
zfcp: Clean up _zfcp_san_dbf_event_common_els
zfcp: Introduce a helper function that dumps hex data to a zfcp trace.

Matthew Wilcox (1):
BusLogic: make FlashPoint support x86-32 only

Matthias Kaehlcke (1):
mpt fusion: convert inactive_list_mutex to a mutex

Michael Reed (1):

      mptsas: do not use ioc->handle to locate hba portinfo structure

Mike Christie (2):
qla4xxx: Add target reset functionality
scsi_error: add target reset handler

Paul Bolle (1):
aacraid: Do not describe check_reset parameter with its value

Prakash, Sathya (1):
mpt fusion: Enable MSI by default for SAS controllers

Ravi Anand (1):
qla2xxx: Assign mailbox command timeout values in a consistent manner.

Roel Kluin (1):
aic7xxx: Test opcode, not definition in aicasm:type_check()

Seokmann Ju (4):
qla2xxx: Correct vport configuration-change handling.
qla2xxx: Use proper HA during asynchrounous event handling.
qla2xxx: Check DFLG_NO_CABLE only on physical port.
qla2xxx: Consistently access the physical HA port.

Thomas Bogendoerfer (1):
WD33C93: let platform stub override no_sync/fast/dma_mode

bo yang (3):
megaraid_sas: Add the new controller(1078DE) support to the driver
megaraid_sas: Fix the frame count calculation
megaraid_sas: rollback the sense info implementation

and the diffstat:

 Documentation/scsi/st.txt                 |   12 
 arch/ia64/hp/sim/simscsi.c                |   23 
 block/bsg.c                               |   52 -
 drivers/ata/libata-scsi.c                 |    6 
 drivers/base/transport_class.c            |    3 
 drivers/message/fusion/mptbase.c          |   29 
 drivers/message/fusion/mptbase.h          |    5 
 drivers/message/fusion/mptsas.c           |   22 
 drivers/message/fusion/mptscsih.c         |    8 
 drivers/s390/scsi/zfcp_aux.c              |   33 
 drivers/s390/scsi/zfcp_ccw.c              |   24 
 drivers/s390/scsi/zfcp_dbf.c              | 1281 ++++++++++++++++++------------
 drivers/s390/scsi/zfcp_dbf.h              |  228 +++++
 drivers/s390/scsi/zfcp_def.h              |  169 ---
 drivers/s390/scsi/zfcp_erp.c              |  688 +++++-----------
 drivers/s390/scsi/zfcp_ext.h              |   59 -
 drivers/s390/scsi/zfcp_fsf.c              |  397 ++-------
 drivers/s390/scsi/zfcp_qdio.c             |    7 
 drivers/s390/scsi/zfcp_scsi.c             |   69 -
 drivers/s390/scsi/zfcp_sysfs_adapter.c    |   11 
 drivers/s390/scsi/zfcp_sysfs_port.c       |    9 
 drivers/s390/scsi/zfcp_sysfs_unit.c       |    5 
 drivers/scsi/3w-9xxx.c                    |   23 
 drivers/scsi/3w-xxxx.c                    |   14 
 drivers/scsi/BusLogic.c                   |    5 
 drivers/scsi/BusLogic.h                   |   21 
 drivers/scsi/FlashPoint.c                 |    6 
 drivers/scsi/Kconfig                      |   14 
 drivers/scsi/a2091.c                      |    3 
 drivers/scsi/a3000.c                      |    3 
 drivers/scsi/aacraid/aachba.c             |   69 -
 drivers/scsi/aacraid/commsup.c            |   10 
 drivers/scsi/aic7xxx/aic79xx_osm.c        |    7 
 drivers/scsi/aic7xxx/aic7xxx_osm.c        |   10 
 drivers/scsi/aic7xxx/aicasm/aicasm_gram.y |    2 
 drivers/scsi/aic94xx/aic94xx.h            |   16 
 drivers/scsi/aic94xx/aic94xx_dev.c        |    8 
 drivers/scsi/aic94xx/aic94xx_dump.c       |   10 
 drivers/scsi/aic94xx/aic94xx_dump.h       |    9 
 drivers/scsi/aic94xx/aic94xx_hwi.c        |   44 -
 drivers/scsi/aic94xx/aic94xx_hwi.h        |    2 
 drivers/scsi/aic94xx/aic94xx_init.c       |   10 
 drivers/scsi/aic94xx/aic94xx_reg.c        |   53 -
 drivers/scsi/aic94xx/aic94xx_scb.c        |   33 
 drivers/scsi/aic94xx/aic94xx_sds.c        |    4 
 drivers/scsi/aic94xx/aic94xx_seq.c        |   31 
 drivers/scsi/aic94xx/aic94xx_seq.h        |    4 
 drivers/scsi/aic94xx/aic94xx_task.c       |   12 
 drivers/scsi/aic94xx/aic94xx_tmf.c        |    2 
 drivers/scsi/arm/acornscsi.c              |    1 
 drivers/scsi/arm/cumana_1.c               |    1 
 drivers/scsi/ch.c                         |   33 
 drivers/scsi/dc395x.c                     |    1 
 drivers/scsi/eata_pio.c                   |    2 
 drivers/scsi/gdth.c                       |  320 +++----
 drivers/scsi/gdth.h                       |    2 
 drivers/scsi/gvp11.c                      |    3 
 drivers/scsi/hosts.c                      |    1 
 drivers/scsi/hptiop.c                     |    7 
 drivers/scsi/initio.c                     |    9 
 drivers/scsi/ips.c                        |   89 --
 drivers/scsi/iscsi_tcp.c                  |   31 
 drivers/scsi/libiscsi.c                   |  140 ++-
 drivers/scsi/libsas/sas_ata.c             |    2 
 drivers/scsi/libsas/sas_scsi_host.c       |   41 
 drivers/scsi/lpfc/lpfc.h                  |    5 
 drivers/scsi/lpfc/lpfc_attr.c             |   10 
 drivers/scsi/lpfc/lpfc_ct.c               |   48 -
 drivers/scsi/lpfc/lpfc_debugfs.c          |    2 
 drivers/scsi/lpfc/lpfc_els.c              |  121 +-
 drivers/scsi/lpfc/lpfc_hbadisc.c          |   73 -
 drivers/scsi/lpfc/lpfc_init.c             |   98 +-
 drivers/scsi/lpfc/lpfc_nportdisc.c        |   40 
 drivers/scsi/lpfc/lpfc_scsi.c             |   34 
 drivers/scsi/lpfc/lpfc_sli.c              |  112 +-
 drivers/scsi/lpfc/lpfc_version.h          |    2 
 drivers/scsi/lpfc/lpfc_vport.c            |    3 
 drivers/scsi/mac_scsi.c                   |    1 
 drivers/scsi/megaraid/megaraid_sas.c      |   49 -
 drivers/scsi/megaraid/megaraid_sas.h      |    5 
 drivers/scsi/mvme147.c                    |    3 
 drivers/scsi/ps3rom.c                     |  101 --
 drivers/scsi/qla1280.c                    |    5 
 drivers/scsi/qla2xxx/Kconfig              |    3 
 drivers/scsi/qla2xxx/qla_attr.c           |   36 
 drivers/scsi/qla2xxx/qla_dbg.c            |  124 --
 drivers/scsi/qla2xxx/qla_dbg.h            |   23 
 drivers/scsi/qla2xxx/qla_def.h            |   78 +
 drivers/scsi/qla2xxx/qla_dfs.c            |    2 
 drivers/scsi/qla2xxx/qla_fw.h             |  173 +++-
 drivers/scsi/qla2xxx/qla_gbl.h            |   33 
 drivers/scsi/qla2xxx/qla_gs.c             |   16 
 drivers/scsi/qla2xxx/qla_init.c           |  192 +++-
 drivers/scsi/qla2xxx/qla_inline.h         |   87 --
 drivers/scsi/qla2xxx/qla_iocb.c           |    5 
 drivers/scsi/qla2xxx/qla_isr.c            |  223 ++---
 drivers/scsi/qla2xxx/qla_mbx.c            |  318 +++++--
 drivers/scsi/qla2xxx/qla_mid.c            |   30 
 drivers/scsi/qla2xxx/qla_os.c             |  443 +++++-----
 drivers/scsi/qla2xxx/qla_settings.h       |   16 
 drivers/scsi/qla2xxx/qla_sup.c            |  315 +++++--
 drivers/scsi/qla2xxx/qla_version.h        |    6 
 drivers/scsi/qla4xxx/ql4_fw.h             |    4 
 drivers/scsi/qla4xxx/ql4_glbl.h           |    4 
 drivers/scsi/qla4xxx/ql4_iocb.c           |   14 
 drivers/scsi/qla4xxx/ql4_isr.c            |   40 
 drivers/scsi/qla4xxx/ql4_mbx.c            |   39 
 drivers/scsi/qla4xxx/ql4_os.c             |   96 +-
 drivers/scsi/raid_class.c                 |    2 
 drivers/scsi/scsi.c                       |  229 +++--
 drivers/scsi/scsi_debug.c                 | 1219 ++++++++++++----------------
 drivers/scsi/scsi_debug.h                 |   24 
 drivers/scsi/scsi_error.c                 |  150 +++
 drivers/scsi/scsi_lib.c                   |   12 
 drivers/scsi/scsi_tgt_lib.c               |    4 
 drivers/scsi/sgiwd93.c                    |    7 
 drivers/scsi/st.c                         |   89 +-
 drivers/scsi/st.h                         |    3 
 drivers/scsi/st_options.h                 |    6 
 drivers/scsi/stex.c                       |   83 -
 drivers/scsi/sun3_scsi_vme.c              |    1 
 drivers/scsi/wd33c93.c                    |    3 
 include/linux/attribute_container.h       |    2 
 include/linux/mtio.h                      |    1 
 include/linux/scatterlist.h               |    5 
 include/linux/transport_class.h           |    5 
 include/scsi/iscsi_proto.h                |    6 
 include/scsi/libsas.h                     |    2 
 include/scsi/sas_ata.h                    |    4 
 include/scsi/scsi_cmnd.h                  |   17 
 include/scsi/scsi_eh.h                    |    5 
 include/scsi/scsi_host.h                  |    1 
 lib/scatterlist.c                         |  102 ++
 133 files changed, 4873 insertions(+), 4389 deletions(-)

James

#1 lore raw
Quoted 1 line · James Bottomley

libsas: Provide a transport-level facility to request SAS addrs

this (or a nearby) commit caused a build regression:

drivers/built-in.o: In function `sas_request_addr':
: undefined reference to request_firmware' drivers/built-in.o: In function sas_request_addr':
: undefined reference to `release_firmware'

config can be found at:

http://redhat.com/~mingo/misc/config-Sat_Apr_19_16_58_35_CEST_2008.bad

... brought to you by x86.git's randconfig build and boot service ;-)

Ingo

author maintainer
#2 lore raw
Quoted 11 lines · Ingo Molnar
Quoted 1 line · James Bottomley

libsas: Provide a transport-level facility to request SAS addrs

this (or a nearby) commit caused a build regression:

drivers/built-in.o: In function `sas_request_addr':
: undefined reference to request_firmware' drivers/built-in.o: In function sas_request_addr':
: undefined reference to `release_firmware'

Quoted 5 lines

config can be found at:

   http://redhat.com/~mingo/misc/config-Sat_Apr_19_16_58_35_CEST_2008.bad

... brought to you by x86.git's randconfig build and boot service ;-)

This one's fun. The root cause is

CONFIG_SCSI_SAS_LIBSAS=y
CONFIG_FW_LOADER=m

The problem is that libsas doesn't depend on the FW loader and doesn't want to. It just wants to use it if it's available. The definitions in include/linux/firmware.h have stubs to facilitate this.

However, CONFIG_FW_LOADER=m defeats the stubs.

This is a bit nasty to fix; however, I think this patch does. I've also put a large comment in to explain what's going on.

---

include/linux/firmware.h +10 -1
index 4d10c73..01e39a1 100644
@@ -13,7 +13,16 @@ struct firmware {
 
 struct device;
 
-#if defined(CONFIG_FW_LOADER) || defined(CONFIG_FW_LOADER_MODULE)
+
+/*
+ * This is very subtle.  If the Firmware loader is built in then the
+ * request/release calls can be accessed by anything.  If it's built
+ * as a module then only other modules can access it.  The check on
+ * MODULE specifically fixes the case where the firmware loader is
+ * built as a module, but a built in kernel component tries to use the
+ * request/release functions.
+ */
+#if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && defined(MODULE))
 int request_firmware(const struct firmware **fw, const char *name,
 		     struct device *device);
 int request_firmware_nowait(
Tested-by
#3 lore raw
Quoted 18 lines · James Bottomley
Quoted 8 lines

drivers/built-in.o: In function `sas_request_addr':
: undefined reference to `release_firmware'

config can be found at:

   http://redhat.com/~mingo/misc/config-Sat_Apr_19_16_58_35_CEST_2008.bad

... brought to you by x86.git's randconfig build and boot service ;-)

This one's fun. The root cause is

CONFIG_SCSI_SAS_LIBSAS=y
CONFIG_FW_LOADER=m

The problem is that libsas doesn't depend on the FW loader and doesn't want to. It just wants to use it if it's available. The definitions in include/linux/firmware.h have stubs to facilitate this.

Quoted 6 lines

However, CONFIG_FW_LOADER=m defeats the stubs.

This is a bit nasty to fix; however, I think this patch does. I've also put a large comment in to explain what's going on.

thanks James - your fix works fine here.

Ingo

#4 lore raw
In reply to James Bottomley
Quoted 28 lines · James Bottomley
Quoted 11 lines · Ingo Molnar
Quoted 1 line · James Bottomley

libsas: Provide a transport-level facility to request SAS addrs

this (or a nearby) commit caused a build regression:

drivers/built-in.o: In function `sas_request_addr':
: undefined reference to request_firmware' drivers/built-in.o: In function sas_request_addr':
: undefined reference to `release_firmware'

Quoted 5 lines

config can be found at:

   http://redhat.com/~mingo/misc/config-Sat_Apr_19_16_58_35_CEST_2008.bad

... brought to you by x86.git's randconfig build and boot service ;-)

This one's fun. The root cause is

CONFIG_SCSI_SAS_LIBSAS=y
CONFIG_FW_LOADER=m

The problem is that libsas doesn't depend on the FW loader and doesn't want to. It just wants to use it if it's available. The definitions in include/linux/firmware.h have stubs to facilitate this.

Quoted 5 lines

However, CONFIG_FW_LOADER=m defeats the stubs.

This is a bit nasty to fix; however, I think this patch does. I've also put a large comment in to explain what's going on.
...

Your patch fixes the build error, but I'm not sure whether it's the correct fix:

CONFIG_SCSI_SAS_LIBSAS=y, CONFIG_FW_LOADER=m, CONFIG_SCSI_AIC94XX=m will now pass all randconfig tests, but the effects of the "select FW_LOADER" in the SCSI_AIC94XX driver would be lost in this configuration, and we might get bug reports like "driver works built-in but fails with 'No SAS Address provided for %s\n' when built modular."

IMHO SCSI_SAS_LIBSAS should simply select FW_LOADER.

cu
Adrian

Signature · 4 lines
       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed
author maintainer
#5 lore raw
Quoted 38 lines · Adrian Bunk
Quoted 28 lines · James Bottomley
Quoted 11 lines · Ingo Molnar
Quoted 1 line · James Bottomley

libsas: Provide a transport-level facility to request SAS addrs

this (or a nearby) commit caused a build regression:

drivers/built-in.o: In function `sas_request_addr':
: undefined reference to request_firmware' drivers/built-in.o: In function sas_request_addr':
: undefined reference to `release_firmware'

Quoted 5 lines

config can be found at:

   http://redhat.com/~mingo/misc/config-Sat_Apr_19_16_58_35_CEST_2008.bad

... brought to you by x86.git's randconfig build and boot service ;-)

This one's fun. The root cause is

CONFIG_SCSI_SAS_LIBSAS=y
CONFIG_FW_LOADER=m

The problem is that libsas doesn't depend on the FW loader and doesn't want to. It just wants to use it if it's available. The definitions in include/linux/firmware.h have stubs to facilitate this.

Quoted 5 lines

However, CONFIG_FW_LOADER=m defeats the stubs.

This is a bit nasty to fix; however, I think this patch does. I've also put a large comment in to explain what's going on.
...

Your patch fixes the build error, but I'm not sure whether it's the correct fix:

Quoted 7 lines

CONFIG_SCSI_SAS_LIBSAS=y, CONFIG_FW_LOADER=m, CONFIG_SCSI_AIC94XX=m will now pass all randconfig tests, but the effects of the "select FW_LOADER" in the SCSI_AIC94XX driver would be lost in this configuration, and we might get bug reports like "driver works built-in but fails with 'No SAS Address provided for %s\n' when built modular."

IMHO SCSI_SAS_LIBSAS should simply select FW_LOADER.

Well, we already had this argument on linux-scsi when the interface was first proposed: Apart from select being very nasty and should only be sparingly used, this interface is really only provided for specific OS defined cases. It's users are expected to know the pitfalls and the dangers (it's only for boards with no fw in either pre-test or embedded situations), so the casual user should never have to use it.

Even if I changed it to select, a user could still defeat it by making CONFIG_SCSI_AIC94XX=y (because now the initialisation is before early boot userland, so the request always fails).

James

#6 lore raw
Quoted 4 lines · James Bottomley

Well, we already had this argument on linux-scsi when the interface was first proposed: Apart from select being very nasty and should only be sparingly used

I'm still not understanding people who say that.

Yes, select has problems, in that it doesn't guarantee dependencies.

But that said, select is a whole lot better than having simply *different* behaviour (or build errors) depending on totally unrelated config options.

So people: stop this total *idiocy* with "select is bad". It's not. The lack of select is *much* worse.

If you want LIBSAS to have two different modes, how about just making that explicit in the configuration, ie using something like

config LIBSAS_SET_ADDR
bool "Provide user-settable SAS address" depends on SCSI_SAS_LIBSAS
help
This allows a runtime "firmware" loader that loads the SAS address

config LIBSAS_SET_ADDR_FWLOAD
tristate
depends on SCSI_SAS_LIBSAS && LIBSAS_SET_ADDR default y
select FW_LOADER

and now you have the explicit option to turn it on or off - and FW_LOADER is loaded appropriately.

Quoted 3 lines

Even if I changed it to select, a user could still defeat it by making CONFIG_SCSI_AIC94XX=y (because now the initialisation is before early boot userland, so the request always fails).

Not true. That's why we have initrd and ramdisks etc.

Not that any normal people will ever care about LIBSAS, but anyway.. The ones that do care about these kinds of things are probably happy to have special initrd images.

Linus

#7 lore raw
Quoted 2 lines · Linus Torvalds

So people: stop this total *idiocy* with "select is bad". It's not. The lack of select is *much* worse.

Could we consider applying this patch then?

----

'select' not Considered Evil

While select should be used with care, it is not actually evil.

Documentation/kbuild/kconfig-language.txt +3 -3
index 649cb87..dacfcf2 100644
@@ -104,9 +104,9 @@ applicable everywhere (see syntax).
   Reverse dependencies can only be used with boolean or tristate
   symbols.
   Note:
-	select is evil.... select will by brute force set a symbol
-	equal to 'y' without visiting the dependencies. So abusing
-	select you are able to select a symbol FOO even if FOO depends
+	select should be used with care.  It can set a symbol
+	without visiting the dependencies.  By abusing select you are
+	able to select a symbol FOO even if FOO depends
 	on BAR that is not set. In general use select only for
 	non-visible symbols (no prompts anywhere) and for symbols with
 	no dependencies. That will limit the usefulness but on the
Signature · 4 lines
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."
#8 lore raw
Quoted 5 lines · Matthew Wilcox
Documentation/kbuild/kconfig-language.txt +0 -0
@@ -104,9 +104,9 @@ applicable everywhere (see syntax).
   Reverse dependencies can only be used with boolean or tristate
   symbols.

Documentation/kbuild/kconfig-language.txt applicable everywhere (see syntax). · line 112

Quoted 10 lines
Documentation/kbuild/kconfig-language.txt +3 -3
   Note:
-	select is evil.... select will by brute force set a symbol
-	equal to 'y' without visiting the dependencies. So abusing
-	select you are able to select a symbol FOO even if FOO depends
+	select should be used with care.  It can set a symbol
+	without visiting the dependencies.  By abusing select you are
+	able to select a symbol FOO even if FOO depends
 	on BAR that is not set. In general use select only for
 	non-visible symbols (no prompts anywhere) and for symbols with
 	no dependencies. That will limit the usefulness but on the

"It can set a symbol without visiting the dependencies." = imprecise

"select will by brute force set a symbol [...] without visiting the dependencies." = clear. So why not leave this portion of the text as it is.

Maybe change

	"equal to 'y'"
to
	"equal to 'y' or 'm'"
or to
	""

to make it entirely correct.

Signature · 3 lines
Stefan Richter
-=====-==--- -=-- =-=--
http://arcgraph.de/sr/
#9 lore raw
Quoted 16 lines · Stefan Richter
Quoted 9 lines · Matthew Wilcox
Documentation/kbuild/kconfig-language.txt +3 -3
  Note:
-	select is evil.... select will by brute force set a symbol
-	equal to 'y' without visiting the dependencies. So abusing
-	select you are able to select a symbol FOO even if FOO depends
+	select should be used with care.  It can set a symbol
+	without visiting the dependencies.  By abusing select you are
+	able to select a symbol FOO even if FOO depends
Quoted 3 lines

on BAR that is not set. In general use select only for non-visible symbols (no prompts anywhere) and for symbols with no dependencies. That will limit the usefulness but on the

"It can set a symbol without visiting the dependencies." = imprecise

Quoted 2 lines

"select will by brute force set a symbol [...] without visiting the dependencies." = clear. So why not leave this portion of the text as it is.

"brute force" is pejorative, not clear. I'm not really interested in bikeshedding this issue.

Quoted 11 lines

Maybe change

	"equal to 'y'"
to
	"equal to 'y' or 'm'"
or to
	""

to make it entirely correct.

Signature · 3 lines
Stefan Richter
-=====-==--- -=-- =-=--
http://arcgraph.de/sr/
Signature · 4 lines
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."
#10 lore raw
Quoted 1 line · Matthew Wilcox
Quoted 1 line
Quoted 1 line
Documentation/kbuild/kconfig-language.txt +0 -1
-	select is evil.... select will by brute force set a symbol

...

Quoted 1 line
Quoted 1 line
Quoted 1 line
Documentation/kbuild/kconfig-language.txt +1 -0
+	select should be used with care.  It can set a symbol

...

Quoted 1 line

I'm not really interested in bikeshedding this issue.

At least don't replace "will" by "can".

Signature · 3 lines
Stefan Richter
-=====-==--- -=-- =-=--
http://arcgraph.de/sr/
#11 lore raw
In reply to Matthew Wilcox
Quoted 5 lines · Matthew Wilcox
Quoted 2 lines · Linus Torvalds

So people: stop this total *idiocy* with "select is bad". It's not. The lack of select is *much* worse.

Could we consider applying this patch then?

I applied the following.

Sam

commit dfecbec8b54038ef02835d2f8181e1f44bd080d2

Author: Matthew Wilcox <[email protected]>
Date:   Sat Apr 19 14:45:11 2008 -0600

kconifg: 'select' considered less evil

While select should be used with care, it is not actually evil.

Documentation/kbuild/kconfig-language.txt +9 -8
index 649cb87..00b950d 100644
@@ -104,14 +104,15 @@ applicable everywhere (see syntax).
   Reverse dependencies can only be used with boolean or tristate
   symbols.
   Note:
-	select is evil.... select will by brute force set a symbol
-	equal to 'y' without visiting the dependencies. So abusing
-	select you are able to select a symbol FOO even if FOO depends
-	on BAR that is not set. In general use select only for
-	non-visible symbols (no prompts anywhere) and for symbols with
-	no dependencies. That will limit the usefulness but on the
-	other hand avoid the illegal configurations all over. kconfig
-	should one day warn about such things.
+	select should be used with care. select will force
+	a symbol to a value without visiting the dependencies.
+	By abusing select you are able to select a symbol FOO even
+	if FOO depends on BAR that is not set.
+	In general use select only for non-visible symbols
+	(no prompts anywhere) and for symbols with no dependencies.
+	That will limit the usefulness but on the other hand avoid
+	the illegal configurations all over.
+	kconfig should one day warn about such things.
 
 - numerical ranges: "range" <symbol> <symbol> ["if" <expr>]
   This allows to limit the range of possible input values for int
#12 lore raw
Quoted 8 lines · Sam Ravnborg
Quoted 5 lines · Matthew Wilcox
Quoted 2 lines · Linus Torvalds

So people: stop this total *idiocy* with "select is bad". It's not. The lack of select is *much* worse.

Could we consider applying this patch then?

I applied the following.

Reads much better. Thanks!

Signature · 4 lines
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."
author maintainer
#13 lore raw
In reply to Linus Torvalds
Quoted 7 lines · Linus Torvalds
Quoted 4 lines · James Bottomley

Well, we already had this argument on linux-scsi when the interface was first proposed: Apart from select being very nasty and should only be sparingly used

I'm still not understanding people who say that.

Well, I can ... even though SCSI is one of the major users of select in the tree.

The argument goes that abuse of select is very easy to do ... just over select on a lot of options. The result is pieces of the kernel get built in and you have a devil of a time trying to work out how to get them to be turned off (this is annoying when you get bugs that say without feature X enabled, Y bug happens).

Conversely depends has the opposite problem: config options disappear for no readily apparent reason (at least until you read the Kconfig file, then it becomes apparent).

What I try to do in SCSI is sort our Kconfig features into primary and subsidiary, so primary features (like HBA drivers) are allowed to select subsidiary features (like transport classes). However, for most other cases, we use depends instead.

Quoted 1 line

Yes, select has problems, in that it doesn't guarantee dependencies.

Yes, over use of select trips across this fairly quickly.

Quoted 16 lines

But that said, select is a whole lot better than having simply *different* behaviour (or build errors) depending on totally unrelated config options.

So people: stop this total *idiocy* with "select is bad". It's not. The lack of select is *much* worse.

If you want LIBSAS to have two different modes, how about just making that explicit in the configuration, ie using something like

config LIBSAS_SET_ADDR
bool "Provide user-settable SAS address" depends on SCSI_SAS_LIBSAS
help
This allows a runtime "firmware" loader that loads the SAS address

Quoted 5 lines

config LIBSAS_SET_ADDR_FWLOAD
tristate
depends on SCSI_SAS_LIBSAS && LIBSAS_SET_ADDR default y
select FW_LOADER

Yes, that works for libsas ... but what do we do about the prototypes in firmware.h ... just take away the stub and force everything relying on it to break so we can put in a Kconfig sequence like the above?

Quoted 8 lines

and now you have the explicit option to turn it on or off - and FW_LOADER is loaded appropriately.

Quoted 3 lines

Even if I changed it to select, a user could still defeat it by making CONFIG_SCSI_AIC94XX=y (because now the initialisation is before early boot userland, so the request always fails).

Not true. That's why we have initrd and ramdisks etc.

They don't currently seem to work for compiled in drivers ... we have the same issue with drivers that actually need firmware (like aic94xx or qla2xxx): no-one seems to have figured out how to provide it for them; it seems the device is already initialised and has failed to find its firmware before the initrd/initramfs is running.

Quoted 3 lines

Not that any normal people will ever care about LIBSAS, but anyway.. The ones that do care about these kinds of things are probably happy to have special initrd images.

Right ... that's more or less my argument about how far do we really need to go to make Kconfig foolproof.

James

#14 lore raw
Quoted 7 lines · James Bottomley
Quoted 1 line

Not true. That's why we have initrd and ramdisks etc.

They don't currently seem to work for compiled in drivers ... we have the same issue with drivers that actually need firmware (like aic94xx or qla2xxx): no-one seems to have figured out how to provide it for them; it seems the device is already initialised and has failed to find its firmware before the initrd/initramfs is running.

WorksForMe ... or it did a few months ago. I can re-test it with a modern kernel ... wonder which machine I had that needed it ...

Signature · 4 lines
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."
#15 lore raw
In reply to James Bottomley
Quoted 11 lines · James Bottomley
Quoted 7 lines · Linus Torvalds
Quoted 4 lines · James Bottomley

Well, we already had this argument on linux-scsi when the interface was first proposed: Apart from select being very nasty and should only be sparingly used

I'm still not understanding people who say that.

Well, I can ... even though SCSI is one of the major users of select in the tree.

you are handling it well then :) I tend to be amongst the first ones to notice select trouble (randconfig is quite sensitive to it) and by far the leading area of failure "in practice" is video drivers and sound modules. Networking is an occasional blip on the radar (but that's really just due to its sheer natural size and complexity) and i dont remember any deep SCSI trouble there ever.

pie-in-the-sky: a fundamentally simpler Kconfig space would be nice, with strict hierarchies and where enabling a lower component automatically selects all the upper components as well. The Kconfig language should detect loops in hierarchies. (it already does so in a number of situations)

i.e. we should only describe what depends on what - and the rest would be figured out automatically. In that sense "select" should be a _user action_ that would never pop up in the language at all - and the "selection" would all trickle through automatically via the 'depends on' links without having to mention it in the Kconfig language.

whenever due to a selection that a user made something becomes unavailable, the kconfig tool/GUI should warn about that with an extra confirmation step. I.e. it should all really be analogous to normal user-space package management which has its own dependency resolution process.

Ingo