Re: [VERY EARLY RFC 00/13] Rework SCSI results handling

2018-04-20 Thread Martin K. Petersen 

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

2018-04-19 Thread Bart Van Assche 
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

2018-04-19 Thread Hannes Reinecke 
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

2018-04-19 Thread Johannes Thumshirn 
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

2018-04-18 Thread Douglas Gilbert 

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

2018-04-18 Thread Bart Van Assche 
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

2018-04-18 Thread Johannes Thumshirn 
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