Re: [RFC PATCH 76/71] ncr5380: Enable PDMA for DTC chips

[Finn Thain]

On Fri, 4 Dec 2015, Ondrej Zary wrote:

> Add I/O register mapping for DTC chips and enable PDMA mode.
> 
> These chips have 16-bit wide HOST BUFFER register (counter register at 
> offset 0x0d increments by 2 on each HOST BUFFER read).
> 
> Large PIO transfers crash at least the DTCT-436P chip (all reads result 
> in 0xFF) so this patch actually makes it work.
> 
> The chip also crashes when we bang the C400 host status register too
> heavily after PDMA write - a small udelay is needed.
> 
> Signed-off-by: Ondrej Zary 
> ---
> # hdparm -t --direct /dev/sdb
> 
> /dev/sdb:
>  Timing O_DIRECT disk reads:   4 MB in  3.78 seconds =   1.06 MB/sec
> 
> 
>  drivers/scsi/NCR5380.h   |1 +
>  drivers/scsi/g_NCR5380.c |   47 
> +++---
>  2 files changed, 25 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/scsi/NCR5380.h b/drivers/scsi/NCR5380.h
> index 5092580..e3b8149 100644
> --- a/drivers/scsi/NCR5380.h
> +++ b/drivers/scsi/NCR5380.h
> @@ -222,6 +222,7 @@
>  
>  #define FLAG_NO_DMA_FIXUP1   /* No DMA errata workarounds */
>  #define FLAG_NO_PSEUDO_DMA   8   /* Inhibit DMA */
> +#define FLAG_16BIT   16  /* 16-bit PDMA */

Can we give this a better name? FLAG_16BIT could be taken to mean "16-bit 
ISA card" but do we really want a flag for that? How about 
FLAG_16BIT_BUF_REG or FLAG_WORD_IO_BUF?

All active flags appear in the console log, thanks to prepare_info(). It 
might be helpful to include this one; FLAG_DTC3181E is likely to 
disappear.

-- 

>  #define FLAG_LATE_DMA_SETUP  32  /* Setup NCR before DMA H/W */
>  #define FLAG_TAGGED_QUEUING  64  /* as X3T9.2 spelled it */
>  #define FLAG_TOSHIBA_DELAY   128 /* Allow for borken CD-ROMs */
> diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
> index fae4332..04f6c29 100644
> --- a/drivers/scsi/g_NCR5380.c
> +++ b/drivers/scsi/g_NCR5380.c
> @@ -331,7 +331,7 @@ static int __init generic_NCR5380_detect(struct 
> scsi_host_template *tpnt)
>   ports = ncr_53c400a_ports;
>   break;
>   case BOARD_DTC3181E:
> - flags = FLAG_NO_PSEUDO_DMA;
> + flags = FLAG_NO_DMA_FIXUP | FLAG_16BIT;
>   ports = dtc_3181e_ports;
>   break;
>   }
> @@ -415,7 +415,8 @@ static int __init generic_NCR5380_detect(struct 
> scsi_host_template *tpnt)
>   hostdata->c400_blk_cnt = 1;
>   hostdata->c400_host_buf = 4;
>   }
> - if (overrides[current_override].board == BOARD_NCR53C400A) {
> + if (overrides[current_override].board == BOARD_NCR53C400A ||
> + overrides[current_override].board == BOARD_DTC3181E) {
>   hostdata->c400_ctl_status = 9;
>   hostdata->c400_blk_cnt = 10;
>   hostdata->c400_host_buf = 8;
> @@ -434,7 +435,8 @@ static int __init generic_NCR5380_detect(struct 
> scsi_host_template *tpnt)
>   goto out_unregister;
>  
>   if (overrides[current_override].board == BOARD_NCR53C400 ||
> - overrides[current_override].board == BOARD_NCR53C400A)
> + overrides[current_override].board == BOARD_NCR53C400A ||
> + overrides[current_override].board == BOARD_DTC3181E)
>   NCR5380_write(hostdata->c400_ctl_status, CSR_BASE);
>  
>   NCR5380_maybe_reset_bus(instance);
> @@ -561,11 +563,10 @@ static inline int NCR5380_pread(struct Scsi_Host 
> *instance, unsigned char *dst,
>   while (NCR5380_read(hostdata->c400_ctl_status) & 
> CSR_HOST_BUF_NOT_RDY);
>  
>  #ifndef SCSI_G_NCR5380_MEM
> - {
> - int i;
> - for (i = 0; i < 128; i++)
> - dst[start + i] = 
> NCR5380_read(hostdata->c400_host_buf);
> - }
> + if (hostdata->flags & FLAG_16BIT)
> + insw(instance->io_port + hostdata->c400_host_buf, dst + 
> start, 64);
> + else
> + insb(instance->io_port + hostdata->c400_host_buf, dst + 
> start, 128);
>  #else
>   /* implies SCSI_G_NCR5380_MEM */
>   memcpy_fromio(dst + start,
> @@ -582,11 +583,10 @@ static inline int NCR5380_pread(struct Scsi_Host 
> *instance, unsigned char *dst,
>   }
>  
>  #ifndef SCSI_G_NCR5380_MEM
> - {
> - int i;  
> - for (i = 0; i < 128; i++)
> - dst[start + i] = 
> NCR5380_read(hostdata->c400_host_buf);
> - }
> + if (hostdata->flags & FLAG_16BIT)
> + insw(instance->io_port + hostdata->c400_host_buf, dst + 
> start, 64);
> + else
> + 

Re: [PATCH 08/10] aacraid: Disable device ID wildcard

[Christoph Hellwig]

On Thu, Dec 03, 2015 at 09:32:18PM +, Raghava Aditya Renukunta wrote:
> This will enable us to prevent aacraid from loading for PCI devices that match
> device ID wildcards. Enabling us to use say a new driver for future devices.

This looks like a bogus reason.  The same PCI ID should always be
compatible and mathed by the same driver.  Even if you add a new driver
to expose additional feature and break these semantics there is no point
to do a) reject them conditionally on a module option and b) do this
before said driver is merged.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 76/71] ncr5380: Enable PDMA for DTC chips

[Finn Thain]

On Fri, 4 Dec 2015, Julian Calaby wrote:

> > -   if (overrides[current_override].board == BOARD_NCR53C400A) {
> > +   if (overrides[current_override].board == BOARD_NCR53C400A ||
> > +   overrides[current_override].board == BOARD_DTC3181E) {
> 
> These if statements are starting to get a bit long, would it make
> sense to replace them with a flag or equivalent?

To what end? Shorter lines? As in,

if (board_is_ncr53c400a || board_is_dtc3181e) {
/* ... */
}

I suppose that could be an improvement if new flags would entirely replace 
the override.board struct member and the existing switch statement,

switch (overrides[current_override].board) {
/* ... */
}

Or maybe you meant testing a new flag something like this,

if (hostdata->ncr53c400_compatible) {
/* ... */
}

If your concern is the Don't Repeat Yourself rule, I'm not sure that new 
flag would get tested more than once (?) And it would still have to be 
assigned using an "objectionably" long expression, e.g.

hostdata->ncr53c400_compatible =
overrides[current_override].board == BOARD_NCR53C400 ||
overrides[current_override].board == BOARD_NCR53C400A ||
overrides[current_override].board == BOARD_DTC3181E;

Rather than add new flags, perhaps a 'switch' statement instead of an 'if' 
statement would be shorter (if the size of the expression is the problem).

-- 
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 76/71] ncr5380: Enable PDMA for DTC chips

[Finn Thain]

On Fri, 4 Dec 2015, Ondrej Zary wrote:

> @@ -685,8 +684,10 @@ static inline int NCR5380_pwrite(struct Scsi_Host 
> *instance, unsigned char *src,
>   /* All documentation says to check for this. Maybe my hardware is too
>* fast. Waiting for it seems to work fine! KLL
>*/
> - while (!(i = NCR5380_read(hostdata->c400_ctl_status) & 
> CSR_GATED_53C80_IRQ))
> + while (!(i = NCR5380_read(hostdata->c400_ctl_status) & 
> CSR_GATED_53C80_IRQ)) {
> + udelay(4); /* DTC436 chip hangs without this */
>   ;   // FIXME - no timeout
> + }
>  
>   /*
>* I know. i is certainly != 0 here but the loop is new. See previous
> 

Given that you've added braces, the redundant semicolon can be removed.

-- 
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] storvsc: add more logging for error and warning messages

[Vitaly Kuznetsov]

Long Li  writes:

> Introduce a logging level for storvsc to log certain error/warning
> messages. Those messages are helpful in some environments,
> e.g. Microsoft Azure, for customer support and troubleshooting
> purposes.

I have an alternative suggestion: let's use dynamic debug! Basically, we
need to convert all non-error logging to using dev_dbg() and this can be
enabled dynamically when needed, even reboot won't be required.

>
> Signed-off-by: Long Li 
> ---
>  drivers/scsi/storvsc_drv.c | 30 +-
>  1 file changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 40c43ae..afa1647 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -164,6 +164,21 @@ static int sense_buffer_size = 
> PRE_WIN8_STORVSC_SENSE_BUFFER_SIZE;
>  */
>  static int vmstor_proto_version;
>
> +#define STORVSC_LOGGING_NONE 0
> +#define STORVSC_LOGGING_ERROR1
> +#define STORVSC_LOGGING_WARN 2
> +
> +static int logging_level = STORVSC_LOGGING_ERROR;
> +module_param(logging_level, int, S_IRUGO|S_IWUSR);
> +MODULE_PARM_DESC(logging_level,
> + "Logging level, 0 - None, 1 - Error (default), 2 - Warning.");
> +
> +static inline bool do_logging(int level)
> +{
> + return (logging_level >= level) ? true : false;
> +}
> +
> +
>  struct vmscsi_win8_extension {
>   /*
>* The following were added in Windows 8
> @@ -1183,7 +1198,7 @@ static void storvsc_command_completion(struct 
> storvsc_cmd_request *cmd_request)
>
>   scmnd->result = vm_srb->scsi_status;
>
> - if (scmnd->result) {
> + if (scmnd->result && do_logging(STORVSC_LOGGING_ERROR)) {
>   if (scsi_normalize_sense(scmnd->sense_buffer,
>   SCSI_SENSE_BUFFERSIZE, _hdr))
>   scsi_print_sense_hdr(scmnd->device, "storvsc",
> @@ -1239,12 +1254,25 @@ static void storvsc_on_io_completion(struct hv_device 
> *device,
>   stor_pkt->vm_srb.sense_info_length =
>   vstor_packet->vm_srb.sense_info_length;
>
> + if (vstor_packet->vm_srb.scsi_status != 0 ||
> + vstor_packet->vm_srb.srb_status != SRB_STATUS_SUCCESS)
> + if (do_logging(STORVSC_LOGGING_WARN))
> + dev_warn(>device,
> + "cmd 0x%x scsi status 0x%x srb status 0x%x\n",
> + stor_pkt->vm_srb.cdb[0],
> + vstor_packet->vm_srb.scsi_status,
> + vstor_packet->vm_srb.srb_status);
>
>   if ((vstor_packet->vm_srb.scsi_status & 0xFF) == 0x02) {
>   /* CHECK_CONDITION */
>   if (vstor_packet->vm_srb.srb_status &
>   SRB_STATUS_AUTOSENSE_VALID) {
>   /* autosense data available */
> + if (do_logging(STORVSC_LOGGING_WARN))
> + dev_warn(>device,
> + "stor pkt %p autosense data valid - len 
> %d\n",
> + request,
> + vstor_packet->vm_srb.sense_info_length);
>
>   memcpy(request->cmd->sense_buffer,
>  vstor_packet->vm_srb.sense_data,

-- 
  Vitaly
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 76/71] ncr5380: Enable PDMA for DTC chips

[Ondrej Zary]

On Friday 04 December 2015, Finn Thain wrote:
> 
> On Fri, 4 Dec 2015, Ondrej Zary wrote:
> 
> > Add I/O register mapping for DTC chips and enable PDMA mode.
> > 
> > These chips have 16-bit wide HOST BUFFER register (counter register at 
> > offset 0x0d increments by 2 on each HOST BUFFER read).
> > 
> > Large PIO transfers crash at least the DTCT-436P chip (all reads result 
> > in 0xFF) so this patch actually makes it work.
> > 
> > The chip also crashes when we bang the C400 host status register too
> > heavily after PDMA write - a small udelay is needed.
> > 
> > Signed-off-by: Ondrej Zary 
> > ---
> > # hdparm -t --direct /dev/sdb
> > 
> > /dev/sdb:
> >  Timing O_DIRECT disk reads:   4 MB in  3.78 seconds =   1.06 MB/sec
> > 
> > 
> >  drivers/scsi/NCR5380.h   |1 +
> >  drivers/scsi/g_NCR5380.c |   47 
> > +++---
> >  2 files changed, 25 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/scsi/NCR5380.h b/drivers/scsi/NCR5380.h
> > index 5092580..e3b8149 100644
> > --- a/drivers/scsi/NCR5380.h
> > +++ b/drivers/scsi/NCR5380.h
> > @@ -222,6 +222,7 @@
> >  
> >  #define FLAG_NO_DMA_FIXUP  1   /* No DMA errata workarounds */
> >  #define FLAG_NO_PSEUDO_DMA 8   /* Inhibit DMA */
> > +#define FLAG_16BIT 16  /* 16-bit PDMA */
> 
> Can we give this a better name? FLAG_16BIT could be taken to mean "16-bit 
> ISA card" but do we really want a flag for that? How about 
> FLAG_16BIT_BUF_REG or FLAG_WORD_IO_BUF?
> 
> All active flags appear in the console log, thanks to prepare_info(). It 
> might be helpful to include this one; FLAG_DTC3181E is likely to 
> disappear.

Thinking more about this, we can probably detect the host buffer register
width instead of adding another flag. Read the counter, then read once from
the host buffer and read the counter again to see if it increments by 1 or 2.
Or maybe even 4 for PCI cards.

-- 
Ondrej Zary
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/8] aic7xxx: Avoid name collision with

[Michal Marek]

Rename the local definition of LIST_HEAD to BSD_LIST_HEAD. This fixes a
ctags error if we apply the C rules to header files  as well:

ctags: Warning: drivers/scsi/aic7xxx/aic79xx.h:1072: null expansion of name 
pattern "\3"
ctags: Warning: drivers/scsi/aic7xxx/aic7xxx.h:919: null expansion of name 
pattern "\3"

Cc: linux-scsi@vger.kernel.org
Signed-off-by: Michal Marek 
---
v2: No change

 drivers/scsi/aic7xxx/aic79xx.h | 4 ++--
 drivers/scsi/aic7xxx/aic79xx_osm.h | 5 -
 drivers/scsi/aic7xxx/aic7xxx.h | 2 +-
 drivers/scsi/aic7xxx/aic7xxx_osm.h | 5 -
 drivers/scsi/aic7xxx/queue.h   | 2 +-
 5 files changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/scsi/aic7xxx/aic79xx.h b/drivers/scsi/aic7xxx/aic79xx.h
index df2e0e5367d2..d47b527b25dd 100644
--- a/drivers/scsi/aic7xxx/aic79xx.h
+++ b/drivers/scsi/aic7xxx/aic79xx.h
@@ -624,7 +624,7 @@ struct scb {
 };
 
 TAILQ_HEAD(scb_tailq, scb);
-LIST_HEAD(scb_list, scb);
+BSD_LIST_HEAD(scb_list, scb);
 
 struct scb_data {
/*
@@ -1069,7 +1069,7 @@ struct ahd_softc {
/*
 * SCBs that have been sent to the controller
 */
-   LIST_HEAD(, scb)  pending_scbs;
+   BSD_LIST_HEAD(, scb)  pending_scbs;
 
/*
 * Current register window mode information.
diff --git a/drivers/scsi/aic7xxx/aic79xx_osm.h 
b/drivers/scsi/aic7xxx/aic79xx_osm.h
index c58fa33c6592..728193a42e6e 100644
--- a/drivers/scsi/aic7xxx/aic79xx_osm.h
+++ b/drivers/scsi/aic7xxx/aic79xx_osm.h
@@ -65,11 +65,6 @@
 /* Core SCSI definitions */
 #define AIC_LIB_PREFIX ahd
 
-/* Name space conflict with BSD queue macros */
-#ifdef LIST_HEAD
-#undef LIST_HEAD
-#endif
-
 #include "cam.h"
 #include "queue.h"
 #include "scsi_message.h"
diff --git a/drivers/scsi/aic7xxx/aic7xxx.h b/drivers/scsi/aic7xxx/aic7xxx.h
index f695774645c1..4ce4e903a759 100644
--- a/drivers/scsi/aic7xxx/aic7xxx.h
+++ b/drivers/scsi/aic7xxx/aic7xxx.h
@@ -916,7 +916,7 @@ struct ahc_softc {
/*
 * SCBs that have been sent to the controller
 */
-   LIST_HEAD(, scb)  pending_scbs;
+   BSD_LIST_HEAD(, scb)  pending_scbs;
 
/*
 * Counting lock for deferring the release of additional
diff --git a/drivers/scsi/aic7xxx/aic7xxx_osm.h 
b/drivers/scsi/aic7xxx/aic7xxx_osm.h
index bc4cca92ff04..54c702864103 100644
--- a/drivers/scsi/aic7xxx/aic7xxx_osm.h
+++ b/drivers/scsi/aic7xxx/aic7xxx_osm.h
@@ -82,11 +82,6 @@
 /* Core SCSI definitions */
 #define AIC_LIB_PREFIX ahc
 
-/* Name space conflict with BSD queue macros */
-#ifdef LIST_HEAD
-#undef LIST_HEAD
-#endif
-
 #include "cam.h"
 #include "queue.h"
 #include "scsi_message.h"
diff --git a/drivers/scsi/aic7xxx/queue.h b/drivers/scsi/aic7xxx/queue.h
index 8adf8003a164..ba602981f193 100644
--- a/drivers/scsi/aic7xxx/queue.h
+++ b/drivers/scsi/aic7xxx/queue.h
@@ -246,7 +246,7 @@ struct {
\
 /*
  * List declarations.
  */
-#defineLIST_HEAD(name, type)   
\
+#defineBSD_LIST_HEAD(name, type)   
\
 struct name {  \
struct type *lh_first;  /* first element */ \
 }
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 04/10] aacraid: Fix memory leak in aac_fib_map_free

[Tomas Henzl]

On 1.12.2015 13:39, Raghava Aditya Renukunta wrote:
> From: Raghava Aditya Renukunta 
>
> aac_fib_map_free() calls pci_free_consistent() without checking that
> dev->hw_fib_va is not NULL and dev->max_fib_size is not zero.If they
> are indeed NULL/0, this will result in a hang as pci_free_consistent()
> will attempt to invalidate cache for the entire 64-bit address space
> (which would take a very long time).
>
> Fixed by adding a check to make sure that dev->hw_fib_va and
> dev->max_fib_size are not NULL and 0 respectively.
>
> Signed-off-by: Raghava Aditya Renukunta 

Reviewed-by: Tomas Henzl 

Is the can_queue constant during the driver's life, or is it possible
to manipulate it (aac_change_queue_depth)?

Tomas

> ---
>  drivers/scsi/aacraid/commsup.c | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
> index b257d3b..9533f47 100644
> --- a/drivers/scsi/aacraid/commsup.c
> +++ b/drivers/scsi/aacraid/commsup.c
> @@ -83,9 +83,12 @@ static int fib_map_alloc(struct aac_dev *dev)
>  
>  void aac_fib_map_free(struct aac_dev *dev)
>  {
> - pci_free_consistent(dev->pdev,
> -   dev->max_fib_size * (dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB),
> -   dev->hw_fib_va, dev->hw_fib_pa);
> + if (dev->hw_fib_va && dev->max_fib_size) {
> + pci_free_consistent(dev->pdev,
> + (dev->max_fib_size *
> + (dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB)),
> + dev->hw_fib_va, dev->hw_fib_pa);
> + }
>   dev->hw_fib_va = NULL;
>   dev->hw_fib_pa = 0;
>  }

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/10] aacraid: Set correct msix count for EEH recovery

[Tomas Henzl]

On 1.12.2015 13:39, Raghava Aditya Renukunta wrote:
> From: Raghava Aditya Renukunta 
>
> During EEH recovery number of online CPU's might change thereby changing
> the number of MSIx vectors. Since each fib is allocated to a vector,
> changes in the number of vectors causes fib to be sent thru invalid
> vectors.In addition the correct number of MSIx vectors is not
> updated in the INIT struct sent to the controller, when it is
> reinitialized.
>
> Fixed by reassigning vectors to fibs based on the updated number of MSIx
> vectors and updating the INIT structure before sending to controller.
>
> Signed-off-by: Raghava Aditya Renukunta 
> ---
>  drivers/scsi/aacraid/linit.c | 25 -
>  1 file changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
> index 0147210..f88f1132 100644
> --- a/drivers/scsi/aacraid/linit.c
> +++ b/drivers/scsi/aacraid/linit.c
> @@ -1355,6 +1355,7 @@ static int aac_acquire_resources(struct aac_dev *dev)
>   int i, j;
>   int instance = dev->id;
>   const char *name = dev->name;
> + int vector = 0;
>   unsigned long status;
>   /*
>*  First clear out all interrupts.  Then enable the one's that we
> @@ -1409,9 +1410,31 @@ static int aac_acquire_resources(struct aac_dev *dev)
>   }
>  
>   aac_adapter_enable_int(dev);
> + /*max msix may change  after EEH
> +  * Re-assign vectors to fibs
> +  */
> +  for (i = 0;
> + i < (dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB);
> + i++) {
> + if ((dev->max_msix == 1) ||
> +(i > ((dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB - 1)
> + - dev->vector_cap))) {
> + dev->fibs[i].vector_no = 0;
> + } else {
> + dev->fibs[i].vector_no = vector;
> + vector++;
> + if (vector == dev->max_msix)
> + vector = 1;
> + }
> + }

The above hunk added code looks identical to the part you have just added
in 02/10 "aacraid: Fix RRQ overload" could you make this to function ?
Thanks, tomash

>  
> - if (!dev->sync_mode)
> + if (!dev->sync_mode) {
> + /* After EEH recovery or suspend resume, max_msix count
> +  * may change, therfore updating in init as well.
> +  */
>   aac_adapter_start(dev);
> + dev->init->Sa_MSIXVectors = cpu_to_le32(dev->max_msix);
> + }
>   return 0;
>  
>  error_iounmap:

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/10] aacraid: Fix RRQ overload

[Tomas Henzl]

On 1.12.2015 13:39, Raghava Aditya Renukunta wrote:
> From: Raghava Aditya Renukunta 
>
> The driver utilizes an array of atomic variables to keep track of
> IO submissions to each vector. To submit an IO multiple threads
> iterate through the array to find a vector which has empty slots
> to send an IO. The reading and updating of the variable is not atomic,
> causing race conditions when a thread uses a full vector to
> submit an IO.
>
> Fixed by mapping each FIB to a vector, the submission path then uses
> said vector to submit IO thereby removing the possibly of a race
> condition.The vector assignment is started from 1 since vector 0 is
> reserved for the use of AIF management FIBS.If the number of MSIx
> vectors is 1 (MSI or INTx mode) then all the fibs are allocated to
> vector 0.
>
> Signed-off-by: Raghava Aditya Renukunta 

Reviewed-by: Tomas Henzl 

Tomas

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 03/10] aacraid: Added EEH support

[Tomas Henzl]

On 1.12.2015 13:39, Raghava Aditya Renukunta wrote:
> From: Raghava Aditya Renukunta 
>
> Added support for PCI EEH(extended error handling).
>
> Signed-off-by: Raghava Aditya Renukunta 

Reviewed-by: Tomas Henzl 

Tomas

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] mpt3sas: add PCI dependency for CONFIG_SCSI_MPT2SAS

[Arnd Bergmann]

CONFIG_SCSI_MPT2SAS was added as a backwards-compatibility helper that
selects the replacement SCSI_MPT3SAS symbol, but lacks the dependencies:

warning: (SCSI_MPT2SAS) selects SCSI_MPT3SAS which has unmet direct 
dependencies (SCSI_LOWLEVEL && PCI && SCSI)
0x7E5F9A79 Fri Dec 4 12:36:08 CET 2015 failed
drivers/scsi/mpt3sas/mpt3sas_base.c: In function 'mpt3sas_remove_dead_ioc_func':
drivers/scsi/mpt3sas/mpt3sas_base.c:140:2: error: implicit declaration of 
function 'pci_stop_and_remove_bus_device_locked' 
[-Werror=implicit-function-declaration]
drivers/scsi/mpt3sas/mpt3sas_base.c: In function '_base_disable_msix':
drivers/scsi/mpt3sas/mpt3sas_base.c:1921:2: error: implicit declaration of 
function 'pci_disable_msix' [-Werror=implicit-function-declaration]

This adds the same dependencies that SCSI_MPT3SAS has.

Signed-off-by: Arnd Bergmann 
Fixes: b840c3627b6f ("mpt3sas: Add dummy Kconfig option for backwards 
compatibility")
---
This appeared on today's linux-next with ARM randconfig builds

diff --git a/drivers/scsi/mpt3sas/Kconfig b/drivers/scsi/mpt3sas/Kconfig
index 25dc38f25ec6..33dc427cfe9a 100644
--- a/drivers/scsi/mpt3sas/Kconfig
+++ b/drivers/scsi/mpt3sas/Kconfig
@@ -74,6 +74,7 @@ config SCSI_MPT3SAS_MAX_SGE
 
 config SCSI_MPT2SAS
tristate "Legacy MPT2SAS config option"
+   depends on PCI && SCSI
default n
select SCSI_MPT3SAS
---help---

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mpt3sas: add PCI dependency for CONFIG_SCSI_MPT2SAS

[Arnd Bergmann]

On Friday 04 December 2015 08:28:51 James Bottomley wrote:
> On Fri, 2015-12-04 at 15:27 +0100, Arnd Bergmann wrote:
> > CONFIG_SCSI_MPT2SAS was added as a backwards-compatibility helper that
> > selects the replacement SCSI_MPT3SAS symbol, but lacks the dependencies:
> > 
> > warning: (SCSI_MPT2SAS) selects SCSI_MPT3SAS which has unmet direct 
> > dependencies (SCSI_LOWLEVEL && PCI && SCSI)
> > 0x7E5F9A79 Fri Dec 4 12:36:08 CET 2015 failed
> > drivers/scsi/mpt3sas/mpt3sas_base.c: In function 
> > 'mpt3sas_remove_dead_ioc_func':
> > drivers/scsi/mpt3sas/mpt3sas_base.c:140:2: error: implicit declaration of 
> > function 'pci_stop_and_remove_bus_device_locked' 
> > [-Werror=implicit-function-declaration]
> > drivers/scsi/mpt3sas/mpt3sas_base.c: In function '_base_disable_msix':
> > drivers/scsi/mpt3sas/mpt3sas_base.c:1921:2: error: implicit declaration of 
> > function 'pci_disable_msix' [-Werror=implicit-function-declaration]
> > 
> > This adds the same dependencies that SCSI_MPT3SAS has.
> 
> OK, you're about the fifth person to complain about this and this patch
> was posted a few days ago and is now here:
> 
> http://git.kernel.org/cgit/linux/kernel/git/jejb/scsi.git/commit/?h=fixes=3ddda3e4c82dea58933bde8d0f6ef34470c360cb
> 
> It's even been in for-next for nearly 24h

I only found the bug on today on the latest linux-next, which should have
contained it if you pushed out the branch in time, but it's not in there.

2015-12-03 16:33:05 6e3ac04845fb "Merge branch 'fixes' into for-next"
2015-12-03 21:44:54 de84dfc24842 "Merge remote-tracking branch 'scsi/for-next'"

I'm sure it will be there in the next next.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: kernel BUG at drivers/scsi/scsi_lib.c:1096!

[Takashi Iwai]

On Fri, 04 Dec 2015 18:02:58 +0100,
Jens Axboe wrote:
> 
> On 12/04/2015 09:59 AM, Takashi Iwai wrote:
> > On Wed, 25 Nov 2015 20:01:47 +0100,
> > Hannes Reinecke wrote:
> >>
> >> On 11/25/2015 07:01 PM, Mike Snitzer wrote:
> >>> On Wed, Nov 25 2015 at  4:04am -0500,
> >>> Hannes Reinecke  wrote:
> >>>
>  On 11/20/2015 04:28 PM, Ewan Milne wrote:
> > On Fri, 2015-11-20 at 15:55 +0100, Hannes Reinecke wrote:
> >> Can't we have a joint effort here?
> >> I've been spending a _LOT_ of time trying to debug things here, but
> >> none of the ideas I've come up with have been able to fix anything.
> >
> > Yes.  I'm not the one primarily looking at it, and we don't have a
> > reproducer in-house.  We just have the one dump right now.
> >
> >>
> >> I'm almost tempted to increase the count from scsi_alloc_sgtable()
> >> by one and be done with ...
> >>
> >
> > That might not fix it if it is a problem with the merge code, though.
> >
>  And indeed, it doesn't.
> >>>
> >>> How did you arrive at that?  Do you have a reproducer now?
> >>>
> >> Not a reproducer, but several dumps for analysis.
> >>
>  Seems I finally found the culprit.
> 
>  What happens is this:
>  We have two paths, with these seg_boundary_masks:
> 
>  path-1:seg_boundary_mask = 65535,
>  path-2:seg_boundary_mask = 4294967295,
> 
>  consequently the DM request queue has this:
> 
>  md-1:seg_boundary_mask = 65535,
> 
>  What happens now is that a request is being formatted, and sent
>  to path 2. During submission req->nr_phys_segments is formatted
>  with the limits of path 2, arriving at a count of 3.
>  Now the request gets retried on path 1, but as the NOMERGE request
>  flag is set req->nr_phys_segments is never updated.
>  But blk_rq_map_sg() ignores all counters, and just uses the
>  bi_vec directly, resulting in a count of 4 -> boom.
> 
>  So the culprit here is the NOMERGE flag,
> >>>
> >>> NOMERGE is always set in __blk_rq_prep_clone() for cloned requests.
> >>>
> >> Yes.
> >>
>  which is evaluated via
>  ->dm_dispatch_request()
>  ->blk_insert_cloned_request()
>    ->blk_rq_check_limits()
> >>>
> >>> blk_insert_cloned_request() is the only caller of blk_rq_check_limits();
> >>> anyway after reading your mail I'm still left wondering if your proposed
> >>> patch is correct.
> >>>
>  If the above assessment is correct, the following patch should
>  fix it:
> 
>  diff --git a/block/blk-core.c b/block/blk-core.c
>  index 801ced7..12cccd6 100644
>  --- a/block/blk-core.c
>  +++ b/block/blk-core.c
>  @@ -1928,7 +1928,7 @@ EXPORT_SYMBOL(submit_bio);
>  */
> int blk_rq_check_limits(struct request_queue *q, struct request *rq)
> {
>  -   if (!rq_mergeable(rq))
>  +   if (rq->cmd_type != REQ_TYPE_FS)
>    return 0;
> 
>    if (blk_rq_sectors(rq) > blk_queue_get_max_sectors(q,
>  rq->cmd_flags)) {
> 
> 
>  Mike? Jens?
>  Can you comment on it?
> >>>
> >>> You're not explaining the actual change in the patch very well; I think
> >>> you're correct but you're leaving the justification as an exercise to
> >>> the reviewer:
> >>>
> >>> blk_rq_check_limits() will call blk_recalc_rq_segments() after the
> >>> !rq_mergeable() check but you're saying for this case in question we
> >>> never get there -- due to the cloned request having NOMERGE set.
> >>>
> >>> So in blk_rq_check_limits() you've unrolled rq_mergeable() and
> >>> open-coded the lone remaining check (rq->cmd_type != REQ_TYPE_FS)
> >>>
> >>> I agree that the (rq->cmd_flags & REQ_NOMERGE_FLAGS) check in
> >>> the blk_insert_cloned_request() call-chain (via rq_mergeable()) makes no
> >>> sense for cloned requests that always have NOMERGE set.
> >>>
> >>> So you're saying that by having blk_rq_check_limits() go on to call
> >>> blk_recalc_rq_segments() this bug will be fixed?
> >>>
> >> That is the idea.
> >>
> >> I've already established that in all instances I have seen so far
> >> req->nr_phys_segments is _less_ than req->bio->bi_phys_segments.
> >>
> >> As it turns out, req->nr_phys_segemnts _would_ have been updated in
> >> blk_rq_check_limits(), but isn't due to the NOMERGE flag being set
> >> for the cloned request.
> >> So each cloned request inherits the values from the original request,
> >> despite the fact that req->nr_phys_segments _has_ to be evaluated in
> >> the final request_queue context, as the queue limits _might_ be
> >> different from the original (merged) queue limits of the multipath
> >> request queue.
> >>
> >>> BTW, I think blk_rq_check_limits()'s export should be removed and the
> >>> function made static and renamed to blk_clone_rq_check_limits(), again:
> >>> blk_insert_cloned_request() is the only caller of blk_rq_check_limits()
> >>>
> >> 

Re: BUG: KASAN: slab-out-of-bounds in ses_enclosure_data_process+0x900/0xe50

[Ewan Milne]

On Thu, 2015-12-03 at 23:20 +0100, Andrea Gelmini wrote:
> On Thu, Dec 03, 2015 at 12:59:06PM -0800, James Bottomley wrote:
> > sg_map -i
> > 
> > in your system, you should see something with an inquiry string like
> > enclosure.  It's the /dev/sg of that you need to run sg_ses on.
> 
> root@glen:/home/gelma# sg_map -i
> /dev/sg0  /dev/sda  ATA   Samsung SSD 850   1B6Q
> /dev/sg1  /dev/sr0  HL-DT-ST  DVDRAM GU40N  QX23
> /dev/sg2  /dev/sdb  WDMy Passport 0820  1007
> /dev/sg3  WDSES Device1007
> 
> And following Douglas' instructions:
> 
> root@glen:/home/gelma# lsscsi -gs
> [0:0:0:0]diskATA  Samsung SSD 850  1B6Q  /dev/sda   /dev/sg0   
> 1.02TB
> [1:0:0:0]cd/dvd  HL-DT-ST DVDRAM GU40N QX23  /dev/sr0   /dev/sg1  
>   -
> [8:0:0:0]diskWD   My Passport 0820 1007  /dev/sdb   /dev/sg2   
> 2.00TB
> [8:0:0:1]enclosu WD   SES Device   1007  -  /dev/sg3  
>  
> 
> root@glen:/home/gelma# sg_ses /dev/sg3
>   WDSES Device1007
> Supported diagnostic pages:
>   Supported Diagnostic Pages [sdp] [0x0]
>   Short Enclosure Status (SES) [ses] [0x8]
>[0x80]
>[0x83]
>[0x84]
>[0x85]
> 
> 
> Well, if it's better for you, I can give you root access to a machine with 
> this device
> connected to.
> 
> Thanks a lot for your time,
> Andrea

There seems to be a problem with the ses code if the device only reports
Short Enclosure Status.  We probably need to do something like:

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index eba183c..065a528 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -537,6 +537,15 @@ static int ses_intf_add(struct device *cdev,
if (result)
goto recv_failed;
 
+   /* If enclosure only supports Short Enclosure Status page (08),
+* it returns that page regardless of what we requested, and
+* only returns vendor-specific status.  There is nothing wrong
+* with such enclosures, we just can't make use of them. */
+   if (hdr_buf[0] == 8) {
+   err = -ENODEV;
+   goto err_init_free_nomsg;
+   }
+
len = (hdr_buf[2] << 8) + hdr_buf[3] + 4;
buf = kzalloc(len, GFP_KERNEL);
if (!buf)
@@ -646,9 +655,10 @@ static int ses_intf_add(struct device *cdev,
kfree(ses_dev->page2);
kfree(ses_dev->page1);
  err_init_free:
+   sdev_printk(KERN_ERR, sdev, "Failed to bind enclosure %d\n",
err);
+ err_init_free_nomsg:
kfree(ses_dev);
kfree(hdr_buf);
-   sdev_printk(KERN_ERR, sdev, "Failed to bind enclosure %d\n",
err);
return err;
 }

Otherwise we go off issuing commands for pages the device says it won't
return.  I don't see offhand how this would cause KASAN errors though.

-Ewan


--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mpt3sas: add PCI dependency for CONFIG_SCSI_MPT2SAS

[James Bottomley]

On Fri, 2015-12-04 at 15:27 +0100, Arnd Bergmann wrote:
> CONFIG_SCSI_MPT2SAS was added as a backwards-compatibility helper that
> selects the replacement SCSI_MPT3SAS symbol, but lacks the dependencies:
> 
> warning: (SCSI_MPT2SAS) selects SCSI_MPT3SAS which has unmet direct 
> dependencies (SCSI_LOWLEVEL && PCI && SCSI)
> 0x7E5F9A79 Fri Dec 4 12:36:08 CET 2015 failed
> drivers/scsi/mpt3sas/mpt3sas_base.c: In function 
> 'mpt3sas_remove_dead_ioc_func':
> drivers/scsi/mpt3sas/mpt3sas_base.c:140:2: error: implicit declaration of 
> function 'pci_stop_and_remove_bus_device_locked' 
> [-Werror=implicit-function-declaration]
> drivers/scsi/mpt3sas/mpt3sas_base.c: In function '_base_disable_msix':
> drivers/scsi/mpt3sas/mpt3sas_base.c:1921:2: error: implicit declaration of 
> function 'pci_disable_msix' [-Werror=implicit-function-declaration]
> 
> This adds the same dependencies that SCSI_MPT3SAS has.

OK, you're about the fifth person to complain about this and this patch
was posted a few days ago and is now here:

http://git.kernel.org/cgit/linux/kernel/git/jejb/scsi.git/commit/?h=fixes=3ddda3e4c82dea58933bde8d0f6ef34470c360cb

It's even been in for-next for nearly 24h

James

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: kernel BUG at drivers/scsi/scsi_lib.c:1096!

[Jens Axboe]

On 12/04/2015 09:59 AM, Takashi Iwai wrote:

On Wed, 25 Nov 2015 20:01:47 +0100,
Hannes Reinecke wrote:


On 11/25/2015 07:01 PM, Mike Snitzer wrote:

On Wed, Nov 25 2015 at  4:04am -0500,
Hannes Reinecke  wrote:


On 11/20/2015 04:28 PM, Ewan Milne wrote:

On Fri, 2015-11-20 at 15:55 +0100, Hannes Reinecke wrote:

Can't we have a joint effort here?
I've been spending a _LOT_ of time trying to debug things here, but
none of the ideas I've come up with have been able to fix anything.


Yes.  I'm not the one primarily looking at it, and we don't have a
reproducer in-house.  We just have the one dump right now.



I'm almost tempted to increase the count from scsi_alloc_sgtable()
by one and be done with ...



That might not fix it if it is a problem with the merge code, though.


And indeed, it doesn't.


How did you arrive at that?  Do you have a reproducer now?


Not a reproducer, but several dumps for analysis.


Seems I finally found the culprit.

What happens is this:
We have two paths, with these seg_boundary_masks:

path-1:seg_boundary_mask = 65535,
path-2:seg_boundary_mask = 4294967295,

consequently the DM request queue has this:

md-1:seg_boundary_mask = 65535,

What happens now is that a request is being formatted, and sent
to path 2. During submission req->nr_phys_segments is formatted
with the limits of path 2, arriving at a count of 3.
Now the request gets retried on path 1, but as the NOMERGE request
flag is set req->nr_phys_segments is never updated.
But blk_rq_map_sg() ignores all counters, and just uses the
bi_vec directly, resulting in a count of 4 -> boom.

So the culprit here is the NOMERGE flag,


NOMERGE is always set in __blk_rq_prep_clone() for cloned requests.


Yes.


which is evaluated via
->dm_dispatch_request()
->blk_insert_cloned_request()
  ->blk_rq_check_limits()


blk_insert_cloned_request() is the only caller of blk_rq_check_limits();
anyway after reading your mail I'm still left wondering if your proposed
patch is correct.


If the above assessment is correct, the following patch should
fix it:

diff --git a/block/blk-core.c b/block/blk-core.c
index 801ced7..12cccd6 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1928,7 +1928,7 @@ EXPORT_SYMBOL(submit_bio);
*/
   int blk_rq_check_limits(struct request_queue *q, struct request *rq)
   {
-   if (!rq_mergeable(rq))
+   if (rq->cmd_type != REQ_TYPE_FS)
  return 0;

  if (blk_rq_sectors(rq) > blk_queue_get_max_sectors(q,
rq->cmd_flags)) {


Mike? Jens?
Can you comment on it?


You're not explaining the actual change in the patch very well; I think
you're correct but you're leaving the justification as an exercise to
the reviewer:

blk_rq_check_limits() will call blk_recalc_rq_segments() after the
!rq_mergeable() check but you're saying for this case in question we
never get there -- due to the cloned request having NOMERGE set.

So in blk_rq_check_limits() you've unrolled rq_mergeable() and
open-coded the lone remaining check (rq->cmd_type != REQ_TYPE_FS)

I agree that the (rq->cmd_flags & REQ_NOMERGE_FLAGS) check in
the blk_insert_cloned_request() call-chain (via rq_mergeable()) makes no
sense for cloned requests that always have NOMERGE set.

So you're saying that by having blk_rq_check_limits() go on to call
blk_recalc_rq_segments() this bug will be fixed?


That is the idea.

I've already established that in all instances I have seen so far
req->nr_phys_segments is _less_ than req->bio->bi_phys_segments.

As it turns out, req->nr_phys_segemnts _would_ have been updated in
blk_rq_check_limits(), but isn't due to the NOMERGE flag being set
for the cloned request.
So each cloned request inherits the values from the original request,
despite the fact that req->nr_phys_segments _has_ to be evaluated in
the final request_queue context, as the queue limits _might_ be
different from the original (merged) queue limits of the multipath
request queue.


BTW, I think blk_rq_check_limits()'s export should be removed and the
function made static and renamed to blk_clone_rq_check_limits(), again:
blk_insert_cloned_request() is the only caller of blk_rq_check_limits()


Actually, seeing Jens' last comment the check for REQ_TYPE_FS is
pointless, too, so we might as well remove the entire if-clause.


Seems prudent to make that change now to be clear that this code is only
used by cloned requests.


Yeah, that would make sense. I'll be preparing a patch.
With a more detailed description :-)


Do we have already a fix?  Right now I got (likely) this kernel BUG()
on the almost latest Linus tree (commit 25364a9e54fb8296).  It
happened while I started a KVM right after a fresh boot.  The machine
paniced even before that, so I hit this twice today.


Update to the tree as-of yesterday (or today) and it should work. 
25364a9e54fb8296 doesn't include the latest block fixes that were sent 
in yesterday, that should fix it. You need commit a88d32af18b8 or newer.


--

Re: [PATCH v2 0/3] Badblock tracking for gendisks

[Verma, Vishal L]

On Wed, 2015-11-25 at 11:43 -0700, Vishal Verma wrote:
> v2:
>   - In badblocks_free, make 'page' NULL (patch 1)
>   - Move the core badblocks code to a new .c file (patch 1) (Jens)
>   - Fix a sizeof usage in disk_alloc_badblocks (patch 2) (Dan)
>   - Since disk_alloc_badblocks can fail, check disk->bb for NULL in
> the
> genhd wrappers (patch 2) (Jeff)
>   - Update the md conversion to also ise the badblocks init and free
> functions (patch 3)
>   - Remove the BB_* macros from md.h as they are now in badblocks.h
> (patch 3)
> 
> Patch 1 copies badblock management code into a header of its own,
> making it generally available. It follows common libraries of code
> such as linked lists, where anyone may embed a core data structure
> in another place, and use the provided accessor functions to
> manipulate the data.
> 
> Patch 2 adds badblock tracking to gendisks (in preparation for use
> by NVDIMM devices). Right now, it is turned on unconditionally - I'd
> appreciate comments on if that is the right path.
> 
> Patch 3 converts md over to use the new badblocks 'library'. I have
> done some pretty simple testing on this - created a raid 1 device,
> made sure the sysfs entries show up, and can be used to add and view
> badblocks. A closer look by the md folks would be nice here.
> 
> 
> Vishal Verma (3):
>   badblocks: Add core badblock management code
>   block: Add badblock management for gendisks
>   md: convert to use the generic badblocks code
> 

Ping.

Jens, are you ok taking this through the block tree?
Any other comments from anyone else?

Thanks,
-Vishal

Re: [PATCH v2 1/3] badblocks: Add core badblock management code

[James Bottomley]

On Wed, 2015-11-25 at 11:43 -0700, Vishal Verma wrote:
> Take the core badblocks implementation from md, and make it generally
> available. This follows the same style as kernel implementations of
> linked lists, rb-trees etc, where you can have a structure that can be
> embedded anywhere, and accessor functions to manipulate the data.
> 
> The only changes in this copy of the code are ones to generalize
> function/variable names from md-specific ones. Also add init and free
> functions.
> 
> Signed-off-by: Vishal Verma 
> ---
>  block/Makefile|   2 +-
>  block/badblocks.c | 523 
> ++
>  include/linux/badblocks.h |  53 +
>  3 files changed, 577 insertions(+), 1 deletion(-)
>  create mode 100644 block/badblocks.c
>  create mode 100644 include/linux/badblocks.h
> 
> diff --git a/block/Makefile b/block/Makefile
> index 00ecc97..db5f622 100644
> --- a/block/Makefile
> +++ b/block/Makefile
> @@ -8,7 +8,7 @@ obj-$(CONFIG_BLOCK) := bio.o elevator.o blk-core.o blk-tag.o 
> blk-sysfs.o \
>   blk-iopoll.o blk-lib.o blk-mq.o blk-mq-tag.o \
>   blk-mq-sysfs.o blk-mq-cpu.o blk-mq-cpumap.o ioctl.o \
>   genhd.o scsi_ioctl.o partition-generic.o ioprio.o \
> - partitions/
> + badblocks.o partitions/
>  
>  obj-$(CONFIG_BOUNCE) += bounce.o
>  obj-$(CONFIG_BLK_DEV_BSG)+= bsg.o
> diff --git a/block/badblocks.c b/block/badblocks.c
> new file mode 100644
> index 000..6e07855
> --- /dev/null
> +++ b/block/badblocks.c
> @@ -0,0 +1,523 @@
> +/*
> + * Bad block management
> + *
> + * - Heavily based on MD badblocks code from Neil Brown
> + *
> + * Copyright (c) 2015, Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/*
> + * We can record which blocks on each device are 'bad' and so just
> + * fail those blocks, or that stripe, rather than the whole device.
> + * Entries in the bad-block table are 64bits wide.  This comprises:
> + * Length of bad-range, in sectors: 0-511 for lengths 1-512
> + * Start of bad-range, sector offset, 54 bits (allows 8 exbibytes)
> + *  A 'shift' can be set so that larger blocks are tracked and
> + *  consequently larger devices can be covered.
> + * 'Acknowledged' flag - 1 bit. - the most significant bit.
> + *
> + * Locking of the bad-block table uses a seqlock so badblocks_check
> + * might need to retry if it is very unlucky.
> + * We will sometimes want to check for bad blocks in a bi_end_io function,
> + * so we use the write_seqlock_irq variant.
> + *
> + * When looking for a bad block we specify a range and want to
> + * know if any block in the range is bad.  So we binary-search
> + * to the last range that starts at-or-before the given endpoint,
> + * (or "before the sector after the target range")
> + * then see if it ends after the given start.
> + * We return
> + *  0 if there are no known bad blocks in the range
> + *  1 if there are known bad block which are all acknowledged
> + * -1 if there are bad blocks which have not yet been acknowledged in 
> metadata.
> + * plus the start/length of the first bad section we overlap.
> + */

This comment should be docbook.

> +int badblocks_check(struct badblocks *bb, sector_t s, int sectors,
> + sector_t *first_bad, int *bad_sectors)
[...]
> +
> +/*
> + * Add a range of bad blocks to the table.
> + * This might extend the table, or might contract it
> + * if two adjacent ranges can be merged.
> + * We binary-search to find the 'insertion' point, then
> + * decide how best to handle it.
> + */

And this one, plus you don't document returns.  It looks like this
function returns 1 on success and zero on failure, which is really
counter-intuitive for the kernel: zero is usually returned on success
and negative error on failure.

> +int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
> + int acknowledged)
[...]
> +
> +/*
> + * Remove a range of bad blocks from the table.
> + * This may involve extending the table if we spilt a region,
> + * but it must not fail.  So if the table becomes full, we just
> + * drop the remove request.
> + */

Docbook and document returns.  This time they're the kernel standard of
0 on success and negative error on failure making the convention for
badblocks_set even more counterintuitive.

> +int badblocks_clear(struct 

RE: [PATCH] storvsc: add more logging for error and warning messages

[Long Li]

> -Original Message-
> From: Vitaly Kuznetsov [mailto:vkuzn...@redhat.com]
> Sent: Friday, December 4, 2015 1:53 AM
> To: Long Li 
> Cc: KY Srinivasan ; Haiyang Zhang
> ; James E.J. Bottomley ;
> de...@linuxdriverproject.org; linux-ker...@vger.kernel.org; linux-
> s...@vger.kernel.org
> Subject: Re: [PATCH] storvsc: add more logging for error and warning
> messages
> 
> Long Li  writes:
> 
> > Introduce a logging level for storvsc to log certain error/warning
> > messages. Those messages are helpful in some environments, e.g.
> > Microsoft Azure, for customer support and troubleshooting purposes.
> 
> I have an alternative suggestion: let's use dynamic debug! Basically, we need
> to convert all non-error logging to using dev_dbg() and this can be enabled
> dynamically when needed, even reboot won't be required.

This is great idea for debugging!

I think the messages (srb errors) we want to log in this patch are real errors. 
They are not for debugging, but for customer support in production environment.

Those errors can be ignored in certain specific storage configurations due to 
some quirks. They are real errors on Azure, so we want to always log them.
 
> 
> >
> > Signed-off-by: Long Li 
> > ---
> >  drivers/scsi/storvsc_drv.c | 30 +-
> >  1 file changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> > index 40c43ae..afa1647 100644
> > --- a/drivers/scsi/storvsc_drv.c
> > +++ b/drivers/scsi/storvsc_drv.c
> > @@ -164,6 +164,21 @@ static int sense_buffer_size =
> > PRE_WIN8_STORVSC_SENSE_BUFFER_SIZE;
> >  */
> >  static int vmstor_proto_version;
> >
> > +#define STORVSC_LOGGING_NONE   0
> > +#define STORVSC_LOGGING_ERROR  1
> > +#define STORVSC_LOGGING_WARN   2
> > +
> > +static int logging_level = STORVSC_LOGGING_ERROR;
> > +module_param(logging_level, int, S_IRUGO|S_IWUSR);
> > +MODULE_PARM_DESC(logging_level,
> > +   "Logging level, 0 - None, 1 - Error (default), 2 - Warning.");
> > +
> > +static inline bool do_logging(int level) {
> > +   return (logging_level >= level) ? true : false; }
> > +
> > +
> >  struct vmscsi_win8_extension {
> > /*
> >  * The following were added in Windows 8 @@ -1183,7 +1198,7 @@
> > static void storvsc_command_completion(struct storvsc_cmd_request
> > *cmd_request)
> >
> > scmnd->result = vm_srb->scsi_status;
> >
> > -   if (scmnd->result) {
> > +   if (scmnd->result && do_logging(STORVSC_LOGGING_ERROR)) {
> > if (scsi_normalize_sense(scmnd->sense_buffer,
> > SCSI_SENSE_BUFFERSIZE, _hdr))
> > scsi_print_sense_hdr(scmnd->device, "storvsc", @@
> -1239,12
> > +1254,25 @@ static void storvsc_on_io_completion(struct hv_device
> *device,
> > stor_pkt->vm_srb.sense_info_length =
> > vstor_packet->vm_srb.sense_info_length;
> >
> > +   if (vstor_packet->vm_srb.scsi_status != 0 ||
> > +   vstor_packet->vm_srb.srb_status != SRB_STATUS_SUCCESS)
> > +   if (do_logging(STORVSC_LOGGING_WARN))
> > +   dev_warn(>device,
> > +   "cmd 0x%x scsi status 0x%x srb status
> 0x%x\n",
> > +   stor_pkt->vm_srb.cdb[0],
> > +   vstor_packet->vm_srb.scsi_status,
> > +   vstor_packet->vm_srb.srb_status);
> >
> > if ((vstor_packet->vm_srb.scsi_status & 0xFF) == 0x02) {
> > /* CHECK_CONDITION */
> > if (vstor_packet->vm_srb.srb_status &
> > SRB_STATUS_AUTOSENSE_VALID) {
> > /* autosense data available */
> > +   if (do_logging(STORVSC_LOGGING_WARN))
> > +   dev_warn(>device,
> > +   "stor pkt %p autosense data valid -
> len %d\n",
> > +   request,
> > +   vstor_packet-
> >vm_srb.sense_info_length);
> >
> > memcpy(request->cmd->sense_buffer,
> >vstor_packet->vm_srb.sense_data,
> 
> --
>   Vitaly
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 73/71] ncr5380: Use runtime register mapping

[Ondrej Zary]

Convert compile-time C400_ register mapping to runtime mapping.
This removes the weird negative register offsets and allows adding
additional mappings.

Signed-off-by: Ondrej Zary 
---
 drivers/scsi/NCR5380.h   |   13 +-
 drivers/scsi/g_NCR5380.c |   61 ++
 drivers/scsi/g_NCR5380.h |   12 ++---
 3 files changed, 43 insertions(+), 43 deletions(-)

diff --git a/drivers/scsi/NCR5380.h b/drivers/scsi/NCR5380.h
index 36779df..923db6c 100644
--- a/drivers/scsi/NCR5380.h
+++ b/drivers/scsi/NCR5380.h
@@ -163,8 +163,7 @@
 /* Write any value to this register to start an ini mode DMA receive */
 #define START_DMA_INITIATOR_RECEIVE_REG 7  /* wo */
 
-#define C400_CONTROL_STATUS_REG NCR53C400_register_offset-8/* rw */
-
+/* NCR 53C400(A) Control Status Register bits: */
 #define CSR_RESET  0x80/* wo  Resets 53c400 */
 #define CSR_53C80_REG  0x80/* ro  5380 registers busy */
 #define CSR_TRANS_DIR  0x40/* rw  Data transfer direction */
@@ -181,16 +180,6 @@
 #define CSR_BASE CSR_53C80_INTR
 #endif
 
-/* Number of 128-byte blocks to be transferred */
-#define C400_BLOCK_COUNTER_REG   NCR53C400_register_offset-7   /* rw */
-
-/* Resume transfer after disconnect */
-#define C400_RESUME_TRANSFER_REG NCR53C400_register_offset-6   /* wo */
-
-/* Access to host buffer stack */
-#define C400_HOST_BUFFER NCR53C400_register_offset-4   /* rw */
-
-
 /* Note : PHASE_* macros are based on the values of the STATUS register */
 #define PHASE_MASK (SR_MSG | SR_CD | SR_IO)
 
diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index a9a237f..ce444da 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -253,6 +253,7 @@ static int __init generic_NCR5380_detect(struct 
scsi_host_template *tpnt)
};
int flags;
struct Scsi_Host *instance;
+   struct NCR5380_hostdata *hostdata;
 #ifdef SCSI_G_NCR5380_MEM
unsigned long base;
void __iomem *iomem;
@@ -395,6 +396,7 @@ static int __init generic_NCR5380_detect(struct 
scsi_host_template *tpnt)
instance = scsi_register(tpnt, sizeof(struct NCR5380_hostdata));
if (instance == NULL)
goto out_release;
+   hostdata = shost_priv(instance);
 
 #ifndef SCSI_G_NCR5380_MEM
instance->io_port = 
overrides[current_override].NCR5380_map_name;
@@ -404,18 +406,27 @@ static int __init generic_NCR5380_detect(struct 
scsi_host_template *tpnt)
 * On NCR53C400 boards, NCR5380 registers are mapped 8 past
 * the base address.
 */
-   if (overrides[current_override].board == BOARD_NCR53C400)
+   if (overrides[current_override].board == BOARD_NCR53C400) {
instance->io_port += 8;
+   hostdata->c400_ctl_status = 0;
+   hostdata->c400_blk_cnt = 1;
+   hostdata->c400_host_buf = 4;
+   }
 #else
instance->base = overrides[current_override].NCR5380_map_name;
-   ((struct NCR5380_hostdata *)instance->hostdata)->iomem = iomem;
+   hostdata->iomem = iomem;
+   if (overrides[current_override].board == BOARD_NCR53C400) {
+   hostdata->c400_ctl_status = 0x100;
+   hostdata->c400_blk_cnt = 0x101;
+   hostdata->c400_host_buf = 0x104;
+   }
 #endif
 
if (NCR5380_init(instance, flags))
goto out_unregister;
 
if (overrides[current_override].board == BOARD_NCR53C400)
-   NCR5380_write(C400_CONTROL_STATUS_REG, CSR_BASE);
+   NCR5380_write(hostdata->c400_ctl_status, CSR_BASE);
 
NCR5380_maybe_reset_bus(instance);
 
@@ -523,30 +534,28 @@ generic_NCR5380_biosparam(struct scsi_device *sdev, 
struct block_device *bdev,
  
 static inline int NCR5380_pread(struct Scsi_Host *instance, unsigned char 
*dst, int len)
 {
-#ifdef SCSI_G_NCR5380_MEM
struct NCR5380_hostdata *hostdata = shost_priv(instance);
-#endif
int blocks = len / 128;
int start = 0;
int bl;
 
-   NCR5380_write(C400_CONTROL_STATUS_REG, CSR_BASE | CSR_TRANS_DIR);
-   NCR5380_write(C400_BLOCK_COUNTER_REG, blocks);
+   NCR5380_write(hostdata->c400_ctl_status, CSR_BASE | CSR_TRANS_DIR);
+   NCR5380_write(hostdata->c400_blk_cnt, blocks);
while (1) {
-   if ((bl = NCR5380_read(C400_BLOCK_COUNTER_REG)) == 0) {
+   if ((bl = NCR5380_read(hostdata->c400_blk_cnt)) == 0) {
break;
}
-   if (NCR5380_read(C400_CONTROL_STATUS_REG) & 
CSR_GATED_53C80_IRQ) {
+   if (NCR5380_read(hostdata->c400_ctl_status) & 
CSR_GATED_53C80_IRQ) {
printk(KERN_ERR 

Re: BUG: KASAN: slab-out-of-bounds in ses_enclosure_data_process+0x900/0xe50

[Ewan Milne]

On Fri, 2015-12-04 at 11:16 -0800, James Bottomley wrote:
> On Fri, 2015-12-04 at 11:58 -0500, Ewan Milne wrote:
> > On Thu, 2015-12-03 at 23:20 +0100, Andrea Gelmini wrote:
> > > On Thu, Dec 03, 2015 at 12:59:06PM -0800, James Bottomley wrote:
> > > > sg_map -i
> > > > 
> > > > in your system, you should see something with an inquiry string like
> > > > enclosure.  It's the /dev/sg of that you need to run sg_ses on.
> > > 
> > > root@glen:/home/gelma# sg_map -i
> > > /dev/sg0  /dev/sda  ATA   Samsung SSD 850   1B6Q
> > > /dev/sg1  /dev/sr0  HL-DT-ST  DVDRAM GU40N  QX23
> > > /dev/sg2  /dev/sdb  WDMy Passport 0820  1007
> > > /dev/sg3  WDSES Device1007
> > > 
> > > And following Douglas' instructions:
> > > 
> > > root@glen:/home/gelma# lsscsi -gs
> > > [0:0:0:0]diskATA  Samsung SSD 850  1B6Q  /dev/sda   /dev/sg0  
> > >  1.02TB
> > > [1:0:0:0]cd/dvd  HL-DT-ST DVDRAM GU40N QX23  /dev/sr0   /dev/sg1  
> > >   -
> > > [8:0:0:0]diskWD   My Passport 0820 1007  /dev/sdb   /dev/sg2  
> > >  2.00TB
> > > [8:0:0:1]enclosu WD   SES Device   1007  -  /dev/sg3  
> > >  
> > > 
> > > root@glen:/home/gelma# sg_ses /dev/sg3
> > >   WDSES Device1007
> > > Supported diagnostic pages:
> > >   Supported Diagnostic Pages [sdp] [0x0]
> > >   Short Enclosure Status (SES) [ses] [0x8]
> > >[0x80]
> > >[0x83]
> > >[0x84]
> > >[0x85]
> > > 
> > > 
> > > Well, if it's better for you, I can give you root access to a machine 
> > > with this device
> > > connected to.
> > > 
> > > Thanks a lot for your time,
> > > Andrea
> > 
> > There seems to be a problem with the ses code if the device only reports
> > Short Enclosure Status.  We probably need to do something like:
> > 
> > diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
> > index eba183c..065a528 100644
> > --- a/drivers/scsi/ses.c
> > +++ b/drivers/scsi/ses.c
> > @@ -537,6 +537,15 @@ static int ses_intf_add(struct device *cdev,
> > if (result)
> > goto recv_failed;
> >  
> > +   /* If enclosure only supports Short Enclosure Status page (08),
> > +* it returns that page regardless of what we requested, and
> > +* only returns vendor-specific status.  There is nothing wrong
> > +* with such enclosures, we just can't make use of them. */
> > +   if (hdr_buf[0] == 8) {
> > +   err = -ENODEV;
> > +   goto err_init_free_nomsg;
> > +   }
> > +
> > len = (hdr_buf[2] << 8) + hdr_buf[3] + 4;
> > buf = kzalloc(len, GFP_KERNEL);
> > if (!buf)
> > @@ -646,9 +655,10 @@ static int ses_intf_add(struct device *cdev,
> > kfree(ses_dev->page2);
> > kfree(ses_dev->page1);
> >   err_init_free:
> > +   sdev_printk(KERN_ERR, sdev, "Failed to bind enclosure %d\n",
> > err);
> > + err_init_free_nomsg:
> > kfree(ses_dev);
> > kfree(hdr_buf);
> > -   sdev_printk(KERN_ERR, sdev, "Failed to bind enclosure %d\n",
> > err);
> > return err;
> >  }
> > 
> > Otherwise we go off issuing commands for pages the device says it won't
> > return.  I don't see offhand how this would cause KASAN errors though.
> 
> I think we have two separate bugs.  One is the usual USB devices getting
> into a new SCSI standard and getting it wrong.  The other looks to be an
> enumeration problem ... possibly because ses2 added an indexed
> descriptor which current SES doesn't cope with.
> 
> Anyway, in concentrating on the USB problem first, I think we need to
> code a little more defensively, like this.

This could certainly be the case for a USB device, however I have also
had a report of this for regular SCSI attached devices with EncServ=1
that only supported Short Enclosure Status.  The SES-3 spec 4.3.3 says
that in this case, it "...shall always return the Short Enclosure
Status diagnostic page, regardless of which SES diagnostic page is
requested..."  The person who reported the problem wanted to have the
error messages removed about all his spec-compliant devices.

Short Enclosure Status appears to be vendor-specific, it doesn't look
like we can do anything with it.

But anyway, there's still the other problem...

-Ewan

> 
> James
> 
> ---
> 
> diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
> index dcb0d76..7d9cec5 100644
> --- a/drivers/scsi/ses.c
> +++ b/drivers/scsi/ses.c
> @@ -84,6 +84,7 @@ static void init_device_slot_control(unsigned char 
> *dest_desc,
>  static int ses_recv_diag(struct scsi_device *sdev, int page_code,
>void *buf, int bufflen)
>  {
> + int ret;
>   unsigned char cmd[] = {
>   RECEIVE_DIAGNOSTIC,
>   1,  /* Set PCV bit */
> @@ -92,9 +93,26 @@ static int ses_recv_diag(struct scsi_device *sdev, int 
> page_code,
>   bufflen & 0xff,
>   0
>   };
> + unsigned char recv_page_code;
>  
> - return scsi_execute_req(sdev, 

Re: [PATCH v2] VMW_PVSCSI: Fix the issue of DMA-API related warnings.

[Arvind Kumar]

Thanks Josh!

The patch looks good to me.

Thanks!
Arvind

From: Johannes Thumshirn 
Sent: Thursday, December 3, 2015 5:35 AM
To: Josh Boyer; james.bottom...@hansenpartnership.com; Arvind Kumar; Thomas 
Hellstrom
Cc: linux-scsi@vger.kernel.org; pv-driv...@vmware.com; 
linux-ker...@vger.kernel.org
Subject: Re: [PATCH v2] VMW_PVSCSI: Fix the issue of DMA-API related warnings.

On Thu, 2015-12-03 at 08:27 -0500, Josh Boyer wrote:
> The driver is missing calls to pci_dma_mapping_error() after
> performing the DMA mapping, which caused DMA-API warning to
> show up in dmesg's output. Though that happens only when
> DMA_API_DEBUG option is enabled. This change fixes the issue
> and makes pvscsi_map_buffers() function more robust.
>
> Signed-off-by: Arvind Kumar 
> Cc: Josh Boyer 
> Reviewed-by: Thomas Hellstrom 
> Signed-off-by: Josh Boyer 
> ---
>
>  v2: Use -ENOMEM instead of -1 for the error return code as suggested by
>  Johannes Thumshirn
>
>  drivers/scsi/vmw_pvscsi.c | 45 +++--
>  drivers/scsi/vmw_pvscsi.h |  2 +-
>  2 files changed, 40 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/vmw_pvscsi.c b/drivers/scsi/vmw_pvscsi.c
> index 0f133c1817de..6164634aff18 100644
> --- a/drivers/scsi/vmw_pvscsi.c
> +++ b/drivers/scsi/vmw_pvscsi.c
> @@ -349,9 +349,9 @@ static void pvscsi_create_sg(struct pvscsi_ctx *ctx,
>   * Map all data buffers for a command into PCI space and
>   * setup the scatter/gather list if needed.
>   */
> -static void pvscsi_map_buffers(struct pvscsi_adapter *adapter,
> -struct pvscsi_ctx *ctx, struct scsi_cmnd
> *cmd,
> -struct PVSCSIRingReqDesc *e)
> +static int pvscsi_map_buffers(struct pvscsi_adapter *adapter,
> +   struct pvscsi_ctx *ctx, struct scsi_cmnd *cmd,
> +   struct PVSCSIRingReqDesc *e)
>  {
>   unsigned count;
>   unsigned bufflen = scsi_bufflen(cmd);
> @@ -360,18 +360,30 @@ static void pvscsi_map_buffers(struct pvscsi_adapter
> *adapter,
>   e->dataLen = bufflen;
>   e->dataAddr = 0;
>   if (bufflen == 0)
> - return;
> + return 0;
>
>   sg = scsi_sglist(cmd);
>   count = scsi_sg_count(cmd);
>   if (count != 0) {
>   int segs = scsi_dma_map(cmd);
> - if (segs > 1) {
> +
> + if (segs == -ENOMEM) {
> + scmd_printk(KERN_ERR, cmd,
> + "vmw_pvscsi: Failed to map cmd sglist
> for DMA.\n");
> + return -ENOMEM;
> + } else if (segs > 1) {
>   pvscsi_create_sg(ctx, sg, segs);
>
>   e->flags |= PVSCSI_FLAG_CMD_WITH_SG_LIST;
>   ctx->sglPA = pci_map_single(adapter->dev, ctx->sgl,
>   SGL_SIZE,
> PCI_DMA_TODEVICE);
> + if (pci_dma_mapping_error(adapter->dev, ctx->sglPA))
> {
> + scmd_printk(KERN_ERR, cmd,
> + "vmw_pvscsi: Failed to map ctx
> sglist for DMA.\n");
> + scsi_dma_unmap(cmd);
> + ctx->sglPA = 0;
> + return -ENOMEM;
> + }
>   e->dataAddr = ctx->sglPA;
>   } else
>   e->dataAddr = sg_dma_address(sg);
> @@ -382,8 +394,15 @@ static void pvscsi_map_buffers(struct pvscsi_adapter
> *adapter,
>*/
>   ctx->dataPA = pci_map_single(adapter->dev, sg, bufflen,
>cmd->sc_data_direction);
> + if (pci_dma_mapping_error(adapter->dev, ctx->dataPA)) {
> + scmd_printk(KERN_ERR, cmd,
> + "vmw_pvscsi: Failed to map direct data
> buffer for DMA.\n");
> + return -ENOMEM;
> + }
>   e->dataAddr = ctx->dataPA;
>   }
> +
> + return 0;
>  }
>
>  static void pvscsi_unmap_buffers(const struct pvscsi_adapter *adapter,
> @@ -690,6 +709,12 @@ static int pvscsi_queue_ring(struct pvscsi_adapter
> *adapter,
>   ctx->sensePA = pci_map_single(adapter->dev, cmd-
> >sense_buffer,
> SCSI_SENSE_BUFFERSIZE,
> PCI_DMA_FROMDEVICE);
> + if (pci_dma_mapping_error(adapter->dev, ctx->sensePA)) {
> + scmd_printk(KERN_ERR, cmd,
> + "vmw_pvscsi: Failed to map sense buffer
> for DMA.\n");
> + ctx->sensePA = 0;
> + return -ENOMEM;
> + }
>   e->senseAddr = ctx->sensePA;
>   

Re: [PATCH v2 2/3] block: Add badblock management for gendisks

[James Bottomley]

On Wed, 2015-11-25 at 11:43 -0700, Vishal Verma wrote:
> NVDIMM devices, which can behave more like DRAM rather than block
> devices, may develop bad cache lines, or 'poison'. A block device
> exposed by the pmem driver can then consume poison via a read (or
> write), and cause a machine check. On platforms without machine
> check recovery features, this would mean a crash.
> 
> The block device maintaining a runtime list of all known sectors that
> have poison can directly avoid this, and also provide a path forward
> to enable proper handling/recovery for DAX faults on such a device.
> 
> Use the new badblock management interfaces to add a badblocks list to
> gendisks.
> 
> Signed-off-by: Vishal Verma 
> ---
>  block/genhd.c | 81 
> +++
>  include/linux/genhd.h |  6 
>  2 files changed, 87 insertions(+)
> 
> diff --git a/block/genhd.c b/block/genhd.c
> index 0c706f3..84fd65c 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -20,6 +20,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "blk.h"
>  
> @@ -505,6 +506,20 @@ static int exact_lock(dev_t devt, void *data)
>   return 0;
>  }
>  
> +static void disk_alloc_badblocks(struct gendisk *disk)
> +{
> + disk->bb = kzalloc(sizeof(*(disk->bb)), GFP_KERNEL);
> + if (!disk->bb) {
> + pr_warn("%s: failed to allocate space for badblocks\n",
> + disk->disk_name);
> + return;
> + }
> +
> + if (badblocks_init(disk->bb, 1))
> + pr_warn("%s: failed to initialize badblocks\n",
> + disk->disk_name);
> +}
> +
>  static void register_disk(struct gendisk *disk)
>  {
>   struct device *ddev = disk_to_dev(disk);
> @@ -609,6 +624,7 @@ void add_disk(struct gendisk *disk)
>   disk->first_minor = MINOR(devt);
>  
>   disk_alloc_events(disk);
> + disk_alloc_badblocks(disk);

Why unconditionally do this?  No-one currently uses the interface, but
every disk will now pay the price of an additional structure plus a page
for no benefit.  You should probably either export the initializer for
those who want to use it or, perhaps even better, make it lazily
allocated the first time anyone tries to set a bad block.

If you come up with a really good reason for allocating it
unconditionally, then it should probably be an embedded structure in the
gendisk.

James


--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 76/71] ncr5380: Enable PDMA for DTC chips

[Ondrej Zary]

Add I/O register mapping for DTC chips and enable PDMA mode.

These chips have 16-bit wide HOST BUFFER register (counter register at
offset 0x0d increments by 2 on each HOST BUFFER read). Detect it
automatically.

Large PIO transfers crash at least the DTCT-436P chip (all reads result
in 0xFF) so this patch actually makes it work.

The chip also crashes when we bang the C400 host status register too
heavily after PDMA write - a small udelay is needed.

Tested on DTCT-436P and verified that it does not break 53C400A.

Signed-off-by: Ondrej Zary 
---
 drivers/scsi/g_NCR5380.c |   59 --
 drivers/scsi/g_NCR5380.h |4 +++-
 2 files changed, 39 insertions(+), 24 deletions(-)

diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index 85da3c2..9816b81 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -328,7 +328,7 @@ static int __init generic_NCR5380_detect(struct 
scsi_host_template *tpnt)
ports = ncr_53c400a_ports;
break;
case BOARD_DTC3181E:
-   flags = FLAG_NO_PSEUDO_DMA;
+   flags = FLAG_NO_DMA_FIXUP;
ports = dtc_3181e_ports;
break;
}
@@ -412,10 +412,12 @@ static int __init generic_NCR5380_detect(struct 
scsi_host_template *tpnt)
hostdata->c400_blk_cnt = 1;
hostdata->c400_host_buf = 4;
}
-   if (overrides[current_override].board == BOARD_NCR53C400A) {
+   if (overrides[current_override].board == BOARD_NCR53C400A ||
+   overrides[current_override].board == BOARD_DTC3181E) {
hostdata->c400_ctl_status = 9;
hostdata->c400_blk_cnt = 10;
hostdata->c400_host_buf = 8;
+   hostdata->c400_host_idx = 13;
}
 #else
instance->base = overrides[current_override].NCR5380_map_name;
@@ -430,8 +432,20 @@ static int __init generic_NCR5380_detect(struct 
scsi_host_template *tpnt)
if (NCR5380_init(instance, flags))
goto out_unregister;
 
+#ifndef SCSI_G_NCR5380_MEM
+   /* read initial value of index register */
+   i = NCR5380_read(hostdata->c400_host_idx);
+   /* read something from host buffer */
+   NCR5380_read(hostdata->c400_host_buf);
+   /* I/O width = index register increment */
+   hostdata->io_width = NCR5380_read(hostdata->c400_host_idx) - i;
+   if (hostdata->io_width < 0)
+   hostdata->io_width += 128;
+#endif
+
if (overrides[current_override].board == BOARD_NCR53C400 ||
-   overrides[current_override].board == BOARD_NCR53C400A)
+   overrides[current_override].board == BOARD_NCR53C400A ||
+   overrides[current_override].board == BOARD_DTC3181E)
NCR5380_write(hostdata->c400_ctl_status, CSR_BASE);
 
NCR5380_maybe_reset_bus(instance);
@@ -558,11 +572,10 @@ static inline int NCR5380_pread(struct Scsi_Host 
*instance, unsigned char *dst,
while (NCR5380_read(hostdata->c400_ctl_status) & 
CSR_HOST_BUF_NOT_RDY);
 
 #ifndef SCSI_G_NCR5380_MEM
-   {
-   int i;
-   for (i = 0; i < 128; i++)
-   dst[start + i] = 
NCR5380_read(hostdata->c400_host_buf);
-   }
+   if (hostdata->io_width == 2)
+   insw(instance->io_port + hostdata->c400_host_buf, dst + 
start, 64);
+   else
+   insb(instance->io_port + hostdata->c400_host_buf, dst + 
start, 128);
 #else
/* implies SCSI_G_NCR5380_MEM */
memcpy_fromio(dst + start,
@@ -579,11 +592,10 @@ static inline int NCR5380_pread(struct Scsi_Host 
*instance, unsigned char *dst,
}
 
 #ifndef SCSI_G_NCR5380_MEM
-   {
-   int i;  
-   for (i = 0; i < 128; i++)
-   dst[start + i] = 
NCR5380_read(hostdata->c400_host_buf);
-   }
+   if (hostdata->io_width == 2)
+   insw(instance->io_port + hostdata->c400_host_buf, dst + 
start, 64);
+   else
+   insb(instance->io_port + hostdata->c400_host_buf, dst + 
start, 128);
 #else
/* implies SCSI_G_NCR5380_MEM */
memcpy_fromio(dst + start,
@@ -642,10 +654,10 @@ static inline int NCR5380_pwrite(struct Scsi_Host 
*instance, unsigned char *src,
while (NCR5380_read(hostdata->c400_ctl_status) & 
CSR_HOST_BUF_NOT_RDY)
; // FIXME - timeout
 #ifndef SCSI_G_NCR5380_MEM
-   {
-   

[PATCH 74/71] ncr5380: Enable PDMA for NCR53C400A

[Ondrej Zary]

Add I/O register mapping for NCR53C400A and enable PDMA mode to
improve performance and fix non-working IRQ.

Tested with HP C2502 (and user-space enabler).

Signed-off-by: Ondrej Zary 
---
 drivers/scsi/g_NCR5380.c |   10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index ce444da..cd483c6 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -324,7 +324,7 @@ static int __init generic_NCR5380_detect(struct 
scsi_host_template *tpnt)
 #endif
break;
case BOARD_NCR53C400A:
-   flags = FLAG_NO_PSEUDO_DMA;
+   flags = FLAG_NO_DMA_FIXUP;
ports = ncr_53c400a_ports;
break;
case BOARD_DTC3181E:
@@ -412,6 +412,11 @@ static int __init generic_NCR5380_detect(struct 
scsi_host_template *tpnt)
hostdata->c400_blk_cnt = 1;
hostdata->c400_host_buf = 4;
}
+   if (overrides[current_override].board == BOARD_NCR53C400A) {
+   hostdata->c400_ctl_status = 9;
+   hostdata->c400_blk_cnt = 10;
+   hostdata->c400_host_buf = 8;
+   }
 #else
instance->base = overrides[current_override].NCR5380_map_name;
hostdata->iomem = iomem;
@@ -425,7 +430,8 @@ static int __init generic_NCR5380_detect(struct 
scsi_host_template *tpnt)
if (NCR5380_init(instance, flags))
goto out_unregister;
 
-   if (overrides[current_override].board == BOARD_NCR53C400)
+   if (overrides[current_override].board == BOARD_NCR53C400 ||
+   overrides[current_override].board == BOARD_NCR53C400A)
NCR5380_write(hostdata->c400_ctl_status, CSR_BASE);
 
NCR5380_maybe_reset_bus(instance);
-- 
Ondrej Zary

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] badblocks: Add core badblock management code

[Verma, Vishal L]

On Fri, 2015-12-04 at 15:30 -0800, James Bottomley wrote:
[...]
> > + * We return
> > + *  0 if there are no known bad blocks in the range
> > + *  1 if there are known bad block which are all acknowledged
> > + * -1 if there are bad blocks which have not yet been acknowledged
> > in metadata.
> > + * plus the start/length of the first bad section we overlap.
> > + */
> 
> This comment should be docbook.

Applicable to all your comments - (and they are all valid), I simply
copied over all this from md. I'm happy to make the changes to comments,
and the other two things (see below) if that's the right thing to do --
I just tried to keep my own changes to the original md badblocks code
minimal.
Would it be better (for review-ability) if I made these changes in a new
patch on top of this, or should I just squash them into this one?

> 
> > +int badblocks_check(struct badblocks *bb, sector_t s, int sectors,
> > +   sector_t *first_bad, int *bad_sectors)
> [...]
> > +
> > +/*
> > + * Add a range of bad blocks to the table.
> > + * This might extend the table, or might contract it
> > + * if two adjacent ranges can be merged.
> > + * We binary-search to find the 'insertion' point, then
> > + * decide how best to handle it.
> > + */
> 
> And this one, plus you don't document returns.  It looks like this
> function returns 1 on success and zero on failure, which is really
> counter-intuitive for the kernel: zero is usually returned on success
> and negative error on failure.
> 
> > +int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
> > +   int acknowledged)
> [...]
> > +
> > +/*
> > + * Remove a range of bad blocks from the table.
> > + * This may involve extending the table if we spilt a region,
> > + * but it must not fail.  So if the table becomes full, we just
> > + * drop the remove request.
> > + */
> 
> Docbook and document returns.  This time they're the kernel standard
> of
> 0 on success and negative error on failure making the convention for
> badblocks_set even more counterintuitive.
> 
> > +int badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
> > +{
> [...]
> > +#define DO_DEBUG 1
> 
> Why have this at all if it's unconditionally defined and always set.

Neil - any reason or anything you had in mind for this? Or is it just an
artifact and can be removed.

> 
> > +ssize_t badblocks_store(struct badblocks *bb, const char *page,
> > size_t len,
> > +   int unack)
> [...]
> > +int badblocks_init(struct badblocks *bb, int enable)
> > +{
> > +   bb->count = 0;
> > +   if (enable)
> > +   bb->shift = 0;
> > +   else
> > +   bb->shift = -1;
> > +   bb->page = kmalloc(PAGE_SIZE, GFP_KERNEL);
> 
> Why not __get_free_page(GFP_KERNEL)?  The problem with kmalloc of an
> exactly known page sized quantity is that the slab tracker for this
> requires two contiguous pages for each page because of the overhead.

Cool, I didn't know about __get_free_page - I can fix this up too.

> 
> James
> 
> 

Re: [PATCH v2 1/3] badblocks: Add core badblock management code

[James Bottomley]

On Fri, 2015-12-04 at 23:58 +, Verma, Vishal L wrote:
> On Fri, 2015-12-04 at 15:30 -0800, James Bottomley wrote:
> [...]
> > > + * We return
> > > + *  0 if there are no known bad blocks in the range
> > > + *  1 if there are known bad block which are all acknowledged
> > > + * -1 if there are bad blocks which have not yet been acknowledged
> > > in metadata.
> > > + * plus the start/length of the first bad section we overlap.
> > > + */
> > 
> > This comment should be docbook.
> 
> Applicable to all your comments - (and they are all valid), I simply
> copied over all this from md. I'm happy to make the changes to comments,
> and the other two things (see below) if that's the right thing to do --
> I just tried to keep my own changes to the original md badblocks code
> minimal.
> Would it be better (for review-ability) if I made these changes in a new
> patch on top of this, or should I just squash them into this one?

If you were moving it, that might be appropriate.  However, this is
effectively new code because you're not removing the original, so we
should begin at least with a coherent API. (i.e. corrections to the
original patch rather than incremental).

Thanks,

James


> > 
> > > +int badblocks_check(struct badblocks *bb, sector_t s, int sectors,
> > > + sector_t *first_bad, int *bad_sectors)
> > [...]
> > > +
> > > +/*
> > > + * Add a range of bad blocks to the table.
> > > + * This might extend the table, or might contract it
> > > + * if two adjacent ranges can be merged.
> > > + * We binary-search to find the 'insertion' point, then
> > > + * decide how best to handle it.
> > > + */
> > 
> > And this one, plus you don't document returns.  It looks like this
> > function returns 1 on success and zero on failure, which is really
> > counter-intuitive for the kernel: zero is usually returned on success
> > and negative error on failure.
> > 
> > > +int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
> > > + int acknowledged)
> > [...]
> > > +
> > > +/*
> > > + * Remove a range of bad blocks from the table.
> > > + * This may involve extending the table if we spilt a region,
> > > + * but it must not fail.  So if the table becomes full, we just
> > > + * drop the remove request.
> > > + */
> > 
> > Docbook and document returns.  This time they're the kernel standard
> > of
> > 0 on success and negative error on failure making the convention for
> > badblocks_set even more counterintuitive.
> > 
> > > +int badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
> > > +{
> > [...]
> > > +#define DO_DEBUG 1
> > 
> > Why have this at all if it's unconditionally defined and always set.
> 
> Neil - any reason or anything you had in mind for this? Or is it just an
> artifact and can be removed.
> 
> > 
> > > +ssize_t badblocks_store(struct badblocks *bb, const char *page,
> > > size_t len,
> > > + int unack)
> > [...]
> > > +int badblocks_init(struct badblocks *bb, int enable)
> > > +{
> > > + bb->count = 0;
> > > + if (enable)
> > > + bb->shift = 0;
> > > + else
> > > + bb->shift = -1;
> > > + bb->page = kmalloc(PAGE_SIZE, GFP_KERNEL);
> > 
> > Why not __get_free_page(GFP_KERNEL)?  The problem with kmalloc of an
> > exactly known page sized quantity is that the slab tracker for this
> > requires two contiguous pages for each page because of the overhead.
> 
> Cool, I didn't know about __get_free_page - I can fix this up too.
> 
> > 
> > James
> > 
> > NrybXǧv^)޺{.n+{"{ayʇڙ,jfhzwj:+vwjmzZ+ݢj"!



--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] badblocks: Add core badblock management code

[Verma, Vishal L]

On Fri, 2015-12-04 at 16:06 -0800, James Bottomley wrote:
> On Fri, 2015-12-04 at 23:58 +, Verma, Vishal L wrote:
> > On Fri, 2015-12-04 at 15:30 -0800, James Bottomley wrote:
> > [...]
> > > > + * We return
> > > > + *  0 if there are no known bad blocks in the range
> > > > + *  1 if there are known bad block which are all acknowledged
> > > > + * -1 if there are bad blocks which have not yet been
> > > > acknowledged
> > > > in metadata.
> > > > + * plus the start/length of the first bad section we overlap.
> > > > + */
> > > 
> > > This comment should be docbook.
> > 
> > Applicable to all your comments - (and they are all valid), I simply
> > copied over all this from md. I'm happy to make the changes to
> > comments,
> > and the other two things (see below) if that's the right thing to do
> > --
> > I just tried to keep my own changes to the original md badblocks
> > code
> > minimal.
> > Would it be better (for review-ability) if I made these changes in a
> > new
> > patch on top of this, or should I just squash them into this one?
> 
> If you were moving it, that might be appropriate.  However, this is
> effectively new code because you're not removing the original, so we
> should begin at least with a coherent API. (i.e. corrections to the
> original patch rather than incremental).
> 

Patch 3 does remove the original code, but yes, I agree. Will send
another version.

Thanks for the review.

-Vishal

Re: [PATCH v2 2/3] block: Add badblock management for gendisks

[Verma, Vishal L]

On Fri, 2015-12-04 at 15:33 -0800, James Bottomley wrote:
[...]
> >  static void register_disk(struct gendisk *disk)
> >  {
> >     struct device *ddev = disk_to_dev(disk);
> > @@ -609,6 +624,7 @@ void add_disk(struct gendisk *disk)
> >     disk->first_minor = MINOR(devt);
> >  
> >     disk_alloc_events(disk);
> > +   disk_alloc_badblocks(disk);
> 
> Why unconditionally do this?  No-one currently uses the interface, but
> every disk will now pay the price of an additional structure plus a
> page
> for no benefit.  You should probably either export the initializer for
> those who want to use it or, perhaps even better, make it lazily
> allocated the first time anyone tries to set a bad block.
> 
> If you come up with a really good reason for allocating it
> unconditionally, then it should probably be an embedded structure in
> the gendisk.
> 
Agreed - I'll fix for v3.

I'm considering an embedded structure in gendisk (same as md) (why is
this preferred to pointer chasing, especially when this wastes more
space?), and a new exported initializer that is used by anyone who wants
to use gendisk's badblocks.


-VishalN�r��yb�X��ǧv�^�)޺{.n�+{���"�{ay�ʇڙ�,j��f���h���z��w���
���j:+v���w�j�mzZ+�ݢj"��!�i

RE: [PATCH 05/10] aacraid: Set correct msix count for EEH recovery

[Raghava Aditya Renukunta]

Hello Tomas,

> -Original Message-
> From: Tomas Henzl [mailto:the...@redhat.com]
> Sent: Friday, December 4, 2015 6:10 AM
> To: Raghava Aditya Renukunta; jbottom...@parallels.com; linux-
> s...@vger.kernel.org
> Cc: Mahesh Rajashekhara; Murthy Bhat; Santosh Akula; Gana Sridaran;
> aacr...@pmc-sierra.com; Rich Bono
> Subject: Re: [PATCH 05/10] aacraid: Set correct msix count for EEH recovery
> 
> On 1.12.2015 13:39, Raghava Aditya Renukunta wrote:
> > From: Raghava Aditya Renukunta 
> >
> > During EEH recovery number of online CPU's might change thereby
> changing
> > the number of MSIx vectors. Since each fib is allocated to a vector,
> > changes in the number of vectors causes fib to be sent thru invalid
> > vectors.In addition the correct number of MSIx vectors is not
> > updated in the INIT struct sent to the controller, when it is
> > reinitialized.
> >
> > Fixed by reassigning vectors to fibs based on the updated number of MSIx
> > vectors and updating the INIT structure before sending to controller.
> >
> > Signed-off-by: Raghava Aditya Renukunta
> 
> > ---
> >  drivers/scsi/aacraid/linit.c | 25 -
> >  1 file changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
> > index 0147210..f88f1132 100644
> > --- a/drivers/scsi/aacraid/linit.c
> > +++ b/drivers/scsi/aacraid/linit.c
> > @@ -1355,6 +1355,7 @@ static int aac_acquire_resources(struct aac_dev
> *dev)
> > int i, j;
> > int instance = dev->id;
> > const char *name = dev->name;
> > +   int vector = 0;
> > unsigned long status;
> > /*
> >  *  First clear out all interrupts.  Then enable the one's that we
> > @@ -1409,9 +1410,31 @@ static int aac_acquire_resources(struct aac_dev
> *dev)
> > }
> >
> > aac_adapter_enable_int(dev);
> > +   /*max msix may change  after EEH
> > +* Re-assign vectors to fibs
> > +*/
> > +for (i = 0;
> > +   i < (dev->scsi_host_ptr->can_queue +
> AAC_NUM_MGT_FIB);
> > +   i++) {
> > +   if ((dev->max_msix == 1) ||
> > +  (i > ((dev->scsi_host_ptr->can_queue +
> AAC_NUM_MGT_FIB - 1)
> > +   - dev->vector_cap))) {
> > +   dev->fibs[i].vector_no = 0;
> > +   } else {
> > +   dev->fibs[i].vector_no = vector;
> > +   vector++;
> > +   if (vector == dev->max_msix)
> > +   vector = 1;
> > +   }
> > +   }
> 
> The above hunk added code looks identical to the part you have just added
> in 02/10 "aacraid: Fix RRQ overload" could you make this to function ?
> Thanks, tomash

Yes, I will make the necessary changes.
> >
> > -   if (!dev->sync_mode)
> > +   if (!dev->sync_mode) {
> > +   /* After EEH recovery or suspend resume, max_msix count
> > +* may change, therfore updating in init as well.
> > +*/
> > aac_adapter_start(dev);
> > +   dev->init->Sa_MSIXVectors = cpu_to_le32(dev->max_msix);
> > +   }
> > return 0;
> >
> >  error_iounmap:

Regards,
Raghava Aditya
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 04/10] aacraid: Fix memory leak in aac_fib_map_free

[Raghava Aditya Renukunta]

Hello Tomas,


> -Original Message-
> From: Tomas Henzl [mailto:the...@redhat.com]
> Sent: Friday, December 4, 2015 6:35 AM
> To: Raghava Aditya Renukunta; jbottom...@parallels.com; linux-
> s...@vger.kernel.org
> Cc: Mahesh Rajashekhara; Murthy Bhat; Santosh Akula; Gana Sridaran;
> aacr...@pmc-sierra.com; Rich Bono
> Subject: Re: [PATCH 04/10] aacraid: Fix memory leak in aac_fib_map_free
> 
> On 1.12.2015 13:39, Raghava Aditya Renukunta wrote:
> > From: Raghava Aditya Renukunta 
> >
> > aac_fib_map_free() calls pci_free_consistent() without checking that
> > dev->hw_fib_va is not NULL and dev->max_fib_size is not zero.If they
> > are indeed NULL/0, this will result in a hang as pci_free_consistent()
> > will attempt to invalidate cache for the entire 64-bit address space
> > (which would take a very long time).
> >
> > Fixed by adding a check to make sure that dev->hw_fib_va and
> > dev->max_fib_size are not NULL and 0 respectively.
> >
> > Signed-off-by: Raghava Aditya Renukunta
> 
> 
> Reviewed-by: Tomas Henzl 
> 
> Is the can_queue constant during the driver's life, or is it possible
> to manipulate it (aac_change_queue_depth)?
> 
> Tomas

can_queue is only changed in aac_init_adapter. Do you want to save 
 (dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB) in a variable
So that the whole can_queue dereference need not be used?

Regards,
Raghava Aditya

> > ---
> >  drivers/scsi/aacraid/commsup.c | 9 ++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/scsi/aacraid/commsup.c
> b/drivers/scsi/aacraid/commsup.c
> > index b257d3b..9533f47 100644
> > --- a/drivers/scsi/aacraid/commsup.c
> > +++ b/drivers/scsi/aacraid/commsup.c
> > @@ -83,9 +83,12 @@ static int fib_map_alloc(struct aac_dev *dev)
> >
> >  void aac_fib_map_free(struct aac_dev *dev)
> >  {
> > -   pci_free_consistent(dev->pdev,
> > - dev->max_fib_size * (dev->scsi_host_ptr->can_queue +
> AAC_NUM_MGT_FIB),
> > - dev->hw_fib_va, dev->hw_fib_pa);
> > +   if (dev->hw_fib_va && dev->max_fib_size) {
> > +   pci_free_consistent(dev->pdev,
> > +   (dev->max_fib_size *
> > +   (dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB)),
> > +   dev->hw_fib_va, dev->hw_fib_pa);
> > +   }
> > dev->hw_fib_va = NULL;
> > dev->hw_fib_pa = 0;
> >  }

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 76/71] ncr5380: Enable PDMA for DTC chips

[Julian Calaby]

Hi Finn,

On Sat, Dec 5, 2015 at 1:12 PM, Finn Thain  wrote:
>
> On Sat, 5 Dec 2015, Julian Calaby wrote:
>
>> Hi Finn,
>>
>> On Fri, Dec 4, 2015 at 7:38 PM, Finn Thain  
>> wrote:
>> >
>> > On Fri, 4 Dec 2015, Julian Calaby wrote:
>> >
>> >> > -   if (overrides[current_override].board == 
>> >> > BOARD_NCR53C400A) {
>> >> > +   if (overrides[current_override].board == 
>> >> > BOARD_NCR53C400A ||
>> >> > +   overrides[current_override].board == 
>> >> > BOARD_DTC3181E) {
>> >>
>> >> These if statements are starting to get a bit long, would it make
>> >> sense to replace them with a flag or equivalent?
>> >
>> > To what end? Shorter lines? As in,
>>
>> Pretty much, each expression is quite long and they seem to be growing
>> fairly rapidly as you and Ondrej discover similar boards.
>
> Each BOARD_* macro actually refers to a whole category of devices. No new
> boards, devices or categories of devices have been discovered.
>
> Ondrej is enabling and/or fixing PDMA functionality for three existing
> device categories, for which the driver already has a nominally compatible
> PDMA implementation.

I meant discovering boards which are similar.

Either way, I'm not sure it matters that much.

Thanks,

-- 
Julian Calaby

Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 76/71] ncr5380: Enable PDMA for DTC chips

[Julian Calaby]

Hi Finn,

On Fri, Dec 4, 2015 at 7:38 PM, Finn Thain  wrote:
>
> On Fri, 4 Dec 2015, Julian Calaby wrote:
>
>> > -   if (overrides[current_override].board == BOARD_NCR53C400A) 
>> > {
>> > +   if (overrides[current_override].board == BOARD_NCR53C400A 
>> > ||
>> > +   overrides[current_override].board == BOARD_DTC3181E) {
>>
>> These if statements are starting to get a bit long, would it make
>> sense to replace them with a flag or equivalent?
>
> To what end? Shorter lines? As in,

Pretty much, each expression is quite long and they seem to be growing
fairly rapidly as you and Ondrej discover similar boards.

>
> if (board_is_ncr53c400a || board_is_dtc3181e) {
> /* ... */
> }
>
> I suppose that could be an improvement if new flags would entirely replace
> the override.board struct member and the existing switch statement,
>
> switch (overrides[current_override].board) {
> /* ... */
> }
>
> Or maybe you meant testing a new flag something like this,
>
> if (hostdata->ncr53c400_compatible) {
> /* ... */
> }
>
> If your concern is the Don't Repeat Yourself rule, I'm not sure that new
> flag would get tested more than once (?) And it would still have to be
> assigned using an "objectionably" long expression, e.g.
>
> hostdata->ncr53c400_compatible =
> overrides[current_override].board == BOARD_NCR53C400 ||
> overrides[current_override].board == BOARD_NCR53C400A ||
> overrides[current_override].board == BOARD_DTC3181E;
>
> Rather than add new flags, perhaps a 'switch' statement instead of an 'if'
> statement would be shorter (if the size of the expression is the problem).

I think switch statements would be cleaner in this particular
instance. I was thinking something like:

if (somthing->flags & NCR53C400_COMPATIBLE) {
/* ... */
}

but if it's only ever going to be used once, then it's pretty
pointless and switch statements are cleaner.

Thanks,

-- 
Julian Calaby

Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 76/71] ncr5380: Enable PDMA for DTC chips

[Finn Thain]

On Sat, 5 Dec 2015, Julian Calaby wrote:

> Hi Finn,
> 
> On Fri, Dec 4, 2015 at 7:38 PM, Finn Thain  wrote:
> >
> > On Fri, 4 Dec 2015, Julian Calaby wrote:
> >
> >> > -   if (overrides[current_override].board == 
> >> > BOARD_NCR53C400A) {
> >> > +   if (overrides[current_override].board == 
> >> > BOARD_NCR53C400A ||
> >> > +   overrides[current_override].board == BOARD_DTC3181E) 
> >> > {
> >>
> >> These if statements are starting to get a bit long, would it make
> >> sense to replace them with a flag or equivalent?
> >
> > To what end? Shorter lines? As in,
> 
> Pretty much, each expression is quite long and they seem to be growing 
> fairly rapidly as you and Ondrej discover similar boards.

Each BOARD_* macro actually refers to a whole category of devices. No new 
boards, devices or categories of devices have been discovered.

Ondrej is enabling and/or fixing PDMA functionality for three existing 
device categories, for which the driver already has a nominally compatible 
PDMA implementation.

-- 
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: BUG: KASAN: slab-out-of-bounds in ses_enclosure_data_process+0x900/0xe50

[James Bottomley]

On Fri, 2015-12-04 at 18:46 +0100, Andrea Gelmini wrote:
> On Fri, Dec 04, 2015 at 09:09:32AM -0800, James Bottomley wrote:
> > Actually, that would be really helpful, since I only have access to one,
> > very old, enclosure device.  My ssh key is
> 
> Ok.
> Do you need same PC/Kernel of the report, or is it enough if I plug the USB 
> in any PC and give you access?

I think, since we suspect the device, it's enough to plug it into any PC
running a current kernel.

Thanks,

James



signature.asc
Description: This is a digitally signed message part