Re: [PATCH 5/6] cxlflash: Remove adapter file descriptor cache

[Manoj Kumar]

Acked-by: Manoj N. Kumar 


On 8/9/2016 6:40 PM, Matthew R. Ochs wrote:

The adapter file descriptor was previously cached within the kernel
for a given context in order to support performing a close on behalf
of an application. This is no longer needed as applications are now
required to perform a close on the adapter file descriptor.

Inspired-by: Al Viro 
Signed-off-by: Matthew R. Ochs 


--
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 6/6] cxlflash: Update documentation

[Manoj Kumar]

Acked-by: Manoj N. Kumar 


On 8/9/2016 6:40 PM, Matthew R. Ochs wrote:

Update the block library link in the API documentation.

Signed-off-by: Matthew R. Ochs 
---
 Documentation/powerpc/cxlflash.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/powerpc/cxlflash.txt 
b/Documentation/powerpc/cxlflash.txt
index f4c1190..6d9a2ed 100644
--- a/Documentation/powerpc/cxlflash.txt
+++ b/Documentation/powerpc/cxlflash.txt
@@ -121,7 +121,7 @@ Block library API
 below.

 The block library can be found on GitHub:
-http://www.github.com/mikehollinger/ibmcapikv
+http://github.com/open-power/capiflash


 CXL Flash Driver IOCTLs



--
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 4/6] cxlflash: Transition to application close model

[Manoj Kumar]

Acked-by: Manoj N. Kumar 


On 8/9/2016 6:39 PM, Matthew R. Ochs wrote:

Caching the adapter file descriptor and performing a close on behalf
of an application is a poor design. This is due to the fact that once
a file descriptor in installed, it is free to be altered without the
knowledge of the cxlflash driver. This can lead to inconsistencies
between the application and kernel. Furthermore, the nature of the
former design is more exploitable and thus should be abandoned.

To support applications performing a close on the adapter file that
is associated with a context, a new flag is introduced to the user
API to indicate to applications that they are responsible for the
close following the cleanup (detach) of a context. The documentation
is also updated to reflect this change in behavior.

Inspired-by: Al Viro 
Signed-off-by: Matthew R. Ochs 


--
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 3/6] cxlflash: Add kref to context

[Manoj Kumar]

Acked-by: Manoj N. Kumar 

On 8/9/2016 6:39 PM, Matthew R. Ochs wrote:

Currently, context user references are tracked via the list of LUNs
that have attached to the context. While convenient, this is not
intuitive without a deep study of the code and is inconsistent with
the existing reference tracking patterns within the kernel. This design
choice can lead to future bug injection.

To improve code comprehension and better protect against future bugs, add
explicit reference counting to contexts and migrate the context removal
code to the kref release handler.

Inspired-by: Al Viro 
Signed-off-by: Matthew R. Ochs 


--
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 2/6] cxlflash: Cache owning adapter within context

[Manoj Kumar]

Acked-by: Manoj N. Kumar 

On 8/9/2016 6:39 PM, Matthew R. Ochs wrote:

The context removal routine requires access to the owning adapter
structure to reset the context within the AFU as part of the tear
down sequence. In order to support kref adoption, the owning adapter
must be accessible from the release handler. As the kref framework
only provides the kref reference as the sole parameter, another means
is needed to derive the owning adapter.

As a remedy, the owning adapter reference is saved off within the
context during initialization.

Signed-off-by: Matthew R. Ochs 
---
 drivers/scsi/cxlflash/superpipe.c | 1 +
 drivers/scsi/cxlflash/superpipe.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/scsi/cxlflash/superpipe.c 
b/drivers/scsi/cxlflash/superpipe.c
index ab5c893..640c3a2 100644
--- a/drivers/scsi/cxlflash/superpipe.c
+++ b/drivers/scsi/cxlflash/superpipe.c
@@ -804,6 +804,7 @@ static void init_context(struct ctx_info *ctxi, struct 
cxlflash_cfg *cfg,
ctxi->lfd = adap_fd;
ctxi->pid = current->tgid; /* tgid = pid */
ctxi->ctx = ctx;
+   ctxi->cfg = cfg;
ctxi->file = file;
ctxi->initialized = true;
mutex_init(>mutex);
diff --git a/drivers/scsi/cxlflash/superpipe.h 
b/drivers/scsi/cxlflash/superpipe.h
index 5f9a091..61404f2 100644
--- a/drivers/scsi/cxlflash/superpipe.h
+++ b/drivers/scsi/cxlflash/superpipe.h
@@ -107,6 +107,7 @@ struct ctx_info {
bool err_recovery_active;
struct mutex mutex; /* Context protection */
struct cxl_context *ctx;
+   struct cxlflash_cfg *cfg;
struct list_head luns;  /* LUNs attached to this context */
const struct vm_operations_struct *cxl_mmap_vmops;
struct file *file;



--
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 1/6] cxlflash: Avoid mutex when destroying context

[Manoj Kumar]

Acked-by: Manoj N. Kumar 

On 8/9/2016 6:39 PM, Matthew R. Ochs wrote:

Context information structures are protected by a mutex that is held
when accessing/manipulating the context. When the code that manages
these structures was authored, a decision was made to include taking
the mutex as part of the allocation/initialization sequence and also
handle the scenario where the mutex was already held when freeing the
context.

While not a problem outright, this design decision has been deemed as
too flexible and the code should be made more rigid to avoid future bugs.
In addition, further review of the code yields that the existing mutex
manipulations in both of these context management paths are superfluous.

This commit removes the obtaining of the context mutex in the context
initialization routine and assumes the mutex is not held in the context
free path.

Inspired-by: Al Viro 
Signed-off-by: Matthew R. Ochs 


--
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 3/3] cxlflash: Shutdown notify support for CXL Flash cards

[Manoj Kumar]

On 6/15/2016 6:49 PM, Uma Krishnan wrote:

Some CXL Flash cards need notification of device shutdown
in order to flush pending I/Os.

A PCI notification hook for shutdown has been added where
the driver notifies the card and returns. When the device
is removed in the PCI remove path, notification code will
wait for shutdown processing to complete.

Signed-off-by: Uma Krishnan 


Acked-by: Manoj N. Kumar 



--
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 2/3] cxlflash: Add device dependent flags

[Manoj Kumar]

On 6/15/2016 6:49 PM, Uma Krishnan wrote:

Device dependent flags are needed to support functions that are
specific to a particular device.




Acked-by: Manoj N. Kumar 


--
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: MAINTAINERS: use new email address for James Bottomley

[Manoj Kumar]

Reviewed-by: Manoj Kumar <ma...@linux.vnet.ibm.com>

---
Manoj Kumar

On 3/14/2016 2:42 PM, James Bottomley wrote:

The @odin.com one has been bouncing for a while now, so replace with
new Employer email.

Signed-off-by: James Bottomley <j...@linux.vnet.ibm.com>

---

diff --git a/MAINTAINERS b/MAINTAINERS
index a7c4466..59f12c8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9644,7 +9644,7 @@ F:drivers/scsi/sg.c
  F:include/scsi/sg.h

  SCSI SUBSYSTEM
-M: "James E.J. Bottomley" <jbottom...@odin.com>
+M: "James E.J. Bottomley" <j...@linux.vnet.ibm.com>
  T:git git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git
  M:"Martin K. Petersen" <martin.peter...@oracle.com>
  T:git git://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git

--
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



--
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: [PATCHv2] scsi_transport_sas: add 'scsi_target_id' sysfs attribute

[Manoj Kumar]

Hannes:

Thanks for correcting the last argument.

Reviewed-by: Manoj Kumar <ma...@linux.vnet.ibm.com>

---
Manoj Kumar

On 3/14/2016 4:43 AM, Hannes Reinecke wrote:

There is no way to detect the scsi_target_id for any given SAS
remote port, so add a new sysfs attribute 'scsi_target_id'.

Signed-off-by: Hannes Reinecke <h...@suse.com>
---
  drivers/scsi/scsi_sas_internal.h  | 2 +-
  drivers/scsi/scsi_transport_sas.c | 2 ++
  2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_sas_internal.h b/drivers/scsi/scsi_sas_internal.h
index 6266a5d..e659912 100644
--- a/drivers/scsi/scsi_sas_internal.h
+++ b/drivers/scsi/scsi_sas_internal.h
@@ -4,7 +4,7 @@
  #define SAS_HOST_ATTRS0
  #define SAS_PHY_ATTRS 17
  #define SAS_PORT_ATTRS1
-#define SAS_RPORT_ATTRS7
+#define SAS_RPORT_ATTRS8
  #define SAS_END_DEV_ATTRS 5
  #define SAS_EXPANDER_ATTRS7

diff --git a/drivers/scsi/scsi_transport_sas.c 
b/drivers/scsi/scsi_transport_sas.c
index 80520e2..b6f958193 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -1286,6 +1286,7 @@ sas_rphy_protocol_attr(identify.target_port_protocols, 
target_port_protocols);
  sas_rphy_simple_attr(identify.sas_address, sas_address, "0x%016llx\n",
unsigned long long);
  sas_rphy_simple_attr(identify.phy_identifier, phy_identifier, "%d\n", u8);
+sas_rphy_simple_attr(scsi_target_id, scsi_target_id, "%d\n", u32);

  /* only need 8 bytes of data plus header (4 or 8) */
  #define BUF_SIZE 64
@@ -1886,6 +1887,7 @@ sas_attach_transport(struct sas_function_template *ft)
SETUP_RPORT_ATTRIBUTE(rphy_device_type);
SETUP_RPORT_ATTRIBUTE(rphy_sas_address);
SETUP_RPORT_ATTRIBUTE(rphy_phy_identifier);
+   SETUP_RPORT_ATTRIBUTE(rphy_scsi_target_id);
SETUP_OPTIONAL_RPORT_ATTRIBUTE(rphy_enclosure_identifier,
   get_enclosure_identifier);
SETUP_OPTIONAL_RPORT_ATTRIBUTE(rphy_bay_identifier,



--
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] scsi_transport_sas: add 'scsi_target_id' sysfs attribute

[Manoj Kumar]

On 3/11/2016 9:33 AM, Hannes Reinecke wrote:

There is no way to detect the scsi_target_id for any given SAS
remote port, so add a new sysfs attribute 'scsi_target_id'.

Signed-off-by: Hannes Reinecke <h...@suse.com>
---
  drivers/scsi/scsi_sas_internal.h  | 2 +-
  drivers/scsi/scsi_transport_sas.c | 2 ++
  2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_sas_internal.h b/drivers/scsi/scsi_sas_internal.h
index 6266a5d..e659912 100644
--- a/drivers/scsi/scsi_sas_internal.h
+++ b/drivers/scsi/scsi_sas_internal.h
@@ -4,7 +4,7 @@
  #define SAS_HOST_ATTRS0
  #define SAS_PHY_ATTRS 17
  #define SAS_PORT_ATTRS1
-#define SAS_RPORT_ATTRS7
+#define SAS_RPORT_ATTRS8
  #define SAS_END_DEV_ATTRS 5
  #define SAS_EXPANDER_ATTRS7

diff --git a/drivers/scsi/scsi_transport_sas.c 
b/drivers/scsi/scsi_transport_sas.c
index 80520e2..deb3fde 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -1286,6 +1286,7 @@ sas_rphy_protocol_attr(identify.target_port_protocols, 
target_port_protocols);
  sas_rphy_simple_attr(identify.sas_address, sas_address, "0x%016llx\n",
unsigned long long);
  sas_rphy_simple_attr(identify.phy_identifier, phy_identifier, "%d\n", u8);
+sas_rphy_simple_attr(scsi_target_id, scsi_target_id, "%d\n", u8);


Hannes:

Shouldn't the last argument be u32?

- Manoj Kumar


--
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] scsi_dh_alua: uninitialized variable in alua_check_vpd()

[Manoj Kumar]

On 3/11/2016 5:19 AM, Dan Carpenter wrote:

The pg_updated variable is support to be set to false at the start but
it is uninitialized.

Fixes: cb0a168cb6b8 ('scsi_dh_alua: update 'access_state' field')
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c 
b/drivers/scsi/device_handler/scsi_dh_alua.c
index 5bcdf8d..a404a41 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -332,7 +332,7 @@ static int alua_check_vpd(struct scsi_device *sdev, struct 
alua_dh_data *h,
  {
int rel_port = -1, group_id;
struct alua_port_group *pg, *old_pg = NULL;
-   bool pg_updated;
+   bool pg_updated = false;
unsigned long flags;


Reviewed-by: Manoj Kumar <ma...@linux.vnet.ibm.com>

---
Manoj Kumar


--
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 v6 19/20] cxlflash: Use new cxl_pci_read_adapter_vpd() API

[Manoj Kumar]

Fred:

Thanks for submitting this. Including linux-scsi.

Acked-by: Manoj N. Kumar 

- Manoj


On 3/4/2016 5:26 AM, Frederic Barrat wrote:

To read the adapter VPD, drivers can't rely on pci config APIs, as it
wouldn't work on powerVM. cxl introduced a new kernel API especially
for this, so start using it.

Co-authored-by: Christophe Lombard 
Signed-off-by: Frederic Barrat 
Signed-off-by: Christophe Lombard 
---
  drivers/scsi/cxlflash/common.h |  1 -
  drivers/scsi/cxlflash/main.c   | 18 ++
  2 files changed, 2 insertions(+), 17 deletions(-)

diff --git a/drivers/scsi/cxlflash/common.h b/drivers/scsi/cxlflash/common.h
index 5ada926..580f370 100644
--- a/drivers/scsi/cxlflash/common.h
+++ b/drivers/scsi/cxlflash/common.h
@@ -106,7 +106,6 @@ struct cxlflash_cfg {
atomic_t scan_host_needed;

struct cxl_afu *cxl_afu;
-   struct pci_dev *parent_dev;

atomic_t recovery_threads;
struct mutex ctx_recovery_mutex;
diff --git a/drivers/scsi/cxlflash/main.c b/drivers/scsi/cxlflash/main.c
index f6d90ce..e04aae7 100644
--- a/drivers/scsi/cxlflash/main.c
+++ b/drivers/scsi/cxlflash/main.c
@@ -1407,7 +1407,7 @@ static int start_context(struct cxlflash_cfg *cfg)
   */
  static int read_vpd(struct cxlflash_cfg *cfg, u64 wwpn[])
  {
-   struct pci_dev *dev = cfg->parent_dev;
+   struct pci_dev *dev = cfg->dev;
int rc = 0;
int ro_start, ro_size, i, j, k;
ssize_t vpd_size;
@@ -1416,7 +1416,7 @@ static int read_vpd(struct cxlflash_cfg *cfg, u64 wwpn[])
char *wwpn_vpd_tags[NUM_FC_PORTS] = { "V5", "V6" };

/* Get the VPD data from the device */
-   vpd_size = pci_read_vpd(dev, 0, sizeof(vpd_data), vpd_data);
+   vpd_size = cxl_read_adapter_vpd(dev, vpd_data, sizeof(vpd_data));
if (unlikely(vpd_size <= 0)) {
dev_err(>dev, "%s: Unable to read VPD (size = %ld)\n",
   __func__, vpd_size);
@@ -2392,7 +2392,6 @@ static int cxlflash_probe(struct pci_dev *pdev,
  {
struct Scsi_Host *host;
struct cxlflash_cfg *cfg = NULL;
-   struct device *phys_dev;
struct dev_dependent_vals *ddv;
int rc = 0;

@@ -2458,19 +2457,6 @@ static int cxlflash_probe(struct pci_dev *pdev,

pci_set_drvdata(pdev, cfg);

-   /*
-* Use the special service provided to look up the physical
-* PCI device, since we are called on the probe of the virtual
-* PCI host bus (vphb)
-*/
-   phys_dev = cxl_get_phys_dev(pdev);
-   if (!dev_is_pci(phys_dev)) {
-   dev_err(>dev, "%s: not a pci dev\n", __func__);
-   rc = -ENODEV;
-   goto out_remove;
-   }
-   cfg->parent_dev = to_pci_dev(phys_dev);
-
cfg->cxl_afu = cxl_pci_to_afu(pdev);

rc = init_pci(cfg);



--
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/7] ibmvscsi: Correct values for several viosrp_crq_format enums

[Manoj Kumar]

Reviewed-by: Manoj Kumar <ma...@linux.vnet.ibm.com>

---
Manoj Kumar

On 2/10/2016 7:32 PM, Tyrel Datwyler wrote:

The enum values for VIOSRP_LINUX_FORMAT and VIOSRP_INLINE_FORMAT are
off by one. They are currently defined as 0x06 and 0x07 respetively.
These values are defined in PAPR correctly as 0x05 and 0x06. This
inconsistency has gone unnoticed as neither enum is currently used.
The possible future support of PING messages between the VIOS and
client adapter relies on VIOSRP_INLINE_FORMAT crq messages.
Corrected these enum values to match PAPR definitions.

Signed-off-by: Tyrel Datwyler <tyr...@linux.vnet.ibm.com>
Reviewed-by: Johannes Thumshirn <jthumsh...@suse.de>
---
  drivers/scsi/ibmvscsi/viosrp.h | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ibmvscsi/viosrp.h b/drivers/scsi/ibmvscsi/viosrp.h
index 1162430..d1044e9 100644
--- a/drivers/scsi/ibmvscsi/viosrp.h
+++ b/drivers/scsi/ibmvscsi/viosrp.h
@@ -56,8 +56,8 @@ enum viosrp_crq_formats {
VIOSRP_MAD_FORMAT = 0x02,
VIOSRP_OS400_FORMAT = 0x03,
VIOSRP_AIX_FORMAT = 0x04,
-   VIOSRP_LINUX_FORMAT = 0x06,
-   VIOSRP_INLINE_FORMAT = 0x07
+   VIOSRP_LINUX_FORMAT = 0x05,
+   VIOSRP_INLINE_FORMAT = 0x06
  };

  enum viosrp_crq_status {



--
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 2/7] ibmvscsi: Add and use enums for valid CRQ header values

[Manoj Kumar]

Tyrel:

Thanks for incorporating the suggestions.

Reviewed-by: Manoj Kumar <ma...@linux.vnet.ibm.com>

---
Manoj Kumar

On 2/10/2016 7:32 PM, Tyrel Datwyler wrote:

The PAPR defines four valid header values for the first byte of a
CRQ message. Namely, an unused/empty message (0x00), a valid
command/response entry (0x80), a valid initialization entry (0xC0),
and a valid transport event (0xFF). Further, initialization responses
have two formats namely initialize (0x01) and initialize complete
(0x02). Define these values as enums and use them in the code in
place of their magic number equivalents.

Signed-off-by: Tyrel Datwyler <tyr...@linux.vnet.ibm.com>
---
  drivers/scsi/ibmvscsi/ibmvscsi.c | 18 +-
  drivers/scsi/ibmvscsi/viosrp.h   | 12 
  2 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index adfef9d..c888ea1 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -182,7 +182,7 @@ static struct viosrp_crq *crq_queue_next_crq(struct 
crq_queue *queue)

spin_lock_irqsave(>lock, flags);
crq = >msgs[queue->cur];
-   if (crq->valid & 0x80) {
+   if (crq->valid != VIOSRP_CRQ_FREE) {
if (++queue->cur == queue->size)
queue->cur = 0;

@@ -231,7 +231,7 @@ static void ibmvscsi_task(void *data)
/* Pull all the valid messages off the CRQ */
while ((crq = crq_queue_next_crq(>queue)) != NULL) {
ibmvscsi_handle_crq(crq, hostdata);
-   crq->valid = 0x00;
+   crq->valid = VIOSRP_CRQ_FREE;
}

vio_enable_interrupts(vdev);
@@ -239,7 +239,7 @@ static void ibmvscsi_task(void *data)
if (crq != NULL) {
vio_disable_interrupts(vdev);
ibmvscsi_handle_crq(crq, hostdata);
-   crq->valid = 0x00;
+   crq->valid = VIOSRP_CRQ_FREE;
} else {
done = 1;
}
@@ -474,7 +474,7 @@ static int initialize_event_pool(struct event_pool *pool,
struct srp_event_struct *evt = >events[i];
memset(>crq, 0x00, sizeof(evt->crq));
atomic_set(>free, 1);
-   evt->crq.valid = 0x80;
+   evt->crq.valid = VIOSRP_CRQ_CMD_RSP;
evt->crq.IU_length = cpu_to_be16(sizeof(*evt->xfer_iu));
evt->crq.IU_data_ptr = cpu_to_be64(pool->iu_token +
sizeof(*evt->xfer_iu) * i);
@@ -1767,9 +1767,9 @@ static void ibmvscsi_handle_crq(struct viosrp_crq *crq,
struct srp_event_struct *evt_struct =
(__force struct srp_event_struct *)crq->IU_data_ptr;
switch (crq->valid) {
-   case 0xC0:  /* initialization */
+   case VIOSRP_CRQ_INIT_RSP:   /* initialization */
switch (crq->format) {
-   case 0x01:  /* Initialization message */
+   case VIOSRP_CRQ_INIT:   /* Initialization message */
dev_info(hostdata->dev, "partner initialized\n");
/* Send back a response */
rc = ibmvscsi_send_crq(hostdata, 0xC002LL, 
0);
@@ -1781,7 +1781,7 @@ static void ibmvscsi_handle_crq(struct viosrp_crq *crq,
}

break;
-   case 0x02:  /* Initialization response */
+   case VIOSRP_CRQ_INIT_COMPLETE:  /* Initialization response */
dev_info(hostdata->dev, "partner initialization 
complete\n");

/* Now login */
@@ -1791,7 +1791,7 @@ static void ibmvscsi_handle_crq(struct viosrp_crq *crq,
dev_err(hostdata->dev, "unknown crq message type: %d\n", 
crq->format);
}
return;
-   case 0xFF:  /* Hypervisor telling us the connection is closed */
+   case VIOSRP_CRQ_XPORT_EVENT:/* Hypervisor telling us the connection 
is closed */
scsi_block_requests(hostdata->host);
atomic_set(>request_limit, 0);
if (crq->format == 0x06) {
@@ -1807,7 +1807,7 @@ static void ibmvscsi_handle_crq(struct viosrp_crq *crq,
ibmvscsi_reset_host(hostdata);
}
return;
-   case 0x80:  /* real payload */
+   case VIOSRP_CRQ_CMD_RSP:/* real payload */
break;
default:
dev_err(hostdata->dev, "got an invalid message type 0x%02x\n",
diff --git a/drivers/scsi/ibmvscsi/viosrp.h b/drivers/scsi/ibmvscsi/viosrp.h
index d1044

Re: [PATCH v2 3/7] ibmvscsi: Replace magic values in set_adpater_info() with defines

[Manoj Kumar]

Reviewed-by: Manoj Kumar <ma...@linux.vnet.ibm.com>

---
Manoj Kumar

On 2/10/2016 7:32 PM, Tyrel Datwyler wrote:

Add defines for mad version and mad os_type, and replace the magic
numbers in set_adapter_info() accordingly.

Signed-off-by: Tyrel Datwyler <tyr...@linux.vnet.ibm.com>
---
  drivers/scsi/ibmvscsi/ibmvscsi.c | 8 
  drivers/scsi/ibmvscsi/viosrp.h   | 3 +++
  2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index c888ea1..4b09a9b 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -283,8 +283,8 @@ static void set_adapter_info(struct ibmvscsi_host_data 
*hostdata)
hostdata->madapter_info.partition_number =
cpu_to_be32(partition_number);

-   hostdata->madapter_info.mad_version = cpu_to_be32(1);
-   hostdata->madapter_info.os_type = cpu_to_be32(2);
+   hostdata->madapter_info.mad_version = cpu_to_be32(SRP_MAD_VERSION_1);
+   hostdata->madapter_info.os_type = cpu_to_be32(SRP_MAD_OS_LINUX);
  }

  /**
@@ -1398,7 +1398,7 @@ static void adapter_info_rsp(struct srp_event_struct 
*evt_struct)
hostdata->host->max_sectors =
be32_to_cpu(hostdata->madapter_info.port_max_txu[0]) 
>> 9;

-   if (be32_to_cpu(hostdata->madapter_info.os_type) == 3 &&
+   if (be32_to_cpu(hostdata->madapter_info.os_type) == SRP_MAD_OS_AIX 
&&
strcmp(hostdata->madapter_info.srp_version, "1.6a") <= 0) {
dev_err(hostdata->dev, "host (Ver. %s) doesn't support large 
transfers\n",
hostdata->madapter_info.srp_version);
@@ -1407,7 +1407,7 @@ static void adapter_info_rsp(struct srp_event_struct 
*evt_struct)
hostdata->host->sg_tablesize = MAX_INDIRECT_BUFS;
}

-   if (be32_to_cpu(hostdata->madapter_info.os_type) == 3) {
+   if (be32_to_cpu(hostdata->madapter_info.os_type) == 
SRP_MAD_OS_AIX) {
enable_fast_fail(hostdata);
return;
}
diff --git a/drivers/scsi/ibmvscsi/viosrp.h b/drivers/scsi/ibmvscsi/viosrp.h
index 3d20851..d0f689b 100644
--- a/drivers/scsi/ibmvscsi/viosrp.h
+++ b/drivers/scsi/ibmvscsi/viosrp.h
@@ -221,7 +221,10 @@ struct mad_adapter_info_data {
char srp_version[8];
char partition_name[96];
__be32 partition_number;
+#define SRP_MAD_VERSION_1 1
__be32 mad_version;
+#define SRP_MAD_OS_LINUX 2
+#define SRP_MAD_OS_AIX 3
__be32 os_type;
__be32 port_max_txu[8]; /* per-port maximum transfer */
  };



--
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 2/6] ibmvscsi: Add and use enums for valid CRQ header values

[Manoj Kumar]

Yeah, I can see how that is confusing. Since, all three possible valid
crq message types have the first bit set I think this was originally a
cute hack to grab anything that was likely valid. Then in
ibmvscsi_handle_crq() we explicitly match the full header value in a
switch statement logging anything that turned out actually invalid.



If 'valid' will only have one of these four enums defined, would
this be better written as:

if (crq->valid != VIOSRP_CRQ_FREE)


This definitely would make the logic easier to read and follow. Also,
this would make sure any crq with an invalid header that doesn't have
its first bit set will also be logged by the ibmvscsi_handle_crq()
switch statement default block and not silently ignored.

-Tyrel


Sounds good, Tyrel. Does this mean I should expect a v2 of this patch
series?

- Manoj N. Kumar

--
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 2/6] ibmvscsi: Add and use enums for valid CRQ header values

[Manoj Kumar]

On 2/3/2016 5:28 PM, Tyrel Datwyler wrote:

The PAPR defines four valid header values for the first byte of a
CRQ message. Namely, an unused/empty message (0x00), a valid
command/response entry (0x80), a valid initialization entry (0xC0),
and a transport event (0xFF). Define these values as enums and use
them in the code in place of their magic number equivalents.

Signed-off-by: Tyrel Datwyler 
---
  drivers/scsi/ibmvscsi/ibmvscsi.c | 14 +++---
  drivers/scsi/ibmvscsi/viosrp.h   |  7 +++
  2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index adfef9d..176260d 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -182,7 +182,7 @@ static struct viosrp_crq *crq_queue_next_crq(struct 
crq_queue *queue)

spin_lock_irqsave(>lock, flags);
crq = >msgs[queue->cur];
-   if (crq->valid & 0x80) {
+   if (crq->valid & VIOSRP_CRQ_VALID) {


After the switch to enums, bitwise operators are a bit misleading.
Especially in this case since multiple values would satisfy this
condition: VIOSRP_CRQ_VALID, VIOSRP_CRQ_INIT and
VIOSRP_CRQ_TRANSPORT.

If 'valid' will only have one of these four enums defined, would
this be better written as:

if (crq->valid != VIOSRP_CRQ_FREE)


if (++queue->cur == queue->size)
queue->cur = 0;

@@ -231,7 +231,7 @@ static void ibmvscsi_task(void *data)
/* Pull all the valid messages off the CRQ */
while ((crq = crq_queue_next_crq(>queue)) != NULL) {
ibmvscsi_handle_crq(crq, hostdata);
-   crq->valid = 0x00;
+   crq->valid = VIOSRP_CRQ_FREE;
}

vio_enable_interrupts(vdev);
@@ -239,7 +239,7 @@ static void ibmvscsi_task(void *data)
if (crq != NULL) {
vio_disable_interrupts(vdev);
ibmvscsi_handle_crq(crq, hostdata);
-   crq->valid = 0x00;
+   crq->valid = VIOSRP_CRQ_FREE;
} else {
done = 1;
}
@@ -474,7 +474,7 @@ static int initialize_event_pool(struct event_pool *pool,
struct srp_event_struct *evt = >events[i];
memset(>crq, 0x00, sizeof(evt->crq));
atomic_set(>free, 1);
-   evt->crq.valid = 0x80;
+   evt->crq.valid = VIOSRP_CRQ_VALID;
evt->crq.IU_length = cpu_to_be16(sizeof(*evt->xfer_iu));
evt->crq.IU_data_ptr = cpu_to_be64(pool->iu_token +
sizeof(*evt->xfer_iu) * i);
@@ -1767,7 +1767,7 @@ static void ibmvscsi_handle_crq(struct viosrp_crq *crq,
struct srp_event_struct *evt_struct =
(__force struct srp_event_struct *)crq->IU_data_ptr;
switch (crq->valid) {
-   case 0xC0:  /* initialization */
+   case VIOSRP_CRQ_INIT:   /* initialization */
switch (crq->format) {
case 0x01:  /* Initialization message */
dev_info(hostdata->dev, "partner initialized\n");
@@ -1791,7 +1791,7 @@ static void ibmvscsi_handle_crq(struct viosrp_crq *crq,
dev_err(hostdata->dev, "unknown crq message type: %d\n", 
crq->format);
}
return;
-   case 0xFF:  /* Hypervisor telling us the connection is closed */
+   case VIOSRP_CRQ_TRANSPORT:  /* Hypervisor telling us the connection 
is closed */
scsi_block_requests(hostdata->host);
atomic_set(>request_limit, 0);
if (crq->format == 0x06) {
@@ -1807,7 +1807,7 @@ static void ibmvscsi_handle_crq(struct viosrp_crq *crq,
ibmvscsi_reset_host(hostdata);
}
return;
-   case 0x80:  /* real payload */
+   case VIOSRP_CRQ_VALID:  /* real payload */
break;
default:
dev_err(hostdata->dev, "got an invalid message type 0x%02x\n",
diff --git a/drivers/scsi/ibmvscsi/viosrp.h b/drivers/scsi/ibmvscsi/viosrp.h
index d1044e9..17f2de0 100644
--- a/drivers/scsi/ibmvscsi/viosrp.h
+++ b/drivers/scsi/ibmvscsi/viosrp.h
@@ -51,6 +51,13 @@ union srp_iu {
u8 reserved[SRP_MAX_IU_LEN];
  };

+enum viosrp_crq_headers {
+   VIOSRP_CRQ_FREE = 0x00,
+   VIOSRP_CRQ_VALID = 0x80,
+   VIOSRP_CRQ_INIT = 0xC0,
+   VIOSRP_CRQ_TRANSPORT = 0xFF
+};
+
  enum viosrp_crq_formats {
VIOSRP_SRP_FORMAT = 0x01,
VIOSRP_MAD_FORMAT = 0x02,



--
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 3/6] cxlflash: Removed driver date print

[Manoj Kumar]

On 12/14/2015 3:06 PM, Uma Krishnan wrote:

Having a date for the driver requires it to be updated quite
often. Removing the date which is not necessary. Also made
use of the existing symbol to print the driver name.

Signed-off-by: Uma Krishnan 
---


Acked-by: Manoj N. Kumar 

--
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 6/6] cxlflash: Enable device id for future IBM CXL adapter

[Manoj Kumar]

On 12/13/2015 9:47 PM, Andrew Donnellan wrote:

On 11/12/15 09:54, Uma Krishnan wrote:

From: Manoj Kumar <ma...@linux.vnet.ibm.com>

This drop enables a future card with a device id
of 0x0600 to be recognized by the cxlflash driver.
No card specific programming has been added. These
card specific changes will be staged in later.

Signed-off-by: Manoj N. Kumar <ma...@linux.vnet.ibm.com>


Without the card-specific code, how does the driver behave if the new
card is plugged in?


Andrew:

As per the design, the Accelerator Function Unit (AFU)
for this new IBM CXL Flash Adapater retains the same host
interface as the previous generation. For the early prototypes
of the new card, the driver with this change behaves exactly as
the driver prior to this behaved with the earlier generation
card. i.e. No card specific changes are required. However, I
left the staging comment in there, in case later versions of the
card deviate from the prototype.

- Manoj

--
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 2/6] cxlflash: Fix to avoid virtual LUN failover failure

[Manoj Kumar]

On 12/10/2015 4:53 PM, Uma Krishnan wrote:

From: "Matthew R. Ochs" <mro...@linux.vnet.ibm.com>
To remedy this scenario, provide feedback back to the application on
virtual LUN creation as to which ports the LUN may be accessed. LUN's
spanning both ports are candidates for a retry in a presence of an I/O
failure.

Signed-off-by: Matthew R. Ochs <mro...@linux.vnet.ibm.com>


Acked-by: Manoj Kumar <ma...@linux.vnet.ibm.com>



--
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 RESEND] scsi: Set sg_tablesize to 1, for LLDDs that set SG_NONE

[Manoj Kumar]

Hannes:

A very valid point. Thanks for your suggestion.
Will look at resolving the issue in blk-mq.

Regards,
- Manoj

On 11/11/2015 1:28 AM, Hannes Reinecke wrote:

Shouldn't we rather fixup blk-mq to properly support SG_NONE?
Silently converting SG_NONE (=0) to 1 has a fair chance of breaking
non-mq enabled setups, which happily work with SG_NONE currently.

Cheers,

Hannes



--
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 RESEND] scsi: Set sg_tablesize to 1, for LLDDs that set SG_NONE

[Manoj Kumar]

Oops while testing blk_mq over the new cxlflash driver.

[ 2960.817172] Oops: Kernel access of bad area, sig: 11 [#5]
[ 2960.817309] NIP  __blk_mq_run_hw_queue+0x278/0x4c0
[ 2960.817313] LR __blk_mq_run_hw_queue+0x2bc/0x4c0
[ 2960.817314] Call Trace:
[ 2960.817320] __blk_mq_run_hw_queue+0x2bc/0x4c0 (unreliable)
[ 2960.817324] blk_mq_run_hw_queue+0xd8/0x100
[ 2960.817329] blk_mq_insert_requests+0x14c/0x1f0
[ 2960.817333] blk_mq_flush_plug_list+0x150/0x190
[ 2960.817338] blk_flush_plug_list+0x11c/0x2b0
[ 2960.817344] blk_finish_plug+0x58/0x80
[ 2960.817348] __do_page_cache_readahead+0x1c0/0x2e0
[ 2960.817352] force_page_cache_readahead+0x68/0xd0
[ 2960.817356] generic_file_read_iter+0x43c/0x6a0
[ 2960.817359] blkdev_read_iter+0x68/0xa0
[ 2960.817361] __vfs_read+0x11c/0x180
[ 2960.817364] vfs_read+0xa4/0x1c0
[ 2960.817366] SyS_read+0x6c/0x110
[ 2960.817369] system_call+0x38/0xb4

The root cause of the problem was this low level device driver(LLDD),
in this case cxlflash, does not support scatter-gather and hence had
set it's sg_tablesize to SG_NONE (value of 0). In reality the tablesize
is of length 1. This value of SG_NONE does not cause any problems with
the standard block driver stack but causes issues for blk_mq, as shown
above. Since quite a few of the legacy LLDDs are setting sg_tablesize
to SG_NONE, it was preferable to override the LLDD provided value in
scsi_host_alloc().

Signed-off-by: Manoj N. Kumar 
Signed-off-by: Youngjae Lee 
Reviewed-by: Matthew R. Ochs 
---
 drivers/scsi/hosts.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 8bb173e..bd13c9d 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -413,7 +413,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template 
*sht, int privsize)
shost->hostt = sht;
shost->this_id = sht->this_id;
shost->can_queue = sht->can_queue;
-   shost->sg_tablesize = sht->sg_tablesize;
+   shost->sg_tablesize = (sht->sg_tablesize ? sht->sg_tablesize : 1);
shost->sg_prot_tablesize = sht->sg_prot_tablesize;
shost->cmd_per_lun = sht->cmd_per_lun;
shost->unchecked_isa_dma = sht->unchecked_isa_dma;
-- 
2.1.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 v2 03/27] hpsa: remove unused hpsa_tag_discard_error_bits

[Manoj Kumar]

Don:

Thanks for not redefining HPSA_PERF_ERROR_BITS.

Reviewed-by: Manoj Kumar <ma...@linux.vnet.ibm.com>

---
Manoj Kumar

On 11/4/2015 3:50 PM, Don Brace wrote:

This function is no longer used.

Reviewed-by: Tomas Henzl <the...@redhat.com>
Reviewed-by: Hannes Reinecke <h...@suse.de>
Signed-off-by: Don Brace <don.br...@pmcs.com>
---
  drivers/scsi/hpsa.c |   11 +--
  1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 4085401..5a996273 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -230,6 +230,7 @@ static int fill_cmd(struct CommandList *c, u8 cmd, struct 
ctlr_info *h,
int cmd_type);
  static void hpsa_free_cmd_pool(struct ctlr_info *h);
  #define VPD_PAGE (1 << 8)
+#define HPSA_SIMPLE_ERROR_BITS 0x03

  static int hpsa_scsi_queue_command(struct Scsi_Host *h, struct scsi_cmnd 
*cmd);
  static void hpsa_scan_start(struct Scsi_Host *);
@@ -6436,16 +6437,6 @@ static inline void finish_cmd(struct CommandList *c)
complete(c->waiting);
  }

-
-static inline u32 hpsa_tag_discard_error_bits(struct ctlr_info *h, u32 tag)
-{
-#define HPSA_PERF_ERROR_BITS ((1 << DIRECT_LOOKUP_SHIFT) - 1)
-#define HPSA_SIMPLE_ERROR_BITS 0x03
-   if (unlikely(!(h->transMethod & CFGTBL_Trans_Performant)))
-   return tag & ~HPSA_SIMPLE_ERROR_BITS;
-   return tag & ~HPSA_PERF_ERROR_BITS;
-}
-
  /* process completion of an indexed ("direct lookup") command */
  static inline void process_indexed_cmd(struct ctlr_info *h,
u32 raw_tag)

--
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



--
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 2/5] ipr: Don't set NO_ULEN_CHK bit when resource is a vset.

[Manoj Kumar]

Gabriel:

On applying this patch, I noticed that this statement
seems to be unnecessary:

else
ioarcb->cmd_pkt.flags_lo |= 
IPR_FLAGS_LO_UNTAGGED_TASK;


As the value is 0x00:
#define IPR_FLAGS_LO_UNTAGGED_TASK  0x00

You can resolve this in a future update.

Reviewed-by: Manoj Kumar <ma...@linux.vnet.ibm.com>

---
Manoj Kumar

On 11/3/2015 12:26 PM, Gabriel Krisman Bertazi wrote:

According to the IPR specification, Inhibit Underlength Checking bit
must be disabled when issuing commands to vsets.  Enabling it in this
case might cause SCSI commands to fail with an Illegal Request, so make
sure we keep this bit cleared when resource is a vset.

Changes since v1:
- Put gsci exclusive stuff in a separate block.

Signed-off-by: Gabriel Krisman Bertazi <kris...@linux.vnet.ibm.com>
---
  drivers/scsi/ipr.c | 10 +++---
  1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index 238efab..6849b7f 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -6363,15 +6363,19 @@ static int ipr_queuecommand(struct Scsi_Host *shost,
ipr_cmd->scsi_cmd = scsi_cmd;
ipr_cmd->done = ipr_scsi_eh_done;

-   if (ipr_is_gscsi(res) || ipr_is_vset_device(res)) {
+   if (ipr_is_gscsi(res)) {
if (scsi_cmd->underflow == 0)
ioarcb->cmd_pkt.flags_hi |= IPR_FLAGS_HI_NO_ULEN_CHK;

-   ioarcb->cmd_pkt.flags_hi |= IPR_FLAGS_HI_NO_LINK_DESC;
-   if (ipr_is_gscsi(res) && res->reset_occurred) {
+   if (res->reset_occurred) {
res->reset_occurred = 0;
ioarcb->cmd_pkt.flags_lo |= 
IPR_FLAGS_LO_DELAY_AFTER_RST;
}
+   }
+
+   if (ipr_is_gscsi(res) || ipr_is_vset_device(res)) {
+   ioarcb->cmd_pkt.flags_hi |= IPR_FLAGS_HI_NO_LINK_DESC;
+
ioarcb->cmd_pkt.flags_lo |= IPR_FLAGS_LO_ALIGNED_BFR;
if (scsi_cmd->flags & SCMD_TAGGED)
ioarcb->cmd_pkt.flags_lo |= IPR_FLAGS_LO_SIMPLE_TASK;



--
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 v4.3-rc7] be2iscsi : Fix bogus WARN_ON length check

[Manoj Kumar]

Tim:

Reviewed-by: Manoj Kumar <ma...@linux.vnet.ibm.com>

---
Manoj Kumar

On 10/30/2015 1:22 PM, tim.gard...@canonical.com wrote:

From: Tim Gardner <tim.gard...@canonical.com>

drivers/scsi/be2iscsi/be_main.c: In function 'be_sgl_create_contiguous':
drivers/scsi/be2iscsi/be_main.c:3187:18: warning: logical not is only applied 
to the left hand side of comparison [-Wlogical-not-parentheses]
   WARN_ON(!length > 0);

gcc version 5.2.1

Cc: Jayamohan Kallickal <jayamohan.kallic...@avagotech.com>
Cc: Minh Tran <minh.t...@avagotech.com>
Cc: John Soni Jose <sony.joh...@avagotech.com>
Cc: "James E.J. Bottomley" <jbottom...@odin.com>
Signed-off-by: Tim Gardner <tim.gard...@canonical.com>
---
  drivers/scsi/be2iscsi/be_main.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
index 7a6dbfb..5cdcd29 100644
--- a/drivers/scsi/be2iscsi/be_main.c
+++ b/drivers/scsi/be2iscsi/be_main.c
@@ -3184,7 +3184,7 @@ be_sgl_create_contiguous(void *virtual_address,
  {
WARN_ON(!virtual_address);
WARN_ON(!physical_address);
-   WARN_ON(!length > 0);
+   WARN_ON(!length);
WARN_ON(!sgl);

sgl->va = virtual_address;



--
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 2/5] ipr: Clear NO_ULEN_CHK bit when resource is a vset.

[Manoj Kumar]

On 10/30/2015 11:49 AM, Gabriel Krisman Bertazi wrote:


if (ipr_is_gscsi(res) || ipr_is_vset_device(res)) {
-   if (scsi_cmd->underflow == 0)
+   if (scsi_cmd->underflow == 0 && !ipr_is_vset_device(res))


This section is getting quite convoluted. If there isn't really that
much common between ipr_is_gscsi(res) and ipr_is_vset_device(res)
anymore, it would read much better as distinct segments:

if (ipr_is_gscsi(res))
...
if (ipr_is_vset_devices(res))
...

This will avoid having multiple calls to ipr_is_gscsi() and
ipr_is_vset_device() in the same section of code.

---
Manoj Kumar

--
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 1 02/25] hpsa: remove unused hpsa_tag_discard_error_bits

[Manoj Kumar]

Don: See comment below.

- Manoj Kumar

On 10/28/2015 5:04 PM, Don Brace wrote:

This function is no longer used.

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
+#define HPSA_PERF_ERROR_BITS ((1 << DIRECT_LOOKUP_SHIFT) - 1)


HPSA_PERF_ERROR_BITS seems to be only used in the function that was
removed. Is there a reason to redefine this?


--
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 1 03/25] hpsa: check for null arguments to dev_printk

[Manoj Kumar]

On 10/28/2015 5:04 PM, Don Brace wrote:

Check for NULLs.
-   int devtype;
+   unsigned int devtype;


Don:

Unrelated to the NULL argument check. Would have been preferable in a
distinct patch.

Reviewed-by: Manoj Kumar <ma...@linux.vnet.ibm.com>

---
Manoj Kumar


--
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] scsi: Set sg_tablesize to 1, for LLDDs that set SG_NONE

[Manoj Kumar]

Oops while testing blk_mq over the new cxlflash driver.

[ 2960.817172] Oops: Kernel access of bad area, sig: 11 [#5]
[ 2960.817309] NIP  __blk_mq_run_hw_queue+0x278/0x4c0
[ 2960.817313] LR __blk_mq_run_hw_queue+0x2bc/0x4c0
[ 2960.817314] Call Trace:
[ 2960.817320] __blk_mq_run_hw_queue+0x2bc/0x4c0 (unreliable)
[ 2960.817324] blk_mq_run_hw_queue+0xd8/0x100
[ 2960.817329] blk_mq_insert_requests+0x14c/0x1f0
[ 2960.817333] blk_mq_flush_plug_list+0x150/0x190
[ 2960.817338] blk_flush_plug_list+0x11c/0x2b0
[ 2960.817344] blk_finish_plug+0x58/0x80
[ 2960.817348] __do_page_cache_readahead+0x1c0/0x2e0
[ 2960.817352] force_page_cache_readahead+0x68/0xd0
[ 2960.817356] generic_file_read_iter+0x43c/0x6a0
[ 2960.817359] blkdev_read_iter+0x68/0xa0
[ 2960.817361] __vfs_read+0x11c/0x180
[ 2960.817364] vfs_read+0xa4/0x1c0
[ 2960.817366] SyS_read+0x6c/0x110
[ 2960.817369] system_call+0x38/0xb4

The root cause of the problem was this low level device driver(LLDD),
in this case cxlflash, does not support scatter-gather and hence had
set it's sg_tablesize to SG_NONE (value of 0). In reality the tablesize
is of length 1. This value of SG_NONE does not cause any problems with
the standard block driver stack but causes issues for blk_mq, as shown
above. Since quite a few of the legacy LLDDs are setting sg_tablesize
to SG_NONE, it was preferable to override the LLDD provided value in
scsi_host_alloc().

Signed-off-by: Manoj N. Kumar 
Signed-off-by: Youngjae Lee 
---
 drivers/scsi/hosts.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 8bb173e..bd13c9d 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -413,7 +413,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template 
*sht, int privsize)
shost->hostt = sht;
shost->this_id = sht->this_id;
shost->can_queue = sht->can_queue;
-   shost->sg_tablesize = sht->sg_tablesize;
+   shost->sg_tablesize = (sht->sg_tablesize ? sht->sg_tablesize : 1);
shost->sg_prot_tablesize = sht->sg_prot_tablesize;
shost->cmd_per_lun = sht->cmd_per_lun;
shost->unchecked_isa_dma = sht->unchecked_isa_dma;
-- 
2.1.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 v6 35/37] cxlflash: Fix to avoid corrupting port selection mask

[Manoj Kumar]

Acked-by: Manoj Kumar <ma...@linux.vnet.ibm.com>

On 10/21/2015 3:16 PM, Matthew R. Ochs wrote:

The port selection mask of a LUN can be corrupted when the manage LUN
ioctl (DK_CXLFLASH_MANAGE_LUN) is issued more than once for any device.


--
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 v6 37/37] cxlflash: Fix to avoid bypassing context cleanup

[Manoj Kumar]

Acked-by: Manoj Kumar <ma...@linux.vnet.ibm.com>

On 10/21/2015 3:16 PM, Matthew R. Ochs wrote:

Contexts may be skipped over for cleanup in situations where contention
for the adapter's table-list mutex is experienced in the presence of a
signal during the execution of the release handler.


--
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 v6 36/37] cxlflash: Fix to avoid lock instrumentation rejection

[Manoj Kumar]

Acked-by: Manoj Kumar <ma...@linux.vnet.ibm.com>

On 10/21/2015 3:16 PM, Matthew R. Ochs wrote:

When running with lock instrumentation (e.g. lockdep), some of the
instrumentation can become disabled at probe time for a cxlflash
adapter. This is due to a missing lock registration for the tmf_slock.

The fix is to call spin_lock_init() for the tmf_slock during probe.


--
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 3/3] cxlflash: drop unlikely before IS_ERR_OR_NULL

[Manoj Kumar]

Geliang:

Thanks for catching this.

- Manoj

Acked-by: Manoj Kumar <ma...@linux.vnet.ibm.com>


On 9/30/2015 9:55 PM, Geliang Tang wrote:

IS_ERR_OR_NULL already contain an unlikely compiler flag. Drop it.

Signed-off-by: Geliang Tang <geliangt...@163.com>
---
  drivers/scsi/cxlflash/superpipe.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/cxlflash/superpipe.c 
b/drivers/scsi/cxlflash/superpipe.c
index f1b62ce..eb1b01e 100644
--- a/drivers/scsi/cxlflash/superpipe.c
+++ b/drivers/scsi/cxlflash/superpipe.c
@@ -1307,7 +1307,7 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
}

ctx = cxl_dev_context_init(cfg->dev);
-   if (unlikely(IS_ERR_OR_NULL(ctx))) {
+   if (IS_ERR_OR_NULL(ctx)) {
dev_err(dev, "%s: Could not initialize context %p\n",
__func__, ctx);
rc = -ENODEV;
@@ -1432,7 +1432,7 @@ static int recover_context(struct cxlflash_cfg *cfg, 
struct ctx_info *ctxi)
struct afu *afu = cfg->afu;

ctx = cxl_dev_context_init(cfg->dev);
-   if (unlikely(IS_ERR_OR_NULL(ctx))) {
+   if (IS_ERR_OR_NULL(ctx)) {
dev_err(dev, "%s: Could not initialize context %p\n",
__func__, ctx);
rc = -ENODEV;



--
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 v5 33/34] cxlflash: Fix to avoid leaving dangling interrupt resources

[Manoj Kumar]

Acked-by: Manoj Kumar <ma...@linux.vnet.ibm.com>

--
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] cxlflash: a couple off by one bugs

[Manoj Kumar]

Reviewed-by: Manoj Kumar <ma...@linux.vnet.ibm.com>

---
Manoj Kumar

On 9/22/2015 7:32 AM, Dan Carpenter wrote:

The "> MAX_CONTEXT" should be ">= MAX_CONTEXT".  Otherwise we go one
step beyond the end of the cfg->ctx_tbl[] array.

Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/scsi/cxlflash/superpipe.c 
b/drivers/scsi/cxlflash/superpipe.c
index f1b62ce..05e0ecf 100644
--- a/drivers/scsi/cxlflash/superpipe.c
+++ b/drivers/scsi/cxlflash/superpipe.c
@@ -1315,7 +1315,7 @@ static int cxlflash_disk_attach(struct scsi_device *sdev,
}

ctxid = cxl_process_element(ctx);
-   if (unlikely((ctxid > MAX_CONTEXT) || (ctxid < 0))) {
+   if (unlikely((ctxid >= MAX_CONTEXT) || (ctxid < 0))) {
dev_err(dev, "%s: ctxid (%d) invalid!\n", __func__, ctxid);
rc = -EPERM;
goto err1;
@@ -1440,7 +1440,7 @@ static int recover_context(struct cxlflash_cfg *cfg, 
struct ctx_info *ctxi)
}

ctxid = cxl_process_element(ctx);
-   if (unlikely((ctxid > MAX_CONTEXT) || (ctxid < 0))) {
+   if (unlikely((ctxid >= MAX_CONTEXT) || (ctxid < 0))) {
dev_err(dev, "%s: ctxid (%d) invalid!\n", __func__, ctxid);
rc = -EPERM;
goto err1;



--
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 v6 1/3] cxlflash: Base error recovery support

[Manoj Kumar]

James:

Wondering whether there is anything else you were expecting from us
before pulling this patch series in.

Regards,
- Manoj Kumar


On 8/13/2015 9:47 PM, Matthew R. Ochs wrote:

Introduce support for enhanced I/O error handling.

A device state is added to track 3 possible states of the device:

Normal - the device is operating normally and is fully operational

Limbo - the device is in a reset/recovery scenario and its operational
 status is paused

Failed/terminating - the device has either failed to be reset/recovered
  or is being terminated (removed); it is no longer
  operational

All operations are allowed when the device is operating normally. When the
device transitions to limbo state, I/O must be paused. To help accomplish
this, a wait queue is introduced where existing and new threads can wait
until the device is no longer in limbo. When coming out of limbo, threads
need to check the state and error out gracefully when encountering the
failed state. When the device transitions to the failed/terminating state,
normal operations are no longer allowed. Only specially designated
operations related to graceful cleanup are permitted.

Signed-off-by: Matthew R. Ochs mro...@linux.vnet.ibm.com
Signed-off-by: Manoj N. Kumar ma...@linux.vnet.ibm.com
Reviewed-by: Daniel Axtens d...@axtens.net
Reviewed-by: Michael Neuling mi...@neuling.org
Reviewed-by: Wen Xiong wenxi...@linux.vnet.ibm.com
---
  drivers/scsi/cxlflash/Kconfig   |   2 +-
  drivers/scsi/cxlflash/common.h  |  11 ++-
  drivers/scsi/cxlflash/main.c| 174 +---
  drivers/scsi/cxlflash/main.h|   6 +-
  drivers/scsi/cxlflash/sislite.h |   0
  5 files changed, 177 insertions(+), 16 deletions(-)
  mode change 100755 = 100644 drivers/scsi/cxlflash/sislite.h

diff --git a/drivers/scsi/cxlflash/Kconfig b/drivers/scsi/cxlflash/Kconfig
index c707508..c052104 100644
--- a/drivers/scsi/cxlflash/Kconfig
+++ b/drivers/scsi/cxlflash/Kconfig
@@ -4,7 +4,7 @@

  config CXLFLASH
tristate Support for IBM CAPI Flash
-   depends on PCI  SCSI  CXL
+   depends on PCI  SCSI  CXL  EEH
default m
help
  Allows CAPI Accelerated IO to Flash
diff --git a/drivers/scsi/cxlflash/common.h b/drivers/scsi/cxlflash/common.h
index fe86bfe..ffdbc57 100644
--- a/drivers/scsi/cxlflash/common.h
+++ b/drivers/scsi/cxlflash/common.h
@@ -76,6 +76,12 @@ enum cxlflash_init_state {
INIT_STATE_SCSI
  };

+enum cxlflash_state {
+   STATE_NORMAL,   /* Normal running state, everything good */
+   STATE_LIMBO,/* Limbo running state, trying to reset/recover */
+   STATE_FAILTERM  /* Failed/terminating state, error out users/threads */
+};
+
  /*
   * Each context has its own set of resource handles that is visible
   * only from that context.
@@ -91,8 +97,6 @@ struct cxlflash_cfg {

ulong cxlflash_regs_pci;

-   wait_queue_head_t eeh_waitq;
-
struct work_struct work_q;
enum cxlflash_init_state init_state;
enum cxlflash_lr_state lr_state;
@@ -105,7 +109,8 @@ struct cxlflash_cfg {

wait_queue_head_t tmf_waitq;
bool tmf_active;
-   u8 err_recovery_active:1;
+   wait_queue_head_t limbo_waitq;
+   enum cxlflash_state state;
  };

  struct afu_cmd {
diff --git a/drivers/scsi/cxlflash/main.c b/drivers/scsi/cxlflash/main.c
index 76a7286..4df1ff6 100644
--- a/drivers/scsi/cxlflash/main.c
+++ b/drivers/scsi/cxlflash/main.c
@@ -353,6 +353,7 @@ static int cxlflash_queuecommand(struct Scsi_Host *host, 
struct scsi_cmnd *scp)
struct cxlflash_cfg *cfg = (struct cxlflash_cfg *)host-hostdata;
struct afu *afu = cfg-afu;
struct pci_dev *pdev = cfg-dev;
+   struct device *dev = cfg-dev-dev;
struct afu_cmd *cmd;
u32 port_sel = scp-device-channel + 1;
int nseg, i, ncount;
@@ -380,6 +381,21 @@ static int cxlflash_queuecommand(struct Scsi_Host *host, 
struct scsi_cmnd *scp)
}
spin_unlock_irqrestore(cfg-tmf_waitq.lock, lock_flags);

+   switch (cfg-state) {
+   case STATE_LIMBO:
+   dev_dbg_ratelimited(dev, %s: device in limbo!\n, __func__);
+   rc = SCSI_MLQUEUE_HOST_BUSY;
+   goto out;
+   case STATE_FAILTERM:
+   dev_dbg_ratelimited(dev, %s: device has failed!\n, __func__);
+   scp-result = (DID_NO_CONNECT  16);
+   scp-scsi_done(scp);
+   rc = 0;
+   goto out;
+   default:
+   break;
+   }
+
cmd = cxlflash_cmd_checkout(afu);
if (unlikely(!cmd)) {
pr_err(%s: could not get a free command\n, __func__);
@@ -455,9 +471,21 @@ static int cxlflash_eh_device_reset_handler(struct 
scsi_cmnd *scp)
 get_unaligned_be32(((u32 *)scp-cmnd)[2]),
 get_unaligned_be32(((u32 *)scp-cmnd)[3]));

-   rcr = send_tmf(afu, scp, TMF_LUN_RESET);
-   if (unlikely

Re: [PATCH v5 2/3] cxlflash: Superpipe support

[Manoj Kumar]

Mikey:

Good catch. Will resolve this in the v6 patch.

Thanks,
- Manoj

On 8/13/2015 5:53 AM, Michael Neuling wrote:


+
+   ctxi = kzalloc(sizeof(*ctxi), GFP_KERNEL);
+   lli = kzalloc((MAX_RHT_PER_CONTEXT * sizeof(*lli)), GFP_KERNEL);
+   if (unlikely(!ctxi || !lli)) {
+   dev_err(dev, %s: Unable to allocate context!\n, __func__);
+   goto out;


If only one of these allocations fails you'll leak some memory.  I
suggest making this goto err, remove the out label and make err
look like this:

err:
if lli
kfree(lli);
if ctxi
kfree(ctxi);
return NULL;


Since kfree() handles being passed a NULL pointer, we will change this 
also to 'goto err'.


--
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 v5 3/3] cxlflash: Virtual LUN support

[Manoj Kumar]

Mikey:

Thanks for pointing this out. The patch for 2/3 should address this
issue.

Regards,
- Manoj Kumar


On 8/13/2015 8:08 PM, Michael Neuling wrote:

On Thu, 2015-08-13 at 18:43 -0500, Manoj Kumar wrote:

Mikey:

Thanks for your review. See comment inline below.

- Manoj Kumar

On 8/13/2015 7:03 AM, Michael Neuling wrote:

Thanks for integrating my suggestions.  create_context() has the same
freeing bug as 2/3 but if you fix that I'm happy if you add my
reviewed by:

Reviewed-by: Michael Neuling mi...@neuling.org


I believe create_context() is in 2/3, not here. I did not find the same
issue in this patch.



This is the section I referring to from this patch.  Seems to be
building on the 2/3 issue.


@@ -693,11 +737,13 @@ static struct ctx_info *create_context(struct 
cxlflash_cfg *cfg,
struct afu *afu = cfg-afu;
struct ctx_info *ctxi = NULL;
struct llun_info **lli = NULL;
+   bool *ws = NULL;
struct sisl_rht_entry *rhte;

ctxi = kzalloc(sizeof(*ctxi), GFP_KERNEL);
lli = kzalloc((MAX_RHT_PER_CONTEXT * sizeof(*lli)), GFP_KERNEL);
-   if (unlikely(!ctxi || !lli)) {
+   ws = kzalloc((MAX_RHT_PER_CONTEXT * sizeof(*ws)), GFP_KERNEL);
+   if (unlikely(!ctxi || !lli || !ws)) {
dev_err(dev, %s: Unable to allocate context!\n, __func__);
goto out;


This will be changed to 'goto err' by the v6 patch 2/3.



--
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 v4 3/3] cxlflash: Virtual LUN support

[Manoj Kumar]

Mikey: Thanks for your review. Comments inline below.

On 8/11/2015 5:54 AM, Michael Neuling wrote:


I'm not keen on the numerous pr_err() in here.  I think it'll make the driver
chatty especially with a badly behaving userspace.



Will look at all the pr_err() and limit them to errors that are 
indicative of a mis-behaving device, as opposed to a mis-behaving 
application.



-   .ioctl = cxlflash_ioctl,



Where are you hooking this in now?  This seems broken.


This was an error in splitting up the patch. Will correct in v5.


size = (MAX_RHT_PER_CONTEXT * sizeof(*ctxi-rht_lun));
+   size += (MAX_RHT_PER_CONTEXT * sizeof(*ctxi-rht_needs_ws));


This needs to be cleaned up as per my comment on patch 2.  Make this a separate
allocation.


Okay. Will split this into multiple allocations.



+   {sizeof(struct dk_cxlflash_clone), (sioctl)cxlflash_disk_clone},
{sizeof(struct dk_cxlflash_recover_afu), (sioctl)cxlflash_afu_recover},
{sizeof(struct dk_cxlflash_manage_lun), (sioctl)cxlflash_manage_lun},


Does this actually work if we don't have this patch?  If the IOCTLS are sparse
(like with only patch 2/3) won't this table be broken?


Agreed. Look for these ioctls being re-ordered in v5 to avoid sparseness 
in the individual patches.



switch (cmd) {
+   case DK_CXLFLASH_USER_VIRTUAL:
+   case DK_CXLFLASH_VLUN_RESIZE:
case DK_CXLFLASH_RELEASE:
+   case DK_CXLFLASH_CLONE:
pr_err(%s: %s not supported for lun_mode=%d\n,
   __func__, decode_ioctl(cmd), afu-internal_lun);
rc = -EINVAL;


If someone creates an internal lun and then does this, then we are going to get
a bunch of errors on the console.  I don't think that should happen.  Just
return it to the caller.


Will remove the pr_err().



Be good to do some clear sanity check the struct dk_cxlflash_resize *resize
here.  It's passed from userspace but then gets propogated to a bunch of other
things here like nsectors, get_context etc who will all now be responsible for
handling any dodgy data passed in.

Same with all the other ioctl calls.  Sanity check the parameters ASAP.  Don't
propagate potentially dodgy values all over the code base.



The ioctls have a standard header structure, with version etc. that are 
sanity checked before we get here. The other fields are sanity checked 
where they are used, i.e. in get_context().



+   /*
+* The requested size (req_size) is always assumed to be in 4k blocks,
+* so we have to convert it here from 4k to chunk size.
+*/
+   nsectors = (resize-req_size * CXLFLASH_BLOCK_SIZE) / gli-blk_len;
+   new_size = DIV_ROUND_UP(nsectors, MC_CHUNK_SIZE);


Like here. resize-req_size = new_size = grow_lxt() now grow_lxt() need to
sanity check new_size.



This is a best effort allocator. If an allocation request y, cannot be 
satisfied because of insufficient space in the LUN (i.e. only x amount 
of space is available), then the allocator returns the remaining space 
(x). This best effort allocation mechanism avoids having to sanity check 
the size parameter.




+   struct dk_cxlflash_uvirtual *virt = (struct dk_cxlflash_uvirtual *)arg;


Again, this should be sanity checked.


Same as comment above.


+   /* Resize even if requested size is 0 */
+   marshal_virt_to_resize(virt, resize);


Virt has not been sanity checked.  So now resize can contain bad data.



+   resize.rsrc_handle = rsrc_handle;


Same as above. As mentioned earlier, the size is immaterial. The rest of 
the parameters are set here (rsrc_handle).




+   dma_wmb(); /* Make RHT entry's LXT table size update visible */


Nice documentation here for the memory barriers!


Thank you!


+#define LXT_LUNIDX_SHIFT  8/* LXT entry, shift for LUN index */
+#define LXT_PERM_SHIFT4/* LXT entry, shift for permission bits */


What is LXT?


LXT = lun translation table. There is one LXT entry per set of 
contiguous blocks for a virtual LUN (known both to the host and to the 
AFU). Will clarify this with inline comments.



+
+#define BITS_PER_LONG 64


Please use #include asm/bitsperlong.h


Will re-use this, instead of creating our own definition.




+#define BPL   BITS_PER_LONG


Don't redefine this.  Make it harder for others to read.  No one wants to learn
your TLAs.


Same as above.


+   void *ba_lun_handle;


ba_lun_handle seems to be commonly cast to a struct ba_lun_info *.  Can it just
be a pointer to that?


Good catch. Will change in v5.


--
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 v4 2/3] cxlflash: Superpipe support

[Manoj Kumar]

Ben:

Comments inline below.


On 8/11/2015 12:29 AM, Benjamin Herrenschmidt wrote:

  So in a similar vein to the previous review, I am missing a lot of
context here but a few things did spring to me eyes:


Thanks for your review.


+   list_for_each_entry_safe(lli, temp, cfg-lluns, list)
+   if (!memcmp(lli-wwid, wwid, DK_CXLFLASH_MANAGE_LUN_WWID_LEN)) {
+   lli-newly_created = false;


This is weird ... a lookup effectively changes the state of the object
looked up... what for ? There is something oddball here.

It might be legit but in that case, you should really add a comment
explaining the logic around that 'newly_created' field.


As suggested later, will rename these functions in v5.


Also you drop the lock right below but you have no refcounting, are
these objects ever disposed of ?


These objects are long lived. The local lun info structure lives as
long as the card is available, and the global lun info is kept around
as long as the module is loaded.


In general, can you provide a clearer explanation of what are global
vs local LUNs ?


This is a good idea. Will clarify in v5.


Same ...


Same as above. Will address by renaming.


Make the function name more explicit: find_or_create_lun() for example.
I very much dislike a function called lookup that has side effects.


Good point. Will rename in v5.



+   lli = create_local(sdev, wwid);
+   if (unlikely(!lli))
+   goto out;


Similar question to earlier, you have no refcounting, should I assume
these things never get removed ?


Right, these are long lived.



+   lli-parent = gli;
+   spin_lock_irqsave(cfg-slock, lock_flags);
+   list_add(lli-list, cfg-lluns);
+   spin_unlock_irqrestore(cfg-slock, lock_flags);
+
+   spin_lock_irqsave(global.slock, lock_flags);
+   list_add(gli-list, global.gluns);
+   spin_unlock_irqrestore(global.slock, lock_flags);


Your locks are extremely fine grained... too much ? Any reason why you
don't have a simple/single lock handling all these ? IE, do you expect
frequent accesses ?

Also, a function called lookup_something that has the side effect of
adding that something to two lists doesn't look great to me. You may
want to review the function naming a bit.

Finally, what happens if two processes call this trying to effectively
create the same global LUN simultaneously ?

IE, can't you have a case where both lookup fail, then they both hit the
create_global() case for the same WWID ? Should you have a single lock
or a mutex wrapping the whole thing ? That would make the code a lot
simpler to review as well...


Good catch. Will look into simplifying to a mutex in v5, wrapping the
whole lookup/create sequence.



+void cxlflash_list_init(void)
+{
+   INIT_LIST_HEAD(global.gluns);
+   spin_lock_init(global.slock);
+   global.err_page = NULL;
+}


Wouldn't it make the code nicer to have all that LUN management in a
separate file ?


Good suggestion. Will look at moving these LUN management to a separate
file.


+   rc = mutex_lock_interruptible(cfg-ctx_tbl_list_mutex);
+   if (rc)
+   goto out;


This can be interrupted by any signal, I assume your userspace deals
with it ?


That is correct. We do want to be interrupted by any signal (SIGTERM, 
SIGKILL, SIGINT etc.) at this point. We fail the context validation, and 
ultimately the ioctl and leave it to user-space to deal with it.




+   rc = mutex_trylock(ctxi-mutex);
+   mutex_unlock(cfg-ctx_tbl_list_mutex);
+   if (!rc)
+   goto retry;


Ouch.. that's a nasty one. Are you using the above construct to avoid
an A-B/B-A deadlock scenario where somebody else might be taking
the list mutex while holding the context one ?


No, this is not addressing a lock ordering issue. Will clarify with a
comment why the list mutex is being released and reacquired in the
retry loop.


--
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 3/3] cxlflash: Virtual LUN support

[Manoj Kumar]

Wendy:

Thanks for taking the time to review this patch. Comments inline below.

- Manoj Kumar

On 7/29/2015 5:13 PM, wenxi...@linux.vnet.ibm.com wrote:

+/* Update the free_curr_idx */
+if (bit_pos == 63)
+lun_info-free_curr_idx = bit_word + 1;


Predefined Macros for 63 and 64?


Good point. We will add definitions to indicate that the bit_word is 8 
bytes.



+/**
+ * ba_clone() - frees a block from the block allocator
+ * @ba_lun:Block allocator from which to allocate a block.
+ * @to_free:Block to free.
+ *
+ * Return: 0 on success, -1 on failure
+ */


More accurate description about ba_clone() function.


Good catch. Will correct in the next version of this patch (v3).


+rc = ba_init(blka-ba_lun);


init_ba() and ba_init(). Probably one of them needs more accurate name.


Agreed. We will disambiguate in v3.


free cmd_buf and sense_buf?


Same issue that Brian had pointed out. We had already corrected earlier. 
You will see it in our next submission.



+mutex_unlock(blka-mutex);
+


Should hold the lock for lightwight sync?


+/*
+ * The following sequence is prescribed in the SISlite spec
+ * for syncing up with the AFU when adding LXT entries.
+ */
+dma_wmb(); /* Make LXT updates are visible */
+
+rhte-lxt_start = lxt;
+dma_wmb(); /* Make RHT entry's LXT table update visible */
+
+rhte-lxt_cnt = my_new_size;
+dma_wmb(); /* Make RHT entry's LXT table size update visible */
+
+cxlflash_afu_sync(afu, ctxid, rhndl, AFU_LW_SYNC);
+


cxlflash_afu_sync() does ensure that only one of these SYNC commands is 
outstanding at one time. No additional serialization is required.



Should hold the lock for lightwight sync?


+cxlflash_afu_sync(afu, ctxid, rhndl, AFU_HW_SYNC);


Same issue as the one discussed above. No additional serialization 
should be necessary.



+/* Setup the LUN table on the first call */
+rc = init_lun_table(cfg, lli);
+if (rc) {
+pr_err(%s: call to init_lun_table failed rc=%d!\n,
+   __func__, rc);
+goto out;
+}
+
+rc = init_ba(lli);
+if (rc) {
+pr_err(%s: call to init_ba failed rc=%d!\n,
+   __func__, rc);
+rc = -ENOMEM;


Do you need to remove the entry you create in init_lun_table() if
init_ba() fails?


The LUN table is global to the adapter. If there are two threads 
creating two virtual LUNs concurrently, the first one inserts the LUN 
into the table. Cannot have that table entry be deleted, even if 
init_ba() fails, as the other thread could be using it.




--
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] cxlflash: Base error recovery support

[Manoj Kumar]

Wendy:

Thanks for your review. Comment inline below.

- Manoj Kumar

On 7/29/2015 5:12 PM, wenxi...@linux.vnet.ibm.com wrote:

+
+cfg-eeh_active = EEH_STATE_NONE;
+wake_up_all(cfg-eeh_waitq);
+}
+


Do you need host-lock in these EEH callback functions?


These are synchronous callbacks and only one of them can be active on a 
card at a time. So, I do not believe these need the host-lock.


--
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 v5] cxlflash: Base support for IBM CXL Flash Adapter

[Manoj Kumar]

On 6/9/2015 6:29 AM, Brian King wrote:


Pulled out going to sleep in the queuecommand path.


udelay doesn't sleep, its a busy wait, so you can still use it in queuecommand,
just don't spend too much time, and its probably better to udelay then to
just re-read in a tight loop.


Thanks for the clarification. Will update in the next patch (v6).



This was the optimization to avoid the MMIO for both threads. The other thread 
that raced should do the atomic set of afu-room to a positive value.


Let's take the simpler scenario of just one thread.

Let's start with afu-room = 1
We call atomic64_dec_if_positive, which results in afu-room going to zero and 
0 being returned,
so we go into the if leg.

If afu-room is zero every time we read it from the adapter and we exhaust our 
retries,
we return SCSI_MLQUEUE_HOST_BUSY. However, the next time we enter 
cxlflash_send_cmd,
since afu-cmd is now 0, it will no longer get decremented, but the return 
value will
be -1, so we'll go down the else if leg. We'll never get into the if leg again 
to
re-read afu-room from the AFU. The simplest fix might just be to set afu-room 
= 1
if you ever leave the if leg without having room.


Good suggestion. Will atomic64_set(afu-room, 1), if we exhaust retries 
in both legs.





--
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 v4] cxlflash: Base support for IBM CXL Flash Adapter

[Manoj Kumar]

Brian:

Thank you for your review. Comments are inline.

- Manoj

On 6/8/2015 12:54 PM, Brian King wrote:

Looking pretty good. A few more comments.

Thanks,

Brian


+   spin_lock_irqsave(cfg-tmf_waitq.lock, lock_flags);
+   if (cfg-tmf_active)
+   wait_event_interruptible_locked_irq(cfg-tmf_waitq,
+   !cfg-tmf_active);
+   spin_unlock_irqrestore(cfg-tmf_waitq.lock, lock_flags);


This needs to return SCSI_MLQUEUE_HOST_BUSY instead of sleeping. You can't sleep
in queuecommand.



Okay, will revise in v5 to return an error instead of sleeping.


+   if (atomic64_dec_and_test(afu-room)) {


If you have two threads executing this code concurrently you could have a 
problem.
If afu-room = 1 and thread 1 decrements it and the return value is 0, we go 
into this
leg. If a second thread then comes in right after afu-room goes to zero, 
afu-room
will then get decremented to -1, and you'll send the command, regardless of 
whether
the AFU has room or not. Either the AFU will have room and afu-room will then
end up being off by one, or it won't have room and you'll send a command when it
does not have room.

I think if you use atomic_dec_if_positive instead, you can get rid of this race 
condition.
You'd then need to check the return value. If its positive, there is room, if 
it zero,
you are out of room and you are the thread that will reset afu-room from the 
AFU. If
it is negative, then you have to either return host busy, or wait for the other 
thread to
reset afu-room and simply try the atomic_dec_if_positive again in the loop here
instead of reading from the adapter and trying to set it from two threads.



Good catch. Will switch to atomic_dec_if_positive() in v5 to avoid the race.



--
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 v4] cxlflash: Base support for IBM CXL Flash Adapter

[Manoj Kumar]

On 6/8/2015 12:54 PM, Brian King wrote:


+
+   rcr = send_tmf(afu, scp, TMF_LUN_RESET);
+   if (unlikely(rcr))
+   rc = FAILED;


Do you need to wait for all commands to the LUN to be returned before returning 
from
here? You could put a simple loop here, polling until there are no ops 
outstanding
to this LUN, if needed...


Brian:

Good suggestion. Would it be acceptable to add this capability in a 
future patch?


- Manoj

--
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 v5] cxlflash: Base support for IBM CXL Flash Adapter

[Manoj Kumar]

Brian:

Thanks for the quick review. Comments below.

- Manoj

On 6/8/2015 5:56 PM, Brian King wrote:

+retry:
+   newval = atomic64_dec_if_positive(afu-room);
+   if (!newval) {
+   do {
+   room = readq_be(afu-host_map-cmd_room);
+   atomic64_set(afu-room, room);
+   if (room)
+   goto write_ioarrin;
+   } while (nretry++  MC_ROOM_RETRY_CNT);


It looks like you removed the udelay here. Was that intentional?


Pulled out going to sleep in the queuecommand path.


+
+   pr_err(%s: no cmd_room to send 0x%X\n,
+  __func__, cmd-rcb.cdb[0]);
+   rc = SCSI_MLQUEUE_HOST_BUSY;


If you actually get here, how do you get out of this state? Since now afu-room 
is
zero and anyone that comes through here will go to the else if leg.


This was the optimization to avoid the MMIO for both threads. The other 
thread that raced should do the atomic set of afu-room to a positive value.



+   goto out;
+   } else if (unlikely(newval  0)) {
+   /* This should be rare. i.e. Only if two threads race and
+* decrement before the MMIO read is done. In this case
+* just benefit from the other thread having updated
+* afu-room.
+*/
+   if (nretry++  MC_ROOM_RETRY_CNT)


I'm guessing you'd want the udelay here as well.


Same reason as the queuecommand issue above.


--
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 v3] cxlflash: Base support for IBM CXL Flash Adapter

[Manoj Kumar]

Brian:

Thanks for your review. Responses are inline below.

- Manoj Kumar


On 6/4/2015 9:38 AM, Brian King wrote:

+
+   write_lock(cfg-tmf_lock);


What is this lock protecting? The only thing it seems to be accomplishing is
making sure one thread isn't sending a TMF and another thread is sending
a normal I/O command at the exact same time, yet it looks like you still
allow a TMF to be sent and a normal I/O to be sent immediately after, before
receiving the TMF response.



Originally this section was waiting for the TMF response. I see that is 
no longer the case. Will restore the original behavior, with adequate 
locking.




+   afu-room = readq_be(afu-host_map-cmd_room);


Looks like you now have an MMIO load as part of sending every command,
including commands coming from queuecommand. Won't that be a performance issue?
Is there any way to avoid this? Could you perhaps decrement afu-room
in this function and only re-read it from the AFU when the counter hits zero?



Good point. Will revise to avoid MMIO in this performance path.



--
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