From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S938480AbcJXOri (ORCPT ); Mon, 24 Oct 2016 10:47:38 -0400 Received: from mx2.suse.de ([195.135.220.15]:56482 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934973AbcJXOrg (ORCPT ); Mon, 24 Oct 2016 10:47:36 -0400 Subject: Re: [PATCH 03/10] mpt3sas: Implement device_remove_in_progress check in IOCTL path To: Suganath Prabu S , JBottomley@Parallels.com, jejb@kernel.org, hch@infradead.org References: <1476966018-10457-1-git-send-email-suganath-prabu.subramani@broadcom.com> <1476966018-10457-4-git-send-email-suganath-prabu.subramani@broadcom.com> Cc: martin.petersen@oracle.com, linux-scsi@vger.kernel.org, Sathya.Prakash@broadcom.com, kashyap.desai@broadcom.com, krishnaraddi.mankani@broadcom.com, linux-kernel@vger.kernel.org, chaitra.basappa@broadcom.com, sreekanth.reddy@broadcom.com From: Hannes Reinecke Message-ID: Date: Mon, 24 Oct 2016 16:47:32 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1476966018-10457-4-git-send-email-suganath-prabu.subramani@broadcom.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 10/20/2016 02:20 PM, Suganath Prabu S wrote: > When device missing event arrives, device_remove_in_progress bit will be > set and hence driver has to stop sending IOCTL commands.Now the check has > been added in IOCTL path to test device_remove_in_progress bit is set, if > so then IOCTL will be failed printing failure message. > > Signed-off-by: Chaitra P B > Signed-off-by: Sathya Prakash > Signed-off-by: Suganath Prabu S > --- > drivers/scsi/mpt3sas/mpt3sas_base.c | 19 +++++++++++++++ > drivers/scsi/mpt3sas/mpt3sas_base.h | 5 ++++ > drivers/scsi/mpt3sas/mpt3sas_ctl.c | 46 ++++++++++++++++++++++++++++++------ > drivers/scsi/mpt3sas/mpt3sas_scsih.c | 24 ++++++++++++++++++- > 4 files changed, 86 insertions(+), 8 deletions(-) > > diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c > index 4ea81e1..9ad7f7c 100644 > --- a/drivers/scsi/mpt3sas/mpt3sas_base.c > +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c > @@ -5334,6 +5334,21 @@ mpt3sas_base_attach(struct MPT3SAS_ADAPTER *ioc) > goto out_free_resources; > } > > + /* allocate memory for pending OS device add list */ > + ioc->pend_os_device_add_sz = (ioc->facts.MaxDevHandle / 8); > + if (ioc->facts.MaxDevHandle % 8) > + ioc->pend_os_device_add_sz++; > + ioc->pend_os_device_add = kzalloc(ioc->pend_os_device_add_sz, > + GFP_KERNEL); > + if (!ioc->pend_os_device_add) > + goto out_free_resources; > + > + ioc->device_remove_in_progress_sz = ioc->pend_os_device_add_sz; > + ioc->device_remove_in_progress = > + kzalloc(ioc->device_remove_in_progress_sz, GFP_KERNEL); > + if (!ioc->device_remove_in_progress) > + goto out_free_resources; > + > ioc->fwfault_debug = mpt3sas_fwfault_debug; > > /* base internal command bits */ > @@ -5416,6 +5431,8 @@ mpt3sas_base_attach(struct MPT3SAS_ADAPTER *ioc) > kfree(ioc->reply_post_host_index); > kfree(ioc->pd_handles); > kfree(ioc->blocking_handles); > + kfree(ioc->device_remove_in_progress); > + kfree(ioc->pend_os_device_add); > kfree(ioc->tm_cmds.reply); > kfree(ioc->transport_cmds.reply); > kfree(ioc->scsih_cmds.reply); > @@ -5457,6 +5474,8 @@ mpt3sas_base_detach(struct MPT3SAS_ADAPTER *ioc) > kfree(ioc->reply_post_host_index); > kfree(ioc->pd_handles); > kfree(ioc->blocking_handles); > + kfree(ioc->device_remove_in_progress); > + kfree(ioc->pend_os_device_add); > kfree(ioc->pfacts); > kfree(ioc->ctl_cmds.reply); > kfree(ioc->ctl_cmds.sense); > diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h b/drivers/scsi/mpt3sas/mpt3sas_base.h > index 3e71bc1..4221a4d 100644 > --- a/drivers/scsi/mpt3sas/mpt3sas_base.h > +++ b/drivers/scsi/mpt3sas/mpt3sas_base.h > @@ -1079,6 +1079,9 @@ struct MPT3SAS_ADAPTER { > void *pd_handles; > u16 pd_handles_sz; > > + void *pend_os_device_add; > + u16 pend_os_device_add_sz; > + > /* config page */ > u16 config_page_sz; > void *config_page; > @@ -1187,6 +1190,8 @@ struct MPT3SAS_ADAPTER { > struct SL_WH_EVENT_TRIGGERS_T diag_trigger_event; > struct SL_WH_SCSI_TRIGGERS_T diag_trigger_scsi; > struct SL_WH_MPI_TRIGGERS_T diag_trigger_mpi; > + void *device_remove_in_progress; > + u16 device_remove_in_progress_sz; > }; > > typedef u8 (*MPT_CALLBACK)(struct MPT3SAS_ADAPTER *ioc, u16 smid, u8 msix_index, > diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c > index 26cdc12..f204ce1 100644 > --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c > +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c > @@ -654,6 +654,7 @@ _ctl_do_mpt_command(struct MPT3SAS_ADAPTER *ioc, struct mpt3_ioctl_command karg, > size_t data_in_sz = 0; > long ret; > u16 wait_state_count; > + u16 device_handle = MPT3SAS_INVALID_DEVICE_HANDLE; > > issue_reset = 0; > > @@ -738,10 +739,13 @@ _ctl_do_mpt_command(struct MPT3SAS_ADAPTER *ioc, struct mpt3_ioctl_command karg, > data_in_sz = karg.data_in_size; > > if (mpi_request->Function == MPI2_FUNCTION_SCSI_IO_REQUEST || > - mpi_request->Function == MPI2_FUNCTION_RAID_SCSI_IO_PASSTHROUGH) { > - if (!le16_to_cpu(mpi_request->FunctionDependent1) || > - le16_to_cpu(mpi_request->FunctionDependent1) > > - ioc->facts.MaxDevHandle) { > + mpi_request->Function == MPI2_FUNCTION_RAID_SCSI_IO_PASSTHROUGH || > + mpi_request->Function == MPI2_FUNCTION_SCSI_TASK_MGMT || > + mpi_request->Function == MPI2_FUNCTION_SATA_PASSTHROUGH) { > + > + device_handle = le16_to_cpu(mpi_request->FunctionDependent1); > + if (!device_handle || (device_handle > > + ioc->facts.MaxDevHandle)) { > ret = -EINVAL; > mpt3sas_base_free_smid(ioc, smid); > goto out; > @@ -799,10 +803,16 @@ _ctl_do_mpt_command(struct MPT3SAS_ADAPTER *ioc, struct mpt3_ioctl_command karg, > memset(ioc->ctl_cmds.sense, 0, SCSI_SENSE_BUFFERSIZE); > ioc->build_sg(ioc, psge, data_out_dma, data_out_sz, > data_in_dma, data_in_sz); > - > + if (test_bit(device_handle, ioc->device_remove_in_progress)) { > + dtmprintk(ioc, pr_info(MPT3SAS_FMT > + "handle(0x%04x) :ioctl failed due to device removal in progress\n", > + ioc->name, device_handle)); > + mpt3sas_base_free_smid(ioc, smid); > + ret = -EINVAL; > + goto out; > + } > if (mpi_request->Function == MPI2_FUNCTION_SCSI_IO_REQUEST) > - mpt3sas_base_put_smid_scsi_io(ioc, smid, > - le16_to_cpu(mpi_request->FunctionDependent1)); > + mpt3sas_base_put_smid_scsi_io(ioc, smid, device_handle); > else > mpt3sas_base_put_smid_default(ioc, smid); > break; Where is the point in building a sg_list (via the ->build_sg callback) before checking it a removal is in progress? > @@ -827,6 +837,14 @@ _ctl_do_mpt_command(struct MPT3SAS_ADAPTER *ioc, struct mpt3_ioctl_command karg, > } > } > > + if (test_bit(device_handle, ioc->device_remove_in_progress)) { > + dtmprintk(ioc, pr_info(MPT3SAS_FMT > + "handle(0x%04x) :ioctl failed due to device removal in progress\n", > + ioc->name, device_handle)); > + mpt3sas_base_free_smid(ioc, smid); > + ret = -EINVAL; > + goto out; > + } > mpt3sas_scsih_set_tm_flag(ioc, le16_to_cpu( > tm_request->DevHandle)); > ioc->build_sg_mpi(ioc, psge, data_out_dma, data_out_sz, > @@ -866,6 +884,20 @@ _ctl_do_mpt_command(struct MPT3SAS_ADAPTER *ioc, struct mpt3_ioctl_command karg, > break; > } > case MPI2_FUNCTION_SATA_PASSTHROUGH: > + { > + ioc->build_sg(ioc, psge, data_out_dma, data_out_sz, data_in_dma, > + data_in_sz); > + if (test_bit(device_handle, ioc->device_remove_in_progress)) { > + dtmprintk(ioc, pr_info(MPT3SAS_FMT > + "handle(0x%04x) :ioctl failed due to device removal in progress\n", > + ioc->name, device_handle)); > + mpt3sas_base_free_smid(ioc, smid); > + ret = -EINVAL; > + goto out; > + } > + mpt3sas_base_put_smid_default(ioc, smid); > + break; > + } > case MPI2_FUNCTION_FW_DOWNLOAD: > case MPI2_FUNCTION_FW_UPLOAD: > { Same here. > diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c > index 282ca40..9584d6b 100644 > --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c > +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c > @@ -788,6 +788,11 @@ _scsih_sas_device_add(struct MPT3SAS_ADAPTER *ioc, > list_add_tail(&sas_device->list, &ioc->sas_device_list); > spin_unlock_irqrestore(&ioc->sas_device_lock, flags); > > + if (ioc->hide_drives) { > + clear_bit(sas_device->handle, ioc->pend_os_device_add); > + return; > + } > + > if (!mpt3sas_transport_port_add(ioc, sas_device->handle, > sas_device->sas_address_parent)) { > _scsih_sas_device_remove(ioc, sas_device); > @@ -803,7 +808,8 @@ _scsih_sas_device_add(struct MPT3SAS_ADAPTER *ioc, > sas_device->sas_address_parent); > _scsih_sas_device_remove(ioc, sas_device); > } > - } > + } else > + clear_bit(sas_device->handle, ioc->pend_os_device_add); > } > > /** > @@ -3138,6 +3144,8 @@ _scsih_tm_tr_send(struct MPT3SAS_ADAPTER *ioc, u16 handle) > if (test_bit(handle, ioc->pd_handles)) > return; > > + clear_bit(handle, ioc->pend_os_device_add); > + > spin_lock_irqsave(&ioc->sas_device_lock, flags); > sas_device = __mpt3sas_get_sdev_by_handle(ioc, handle); > if (sas_device && sas_device->starget && > @@ -3192,6 +3200,7 @@ _scsih_tm_tr_send(struct MPT3SAS_ADAPTER *ioc, u16 handle) > mpi_request->Function = MPI2_FUNCTION_SCSI_TASK_MGMT; > mpi_request->DevHandle = cpu_to_le16(handle); > mpi_request->TaskType = MPI2_SCSITASKMGMT_TASKTYPE_TARGET_RESET; > + set_bit(handle, ioc->device_remove_in_progress); > mpt3sas_base_put_smid_hi_priority(ioc, smid, 0); > mpt3sas_trigger_master(ioc, MASTER_TRIGGER_DEVICE_REMOVAL); > > @@ -3326,6 +3335,11 @@ _scsih_sas_control_complete(struct MPT3SAS_ADAPTER *ioc, u16 smid, > ioc->name, le16_to_cpu(mpi_reply->DevHandle), smid, > le16_to_cpu(mpi_reply->IOCStatus), > le32_to_cpu(mpi_reply->IOCLogInfo))); > + if (le16_to_cpu(mpi_reply->IOCStatus) == > + MPI2_IOCSTATUS_SUCCESS) { > + clear_bit(le16_to_cpu(mpi_reply->DevHandle), > + ioc->device_remove_in_progress); > + } > } else { > pr_err(MPT3SAS_FMT "mpi_reply not valid at %s:%d/%s()!\n", > ioc->name, __FILE__, __LINE__, __func__); > @@ -5449,6 +5463,7 @@ _scsih_add_device(struct MPT3SAS_ADAPTER *ioc, u16 handle, u8 phy_num, > device_info = le32_to_cpu(sas_device_pg0.DeviceInfo); > if (!(_scsih_is_end_device(device_info))) > return -1; > + set_bit(handle, ioc->pend_os_device_add); > sas_address = le64_to_cpu(sas_device_pg0.SASAddress); > > /* check if device is present */ > @@ -5467,6 +5482,7 @@ _scsih_add_device(struct MPT3SAS_ADAPTER *ioc, u16 handle, u8 phy_num, > sas_device = mpt3sas_get_sdev_by_addr(ioc, > sas_address); > if (sas_device) { > + clear_bit(handle, ioc->pend_os_device_add); > sas_device_put(sas_device); > return -1; > } Why don't you need to check for the size of the bitmap here? > @@ -5790,6 +5806,9 @@ _scsih_sas_topology_change_event(struct MPT3SAS_ADAPTER *ioc, > _scsih_check_device(ioc, sas_address, handle, > phy_number, link_rate); > > + if (!test_bit(handle, ioc->pend_os_device_add)) > + break; > + > > case MPI2_EVENT_SAS_TOPO_RC_TARG_ADDED: > > @@ -7707,6 +7726,9 @@ mpt3sas_scsih_reset_handler(struct MPT3SAS_ADAPTER *ioc, int reset_phase) > complete(&ioc->tm_cmds.done); > } > > + memset(ioc->pend_os_device_add, 0, ioc->pend_os_device_add_sz); > + memset(ioc->device_remove_in_progress, 0, > + ioc->device_remove_in_progress_sz); > _scsih_fw_event_cleanup_queue(ioc); > _scsih_flush_running_cmds(ioc); > break; > Cheers, Hannes -- Dr. Hannes Reinecke Teamlead Storage & Networking hare@suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg)