Re: [PATCH v2 22/30] scsi: aacraid: Merge adapter setup with resolve luns

[Nikola Pajkovsky]

Raghava Aditya Renukunta <raghavaaditya.renuku...@microsemi.com> writes:

> Hi Nikola,
>
>> -Original Message-----
>> From: Nikola Pajkovsky [mailto:npajkov...@suse.cz]
>> Sent: Wednesday, January 3, 2018 2:02 AM
>> To: Raghava Aditya Renukunta
>> <raghavaaditya.renuku...@microsemi.com>
>> Cc: j...@linux.vnet.ibm.com; martin.peter...@oracle.com; linux-
>> s...@vger.kernel.org; Scott Benesh <scott.ben...@microsemi.com>; Tom
>> White <tom.wh...@microsemi.com>; dl-esc-Aacraid Linux Driver
>> <aacr...@microsemi.com>; Guilherme G . Piccoli
>> <gpicc...@linux.vnet.ibm.com>; Bart Van Assche
>> <bart.vanass...@wdc.com>
>> Subject: Re: [PATCH v2 22/30] scsi: aacraid: Merge adapter setup with resolve
>> luns
>> 
>> EXTERNAL EMAIL
>> 
>> 
>> Raghava Aditya Renukunta <raghavaaditya.renuku...@microsemi.com>
>> writes:
>> 
>> > The device hotplug events are processed only after retrieving the updated
>> > lun information from the fw. Does not make sense to keep them separate.
>> >
>> > Merge both the hotplug handling and safw adapter setup code into single
>> > function.
>> >
>> > Signed-off-by: Raghava Aditya Renukunta
>> <raghavaaditya.renuku...@microsemi.com>
>> 
>> According to subsequent commit
>> 
>>   [PATCH v2 23/30] scsi: aacraid: Block concurrent hotplug event handling
>> 
>> this commit is racy, because 23/30 adds ->scan_mutex. Shouldn't be these
>> commits squashed?
>
> I tried to make the patches as logically distinct as possible, maybe I
> got a bit too ambitious and I expected the patches to go thru as a set so
> I don’t think it would make any difference. What do you think?

It does make difference, when you start cherry-picking patches to
downstream kernel. However, I don't have strong opinion here, so it can
stay as is.

-- 
Nikola

Re: [PATCH v2 22/30] scsi: aacraid: Merge adapter setup with resolve luns

[Nikola Pajkovsky]

Raghava Aditya Renukunta  writes:

> The device hotplug events are processed only after retrieving the updated
> lun information from the fw. Does not make sense to keep them separate.
>
> Merge both the hotplug handling and safw adapter setup code into single
> function.
>
> Signed-off-by: Raghava Aditya Renukunta 
> 

According to subsequent commit

  [PATCH v2 23/30] scsi: aacraid: Block concurrent hotplug event handling

this commit is racy, because 23/30 adds ->scan_mutex. Shouldn't be these
commits squashed?

--
Nikola

[PATCH] scsi: aacraid: error: testing array offset 'bus' after use

[Nikola Pajkovsky]

Fix possible indexing array of bound for >hba_map[bus][cid],
where bus and cid boundary check happens later.

Fixes: 0d643ff3c353 ("scsi: aacraid: use aac_tmf_callback for reset fib")
Signed-off-by: Nikola Pajkovsky <npajkov...@suse.cz>
---
 drivers/scsi/aacraid/linit.c | 20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
index 87cc4a93e637..62beb2596466 100644
--- a/drivers/scsi/aacraid/linit.c
+++ b/drivers/scsi/aacraid/linit.c
@@ -906,12 +906,14 @@ static int aac_eh_dev_reset(struct scsi_cmnd *cmd)
 
bus = aac_logical_to_phys(scmd_channel(cmd));
cid = scmd_id(cmd);
-   info = >hba_map[bus][cid];
-   if (bus >= AAC_MAX_BUSES || cid >= AAC_MAX_TARGETS ||
-   info->devtype != AAC_DEVTYPE_NATIVE_RAW)
+
+   if (bus >= AAC_MAX_BUSES || cid >= AAC_MAX_TARGETS)
return FAILED;
 
-   if (info->reset_state > 0)
+   info = >hba_map[bus][cid];
+
+   if (info->devtype != AAC_DEVTYPE_NATIVE_RAW &&
+   info->reset_state > 0)
return FAILED;
 
pr_err("%s: Host adapter reset request. SCSI hang ?\n",
@@ -962,12 +964,14 @@ static int aac_eh_target_reset(struct scsi_cmnd *cmd)
 
bus = aac_logical_to_phys(scmd_channel(cmd));
cid = scmd_id(cmd);
-   info = >hba_map[bus][cid];
-   if (bus >= AAC_MAX_BUSES || cid >= AAC_MAX_TARGETS ||
-   info->devtype != AAC_DEVTYPE_NATIVE_RAW)
+
+   if (bus >= AAC_MAX_BUSES || cid >= AAC_MAX_TARGETS)
return FAILED;
 
-   if (info->reset_state > 0)
+   info = >hba_map[bus][cid];
+
+   if (info->devtype != AAC_DEVTYPE_NATIVE_RAW &&
+   info->reset_state > 0)
return FAILED;
 
pr_err("%s: Host adapter reset request. SCSI hang ?\n",
-- 
2.13.5

[PATCH 1/3] scsi: aacraid: fix indentation errors

[Nikola Pajkovsky]

fix stupid indent error, no rocket science here.

Signed-off-by: Nikola Pajkovsky <npajkov...@suse.cz>
---
 drivers/scsi/aacraid/comminit.c | 6 +++---
 drivers/scsi/aacraid/linit.c| 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/aacraid/comminit.c b/drivers/scsi/aacraid/comminit.c
index 9ee025b1d0e0..97d269f16888 100644
--- a/drivers/scsi/aacraid/comminit.c
+++ b/drivers/scsi/aacraid/comminit.c
@@ -520,9 +520,9 @@ struct aac_dev *aac_init_adapter(struct aac_dev *dev)
dev->raw_io_64 = 1;
dev->sync_mode = aac_sync_mode;
if (dev->a_ops.adapter_comm &&
-   (status[1] & AAC_OPT_NEW_COMM)) {
-   dev->comm_interface = AAC_COMM_MESSAGE;
-   dev->raw_io_interface = 1;
+   (status[1] & AAC_OPT_NEW_COMM)) {
+   dev->comm_interface = AAC_COMM_MESSAGE;
+   dev->raw_io_interface = 1;
if ((status[1] & AAC_OPT_NEW_COMM_TYPE1)) {
/* driver supports TYPE1 (Tupelo) */
dev->comm_interface = AAC_COMM_MESSAGE_TYPE1;
diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
index 0f277df73af0..d7698a4cf840 100644
--- a/drivers/scsi/aacraid/linit.c
+++ b/drivers/scsi/aacraid/linit.c
@@ -1457,7 +1457,7 @@ static int aac_probe_one(struct pci_dev *pdev, const 
struct pci_device_id *id)
/*
 * Only series 7 needs freset.
 */
-if (pdev->device == PMC_DEVICE_S7)
+   if (pdev->device == PMC_DEVICE_S7)
pdev->needs_freset = 1;
 
list_for_each_entry(aac, _devices, entry) {
-- 
2.13.5

[PATCH 2/3] scsi: aacraid: get rid of one level of indentation

[Nikola Pajkovsky]

  unsigned long byte_count = 0;
  nseg = scsi_dma_map(scsicmd);
  if (nseg < 0)
 return nseg;
  if (nseg) {
 ...
  }
  return byte_count;

is equal to

  unsigned long byte_count = 0;
  nseg = scsi_dma_map(scsicmd);
  if (nseg <= 0)
 return nseg;
  ...
  return byte_count;

No other code has changed.

Signed-off-by: Nikola Pajkovsky <npajkov...@suse.cz>
---
 drivers/scsi/aacraid/aachba.c | 267 +-
 1 file changed, 131 insertions(+), 136 deletions(-)

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index a1a2c71e1626..3c354766791e 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -3763,6 +3763,8 @@ static long aac_build_sg(struct scsi_cmnd *scsicmd, 
struct sgmap *psg)
struct aac_dev *dev;
unsigned long byte_count = 0;
int nseg;
+   struct scatterlist *sg;
+   int i;
 
dev = (struct aac_dev *)scsicmd->device->host->hostdata;
// Get rid of old data
@@ -3771,32 +3773,29 @@ static long aac_build_sg(struct scsi_cmnd *scsicmd, 
struct sgmap *psg)
psg->sg[0].count = 0;
 
nseg = scsi_dma_map(scsicmd);
-   if (nseg < 0)
+   if (nseg <= 0)
return nseg;
-   if (nseg) {
-   struct scatterlist *sg;
-   int i;
 
-   psg->count = cpu_to_le32(nseg);
+   psg->count = cpu_to_le32(nseg);
 
-   scsi_for_each_sg(scsicmd, sg, nseg, i) {
-   psg->sg[i].addr = cpu_to_le32(sg_dma_address(sg));
-   psg->sg[i].count = cpu_to_le32(sg_dma_len(sg));
-   byte_count += sg_dma_len(sg);
-   }
-   /* hba wants the size to be exact */
-   if (byte_count > scsi_bufflen(scsicmd)) {
-   u32 temp = le32_to_cpu(psg->sg[i-1].count) -
-   (byte_count - scsi_bufflen(scsicmd));
-   psg->sg[i-1].count = cpu_to_le32(temp);
-   byte_count = scsi_bufflen(scsicmd);
-   }
-   /* Check for command underflow */
-   if(scsicmd->underflow && (byte_count < scsicmd->underflow)){
-   printk(KERN_WARNING"aacraid: cmd len %08lX cmd 
underflow %08X\n",
-   byte_count, scsicmd->underflow);
-   }
+   scsi_for_each_sg(scsicmd, sg, nseg, i) {
+   psg->sg[i].addr = cpu_to_le32(sg_dma_address(sg));
+   psg->sg[i].count = cpu_to_le32(sg_dma_len(sg));
+   byte_count += sg_dma_len(sg);
+   }
+   /* hba wants the size to be exact */
+   if (byte_count > scsi_bufflen(scsicmd)) {
+   u32 temp = le32_to_cpu(psg->sg[i-1].count) -
+   (byte_count - scsi_bufflen(scsicmd));
+   psg->sg[i-1].count = cpu_to_le32(temp);
+   byte_count = scsi_bufflen(scsicmd);
+   }
+   /* Check for command underflow */
+   if(scsicmd->underflow && (byte_count < scsicmd->underflow)){
+   printk(KERN_WARNING"aacraid: cmd len %08lX cmd underflow 
%08X\n",
+  byte_count, scsicmd->underflow);
}
+
return byte_count;
 }
 
@@ -3807,6 +3806,8 @@ static long aac_build_sg64(struct scsi_cmnd *scsicmd, 
struct sgmap64 *psg)
unsigned long byte_count = 0;
u64 addr;
int nseg;
+   struct scatterlist *sg;
+   int i;
 
dev = (struct aac_dev *)scsicmd->device->host->hostdata;
// Get rid of old data
@@ -3816,34 +3817,31 @@ static long aac_build_sg64(struct scsi_cmnd *scsicmd, 
struct sgmap64 *psg)
psg->sg[0].count = 0;
 
nseg = scsi_dma_map(scsicmd);
-   if (nseg < 0)
+   if (nseg <= 0)
return nseg;
-   if (nseg) {
-   struct scatterlist *sg;
-   int i;
-
-   scsi_for_each_sg(scsicmd, sg, nseg, i) {
-   int count = sg_dma_len(sg);
-   addr = sg_dma_address(sg);
-   psg->sg[i].addr[0] = cpu_to_le32(addr & 0x);
-   psg->sg[i].addr[1] = cpu_to_le32(addr>>32);
-   psg->sg[i].count = cpu_to_le32(count);
-   byte_count += count;
-   }
-   psg->count = cpu_to_le32(nseg);
-   /* hba wants the size to be exact */
-   if (byte_count > scsi_bufflen(scsicmd)) {
-   u32 temp = le32_to_cpu(psg->sg[i-1].count) -
-   (byte_count - scsi_bufflen(scsicmd));
-   psg->sg[i-1].count = cpu_to_le32(temp);
-   byte_count = scsi_bufflen(scsicmd);
-   }
-   /* Che

[PATCH 3/3] scsi: aacraid: report -ENOMEM to upper layer from aac_convert_sgraw2()

[Nikola Pajkovsky]

aac_convert_sgraw2() kmalloc memory and return -1 on error, which
should be -ENOMEM. However, nobody is checking return value, so with
this change, -ENOMEM is propagated to upper layer.

Signed-off-by: Nikola Pajkovsky <npajkov...@suse.cz>
---
 drivers/scsi/aacraid/aachba.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index 3c354766791e..ca38c2b52b47 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -3956,8 +3956,12 @@ static long aac_build_sgraw2(struct scsi_cmnd *scsicmd,
if (!err_found)
break;
}
-   if (i > 0 && nseg_new <= sg_max)
-   aac_convert_sgraw2(rio2, i, nseg, nseg_new);
+   if (i > 0 && nseg_new <= sg_max) {
+   int ret = aac_convert_sgraw2(rio2, i, nseg, nseg_new);
+
+   if (ret < 0)
+   return ret;
+   }
} else
rio2->flags |= cpu_to_le16(RIO2_SGL_CONFORMANT);
 
@@ -3981,7 +3985,7 @@ static int aac_convert_sgraw2(struct aac_raw_io2 *rio2, 
int pages, int nseg, int
 
sge = kmalloc(nseg_new * sizeof(struct sge_ieee1212), GFP_ATOMIC);
if (sge == NULL)
-   return -1;
+   return -ENOMEM;
 
for (i = 1, pos = 1; i < nseg-1; ++i) {
for (j = 0; j < rio2->sge[i].length / (pages * PAGE_SIZE); ++j) 
{
-- 
2.13.5

aac_convert_sgraw2() masks -ENOMEM

[Nikola Pajkovsky]

Hey,

today, I have run smatch and it reports, that aac_convert_sgraw2()
returns -1 instead of -ENOMEM on kmalloc(). This is easy to fix, but
nobody tests what aac_convert_sgraw2() returns in aac_build_sgraw2()

Since commit 0b4334473d48 ("[SCSI] aacraid: SCSI dma mapping failure
case handling"), aac_build_sgraw2() should have better error propagation
in aac_{read,write}_raw_io().

The question is, that if it's intentional (and I hope it's not) or it's
just leftover fix?

-- 
Nikola

Re: [PATCH 01/21] lpfc: Fix opps when ExpressLane is enabled

[Nikola Pajkovsky]

James Smart  writes:

> From: Dick Kennedy 
>
> Null pointer dereference in lpfc_sli4_fof_intr_handler
>
> The driver does not set up cq->assoc_qp for sli4_hba->oas_cq
>
> Initialize cq->assoc_qp before accessing it
>
> Signed-off-by: Dick Kennedy 
> Signed-off-by: James Smart 
> ---
>  drivers/scsi/lpfc/lpfc_sli.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
> index e948ea05fd33..0615bf9def23 100644
> --- a/drivers/scsi/lpfc/lpfc_sli.c
> +++ b/drivers/scsi/lpfc/lpfc_sli.c
> @@ -13557,6 +13557,9 @@ lpfc_sli4_fof_handle_eqe(struct lpfc_hba *phba, 
> struct lpfc_eqe *eqe)
>   /* Save EQ associated with this CQ */
>   cq->assoc_qp = phba->sli4_hba.fof_eq;
>  
> + /* Save EQ associated with this CQ */
> + cq->assoc_qp = phba->sli4_hba.fof_eq;
> +

Copy & paste error? Above lines are similar.

-- 
Nikola