> -----Original Message----- > From: Martin K. Petersen [mailto:martin.petersen@xxxxxxxxxx] > Sent: Wednesday, August 12, 2015 6:59 AM > To: Sumit.Saxena@xxxxxxxxxxxxx > Cc: linux-scsi@xxxxxxxxxxxxxxx; thenzl@xxxxxxxxxx; > martin.petersen@xxxxxxxxxx; hch@xxxxxxxxxxxxx; jbottomley@xxxxxxxxxxxxx; > kashyap.desai@xxxxxxxxxxxxx; kiran-kumar.kasturi@xxxxxxxxxxxxx; > uday.lingala@xxxxxxxxxxxxx > Subject: Re: [PATCH 1/7] megaraid_sas : Jbod sequence number support > > > Sumit, > > @@ -973,7 +973,12 @@ struct megasas_ctrl_info { > > struct { > #if defined(__BIG_ENDIAN_BITFIELD) > - u32 reserved:12; > + u32 reserved:7; > + u32 useSeqNumJbodFP:1; > + u32 supportExtendedSSCSize:1; > + u32 supportDiskCacheSettingForSysPDs:1; > + u32 supportCPLDUpdate:1; > + u32 supportTTYLogCompression:1; > u32 discardCacheDuringLDDelete:1; > u32 supportSecurityonJBOD:1; > u32 supportCacheBypassModes:1; > @@ -1013,7 +1018,12 @@ struct megasas_ctrl_info { > u32 supportCacheBypassModes:1; > u32 supportSecurityonJBOD:1; > u32 discardCacheDuringLDDelete:1; > - u32 reserved:12; > + u32 supportTTYLogCompression:1; > + u32 supportCPLDUpdate:1; > + u32 supportDiskCacheSettingForSysPDs:1; > + u32 supportExtendedSSCSize:1; > + u32 useSeqNumJbodFP:1; > + u32 reserved:7; > #endif > } adapterOperations3; > > Looks like most of these new flags should be in a separate patch. I will create separate patch for these new flags which are not related to JBOD sequence support. These flags are created to keep APIs in sync across driver and firmware. > > @@ -4482,6 +4506,62 @@ megasas_destroy_irqs(struct megasas_instance > *instance) { } > > /** > + * megasas_setup_jbod_map - setup jbod map for FP seq_number. > + * @instance: Adapter soft state > + * @is_probe: Driver probe check > + * > + * Return 0 on success. > + */ > +void > +megasas_setup_jbod_map(struct megasas_instance *instance) { > + int i; > + struct fusion_context *fusion = instance->ctrl_context; > + u32 pd_seq_map_sz; > + > + pd_seq_map_sz = sizeof(struct MR_PD_CFG_SEQ_NUM_SYNC) + > + (sizeof(struct MR_PD_CFG_SEQ) * (MAX_PHYSICAL_DEVICES - > 1)); > > Why -1 here? Presumably MAX_PHYSICAL_DEVICES is a count, not an index. Yes it is count only. struct MR_PD_CFG_SEQ_NUM_SYNC has "struct MR_PD_CFG_SEQ" as one of its member so struct MR_PD_CFG_SEQ corresponding to PD index "0" is embedded inside struct MR_PD_CFG_SEQ_NUM_SYNC. That's why -1 here. > > + > + if (reset_devices || !fusion || > + !instance->ctrl_info->adapterOperations3.useSeqNumJbodFP) { > + dev_info(&instance->pdev->dev, > + "Jbod map is not supported %s %d\n", > + __func__, __LINE__); > + instance->use_seqnum_jbod_fp = 0; > > Nitpick: It's a bool so use false. Occurs throughout the patch. Agree, will do this. > > + return; > + } > + > + if (fusion->pd_seq_sync[0]) > + goto skip_alloc; > + > + for (i = 0; i < 2; i++) { > > Magic number. See comment below. > > @@ -5510,10 +5594,16 @@ static void megasas_shutdown_controller(struct > megasas_instance *instance, > > if (instance->aen_cmd) > megasas_issue_blocked_abort_cmd(instance, > - instance->aen_cmd, 30); > + instance->aen_cmd, > + MEGASAS_BLOCKED_CMD_TIMEOUT); > if (instance->map_update_cmd) > megasas_issue_blocked_abort_cmd(instance, > - instance->map_update_cmd, 30); > + instance->map_update_cmd, > + MEGASAS_BLOCKED_CMD_TIMEOUT); > + if (instance->jbod_seq_cmd) > + megasas_issue_blocked_abort_cmd(instance, > + instance->jbod_seq_cmd, > + MEGASAS_BLOCKED_CMD_TIMEOUT); > dcmd = &cmd->frame->dcmd; > > Nice cleanup but ideally those non-jbod timeouts would have been in a separate > patch. Please make sure your patches only do what they purport to do. Agree, will create separate patch. > > > memset(dcmd->mbox.b, 0, MFI_MBOX_SIZE); @@ -5648,6 +5738,7 @@ > megasas_resume(struct pci_dev *pdev) > } > if (!megasas_get_map_info(instance)) > megasas_sync_map_info(instance); > + > } > break; > default: > > Whitespace snafu. Oops.. My bad. Will work on it. > > @@ -5781,6 +5874,9 @@ static void megasas_detach_one(struct pci_dev > *pdev) > case PCI_DEVICE_ID_LSI_INVADER: > case PCI_DEVICE_ID_LSI_FURY: > megasas_release_fusion(instance); > + pd_seq_map_sz = sizeof(struct > MR_PD_CFG_SEQ_NUM_SYNC) + > + (sizeof(struct MR_PD_CFG_SEQ) * > + (MAX_PHYSICAL_DEVICES - 1)); > for (i = 0; i < 2 ; i++) { > if (fusion->ld_map[i]) > dma_free_coherent(&instance->pdev->dev, > > Minus one again. Explained above. > > + pd_sync = (void *)fusion->pd_seq_sync[(instance->pd_seq_map_id & > 1)]; > + pd_seq_h = fusion->pd_seq_phys[(instance->pd_seq_map_id & 1)]; > + pd_seq_map_sz = sizeof(struct MR_PD_CFG_SEQ_NUM_SYNC) + > + (sizeof(struct MR_PD_CFG_SEQ) * > + (MAX_PHYSICAL_DEVICES - 1)); > > Patch description could do with a description of the data structures you set up. Had given high level description of what patch does. Will rework to provide description of data structures used in patch as well. > > > + /*Want to send all IO via FW path*/ > > Comment needs spaces. I will work on this. > > @@ -828,6 +844,8 @@ struct fusion_context { > u32 current_map_sz; > u32 drv_map_sz; > u32 drv_map_pages; > + struct MR_PD_CFG_SEQ_NUM_SYNC *pd_seq_sync[2]; > + dma_addr_t pd_seq_phys[2]; > > Please define a suitable constant for 2 and use it in the code. Sure, will use a suitable macro for 2 and resend patch. > > -- > Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
