* [PATCH v2 00/11] mptfusion/mpt2/mpt3 static checker fixups
@ 2014-06-25 21:00 Joe Lawrence
2014-06-25 21:03 ` [PATCH v2 01/11] mpt2sas: correct scsi_{target,device} hostdata allocation Joe Lawrence
` (10 more replies)
0 siblings, 11 replies; 16+ messages in thread
From: Joe Lawrence @ 2014-06-25 21:00 UTC (permalink / raw)
To: linux-scsi
Cc: Dan Carpenter, Christoph Hellwig, Sreekanth Reddy, Joe Lawrence
v2:
Combined the mptfusion and mpt{2,3}sas static checker patches,
re-ordering them by driver. Updated Reviewed-by and Acked-by tags as
well as Sreekanth's email address. Based off v3.16-rc2, compile tested.
patches dropped:
* mpt3sas: fix possible memory leak in mpt3sas_send_trigger_data_event
Christoph suggested combining this into a single allocation, so this
patch was transformed into two new patches (removing the Reviewed-by
and Acked-by tags):
mpt2sas-combine-fw_event_work-and-its-event_data.patch
mpt3sas-combine-fw_event_work-and-its-event_data.patch
* mptfusion: initChainBuffers should return errno
Christoph noted that the whole callchain uses -1 instead of errno.
Let it be.
patches modified:
* mptfusion: zero kernel-space source of copy_to_user
A static checker false-positive brought me here, Christoph suggested
using memdup_user.
* mptfusion: combine fw_event_work and its event_data
Remove the unnecessary sz variables.
* mptfusion: tweak null pointer checks
Moved commentry from myself (JL) and Christoph (HCH) into the commit
message.
In the mpt{2,3}sas-combine-fw_event_work-and-its-event_data patches, I
was wondering if the alignment attribute should be:
__attribute__ ((aligned (sizeof(unsigned long))))
instead of:
char event_data[0] __aligned(4)
Regards,
Joe Lawrence (11):
mpt2sas: correct scsi_{target,device} hostdata allocation
mpt2sas: combine fw_event_work and its event_data
mpt2sas: annotate ioc->reply_post_host_index as __iomem
mpt3sas: correct scsi_{target,device} hostdata allocation
mpt3sas: combine fw_event_work and its event_data
mptfusion: mark file-private functions as static
mptfusion: remove redundant kfree checks
mptfusion: use memdup_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 | 23 +++++------
drivers/message/fusion/mptbase.h | 2 +-
drivers/message/fusion/mptctl.c | 18 +++------
drivers/message/fusion/mptfc.c | 3 +-
drivers/message/fusion/mptsas.c | 74 ++++++++++++++++------------------
drivers/message/fusion/mptsas.h | 2 +-
drivers/message/fusion/mptscsih.c | 19 ++++-----
drivers/message/fusion/mptspi.c | 5 +--
drivers/scsi/mpt2sas/mpt2sas_base.c | 9 +++--
drivers/scsi/mpt2sas/mpt2sas_base.h | 2 +-
drivers/scsi/mpt2sas/mpt2sas_scsih.c | 58 +++++++++++++++-----------
drivers/scsi/mpt3sas/mpt3sas_scsih.c | 62 +++++++++++++++-------------
12 files changed, 137 insertions(+), 140 deletions(-)
--
1.7.10.4
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 01/11] mpt2sas: correct scsi_{target,device} hostdata allocation
2014-06-25 21:00 [PATCH v2 00/11] mptfusion/mpt2/mpt3 static checker fixups Joe Lawrence
@ 2014-06-25 21:03 ` Joe Lawrence
2014-06-25 21:03 ` [PATCH v2 02/11] mpt2sas: combine fw_event_work and its event_data Joe Lawrence
` (9 subsequent siblings)
10 siblings, 0 replies; 16+ messages in thread
From: Joe Lawrence @ 2014-06-25 21:03 UTC (permalink / raw)
To: linux-scsi
Cc: Dan Carpenter, Christoph Hellwig, Sreekanth Reddy, Joe Lawrence
In _scsih_{slave,target}_alloc, an incorrect structure type is passed
to sizeof() when allocating storage for hostdata. Luckily larger
structure types were used, so at least the wrong sizes were safe:
struct scsi_device (1784 bytes) > struct MPT2SAS_DEVICE (24 bytes)
struct scsi_target (760 bytes) > struct MPT2SAS_TARGET (40 bytes)
This fixes the following smatch warnings:
drivers/scsi/mpt2sas/mpt2sas_scsih.c:1295 _scsih_target_alloc()
warn: struct type mismatch 'MPT2SAS_TARGET vs scsi_target'
drivers/scsi/mpt2sas/mpt2sas_scsih.c:1409 _scsih_slave_alloc()
warn: struct type mismatch 'MPT2SAS_DEVICE vs scsi_device'
Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Acked-by: Sreekanth Reddy <Sreekanth.Reddy@avagotech.com>
---
drivers/scsi/mpt2sas/mpt2sas_scsih.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
index 5055f92..13e49c3 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
+++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
@@ -1292,7 +1292,8 @@ _scsih_target_alloc(struct scsi_target *starget)
unsigned long flags;
struct sas_rphy *rphy;
- sas_target_priv_data = kzalloc(sizeof(struct scsi_target), GFP_KERNEL);
+ sas_target_priv_data = kzalloc(sizeof(*sas_target_priv_data),
+ GFP_KERNEL);
if (!sas_target_priv_data)
return -ENOMEM;
@@ -1406,7 +1407,8 @@ _scsih_slave_alloc(struct scsi_device *sdev)
struct _sas_device *sas_device;
unsigned long flags;
- sas_device_priv_data = kzalloc(sizeof(struct scsi_device), GFP_KERNEL);
+ sas_device_priv_data = kzalloc(sizeof(*sas_device_priv_data),
+ GFP_KERNEL);
if (!sas_device_priv_data)
return -ENOMEM;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 02/11] mpt2sas: combine fw_event_work and its event_data
2014-06-25 21:00 [PATCH v2 00/11] mptfusion/mpt2/mpt3 static checker fixups Joe Lawrence
2014-06-25 21:03 ` [PATCH v2 01/11] mpt2sas: correct scsi_{target,device} hostdata allocation Joe Lawrence
@ 2014-06-25 21:03 ` Joe Lawrence
2014-07-01 18:39 ` Christoph Hellwig
2014-06-25 21:04 ` [PATCH v2 03/11] mpt2sas: annotate ioc->reply_post_host_index as __iomem Joe Lawrence
` (8 subsequent siblings)
10 siblings, 1 reply; 16+ messages in thread
From: Joe Lawrence @ 2014-06-25 21:03 UTC (permalink / raw)
To: linux-scsi
Cc: Dan Carpenter, Christoph Hellwig, Sreekanth Reddy, Joe Lawrence,
Christoph Hellwig
Tack the firmware reply event_data payload to the end of its
corresponding struct fw_event_work allocation. This matches the
convention in the mptfusion driver and simplifies the code.
Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Sreekanth Reddy <Sreekanth.Reddy@avagotech.com>
---
drivers/scsi/mpt2sas/mpt2sas_scsih.c | 52 ++++++++++++++++++++--------------
1 file changed, 30 insertions(+), 22 deletions(-)
diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
index 13e49c3..e7801ff 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
+++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
@@ -173,7 +173,7 @@ struct fw_event_work {
u8 VP_ID;
u8 ignore;
u16 event;
- void *event_data;
+ char event_data[0] __aligned(4);
};
/* raid transport support */
@@ -2834,7 +2834,6 @@ _scsih_fw_event_free(struct MPT2SAS_ADAPTER *ioc, struct fw_event_work
spin_lock_irqsave(&ioc->fw_event_lock, flags);
list_del(&fw_event->list);
- kfree(fw_event->event_data);
kfree(fw_event);
spin_unlock_irqrestore(&ioc->fw_event_lock, flags);
}
@@ -3520,7 +3519,8 @@ _scsih_check_topo_delete_events(struct MPT2SAS_ADAPTER *ioc,
if (fw_event->event != MPI2_EVENT_SAS_TOPOLOGY_CHANGE_LIST ||
fw_event->ignore)
continue;
- local_event_data = fw_event->event_data;
+ local_event_data = (Mpi2EventDataSasTopologyChangeList_t *)
+ fw_event->event_data;
if (local_event_data->ExpStatus ==
MPI2_EVENT_SAS_TOPO_ES_ADDED ||
local_event_data->ExpStatus ==
@@ -5504,7 +5504,9 @@ _scsih_sas_topology_change_event(struct MPT2SAS_ADAPTER *ioc,
u64 sas_address;
unsigned long flags;
u8 link_rate, prev_link_rate;
- Mpi2EventDataSasTopologyChangeList_t *event_data = fw_event->event_data;
+ Mpi2EventDataSasTopologyChangeList_t *event_data =
+ (Mpi2EventDataSasTopologyChangeList_t *)
+ fw_event->event_data;
#ifdef CONFIG_SCSI_MPT2SAS_LOGGING
if (ioc->logging_level & MPT_DEBUG_EVENT_WORK_TASK)
@@ -5699,7 +5701,8 @@ _scsih_sas_device_status_change_event(struct MPT2SAS_ADAPTER *ioc,
u64 sas_address;
unsigned long flags;
Mpi2EventDataSasDeviceStatusChange_t *event_data =
- fw_event->event_data;
+ (Mpi2EventDataSasDeviceStatusChange_t *)
+ fw_event->event_data;
#ifdef CONFIG_SCSI_MPT2SAS_LOGGING
if (ioc->logging_level & MPT_DEBUG_EVENT_WORK_TASK)
@@ -5794,6 +5797,7 @@ _scsih_sas_enclosure_dev_status_change_event(struct MPT2SAS_ADAPTER *ioc,
#ifdef CONFIG_SCSI_MPT2SAS_LOGGING
if (ioc->logging_level & MPT_DEBUG_EVENT_WORK_TASK)
_scsih_sas_enclosure_dev_status_change_event_debug(ioc,
+ (Mpi2EventDataSasEnclDevStatusChange_t *)
fw_event->event_data);
#endif
}
@@ -5818,7 +5822,9 @@ _scsih_sas_broadcast_primitive_event(struct MPT2SAS_ADAPTER *ioc,
u32 termination_count;
u32 query_count;
Mpi2SCSITaskManagementReply_t *mpi_reply;
- Mpi2EventDataSasBroadcastPrimitive_t *event_data = fw_event->event_data;
+ Mpi2EventDataSasBroadcastPrimitive_t *event_data =
+ (Mpi2EventDataSasBroadcastPrimitive_t *)
+ fw_event->event_data;
u16 ioc_status;
unsigned long flags;
int r;
@@ -5969,7 +5975,9 @@ static void
_scsih_sas_discovery_event(struct MPT2SAS_ADAPTER *ioc,
struct fw_event_work *fw_event)
{
- Mpi2EventDataSasDiscovery_t *event_data = fw_event->event_data;
+ Mpi2EventDataSasDiscovery_t *event_data =
+ (Mpi2EventDataSasDiscovery_t *)
+ fw_event->event_data;
#ifdef CONFIG_SCSI_MPT2SAS_LOGGING
if (ioc->logging_level & MPT_DEBUG_EVENT_WORK_TASK) {
@@ -6357,7 +6365,9 @@ _scsih_sas_ir_config_change_event(struct MPT2SAS_ADAPTER *ioc,
Mpi2EventIrConfigElement_t *element;
int i;
u8 foreign_config;
- Mpi2EventDataIrConfigChangeList_t *event_data = fw_event->event_data;
+ Mpi2EventDataIrConfigChangeList_t *event_data =
+ (Mpi2EventDataIrConfigChangeList_t *)
+ fw_event->event_data;
#ifdef CONFIG_SCSI_MPT2SAS_LOGGING
if ((ioc->logging_level & MPT_DEBUG_EVENT_WORK_TASK)
@@ -6425,7 +6435,9 @@ _scsih_sas_ir_volume_event(struct MPT2SAS_ADAPTER *ioc,
u16 handle;
u32 state;
int rc;
- Mpi2EventDataIrVolume_t *event_data = fw_event->event_data;
+ Mpi2EventDataIrVolume_t *event_data =
+ (Mpi2EventDataIrVolume_t *)
+ fw_event->event_data;
if (ioc->shost_recovery)
return;
@@ -6509,7 +6521,9 @@ _scsih_sas_ir_physical_disk_event(struct MPT2SAS_ADAPTER *ioc,
Mpi2ConfigReply_t mpi_reply;
Mpi2SasDevicePage0_t sas_device_pg0;
u32 ioc_status;
- Mpi2EventDataIrPhysicalDisk_t *event_data = fw_event->event_data;
+ Mpi2EventDataIrPhysicalDisk_t *event_data =
+ (Mpi2EventDataIrPhysicalDisk_t *)
+ fw_event->event_data;
u64 sas_address;
if (ioc->shost_recovery)
@@ -6632,7 +6646,9 @@ static void
_scsih_sas_ir_operation_status_event(struct MPT2SAS_ADAPTER *ioc,
struct fw_event_work *fw_event)
{
- Mpi2EventDataIrOperationStatus_t *event_data = fw_event->event_data;
+ Mpi2EventDataIrOperationStatus_t *event_data =
+ (Mpi2EventDataIrOperationStatus_t *)
+ fw_event->event_data;
static struct _raid_device *raid_device;
unsigned long flags;
u16 handle;
@@ -7592,23 +7608,15 @@ mpt2sas_scsih_event_callback(struct MPT2SAS_ADAPTER *ioc, u8 msix_index,
return;
}
- fw_event = kzalloc(sizeof(struct fw_event_work), GFP_ATOMIC);
- if (!fw_event) {
- printk(MPT2SAS_ERR_FMT "failure at %s:%d/%s()!\n",
- ioc->name, __FILE__, __LINE__, __func__);
- return;
- }
sz = le16_to_cpu(mpi_reply->EventDataLength) * 4;
- fw_event->event_data = kzalloc(sz, GFP_ATOMIC);
- if (!fw_event->event_data) {
+ fw_event = kzalloc(sizeof(*fw_event) + sz, GFP_ATOMIC);
+ if (!fw_event) {
printk(MPT2SAS_ERR_FMT "failure at %s:%d/%s()!\n",
ioc->name, __FILE__, __LINE__, __func__);
- kfree(fw_event);
return;
}
- memcpy(fw_event->event_data, mpi_reply->EventData,
- sz);
+ memcpy(fw_event->event_data, mpi_reply->EventData, sz);
fw_event->ioc = ioc;
fw_event->VF_ID = mpi_reply->VF_ID;
fw_event->VP_ID = mpi_reply->VP_ID;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 03/11] mpt2sas: annotate ioc->reply_post_host_index as __iomem
2014-06-25 21:00 [PATCH v2 00/11] mptfusion/mpt2/mpt3 static checker fixups Joe Lawrence
2014-06-25 21:03 ` [PATCH v2 01/11] mpt2sas: correct scsi_{target,device} hostdata allocation Joe Lawrence
2014-06-25 21:03 ` [PATCH v2 02/11] mpt2sas: combine fw_event_work and its event_data Joe Lawrence
@ 2014-06-25 21:04 ` Joe Lawrence
2014-06-25 21:04 ` [PATCH v2 04/11] mpt3sas: correct scsi_{target,device} hostdata allocation Joe Lawrence
` (7 subsequent siblings)
10 siblings, 0 replies; 16+ messages in thread
From: Joe Lawrence @ 2014-06-25 21:04 UTC (permalink / raw)
To: linux-scsi
Cc: Dan Carpenter, Christoph Hellwig, Sreekanth Reddy, Joe Lawrence
The MPT2SAS_ADAPTER reply_post_host_index[] holds calculated addresses
in memory mapped register space. Add an "__iomem" annotation to silence
the following sparse warnings:
drivers/scsi/mpt2sas/mpt2sas_base.c:1006:43:
warning: incorrect type in argument 2 (different address spaces)
expected void volatile [noderef] <asn:2>*addr
got unsigned long long [usertype] *<noident>
drivers/scsi/mpt2sas/mpt2sas_base.c:4299:22:
warning: cast removes address space of expression
drivers/scsi/mpt2sas/mpt2sas_base.c:4303:27:
warning: cast removes address space of expression
Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Acked-by: Sreekanth Reddy <Sreekanth.Reddy@avagotech.com>
---
drivers/scsi/mpt2sas/mpt2sas_base.c | 9 +++++----
drivers/scsi/mpt2sas/mpt2sas_base.h | 2 +-
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c b/drivers/scsi/mpt2sas/mpt2sas_base.c
index 8b88118..a31397c 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_base.c
+++ b/drivers/scsi/mpt2sas/mpt2sas_base.c
@@ -4295,12 +4295,13 @@ mpt2sas_base_attach(struct MPT2SAS_ADAPTER *ioc)
goto out_free_resources;
if (ioc->is_warpdrive) {
- ioc->reply_post_host_index[0] =
- (resource_size_t *)&ioc->chip->ReplyPostHostIndex;
+ ioc->reply_post_host_index[0] = (resource_size_t __iomem *)
+ &ioc->chip->ReplyPostHostIndex;
for (i = 1; i < ioc->cpu_msix_table_sz; i++)
- ioc->reply_post_host_index[i] = (resource_size_t *)
- ((u8 *)&ioc->chip->Doorbell + (0x4000 + ((i - 1)
+ ioc->reply_post_host_index[i] =
+ (resource_size_t __iomem *)
+ ((u8 __iomem *)&ioc->chip->Doorbell + (0x4000 + ((i - 1)
* 4)));
}
diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.h b/drivers/scsi/mpt2sas/mpt2sas_base.h
index fd3b998..0ac5815 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_base.h
+++ b/drivers/scsi/mpt2sas/mpt2sas_base.h
@@ -837,7 +837,7 @@ struct MPT2SAS_ADAPTER {
u8 msix_enable;
u16 msix_vector_count;
u8 *cpu_msix_table;
- resource_size_t **reply_post_host_index;
+ resource_size_t __iomem **reply_post_host_index;
u16 cpu_msix_table_sz;
u32 ioc_reset_count;
MPT2SAS_FLUSH_RUNNING_CMDS schedule_dead_ioc_flush_running_cmds;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 04/11] mpt3sas: correct scsi_{target,device} hostdata allocation
2014-06-25 21:00 [PATCH v2 00/11] mptfusion/mpt2/mpt3 static checker fixups Joe Lawrence
` (2 preceding siblings ...)
2014-06-25 21:04 ` [PATCH v2 03/11] mpt2sas: annotate ioc->reply_post_host_index as __iomem Joe Lawrence
@ 2014-06-25 21:04 ` Joe Lawrence
2014-06-25 21:05 ` [PATCH v2 05/11] mpt3sas: combine fw_event_work and its event_data Joe Lawrence
` (6 subsequent siblings)
10 siblings, 0 replies; 16+ messages in thread
From: Joe Lawrence @ 2014-06-25 21:04 UTC (permalink / raw)
To: linux-scsi
Cc: Dan Carpenter, Christoph Hellwig, Sreekanth Reddy, Joe Lawrence
In _scsih_{slave,target}_alloc, an incorrect structure type is passed
to sizeof() when allocating storage for hostdata. Luckily larger
structure types were used, so at least the wrong sizes were safe:
struct scsi_device (1784 bytes) > struct MPT3SAS_DEVICE (24 bytes)
struct scsi_target (760 bytes) > struct MPT3SAS_TARGET (32 bytes)
This fixes the following smatch warnings:
drivers/scsi/mpt3sas/mpt3sas_scsih.c:1166 _scsih_target_alloc()
warn: struct type mismatch 'MPT3SAS_TARGET vs scsi_target'
drivers/scsi/mpt3sas/mpt3sas_scsih.c:1280 _scsih_slave_alloc()
warn: struct type mismatch 'MPT3SAS_DEVICE vs scsi_device'
Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Acked-by: Sreekanth Reddy <Sreekanth.Reddy@avagotech.com>
---
drivers/scsi/mpt3sas/mpt3sas_scsih.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 18e713d..f3ee3b4 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -1163,7 +1163,8 @@ _scsih_target_alloc(struct scsi_target *starget)
unsigned long flags;
struct sas_rphy *rphy;
- sas_target_priv_data = kzalloc(sizeof(struct scsi_target), GFP_KERNEL);
+ sas_target_priv_data = kzalloc(sizeof(*sas_target_priv_data),
+ GFP_KERNEL);
if (!sas_target_priv_data)
return -ENOMEM;
@@ -1277,7 +1278,8 @@ _scsih_slave_alloc(struct scsi_device *sdev)
struct _sas_device *sas_device;
unsigned long flags;
- sas_device_priv_data = kzalloc(sizeof(struct scsi_device), GFP_KERNEL);
+ sas_device_priv_data = kzalloc(sizeof(*sas_device_priv_data),
+ GFP_KERNEL);
if (!sas_device_priv_data)
return -ENOMEM;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 05/11] mpt3sas: combine fw_event_work and its event_data
2014-06-25 21:00 [PATCH v2 00/11] mptfusion/mpt2/mpt3 static checker fixups Joe Lawrence
` (3 preceding siblings ...)
2014-06-25 21:04 ` [PATCH v2 04/11] mpt3sas: correct scsi_{target,device} hostdata allocation Joe Lawrence
@ 2014-06-25 21:05 ` Joe Lawrence
2014-07-01 18:40 ` Christoph Hellwig
2014-06-25 21:05 ` [PATCH v2 06/11] mptfusion: mark file-private functions as static Joe Lawrence
` (5 subsequent siblings)
10 siblings, 1 reply; 16+ messages in thread
From: Joe Lawrence @ 2014-06-25 21:05 UTC (permalink / raw)
To: linux-scsi
Cc: Dan Carpenter, Christoph Hellwig, Sreekanth Reddy, Joe Lawrence
Tack the firmware reply event_data payload to the end of its
corresponding struct fw_event_work allocation. This matches the
convention in the mptfusion driver and simplifies the code.
This avoids the following smatch warning:
drivers/scsi/mpt3sas/mpt3sas_scsih.c:2519
mpt3sas_send_trigger_data_event() warn: possible memory leak of
'fw_event'
Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Sreekanth Reddy <Sreekanth.Reddy@avagotech.com>
Cc: Christoph Hellwig <hch@lst.de>
---
drivers/scsi/mpt3sas/mpt3sas_scsih.c | 56 +++++++++++++++++++---------------
1 file changed, 31 insertions(+), 25 deletions(-)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index f3ee3b4..a14be8f 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -190,7 +190,7 @@ struct fw_event_work {
u8 VP_ID;
u8 ignore;
u16 event;
- void *event_data;
+ char event_data[0] __aligned(4);
};
/* raid transport support */
@@ -2492,7 +2492,6 @@ _scsih_fw_event_free(struct MPT3SAS_ADAPTER *ioc, struct fw_event_work
spin_lock_irqsave(&ioc->fw_event_lock, flags);
list_del(&fw_event->list);
- kfree(fw_event->event_data);
kfree(fw_event);
spin_unlock_irqrestore(&ioc->fw_event_lock, flags);
}
@@ -2513,12 +2512,10 @@ mpt3sas_send_trigger_data_event(struct MPT3SAS_ADAPTER *ioc,
if (ioc->is_driver_loading)
return;
- fw_event = kzalloc(sizeof(struct fw_event_work), GFP_ATOMIC);
+ fw_event = kzalloc(sizeof(*fw_event) + sizeof(*event_data),
+ GFP_ATOMIC);
if (!fw_event)
return;
- fw_event->event_data = kzalloc(sizeof(*event_data), GFP_ATOMIC);
- if (!fw_event->event_data)
- return;
fw_event->event = MPT3SAS_PROCESS_TRIGGER_DIAG;
fw_event->ioc = ioc;
memcpy(fw_event->event_data, event_data, sizeof(*event_data));
@@ -3213,7 +3210,8 @@ _scsih_check_topo_delete_events(struct MPT3SAS_ADAPTER *ioc,
if (fw_event->event != MPI2_EVENT_SAS_TOPOLOGY_CHANGE_LIST ||
fw_event->ignore)
continue;
- local_event_data = fw_event->event_data;
+ local_event_data = (Mpi2EventDataSasTopologyChangeList_t *)
+ fw_event->event_data;
if (local_event_data->ExpStatus ==
MPI2_EVENT_SAS_TOPO_ES_ADDED ||
local_event_data->ExpStatus ==
@@ -5045,7 +5043,9 @@ _scsih_sas_topology_change_event(struct MPT3SAS_ADAPTER *ioc,
u64 sas_address;
unsigned long flags;
u8 link_rate, prev_link_rate;
- Mpi2EventDataSasTopologyChangeList_t *event_data = fw_event->event_data;
+ Mpi2EventDataSasTopologyChangeList_t *event_data =
+ (Mpi2EventDataSasTopologyChangeList_t *)
+ fw_event->event_data;
#ifdef CONFIG_SCSI_MPT3SAS_LOGGING
if (ioc->logging_level & MPT_DEBUG_EVENT_WORK_TASK)
@@ -5243,7 +5243,8 @@ _scsih_sas_device_status_change_event(struct MPT3SAS_ADAPTER *ioc,
u64 sas_address;
unsigned long flags;
Mpi2EventDataSasDeviceStatusChange_t *event_data =
- fw_event->event_data;
+ (Mpi2EventDataSasDeviceStatusChange_t *)
+ fw_event->event_data;
#ifdef CONFIG_SCSI_MPT3SAS_LOGGING
if (ioc->logging_level & MPT_DEBUG_EVENT_WORK_TASK)
@@ -5339,6 +5340,7 @@ _scsih_sas_enclosure_dev_status_change_event(struct MPT3SAS_ADAPTER *ioc,
#ifdef CONFIG_SCSI_MPT3SAS_LOGGING
if (ioc->logging_level & MPT_DEBUG_EVENT_WORK_TASK)
_scsih_sas_enclosure_dev_status_change_event_debug(ioc,
+ (Mpi2EventDataSasEnclDevStatusChange_t *)
fw_event->event_data);
#endif
}
@@ -5363,7 +5365,9 @@ _scsih_sas_broadcast_primitive_event(struct MPT3SAS_ADAPTER *ioc,
u32 termination_count;
u32 query_count;
Mpi2SCSITaskManagementReply_t *mpi_reply;
- Mpi2EventDataSasBroadcastPrimitive_t *event_data = fw_event->event_data;
+ Mpi2EventDataSasBroadcastPrimitive_t *event_data =
+ (Mpi2EventDataSasBroadcastPrimitive_t *)
+ fw_event->event_data;
u16 ioc_status;
unsigned long flags;
int r;
@@ -5515,7 +5519,8 @@ static void
_scsih_sas_discovery_event(struct MPT3SAS_ADAPTER *ioc,
struct fw_event_work *fw_event)
{
- Mpi2EventDataSasDiscovery_t *event_data = fw_event->event_data;
+ Mpi2EventDataSasDiscovery_t *event_data =
+ (Mpi2EventDataSasDiscovery_t *) fw_event->event_data;
#ifdef CONFIG_SCSI_MPT3SAS_LOGGING
if (ioc->logging_level & MPT_DEBUG_EVENT_WORK_TASK) {
@@ -6001,7 +6006,9 @@ _scsih_sas_ir_config_change_event(struct MPT3SAS_ADAPTER *ioc,
Mpi2EventIrConfigElement_t *element;
int i;
u8 foreign_config;
- Mpi2EventDataIrConfigChangeList_t *event_data = fw_event->event_data;
+ Mpi2EventDataIrConfigChangeList_t *event_data =
+ (Mpi2EventDataIrConfigChangeList_t *)
+ fw_event->event_data;
#ifdef CONFIG_SCSI_MPT3SAS_LOGGING
if (ioc->logging_level & MPT_DEBUG_EVENT_WORK_TASK)
@@ -6071,7 +6078,8 @@ _scsih_sas_ir_volume_event(struct MPT3SAS_ADAPTER *ioc,
u16 handle;
u32 state;
int rc;
- Mpi2EventDataIrVolume_t *event_data = fw_event->event_data;
+ Mpi2EventDataIrVolume_t *event_data =
+ (Mpi2EventDataIrVolume_t *) fw_event->event_data;
if (ioc->shost_recovery)
return;
@@ -6154,7 +6162,8 @@ _scsih_sas_ir_physical_disk_event(struct MPT3SAS_ADAPTER *ioc,
Mpi2ConfigReply_t mpi_reply;
Mpi2SasDevicePage0_t sas_device_pg0;
u32 ioc_status;
- Mpi2EventDataIrPhysicalDisk_t *event_data = fw_event->event_data;
+ Mpi2EventDataIrPhysicalDisk_t *event_data =
+ (Mpi2EventDataIrPhysicalDisk_t *) fw_event->event_data;
u64 sas_address;
if (ioc->shost_recovery)
@@ -6274,7 +6283,9 @@ static void
_scsih_sas_ir_operation_status_event(struct MPT3SAS_ADAPTER *ioc,
struct fw_event_work *fw_event)
{
- Mpi2EventDataIrOperationStatus_t *event_data = fw_event->event_data;
+ Mpi2EventDataIrOperationStatus_t *event_data =
+ (Mpi2EventDataIrOperationStatus_t *)
+ fw_event->event_data;
static struct _raid_device *raid_device;
unsigned long flags;
u16 handle;
@@ -7036,7 +7047,9 @@ _mpt3sas_fw_work(struct MPT3SAS_ADAPTER *ioc, struct fw_event_work *fw_event)
switch (fw_event->event) {
case MPT3SAS_PROCESS_TRIGGER_DIAG:
- mpt3sas_process_trigger_data(ioc, fw_event->event_data);
+ mpt3sas_process_trigger_data(ioc,
+ (struct SL_WH_TRIGGERS_EVENT_DATA_T *)
+ fw_event->event_data);
break;
case MPT3SAS_REMOVE_UNRESPONDING_DEVICES:
while (scsi_host_in_recovery(ioc->shost) || ioc->shost_recovery)
@@ -7194,18 +7207,11 @@ mpt3sas_scsih_event_callback(struct MPT3SAS_ADAPTER *ioc, u8 msix_index,
return 1;
}
- fw_event = kzalloc(sizeof(struct fw_event_work), GFP_ATOMIC);
- if (!fw_event) {
- pr_err(MPT3SAS_FMT "failure at %s:%d/%s()!\n",
- ioc->name, __FILE__, __LINE__, __func__);
- return 1;
- }
sz = le16_to_cpu(mpi_reply->EventDataLength) * 4;
- fw_event->event_data = kzalloc(sz, GFP_ATOMIC);
- if (!fw_event->event_data) {
+ fw_event = kzalloc(sizeof(*fw_event) + sz, GFP_ATOMIC);
+ if (!fw_event) {
pr_err(MPT3SAS_FMT "failure at %s:%d/%s()!\n",
ioc->name, __FILE__, __LINE__, __func__);
- kfree(fw_event);
return 1;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 06/11] mptfusion: mark file-private functions as static
2014-06-25 21:00 [PATCH v2 00/11] mptfusion/mpt2/mpt3 static checker fixups Joe Lawrence
` (4 preceding siblings ...)
2014-06-25 21:05 ` [PATCH v2 05/11] mpt3sas: combine fw_event_work and its event_data Joe Lawrence
@ 2014-06-25 21:05 ` Joe Lawrence
2014-07-01 18:40 ` Christoph Hellwig
[not found] ` <CAK=zhgrLTexg0niWEEWwsvd7m9ygOq1wOpEPqSLDs8mdKGOpoA@mail.gmail.com>
2014-06-25 21:06 ` [PATCH v2 07/11] mptfusion: remove redundant kfree checks Joe Lawrence
` (4 subsequent siblings)
10 siblings, 2 replies; 16+ messages in thread
From: Joe Lawrence @ 2014-06-25 21:05 UTC (permalink / raw)
To: linux-scsi
Cc: Dan Carpenter, Christoph Hellwig, Sreekanth Reddy, 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: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Sreekanth Reddy <Sreekanth.Reddy@avagotech.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
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 ebc0af7..ea30033 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 711fcb5..54b51a9 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)
{
@@ -3648,7 +3648,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;
@@ -5321,7 +5321,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 49d1133..7b4db9a 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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 07/11] mptfusion: remove redundant kfree checks
2014-06-25 21:00 [PATCH v2 00/11] mptfusion/mpt2/mpt3 static checker fixups Joe Lawrence
` (5 preceding siblings ...)
2014-06-25 21:05 ` [PATCH v2 06/11] mptfusion: mark file-private functions as static Joe Lawrence
@ 2014-06-25 21:06 ` Joe Lawrence
2014-06-25 21:06 ` [PATCH v2 08/11] mptfusion: use memdup_user Joe Lawrence
` (3 subsequent siblings)
10 siblings, 0 replies; 16+ messages in thread
From: Joe Lawrence @ 2014-06-25 21:06 UTC (permalink / raw)
To: linux-scsi
Cc: Dan Carpenter, Christoph Hellwig, Sreekanth Reddy, 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: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Sreekanth Reddy <Sreekanth.Reddy@avagotech.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
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 02a3eef..21ac845 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 7b4db9a..787933d 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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 08/11] mptfusion: use memdup_user
2014-06-25 21:00 [PATCH v2 00/11] mptfusion/mpt2/mpt3 static checker fixups Joe Lawrence
` (6 preceding siblings ...)
2014-06-25 21:06 ` [PATCH v2 07/11] mptfusion: remove redundant kfree checks Joe Lawrence
@ 2014-06-25 21:06 ` Joe Lawrence
2014-06-25 21:06 ` [PATCH v2 09/11] mptfusion: make adapter prod_name[] a pointer Joe Lawrence
` (2 subsequent siblings)
10 siblings, 0 replies; 16+ messages in thread
From: Joe Lawrence @ 2014-06-25 21:06 UTC (permalink / raw)
To: linux-scsi
Cc: Dan Carpenter, Christoph Hellwig, Sreekanth Reddy, Joe Lawrence,
Christoph Hellwig
Let memdup_user handle the kmalloc, copy_from_user and error checking
kfree code.
Spotted by the following smatch (false positive) 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@avagotech.com>
---
drivers/message/fusion/mptctl.c | 18 +++++-------------
1 file changed, 5 insertions(+), 13 deletions(-)
diff --git a/drivers/message/fusion/mptctl.c b/drivers/message/fusion/mptctl.c
index 8a050e8..b0a892a 100644
--- a/drivers/message/fusion/mptctl.c
+++ b/drivers/message/fusion/mptctl.c
@@ -1261,19 +1261,11 @@ mptctl_getiocinfo (unsigned long arg, unsigned int data_size)
else
return -EFAULT;
- karg = kmalloc(data_size, GFP_KERNEL);
- if (karg == NULL) {
- printk(KERN_ERR MYNAM "%s::mpt_ioctl_iocinfo() @%d - no memory available!\n",
- __FILE__, __LINE__);
- return -ENOMEM;
- }
-
- if (copy_from_user(karg, uarg, data_size)) {
- printk(KERN_ERR MYNAM "%s@%d::mptctl_getiocinfo - "
- "Unable to read in mpt_ioctl_iocinfo struct @ %p\n",
- __FILE__, __LINE__, uarg);
- kfree(karg);
- return -EFAULT;
+ karg = memdup_user(uarg, data_size);
+ if (IS_ERR(karg)) {
+ printk(KERN_ERR MYNAM "%s@%d::mpt_ioctl_iocinfo() - memdup_user returned error [%ld]\n",
+ __FILE__, __LINE__, PTR_ERR(karg));
+ return PTR_ERR(karg);
}
if (((iocnum = mpt_verify_adapter(karg->hdr.iocnum, &ioc)) < 0) ||
--
1.7.10.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 09/11] mptfusion: make adapter prod_name[] a pointer
2014-06-25 21:00 [PATCH v2 00/11] mptfusion/mpt2/mpt3 static checker fixups Joe Lawrence
` (7 preceding siblings ...)
2014-06-25 21:06 ` [PATCH v2 08/11] mptfusion: use memdup_user Joe Lawrence
@ 2014-06-25 21:06 ` Joe Lawrence
2014-06-25 21:06 ` [PATCH v2 10/11] mptfusion: combine fw_event_work and its event_data Joe Lawrence
2014-06-25 21:06 ` [PATCH v2 11/11] mptfusion: tweak null pointer checks Joe Lawrence
10 siblings, 0 replies; 16+ messages in thread
From: Joe Lawrence @ 2014-06-25 21:06 UTC (permalink / raw)
To: linux-scsi
Cc: Dan Carpenter, Christoph Hellwig, Sreekanth Reddy, 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 smatch 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: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Sreekanth Reddy <Sreekanth.Reddy@avagotech.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
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 ea30033..9d4c782 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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 10/11] mptfusion: combine fw_event_work and its event_data
2014-06-25 21:00 [PATCH v2 00/11] mptfusion/mpt2/mpt3 static checker fixups Joe Lawrence
` (8 preceding siblings ...)
2014-06-25 21:06 ` [PATCH v2 09/11] mptfusion: make adapter prod_name[] a pointer Joe Lawrence
@ 2014-06-25 21:06 ` Joe Lawrence
2014-06-25 21:06 ` [PATCH v2 11/11] mptfusion: tweak null pointer checks Joe Lawrence
10 siblings, 0 replies; 16+ messages in thread
From: Joe Lawrence @ 2014-06-25 21:06 UTC (permalink / raw)
To: linux-scsi
Cc: Dan Carpenter, Christoph Hellwig, Sreekanth Reddy, 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: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Sreekanth Reddy <Sreekanth.Reddy@avagotech.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
drivers/message/fusion/mptsas.c | 16 ++++++----------
drivers/message/fusion/mptsas.h | 2 +-
2 files changed, 7 insertions(+), 11 deletions(-)
diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
index 54b51a9..5454d838 100644
--- a/drivers/message/fusion/mptsas.c
+++ b/drivers/message/fusion/mptsas.c
@@ -990,11 +990,10 @@ mptsas_queue_device_delete(MPT_ADAPTER *ioc,
MpiEventDataSasDeviceStatusChange_t *sas_event_data)
{
struct fw_event_work *fw_event;
- int sz;
- sz = offsetof(struct fw_event_work, event_data) +
- sizeof(MpiEventDataSasDeviceStatusChange_t);
- fw_event = kzalloc(sz, GFP_ATOMIC);
+ fw_event = kzalloc(sizeof(*fw_event) +
+ sizeof(MpiEventDataSasDeviceStatusChange_t),
+ GFP_ATOMIC);
if (!fw_event) {
printk(MYIOC_s_WARN_FMT "%s: failed at (line=%d)\n",
ioc->name, __func__, __LINE__);
@@ -1011,10 +1010,8 @@ static void
mptsas_queue_rescan(MPT_ADAPTER *ioc)
{
struct fw_event_work *fw_event;
- int sz;
- sz = offsetof(struct fw_event_work, event_data);
- fw_event = kzalloc(sz, GFP_ATOMIC);
+ fw_event = kzalloc(sizeof(*fw_event), GFP_ATOMIC);
if (!fw_event) {
printk(MYIOC_s_WARN_FMT "%s: failed at (line=%d)\n",
ioc->name, __func__, __LINE__);
@@ -4983,7 +4980,7 @@ static int
mptsas_event_process(MPT_ADAPTER *ioc, EventNotificationReply_t *reply)
{
u32 event = le32_to_cpu(reply->Event);
- int sz, event_data_sz;
+ int event_data_sz;
struct fw_event_work *fw_event;
unsigned long delay;
@@ -5093,8 +5090,7 @@ 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;
- fw_event = kzalloc(sz, GFP_ATOMIC);
+ fw_event = kzalloc(sizeof(*fw_event) + event_data_sz, GFP_ATOMIC);
if (!fw_event) {
printk(MYIOC_s_WARN_FMT "%s: failed at (line=%d)\n", ioc->name,
__func__, __LINE__);
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 11/11] mptfusion: tweak null pointer checks
2014-06-25 21:00 [PATCH v2 00/11] mptfusion/mpt2/mpt3 static checker fixups Joe Lawrence
` (9 preceding siblings ...)
2014-06-25 21:06 ` [PATCH v2 10/11] mptfusion: combine fw_event_work and its event_data Joe Lawrence
@ 2014-06-25 21:06 ` Joe Lawrence
10 siblings, 0 replies; 16+ messages in thread
From: Joe Lawrence @ 2014-06-25 21:06 UTC (permalink / raw)
To: linux-scsi
Cc: Dan Carpenter, Christoph Hellwig, Sreekanth Reddy, Joe Lawrence,
Christoph Hellwig
Fixes the following smatch warnings:
drivers/message/fusion/mptbase.c:652 mptbase_reply() warn: variable
dereferenced before check 'reply' (see line 639)
[JL: No-brainer, the enclosing switch statement dereferences
reply, so we can't get here unless reply is valid.]
drivers/message/fusion/mptsas.c:1255 mptsas_taskmgmt_complete() error:
we previously assumed 'pScsiTmReply' could be null (see line 1227)
[HCH: 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?]
drivers/message/fusion/mptsas.c:3888 mptsas_not_responding_devices()
error: we previously assumed 'port_info->phy_info' could be null
(see line 3875)
[HCH: 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.]
drivers/message/fusion/mptscsih.c:1284 mptscsih_info() error:
we previously assumed 'h' could be null (see line 1274)
[HCH: shost_priv can't return NULL, so the if (h) should be
removed.]
drivers/message/fusion/mptscsih.c:1388 mptscsih_qcmd() error: we
previously assumed 'vdevice' could be null (see line 1373)
[HCH: vdevice can't ever be NULL here, it's allocated in
->slave_alloc and thus guaranteed to be around when
->queuecommand is called.]
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@avagotech.com>
---
drivers/message/fusion/mptbase.c | 10 +++----
drivers/message/fusion/mptsas.c | 52 ++++++++++++++++++-------------------
drivers/message/fusion/mptscsih.c | 19 ++++++--------
3 files changed, 37 insertions(+), 44 deletions(-)
diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
index 9d4c782..a896d94 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 5454d838..e3494a6 100644
--- a/drivers/message/fusion/mptsas.c
+++ b/drivers/message/fusion/mptsas.c
@@ -1203,27 +1203,28 @@ mptsas_taskmgmt_complete(MPT_ADAPTER *ioc, MPT_FRAME_HDR *mf, MPT_FRAME_HDR *mr)
"(mf = %p, mr = %p)\n", ioc->name, mf, mr));
pScsiTmReply = (SCSITaskMgmtReply_t *)mr;
- if (pScsiTmReply) {
- dtmprintk(ioc, printk(MYIOC_s_DEBUG_FMT
- "\tTaskMgmt completed: fw_channel = %d, fw_id = %d,\n"
- "\ttask_type = 0x%02X, iocstatus = 0x%04X "
- "loginfo = 0x%08X,\n\tresponse_code = 0x%02X, "
- "term_cmnds = %d\n", ioc->name,
- pScsiTmReply->Bus, pScsiTmReply->TargetID,
- pScsiTmReply->TaskType,
- le16_to_cpu(pScsiTmReply->IOCStatus),
- le32_to_cpu(pScsiTmReply->IOCLogInfo),
- pScsiTmReply->ResponseCode,
- le32_to_cpu(pScsiTmReply->TerminationCount)));
-
- if (pScsiTmReply->ResponseCode)
- mptscsih_taskmgmt_response_code(ioc,
- pScsiTmReply->ResponseCode);
- }
-
- if (pScsiTmReply && (pScsiTmReply->TaskType ==
+ if (!pScsiTmReply)
+ return 0;
+
+ dtmprintk(ioc, printk(MYIOC_s_DEBUG_FMT
+ "\tTaskMgmt completed: fw_channel = %d, fw_id = %d,\n"
+ "\ttask_type = 0x%02X, iocstatus = 0x%04X "
+ "loginfo = 0x%08X,\n\tresponse_code = 0x%02X, "
+ "term_cmnds = %d\n", ioc->name,
+ pScsiTmReply->Bus, pScsiTmReply->TargetID,
+ pScsiTmReply->TaskType,
+ le16_to_cpu(pScsiTmReply->IOCStatus),
+ le32_to_cpu(pScsiTmReply->IOCLogInfo),
+ pScsiTmReply->ResponseCode,
+ le32_to_cpu(pScsiTmReply->TerminationCount)));
+
+ if (pScsiTmReply->ResponseCode)
+ mptscsih_taskmgmt_response_code(ioc,
+ pScsiTmReply->ResponseCode);
+
+ if (pScsiTmReply->TaskType ==
MPI_SCSITASKMGMT_TASKTYPE_QUERY_TASK || pScsiTmReply->TaskType ==
- MPI_SCSITASKMGMT_TASKTYPE_ABRT_TASK_SET)) {
+ MPI_SCSITASKMGMT_TASKTYPE_ABRT_TASK_SET) {
ioc->taskmgmt_cmds.status |= MPT_MGMT_STATUS_COMMAND_GOOD;
ioc->taskmgmt_cmds.status |= MPT_MGMT_STATUS_RF_VALID;
memcpy(ioc->taskmgmt_cmds.reply, mr,
@@ -3853,10 +3854,8 @@ retry_page:
phy_info = mptsas_find_phyinfo_by_sas_address(ioc,
sas_info->sas_address);
- if (phy_info) {
- mptsas_del_end_device(ioc, phy_info);
- goto redo_device_scan;
- }
+ mptsas_del_end_device(ioc, phy_info);
+ goto redo_device_scan;
} else
mptsas_volume_delete(ioc, sas_info->fw.id);
}
@@ -3867,9 +3866,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 2a1c6f2..6b36c7e 100644
--- a/drivers/message/fusion/mptscsih.c
+++ b/drivers/message/fusion/mptscsih.c
@@ -1271,15 +1271,13 @@ mptscsih_info(struct Scsi_Host *SChost)
h = shost_priv(SChost);
- if (h) {
- if (h->info_kbuf == NULL)
- if ((h->info_kbuf = kmalloc(0x1000 /* 4Kb */, GFP_KERNEL)) == NULL)
- return h->info_kbuf;
- h->info_kbuf[0] = '\0';
-
- mpt_print_ioc_summary(h->ioc, h->info_kbuf, &size, 0, 0);
- h->info_kbuf[size-1] = '\0';
- }
+ if (h->info_kbuf == NULL)
+ if ((h->info_kbuf = kmalloc(0x1000 /* 4Kb */, GFP_KERNEL)) == NULL)
+ return h->info_kbuf;
+ h->info_kbuf[0] = '\0';
+
+ mpt_print_ioc_summary(h->ioc, h->info_kbuf, &size, 0, 0);
+ h->info_kbuf[size-1] = '\0';
return h->info_kbuf;
}
@@ -1368,8 +1366,7 @@ mptscsih_qcmd(struct scsi_cmnd *SCpnt)
/* 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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 02/11] mpt2sas: combine fw_event_work and its event_data
2014-06-25 21:03 ` [PATCH v2 02/11] mpt2sas: combine fw_event_work and its event_data Joe Lawrence
@ 2014-07-01 18:39 ` Christoph Hellwig
0 siblings, 0 replies; 16+ messages in thread
From: Christoph Hellwig @ 2014-07-01 18:39 UTC (permalink / raw)
To: Joe Lawrence
Cc: linux-scsi, Dan Carpenter, Christoph Hellwig, Sreekanth Reddy,
Christoph Hellwig
Sreekanth,
can you give me a review for this one?
On Wed, Jun 25, 2014 at 05:03:33PM -0400, Joe Lawrence wrote:
> Tack the firmware reply event_data payload to the end of its
> corresponding struct fw_event_work allocation. This matches the
> convention in the mptfusion driver and simplifies the code.
>
> Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Sreekanth Reddy <Sreekanth.Reddy@avagotech.com>
> ---
> drivers/scsi/mpt2sas/mpt2sas_scsih.c | 52 ++++++++++++++++++++--------------
> 1 file changed, 30 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> index 13e49c3..e7801ff 100644
> --- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> +++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> @@ -173,7 +173,7 @@ struct fw_event_work {
> u8 VP_ID;
> u8 ignore;
> u16 event;
> - void *event_data;
> + char event_data[0] __aligned(4);
> };
>
> /* raid transport support */
> @@ -2834,7 +2834,6 @@ _scsih_fw_event_free(struct MPT2SAS_ADAPTER *ioc, struct fw_event_work
>
> spin_lock_irqsave(&ioc->fw_event_lock, flags);
> list_del(&fw_event->list);
> - kfree(fw_event->event_data);
> kfree(fw_event);
> spin_unlock_irqrestore(&ioc->fw_event_lock, flags);
> }
> @@ -3520,7 +3519,8 @@ _scsih_check_topo_delete_events(struct MPT2SAS_ADAPTER *ioc,
> if (fw_event->event != MPI2_EVENT_SAS_TOPOLOGY_CHANGE_LIST ||
> fw_event->ignore)
> continue;
> - local_event_data = fw_event->event_data;
> + local_event_data = (Mpi2EventDataSasTopologyChangeList_t *)
> + fw_event->event_data;
> if (local_event_data->ExpStatus ==
> MPI2_EVENT_SAS_TOPO_ES_ADDED ||
> local_event_data->ExpStatus ==
> @@ -5504,7 +5504,9 @@ _scsih_sas_topology_change_event(struct MPT2SAS_ADAPTER *ioc,
> u64 sas_address;
> unsigned long flags;
> u8 link_rate, prev_link_rate;
> - Mpi2EventDataSasTopologyChangeList_t *event_data = fw_event->event_data;
> + Mpi2EventDataSasTopologyChangeList_t *event_data =
> + (Mpi2EventDataSasTopologyChangeList_t *)
> + fw_event->event_data;
>
> #ifdef CONFIG_SCSI_MPT2SAS_LOGGING
> if (ioc->logging_level & MPT_DEBUG_EVENT_WORK_TASK)
> @@ -5699,7 +5701,8 @@ _scsih_sas_device_status_change_event(struct MPT2SAS_ADAPTER *ioc,
> u64 sas_address;
> unsigned long flags;
> Mpi2EventDataSasDeviceStatusChange_t *event_data =
> - fw_event->event_data;
> + (Mpi2EventDataSasDeviceStatusChange_t *)
> + fw_event->event_data;
>
> #ifdef CONFIG_SCSI_MPT2SAS_LOGGING
> if (ioc->logging_level & MPT_DEBUG_EVENT_WORK_TASK)
> @@ -5794,6 +5797,7 @@ _scsih_sas_enclosure_dev_status_change_event(struct MPT2SAS_ADAPTER *ioc,
> #ifdef CONFIG_SCSI_MPT2SAS_LOGGING
> if (ioc->logging_level & MPT_DEBUG_EVENT_WORK_TASK)
> _scsih_sas_enclosure_dev_status_change_event_debug(ioc,
> + (Mpi2EventDataSasEnclDevStatusChange_t *)
> fw_event->event_data);
> #endif
> }
> @@ -5818,7 +5822,9 @@ _scsih_sas_broadcast_primitive_event(struct MPT2SAS_ADAPTER *ioc,
> u32 termination_count;
> u32 query_count;
> Mpi2SCSITaskManagementReply_t *mpi_reply;
> - Mpi2EventDataSasBroadcastPrimitive_t *event_data = fw_event->event_data;
> + Mpi2EventDataSasBroadcastPrimitive_t *event_data =
> + (Mpi2EventDataSasBroadcastPrimitive_t *)
> + fw_event->event_data;
> u16 ioc_status;
> unsigned long flags;
> int r;
> @@ -5969,7 +5975,9 @@ static void
> _scsih_sas_discovery_event(struct MPT2SAS_ADAPTER *ioc,
> struct fw_event_work *fw_event)
> {
> - Mpi2EventDataSasDiscovery_t *event_data = fw_event->event_data;
> + Mpi2EventDataSasDiscovery_t *event_data =
> + (Mpi2EventDataSasDiscovery_t *)
> + fw_event->event_data;
>
> #ifdef CONFIG_SCSI_MPT2SAS_LOGGING
> if (ioc->logging_level & MPT_DEBUG_EVENT_WORK_TASK) {
> @@ -6357,7 +6365,9 @@ _scsih_sas_ir_config_change_event(struct MPT2SAS_ADAPTER *ioc,
> Mpi2EventIrConfigElement_t *element;
> int i;
> u8 foreign_config;
> - Mpi2EventDataIrConfigChangeList_t *event_data = fw_event->event_data;
> + Mpi2EventDataIrConfigChangeList_t *event_data =
> + (Mpi2EventDataIrConfigChangeList_t *)
> + fw_event->event_data;
>
> #ifdef CONFIG_SCSI_MPT2SAS_LOGGING
> if ((ioc->logging_level & MPT_DEBUG_EVENT_WORK_TASK)
> @@ -6425,7 +6435,9 @@ _scsih_sas_ir_volume_event(struct MPT2SAS_ADAPTER *ioc,
> u16 handle;
> u32 state;
> int rc;
> - Mpi2EventDataIrVolume_t *event_data = fw_event->event_data;
> + Mpi2EventDataIrVolume_t *event_data =
> + (Mpi2EventDataIrVolume_t *)
> + fw_event->event_data;
>
> if (ioc->shost_recovery)
> return;
> @@ -6509,7 +6521,9 @@ _scsih_sas_ir_physical_disk_event(struct MPT2SAS_ADAPTER *ioc,
> Mpi2ConfigReply_t mpi_reply;
> Mpi2SasDevicePage0_t sas_device_pg0;
> u32 ioc_status;
> - Mpi2EventDataIrPhysicalDisk_t *event_data = fw_event->event_data;
> + Mpi2EventDataIrPhysicalDisk_t *event_data =
> + (Mpi2EventDataIrPhysicalDisk_t *)
> + fw_event->event_data;
> u64 sas_address;
>
> if (ioc->shost_recovery)
> @@ -6632,7 +6646,9 @@ static void
> _scsih_sas_ir_operation_status_event(struct MPT2SAS_ADAPTER *ioc,
> struct fw_event_work *fw_event)
> {
> - Mpi2EventDataIrOperationStatus_t *event_data = fw_event->event_data;
> + Mpi2EventDataIrOperationStatus_t *event_data =
> + (Mpi2EventDataIrOperationStatus_t *)
> + fw_event->event_data;
> static struct _raid_device *raid_device;
> unsigned long flags;
> u16 handle;
> @@ -7592,23 +7608,15 @@ mpt2sas_scsih_event_callback(struct MPT2SAS_ADAPTER *ioc, u8 msix_index,
> return;
> }
>
> - fw_event = kzalloc(sizeof(struct fw_event_work), GFP_ATOMIC);
> - if (!fw_event) {
> - printk(MPT2SAS_ERR_FMT "failure at %s:%d/%s()!\n",
> - ioc->name, __FILE__, __LINE__, __func__);
> - return;
> - }
> sz = le16_to_cpu(mpi_reply->EventDataLength) * 4;
> - fw_event->event_data = kzalloc(sz, GFP_ATOMIC);
> - if (!fw_event->event_data) {
> + fw_event = kzalloc(sizeof(*fw_event) + sz, GFP_ATOMIC);
> + if (!fw_event) {
> printk(MPT2SAS_ERR_FMT "failure at %s:%d/%s()!\n",
> ioc->name, __FILE__, __LINE__, __func__);
> - kfree(fw_event);
> return;
> }
>
> - memcpy(fw_event->event_data, mpi_reply->EventData,
> - sz);
> + memcpy(fw_event->event_data, mpi_reply->EventData, sz);
> fw_event->ioc = ioc;
> fw_event->VF_ID = mpi_reply->VF_ID;
> fw_event->VP_ID = mpi_reply->VP_ID;
> --
> 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---
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 05/11] mpt3sas: combine fw_event_work and its event_data
2014-06-25 21:05 ` [PATCH v2 05/11] mpt3sas: combine fw_event_work and its event_data Joe Lawrence
@ 2014-07-01 18:40 ` Christoph Hellwig
0 siblings, 0 replies; 16+ messages in thread
From: Christoph Hellwig @ 2014-07-01 18:40 UTC (permalink / raw)
To: Joe Lawrence
Cc: linux-scsi, Dan Carpenter, Christoph Hellwig, Sreekanth Reddy
Sreekanth, can you give me a review for this patch? Thanks!
On Wed, Jun 25, 2014 at 05:05:34PM -0400, Joe Lawrence wrote:
> Tack the firmware reply event_data payload to the end of its
> corresponding struct fw_event_work allocation. This matches the
> convention in the mptfusion driver and simplifies the code.
>
> This avoids the following smatch warning:
>
> drivers/scsi/mpt3sas/mpt3sas_scsih.c:2519
> mpt3sas_send_trigger_data_event() warn: possible memory leak of
> 'fw_event'
>
> Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Cc: Sreekanth Reddy <Sreekanth.Reddy@avagotech.com>
> Cc: Christoph Hellwig <hch@lst.de>
> ---
> drivers/scsi/mpt3sas/mpt3sas_scsih.c | 56 +++++++++++++++++++---------------
> 1 file changed, 31 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> index f3ee3b4..a14be8f 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> @@ -190,7 +190,7 @@ struct fw_event_work {
> u8 VP_ID;
> u8 ignore;
> u16 event;
> - void *event_data;
> + char event_data[0] __aligned(4);
> };
>
> /* raid transport support */
> @@ -2492,7 +2492,6 @@ _scsih_fw_event_free(struct MPT3SAS_ADAPTER *ioc, struct fw_event_work
>
> spin_lock_irqsave(&ioc->fw_event_lock, flags);
> list_del(&fw_event->list);
> - kfree(fw_event->event_data);
> kfree(fw_event);
> spin_unlock_irqrestore(&ioc->fw_event_lock, flags);
> }
> @@ -2513,12 +2512,10 @@ mpt3sas_send_trigger_data_event(struct MPT3SAS_ADAPTER *ioc,
>
> if (ioc->is_driver_loading)
> return;
> - fw_event = kzalloc(sizeof(struct fw_event_work), GFP_ATOMIC);
> + fw_event = kzalloc(sizeof(*fw_event) + sizeof(*event_data),
> + GFP_ATOMIC);
> if (!fw_event)
> return;
> - fw_event->event_data = kzalloc(sizeof(*event_data), GFP_ATOMIC);
> - if (!fw_event->event_data)
> - return;
> fw_event->event = MPT3SAS_PROCESS_TRIGGER_DIAG;
> fw_event->ioc = ioc;
> memcpy(fw_event->event_data, event_data, sizeof(*event_data));
> @@ -3213,7 +3210,8 @@ _scsih_check_topo_delete_events(struct MPT3SAS_ADAPTER *ioc,
> if (fw_event->event != MPI2_EVENT_SAS_TOPOLOGY_CHANGE_LIST ||
> fw_event->ignore)
> continue;
> - local_event_data = fw_event->event_data;
> + local_event_data = (Mpi2EventDataSasTopologyChangeList_t *)
> + fw_event->event_data;
> if (local_event_data->ExpStatus ==
> MPI2_EVENT_SAS_TOPO_ES_ADDED ||
> local_event_data->ExpStatus ==
> @@ -5045,7 +5043,9 @@ _scsih_sas_topology_change_event(struct MPT3SAS_ADAPTER *ioc,
> u64 sas_address;
> unsigned long flags;
> u8 link_rate, prev_link_rate;
> - Mpi2EventDataSasTopologyChangeList_t *event_data = fw_event->event_data;
> + Mpi2EventDataSasTopologyChangeList_t *event_data =
> + (Mpi2EventDataSasTopologyChangeList_t *)
> + fw_event->event_data;
>
> #ifdef CONFIG_SCSI_MPT3SAS_LOGGING
> if (ioc->logging_level & MPT_DEBUG_EVENT_WORK_TASK)
> @@ -5243,7 +5243,8 @@ _scsih_sas_device_status_change_event(struct MPT3SAS_ADAPTER *ioc,
> u64 sas_address;
> unsigned long flags;
> Mpi2EventDataSasDeviceStatusChange_t *event_data =
> - fw_event->event_data;
> + (Mpi2EventDataSasDeviceStatusChange_t *)
> + fw_event->event_data;
>
> #ifdef CONFIG_SCSI_MPT3SAS_LOGGING
> if (ioc->logging_level & MPT_DEBUG_EVENT_WORK_TASK)
> @@ -5339,6 +5340,7 @@ _scsih_sas_enclosure_dev_status_change_event(struct MPT3SAS_ADAPTER *ioc,
> #ifdef CONFIG_SCSI_MPT3SAS_LOGGING
> if (ioc->logging_level & MPT_DEBUG_EVENT_WORK_TASK)
> _scsih_sas_enclosure_dev_status_change_event_debug(ioc,
> + (Mpi2EventDataSasEnclDevStatusChange_t *)
> fw_event->event_data);
> #endif
> }
> @@ -5363,7 +5365,9 @@ _scsih_sas_broadcast_primitive_event(struct MPT3SAS_ADAPTER *ioc,
> u32 termination_count;
> u32 query_count;
> Mpi2SCSITaskManagementReply_t *mpi_reply;
> - Mpi2EventDataSasBroadcastPrimitive_t *event_data = fw_event->event_data;
> + Mpi2EventDataSasBroadcastPrimitive_t *event_data =
> + (Mpi2EventDataSasBroadcastPrimitive_t *)
> + fw_event->event_data;
> u16 ioc_status;
> unsigned long flags;
> int r;
> @@ -5515,7 +5519,8 @@ static void
> _scsih_sas_discovery_event(struct MPT3SAS_ADAPTER *ioc,
> struct fw_event_work *fw_event)
> {
> - Mpi2EventDataSasDiscovery_t *event_data = fw_event->event_data;
> + Mpi2EventDataSasDiscovery_t *event_data =
> + (Mpi2EventDataSasDiscovery_t *) fw_event->event_data;
>
> #ifdef CONFIG_SCSI_MPT3SAS_LOGGING
> if (ioc->logging_level & MPT_DEBUG_EVENT_WORK_TASK) {
> @@ -6001,7 +6006,9 @@ _scsih_sas_ir_config_change_event(struct MPT3SAS_ADAPTER *ioc,
> Mpi2EventIrConfigElement_t *element;
> int i;
> u8 foreign_config;
> - Mpi2EventDataIrConfigChangeList_t *event_data = fw_event->event_data;
> + Mpi2EventDataIrConfigChangeList_t *event_data =
> + (Mpi2EventDataIrConfigChangeList_t *)
> + fw_event->event_data;
>
> #ifdef CONFIG_SCSI_MPT3SAS_LOGGING
> if (ioc->logging_level & MPT_DEBUG_EVENT_WORK_TASK)
> @@ -6071,7 +6078,8 @@ _scsih_sas_ir_volume_event(struct MPT3SAS_ADAPTER *ioc,
> u16 handle;
> u32 state;
> int rc;
> - Mpi2EventDataIrVolume_t *event_data = fw_event->event_data;
> + Mpi2EventDataIrVolume_t *event_data =
> + (Mpi2EventDataIrVolume_t *) fw_event->event_data;
>
> if (ioc->shost_recovery)
> return;
> @@ -6154,7 +6162,8 @@ _scsih_sas_ir_physical_disk_event(struct MPT3SAS_ADAPTER *ioc,
> Mpi2ConfigReply_t mpi_reply;
> Mpi2SasDevicePage0_t sas_device_pg0;
> u32 ioc_status;
> - Mpi2EventDataIrPhysicalDisk_t *event_data = fw_event->event_data;
> + Mpi2EventDataIrPhysicalDisk_t *event_data =
> + (Mpi2EventDataIrPhysicalDisk_t *) fw_event->event_data;
> u64 sas_address;
>
> if (ioc->shost_recovery)
> @@ -6274,7 +6283,9 @@ static void
> _scsih_sas_ir_operation_status_event(struct MPT3SAS_ADAPTER *ioc,
> struct fw_event_work *fw_event)
> {
> - Mpi2EventDataIrOperationStatus_t *event_data = fw_event->event_data;
> + Mpi2EventDataIrOperationStatus_t *event_data =
> + (Mpi2EventDataIrOperationStatus_t *)
> + fw_event->event_data;
> static struct _raid_device *raid_device;
> unsigned long flags;
> u16 handle;
> @@ -7036,7 +7047,9 @@ _mpt3sas_fw_work(struct MPT3SAS_ADAPTER *ioc, struct fw_event_work *fw_event)
>
> switch (fw_event->event) {
> case MPT3SAS_PROCESS_TRIGGER_DIAG:
> - mpt3sas_process_trigger_data(ioc, fw_event->event_data);
> + mpt3sas_process_trigger_data(ioc,
> + (struct SL_WH_TRIGGERS_EVENT_DATA_T *)
> + fw_event->event_data);
> break;
> case MPT3SAS_REMOVE_UNRESPONDING_DEVICES:
> while (scsi_host_in_recovery(ioc->shost) || ioc->shost_recovery)
> @@ -7194,18 +7207,11 @@ mpt3sas_scsih_event_callback(struct MPT3SAS_ADAPTER *ioc, u8 msix_index,
> return 1;
> }
>
> - fw_event = kzalloc(sizeof(struct fw_event_work), GFP_ATOMIC);
> - if (!fw_event) {
> - pr_err(MPT3SAS_FMT "failure at %s:%d/%s()!\n",
> - ioc->name, __FILE__, __LINE__, __func__);
> - return 1;
> - }
> sz = le16_to_cpu(mpi_reply->EventDataLength) * 4;
> - fw_event->event_data = kzalloc(sz, GFP_ATOMIC);
> - if (!fw_event->event_data) {
> + fw_event = kzalloc(sizeof(*fw_event) + sz, GFP_ATOMIC);
> + if (!fw_event) {
> pr_err(MPT3SAS_FMT "failure at %s:%d/%s()!\n",
> ioc->name, __FILE__, __LINE__, __func__);
> - kfree(fw_event);
> return 1;
> }
>
> --
> 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---
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 06/11] mptfusion: mark file-private functions as static
2014-06-25 21:05 ` [PATCH v2 06/11] mptfusion: mark file-private functions as static Joe Lawrence
@ 2014-07-01 18:40 ` Christoph Hellwig
[not found] ` <CAK=zhgrLTexg0niWEEWwsvd7m9ygOq1wOpEPqSLDs8mdKGOpoA@mail.gmail.com>
1 sibling, 0 replies; 16+ messages in thread
From: Christoph Hellwig @ 2014-07-01 18:40 UTC (permalink / raw)
To: Joe Lawrence
Cc: linux-scsi, Dan Carpenter, Christoph Hellwig, Sreekanth Reddy
Sreekanth, can you give me a review for this (and anything else in the series
to avoid repeating myself too often)
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 06/11] mptfusion: mark file-private functions as static
[not found] ` <CAK=zhgrLTexg0niWEEWwsvd7m9ygOq1wOpEPqSLDs8mdKGOpoA@mail.gmail.com>
@ 2014-07-02 14:05 ` Christoph Hellwig
0 siblings, 0 replies; 16+ messages in thread
From: Christoph Hellwig @ 2014-07-02 14:05 UTC (permalink / raw)
To: Sreekanth Reddy
Cc: Joe Lawrence, linux-scsi, Dan Carpenter, Christoph Hellwig,
Nagalakshmi Nandigama
Hi Sreekanth,
should I treat your replies as an Reviewd-by: or Acked-by: tag?
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2014-07-02 14:05 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-25 21:00 [PATCH v2 00/11] mptfusion/mpt2/mpt3 static checker fixups Joe Lawrence
2014-06-25 21:03 ` [PATCH v2 01/11] mpt2sas: correct scsi_{target,device} hostdata allocation Joe Lawrence
2014-06-25 21:03 ` [PATCH v2 02/11] mpt2sas: combine fw_event_work and its event_data Joe Lawrence
2014-07-01 18:39 ` Christoph Hellwig
2014-06-25 21:04 ` [PATCH v2 03/11] mpt2sas: annotate ioc->reply_post_host_index as __iomem Joe Lawrence
2014-06-25 21:04 ` [PATCH v2 04/11] mpt3sas: correct scsi_{target,device} hostdata allocation Joe Lawrence
2014-06-25 21:05 ` [PATCH v2 05/11] mpt3sas: combine fw_event_work and its event_data Joe Lawrence
2014-07-01 18:40 ` Christoph Hellwig
2014-06-25 21:05 ` [PATCH v2 06/11] mptfusion: mark file-private functions as static Joe Lawrence
2014-07-01 18:40 ` Christoph Hellwig
[not found] ` <CAK=zhgrLTexg0niWEEWwsvd7m9ygOq1wOpEPqSLDs8mdKGOpoA@mail.gmail.com>
2014-07-02 14:05 ` Christoph Hellwig
2014-06-25 21:06 ` [PATCH v2 07/11] mptfusion: remove redundant kfree checks Joe Lawrence
2014-06-25 21:06 ` [PATCH v2 08/11] mptfusion: use memdup_user Joe Lawrence
2014-06-25 21:06 ` [PATCH v2 09/11] mptfusion: make adapter prod_name[] a pointer Joe Lawrence
2014-06-25 21:06 ` [PATCH v2 10/11] mptfusion: combine fw_event_work and its event_data Joe Lawrence
2014-06-25 21:06 ` [PATCH v2 11/11] mptfusion: tweak null pointer checks Joe Lawrence
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.