Re: [PATCH 02/21] megaraid_sas: Fix calculation of target ID
On Fri, Apr 26, 2019 at 6:36 PM Sasha Levin wrote:
>
> Hi,
>
> [This is an automated email]
>
> This commit has been processed because it contains a -stable tag.
> The stable tag indicates that it's relevant for the following trees: all
>
> The bot has tested the following trees: v5.0.9, v4.19.36, v4.14.113,
> v4.9.170, v4.4.178, v3.18.138.
>
> v5.0.9: Build OK!
> v4.19.36: Build OK!
> v4.14.113: Build OK!
> v4.9.170: Failed to apply! Possible dependencies:
> 15dd03811d99 ("scsi: megaraid_sas: NVME Interface detection and prop
> settings")
> 18103efcacee ("scsi: megaraid-sas: request irqs later")
> 2493c67e518c ("scsi: megaraid_sas: 128 MSIX Support")
> 45f4f2eb3da3 ("scsi: megaraid_sas: Add new pci device Ids for SAS3.5
> Generic Megaraid Controllers")
> 69c337c0f8d7 ("scsi: megaraid_sas: SAS3.5 Generic Megaraid Controllers
> Fast Path for RAID 1/10 Writes")
> 96188a89cc6d ("scsi: megaraid_sas: NVME interface target prop added")
> d0fc91d67c59 ("scsi: megaraid_sas: Send SYNCHRONIZE_CACHE for VD to
> firmware")
> d889344e4e59 ("scsi: megaraid_sas: Dynamic Raid Map Changes for SAS3.5
> Generic Megaraid Controllers")
> fad119b707f8 ("scsi: megaraid_sas: switch to pci_alloc_irq_vectors")
> fdd84e2514b0 ("scsi: megaraid_sas: SAS3.5 Generic Megaraid Controllers
> Stream Detection and IO Coalescing")
>
> v4.4.178: Failed to apply! Possible dependencies:
> 15dd03811d99 ("scsi: megaraid_sas: NVME Interface detection and prop
> settings")
> 179ac14291a0 ("megaraid_sas: Reply Descriptor Post Queue (RDPQ) support")
> 18365b138508 ("megaraid_sas: Task management support")
> 2216c30523b0 ("megaraid_sas: Update device queue depth based on interface
> type")
> 2c048351c8e3 ("megaraid_sas: Syncing request flags macro names with
> firmware")
> 6d40afbc7d13 ("megaraid_sas: MFI IO timeout handling")
> 96188a89cc6d ("scsi: megaraid_sas: NVME interface target prop added")
> d889344e4e59 ("scsi: megaraid_sas: Dynamic Raid Map Changes for SAS3.5
> Generic Megaraid Controllers")
>
> v3.18.138: Failed to apply! Possible dependencies:
> 0b48d12d0365 ("megaraid_sas: Make PI enabled VD 8 byte DMA aligned")
> 0d5b47a724ba ("megaraid_sas: Expose TAPE drives unconditionally")
> 16b8528d2060 ("megaraid_sas: use raw_smp_processor_id()")
> 18365b138508 ("megaraid_sas: Task management support")
> 2216c30523b0 ("megaraid_sas: Update device queue depth based on interface
> type")
> 2be2a98845e6 ("megaraid_sas : Modify return value of
> megasas_issue_blocked_cmd() and wait_and_poll() to consider command status
> returned by firmware")
> 4026e9aac3ff ("megaraid_sas : Use Block layer tag support for internal
> command indexing")
> 4a5c814d9339 ("megaraid_sas : Add separate functions for building sysPD
> IOs and non RW LDIOs")
> 5765c5b8b38a ("megaraid_sas : Support for Avago's Single server High
> Availability product")
> 7497cde883b1 ("megaraid_sas: add support for secure JBOD")
> 8a232bb39917 ("megaraid_sas : add missing __iomem annotations")
> 96188a89cc6d ("scsi: megaraid_sas: NVME interface target prop added")
> aed335eecf8f ("megaraid_sas: Make tape drives visible on PERC5
> controllers")
> d009b5760f57 ("megaraid_sas: online Firmware upgrade support for Extended
> VD feature")
> da0dc9fb4e6b ("megaraid_sas: fix whitespace errors")
>
>
> How should we proceed with this patch?
>
Hi Sasha,
This patch is applicable for 4.11.x and later stable trees only.
I will send out a v2 of this patch with appropriate kernel version requirement
along with the -stable tag.
Thanks,
Shivasharan
> --
> Thanks,
> Sasha
RE: [PATCH 1/2] megaraid_sas: Update structures for HOST_DEVICE_LIST DCMD
> > Move fields in a structure used for communication with the HBA? > > How is that supposed to work? > > Do we need a firmware update for the HBA to handle this correctly? > > Or did it never work, and we need the padding to retrieve the > > information correctly? > > > Please clarify. > > I just merged the support for the new operation a few days ago. I assume that > this is a fix-up and that none of these have shipped yet? > Yes, that is correct. Firmware with support for this new DCMD has not yet shipped. The driver changes were tested earlier, but due to additional requirements the structure needed changes later. And this introduced the alignment mismatch and a fix to the earlier submission. Thanks, Shivasharan
Re: [PATCH] megaraid_sas: Add support for MegaRAID Aero controllers
On Fri, Nov 9, 2018 at 7:43 AM Martin K. Petersen wrote: > Shivasharan, > > > This patch adds support for MegaRAID Aero controller PCI IDs. > > Throw a warning message when a Configurable secure type controller is > > encountered. > > > + dev_warn(&pdev->dev, "Adapter is in configurable secure > > mode\n"); > > Why warn and not info? Hi Martin, Agree that a dev_info would be the better choice here instead of warn. Will send out v2 of this patch with the change. Thanks, Shivasharan > > -- > Martin K. Petersen Oracle Linux Engineering
RE: [PATCH V2 00/19] megaraid_sas: Driver updates
Hi Martin, > Changes in V2: > - Rebased the entire series on top of below patches - > "scsi: megaraid_sas: fix a missing-check bug" > "megaraid_sas: switch to generic DMA API" > - Patch 2/19: Replace PCI DMA APIs with generic DMA APIs. > > Shivasharan S (19): > megaraid_sas: Add watchdog thread to detect Firmware fault > megaraid_sas: Add support for FW snap dump > megaraid_sas: Fix msleep granularity > megaraid_sas: Add check for reset adapter bit > megaraid_sas: Update copyright information > megaraid_sas: Fix goto labels in error handling > megaraid_sas: Fix module parameter description > megaraid_sas: Fix combined reply queue mode detection > megaraid_sas: For SRIOV, do not set STOP_ADP bit > megaraid_sas: Fail init if heartbeat timer fails > megaraid_sas: optimize raid context access in IO path > megaraid_sas: Remove spin lock for dpc operation > megaraid_sas: Rename scratch_pad registers > megaraid_sas: Re-use max_mfi_cmds to calculate queue sizes > megaraid_sas: Remove double endian conversion > megaraid_sas: increase timeout for IOC INIT to 180seconds > megaraid_sas: remove unused macro > megaraid_sas: modify max supported lds related print > megaraid_sas: Update driver version > > drivers/scsi/megaraid/megaraid_sas.h| 65 +++-- > drivers/scsi/megaraid/megaraid_sas_base.c | 298 - > drivers/scsi/megaraid/megaraid_sas_fp.c | 14 +- > drivers/scsi/megaraid/megaraid_sas_fusion.c | 384 ++ > -- drivers/scsi/megaraid/megaraid_sas_fusion.h | 26 +- > 5 files changed, 546 insertions(+), 241 deletions(-) > > -- > 2.16.1 Can you please let us know if above patch series is good to be picked for the next kernel version? Thanks, Shivasharan
RE: [PATCH 02/19] megaraid_sas: Add support for FW snap dump
> > + > > + instance->snapdump_prop = > > + pci_alloc_consistent(pdev, > > +sizeof(struct > MR_SNAPDUMP_PROPERTIES), > > +&instance->snapdump_prop_h); > > No new calls to the PCI DMA API please. > > Please also review my patch titled > "[PATCH 13/28] megaraid_sas: switch to generic DMA API" that I sent to the > list earlier this week and preferably rebase on top of it. Sure Christoph. I had missed your patch series. Will review the same. And will send out V2 of this patch series rebased on top of your changes.
RE: [PATCH] scsi: megaraid_sas: Do not log an error if FW successfully initializes.
Hi Vinson,
> Fixes: 2d2c2331673c ("scsi: megaraid_sas: modified few prints in OCR and
IOC
> INIT path")
> Signed-off-by: Vinson Lee
> ---
> drivers/scsi/megaraid/megaraid_sas_fusion.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
Thanks for the patch.
Acked-by: Shivasharan S
Thanks,
Shivasharan
RE: [PATCH] megaraid_sas: Re-enable WRITE SAME
> -Original Message- > From: Martin K. Petersen [mailto:martin.peter...@oracle.com] > Sent: Thursday, February 22, 2018 9:52 AM > To: Shivasharan S > Cc: linux-scsi@vger.kernel.org; kashyap.de...@broadcom.com; > sumit.sax...@broadcom.com > Subject: Re: [PATCH] megaraid_sas: Re-enable WRITE SAME > > > Hi Shivasharan! > > > For MegaRAID controller we want to support WRITE SAME for non-raid > > volumes. Because of host wide "no_write_same = 1" settings, we are > > not able to use provision mode. > > The no_write_same flag gates whether the WRITE SAME(10/16) commands > are > used to zero block ranges. It does not influence whether WRITE SAME with > the UNMAP bit set is used to deallocate block ranges on devices which > support logical block provisioning. Hi Martin, Thanks for this clarification. I checked further to understand difference between zeroout block ranges and deallocation (unmap) of block ranges using WRITE SAME command and impact of setting no_write_same flag. So my understanding is when driver sets no_write_same flag, it is only disabling BLKZEROOUT/write_zeroes requests from using WRITE SAME command. Instead individual writes are sent to zero the blocks (if BLKDEV_ZERO_NOFALLBACK flag is not set). > > > Virtual Disk will not enable WRITE SAME feature providing correct VPD > > block limit page. > > This wasn't always the case. > > If you want to enable WRITE SAME you'll have to provide some sort of > heuristic to avoid breaking things on firmware versions and controllers > that predate correct reporting. > Our inhouse driver code did not set the no_write_same flag. And so far we have not seen any issues in this area with inhouse driver. I am running tests now to check firmware behavior you mentioned. Will get back to you with the results. Are you aware of any issue that led to this change? > > Virtual Disk in MegaRAID provides below values in VPD page 0xb0 which > > will disable WRITE SAME commands. > > A value of 0 in > > Maximum write same length: 0x0 blocks > > means the maximum length is not reported. It does not mean that WRITE > SAME isn't supported. > Ok. With latest firmware, for virtual disks we do not set write same/unmap support in the logical block provisioning VPD page. Will check behavior with older firmware versions also and get back. Logical block provisioning VPD page (SBC): Unmap command supported (LBPU): 0 Write same (16) with unmap bit supported (LBWS): 0 Write same (10) with unmap bit supported (LBWS10): 0 Logical block provisioning read zeros (LBPRZ): 0 Anchored LBAs supported (ANC_SUP): 0 Threshold exponent: 0 Descriptor present (DP): 0 Minimum percentage: 0 Provisioning type: 0 Threshold percentage: 0 Thanks, Shivasharan > -- > Martin K. PetersenOracle Linux Engineering
RE: [PATCH] scsi: megaraid: Use zeroing memory allocator than allocator/memset
> -Original Message- > From: Himanshu Jha [mailto:himanshujha199...@gmail.com] > Sent: Saturday, December 30, 2017 9:18 PM > To: martin.peter...@oracle.com; j...@linux.vnet.ibm.com > Cc: kashyap.de...@broadcom.com; sumit.sax...@broadcom.com; > shivasharan.srikanteshw...@broadcom.com; > megaraidlinux@broadcom.com; linux-scsi@vger.kernel.org; linux- > ker...@vger.kernel.org; mcg...@kernel.org; Himanshu Jha > > Subject: [PATCH] scsi: megaraid: Use zeroing memory allocator than > allocator/memset > > Use pci_zalloc_consistent for allocating zeroed memory and remove > unnecessary memset function. > > Done using Coccinelle. > Generated-by: scripts/coccinelle/api/alloc/kzalloc-simple.cocci > 0-day tested with no failures. > > Suggested-by: Luis R. Rodriguez > Signed-off-by: Himanshu Jha > --- > drivers/scsi/megaraid/megaraid_sas_base.c | 19 ++- > drivers/scsi/megaraid/megaraid_sas_fusion.c | 5 ++--- > 2 files changed, 8 insertions(+), 16 deletions(-) > Hi Himanshu, I can see one more allocation done in megasas_get_seq_num() that could make use of these changes. Rest of the changes looks fine. Also, there is a driver update patchset that I am planning to post today. Adding this patch first might require additional rebasing of this patchset. Would you be ok if I send a separate patch for this along with the change mentioned above once my patch series gets committed? That would save me some rebasing effort. :-) Thanks, Shivasharan
RE: [PATCH] scsi: megaraid: Convert timers to use timer_setup()
> -Original Message- > From: Sumit Saxena [mailto:sumit.sax...@broadcom.com] > Sent: Wednesday, November 1, 2017 12:18 AM > To: Kees Cook; Martin K. Petersen > Cc: Kashyap Desai; Shivasharan Srikanteshwara; James E.J. Bottomley; > PDL,MEGARAIDLINUX; linux-scsi@vger.kernel.org; linux- > ker...@vger.kernel.org > Subject: RE: [PATCH] scsi: megaraid: Convert timers to use timer_setup() > > -Original Message- > From: Kees Cook [mailto:keesc...@chromium.org] > Sent: Wednesday, October 25, 2017 3:37 PM > To: Martin K. Petersen > Cc: Kashyap Desai; Sumit Saxena; Shivasharan S; James E.J. Bottomley; > megaraidlinux@broadcom.com; linux-scsi@vger.kernel.org; linux- > ker...@vger.kernel.org > Subject: [PATCH] scsi: megaraid: Convert timers to use timer_setup() > > In preparation for unconditionally passing the struct timer_list pointer > to all > timer callbacks, switch to using the new timer_setup() and > from_timer() to pass the timer pointer explicitly. Also consolidates the > timer > setup functions arguments, which are all identical, and corrects on-stack > timer > usage. > Ran basic sanity test for megaraid_sas part, and the changes look good. Acked-by: Shivasharan S Tested-by: Shivasharan S Hannes, Can you please take a look at the megaraid_mbox and megaraid_mm changes? Thanks, Shivasharan
RE: system hung up when offlining CPUs
> -Original Message- > From: Thomas Gleixner [mailto:t...@linutronix.de] > Sent: Tuesday, October 17, 2017 1:57 AM > To: YASUAKI ISHIMATSU > Cc: Kashyap Desai; Hannes Reinecke; Marc Zyngier; Christoph Hellwig; > ax...@kernel.dk; m...@ellerman.id.au; keith.bu...@intel.com; > pet...@infradead.org; LKML; linux-scsi@vger.kernel.org; Sumit Saxena; > Shivasharan Srikanteshwara > Subject: Re: system hung up when offlining CPUs > > Yasuaki, > > On Mon, 16 Oct 2017, YASUAKI ISHIMATSU wrote: > > > Hi Thomas, > > > > > Can you please apply the patch below on top of Linus tree and retest? > > > > > > Please send me the outputs I asked you to provide last time in any > > > case (success or fail). > > > > The issue still occurs even if I applied your patch to linux 4.14.0-rc4. > > Thanks for testing. > > > --- > > [ ...] INFO: task setroubleshootd:4972 blocked for more than 120 seconds. > > [ ...] Not tainted 4.14.0-rc4.thomas.with.irqdebug+ #6 > > [ ...] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this > message. > > [ ...] setroubleshootd D0 4972 1 0x0080 > > [ ...] Call Trace: > > [ ...] __schedule+0x28d/0x890 > > [ ...] ? release_pages+0x16f/0x3f0 > > [ ...] schedule+0x36/0x80 > > [ ...] io_schedule+0x16/0x40 > > [ ...] wait_on_page_bit+0x107/0x150 > > [ ...] ? page_cache_tree_insert+0xb0/0xb0 [ ...] > > truncate_inode_pages_range+0x3dd/0x7d0 > > [ ...] ? schedule_hrtimeout_range_clock+0xad/0x140 > > [ ...] ? remove_wait_queue+0x59/0x60 > > [ ...] ? down_write+0x12/0x40 > > [ ...] ? unmap_mapping_range+0x75/0x130 [ ...] > > truncate_pagecache+0x47/0x60 [ ...] truncate_setsize+0x32/0x40 [ ...] > > xfs_setattr_size+0x100/0x300 [xfs] [ ...] > > xfs_vn_setattr_size+0x40/0x90 [xfs] [ ...] xfs_vn_setattr+0x87/0xa0 > > [xfs] [ ...] notify_change+0x266/0x440 [ ...] do_truncate+0x75/0xc0 > > [ ...] path_openat+0xaba/0x13b0 [ ...] ? > > mem_cgroup_commit_charge+0x31/0x130 > > [ ...] do_filp_open+0x91/0x100 > > [ ...] ? __alloc_fd+0x46/0x170 > > [ ...] do_sys_open+0x124/0x210 > > [ ...] SyS_open+0x1e/0x20 > > [ ...] do_syscall_64+0x67/0x1b0 > > [ ...] entry_SYSCALL64_slow_path+0x25/0x25 > > This is definitely a driver issue. The driver requests an affinity managed > interrupt. Affinity managed interrupts are different from non managed > interrupts in several ways: > > Non-Managed interrupts: > > 1) At setup time the default interrupt affinity is assigned to each > interrupt. The effective affinity is usually a subset of the online > CPUs. > > 2) User space can modify the affinity of the interrupt > > 3) If a CPU in the affinity mask goes offline and there are still online > CPUs in the affinity mask then the effective affinity is moved to a > subset of the online CPUs in the affinity mask. > > If the last CPU in the affinity mask of an interrupt goes offline then > the hotplug code breaks the affinity and makes it affine to the online > CPUs. The effective affinity is a subset of the new affinity setting, > > Managed interrupts: > > 1) At setup time the interrupts of a multiqueue device are evenly spread > over the possible CPUs. If all CPUs in the affinity mask of a given > interrupt are offline at request_irq() time, the interrupt stays shut > down. If the first CPU in the affinity mask comes online later the > interrupt is started up. > > 2) User space cannot modify the affinity of the interrupt > > 3) If a CPU in the affinity mask goes offline and there are still online > CPUs in the affinity mask then the effective affinity is moved a subset > of the online CPUs in the affinity mask. I.e. the same as with > Non-Managed interrupts. > > If the last CPU in the affinity mask of a managed interrupt goes > offline then the interrupt is shutdown. If the first CPU in the > affinity mask becomes online again then the interrupt is started up > again. > Hi Thomas, Thanks for the detailed explanation about the behavior of managed interrupts. This helped me to understand the issue better. This is first time I am checking CPU hotplug system, so my input is very preliminary. Please bear with my understanding and correct me where required. This issue is reproducible on our local setup as well, with managed interrupts. I have few queries on the requirements for device driver that you have mentioned. In managed-interrupts case, interrupts which were affine to the offlined CPU is not getting migrated to another available CPU. But the documentation at below link says that "all
RE: [PATCH V2] megaraid: kmemleak: Track page allocation for fusion
> -Original Message- > From: Christoph Hellwig [mailto:h...@infradead.org] > Sent: Monday, September 18, 2017 9:52 PM > To: Shivasharan Srikanteshwara > Cc: Christoph Hellwig; shuw...@redhat.com; Kashyap Desai; Sumit Saxena; > j...@linux.vnet.ibm.com; martin.peter...@oracle.com; > PDL,MEGARAIDLINUX; linux-scsi@vger.kernel.org; linux- > ker...@vger.kernel.org; ch...@redhat.com; yiz...@redhat.com; > catalin.mari...@arm.com > Subject: Re: [PATCH V2] megaraid: kmemleak: Track page allocation for fusion > > Oh, I missed log_to_span. Well, in that case log_to_span is _the_ candidate > for moving into a separate allocation. > > And in fact you're probably better off by using a sensible data structure for it, > e.g. a radix tree. Thanks Christoph. We will make the changes suggested in phased approach. First we will fix kmemleak false positives by moving log_to_span allocation separate from fusion_context. The data structure change would involve major changes which affects IO path as well. Also driver expects log_to_span and other data structures to be available at load time itself. Considering this, we need to understand if radix tree would be a good choice for the change. Based on internal discussions, we see other similar arrays in driver code that we can change similarly eg. load_balance_info. This is definitely something to add to our to-do lists. These changes need to go through our internal regression test cycle and then submit it to upstream. Best regards, Shivasharan
RE: [PATCH V2] megaraid: kmemleak: Track page allocation for fusion
> -Original Message-
> From: Christoph Hellwig [mailto:h...@infradead.org]
> Sent: Friday, September 15, 2017 11:30 PM
> To: shuw...@redhat.com
> Cc: kashyap.de...@broadcom.com; sumit.sax...@broadcom.com;
> shivasharan.srikanteshw...@broadcom.com; j...@linux.vnet.ibm.com;
> martin.peter...@oracle.com; megaraidlinux@broadcom.com; linux-
> s...@vger.kernel.org; linux-ker...@vger.kernel.org; ch...@redhat.com;
> yiz...@redhat.com; catalin.mari...@arm.com
> Subject: Re: [PATCH V2] megaraid: kmemleak: Track page allocation for
fusion
>
> I think the megaraid fusion code has a deeper problem here.
>
> Instead of playing weird games with get_free_pages and vmalloc the
structure
> just needs to shrink by moving all the arrays of MAX_MSIX_QUEUES_FUSION
> size into a separate allocation for each, and then we have normall,
small
> kmalloc allocations.
Hi Christoph,
We understand your suggestion on shrinking the size of fusion_context so
that we can use kmalloc to allocate the structure.
Size of fusion_context structure now is about 179kB and it is contributed
almost entirely by log_to_span array (~176kB).
The rest of the arrays do not make as much difference to the size.
We will send a new patch that separates allocation for log_to_span array
from fusion_context.
For now it is a Nack for this patch then.
crash> struct -o fusion_context
struct fusion_context {
[0] struct megasas_cmd_fusion **cmd_list;
[8] dma_addr_t req_frames_desc_phys;
[16] u8 *req_frames_desc;
[24] struct dma_pool *io_request_frames_pool;
[32] dma_addr_t io_request_frames_phys;
[40] u8 *io_request_frames;
[48] struct dma_pool *sg_dma_pool;
[56] struct dma_pool *sense_dma_pool;
[64] dma_addr_t reply_frames_desc_phys[128];
[1088] union MPI2_REPLY_DESCRIPTORS_UNION *reply_frames_desc[128];
[2112] struct dma_pool *reply_frames_desc_pool;
[2120] u16 last_reply_idx[128];
[2376] u32 reply_q_depth;
[2380] u32 request_alloc_sz;
[2384] u32 reply_alloc_sz;
[2388] u32 io_frames_alloc_sz;
[2392] struct MPI2_IOC_INIT_RDPQ_ARRAY_ENTRY *rdpq_virt;
[2400] dma_addr_t rdpq_phys;
[2408] u16 max_sge_in_main_msg;
[2410] u16 max_sge_in_chain;
[2412] u8 chain_offset_io_request;
[2413] u8 chain_offset_mfi_pthru;
[2416] struct MR_FW_RAID_MAP_DYNAMIC *ld_map[2];
[2432] dma_addr_t ld_map_phys[2];
[2448] struct MR_DRV_RAID_MAP_ALL *ld_drv_map[2];
[2464] u32 max_map_sz;
[2468] u32 current_map_sz;
[2472] u32 old_map_sz;
[2476] u32 new_map_sz;
[2480] u32 drv_map_sz;
[2484] u32 drv_map_pages;
[2488] struct MR_PD_CFG_SEQ_NUM_SYNC *pd_seq_sync[2];
[2504] dma_addr_t pd_seq_phys[2];
[2520] u8 fast_path_io;
[2528] struct LD_LOAD_BALANCE_INFO *load_balance_info;
[2536] u32 load_balance_info_pages;
[2544] LD_SPAN_INFO log_to_span[256];
[182768] u8 adapter_type;
[182776] struct LD_STREAM_DETECT **stream_detect_by_ld; }
SIZE: 182784
Thanks,
Shivasharan
RE: [PATCH v2 11/15] megaraid_sas: Set device queue_depth same as HBA can_queue value in scsi-mq mode
> -Original Message- > From: Shivasharan Srikanteshwara > [mailto:shivasharan.srikanteshw...@broadcom.com] > Sent: Monday, July 24, 2017 4:59 PM > To: 'Christoph Hellwig' > Cc: 'linux-scsi@vger.kernel.org'; 'martin.peter...@oracle.com'; > 'the...@redhat.com'; 'j...@linux.vnet.ibm.com'; Sumit Saxena; > 'h...@suse.com'; Kashyap Desai > Subject: RE: [PATCH v2 11/15] megaraid_sas: Set device queue_depth same as > HBA can_queue value in scsi-mq mode > > > -Original Message- > > From: Christoph Hellwig [mailto:h...@lst.de] > > Sent: Thursday, July 20, 2017 1:18 PM > > To: Shivasharan Srikanteshwara > > Cc: Christoph Hellwig; linux-scsi@vger.kernel.org; > > martin.peter...@oracle.com; the...@redhat.com; > > j...@linux.vnet.ibm.com; Sumit Saxena; h...@suse.com; Kashyap Desai > > Subject: Re: [PATCH v2 11/15] megaraid_sas: Set device queue_depth > > same as HBA can_queue value in scsi-mq mode > > > > I still don't understand why you don't want to do the same for the non-mq > path. > > Hi Christoph, > > Sorry for delay in response. > > MQ case - > If there is any block layer requeue happens, we see performance drop. So we > avoid re-queue increasing Device QD = HBA QD. Performance drop due to block > layer re-queue is more in case of HDD (sequential IO converted into random IO). > > Non-MQ case. > If we increase Device QD = HBA QD for no-mq case, we see performance drop > for certain profiles. > For example SATA SSD, previous driver in non-mq set Device QD=32. In this case, > if we have more outstanding IO per device (more than 32), block layer attempts > soft merge and eventually end user experience higher performance due to block > layer attempting soft merge. Same is not correct in MQ case, as IO scheduler in > MQ adds overhead if at all there is any throttling or staging due to device QD. > > Below is example of single SATA SSD, Sequential Read, BS=4K, IO depth = 256 > > MQ enable, Device QD = 32 achieves 137K IOPS MQ enable, Device QD = 916 > (HBA QD) achieves 145K IOPS > > MQ disable, Device QD = 32 achieves 237K IOPS MQ disable, Device QD = 916 > (HBA QD) achieves 145K IOPS > > Ideally we want to keep same QD settings in non-MQ mode, but trying to avoid > as we may face some regression from end user as explained. > > Thanks, > Shivasharan Hi Christoph, Can you please let us know your thoughts on this? We understand that the settings should ideally be uniform across non-mq and mq cases. But based on the test results above, for non-mq case we are seeing some difference in performance for certain IO profiles when compared to earlier releases after increasing queue depth. Same is not the case when mq is enabled. Based on these results, we would like to keep this patch as is for this phase. We will run the further tests on this and will update for the next phase. Thanks, Shivasharan
RE: [PATCH v2 11/15] megaraid_sas: Set device queue_depth same as HBA can_queue value in scsi-mq mode
> -Original Message- > From: Christoph Hellwig [mailto:h...@lst.de] > Sent: Thursday, July 20, 2017 1:18 PM > To: Shivasharan Srikanteshwara > Cc: Christoph Hellwig; linux-scsi@vger.kernel.org; martin.peter...@oracle.com; > the...@redhat.com; j...@linux.vnet.ibm.com; Sumit Saxena; h...@suse.com; > Kashyap Desai > Subject: Re: [PATCH v2 11/15] megaraid_sas: Set device queue_depth same as > HBA can_queue value in scsi-mq mode > > I still don't understand why you don't want to do the same for the non-mq path. Hi Christoph, Sorry for delay in response. MQ case - If there is any block layer requeue happens, we see performance drop. So we avoid re-queue increasing Device QD = HBA QD. Performance drop due to block layer re-queue is more in case of HDD (sequential IO converted into random IO). Non-MQ case. If we increase Device QD = HBA QD for no-mq case, we see performance drop for certain profiles. For example SATA SSD, previous driver in non-mq set Device QD=32. In this case, if we have more outstanding IO per device (more than 32), block layer attempts soft merge and eventually end user experience higher performance due to block layer attempting soft merge. Same is not correct in MQ case, as IO scheduler in MQ adds overhead if at all there is any throttling or staging due to device QD. Below is example of single SATA SSD, Sequential Read, BS=4K, IO depth = 256 MQ enable, Device QD = 32 achieves 137K IOPS MQ enable, Device QD = 916 (HBA QD) achieves 145K IOPS MQ disable, Device QD = 32 achieves 237K IOPS MQ disable, Device QD = 916 (HBA QD) achieves 145K IOPS Ideally we want to keep same QD settings in non-MQ mode, but trying to avoid as we may face some regression from end user as explained. Thanks, Shivasharan
RE: [PATCH v2 11/15] megaraid_sas: Set device queue_depth same as HBA can_queue value in scsi-mq mode
> -Original Message- > From: Shivasharan Srikanteshwara > [mailto:shivasharan.srikanteshw...@broadcom.com] > Sent: Wednesday, July 12, 2017 1:51 PM > To: Kashyap Desai; 'Christoph Hellwig' > Cc: 'linux-scsi@vger.kernel.org'; 'martin.peter...@oracle.com'; > 'the...@redhat.com'; 'j...@linux.vnet.ibm.com'; Sumit Saxena; > 'h...@suse.com' > Subject: RE: [PATCH v2 11/15] megaraid_sas: Set device queue_depth same as > HBA can_queue value in scsi-mq mode > > > -Original Message- > > From: Kashyap Desai [mailto:kashyap.de...@broadcom.com] > > Sent: Tuesday, July 11, 2017 9:18 PM > > To: Christoph Hellwig; Shivasharan Srikanteshwara > > Cc: linux-scsi@vger.kernel.org; martin.peter...@oracle.com; > > the...@redhat.com; j...@linux.vnet.ibm.com; Sumit Saxena; h...@suse.com > > Subject: RE: [PATCH v2 11/15] megaraid_sas: Set device queue_depth same > > as > > HBA can_queue value in scsi-mq mode > > > > > -Original Message- > > > From: Christoph Hellwig [mailto:h...@lst.de] > > > Sent: Tuesday, July 11, 2017 7:28 PM > > > To: Shivasharan S > > > Cc: linux-scsi@vger.kernel.org; martin.peter...@oracle.com; > > > the...@redhat.com; j...@linux.vnet.ibm.com; > > > kashyap.de...@broadcom.com; sumit.sax...@broadcom.com; > > h...@suse.com; > > > h...@lst.de > > > Subject: Re: [PATCH v2 11/15] megaraid_sas: Set device queue_depth > > > same > > as > > > HBA can_queue value in scsi-mq mode > > > > > > On Wed, Jul 05, 2017 at 05:00:25AM -0700, Shivasharan S wrote: > > > > Currently driver sets default queue_depth for VDs at 256 and JBODs > > > > based on interface type, ie., for SAS JBOD QD will be 64, for SATA > > JBOD QD > > > will be 32. > > > > During performance runs with scsi-mq enabled, we are seeing better > > > > results by setting QD same as HBA queue_depth. > > > > > > Please no scsi-mq specifics. just do this unconditionally. > > > > Chris - Intent for mq specific check is mainly because of sequential > > work load > > for HDD is having penalty due to mq scheduler issue. > > We did this exercise prior to mq-deadline support. > > > > Making generic change for non-mq and mq was good, but we may see some > > user may not like to see regression. > > E.a In case of, QD = 32 for SATA PD file system creation may be faster > compare > > to large QD. There may be a soft merger at block layer due to queue > > depth > > throttling. Eventually, FS creation goes fast due to IO merges, but same > > will > not > > be true if we change queue depth logic (means, increase device queue > > depth > to > > HBA QD.) > > > > We have choice to completely remove this patch and ask users to do sysfs > > settings in case of scsi-mq performance issue for HDD sequential work > > load. > > Having this patch, we want to provide better QD settings as default from > driver. > > > > > > Thanks, Kashyap > > Hi Christoph, > As Kashyap mentioned, the performance issues seen were specific to scsi-mq > enabled case when running sequential workloads with HDDs. > Making this generic might result in regressions in some scenarios for > non-mq. > That was the idea behind making the change specific to scsi-mq only. > > Let us know if you are ok with having this as is or we could remove > this patch completely and have users manually tune queue depth settings if > they > are seeing performance issues with scsi-mq enabled. > > Thanks, > Shivasharan Hi Christoph, Can you please let us know your thoughts on this patch? Are we good to keep the changes as is? Thanks, Shivasharan
RE: [PATCH v2 08/15] megaraid_sas: Use SMID for Task abort case only
> -Original Message- > From: Shivasharan S [mailto:shivasharan.srikanteshw...@broadcom.com] > Sent: Wednesday, July 05, 2017 5:30 PM > To: linux-scsi@vger.kernel.org > Cc: martin.peter...@oracle.com; the...@redhat.com; > j...@linux.vnet.ibm.com; kashyap.de...@broadcom.com; > sumit.sax...@broadcom.com; h...@suse.com; h...@lst.de; Shivasharan S > Subject: [PATCH v2 08/15] megaraid_sas: Use SMID for Task abort case only > > Fix - In TM code, smid_task is valid only in case of task aborts. > > Signed-off-by: Kashyap Desai > Signed-off-by: Shivasharan S > Reviewed-by: Hannes Reinecke Hi Tomas, Looks like this one patch got missed during your review. Can you please review if the changes in this patch are fine? Thanks, Shivasharan
RE: [PATCH v2 11/15] megaraid_sas: Set device queue_depth same as HBA can_queue value in scsi-mq mode
> -Original Message- > From: Kashyap Desai [mailto:kashyap.de...@broadcom.com] > Sent: Tuesday, July 11, 2017 9:18 PM > To: Christoph Hellwig; Shivasharan Srikanteshwara > Cc: linux-scsi@vger.kernel.org; martin.peter...@oracle.com; > the...@redhat.com; j...@linux.vnet.ibm.com; Sumit Saxena; h...@suse.com > Subject: RE: [PATCH v2 11/15] megaraid_sas: Set device queue_depth same as > HBA can_queue value in scsi-mq mode > > > -Original Message- > > From: Christoph Hellwig [mailto:h...@lst.de] > > Sent: Tuesday, July 11, 2017 7:28 PM > > To: Shivasharan S > > Cc: linux-scsi@vger.kernel.org; martin.peter...@oracle.com; > > the...@redhat.com; j...@linux.vnet.ibm.com; > > kashyap.de...@broadcom.com; sumit.sax...@broadcom.com; > h...@suse.com; > > h...@lst.de > > Subject: Re: [PATCH v2 11/15] megaraid_sas: Set device queue_depth > > same > as > > HBA can_queue value in scsi-mq mode > > > > On Wed, Jul 05, 2017 at 05:00:25AM -0700, Shivasharan S wrote: > > > Currently driver sets default queue_depth for VDs at 256 and JBODs > > > based on interface type, ie., for SAS JBOD QD will be 64, for SATA > JBOD QD > > will be 32. > > > During performance runs with scsi-mq enabled, we are seeing better > > > results by setting QD same as HBA queue_depth. > > > > Please no scsi-mq specifics. just do this unconditionally. > > Chris - Intent for mq specific check is mainly because of sequential work > load > for HDD is having penalty due to mq scheduler issue. > We did this exercise prior to mq-deadline support. > > Making generic change for non-mq and mq was good, but we may see some > user may not like to see regression. > E.a In case of, QD = 32 for SATA PD file system creation may be faster > compare > to large QD. There may be a soft merger at block layer due to queue depth > throttling. Eventually, FS creation goes fast due to IO merges, but same > will not > be true if we change queue depth logic (means, increase device queue depth > to > HBA QD.) > > We have choice to completely remove this patch and ask users to do sysfs > settings in case of scsi-mq performance issue for HDD sequential work > load. > Having this patch, we want to provide better QD settings as default from > driver. > > > Thanks, Kashyap Hi Christoph, As Kashyap mentioned, the performance issues seen were specific to scsi-mq enabled case when running sequential workloads with HDDs. Making this generic might result in regressions in some scenarios for non-mq. That was the idea behind making the change specific to scsi-mq only. Let us know if you are ok with having this as is or we could remove this patch completely and have users manually tune queue depth settings if they are seeing performance issues with scsi-mq enabled. Thanks, Shivasharan
RE: [PATCH v2 09/15] megaraid_sas: use vmalloc for crash dump buffers and driver's local RAID map
> -Original Message-
> From: Tomas Henzl [mailto:the...@redhat.com]
> Sent: Monday, July 10, 2017 7:15 PM
> To: Shivasharan S; linux-scsi@vger.kernel.org
> Cc: martin.peter...@oracle.com; j...@linux.vnet.ibm.com;
> kashyap.de...@broadcom.com; sumit.sax...@broadcom.com;
> h...@suse.com; h...@lst.de
> Subject: Re: [PATCH v2 09/15] megaraid_sas: use vmalloc for crash dump
> buffers and driver's local RAID map
>
> On 5.7.2017 14:00, Shivasharan S wrote:
> > Signed-off-by: Kashyap Desai
> > Signed-off-by: Shivasharan S
> > Reviewed-by: Hannes Reinecke
> > ---
> > drivers/scsi/megaraid/megaraid_sas.h| 1 -
> > drivers/scsi/megaraid/megaraid_sas_base.c | 12 ++-
> > drivers/scsi/megaraid/megaraid_sas_fusion.c | 121
> > ++--
> > 3 files changed, 88 insertions(+), 46 deletions(-)
> >
> > diff --git a/drivers/scsi/megaraid/megaraid_sas.h
> > b/drivers/scsi/megaraid/megaraid_sas.h
> > index 2b209bb..6d9f111 100644
> > --- a/drivers/scsi/megaraid/megaraid_sas.h
> > +++ b/drivers/scsi/megaraid/megaraid_sas.h
> > @@ -2115,7 +2115,6 @@ struct megasas_instance {
> > u32 *crash_dump_buf;
> > dma_addr_t crash_dump_h;
> > void *crash_buf[MAX_CRASH_DUMP_SIZE];
> > - u32 crash_buf_pages;
> > unsigned intfw_crash_buffer_size;
> > unsigned intfw_crash_state;
> > unsigned intfw_crash_buffer_offset;
> > diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c
> > b/drivers/scsi/megaraid/megaraid_sas_base.c
> > index e490272..c63ef88 100644
> > --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> > +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
> > @@ -49,6 +49,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include
> > #include
> > @@ -6672,9 +6673,14 @@ static void megasas_detach_one(struct pci_dev
> *pdev)
> > fusion->max_map_sz,
> > fusion->ld_map[i],
> > fusion->ld_map_phys[i]);
> > - if (fusion->ld_drv_map[i])
> > - free_pages((ulong)fusion->ld_drv_map[i],
> > - fusion->drv_map_pages);
> > + if (fusion->ld_drv_map[i]) {
> > + if (is_vmalloc_addr(fusion->ld_drv_map[i]))
> > + vfree(fusion->ld_drv_map[i]);
> > + else
> > + free_pages((ulong)fusion-
> >ld_drv_map[i],
> > + fusion->drv_map_pages);
> > + }
> > +
> > if (fusion->pd_seq_sync[i])
> > dma_free_coherent(&instance->pdev->dev,
> > pd_seq_map_sz,
> > diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > index c239762..ff4a3a8 100644
> > --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > @@ -1257,6 +1257,80 @@ static int
> > megasas_create_sg_sense_fusion(struct megasas_instance *instance) }
> >
> > /**
> > + * megasas_allocate_raid_maps -Allocate memory for RAID maps
> > + * @instance: Adapter soft state
> > + *
> > + * return: if success: return 0
> > + * failed: return -ENOMEM
> > + */
> > +static inline int megasas_allocate_raid_maps(struct megasas_instance
> > +*instance) {
> > + struct fusion_context *fusion;
> > + int i = 0;
> > +
> > + fusion = instance->ctrl_context;
> > +
> > + fusion->drv_map_pages = get_order(fusion->drv_map_sz);
> > +
> > + for (i = 0; i < 2; i++) {
> > + fusion->ld_map[i] = NULL;
>
> Hi, does this assignment^ mean, that you need a fusion->ld_drv_map[0;1] =
> NULL setting before this for cycle as well or is it just superfluos ?
>
Hi Tomas,
Initializing ld_map[i] = NULL is not necessary but that got carried over
from
earlier code. We do not need to set fusion->ld_drv_map[0:1] to NULL here as
fusion_context is memset to zero during allocation.
>
> > +
> > + fusion->ld_drv_map[i] = (void *)
> > + __get_free_pages(__GFP_ZERO | GFP_KERNEL,
> > +fusion->drv_map_pages);
>
> The subject says - 'use vmalloc for ... and driver's local RAID map'
> in the code here you use vmalloc only if __get_free_pages fails is this
> intended ?
> (maybe an explanation in the mail body would be nice)
>
> tomash
>
I will send out v3 of the series with a more detailed commit description.
The use of __get_free_pages first for driver's local RAID map is intentional
as this
structure is frequently accessed. But we do not want to fail device probe
due
to unavailability of contiguous memory.
Thanks,
Shivasharan
> > +
> > + if (!fusion->ld_drv_map[i]) {
> > +
RE: [PATCH 05/15] megaraid_sas: Do not re-fire shutdown DCMD after OCR
> -Original Message-
> From: Shivasharan Srikanteshwara
> [mailto:shivasharan.srikanteshw...@broadcom.com]
> Sent: Tuesday, July 04, 2017 12:39 PM
> To: 'Hannes Reinecke'; 'linux-scsi@vger.kernel.org'
> Cc: 'martin.peter...@oracle.com'; 'the...@redhat.com';
> 'j...@linux.vnet.ibm.com'; Kashyap Desai; Sumit Saxena; 'h...@suse.com';
> 'h...@lst.de'
> Subject: RE: [PATCH 05/15] megaraid_sas: Do not re-fire shutdown DCMD
> after
> OCR
>
> > -Original Message-
> > From: Hannes Reinecke [mailto:h...@suse.de]
> > Sent: Friday, June 30, 2017 7:00 PM
> > To: Shivasharan S; linux-scsi@vger.kernel.org
> > Cc: martin.peter...@oracle.com; the...@redhat.com;
> > j...@linux.vnet.ibm.com; kashyap.de...@broadcom.com;
> > sumit.sax...@broadcom.com; h...@suse.com; h...@lst.de
> > Subject: Re: [PATCH 05/15] megaraid_sas: Do not re-fire shutdown DCMD
> > after OCR
> >
> > On 06/30/2017 10:29 AM, Shivasharan S wrote:
> > > Signed-off-by: Kashyap Desai
> > > Signed-off-by: Shivasharan S
> > >
> > > ---
> > > drivers/scsi/megaraid/megaraid_sas_fusion.c | 9 +
> > > 1 file changed, 9 insertions(+)
> > >
> > > diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > > b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > > index 0f13c58..a308e14 100644
> > > --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > > +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > > @@ -3618,6 +3618,15 @@ void megasas_refire_mgmt_cmd(struct
> > > megasas_instance *instance)
> > >
> > > if (!smid)
> > > continue;
> > > +
> > > + /* Do not refire shutdown command */
> > > + if (le32_to_cpu(cmd_mfi->frame->dcmd.opcode) ==
> > > + MR_DCMD_CTRL_SHUTDOWN) {
> > > + cmd_mfi->frame->dcmd.cmd_status = MFI_STAT_OK;
> > > + megasas_complete_cmd(instance, cmd_mfi, DID_OK);
> > > + continue;
> > > + }
> > > +
> > > req_desc = megasas_get_request_descriptor
> > > (instance, smid - 1);
> > > refire_cmd = req_desc && ((cmd_mfi->frame->dcmd.opcode !=
> > >
> > Please, no.
> > You already have a selector further down whether to refire the
> > command, pending on the DRV_DCMD_SKIP_REFIRE flag.
> > If you always set this flag for the shutdown command you wouldn't need
> > to touch this at all.
> > And if you particularly dislike that solution convert the 'refire_cmd ='
> > statement into a proper switch and blank it out from there.
> >
>
> Hi Hannes,
> The management commands that get skipped further down in this function are
> internally generated by the driver itself so we don’t need to complete it
> back to
> the application.
> In case of shutdown DCMD, it’s an IOCTL command(issued by application) so
> we
> need to return status back to application.
> Combining handling of both the cases or using DRV_DCMD_SKIP_REFIRE will
> not
> be as straightforward.
>
> Thanks,
> Shivasharan
>
Hi Hannes,
We will implement the switch case approach during our next submission as
this
the current code is well tested internally and want to avoid regressions.
Are you ok if we keep it this way for now?
Thanks,
Shivasharan
> > Cheers,
> >
> > Hannes
> > --
> > Dr. Hannes ReineckeTeamlead Storage & Networking
> > h...@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)
RE: [PATCH 11/15] megaraid_sas: Set device queue_depth same as HBA can_queue value in scsi-mq mode
> -Original Message- > From: Hannes Reinecke [mailto:h...@suse.de] > Sent: Friday, June 30, 2017 7:10 PM > To: Shivasharan S; linux-scsi@vger.kernel.org > Cc: martin.peter...@oracle.com; the...@redhat.com; > j...@linux.vnet.ibm.com; kashyap.de...@broadcom.com; > sumit.sax...@broadcom.com; h...@suse.com; h...@lst.de > Subject: Re: [PATCH 11/15] megaraid_sas: Set device queue_depth same as > HBA > can_queue value in scsi-mq mode > > On 06/30/2017 10:30 AM, Shivasharan S wrote: > > Currently driver sets default queue_depth for VDs at 256 and JBODs > > based on interface type, ie., for SAS JBOD QD will be 64, for SATA JBOD > > QD will > be 32. > > During performance runs with scsi-mq enabled, we are seeing better > > results by setting QD same as HBA queue_depth. > > > > Signed-off-by: Kashyap Desai > > Signed-off-by: Shivasharan S > > --- > > drivers/scsi/megaraid/megaraid_sas_base.c | 6 +- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c > > b/drivers/scsi/megaraid/megaraid_sas_base.c > > index 0230929..c200f1a 100644 > > --- a/drivers/scsi/megaraid/megaraid_sas_base.c > > +++ b/drivers/scsi/megaraid/megaraid_sas_base.c > > @@ -1891,7 +1891,8 @@ static void > megasas_set_static_target_properties(struct scsi_device *sdev, > > if (instance->nvme_page_size && max_io_size_kb) > > megasas_set_nvme_device_properties(sdev, (max_io_size_kb > << 10)); > > > > - scsi_change_queue_depth(sdev, device_qd); > > + if (!shost_use_blk_mq(sdev->host)) > > + scsi_change_queue_depth(sdev, device_qd); > > > > } > > > > @@ -5914,6 +5915,9 @@ static int megasas_io_attach(struct > megasas_instance *instance) > > host->max_lun = MEGASAS_MAX_LUN; > > host->max_cmd_len = 16; > > > > + if (shost_use_blk_mq(host)) > > + host->cmd_per_lun = host->can_queue; > > + > > /* > > * Notify the mid-layer about the new controller > > */ > > > Is this bit really necessary? It will be adjusted by the above hunk, would > it not? > The default cmd_per_lun is now set to 256 (MEGASAS_DEFAULT_CMD_PER_LUN). In the above snip, we are changing cmd_per_lun to be equal to HBA queue_depth for multiqueue enabled case. So any scsi_device getting configured with scsi-mq enabled will have queue_depth set to cmd_per_lun value. The hunk in megasas_set_static_target_properties is to update the queue_depth for non-mq case only based on value provided by firmware. Thanks, Shivasharan > Cheers, > > Hannes > -- > Dr. Hannes Reinecke Teamlead Storage & Networking > h...@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)
RE: [PATCH 05/15] megaraid_sas: Do not re-fire shutdown DCMD after OCR
> -Original Message-
> From: Hannes Reinecke [mailto:h...@suse.de]
> Sent: Friday, June 30, 2017 7:00 PM
> To: Shivasharan S; linux-scsi@vger.kernel.org
> Cc: martin.peter...@oracle.com; the...@redhat.com;
> j...@linux.vnet.ibm.com; kashyap.de...@broadcom.com;
> sumit.sax...@broadcom.com; h...@suse.com; h...@lst.de
> Subject: Re: [PATCH 05/15] megaraid_sas: Do not re-fire shutdown DCMD
> after
> OCR
>
> On 06/30/2017 10:29 AM, Shivasharan S wrote:
> > Signed-off-by: Kashyap Desai
> > Signed-off-by: Shivasharan S
> > ---
> > drivers/scsi/megaraid/megaraid_sas_fusion.c | 9 +
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > index 0f13c58..a308e14 100644
> > --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > @@ -3618,6 +3618,15 @@ void megasas_refire_mgmt_cmd(struct
> > megasas_instance *instance)
> >
> > if (!smid)
> > continue;
> > +
> > + /* Do not refire shutdown command */
> > + if (le32_to_cpu(cmd_mfi->frame->dcmd.opcode) ==
> > + MR_DCMD_CTRL_SHUTDOWN) {
> > + cmd_mfi->frame->dcmd.cmd_status = MFI_STAT_OK;
> > + megasas_complete_cmd(instance, cmd_mfi, DID_OK);
> > + continue;
> > + }
> > +
> > req_desc = megasas_get_request_descriptor
> > (instance, smid - 1);
> > refire_cmd = req_desc && ((cmd_mfi->frame->dcmd.opcode !=
> >
> Please, no.
> You already have a selector further down whether to refire the command,
> pending on the DRV_DCMD_SKIP_REFIRE flag.
> If you always set this flag for the shutdown command you wouldn't need to
> touch this at all.
> And if you particularly dislike that solution convert the 'refire_cmd ='
> statement into a proper switch and blank it out from there.
>
Hi Hannes,
The management commands that get skipped further down in this function
are internally generated by the driver itself so we don’t need to complete
it
back to the application.
In case of shutdown DCMD, it’s an IOCTL command(issued by application)
so we need to return status back to application.
Combining handling of both the cases or using DRV_DCMD_SKIP_REFIRE
will not be as straightforward.
Thanks,
Shivasharan
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke Teamlead Storage & Networking
> h...@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)
RE: [PATCH 04/15] megaraid_sas: Call megasas_complete_cmd_dpc_fusion every 1 second while there are pending commands
> -Original Message- > From: Hannes Reinecke [mailto:h...@suse.de] > Sent: Friday, June 30, 2017 6:55 PM > To: Shivasharan S; linux-scsi@vger.kernel.org > Cc: martin.peter...@oracle.com; the...@redhat.com; > j...@linux.vnet.ibm.com; kashyap.de...@broadcom.com; > sumit.sax...@broadcom.com; h...@suse.com; h...@lst.de > Subject: Re: [PATCH 04/15] megaraid_sas: Call > megasas_complete_cmd_dpc_fusion every 1 second while there are pending > commands > > On 06/30/2017 10:29 AM, Shivasharan S wrote: > > Signed-off-by: Kashyap Desai > > Signed-off-by: Shivasharan S > > --- > > drivers/scsi/megaraid/megaraid_sas_fusion.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c > > b/drivers/scsi/megaraid/megaraid_sas_fusion.c > > index 5018a3f..0f13c58 100644 > > --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c > > +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c > > @@ -3552,6 +3552,7 @@ int megasas_wait_for_outstanding_fusion(struct > megasas_instance *instance, > > } > > } > > > > + megasas_complete_cmd_dpc_fusion((unsigned long)instance); > > outstanding = atomic_read(&instance->fw_outstanding); > > if (!outstanding) > > goto out; > > @@ -3560,8 +3561,6 @@ int megasas_wait_for_outstanding_fusion(struct > megasas_instance *instance, > > dev_notice(&instance->pdev->dev, "[%2d]waiting for > %d " > >"commands to complete for scsi%d\n", i, > >outstanding, instance->host->host_no); > > - megasas_complete_cmd_dpc_fusion( > > - (unsigned long)instance); > > } > > msleep(1000); > > } > > > Please add a changelog why this is necessary. > Sure Hannes. I will update the commit description with the details and send out v2 of the series. > Cheers, > > Hannes > -- > Dr. Hannes Reinecke Teamlead Storage & Networking > h...@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)
RE: [PATCH 09/15] megaraid_sas: use vmalloc for crash dump buffers and driver's local RAID map
> -Original Message-
> From: Tomas Henzl [mailto:the...@redhat.com]
> Sent: Friday, June 30, 2017 8:01 PM
> To: Shivasharan S; linux-scsi@vger.kernel.org
> Cc: martin.peter...@oracle.com; j...@linux.vnet.ibm.com;
> kashyap.de...@broadcom.com; sumit.sax...@broadcom.com;
> h...@suse.com; h...@lst.de
> Subject: Re: [PATCH 09/15] megaraid_sas: use vmalloc for crash dump
> buffers
> and driver's local RAID map
>
> On 30.6.2017 10:30, Shivasharan S wrote:
> > Signed-off-by: Kashyap Desai
> > Signed-off-by: Shivasharan S
> > ---
> > drivers/scsi/megaraid/megaraid_sas.h| 1 -
> > drivers/scsi/megaraid/megaraid_sas_base.c | 12 ++-
> > drivers/scsi/megaraid/megaraid_sas_fusion.c | 113
> > +---
> > 3 files changed, 80 insertions(+), 46 deletions(-)
> >
> > diff --git a/drivers/scsi/megaraid/megaraid_sas.h
> > b/drivers/scsi/megaraid/megaraid_sas.h
> > index 2b209bb..6d9f111 100644
> > --- a/drivers/scsi/megaraid/megaraid_sas.h
> > +++ b/drivers/scsi/megaraid/megaraid_sas.h
> > @@ -2115,7 +2115,6 @@ struct megasas_instance {
> > u32 *crash_dump_buf;
> > dma_addr_t crash_dump_h;
> > void *crash_buf[MAX_CRASH_DUMP_SIZE];
> > - u32 crash_buf_pages;
> > unsigned intfw_crash_buffer_size;
> > unsigned intfw_crash_state;
> > unsigned intfw_crash_buffer_offset;
> > diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c
> > b/drivers/scsi/megaraid/megaraid_sas_base.c
> > index e490272..c63ef88 100644
> > --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> > +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
> > @@ -49,6 +49,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include
> > #include
> > @@ -6672,9 +6673,14 @@ static void megasas_detach_one(struct pci_dev
> *pdev)
> > fusion->max_map_sz,
> > fusion->ld_map[i],
> > fusion->ld_map_phys[i]);
> > - if (fusion->ld_drv_map[i])
> > - free_pages((ulong)fusion->ld_drv_map[i],
> > - fusion->drv_map_pages);
> > + if (fusion->ld_drv_map[i]) {
> > + if (is_vmalloc_addr(fusion->ld_drv_map[i]))
> > + vfree(fusion->ld_drv_map[i]);
> > + else
> > + free_pages((ulong)fusion-
> >ld_drv_map[i],
> > + fusion->drv_map_pages);
> > + }
> > +
> > if (fusion->pd_seq_sync[i])
> > dma_free_coherent(&instance->pdev->dev,
> > pd_seq_map_sz,
> > diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > index c239762..2f5212d 100644
> > --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > @@ -1257,6 +1257,72 @@ megasas_display_intel_branding(struct
> > megasas_instance *instance) }
> >
> > /**
> > + * megasas_allocate_raid_maps -Allocate memory for RAID maps
> > + * @instance: Adapter soft state
> > + *
> > + * return: if success: return 0
> > + * failed: return -ENOMEM
> > + */
> > +static inline int megasas_allocate_raid_maps(struct megasas_instance
> > +*instance) {
> > + struct fusion_context *fusion;
> > + int i = 0;
> > +
> > + fusion = instance->ctrl_context;
> > +
> > + fusion->drv_map_pages = get_order(fusion->drv_map_sz);
> > +
> > + for (i = 0; i < 2; i++) {
> > + fusion->ld_map[i] = NULL;
> > +
> > + fusion->ld_drv_map[i] = (void *)
> > + __get_free_pages(__GFP_ZERO | GFP_KERNEL,
> > +fusion->drv_map_pages);
> > +
> > + if (!fusion->ld_drv_map[i]) {
> > + fusion->ld_drv_map[i] = vzalloc(fusion->drv_map_sz);
> > +
> > + if (!fusion->ld_drv_map[i]) {
> > + dev_err(&instance->pdev->dev,
> > + "Could not allocate memory for local
> map"
> > + " size requested: %d\n",
> > + fusion->drv_map_sz);
> > +
> > + if (fusion->ld_drv_map[0]) {
> > + if (is_vmalloc_addr(fusion-
> >ld_drv_map[0]))
> > + vfree(fusion->ld_drv_map[0]);
> > + else
> > + free_pages((ulong)fusion-
> >ld_drv_map[0],
> > + fusion-
> >drv_map_pages);
> > + }
> > + return -ENOMEM;
> > +
RE: [PATCH v3 19/39] megaraid_sas: MR_TargetIdToLdGet u8 to u16 and avoid invalid raid-map access
> -Original Message-
> From: Hannes Reinecke [mailto:h...@suse.com]
> Sent: Friday, February 10, 2017 5:05 PM
> To: Shivasharan S; linux-scsi@vger.kernel.org
> Cc: martin.peter...@oracle.com; the...@redhat.com;
> j...@linux.vnet.ibm.com; kashyap.de...@broadcom.com;
> sumit.sax...@broadcom.com
> Subject: Re: [PATCH v3 19/39] megaraid_sas: MR_TargetIdToLdGet u8 to u16
> and avoid invalid raid-map access
>
> On 02/10/2017 09:59 AM, Shivasharan S wrote:
> > Change MR_TargetIdToLdGet return type from u8 to u16.
> >
> > ld id range check is added at two places in this patch -
> > @megasas_build_ldio_fusion and @megasas_build_ld_nonrw_fusion.
> > Previous driver code used different data type for lds TargetId
returned from
> MR_TargetIdToLdGet.
> > Prior to this change, above two functions was safeguarded due to
> > function always return u8 and maximum value of ld id returned was 255.
> >
> > In below check, fw_supported_vd_count as of today is 64 or 256 and
> > valid range to support is either 0-63 or 0-255. Ideally want to
> > filter accessing raid map for ld ids which are not valid. With the u16
> > change, invalid ld id value is 0x and we will see kernel panic due
to random
> memory access in MR_LdRaidGet.
> > The changes will ensure we do not call MR_LdRaidGet if ld id is beyond
size of
> ldSpanMap array.
> >
> >if (ld < instance->fw_supported_vd_count)
> >
> > From firmware perspective,ld id 0xFF is invalid and even though
> > current driver code forward such command, firmware fails with target
not
> available.
> >
> > ld target id issue occurs mainly whenever driver loops to populate
raid map
> (ea. MR_ValidateMapInfo).
> > These are the only two places where we may see out of range target ids
> > and wants to protect raid map access based on range provided by
Firmware
> API.
> >
> > Signed-off-by: Shivasharan S
> > Signed-off-by: Kashyap Desai
> > ---
> >
> > fix in v2 - updated description content.
> >
> > drivers/scsi/megaraid/megaraid_sas.h| 2 +-
> > drivers/scsi/megaraid/megaraid_sas_fp.c | 5 +++--
> > drivers/scsi/megaraid/megaraid_sas_fusion.c | 25
> > ++---
> > 3 files changed, 18 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/scsi/megaraid/megaraid_sas.h
> > b/drivers/scsi/megaraid/megaraid_sas.h
> > index 0a20fff..efc01a3 100644
> > --- a/drivers/scsi/megaraid/megaraid_sas.h
> > +++ b/drivers/scsi/megaraid/megaraid_sas.h
> > @@ -2448,7 +2448,7 @@ MR_BuildRaidContext(struct megasas_instance
> *instance,
> > struct IO_REQUEST_INFO *io_info,
> > struct RAID_CONTEXT *pRAID_Context,
> > struct MR_DRV_RAID_MAP_ALL *map, u8 **raidLUN);
> > -u8 MR_TargetIdToLdGet(u32 ldTgtId, struct MR_DRV_RAID_MAP_ALL *map);
> > +u16 MR_TargetIdToLdGet(u32 ldTgtId, struct MR_DRV_RAID_MAP_ALL
> *map);
> > struct MR_LD_RAID *MR_LdRaidGet(u32 ld, struct MR_DRV_RAID_MAP_ALL
> > *map);
> > u16 MR_ArPdGet(u32 ar, u32 arm, struct MR_DRV_RAID_MAP_ALL *map);
> > u16 MR_LdSpanArrayGet(u32 ld, u32 span, struct MR_DRV_RAID_MAP_ALL
> > *map); diff --git a/drivers/scsi/megaraid/megaraid_sas_fp.c
> > b/drivers/scsi/megaraid/megaraid_sas_fp.c
> > index a0b0e68..9d5d485 100644
> > --- a/drivers/scsi/megaraid/megaraid_sas_fp.c
> > +++ b/drivers/scsi/megaraid/megaraid_sas_fp.c
> > @@ -165,7 +165,7 @@ u16 MR_GetLDTgtId(u32 ld, struct
> MR_DRV_RAID_MAP_ALL *map)
> > return le16_to_cpu(map->raidMap.ldSpanMap[ld].ldRaid.targetId);
> > }
> >
> > -u8 MR_TargetIdToLdGet(u32 ldTgtId, struct MR_DRV_RAID_MAP_ALL *map)
> > +u16 MR_TargetIdToLdGet(u32 ldTgtId, struct MR_DRV_RAID_MAP_ALL
> *map)
> > {
> > return map->raidMap.ldTgtIdToLd[ldTgtId];
> > }
> > @@ -1151,7 +1151,7 @@ MR_BuildRaidContext(struct megasas_instance
> > *instance, {
> > struct fusion_context *fusion;
> > struct MR_LD_RAID *raid;
> > - u32 ld, stripSize, stripe_mask;
> > + u32 stripSize, stripe_mask;
> > u64 endLba, endStrip, endRow, start_row, start_strip;
> > u64 regStart;
> > u32 regSize;
> > @@ -1163,6 +1163,7 @@ MR_BuildRaidContext(struct megasas_instance
> *instance,
> > u8 retval = 0;
> > u8 startlba_span = SPAN_INVALID;
> > u64 *pdBlock = &io_info->pdBlock;
> > + u16 ld;
> >
> > ldStartBlock = io_info->ldStartBlock;
> > numBlocks = io_info->numBlocks;
> > diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > index 9019b82..4aaf307 100644
> > --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > @@ -1121,7 +1121,8 @@ megasas_sync_map_info(struct megasas_instance
> *instance)
> > int i;
> > struct megasas_cmd *cmd;
> > struct megasas_dcmd_frame *dcmd;
> > - u32 size_sync_info, num_lds;
> > + u16 num_lds;
> > + u32 size_sync_info;
> > struct fusion_context *fusion;
> > struct MR_LD_TARGET
RE: [PATCH 18/39] megaraid_sas: MR_TargetIdToLdGet u8 to u16 and avoid invalid raid-map access
> -Original Message-
> From: Hannes Reinecke [mailto:h...@suse.com]
> Sent: Monday, February 06, 2017 4:51 PM
> To: Shivasharan S; linux-scsi@vger.kernel.org
> Cc: martin.peter...@oracle.com; the...@redhat.com;
> j...@linux.vnet.ibm.com; kashyap.de...@broadcom.com;
> sumit.sax...@broadcom.com
> Subject: Re: [PATCH 18/39] megaraid_sas: MR_TargetIdToLdGet u8 to u16
and
> avoid invalid raid-map access
>
> On 02/06/2017 10:59 AM, Shivasharan S wrote:
> > If MR_TargetIdToLdGet return >= 0xFF, it is invalid entry.
> > Consider that entry as invalid and do not access raid map for further
> operation.
> >
> > Signed-off-by: Shivasharan S
> > Signed-off-by: Kashyap Desai
> > ---
> > drivers/scsi/megaraid/megaraid_sas.h| 2 +-
> > drivers/scsi/megaraid/megaraid_sas_fp.c | 5 +++--
> > drivers/scsi/megaraid/megaraid_sas_fusion.c | 25
> > ++---
> > 3 files changed, 18 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/scsi/megaraid/megaraid_sas.h
> > b/drivers/scsi/megaraid/megaraid_sas.h
> > index f023e23..ec5f003 100644
> > --- a/drivers/scsi/megaraid/megaraid_sas.h
> > +++ b/drivers/scsi/megaraid/megaraid_sas.h
> > @@ -2448,7 +2448,7 @@ MR_BuildRaidContext(struct megasas_instance
> *instance,
> > struct IO_REQUEST_INFO *io_info,
> > struct RAID_CONTEXT *pRAID_Context,
> > struct MR_DRV_RAID_MAP_ALL *map, u8 **raidLUN);
> > -u8 MR_TargetIdToLdGet(u32 ldTgtId, struct MR_DRV_RAID_MAP_ALL *map);
> > +u16 MR_TargetIdToLdGet(u32 ldTgtId, struct MR_DRV_RAID_MAP_ALL
> *map);
> > struct MR_LD_RAID *MR_LdRaidGet(u32 ld, struct MR_DRV_RAID_MAP_ALL
> > *map);
> > u16 MR_ArPdGet(u32 ar, u32 arm, struct MR_DRV_RAID_MAP_ALL *map);
> > u16 MR_LdSpanArrayGet(u32 ld, u32 span, struct MR_DRV_RAID_MAP_ALL
> > *map); diff --git a/drivers/scsi/megaraid/megaraid_sas_fp.c
> > b/drivers/scsi/megaraid/megaraid_sas_fp.c
> > index a0b0e68..9d5d485 100644
> > --- a/drivers/scsi/megaraid/megaraid_sas_fp.c
> > +++ b/drivers/scsi/megaraid/megaraid_sas_fp.c
> > @@ -165,7 +165,7 @@ u16 MR_GetLDTgtId(u32 ld, struct
> MR_DRV_RAID_MAP_ALL *map)
> > return le16_to_cpu(map->raidMap.ldSpanMap[ld].ldRaid.targetId);
> > }
> >
> > -u8 MR_TargetIdToLdGet(u32 ldTgtId, struct MR_DRV_RAID_MAP_ALL *map)
> > +u16 MR_TargetIdToLdGet(u32 ldTgtId, struct MR_DRV_RAID_MAP_ALL
> *map)
> > {
> > return map->raidMap.ldTgtIdToLd[ldTgtId];
> > }
> > @@ -1151,7 +1151,7 @@ MR_BuildRaidContext(struct megasas_instance
> > *instance, {
> > struct fusion_context *fusion;
> > struct MR_LD_RAID *raid;
> > - u32 ld, stripSize, stripe_mask;
> > + u32 stripSize, stripe_mask;
> > u64 endLba, endStrip, endRow, start_row, start_strip;
> > u64 regStart;
> > u32 regSize;
> > @@ -1163,6 +1163,7 @@ MR_BuildRaidContext(struct megasas_instance
> *instance,
> > u8 retval = 0;
> > u8 startlba_span = SPAN_INVALID;
> > u64 *pdBlock = &io_info->pdBlock;
> > + u16 ld;
> >
> > ldStartBlock = io_info->ldStartBlock;
> > numBlocks = io_info->numBlocks;
> > diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > index 2f523f2..3d4d4b8 100644
> > --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > @@ -1120,7 +1120,8 @@ megasas_sync_map_info(struct megasas_instance
> *instance)
> > int i;
> > struct megasas_cmd *cmd;
> > struct megasas_dcmd_frame *dcmd;
> > - u32 size_sync_info, num_lds;
> > + u16 num_lds;
> > + u32 size_sync_info;
> > struct fusion_context *fusion;
> > struct MR_LD_TARGET_SYNC *ci = NULL;
> > struct MR_DRV_RAID_MAP_ALL *map;
> > @@ -1867,7 +1868,7 @@ megasas_set_pd_lba(struct
> MPI2_RAID_SCSI_IO_REQUEST *io_request, u8 cdb_len,
> >struct MR_DRV_RAID_MAP_ALL *local_map_ptr, u32 ref_tag)
> {
> > struct MR_LD_RAID *raid;
> > - u32 ld;
> > + u16 ld;
> > u64 start_blk = io_info->pdBlock;
> > u8 *cdb = io_request->CDB.CDB32;
> > u32 num_blocks = io_info->numBlocks; @@ -2300,10 +2301,11 @@
> > megasas_build_ldio_fusion(struct megasas_instance *instance,
> >
> > local_map_ptr = fusion->ld_drv_map[(instance->map_id & 1)];
> > ld = MR_TargetIdToLdGet(device_id, local_map_ptr);
> > - raid = MR_LdRaidGet(ld, local_map_ptr);
> >
> > - if ((MR_TargetIdToLdGet(device_id, local_map_ptr) >=
> > - instance->fw_supported_vd_count) ||
(!fusion->fast_path_io)) {
> > + if (ld < instance->fw_supported_vd_count)
> > + raid = MR_LdRaidGet(ld, local_map_ptr);
> > +
> > + if (!raid || (!fusion->fast_path_io)) {
> > io_request->RaidContext.raid_context.reg_lock_flags = 0;
> > fp_possible = false;
> > } else {
> > @@ -2475,12 +2477,12 @@ static void
> > megasas_build_ld_nonrw_fusion(struct megasas_instance *instance, {
> > u32 device_id;
> > s
RE: [PATCH 09/39] megaraid_sas: NVME Interface detection and prop settings
> -Original Message-
> From: Hannes Reinecke [mailto:h...@suse.com]
> Sent: Monday, February 06, 2017 4:17 PM
> To: Shivasharan S; linux-scsi@vger.kernel.org
> Cc: martin.peter...@oracle.com; the...@redhat.com;
> j...@linux.vnet.ibm.com; kashyap.de...@broadcom.com;
> sumit.sax...@broadcom.com
> Subject: Re: [PATCH 09/39] megaraid_sas: NVME Interface detection and
prop
> settings
>
> On 02/06/2017 10:59 AM, Shivasharan S wrote:
> > New functionality
> > Adding detection logic for NVME device attached behind Ventura
controller.
> > Driver set HostPageSize in IOC_INIT frame to inform about page size
for NVME
> devices.
> > Firmware reports NVME page size to the driver.
> > PD INFO DCMD provide new interface type NVME_PD. Driver set property
of
> NVME device.
> >
> > Signed-off-by: Shivasharan S
> > Signed-off-by: Kashyap Desai
> > ---
> > drivers/scsi/megaraid/megaraid_sas.h| 23 ++--
> > drivers/scsi/megaraid/megaraid_sas_base.c | 170
--
> --
> > drivers/scsi/megaraid/megaraid_sas_fusion.c | 6 +-
> > drivers/scsi/megaraid/megaraid_sas_fusion.h | 2 +-
> > 4 files changed, 142 insertions(+), 59 deletions(-)
> >
> [ .. ]
> > @@ -3983,18 +4037,22 @@ dcmd_timeout_ocr_possible(struct
> megasas_instance *instance) {
> > return INITIATE_OCR;
> > }
> >
> > -static int
> > -megasas_get_pd_info(struct megasas_instance *instance, u16 device_id)
> > +static void
> > +megasas_get_pd_info(struct megasas_instance *instance, struct
> > +scsi_device *sdev)
> > {
> > int ret;
> > struct megasas_cmd *cmd;
> > struct megasas_dcmd_frame *dcmd;
> >
> > + struct MR_PRIV_DEVICE *mr_device_priv_data;
> > + u16 device_id = 0;
> > +
> > + device_id = (sdev->channel * MEGASAS_MAX_DEV_PER_CHANNEL) +
> > +sdev->id;
> > cmd = megasas_get_cmd(instance);
> >
> > if (!cmd) {
> > dev_err(&instance->pdev->dev, "Failed to get cmd %s\n",
> __func__);
> > - return -ENOMEM;
> > + return;
> > }
> >
> > dcmd = &cmd->frame->dcmd;
> > @@ -4021,7 +4079,9 @@ megasas_get_pd_info(struct megasas_instance
> > *instance, u16 device_id)
> >
> > switch (ret) {
> > case DCMD_SUCCESS:
> > - instance->pd_list[device_id].interface =
> > + mr_device_priv_data = sdev->hostdata;
> > + le16_to_cpus((u16 *)&instance->pd_info->state.ddf.pdType);
> > + mr_device_priv_data->interface_type =
> > instance->pd_info->state.ddf.pdType.intf;
> > break;
> >
> > @@ -4048,7 +4108,7 @@ megasas_get_pd_info(struct megasas_instance
> *instance, u16 device_id)
> > if (ret != DCMD_TIMEOUT)
> > megasas_return_cmd(instance, cmd);
> >
> > - return ret;
> > + return;
> > }
> > /*
> > * megasas_get_pd_list_info - Returns FW's pd_list structure
> Please don't do this.
> There's a valid reason why there is return value in the first place (hot
removal of
> devices IIRC), and it's always good manners to provide for an error path
instead
> of assuming that everything will be fine.
megasas_get_pd_info is called to get "interface type" (SAS, SATA or NVME)
of non-RAID disks. The interface type is used in IO path (applicable only
for
NVME based non-RAID disks) and also used for setting the device queue
depth.
Even if this function returns some error, driver is not going to do
anything but
continue in low performance mode (for NVME based non-RAID disks driver
will
not form native PRPs and device QD will be set to default value).
>
> And, incidentally, this has nothing to do with the NVMe backend changes,
right?
The "interface type" tells the driver whether a drive is NVME based or
not. And
if NVME, then we form PRPs.
Thanks,
Shivasharan
>
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke zSeries & Storage
> h...@suse.com+49 911 74053 688
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton HRB 21284 (AG
> Nürnberg)
RE: [PATCH 08/39] megaraid_sas: megasas_get_request_descriptor always return valid desc
> -Original Message-
> From: Hannes Reinecke [mailto:h...@suse.com]
> Sent: Monday, February 06, 2017 4:14 PM
> To: Shivasharan S; linux-scsi@vger.kernel.org
> Cc: martin.peter...@oracle.com; the...@redhat.com;
> j...@linux.vnet.ibm.com; kashyap.de...@broadcom.com;
> sumit.sax...@broadcom.com
> Subject: Re: [PATCH 08/39] megaraid_sas: megasas_get_request_descriptor
> always return valid desc
>
> On 02/06/2017 10:59 AM, Shivasharan S wrote:
> > No functional change. Code clean up. Removing error code which is not
> > valid scenario.
> >
> > Signed-off-by: Shivasharan S
> > Signed-off-by: Kashyap Desai
> > ---
> > drivers/scsi/megaraid/megaraid_sas_base.c | 4 +---
> > drivers/scsi/megaraid/megaraid_sas_fusion.c | 33
> > +
> > 2 files changed, 6 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c
> > b/drivers/scsi/megaraid/megaraid_sas_base.c
> > index 80fcdf5..138d028 100644
> > --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> > +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
> > @@ -5602,9 +5602,7 @@ megasas_register_aen(struct megasas_instance
> *instance, u32 seq_num,
> > /*
> > * Issue the aen registration frame
> > */
> > - instance->instancet->issue_dcmd(instance, cmd);
> > -
> > - return 0;
> > + return instance->instancet->issue_dcmd(instance, cmd);
> > }
> >
> > /**
> > diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > index d25268a..1ec482e 100644
> > --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > @@ -974,8 +974,7 @@ megasas_sync_pd_seq_num(struct megasas_instance
> *instance, bool pend) {
> > dcmd->mbox.b[0] = MEGASAS_DCMD_MBOX_PEND_FLAG;
> > dcmd->flags = cpu_to_le16(MFI_FRAME_DIR_WRITE);
> > instance->jbod_seq_cmd = cmd;
> > - instance->instancet->issue_dcmd(instance, cmd);
> > - return 0;
> > + return instance->instancet->issue_dcmd(instance, cmd);
> > }
> >
> > dcmd->flags = cpu_to_le16(MFI_FRAME_DIR_READ); @@ -1115,7
> +1114,7 @@
> > megasas_get_map_info(struct megasas_instance *instance) int
> > megasas_sync_map_info(struct megasas_instance *instance) {
> > - int ret = 0, i;
> > + int i;
> > struct megasas_cmd *cmd;
> > struct megasas_dcmd_frame *dcmd;
> > u32 size_sync_info, num_lds;
> > @@ -1182,9 +1181,7 @@ megasas_sync_map_info(struct megasas_instance
> > *instance)
> >
> > instance->map_update_cmd = cmd;
> >
> > - instance->instancet->issue_dcmd(instance, cmd);
> > -
> > - return ret;
> > + return instance->instancet->issue_dcmd(instance, cmd);
> > }
> >
> > /*
> > @@ -2438,18 +2435,12 @@ megasas_build_io_fusion(struct
> megasas_instance *instance,
> > return 0;
> > }
> >
> > -union MEGASAS_REQUEST_DESCRIPTOR_UNION *
> > +static union MEGASAS_REQUEST_DESCRIPTOR_UNION *
> > megasas_get_request_descriptor(struct megasas_instance *instance, u16
> > index) {
> > u8 *p;
> > struct fusion_context *fusion;
> >
> > - if (index >= instance->max_mpt_cmds) {
> > - dev_err(&instance->pdev->dev, "Invalid SMID (0x%x)request
for
> "
> > - "descriptor for scsi%d\n", index,
> > - instance->host->host_no);
> > - return NULL;
> > - }
> > fusion = instance->ctrl_context;
> > p = fusion->req_frames_desc +
> > sizeof(union MEGASAS_REQUEST_DESCRIPTOR_UNION) *
> index; @@ -2959,7
> > +2950,7 @@ build_mpt_mfi_pass_thru(struct megasas_instance *instance,
> > union MEGASAS_REQUEST_DESCRIPTOR_UNION * build_mpt_cmd(struct
> > megasas_instance *instance, struct megasas_cmd *cmd) {
> > - union MEGASAS_REQUEST_DESCRIPTOR_UNION *req_desc;
> > + union MEGASAS_REQUEST_DESCRIPTOR_UNION *req_desc = NULL;
> > u16 index;
> >
> > if (build_mpt_mfi_pass_thru(instance, cmd)) { @@ -2971,9 +2962,6
@@
> > build_mpt_cmd(struct megasas_instance *instance, struct megasas_cmd
> > *cmd)
> >
> > req_desc = megasas_get_request_descriptor(instance, index - 1);
> >
> > - if (!req_desc)
> > - return NULL;
> > -
> > req_desc->Words = 0;
> > req_desc->SCSIIO.RequestFlags =
> (MPI2_REQ_DESCRIPT_FLAGS_SCSI_IO <<
> >
> MEGASAS_REQ_DESCRIPT_FLAGS_TYPE_SHIFT);
> > @@ -2996,11 +2984,6 @@ megasas_issue_dcmd_fusion(struct
> megasas_instance *instance,
> > union MEGASAS_REQUEST_DESCRIPTOR_UNION *req_desc;
> >
> > req_desc = build_mpt_cmd(instance, cmd);
> > - if (!req_desc) {
> > - dev_info(&instance->pdev->dev, "Failed from %s %d\n",
> > - __func__, __LINE__);
> > - return DCMD_NOT_FIRED;
> > - }
> >
> > megasas_fire_cmd_fusion(instance, req_desc);
> > return DCMD_SUCCESS;
> > @@ -3437,12 +3420,6 @@ megasas_issue_tm(struct megasas_instance
> > *instance, u16 device_handle,
> >
> > req_desc = megasas_get_request_des
RE: [PATCH 06/39] megaraid_sas: RAID map is accessed for SYS PDs when use_seqnum_jbod_fp is not set
> -Original Message-
> From: Hannes Reinecke [mailto:h...@suse.com]
> Sent: Monday, February 06, 2017 4:11 PM
> To: Shivasharan S; linux-scsi@vger.kernel.org
> Cc: martin.peter...@oracle.com; the...@redhat.com;
> j...@linux.vnet.ibm.com; kashyap.de...@broadcom.com;
> sumit.sax...@broadcom.com
> Subject: Re: [PATCH 06/39] megaraid_sas: RAID map is accessed for SYS
PDs
> when use_seqnum_jbod_fp is not set
>
> On 02/06/2017 10:59 AM, Shivasharan S wrote:
> > Signed-off-by: Shivasharan S
> > Signed-off-by: Kashyap Desai
> > ---
> > drivers/scsi/megaraid/megaraid_sas_base.c | 25
> > ++---
> > 1 file changed, 14 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c
> > b/drivers/scsi/megaraid/megaraid_sas_base.c
> > index 6ca49ef..67a205a 100644
> > --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> > +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
> > @@ -1756,28 +1756,31 @@ void megasas_update_sdev_properties(struct
> scsi_device *sdev)
> > fusion = instance->ctrl_context;
> > mr_device_priv_data = sdev->hostdata;
> >
> > - if (!fusion)
> > + if (!fusion || !mr_device_priv_data)
> > return;
> >
> > - if (!MEGASAS_IS_LOGICAL(sdev) &&
> > - instance->use_seqnum_jbod_fp) {
> > - pd_index = (sdev->channel *
> MEGASAS_MAX_DEV_PER_CHANNEL) +
> > - sdev->id;
> > - pd_sync = (void *)fusion->pd_seq_sync
> > - [(instance->pd_seq_map_id - 1) & 1];
> > - mr_device_priv_data->is_tm_capable =
> > - pd_sync->seq[pd_index].capability.tmCapable;
> > - } else {
> > + if (MEGASAS_IS_LOGICAL(sdev)) {
> > device_id = ((sdev->channel % 2) *
> MEGASAS_MAX_DEV_PER_CHANNEL)
> > + sdev->id;
> > local_map_ptr = fusion->ld_drv_map[(instance->map_id &
1)];
> > ld = MR_TargetIdToLdGet(device_id, local_map_ptr);
> > + if (ld >= instance->fw_supported_vd_count)
> > + return;
> > raid = MR_LdRaidGet(ld, local_map_ptr);
> >
> > if (raid->capability.ldPiMode ==
> MR_PROT_INFO_TYPE_CONTROLLER)
> > - blk_queue_update_dma_alignment(sdev->request_queue, 0x7);
> > + blk_queue_update_dma_alignment(sdev-
> >request_queue,
> > + 0x7);
> > +
> > mr_device_priv_data->is_tm_capable =
> > raid->capability.tmCapable;
> > + } else if (instance->use_seqnum_jbod_fp) {
> > + pd_index = (sdev->channel *
> MEGASAS_MAX_DEV_PER_CHANNEL) +
> > + sdev->id;
> > + pd_sync = (void *)fusion->pd_seq_sync
> > + [(instance->pd_seq_map_id - 1) & 1];
> > + mr_device_priv_data->is_tm_capable =
> > +
pd_sync->seq[pd_index].capability.tmCapable;
> > }
> > }
> >
> >
> Don't you need a sanity check on pd_index, too?
> Otherwise you easily overflow the ->seq array ...
"pd_index" sanity check is not really required here. pd_index is function
of "sdev->id", "sdev->channel" and MEGASAS_MAX_DEV_PER_CHANNEL.
Here sdev->channel can either be 0 or 1 as it is under
"!MEGASAS_IS_LOGICAL(sdev)", and sdev->id cannot be greater than
127(MEGASAS_MAX_DEV_PER_CHANNEL -1).
So maximum possible value of pd_index is 255(256th element) and
fusion->pd_seq_sync->seq array size is 256.
So pd_index should not overflow fusion->pd_seq_sync->seq array.
>
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke zSeries & Storage
> h...@suse.com+49 911 74053 688
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton HRB 21284 (AG
> Nürnberg)
RE: [PATCH 04/39] megaraid_sas: 32 bit descriptor fire cmd optimization
> -Original Message- > From: Hannes Reinecke [mailto:h...@suse.com] > Sent: Monday, February 06, 2017 3:54 PM > To: Shivasharan S; linux-scsi@vger.kernel.org > Cc: martin.peter...@oracle.com; the...@redhat.com; > j...@linux.vnet.ibm.com; kashyap.de...@broadcom.com; > sumit.sax...@broadcom.com > Subject: Re: [PATCH 04/39] megaraid_sas: 32 bit descriptor fire cmd > optimization > > On 02/06/2017 10:59 AM, Shivasharan S wrote: > > No functional change. Code refactor. > > megasas_fire_cmd_fusion can always use 32 bit descriptor write for ventura. > No need to pass extra flag. > > Only IOC INIT required 64 bit Descriptor write. > > > > Signed-off-by: Shivasharan S > > Signed-off-by: Kashyap Desai > > --- > > drivers/scsi/megaraid/megaraid_sas_fusion.c | 65 > > +++-- > > 1 file changed, 33 insertions(+), 32 deletions(-) > > > [ .. ] > > @@ -897,7 +893,14 @@ megasas_ioc_init_fusion(struct megasas_instance > *instance) > > break; > > } > > > > - megasas_fire_cmd_fusion(instance, &req_desc, false); > > + /* For Ventura also IOC INIT required 64 bit Descriptor write. */ > > + spin_lock_irqsave(&instance->hba_lock, flags); > > + writel(le32_to_cpu(req_desc.u.low), > > + &instance->reg_set->inbound_low_queue_port); > > + writel(le32_to_cpu(req_desc.u.high), > > + &instance->reg_set->inbound_high_queue_port); > > + mmiowb(); > > + spin_unlock_irqrestore(&instance->hba_lock, flags); > > > > wait_and_poll(instance, cmd, MFI_POLL_TIMEOUT_SECS); > > > You are missing the 64 bit writeq improvements here. We have to use writeq() with additional checks " #if defined(writeq) && defined(CONFIG_64BIT)" as some arch still wants two PCI write as they do not have writeq() support. Since this is a one-time IOC INIT operation we should be good to keep simple code avoiding writeq() usage. > > Cheers, > > Hannes > -- > Dr. Hannes Reinecke zSeries & Storage > h...@suse.com+49 911 74053 688 > SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg > GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton HRB 21284 (AG > Nürnberg)
RE: [PATCH 04/39] megaraid_sas: 32 bit descriptor fire cmd optimization
> -Original Message- > From: Hannes Reinecke [mailto:h...@suse.com] > Sent: Monday, February 06, 2017 3:54 PM > To: Shivasharan S; linux-scsi@vger.kernel.org > Cc: martin.peter...@oracle.com; the...@redhat.com; > j...@linux.vnet.ibm.com; kashyap.de...@broadcom.com; > sumit.sax...@broadcom.com > Subject: Re: [PATCH 04/39] megaraid_sas: 32 bit descriptor fire cmd > optimization > > On 02/06/2017 10:59 AM, Shivasharan S wrote: > > No functional change. Code refactor. > > megasas_fire_cmd_fusion can always use 32 bit descriptor write for ventura. > No need to pass extra flag. > > Only IOC INIT required 64 bit Descriptor write. > > > > Signed-off-by: Shivasharan S > > Signed-off-by: Kashyap Desai > > --- > > drivers/scsi/megaraid/megaraid_sas_fusion.c | 65 > > +++-- > > 1 file changed, 33 insertions(+), 32 deletions(-) > > > [ .. ] > > @@ -897,7 +893,14 @@ megasas_ioc_init_fusion(struct megasas_instance > *instance) > > break; > > } > > > > - megasas_fire_cmd_fusion(instance, &req_desc, false); > > + /* For Ventura also IOC INIT required 64 bit Descriptor write. */ > > + spin_lock_irqsave(&instance->hba_lock, flags); > > + writel(le32_to_cpu(req_desc.u.low), > > + &instance->reg_set->inbound_low_queue_port); > > + writel(le32_to_cpu(req_desc.u.high), > > + &instance->reg_set->inbound_high_queue_port); > > + mmiowb(); > > + spin_unlock_irqrestore(&instance->hba_lock, flags); > > > > wait_and_poll(instance, cmd, MFI_POLL_TIMEOUT_SECS); > > > You are missing the 64 bit writeq improvements here. We have to use writeq() with additional checks " #if defined(writeq) && defined(CONFIG_64BIT)" as some arch still wants two PCI write as they do not have writeq() support. Since this is a one-time IOC INIT operation we should be good to keep simple code avoiding writeq() usage. > > Cheers, > > Hannes > -- > Dr. Hannes Reinecke zSeries & Storage > h...@suse.com+49 911 74053 688 > SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg > GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton HRB 21284 (AG > Nürnberg)
RE: [PATCH 11/39] megaraid_sas: NVME interface target prop added
> -Original Message- > From: Shivasharan S [mailto:shivasharan.srikanteshw...@broadcom.com] > Sent: Monday, February 06, 2017 3:30 PM > To: linux-scsi@vger.kernel.org > Cc: martin.peter...@oracle.com; the...@redhat.com; > j...@linux.vnet.ibm.com; kashyap.de...@broadcom.com; > sumit.sax...@broadcom.com; h...@suse.com; Shivasharan S > Subject: [PATCH 11/39] megaraid_sas: NVME interface target prop added > > This patch depends on patch 0008. > This patch fetch true values of NVME property from FW using New DCMD > interface MR_DCMD_DEV_GET_TARGET_PROP Correction: By mistake I have sent this patch with the commit description of patch 10. See below the updated title and description: megaraid_sas: NVME fast path io support This patch depends on patch 0008, 0009. This patch provide true fast path IO support. Driver creates PRP for NVME drives and send Fast Path for performance. Certain h/w requirement needs to be taken care in driver. I will resend this patch with above changes. Waiting for review to be completed and accumulate all the feedback. > > Signed-off-by: Shivasharan S > Signed-off-by: Kashyap Desai > ---
RE: [PATCH 11/39] megaraid_sas: NVME interface target prop added
> -Original Message- > From: Shivasharan S [mailto:shivasharan.srikanteshw...@broadcom.com] > Sent: Monday, February 06, 2017 3:30 PM > To: linux-scsi@vger.kernel.org > Cc: martin.peter...@oracle.com; the...@redhat.com; > j...@linux.vnet.ibm.com; kashyap.de...@broadcom.com; > sumit.sax...@broadcom.com; h...@suse.com; Shivasharan S > Subject: [PATCH 11/39] megaraid_sas: NVME interface target prop added > > This patch depends on patch 0008. Correction: This patch depends on patch 09: "megaraid_sas: NVME Interface detection and prop settings" > This patch fetch true values of NVME property from FW using New DCMD > interface MR_DCMD_DEV_GET_TARGET_PROP > > Signed-off-by: Shivasharan S > Signed-off-by: Kashyap Desai > ---
RE: [PATCH 10/39] megaraid_sas: NVME interface target prop added
> -Original Message- > From: Shivasharan S [mailto:shivasharan.srikanteshw...@broadcom.com] > Sent: Monday, February 06, 2017 3:30 PM > To: linux-scsi@vger.kernel.org > Cc: martin.peter...@oracle.com; the...@redhat.com; > j...@linux.vnet.ibm.com; kashyap.de...@broadcom.com; > sumit.sax...@broadcom.com; h...@suse.com; Shivasharan S > Subject: [PATCH 10/39] megaraid_sas: NVME interface target prop added > > This patch depends on patch 0008. Correction: This patch depends on patch 09: "megaraid_sas: NVME Interface detection and prop settings" > This patch fetch true values of NVME property from FW using New DCMD > interface MR_DCMD_DEV_GET_TARGET_PROP > > Signed-off-by: Shivasharan S > Signed-off-by: Kashyap Desai > ---
RE: [PATCH 01/39] Revert "scsi: megaraid_sas: Enable or Disable Fast path based on the PCI Threshold Bandwidth"
> -Original Message-
> From: Shivasharan S [mailto:shivasharan.srikanteshw...@broadcom.com]
> Sent: Monday, February 06, 2017 3:30 PM
> To: linux-scsi@vger.kernel.org
> Cc: martin.peter...@oracle.com; the...@redhat.com;
> j...@linux.vnet.ibm.com; kashyap.de...@broadcom.com;
> sumit.sax...@broadcom.com; h...@suse.com; Shivasharan S
> Subject: [PATCH 01/39] Revert "scsi: megaraid_sas: Enable or Disable
Fast path
> based on the PCI Threshold Bandwidth"
>
> This reverts
> commit "3e5eadb1a881" ("scsi: megaraid_sas: Enable or Disable Fast path
based
> on the PCI Threshold Bandwidth")
>
> This patch was aimed to increase performance of R1 Write operation for
large
> IO size.
> Since this method used timer approach, it turn on/off fast path did not
work as
> expected.
> Patch 0011 describes new algorithm and performance number.
Correction: Please refer patch #0012 - "megaraid_sas: raid 1 write
performance for large io"
>
> Signed-off-by: Shivasharan S
> Signed-off-by: Kashyap Desai
> ---
