Re: [VERY EARLY RFC 00/13] Rework SCSI results handling
Johannes, > here's a early preview of my SCSI results rework so we can eventually > discuss things next week at LSF/MM (it still has compiler errors on > aic7xxx and scsi_debug). Here's what I'd like to see: - Forget about cramming everything into that dreaded u32 result. Bart's suggestion of making it a union is fine but it's perpetuating that these fields are one thing. And they really aren't. I'd rather not be confined to the crusty old model throughout SCSI. We can synthesize the u32 result the few places where it's actually required (i.e. returned to userland). - Create separate scsi_cmnd status fields for host, driver, target (SAM), and transport (msg). And maybe block layer errno as suggested. - Fix the naming so it makes sense. No more DID_SPOT_RUN. Let's have HOST_FOO / DRIVER_BAR / SAM_STAT_BAZ so it's clear who said what. - For the first iteration, maybe we can keep the message goo. But ideally I'd like to see the SPI-specifics moved to the SPI transport class. And then have an opaque transport error field in the scsi_cmnd that other transports can use as well. - If the interface naming is sensible, I don't think we need wrappers. But if it helps the driver conversion, I don't have a problem having them as long as they are self-documenting: scsi_set_command_status(HOST_FOO, DRIVER_SENSE, SAM_STAT_BAZ); is infinitely less error prone than: result = (NON_SENSE << 24) | (DID_RUN << 16) | (SPOT_FORGOT_TO_SHIFT); I'll set aside a slot next week... -- Martin K. Petersen Oracle Linux Engineering
Re: [VERY EARLY RFC 00/13] Rework SCSI results handling
On Thu, 2018-04-19 at 10:10 +0200, Johannes Thumshirn wrote: > On Wed, Apr 18, 2018 at 04:00:43PM +, Bart Van Assche wrote: > > Thank you for having come up with this so quickly. Something I do not > > like about this patch series is that several new very short helper functions > > are introduced, e.g. set_scsi_result(), clear_scsi_result(), > > to_scsi_result() > > and from_scsi_result(). If we would make scsi_result a union of a 32-bit > > integer and a struct with the driver, host, msg and status bytes then we > > would not need any of these new helper functions. Additionally, that > > approach > > would allow us to eliminate the {set,get}_{driver,host,msg,status}_byte() > > functions. > > Honestly I don't really like these mini accessor functions as well. > But when using a union we loose all the benefits of the enums as > drivers still can touch the compound result value. Hello Johannes, How about reworking this patch series as follows: - Modify the bsg driver such that it uses another field than the SCSI result for storing its -E... result code. - The next patch changes scsi_result from an int into a union and all SCSI code that sets or uses the result is modified to use the result member of the union. - Modify the SCSI core and all LLDs to access the .driver/.host/.msg/.status bytes of the union instead of .result. - Remove the {set,get}_{driver,host,msg,status}_byte() helper functions. - Remove the .result member of the union. This approach has the following advantages: - No new helper functions have to be introduced. - The LLD changes can be split into one patch per LLD driver and one patch for the SCSI core. I think this will make reviewing a lot easier. Thanks, Bart.
Re: [VERY EARLY RFC 00/13] Rework SCSI results handling
On Thu, 19 Apr 2018 10:10:41 +0200 Johannes Thumshirn wrote: > On Wed, Apr 18, 2018 at 04:00:43PM +, Bart Van Assche wrote: > > Thank you for having come up with this so quickly. Something I do > > not like about this patch series is that several new very short > > helper functions are introduced, e.g. set_scsi_result(), > > clear_scsi_result(), to_scsi_result() and from_scsi_result(). If we > > would make scsi_result a union of a 32-bit integer and a struct > > with the driver, host, msg and status bytes then we would not need > > any of these new helper functions. Additionally, that approach > > would allow us to eliminate the > > {set,get}_{driver,host,msg,status}_byte() functions. > > Honestly I don't really like these mini accessor functions as well. > But when using a union we loose all the benefits of the enums as > drivers still can touch the compound result value. > > I like Hannes' idea of gettting rid of clear_scsi_result() and use > set_scsi_result() for it. This way we zap one of these > helpers. Actually we could even use it for all and thus could > eliminate all the set_{host,driver,msg}_byte() accessors. > I wouldn't got that far; in quite some area the 'result' is build incrementally, so we cannot call set_scsi_result here. Cheers, Hannes
Re: [VERY EARLY RFC 00/13] Rework SCSI results handling
On Wed, Apr 18, 2018 at 04:00:43PM +, Bart Van Assche wrote: > Thank you for having come up with this so quickly. Something I do not > like about this patch series is that several new very short helper functions > are introduced, e.g. set_scsi_result(), clear_scsi_result(), to_scsi_result() > and from_scsi_result(). If we would make scsi_result a union of a 32-bit > integer and a struct with the driver, host, msg and status bytes then we > would not need any of these new helper functions. Additionally, that approach > would allow us to eliminate the {set,get}_{driver,host,msg,status}_byte() > functions. Honestly I don't really like these mini accessor functions as well. But when using a union we loose all the benefits of the enums as drivers still can touch the compound result value. I like Hannes' idea of gettting rid of clear_scsi_result() and use set_scsi_result() for it. This way we zap one of these helpers. Actually we could even use it for all and thus could eliminate all the set_{host,driver,msg}_byte() accessors. Thanks, Johannes -- Johannes Thumshirn Storage jthumsh...@suse.de+49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
Re: [VERY EARLY RFC 00/13] Rework SCSI results handling
On 2018-04-18 11:01 AM, Johannes Thumshirn wrote: Hey all, here's a early preview of my SCSI results rework so we can eventually discuss things next week at LSF/MM (it still has compiler errors on aic7xxx and scsi_debug). The motivation behing this is that some drivers have failed to set the scsi_cmnd::result bytes correctly in the past and this is resulting in hard to case down errors. The open points: 1) 148 files changed, treewide. That's huge. Is it worth it? 2) remove the old status byte definitions 3) add a scsi_cmnd::result == 0 wrapper 3) convert aic7xx's CAM stuff so this series compiles cleanly 4) What the heck is the SDEG_RES_IMMED_MASK stuff in scsi_debug doing It's a hack. The cdb tables have only one delay value per command and for Start Stop Unit and Synchronize Cache those are relatively long delays. However as you may be aware both those commands have an IMMED bit which makes those commands return more or less immediately. In the work flow of the scsi_debug driver the work done by a specific command is done more or less immediately and any delay associated with that command is done by generic code just prior to calling scsi_cmnd::done(). So for those two commands I needed an additional return value from the resp_start_stop() and resp_sync_cache() to indicate that their normal delay should be short circuited. And the signatures of those functions are function pointers in many of the scsi_debug tables, so expanding the number of arguments for these two cases would have been messy. So the (hacky) solution was to overload the return int with an extra flag. And that return int is none other than the infamous SCSI result. With the benefit of hindsight a better solution is to add a new bool to struct sdebug_dev_info: bool immed_shortens_delay; that is set by those two commands when their IMMED bit is set (in their cdbs). Then the code in schedule_resp() can act on that bool (when set) to short circuit the delay. That will be a worthwhile improvement so other commands (if and when implemented) can react properly when their IMMED bit is set (e.g. FORMAT UNIT, PRE-FETCH and SANITIZE). Does that clear up this matter? Doug Gilbert 5) change scsi_execute() so we get a newish 'struct scsi_results' instead of an int 6) {to,from}_scsi_result() are odd 7) find suitable commit messages In case someone want's it in a more viewable form I've pushed the series to my kernel.org git: https://git.kernel.org/pub/scm/linux/kernel/git/jth/linux.git/log/?h=scsi-results Johannes Thumshirn (13): scsi: use host_byte() accessor scsi: remove Scsi_Cmnd typedef scsi: add enum for host byte codes scsi: add enum for driver byte codes scsi: add enum for message byte codes scsi: introduce set_scsi_result scsi: use set_driver_byte treewide: use set_host_byte scsi: use set_msg_byte scsi: introduce set_status_byte and convert LLDDs to use it scsi: Change status bytes to use SAM-3 version reewide: introduce clear_scsi_result() and convert drivers scsi: introduce struct scsi_result arch/ia64/hp/sim/simscsi.c | 6 +- block/bsg-lib.c | 8 +- block/bsg.c | 8 +- block/scsi_ioctl.c | 12 +- drivers/ata/libata-scsi.c | 54 +++--- drivers/firewire/sbp2.c | 2 +- drivers/infiniband/ulp/srp/ib_srp.c | 23 ++- drivers/message/fusion/mptfc.c | 8 +- drivers/message/fusion/mptsas.c | 3 +- drivers/message/fusion/mptscsih.c | 122 - drivers/message/fusion/mptspi.c | 6 +- drivers/s390/scsi/zfcp_fc.h | 2 +- drivers/s390/scsi/zfcp_scsi.c | 4 +- drivers/scsi/3w-9xxx.c | 18 +- drivers/scsi/3w-sas.c | 16 +- drivers/scsi/3w-.c | 36 ++-- drivers/scsi/53c700.c | 3 +- drivers/scsi/BusLogic.c | 24 ++- drivers/scsi/NCR5380.c | 39 +++-- drivers/scsi/a100u2w.c | 2 +- drivers/scsi/aacraid/aachba.c | 221 +--- drivers/scsi/aacraid/commsup.c | 5 +- drivers/scsi/aacraid/linit.c| 12 +- drivers/scsi/advansys.c | 58 +++ drivers/scsi/aha152x.c | 76 drivers/scsi/aha1542.c | 2 +- drivers/scsi/aha1740.c | 11 +- drivers/scsi/aha1740.h | 4 +- drivers/scsi/aic7xxx/aic79xx_osm.c | 5 +- drivers/scsi/aic7xxx/aic79xx_osm.h | 6 +- drivers/scsi/aic7xxx/aic7xxx_osm.c | 5 +- drivers/scsi/aic7xxx/aic7xxx_osm.h
Re: [VERY EARLY RFC 00/13] Rework SCSI results handling
On Wed, 2018-04-18 at 17:01 +0200, Johannes Thumshirn wrote: > here's a early preview of my SCSI results rework so we can eventually > discuss things next week at LSF/MM (it still has compiler errors on > aic7xxx and scsi_debug). > > The motivation behing this is that some drivers have failed to set the > scsi_cmnd::result bytes correctly in the past and this is resulting in > hard to case down errors. > > The open points: > 1) 148 files changed, treewide. That's huge. Is it worth it? > 2) remove the old status byte definitions > 3) add a scsi_cmnd::result == 0 wrapper > 3) convert aic7xx's CAM stuff so this series compiles cleanly > 4) What the heck is the SDEG_RES_IMMED_MASK stuff in scsi_debug doing > 5) change scsi_execute() so we get a newish 'struct scsi_results' instead of > an int > 6) {to,from}_scsi_result() are odd > 7) find suitable commit messages Hello Johannes, Thank you for having come up with this so quickly. Something I do not like about this patch series is that several new very short helper functions are introduced, e.g. set_scsi_result(), clear_scsi_result(), to_scsi_result() and from_scsi_result(). If we would make scsi_result a union of a 32-bit integer and a struct with the driver, host, msg and status bytes then we would not need any of these new helper functions. Additionally, that approach would allow us to eliminate the {set,get}_{driver,host,msg,status}_byte() functions. Thanks, Bart.
[VERY EARLY RFC 00/13] Rework SCSI results handling
Hey all, here's a early preview of my SCSI results rework so we can eventually discuss things next week at LSF/MM (it still has compiler errors on aic7xxx and scsi_debug). The motivation behing this is that some drivers have failed to set the scsi_cmnd::result bytes correctly in the past and this is resulting in hard to case down errors. The open points: 1) 148 files changed, treewide. That's huge. Is it worth it? 2) remove the old status byte definitions 3) add a scsi_cmnd::result == 0 wrapper 3) convert aic7xx's CAM stuff so this series compiles cleanly 4) What the heck is the SDEG_RES_IMMED_MASK stuff in scsi_debug doing 5) change scsi_execute() so we get a newish 'struct scsi_results' instead of an int 6) {to,from}_scsi_result() are odd 7) find suitable commit messages In case someone want's it in a more viewable form I've pushed the series to my kernel.org git: https://git.kernel.org/pub/scm/linux/kernel/git/jth/linux.git/log/?h=scsi-results Johannes Thumshirn (13): scsi: use host_byte() accessor scsi: remove Scsi_Cmnd typedef scsi: add enum for host byte codes scsi: add enum for driver byte codes scsi: add enum for message byte codes scsi: introduce set_scsi_result scsi: use set_driver_byte treewide: use set_host_byte scsi: use set_msg_byte scsi: introduce set_status_byte and convert LLDDs to use it scsi: Change status bytes to use SAM-3 version reewide: introduce clear_scsi_result() and convert drivers scsi: introduce struct scsi_result arch/ia64/hp/sim/simscsi.c | 6 +- block/bsg-lib.c | 8 +- block/bsg.c | 8 +- block/scsi_ioctl.c | 12 +- drivers/ata/libata-scsi.c | 54 +++--- drivers/firewire/sbp2.c | 2 +- drivers/infiniband/ulp/srp/ib_srp.c | 23 ++- drivers/message/fusion/mptfc.c | 8 +- drivers/message/fusion/mptsas.c | 3 +- drivers/message/fusion/mptscsih.c | 122 - drivers/message/fusion/mptspi.c | 6 +- drivers/s390/scsi/zfcp_fc.h | 2 +- drivers/s390/scsi/zfcp_scsi.c | 4 +- drivers/scsi/3w-9xxx.c | 18 +- drivers/scsi/3w-sas.c | 16 +- drivers/scsi/3w-.c | 36 ++-- drivers/scsi/53c700.c | 3 +- drivers/scsi/BusLogic.c | 24 ++- drivers/scsi/NCR5380.c | 39 +++-- drivers/scsi/a100u2w.c | 2 +- drivers/scsi/aacraid/aachba.c | 221 +--- drivers/scsi/aacraid/commsup.c | 5 +- drivers/scsi/aacraid/linit.c| 12 +- drivers/scsi/advansys.c | 58 +++ drivers/scsi/aha152x.c | 76 drivers/scsi/aha1542.c | 2 +- drivers/scsi/aha1740.c | 11 +- drivers/scsi/aha1740.h | 4 +- drivers/scsi/aic7xxx/aic79xx_osm.c | 5 +- drivers/scsi/aic7xxx/aic79xx_osm.h | 6 +- drivers/scsi/aic7xxx/aic7xxx_osm.c | 5 +- drivers/scsi/aic7xxx/aic7xxx_osm.h | 6 +- drivers/scsi/arcmsr/arcmsr_hba.c| 56 +++--- drivers/scsi/arm/acornscsi.c| 11 +- drivers/scsi/arm/fas216.c | 6 +- drivers/scsi/atp870u.c | 18 +- drivers/scsi/be2iscsi/be_main.c | 8 +- drivers/scsi/bfa/bfad_im.c | 38 ++-- drivers/scsi/bfa/bfad_im.h | 1 - drivers/scsi/bnx2fc/bnx2fc_io.c | 17 +- drivers/scsi/ch.c | 2 +- drivers/scsi/constants.c| 4 +- drivers/scsi/csiostor/csio_scsi.c | 12 +- drivers/scsi/cxlflash/main.c| 42 +++-- drivers/scsi/dc395x.c | 57 +++--- drivers/scsi/device_handler/scsi_dh_alua.c | 10 +- drivers/scsi/dpt_i2o.c | 45 +++-- drivers/scsi/esas2r/esas2r_main.c | 19 +- drivers/scsi/esp_scsi.c | 18 +- drivers/scsi/fnic/fnic_scsi.c | 48 +++-- drivers/scsi/gdth.c | 105 ++- drivers/scsi/gdth.h | 10 +- drivers/scsi/gdth_proc.c| 2 +- drivers/scsi/hpsa.c | 94 ++ drivers/scsi/hptiop.c | 27 +-- drivers/scsi/ibmvscsi/ibmvfc.c | 26 ++- drivers/scsi/ibmvscsi/ibmvscsi.c| 28 +-- drivers/scsi/imm.c | 12 +- drivers/scsi/initio.c