[PATCH 0/7] mptfusion static checker fixups

[PATCH 0/7] mptfusion static checker fixups

[Joe Lawrence]

While reviewing the mpt2/mpt3 static checker fixup patchset, Christoph
inquired about mptfusion.  None of the sparse / smatch warnings from the
earlier patchset directly apply to fusion, but there were a few easy to
fix warnings (compile tested only).

The patchset is ordered from the smallest/easiest change up to the last
three, which are bit more involved and should be reviewed by LSI,
especially the last one "mptfusion: tweak null pointer checks".  See the
commentary in those patches after the signed-off-by line.

Cc: Christoph Hellwig <hch@infradead.org>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Sreekanth Reddy <Sreekanth.Reddy@lsi.com>

Joe Lawrence (7):
  mptfusion: mark file-private functions as static
  mptfusion: remove redundant kfree checks
  mptfusion: initChainBuffers should return errno
  mptfusion: zero kernel-space source of copy_to_user
  mptfusion: make adapter prod_name[] a pointer
  mptfusion: combine fw_event_work and its event_data
  mptfusion: tweak null pointer checks

 drivers/message/fusion/mptbase.c  |   29 +++++++++----------
 drivers/message/fusion/mptbase.h  |    2 +-
 drivers/message/fusion/mptctl.c   |    2 +-
 drivers/message/fusion/mptfc.c    |    3 +-
 drivers/message/fusion/mptsas.c   |   57 +++++++++++++++++++------------------
 drivers/message/fusion/mptsas.h   |    2 +-
 drivers/message/fusion/mptscsih.c |    7 +++--
 drivers/message/fusion/mptspi.c   |    5 ++--
 8 files changed, 52 insertions(+), 55 deletions(-)

-- 
1.7.10.4

[PATCH 1/7] mptfusion: mark file-private functions as static

[Joe Lawrence]

Fixes the following sparse warnings:

  drivers/message/fusion/mptbase.c:7011:1: warning: symbol
    'mpt_SoftResetHandler' was not declared. Should it be static?

  drivers/message/fusion/mptsas.c:1578:23: warning: symbol
    'mptsas_refreshing_device_handles' was not declared. Should it be
    static?

  drivers/message/fusion/mptsas.c:3653:24: warning: symbol
    'mptsas_expander_add' was not declared. Should it be static?

  drivers/message/fusion/mptsas.c:5327:1: warning: symbol
    'mptsas_shutdown' was not declared. Should it be static?

  drivers/message/fusion/mptspi.c:624:1: warning: symbol
    'mptscsih_quiesce_raid' was not declared. Should it be static?

Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Sreekanth Reddy <Sreekanth.Reddy@lsi.com>
---
 drivers/message/fusion/mptbase.c |    2 +-
 drivers/message/fusion/mptsas.c  |    6 +++---
 drivers/message/fusion/mptspi.c  |    2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
index 570b18a..5ef3f89 100644
--- a/drivers/message/fusion/mptbase.c
+++ b/drivers/message/fusion/mptbase.c
@@ -7007,7 +7007,7 @@ EXPORT_SYMBOL(mpt_halt_firmware);
  *	IOC doesn't reply to any outstanding request. This will transfer IOC
  *	to READY state.
  **/
-int
+static int
 mpt_SoftResetHandler(MPT_ADAPTER *ioc, int sleepFlag)
 {
 	int		 rc;
diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
index 00d339c..8bb580d 100644
--- a/drivers/message/fusion/mptsas.c
+++ b/drivers/message/fusion/mptsas.c
@@ -1575,7 +1575,7 @@ mptsas_del_end_device(MPT_ADAPTER *ioc, struct mptsas_phyinfo *phy_info)
 	mptsas_port_delete(ioc, phy_info->port_details);
 }
 
-struct mptsas_phyinfo *
+static struct mptsas_phyinfo *
 mptsas_refreshing_device_handles(MPT_ADAPTER *ioc,
 	struct mptsas_devinfo *sas_device)
 {
@@ -3650,7 +3650,7 @@ mptsas_send_expander_event(struct fw_event_work *fw_event)
  * @handle:
  *
  */
-struct mptsas_portinfo *
+static struct mptsas_portinfo *
 mptsas_expander_add(MPT_ADAPTER *ioc, u16 handle)
 {
 	struct mptsas_portinfo buffer, *port_info;
@@ -5323,7 +5323,7 @@ mptsas_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	return error;
 }
 
-void
+static void
 mptsas_shutdown(struct pci_dev *pdev)
 {
 	MPT_ADAPTER *ioc = pci_get_drvdata(pdev);
diff --git a/drivers/message/fusion/mptspi.c b/drivers/message/fusion/mptspi.c
index 5653e50..ed8de0a 100644
--- a/drivers/message/fusion/mptspi.c
+++ b/drivers/message/fusion/mptspi.c
@@ -620,7 +620,7 @@ static void mptspi_read_parameters(struct scsi_target *starget)
 	spi_width(starget) = (nego & MPI_SCSIDEVPAGE0_NP_WIDE) ? 1 : 0;
 }
 
-int
+static int
 mptscsih_quiesce_raid(MPT_SCSI_HOST *hd, int quiesce, u8 channel, u8 id)
 {
 	MPT_ADAPTER	*ioc = hd->ioc;
-- 
1.7.10.4

Re: [PATCH 1/7] mptfusion: mark file-private functions as static

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 2/7] mptfusion: remove redundant kfree checks

[Joe Lawrence]

Fixes the following smatch warnings:

  drivers/message/fusion/mptfc.c:529 mptfc_target_destroy() info:
    redundant null check on starget->hostdata calling kfree()

  drivers/message/fusion/mptspi.c:465 mptspi_target_destroy() info:
    redundant null check on starget->hostdata calling kfree()

Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Sreekanth Reddy <Sreekanth.Reddy@lsi.com>
---
 drivers/message/fusion/mptfc.c  |    3 +--
 drivers/message/fusion/mptspi.c |    3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/message/fusion/mptfc.c b/drivers/message/fusion/mptfc.c
index fd75108..4bac2ad 100644
--- a/drivers/message/fusion/mptfc.c
+++ b/drivers/message/fusion/mptfc.c
@@ -525,8 +525,7 @@ mptfc_target_destroy(struct scsi_target *starget)
 		if (ri)	/* better be! */
 			ri->starget = NULL;
 	}
-	if (starget->hostdata)
-		kfree(starget->hostdata);
+	kfree(starget->hostdata);
 	starget->hostdata = NULL;
 }
 
diff --git a/drivers/message/fusion/mptspi.c b/drivers/message/fusion/mptspi.c
index ed8de0a..7ce9a5e 100644
--- a/drivers/message/fusion/mptspi.c
+++ b/drivers/message/fusion/mptspi.c
@@ -461,8 +461,7 @@ static int mptspi_target_alloc(struct scsi_target *starget)
 static void
 mptspi_target_destroy(struct scsi_target *starget)
 {
-	if (starget->hostdata)
-		kfree(starget->hostdata);
+	kfree(starget->hostdata);
 	starget->hostdata = NULL;
 }
 
-- 
1.7.10.4

Re: [PATCH 2/7] mptfusion: remove redundant kfree checks

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 3/7] mptfusion: initChainBuffers should return errno

[Joe Lawrence]

The lone caller of initChainBuffers checks the return code for < 0, so
it is safe to appease smatch and return the proper errno value.

Fixes the following smatch warnings:

  drivers/message/fusion/mptbase.c:4328 initChainBuffers() warn:
    returning -1 instead of -ENOMEM is sloppy

  drivers/message/fusion/mptbase.c:4335 initChainBuffers() warn:
    returning -1 instead of -ENOMEM is sloppy

  drivers/message/fusion/mptbase.c:4402 initChainBuffers() warn:
    returning -1 instead of -ENOMEM is sloppy

Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Sreekanth Reddy <Sreekanth.Reddy@lsi.com>
---
 drivers/message/fusion/mptbase.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
index 5ef3f89..a70b873 100644
--- a/drivers/message/fusion/mptbase.c
+++ b/drivers/message/fusion/mptbase.c
@@ -4325,14 +4325,14 @@ initChainBuffers(MPT_ADAPTER *ioc)
 		sz = ioc->req_depth * sizeof(int);
 		mem = kmalloc(sz, GFP_ATOMIC);
 		if (mem == NULL)
-			return -1;
+			return -ENOMEM;
 
 		ioc->ReqToChain = (int *) mem;
 		dinitprintk(ioc, printk(MYIOC_s_DEBUG_FMT "ReqToChain alloc  @ %p, sz=%d bytes\n",
 			 	ioc->name, mem, sz));
 		mem = kmalloc(sz, GFP_ATOMIC);
 		if (mem == NULL)
-			return -1;
+			return -ENOMEM;
 
 		ioc->RequestNB = (int *) mem;
 		dinitprintk(ioc, printk(MYIOC_s_DEBUG_FMT "RequestNB alloc  @ %p, sz=%d bytes\n",
@@ -4399,7 +4399,7 @@ initChainBuffers(MPT_ADAPTER *ioc)
 	if (ioc->ChainToChain == NULL) {
 		mem = kmalloc(sz, GFP_ATOMIC);
 		if (mem == NULL)
-			return -1;
+			return -ENOMEM;
 
 		ioc->ChainToChain = (int *) mem;
 		dinitprintk(ioc, printk(MYIOC_s_DEBUG_FMT "ChainToChain alloc @ %p, sz=%d bytes\n",
-- 
1.7.10.4

Re: [PATCH 3/7] mptfusion: initChainBuffers should return errno

[Christoph Hellwig]

On Wed, Jun 04, 2014 at 12:49:45PM -0400, Joe Lawrence wrote:
> The lone caller of initChainBuffers checks the return code for < 0, so
> it is safe to appease smatch and return the proper errno value.

I don't think this is useful on it's own as the whole callchain around
it uses the same -1 for error convention.  Returning proper errnos
seems useful to me, but without converting the whole chain it would
just introduce more inconsistencies.

[PATCH 4/7] mptfusion: zero kernel-space source of copy_to_user

[Joe Lawrence]

Fixes the following smatch warning:

  drivers/message/fusion/mptctl.c:1369 mptctl_getiocinfo() warn:
    possible info leak 'karg'

Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Sreekanth Reddy <Sreekanth.Reddy@lsi.com>
---
 drivers/message/fusion/mptctl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/message/fusion/mptctl.c b/drivers/message/fusion/mptctl.c
index dcc8385..e6d8935 100644
--- a/drivers/message/fusion/mptctl.c
+++ b/drivers/message/fusion/mptctl.c
@@ -1261,7 +1261,7 @@ mptctl_getiocinfo (unsigned long arg, unsigned int data_size)
 	else
 		return -EFAULT;
 
-	karg = kmalloc(data_size, GFP_KERNEL);
+	karg = kzalloc(data_size, GFP_KERNEL);
 	if (karg == NULL) {
 		printk(KERN_ERR MYNAM "%s::mpt_ioctl_iocinfo() @%d - no memory available!\n",
 				__FILE__, __LINE__);
-- 
1.7.10.4

Re: [PATCH 4/7] mptfusion: zero kernel-space source of copy_to_user

[Joe Lawrence]

On Wed, 4 Jun 2014 12:49:46 -0400
Joe Lawrence <joe.lawrence@stratus.com> wrote:

> Fixes the following smatch warning:
> 
>   drivers/message/fusion/mptctl.c:1369 mptctl_getiocinfo() warn:
>     possible info leak 'karg'
> 
> Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Cc: Sreekanth Reddy <Sreekanth.Reddy@lsi.com>
> ---
>  drivers/message/fusion/mptctl.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/message/fusion/mptctl.c b/drivers/message/fusion/mptctl.c
> index dcc8385..e6d8935 100644
> --- a/drivers/message/fusion/mptctl.c
> +++ b/drivers/message/fusion/mptctl.c
> @@ -1261,7 +1261,7 @@ mptctl_getiocinfo (unsigned long arg, unsigned int data_size)
>  	else
>  		return -EFAULT;
>  
> -	karg = kmalloc(data_size, GFP_KERNEL);
> +	karg = kzalloc(data_size, GFP_KERNEL);
>  	if (karg == NULL) {
>  		printk(KERN_ERR MYNAM "%s::mpt_ioctl_iocinfo() @%d - no memory available!\n",
>  				__FILE__, __LINE__);

Hi Dan,

kzalloc silenced that smatch warning, but the code looks like:

  (calculate data_size)
  ...
  karg = kmalloc(data_size, GFP_KERNEL);
  ...
  if (copy_from_user(karg, uarg, data_size)) {
  ...
  if (copy_to_user((char __user *)arg, karg, data_size)) {

where 'data_size' once calculated, is unchanged.  Since the size
allocated is the same copied from the user and the same copied back out
to the user, would this really be considered an info leak?

Regards,

-- Joe

Re: [PATCH 4/7] mptfusion: zero kernel-space source of copy_to_user

[Christoph Hellwig]

On Wed, Jun 04, 2014 at 12:58:36PM -0400, Joe Lawrence wrote:
> Hi Dan,
> 
> kzalloc silenced that smatch warning, but the code looks like:
> 
>   (calculate data_size)
>   ...
>   karg = kmalloc(data_size, GFP_KERNEL);
>   ...
>   if (copy_from_user(karg, uarg, data_size)) {
>   ...
>   if (copy_to_user((char __user *)arg, karg, data_size)) {
> 
> where 'data_size' once calculated, is unchanged.  Since the size
> allocated is the same copied from the user and the same copied back out
> to the user, would this really be considered an info leak?

I think the stastic checker is wrong here.  But the code would still
benefit from switching to memdup_user, which should shut up the checker
in addition to simplifying the code.

Re: [PATCH 4/7] mptfusion: zero kernel-space source of copy_to_user

[Dan Carpenter]

On Wed, Jun 04, 2014 at 12:49:46PM -0400, Joe Lawrence wrote:
> Fixes the following smatch warning:
> 
>   drivers/message/fusion/mptctl.c:1369 mptctl_getiocinfo() warn:
>     possible info leak 'karg'
> 

This is a false positive.  I will push a fix for this in Smatch next
week.

regards,
dan carpenter

[PATCH 5/7] mptfusion: make adapter prod_name[] a pointer

[Joe Lawrence]

The struct _MPT_ADAPTER doesn't need a full copy of the product string,
so prod_name can point to the string literal storage that the driver
already provides.

Avoids the following sparse warning:

  drivers/message/fusion/mptbase.c:2858 MptDisplayIocCapabilities()
    warn: this array is probably non-NULL. 'ioc->prod_name'

Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Sreekanth Reddy <Sreekanth.Reddy@lsi.com>
---
 drivers/message/fusion/mptbase.c |   11 +++++------
 drivers/message/fusion/mptbase.h |    2 +-
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
index a70b873..03263d6 100644
--- a/drivers/message/fusion/mptbase.c
+++ b/drivers/message/fusion/mptbase.c
@@ -1408,8 +1408,8 @@ mpt_verify_adapter(int iocid, MPT_ADAPTER **iocpp)
  *	in /proc/mpt/summary and /sysfs/class/scsi_host/host<X>/version_product
  *
  **/
-static void
-mpt_get_product_name(u16 vendor, u16 device, u8 revision, char *prod_name)
+static const char*
+mpt_get_product_name(u16 vendor, u16 device, u8 revision)
 {
 	char *product_str = NULL;
 
@@ -1635,8 +1635,7 @@ mpt_get_product_name(u16 vendor, u16 device, u8 revision, char *prod_name)
 	}
 
  out:
-	if (product_str)
-		sprintf(prod_name, "%s", product_str);
+	return product_str;
 }
 
 /**
@@ -1887,8 +1886,8 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
 	dinitprintk(ioc, printk(MYIOC_s_INFO_FMT "facts @ %p, pfacts[0] @ %p\n",
 	    ioc->name, &ioc->facts, &ioc->pfacts[0]));
 
-	mpt_get_product_name(pdev->vendor, pdev->device, pdev->revision,
-			     ioc->prod_name);
+	ioc->prod_name = mpt_get_product_name(pdev->vendor, pdev->device,
+					      pdev->revision);
 
 	switch (pdev->device)
 	{
diff --git a/drivers/message/fusion/mptbase.h b/drivers/message/fusion/mptbase.h
index 76c05bc..78c8104 100644
--- a/drivers/message/fusion/mptbase.h
+++ b/drivers/message/fusion/mptbase.h
@@ -605,7 +605,7 @@ typedef struct _MPT_ADAPTER
 	int			 id;		/* Unique adapter id N {0,1,2,...} */
 	int			 pci_irq;	/* This irq           */
 	char			 name[MPT_NAME_LENGTH];	/* "iocN"             */
-	char			 prod_name[MPT_NAME_LENGTH];	/* "LSIFC9x9"         */
+	const char		 *prod_name;	/* "LSIFC9x9"         */
 #ifdef CONFIG_FUSION_LOGGING
 	/* used in mpt_display_event_info */
 	char			 evStr[EVENT_DESCR_STR_SZ];
-- 
1.7.10.4

Re: [PATCH 5/7] mptfusion: make adapter prod_name[] a pointer

[Christoph Hellwig]

On Wed, Jun 04, 2014 at 12:51:55PM -0400, Joe Lawrence wrote:
> The struct _MPT_ADAPTER doesn't need a full copy of the product string,
> so prod_name can point to the string literal storage that the driver
> already provides.
> 
> Avoids the following sparse warning:
> 
>   drivers/message/fusion/mptbase.c:2858 MptDisplayIocCapabilities()
>     warn: this array is probably non-NULL. 'ioc->prod_name'

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 6/7] mptfusion: combine fw_event_work and its event_data

[Joe Lawrence]

Tack the firmware reply event_data payload to the end of its
corresponding struct fw_event_work allocation.  Rework fw_event_work
allocation calculations to include the event_data size where
appropriate.

This clarifies the code a bit and avoids the following smatch warnings:

  drivers/message/fusion/mptsas.c:1003 mptsas_queue_device_delete()
    error: memcpy() 'fw_event->event_data' too small (29 vs 36)

  drivers/message/fusion/mptsas.c:1017 mptsas_queue_rescan() error: not
    allocating enough data 168 vs 160

Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Sreekanth Reddy <Sreekanth.Reddy@lsi.com>
---

JL: In the existing code, struct fw_event_work includes an aligned
    char event_data[1] and when event data payload is required, a
    larger number of bytes is allocated.  This is calculated by
    subtracting the offsetof(struct fw_event_work, event_data) from
    the sizeof(struct fw_event_work) and adding the required payload
    size.  Was a single byte required to guarantee the alignment?
    If not, it seems clearer to just use a 0-length array and avoid the
    offsetof part.

 drivers/message/fusion/mptsas.c |   24 ++++++++++++------------
 drivers/message/fusion/mptsas.h |    2 +-
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
index 8bb580d..35c87b1 100644
--- a/drivers/message/fusion/mptsas.c
+++ b/drivers/message/fusion/mptsas.c
@@ -992,15 +992,15 @@ mptsas_queue_device_delete(MPT_ADAPTER *ioc,
 	struct fw_event_work *fw_event;
 	int sz;
 
-	sz = offsetof(struct fw_event_work, event_data) +
-	    sizeof(MpiEventDataSasDeviceStatusChange_t);
+	sz = sizeof(*fw_event) +
+		sizeof(MpiEventDataSasDeviceStatusChange_t);
 	fw_event = kzalloc(sz, GFP_ATOMIC);
 	if (!fw_event) {
 		printk(MYIOC_s_WARN_FMT "%s: failed at (line=%d)\n",
 		    ioc->name, __func__, __LINE__);
 		return;
 	}
-	memcpy(fw_event->event_data, sas_event_data,
+	memcpy(&fw_event->event_data, sas_event_data,
 	    sizeof(MpiEventDataSasDeviceStatusChange_t));
 	fw_event->event = MPI_EVENT_SAS_DEVICE_STATUS_CHANGE;
 	fw_event->ioc = ioc;
@@ -1013,7 +1013,7 @@ mptsas_queue_rescan(MPT_ADAPTER *ioc)
 	struct fw_event_work *fw_event;
 	int sz;
 
-	sz = offsetof(struct fw_event_work, event_data);
+	sz = sizeof(*fw_event);
 	fw_event = kzalloc(sz, GFP_ATOMIC);
 	if (!fw_event) {
 		printk(MYIOC_s_WARN_FMT "%s: failed at (line=%d)\n",
@@ -3617,7 +3617,7 @@ mptsas_send_expander_event(struct fw_event_work *fw_event)
 
 	ioc = fw_event->ioc;
 	expander_data = (MpiEventDataSasExpanderStatusChange_t *)
-	    fw_event->event_data;
+	    &fw_event->event_data;
 	memcpy(&sas_address, &expander_data->SASAddress, sizeof(__le64));
 	sas_address = le64_to_cpu(sas_address);
 	port_info = mptsas_find_portinfo_by_sas_address(ioc, sas_address);
@@ -3694,7 +3694,7 @@ mptsas_send_link_status_event(struct fw_event_work *fw_event)
 	u8 link_rate;
 
 	ioc = fw_event->ioc;
-	link_data = (MpiEventDataSasPhyLinkStatus_t *)fw_event->event_data;
+	link_data = (MpiEventDataSasPhyLinkStatus_t *) &fw_event->event_data;
 
 	memcpy(&sas_address, &link_data->SASAddress, sizeof(__le64));
 	sas_address = le64_to_cpu(sas_address);
@@ -4043,7 +4043,7 @@ mptsas_handle_queue_full_event(struct fw_event_work *fw_event)
 
 
 	ioc = fw_event->ioc;
-	qfull_data = (EventDataQueueFull_t *)fw_event->event_data;
+	qfull_data = (EventDataQueueFull_t *) &fw_event->event_data;
 	fw_id = qfull_data->TargetID;
 	fw_channel = qfull_data->Bus;
 	current_depth = le16_to_cpu(qfull_data->CurrentDepth);
@@ -4584,7 +4584,7 @@ mptsas_send_sas_event(struct fw_event_work *fw_event)
 
 	ioc = fw_event->ioc;
 	sas_event_data = (EVENT_DATA_SAS_DEVICE_STATUS_CHANGE *)
-	    fw_event->event_data;
+	    &fw_event->event_data;
 	device_info = le32_to_cpu(sas_event_data->DeviceInfo);
 
 	if ((device_info &
@@ -4652,7 +4652,7 @@ mptsas_send_raid_event(struct fw_event_work *fw_event)
 	RaidPhysDiskPage0_t phys_disk;
 
 	ioc = fw_event->ioc;
-	raid_event_data = (EVENT_DATA_RAID *)fw_event->event_data;
+	raid_event_data = (EVENT_DATA_RAID *) &fw_event->event_data;
 	status = le32_to_cpu(raid_event_data->SettingsStatus);
 	state = (status >> 8) & 0xff;
 
@@ -4950,7 +4950,7 @@ mptsas_send_ir2_event(struct fw_event_work *fw_event)
 	RaidPhysDiskPage0_t phys_disk;
 
 	ioc = fw_event->ioc;
-	ir2_data = (MPI_EVENT_DATA_IR2 *)fw_event->event_data;
+	ir2_data = (MPI_EVENT_DATA_IR2 *) &fw_event->event_data;
 	reasonCode = ir2_data->ReasonCode;
 
 	devtprintk(ioc, printk(MYIOC_s_DEBUG_FMT "Entering %s: "
@@ -5095,14 +5095,14 @@ mptsas_event_process(MPT_ADAPTER *ioc, EventNotificationReply_t *reply)
 
 	event_data_sz = ((reply->MsgLength * 4) -
 	    offsetof(EventNotificationReply_t, Data));
-	sz = offsetof(struct fw_event_work, event_data) + event_data_sz;
+	sz = sizeof(*fw_event) + event_data_sz;
 	fw_event = kzalloc(sz, GFP_ATOMIC);
 	if (!fw_event) {
 		printk(MYIOC_s_WARN_FMT "%s: failed at (line=%d)\n", ioc->name,
 		 __func__, __LINE__);
 		return 0;
 	}
-	memcpy(fw_event->event_data, reply->Data, event_data_sz);
+	memcpy(&fw_event->event_data, reply->Data, event_data_sz);
 	fw_event->event = event;
 	fw_event->ioc = ioc;
 	mptsas_add_fw_event(ioc, fw_event, delay);
diff --git a/drivers/message/fusion/mptsas.h b/drivers/message/fusion/mptsas.h
index 57e86ab..c396483 100644
--- a/drivers/message/fusion/mptsas.h
+++ b/drivers/message/fusion/mptsas.h
@@ -110,7 +110,7 @@ struct fw_event_work {
 	MPT_ADAPTER	*ioc;
 	u32			event;
 	u8			retries;
-	u8			__attribute__((aligned(4))) event_data[1];
+	char			event_data[0] __aligned(4);
 };
 
 struct mptsas_discovery_event {
-- 
1.7.10.4

Re: [PATCH 6/7] mptfusion: combine fw_event_work and its event_data

[Christoph Hellwig]

>  
> -	sz = offsetof(struct fw_event_work, event_data) +
> -	    sizeof(MpiEventDataSasDeviceStatusChange_t);
> +	sz = sizeof(*fw_event) +
> +		sizeof(MpiEventDataSasDeviceStatusChange_t);
>  	fw_event = kzalloc(sz, GFP_ATOMIC);

Seems like there is no point in keeping the sz variable here and at
the other occurances.  Not that it really matters, but if we make a pass
over this code we might as well fix that up, too.

> -	sz = offsetof(struct fw_event_work, event_data);
> +	sz = sizeof(*fw_event);

Eww, using offsetoff for an allocation size.  Good riddance that this
gets sorted out.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 6/7] mptfusion: combine fw_event_work and its event_data

[Joe Lawrence]

On Thu, 5 Jun 2014 02:34:29 -0700
Christoph Hellwig <hch@infradead.org> wrote:

> > diff --git a/drivers/message/fusion/mptsas.h b/drivers/message/fusion/mptsas.h
> > index 57e86ab..c396483 100644
> > --- a/drivers/message/fusion/mptsas.h
> > +++ b/drivers/message/fusion/mptsas.h
> > @@ -110,7 +110,7 @@ struct fw_event_work {
> >  	MPT_ADAPTER	*ioc;
> >  	u32			event;
> >  	u8			retries;
> > -	u8			__attribute__((aligned(4))) event_data[1];
> > +	char			event_data[0] __aligned(4);
> >  };

Is this alignment necessary and if so, should it be more like:
  __attribute__ ((aligned (sizeof(unsigned long))))

> >  
> > -	sz = offsetof(struct fw_event_work, event_data) +
> > -	    sizeof(MpiEventDataSasDeviceStatusChange_t);
> > +	sz = sizeof(*fw_event) +
> > +		sizeof(MpiEventDataSasDeviceStatusChange_t);
> >  	fw_event = kzalloc(sz, GFP_ATOMIC);
> 
> Seems like there is no point in keeping the sz variable here and at
> the other occurances.  Not that it really matters, but if we make a pass
> over this code we might as well fix that up, too.
 
I'll take a look and post up a v2 next week, adding these to a v2
mpt2/mpt3 set.

Thanks,

-- Joe

[PATCH 7/7] mptfusion: tweak null pointer checks

[Joe Lawrence]

Fixes the following smatch warnings:

  drivers/message/fusion/mptbase.c:652 mptbase_reply() warn: variable
    dereferenced before check 'reply' (see line 639)

  drivers/message/fusion/mptsas.c:1255 mptsas_taskmgmt_complete() error:
    we previously assumed 'pScsiTmReply' could be null (see line 1227)


  drivers/message/fusion/mptsas.c:3888 mptsas_not_responding_devices()
    error: we previously assumed 'port_info->phy_info' could be null
    (see line 3875)

  drivers/message/fusion/mptscsih.c:1284 mptscsih_info() error:
    we previously assumed 'h' could be null (see line 1274)

  drivers/message/fusion/mptscsih.c:1388 mptscsih_qcmd() error: we
    previously assumed 'vdevice' could be null (see line 1373)

Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Sreekanth Reddy <Sreekanth.Reddy@lsi.com>
---

JL: Comments on the above warnings, in order:

    No-brainer, the enclosing switch statement dereferences 'reply',
    so we can't get here unless 'reply' is valid.

    "Retry target reset" needs to know the target ID and channel, so
    enclose that entire block inside a if (pScsiTmReply) block.

    The code before the loop checks for 'port_info->phy_info', but not
    many other places in the driver bother.  Not sure what to do here.

    'hostdata' is checked and then immediately dereferenced after
    that block.  How about a default return string of "N/A" to indicate
    one isn't available? 

    Not sure what to do here.  'vdevice' is needed to "set up the
    message frame" target ID and channel, so it should probably return
    an error in in this case.

 drivers/message/fusion/mptbase.c  |   10 ++++------
 drivers/message/fusion/mptsas.c   |   27 ++++++++++++++-------------
 drivers/message/fusion/mptscsih.c |    7 ++++---
 3 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
index 03263d6..c68a951 100644
--- a/drivers/message/fusion/mptbase.c
+++ b/drivers/message/fusion/mptbase.c
@@ -649,12 +649,10 @@ mptbase_reply(MPT_ADAPTER *ioc, MPT_FRAME_HDR *req, MPT_FRAME_HDR *reply)
 	case MPI_FUNCTION_CONFIG:
 	case MPI_FUNCTION_SAS_IO_UNIT_CONTROL:
 		ioc->mptbase_cmds.status |= MPT_MGMT_STATUS_COMMAND_GOOD;
-		if (reply) {
-			ioc->mptbase_cmds.status |= MPT_MGMT_STATUS_RF_VALID;
-			memcpy(ioc->mptbase_cmds.reply, reply,
-			    min(MPT_DEFAULT_FRAME_SIZE,
-				4 * reply->u.reply.MsgLength));
-		}
+		ioc->mptbase_cmds.status |= MPT_MGMT_STATUS_RF_VALID;
+		memcpy(ioc->mptbase_cmds.reply, reply,
+		    min(MPT_DEFAULT_FRAME_SIZE,
+			4 * reply->u.reply.MsgLength));
 		if (ioc->mptbase_cmds.status & MPT_MGMT_STATUS_PENDING) {
 			ioc->mptbase_cmds.status &= ~MPT_MGMT_STATUS_PENDING;
 			complete(&ioc->mptbase_cmds.done);
diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
index 35c87b1..72e464fc 100644
--- a/drivers/message/fusion/mptsas.c
+++ b/drivers/message/fusion/mptsas.c
@@ -1252,17 +1252,19 @@ mptsas_taskmgmt_complete(MPT_ADAPTER *ioc, MPT_FRAME_HDR *mf, MPT_FRAME_HDR *mr)
 	    ioc->name, jiffies_to_msecs(jiffies -
 	    target_reset_list->time_count)/1000));
 
-	id = pScsiTmReply->TargetID;
-	channel = pScsiTmReply->Bus;
-	target_reset_list->time_count = jiffies;
+	if (pScsiTmReply) {
+		id = pScsiTmReply->TargetID;
+		channel = pScsiTmReply->Bus;
+		target_reset_list->time_count = jiffies;
 
-	/*
-	 * retry target reset
-	 */
-	if (!target_reset_list->target_reset_issued) {
-		if (mptsas_target_reset(ioc, channel, id))
-			target_reset_list->target_reset_issued = 1;
-		return 1;
+		/*
+		 * retry target reset
+		 */
+		if (!target_reset_list->target_reset_issued) {
+			if (mptsas_target_reset(ioc, channel, id))
+				target_reset_list->target_reset_issued = 1;
+			return 1;
+		}
 	}
 
 	/*
@@ -3872,9 +3874,8 @@ retry_page:
  redo_expander_scan:
 	list_for_each_entry(port_info, &ioc->sas_topology, list) {
 
-		if (port_info->phy_info &&
-		    (!(port_info->phy_info[0].identify.device_info &
-		    MPI_SAS_DEVICE_INFO_SMP_TARGET)))
+		if (!(port_info->phy_info[0].identify.device_info &
+		    MPI_SAS_DEVICE_INFO_SMP_TARGET))
 			continue;
 		found_expander = 0;
 		handle = 0xFFFF;
diff --git a/drivers/message/fusion/mptscsih.c b/drivers/message/fusion/mptscsih.c
index 727819c..475e3c9 100644
--- a/drivers/message/fusion/mptscsih.c
+++ b/drivers/message/fusion/mptscsih.c
@@ -1266,6 +1266,7 @@ mptscsih_resume(struct pci_dev *pdev)
 const char *
 mptscsih_info(struct Scsi_Host *SChost)
 {
+	const char *ret = "N/A";
 	MPT_SCSI_HOST *h;
 	int size = 0;
 
@@ -1279,9 +1280,10 @@ mptscsih_info(struct Scsi_Host *SChost)
 
 		mpt_print_ioc_summary(h->ioc, h->info_kbuf, &size, 0, 0);
 		h->info_kbuf[size-1] = '\0';
+		ret = h->info_kbuf;
 	}
 
-	return h->info_kbuf;
+	return ret;
 }
 
 int mptscsih_show_info(struct seq_file *m, struct Scsi_Host *host)
@@ -1370,8 +1372,7 @@ mptscsih_qcmd(struct scsi_cmnd *SCpnt, void (*done)(struct scsi_cmnd *))
 	/* Default to untagged. Once a target structure has been allocated,
 	 * use the Inquiry data to determine if device supports tagged.
 	 */
-	if (vdevice
-	    && (vdevice->vtarget->tflags & MPT_TARGET_FLAGS_Q_YES)
+	if ((vdevice->vtarget->tflags & MPT_TARGET_FLAGS_Q_YES)
 	    && (SCpnt->device->tagged_supported)) {
 		scsictl = scsidir | MPI_SCSIIO_CONTROL_SIMPLEQ;
 		if (SCpnt->request && SCpnt->request->ioprio) {
-- 
1.7.10.4

Re: [PATCH 7/7] mptfusion: tweak null pointer checks

[Christoph Hellwig]

> JL: Comments on the above warnings, in order:

Can you move these comments into the actual commit message instead of
the part that gets discarded?
> 
>     No-brainer, the enclosing switch statement dereferences 'reply',
>     so we can't get here unless 'reply' is valid.

ok.

>     "Retry target reset" needs to know the target ID and channel, so
>     enclose that entire block inside a if (pScsiTmReply) block.

Reading the code in mptsas_taskmgmt_complete it's pretty obvious
that it can't do anything useful if mr/pScsiTmReply are NULL, so I
suspect it would be best to just return at the beginning of the
function.

I'd love to understand if it actually could ever be zero, which I doubt.
Maybe the LSI people can shed some light on that?

>     The code before the loop checks for 'port_info->phy_info', but not
>     many other places in the driver bother.  Not sure what to do here.

It's pretty obvious from reading mptsas_sas_io_unit_pg0 that we never
register a port_info with a NULL phy_info in the lists, so all NULL
checks on it could be deleted.

>     'hostdata' is checked and then immediately dereferenced after
>     that block.  How about a default return string of "N/A" to indicate
>     one isn't available? 

shost_priv can't return NULL, so the if (h) should be removed.

>     Not sure what to do here.  'vdevice' is needed to "set up the
>     message frame" target ID and channel, so it should probably return
>     an error in in this case.

vdevice can't ever be NULL here, it's allocated in ->slave_alloc and
thus guaranteed to be around when ->queuecommand is called.

Re: [PATCH 0/7] mptfusion static checker fixups

[Christoph Hellwig]

Can I get another review for this series?

On Wed, Jun 04, 2014 at 12:49:42PM -0400, Joe Lawrence wrote:
> While reviewing the mpt2/mpt3 static checker fixup patchset, Christoph
> inquired about mptfusion.  None of the sparse / smatch warnings from the
> earlier patchset directly apply to fusion, but there were a few easy to
> fix warnings (compile tested only).
> 
> The patchset is ordered from the smallest/easiest change up to the last
> three, which are bit more involved and should be reviewed by LSI,
> especially the last one "mptfusion: tweak null pointer checks".  See the
> commentary in those patches after the signed-off-by line.
> 
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Cc: Sreekanth Reddy <Sreekanth.Reddy@lsi.com>
> 
> Joe Lawrence (7):
>   mptfusion: mark file-private functions as static
>   mptfusion: remove redundant kfree checks
>   mptfusion: initChainBuffers should return errno
>   mptfusion: zero kernel-space source of copy_to_user
>   mptfusion: make adapter prod_name[] a pointer
>   mptfusion: combine fw_event_work and its event_data
>   mptfusion: tweak null pointer checks
> 
>  drivers/message/fusion/mptbase.c  |   29 +++++++++----------
>  drivers/message/fusion/mptbase.h  |    2 +-
>  drivers/message/fusion/mptctl.c   |    2 +-
>  drivers/message/fusion/mptfc.c    |    3 +-
>  drivers/message/fusion/mptsas.c   |   57 +++++++++++++++++++------------------
>  drivers/message/fusion/mptsas.h   |    2 +-
>  drivers/message/fusion/mptscsih.c |    7 +++--
>  drivers/message/fusion/mptspi.c   |    5 ++--
>  8 files changed, 52 insertions(+), 55 deletions(-)
> 
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
---end quoted text---

Re: [PATCH 0/7] mptfusion static checker fixups

[Joe Lawrence]

Hi Christoph, Sreekanth,

Would it be easier to combine this set with the mpt2/mpt3 changes and
repost a V2?  There were a few changes Christoph request in each set,
so I can post those up later today.

-- Joe

On Wed, 25 Jun 2014 13:01:50 +0200
Christoph Hellwig <hch@infradead.org> wrote:

> Can I get another review for this series?
> 
> On Wed, Jun 04, 2014 at 12:49:42PM -0400, Joe Lawrence wrote:
> > While reviewing the mpt2/mpt3 static checker fixup patchset, Christoph
> > inquired about mptfusion.  None of the sparse / smatch warnings from the
> > earlier patchset directly apply to fusion, but there were a few easy to
> > fix warnings (compile tested only).
> > 
> > The patchset is ordered from the smallest/easiest change up to the last
> > three, which are bit more involved and should be reviewed by LSI,
> > especially the last one "mptfusion: tweak null pointer checks".  See the
> > commentary in those patches after the signed-off-by line.
> > 
> > Cc: Christoph Hellwig <hch@infradead.org>
> > Cc: Dan Carpenter <dan.carpenter@oracle.com>
> > Cc: Sreekanth Reddy <Sreekanth.Reddy@lsi.com>
> > 
> > Joe Lawrence (7):
> >   mptfusion: mark file-private functions as static
> >   mptfusion: remove redundant kfree checks
> >   mptfusion: initChainBuffers should return errno
> >   mptfusion: zero kernel-space source of copy_to_user
> >   mptfusion: make adapter prod_name[] a pointer
> >   mptfusion: combine fw_event_work and its event_data
> >   mptfusion: tweak null pointer checks
> > 
> >  drivers/message/fusion/mptbase.c  |   29 +++++++++----------
> >  drivers/message/fusion/mptbase.h  |    2 +-
> >  drivers/message/fusion/mptctl.c   |    2 +-
> >  drivers/message/fusion/mptfc.c    |    3 +-
> >  drivers/message/fusion/mptsas.c   |   57 +++++++++++++++++++------------------
> >  drivers/message/fusion/mptsas.h   |    2 +-
> >  drivers/message/fusion/mptscsih.c |    7 +++--
> >  drivers/message/fusion/mptspi.c   |    5 ++--
> >  8 files changed, 52 insertions(+), 55 deletions(-)
> > 
> > -- 
> > 1.7.10.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> ---end quoted text---
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/7] mptfusion static checker fixups

[Christoph Hellwig]

On Wed, Jun 25, 2014 at 10:18:11AM -0400, Joe Lawrence wrote:
> Hi Christoph, Sreekanth,
> 
> Would it be easier to combine this set with the mpt2/mpt3 changes and
> repost a V2?  There were a few changes Christoph request in each set,
> so I can post those up later today.

Fine with me.