All of lore.kernel.org
 help / color / mirror / Atom feed
* RE: [PATCH v2 4/7] megaraid_sas: Send SYNCHRONIZE_CACHE command to firmware
@ 2016-10-21 12:08 Kashyap Desai
  2016-10-21 12:36 ` James Bottomley
  0 siblings, 1 reply; 7+ messages in thread
From: Kashyap Desai @ 2016-10-21 12:08 UTC (permalink / raw)
  To: James Bottomley, Sumit Saxena, linux-scsi; +Cc: martin.petersen, thenzl, stable

> -----Original Message-----
> From: James Bottomley [mailto:jejb@linux.vnet.ibm.com]
> Sent: Friday, October 21, 2016 4:32 PM
> To: Sumit Saxena; linux-scsi@vger.kernel.org
> Cc: martin.petersen@oracle.com; thenzl@redhat.com;
> kashyap.desai@broadcom.com; stable@vger.kernel.org
> Subject: Re: [PATCH v2 4/7] megaraid_sas: Send SYNCHRONIZE_CACHE
> command to firmware
>
> On Thu, 2016-10-20 at 02:05 -0700, Sumit Saxena wrote:
> > From previous patch we have below changes in v2 - 1.  Updated change
> > log.  Provided more detail in change log.
> > 2.  Agreed to remove module parameter. If we remove module parameter,
> > we
> >     can ask customer to disable WCE on drive to get similar impact.
> > 3.  Always Send SYNCHRONIZE_CACHE  for JBOD (non Raid) Device to
> > Firmware.
> >
> > Current megaraid_sas driver returns SYNCHRONIZE_CACHE(related to Drive
> > Cache)  command  back to SCSI layer without sending it down to
> > firmware as firmware supposed to take care of flushing disk cache for
> > all drives belongs to Virtual Disk at the time of system
> > reboot/shutdown.
> >
> > We evaluate and understood that for Raid Volume, why driver should not
> > send SYNC_CACHE command to the Firmware.
> > There may be a some reason in past, but now it looks to be not
> > required and we have fixed this issue as part of this patch.
> >
> > Commit- " 02b01e0 [SCSI] megaraid_sas: return sync cache call with
> > success"
> > added the code in driver to return SYNCHRONIZE_CACHE without sending
> > it to firmware back in 2007. Earlier MR was mainly for Virtual Disk,
> > so same code continue for JBOD as well whenever JBOD support was added
> > and it introduced bug that SYNCHRONIZE_CACHE is not passed to FW for
> > JBOD (non Raid disk).
> >
> > But our recent analysis indicates that, From Day-1 MR Firmware always
> > expect Driver to forward SYNCHRONIZE_CACHE for JBOD (non Raid disk) to
> > the Firmware.
> > We have fixed this as part of this patch.
> >
> >  - Additional background -
> > There are some instance of MegaRaid FW (E.a Gen2 and Gen2.5 FW) set
> > WCE bit for Virtual Disk but firmware does not reply correct status
> > for SYNCH_CACHE.
> > It is very difficult to find out which Device ID and firmware has
> > capability to manage SYNC_CACHE, so we managed to figure out which are
> > the new firmware (canHandleSyncCache is set in scratch pad register at
> > 0xB4) and use that interface for correct behavior as explained above.
> >
> > E.g Liberator/Thunderbolt MegaRaid FW returns SYNC_CACHE as
> > Unsupported command (this is a firmware bug) and eventually command
> > will be failed to SML.
> > This will cause File system to go Read-only.
> >
> >  - New behavior described -
> >
> > IF application requests SYNCH_CACHE
> >    IF 'JBOD'
> >               Driver sends SYNCH_CACHE command to the FW
> >                FW sends SYNCH_CACHE to drive
> >                FW obtains status from drive and returns same status
> > back to driver
> >    ELSEIF 'VirtualDisk'
> >                IF any FW which supports new API bit called
> > canHandleSyncCache
> >                               Driver sends SYNCH_CACHE command to the
> > FW
> >                               FW does not send SYNCH_CACHE to drives
> >                               FW returns SUCCESS
> >                ELSE
> >                               Driver does not send SYNCH_CACHE command
> > to the FW.
> >                               Driver return SUCCESS for that command.
> >                ENDIF
> >     ENDIF
> > ENDIF
> >
> > CC: stable@vger.kernel.org
>
> Can you please split this into two, so we can backport the bug fix without
> any of
> the feature addition junk?

James  - I am sending new patch making two logical fix as you requested.
JBOD fix will be marked for CC:stable and for Virtual disk I will post only
for head (not for stable). Let me know if resending complete series is good
or resending this patch is better.

>
> > Signed-off-by: Kashyap Desai <kashyap.desai@broadcom.com>
> > Signed-off-by: Sumit Saxena <sumit.saxena@broadcom.com>
> > ---
> >  drivers/scsi/megaraid/megaraid_sas.h        |  3 +++
> >  drivers/scsi/megaraid/megaraid_sas_base.c   | 10 ++--------
> >  drivers/scsi/megaraid/megaraid_sas_fusion.c |  5 +++++
> >  3 files changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/scsi/megaraid/megaraid_sas.h
> > b/drivers/scsi/megaraid/megaraid_sas.h
> > index ca86c88..43fd14f 100644
> > --- a/drivers/scsi/megaraid/megaraid_sas.h
> > +++ b/drivers/scsi/megaraid/megaraid_sas.h
> > @@ -1429,6 +1429,8 @@ enum FW_BOOT_CONTEXT {
> >  #define MR_MAX_REPLY_QUEUES_EXT_OFFSET_SHIFT    14
> >  #define MR_MAX_MSIX_REG_ARRAY                   16
> >  #define MR_RDPQ_MODE_OFFSET			0X00800000
> > +#define MR_CAN_HANDLE_SYNC_CACHE_OFFSET		0X01000000
> > +
> >  /*
> >  * register set for both 1068 and 1078 controllers
> >  * structure extended for 1078 registers @@ -2140,6 +2142,7 @@ struct
> > megasas_instance {
> >  	u8 is_imr;
> >  	u8 is_rdpq;
> >  	bool dev_handle;
> > +	bool fw_sync_cache_support;
> >  };
> >  struct MR_LD_VF_MAP {
> >  	u32 size;
> > diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c
> > b/drivers/scsi/megaraid/megaraid_sas_base.c
> > index 3236632..f7a2ab1 100644
> > --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> > +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
> > @@ -1700,16 +1700,10 @@ inline int megasas_cmd_type(struct scsi_cmnd
> > *cmd)
> >  		goto out_done;
> >  	}
> >
> > -	switch (scmd->cmnd[0]) {
> > -	case SYNCHRONIZE_CACHE:
> > -		/*
> > -		 * FW takes care of flush cache on its own
> > -		 * No need to send it down
> > -		 */
> > +	if (MEGASAS_IS_LOGICAL(scmd) && (scmd->cmnd[0] ==
> > SYNCHRONIZE_CACHE) &&
> > +		(!instance->fw_sync_cache_support)) {
> >  		scmd->result = DID_OK << 16;
> >  		goto out_done;
>
> This, minus the  "&& (!instance->fw_sync_cache_support)" is all we need
> for the
> bug fix, correct?
Yes.   I will be sending patch for the same. It need only below check for
JBOD fix.
       if ((scmd->cmnd[0] == SYNCHRONIZE_CACHE) && MEGASAS_IS_LOGICAL(scmd))
{

>
> James
>
> > -	default:
> > -		break;
> >  	}
> >
> >  	return instance->instancet->build_and_issue_cmd(instance,
> > scmd);
> > diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > index 2159f6a..2e61306 100644
> > --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > @@ -748,6 +748,11 @@ static int megasas_create_sg_sense_fusion(struct
> > megasas_instance *instance)
> >  		goto fail_fw_init;
> >  	}
> >
> > +	instance->fw_sync_cache_support = (scratch_pad_2 &
> > +		MR_CAN_HANDLE_SYNC_CACHE_OFFSET) ? 1 : 0;
> > +	dev_info(&instance->pdev->dev, "FW supports sync cache\t:
> > %s\n",
> > +		 instance->fw_sync_cache_support ? "Yes" : "No");
> > +
> >  	IOCInitMessage =
> >  	  dma_alloc_coherent(&instance->pdev->dev,
> >  			     sizeof(struct MPI2_IOC_INIT_REQUEST),

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 4/7] megaraid_sas: Send SYNCHRONIZE_CACHE command to firmware
  2016-10-21 12:08 [PATCH v2 4/7] megaraid_sas: Send SYNCHRONIZE_CACHE command to firmware Kashyap Desai
@ 2016-10-21 12:36 ` James Bottomley
  2016-10-21 13:39   ` Kashyap Desai
  0 siblings, 1 reply; 7+ messages in thread
From: James Bottomley @ 2016-10-21 12:36 UTC (permalink / raw)
  To: Kashyap Desai, Sumit Saxena, linux-scsi; +Cc: martin.petersen, thenzl, stable

On Fri, 2016-10-21 at 17:38 +0530, Kashyap Desai wrote:
> > -----Original Message-----
> > From: James Bottomley [mailto:jejb@linux.vnet.ibm.com]
> > Sent: Friday, October 21, 2016 4:32 PM
> > To: Sumit Saxena; linux-scsi@vger.kernel.org
> > Cc: martin.petersen@oracle.com; thenzl@redhat.com;
> > kashyap.desai@broadcom.com; stable@vger.kernel.org
> > Subject: Re: [PATCH v2 4/7] megaraid_sas: Send SYNCHRONIZE_CACHE
> > command to firmware
> > 
> > On Thu, 2016-10-20 at 02:05 -0700, Sumit Saxena wrote:
> > > From previous patch we have below changes in v2 - 1.  Updated
> > > change
> > > log.  Provided more detail in change log.
> > > 2.  Agreed to remove module parameter. If we remove module
> > > parameter,
> > > we
> > >     can ask customer to disable WCE on drive to get similar
> > > impact.
> > > 3.  Always Send SYNCHRONIZE_CACHE  for JBOD (non Raid) Device to
> > > Firmware.
> > > 
> > > Current megaraid_sas driver returns SYNCHRONIZE_CACHE(related to
> > > Drive
> > > Cache)  command  back to SCSI layer without sending it down to
> > > firmware as firmware supposed to take care of flushing disk cache
> > > for
> > > all drives belongs to Virtual Disk at the time of system
> > > reboot/shutdown.
> > > 
> > > We evaluate and understood that for Raid Volume, why driver
> > > should not
> > > send SYNC_CACHE command to the Firmware.
> > > There may be a some reason in past, but now it looks to be not
> > > required and we have fixed this issue as part of this patch.
> > > 
> > > Commit- " 02b01e0 [SCSI] megaraid_sas: return sync cache call
> > > with
> > > success"
> > > added the code in driver to return SYNCHRONIZE_CACHE without
> > > sending
> > > it to firmware back in 2007. Earlier MR was mainly for Virtual
> > > Disk,
> > > so same code continue for JBOD as well whenever JBOD support was
> > > added
> > > and it introduced bug that SYNCHRONIZE_CACHE is not passed to FW
> > > for
> > > JBOD (non Raid disk).
> > > 
> > > But our recent analysis indicates that, From Day-1 MR Firmware
> > > always
> > > expect Driver to forward SYNCHRONIZE_CACHE for JBOD (non Raid
> > > disk) to
> > > the Firmware.
> > > We have fixed this as part of this patch.
> > > 
> > >  - Additional background -
> > > There are some instance of MegaRaid FW (E.a Gen2 and Gen2.5 FW)
> > > set
> > > WCE bit for Virtual Disk but firmware does not reply correct
> > > status
> > > for SYNCH_CACHE.
> > > It is very difficult to find out which Device ID and firmware has
> > > capability to manage SYNC_CACHE, so we managed to figure out
> > > which are
> > > the new firmware (canHandleSyncCache is set in scratch pad
> > > register at
> > > 0xB4) and use that interface for correct behavior as explained
> > > above.
> > > 
> > > E.g Liberator/Thunderbolt MegaRaid FW returns SYNC_CACHE as
> > > Unsupported command (this is a firmware bug) and eventually
> > > command
> > > will be failed to SML.
> > > This will cause File system to go Read-only.
> > > 
> > >  - New behavior described -
> > > 
> > > IF application requests SYNCH_CACHE
> > >    IF 'JBOD'
> > >               Driver sends SYNCH_CACHE command to the FW
> > >                FW sends SYNCH_CACHE to drive
> > >                FW obtains status from drive and returns same
> > > status
> > > back to driver
> > >    ELSEIF 'VirtualDisk'
> > >                IF any FW which supports new API bit called
> > > canHandleSyncCache
> > >                               Driver sends SYNCH_CACHE command to
> > > the
> > > FW
> > >                               FW does not send SYNCH_CACHE to
> > > drives
> > >                               FW returns SUCCESS
> > >                ELSE
> > >                               Driver does not send SYNCH_CACHE
> > > command
> > > to the FW.
> > >                               Driver return SUCCESS for that
> > > command.
> > >                ENDIF
> > >     ENDIF
> > > ENDIF
> > > 
> > > CC: stable@vger.kernel.org
> > 
> > Can you please split this into two, so we can backport the bug fix
> > without
> > any of
> > the feature addition junk?
> 
> James  - I am sending new patch making two logical fix as you 
> requested. JBOD fix will be marked for CC:stable and for Virtual disk 
> I will post only for head (not for stable). Let me know if resending 
> complete series is good or resending this patch is better.

This is up to Martin, because he has to sort it out, but I think we can
cope with just the two patches.  The bug fix one has to go into the
fixes branch for immediate application and the rest of the series will
go into the misc branch for the 4.10 merge window.

James


> 
> > 
> > > Signed-off-by: Kashyap Desai <kashyap.desai@broadcom.com>
> > > Signed-off-by: Sumit Saxena <sumit.saxena@broadcom.com>
> > > ---
> > >  drivers/scsi/megaraid/megaraid_sas.h        |  3 +++
> > >  drivers/scsi/megaraid/megaraid_sas_base.c   | 10 ++--------
> > >  drivers/scsi/megaraid/megaraid_sas_fusion.c |  5 +++++
> > >  3 files changed, 10 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/scsi/megaraid/megaraid_sas.h
> > > b/drivers/scsi/megaraid/megaraid_sas.h
> > > index ca86c88..43fd14f 100644
> > > --- a/drivers/scsi/megaraid/megaraid_sas.h
> > > +++ b/drivers/scsi/megaraid/megaraid_sas.h
> > > @@ -1429,6 +1429,8 @@ enum FW_BOOT_CONTEXT {
> > >  #define MR_MAX_REPLY_QUEUES_EXT_OFFSET_SHIFT    14
> > >  #define MR_MAX_MSIX_REG_ARRAY                   16
> > >  #define MR_RDPQ_MODE_OFFSET			0X00800000
> > > +#define MR_CAN_HANDLE_SYNC_CACHE_OFFSET		0X0100000
> > > 0
> > > +
> > >  /*
> > >  * register set for both 1068 and 1078 controllers
> > >  * structure extended for 1078 registers @@ -2140,6 +2142,7 @@
> > > struct
> > > megasas_instance {
> > >  	u8 is_imr;
> > >  	u8 is_rdpq;
> > >  	bool dev_handle;
> > > +	bool fw_sync_cache_support;
> > >  };
> > >  struct MR_LD_VF_MAP {
> > >  	u32 size;
> > > diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c
> > > b/drivers/scsi/megaraid/megaraid_sas_base.c
> > > index 3236632..f7a2ab1 100644
> > > --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> > > +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
> > > @@ -1700,16 +1700,10 @@ inline int megasas_cmd_type(struct
> > > scsi_cmnd
> > > *cmd)
> > >  		goto out_done;
> > >  	}
> > > 
> > > -	switch (scmd->cmnd[0]) {
> > > -	case SYNCHRONIZE_CACHE:
> > > -		/*
> > > -		 * FW takes care of flush cache on its own
> > > -		 * No need to send it down
> > > -		 */
> > > +	if (MEGASAS_IS_LOGICAL(scmd) && (scmd->cmnd[0] ==
> > > SYNCHRONIZE_CACHE) &&
> > > +		(!instance->fw_sync_cache_support)) {
> > >  		scmd->result = DID_OK << 16;
> > >  		goto out_done;
> > 
> > This, minus the  "&& (!instance->fw_sync_cache_support)" is all we
> > need
> > for the
> > bug fix, correct?
> Yes.   I will be sending patch for the same. It need only below check
> for
> JBOD fix.
>        if ((scmd->cmnd[0] == SYNCHRONIZE_CACHE) &&
> MEGASAS_IS_LOGICAL(scmd))
> {
> 
> > 
> > James
> > 
> > > -	default:
> > > -		break;
> > >  	}
> > > 
> > >  	return instance->instancet
> > > ->build_and_issue_cmd(instance,
> > > scmd);
> > > diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > > b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > > index 2159f6a..2e61306 100644
> > > --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > > +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > > @@ -748,6 +748,11 @@ static int
> > > megasas_create_sg_sense_fusion(struct
> > > megasas_instance *instance)
> > >  		goto fail_fw_init;
> > >  	}
> > > 
> > > +	instance->fw_sync_cache_support = (scratch_pad_2 &
> > > +		MR_CAN_HANDLE_SYNC_CACHE_OFFSET) ? 1 : 0;
> > > +	dev_info(&instance->pdev->dev, "FW supports sync
> > > cache\t:
> > > %s\n",
> > > +		 instance->fw_sync_cache_support ? "Yes" :
> > > "No");
> > > +
> > >  	IOCInitMessage =
> > >  	  dma_alloc_coherent(&instance->pdev->dev,
> > >  			     sizeof(struct
> > > MPI2_IOC_INIT_REQUEST),
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH v2 4/7] megaraid_sas: Send SYNCHRONIZE_CACHE command to firmware
  2016-10-21 12:36 ` James Bottomley
@ 2016-10-21 13:39   ` Kashyap Desai
  0 siblings, 0 replies; 7+ messages in thread
From: Kashyap Desai @ 2016-10-21 13:39 UTC (permalink / raw)
  To: James Bottomley, Sumit Saxena, linux-scsi; +Cc: martin.petersen, thenzl, stable

> > >
> > > Can you please split this into two, so we can backport the bug fix
> > > without any of the feature addition junk?
> >
> > James  - I am sending new patch making two logical fix as you
> > requested. JBOD fix will be marked for CC:stable and for Virtual disk
> > I will post only for head (not for stable). Let me know if resending
> > complete series is good or resending this patch is better.
>
> This is up to Martin, because he has to sort it out, but I think we can
> cope with
> just the two patches.  The bug fix one has to go into the fixes branch for
> immediate application and the rest of the series will go into the misc
> branch for
> the 4.10 merge window.

I resend patch set ( v3 series) . Martin, please use latest resend v3 for
4.10 merge.

Need Reviewed-by tag for below patch as reworked from original submission.
[PATCH v3 3/8] megaraid_sas: Do not fire DCMDs during PCI shutdown/detach
[PATCH v3 4/8] megaraid_sas: Send SYNCHRONIZE_CACHE for non-raid to firmware
[PATCH v3 5/8] megaraid_sas: Send SYNCHRONIZE_CACHE for VD to firmware

>
> James
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 4/7] megaraid_sas: Send SYNCHRONIZE_CACHE command to firmware
  2016-10-21 11:01   ` James Bottomley
@ 2016-10-21 16:01     ` Tomas Henzl
  0 siblings, 0 replies; 7+ messages in thread
From: Tomas Henzl @ 2016-10-21 16:01 UTC (permalink / raw)
  To: James Bottomley, Sumit Saxena, linux-scsi
  Cc: martin.petersen, kashyap.desai, stable

On 21.10.2016 13:01, James Bottomley wrote:
> On Thu, 2016-10-20 at 02:05 -0700, Sumit Saxena wrote:
>> From previous patch we have below changes in v2 -
>> 1.  Updated change log.  Provided more detail in change log.
>> 2.  Agreed to remove module parameter. If we remove module parameter,
>> we 
>>     can ask customer to disable WCE on drive to get similar impact.
>> 3.  Always Send SYNCHRONIZE_CACHE  for JBOD (non Raid) Device to
>> Firmware.
>>  
>> Current megaraid_sas driver returns SYNCHRONIZE_CACHE(related to
>> Drive
>> Cache)  command  back to SCSI layer without sending it down to
>> firmware as
>> firmware supposed to take care of flushing disk cache for all drives
>> belongs to Virtual Disk at the time of system reboot/shutdown.
>>  
>> We evaluate and understood that for Raid Volume, why driver should
>> not
>> send SYNC_CACHE command to the Firmware.
>> There may be a some reason in past, but now it looks to be not
>> required and
>> we have fixed this issue as part of this patch.
>>
>> Commit- " 02b01e0 [SCSI] megaraid_sas: return sync cache call with
>> success"
>> added the code in driver to return SYNCHRONIZE_CACHE without sending
>> it to 
>> firmware back in 2007. Earlier MR was mainly for Virtual Disk,
>> so same code continue for JBOD as well whenever JBOD support was
>> added and it introduced bug that
>> SYNCHRONIZE_CACHE is not passed to FW for JBOD (non Raid disk).
>>
>> But our recent analysis indicates that, From Day-1 MR Firmware always
>> expect Driver to forward SYNCHRONIZE_CACHE for JBOD (non Raid disk)
>> to the
>> Firmware.
>> We have fixed this as part of this patch.
>>  
>>  - Additional background -
>> There are some instance of MegaRaid FW (E.a Gen2 and Gen2.5 FW) set
>> WCE bit
>> for Virtual Disk but firmware does not reply correct status for
>> SYNCH_CACHE.
>> It is very difficult to find out which Device ID and firmware has
>> capability
>> to manage SYNC_CACHE, so we managed to figure out which are the new
>> firmware
>> (canHandleSyncCache is set in scratch pad register at 0xB4) and use
>> that
>> interface for correct behavior as explained above.
>>  
>> E.g Liberator/Thunderbolt MegaRaid FW returns SYNC_CACHE as
>> Unsupported
>> command (this is a firmware bug) and eventually command will be
>> failed to SML.
>> This will cause File system to go Read-only.
>>  
>>  - New behavior described -
>>  
>> IF application requests SYNCH_CACHE
>>    IF 'JBOD'
>>               Driver sends SYNCH_CACHE command to the FW
>>                FW sends SYNCH_CACHE to drive
>>                FW obtains status from drive and returns same status
>> back to driver
>>    ELSEIF 'VirtualDisk'
>>                IF any FW which supports new API bit called
>> canHandleSyncCache
>>                               Driver sends SYNCH_CACHE command to the
>> FW
>>                               FW does not send SYNCH_CACHE to drives
>>                               FW returns SUCCESS
>>                ELSE
>>                               Driver does not send SYNCH_CACHE
>> command to the FW.
>>                               Driver return SUCCESS for that command.
>>                ENDIF
>>     ENDIF
>> ENDIF
>>
>> CC: stable@vger.kernel.org
> Can you please split this into two, so we can backport the bug fix
> without any of the feature addition junk?
>
>> Signed-off-by: Kashyap Desai <kashyap.desai@broadcom.com>
>> Signed-off-by: Sumit Saxena <sumit.saxena@broadcom.com>
>> ---
>>  drivers/scsi/megaraid/megaraid_sas.h        |  3 +++
>>  drivers/scsi/megaraid/megaraid_sas_base.c   | 10 ++--------
>>  drivers/scsi/megaraid/megaraid_sas_fusion.c |  5 +++++
>>  3 files changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/scsi/megaraid/megaraid_sas.h
>> b/drivers/scsi/megaraid/megaraid_sas.h
>> index ca86c88..43fd14f 100644
>> --- a/drivers/scsi/megaraid/megaraid_sas.h
>> +++ b/drivers/scsi/megaraid/megaraid_sas.h
>> @@ -1429,6 +1429,8 @@ enum FW_BOOT_CONTEXT {
>>  #define MR_MAX_REPLY_QUEUES_EXT_OFFSET_SHIFT    14
>>  #define MR_MAX_MSIX_REG_ARRAY                   16
>>  #define MR_RDPQ_MODE_OFFSET			0X00800000
>> +#define MR_CAN_HANDLE_SYNC_CACHE_OFFSET		0X01000000
>> +
>>  /*
>>  * register set for both 1068 and 1078 controllers
>>  * structure extended for 1078 registers
>> @@ -2140,6 +2142,7 @@ struct megasas_instance {
>>  	u8 is_imr;
>>  	u8 is_rdpq;
>>  	bool dev_handle;
>> +	bool fw_sync_cache_support;
>>  };
>>  struct MR_LD_VF_MAP {
>>  	u32 size;
>> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c
>> b/drivers/scsi/megaraid/megaraid_sas_base.c
>> index 3236632..f7a2ab1 100644
>> --- a/drivers/scsi/megaraid/megaraid_sas_base.c
>> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
>> @@ -1700,16 +1700,10 @@ inline int megasas_cmd_type(struct scsi_cmnd
>> *cmd)
>>  		goto out_done;
>>  	}
>>  
>> -	switch (scmd->cmnd[0]) {
>> -	case SYNCHRONIZE_CACHE:
>> -		/*
>> -		 * FW takes care of flush cache on its own
>> -		 * No need to send it down
>> -		 */
>> +	if (MEGASAS_IS_LOGICAL(scmd) && (scmd->cmnd[0] ==
>> SYNCHRONIZE_CACHE) &&
>> +		(!instance->fw_sync_cache_support)) {
>>  		scmd->result = DID_OK << 16;
>>  		goto out_done;
> This, minus the  "&& (!instance->fw_sync_cache_support)" is all we need
> for the bug fix, correct?

Isn't both a) sending SYNC to JBOD and b) sending SYNC to logical luns 
a bugfix of the same kind?

>
> James
>
>> -	default:
>> -		break;
>>  	}
>>  
>>  	return instance->instancet->build_and_issue_cmd(instance,
>> scmd);
>> diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
>> b/drivers/scsi/megaraid/megaraid_sas_fusion.c
>> index 2159f6a..2e61306 100644
>> --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
>> +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
>> @@ -748,6 +748,11 @@ static int megasas_create_sg_sense_fusion(struct
>> megasas_instance *instance)
>>  		goto fail_fw_init;
>>  	}
>>  
>> +	instance->fw_sync_cache_support = (scratch_pad_2 &
>> +		MR_CAN_HANDLE_SYNC_CACHE_OFFSET) ? 1 : 0;
>> +	dev_info(&instance->pdev->dev, "FW supports sync cache\t:
>> %s\n",
>> +		 instance->fw_sync_cache_support ? "Yes" : "No");
>> +
>>  	IOCInitMessage =
>>  	  dma_alloc_coherent(&instance->pdev->dev,
>>  			     sizeof(struct MPI2_IOC_INIT_REQUEST),
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 4/7] megaraid_sas: Send SYNCHRONIZE_CACHE command to firmware
  2016-10-20  9:05 ` [PATCH v2 4/7] megaraid_sas: Send SYNCHRONIZE_CACHE command to firmware Sumit Saxena
  2016-10-21 10:01   ` Ric Wheeler
@ 2016-10-21 11:01   ` James Bottomley
  2016-10-21 16:01     ` Tomas Henzl
  1 sibling, 1 reply; 7+ messages in thread
From: James Bottomley @ 2016-10-21 11:01 UTC (permalink / raw)
  To: Sumit Saxena, linux-scsi; +Cc: martin.petersen, thenzl, kashyap.desai, stable

On Thu, 2016-10-20 at 02:05 -0700, Sumit Saxena wrote:
> From previous patch we have below changes in v2 -
> 1.  Updated change log.  Provided more detail in change log.
> 2.  Agreed to remove module parameter. If we remove module parameter,
> we 
>     can ask customer to disable WCE on drive to get similar impact.
> 3.  Always Send SYNCHRONIZE_CACHE  for JBOD (non Raid) Device to
> Firmware.
>  
> Current megaraid_sas driver returns SYNCHRONIZE_CACHE(related to
> Drive
> Cache)  command  back to SCSI layer without sending it down to
> firmware as
> firmware supposed to take care of flushing disk cache for all drives
> belongs to Virtual Disk at the time of system reboot/shutdown.
>  
> We evaluate and understood that for Raid Volume, why driver should
> not
> send SYNC_CACHE command to the Firmware.
> There may be a some reason in past, but now it looks to be not
> required and
> we have fixed this issue as part of this patch.
> 
> Commit- " 02b01e0 [SCSI] megaraid_sas: return sync cache call with
> success"
> added the code in driver to return SYNCHRONIZE_CACHE without sending
> it to 
> firmware back in 2007. Earlier MR was mainly for Virtual Disk,
> so same code continue for JBOD as well whenever JBOD support was
> added and it introduced bug that
> SYNCHRONIZE_CACHE is not passed to FW for JBOD (non Raid disk).
> 
> But our recent analysis indicates that, From Day-1 MR Firmware always
> expect Driver to forward SYNCHRONIZE_CACHE for JBOD (non Raid disk)
> to the
> Firmware.
> We have fixed this as part of this patch.
>  
>  - Additional background -
> There are some instance of MegaRaid FW (E.a Gen2 and Gen2.5 FW) set
> WCE bit
> for Virtual Disk but firmware does not reply correct status for
> SYNCH_CACHE.
> It is very difficult to find out which Device ID and firmware has
> capability
> to manage SYNC_CACHE, so we managed to figure out which are the new
> firmware
> (canHandleSyncCache is set in scratch pad register at 0xB4) and use
> that
> interface for correct behavior as explained above.
>  
> E.g Liberator/Thunderbolt MegaRaid FW returns SYNC_CACHE as
> Unsupported
> command (this is a firmware bug) and eventually command will be
> failed to SML.
> This will cause File system to go Read-only.
>  
>  - New behavior described -
>  
> IF application requests SYNCH_CACHE
>    IF 'JBOD'
>               Driver sends SYNCH_CACHE command to the FW
>                FW sends SYNCH_CACHE to drive
>                FW obtains status from drive and returns same status
> back to driver
>    ELSEIF 'VirtualDisk'
>                IF any FW which supports new API bit called
> canHandleSyncCache
>                               Driver sends SYNCH_CACHE command to the
> FW
>                               FW does not send SYNCH_CACHE to drives
>                               FW returns SUCCESS
>                ELSE
>                               Driver does not send SYNCH_CACHE
> command to the FW.
>                               Driver return SUCCESS for that command.
>                ENDIF
>     ENDIF
> ENDIF
> 
> CC: stable@vger.kernel.org

Can you please split this into two, so we can backport the bug fix
without any of the feature addition junk?

> Signed-off-by: Kashyap Desai <kashyap.desai@broadcom.com>
> Signed-off-by: Sumit Saxena <sumit.saxena@broadcom.com>
> ---
>  drivers/scsi/megaraid/megaraid_sas.h        |  3 +++
>  drivers/scsi/megaraid/megaraid_sas_base.c   | 10 ++--------
>  drivers/scsi/megaraid/megaraid_sas_fusion.c |  5 +++++
>  3 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/scsi/megaraid/megaraid_sas.h
> b/drivers/scsi/megaraid/megaraid_sas.h
> index ca86c88..43fd14f 100644
> --- a/drivers/scsi/megaraid/megaraid_sas.h
> +++ b/drivers/scsi/megaraid/megaraid_sas.h
> @@ -1429,6 +1429,8 @@ enum FW_BOOT_CONTEXT {
>  #define MR_MAX_REPLY_QUEUES_EXT_OFFSET_SHIFT    14
>  #define MR_MAX_MSIX_REG_ARRAY                   16
>  #define MR_RDPQ_MODE_OFFSET			0X00800000
> +#define MR_CAN_HANDLE_SYNC_CACHE_OFFSET		0X01000000
> +
>  /*
>  * register set for both 1068 and 1078 controllers
>  * structure extended for 1078 registers
> @@ -2140,6 +2142,7 @@ struct megasas_instance {
>  	u8 is_imr;
>  	u8 is_rdpq;
>  	bool dev_handle;
> +	bool fw_sync_cache_support;
>  };
>  struct MR_LD_VF_MAP {
>  	u32 size;
> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c
> b/drivers/scsi/megaraid/megaraid_sas_base.c
> index 3236632..f7a2ab1 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
> @@ -1700,16 +1700,10 @@ inline int megasas_cmd_type(struct scsi_cmnd
> *cmd)
>  		goto out_done;
>  	}
>  
> -	switch (scmd->cmnd[0]) {
> -	case SYNCHRONIZE_CACHE:
> -		/*
> -		 * FW takes care of flush cache on its own
> -		 * No need to send it down
> -		 */
> +	if (MEGASAS_IS_LOGICAL(scmd) && (scmd->cmnd[0] ==
> SYNCHRONIZE_CACHE) &&
> +		(!instance->fw_sync_cache_support)) {
>  		scmd->result = DID_OK << 16;
>  		goto out_done;

This, minus the  "&& (!instance->fw_sync_cache_support)" is all we need
for the bug fix, correct?

James

> -	default:
> -		break;
>  	}
>  
>  	return instance->instancet->build_and_issue_cmd(instance,
> scmd);
> diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> index 2159f6a..2e61306 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> @@ -748,6 +748,11 @@ static int megasas_create_sg_sense_fusion(struct
> megasas_instance *instance)
>  		goto fail_fw_init;
>  	}
>  
> +	instance->fw_sync_cache_support = (scratch_pad_2 &
> +		MR_CAN_HANDLE_SYNC_CACHE_OFFSET) ? 1 : 0;
> +	dev_info(&instance->pdev->dev, "FW supports sync cache\t:
> %s\n",
> +		 instance->fw_sync_cache_support ? "Yes" : "No");
> +
>  	IOCInitMessage =
>  	  dma_alloc_coherent(&instance->pdev->dev,
>  			     sizeof(struct MPI2_IOC_INIT_REQUEST),


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 4/7] megaraid_sas: Send SYNCHRONIZE_CACHE command to firmware
  2016-10-20  9:05 ` [PATCH v2 4/7] megaraid_sas: Send SYNCHRONIZE_CACHE command to firmware Sumit Saxena
@ 2016-10-21 10:01   ` Ric Wheeler
  2016-10-21 11:01   ` James Bottomley
  1 sibling, 0 replies; 7+ messages in thread
From: Ric Wheeler @ 2016-10-21 10:01 UTC (permalink / raw)
  To: Sumit Saxena, linux-scsi
  Cc: martin.petersen, thenzl, jejb, kashyap.desai, stable

On 10/20/2016 05:05 AM, Sumit Saxena wrote:
>  From previous patch we have below changes in v2 -
> 1.  Updated change log.  Provided more detail in change log.
> 2.  Agreed to remove module parameter. If we remove module parameter, we
>      can ask customer to disable WCE on drive to get similar impact.
> 3.  Always Send SYNCHRONIZE_CACHE  for JBOD (non Raid) Device to Firmware.

Just a note - the user can disable sending the synchronize cache settings by 
mount time options with a file system (nobarrier) or for raw devices, overriding 
cache_type.

This should never be required unless the target device has some quirk that does 
not allow it to handle synchronize_cache correctly.

Regards,

Ric



^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2 4/7] megaraid_sas: Send SYNCHRONIZE_CACHE command to firmware
  2016-10-20  9:04 [PATCH v2 0/7] megaraid_sas: Updates for scsi-next Sumit Saxena
@ 2016-10-20  9:05 ` Sumit Saxena
  2016-10-21 10:01   ` Ric Wheeler
  2016-10-21 11:01   ` James Bottomley
  0 siblings, 2 replies; 7+ messages in thread
From: Sumit Saxena @ 2016-10-20  9:05 UTC (permalink / raw)
  To: linux-scsi
  Cc: martin.petersen, thenzl, jejb, kashyap.desai, Sumit Saxena, stable

>From previous patch we have below changes in v2 -
1.  Updated change log.  Provided more detail in change log.
2.  Agreed to remove module parameter. If we remove module parameter, we 
    can ask customer to disable WCE on drive to get similar impact.
3.  Always Send SYNCHRONIZE_CACHE  for JBOD (non Raid) Device to Firmware.
 
Current megaraid_sas driver returns SYNCHRONIZE_CACHE(related to Drive
Cache)  command  back to SCSI layer without sending it down to firmware as
firmware supposed to take care of flushing disk cache for all drives
belongs to Virtual Disk at the time of system reboot/shutdown.
 
We evaluate and understood that for Raid Volume, why driver should not
send SYNC_CACHE command to the Firmware.
There may be a some reason in past, but now it looks to be not required and
we have fixed this issue as part of this patch.

Commit- " 02b01e0 [SCSI] megaraid_sas: return sync cache call with success"
added the code in driver to return SYNCHRONIZE_CACHE without sending it to 
firmware back in 2007. Earlier MR was mainly for Virtual Disk,
so same code continue for JBOD as well whenever JBOD support was added and it introduced bug that
SYNCHRONIZE_CACHE is not passed to FW for JBOD (non Raid disk).

But our recent analysis indicates that, From Day-1 MR Firmware always
expect Driver to forward SYNCHRONIZE_CACHE for JBOD (non Raid disk) to the
Firmware.
We have fixed this as part of this patch.
 
 - Additional background -
There are some instance of MegaRaid FW (E.a Gen2 and Gen2.5 FW) set WCE bit
for Virtual Disk but firmware does not reply correct status for SYNCH_CACHE.
It is very difficult to find out which Device ID and firmware has capability
to manage SYNC_CACHE, so we managed to figure out which are the new firmware
(canHandleSyncCache is set in scratch pad register at 0xB4) and use that
interface for correct behavior as explained above.
 
E.g Liberator/Thunderbolt MegaRaid FW returns SYNC_CACHE as Unsupported
command (this is a firmware bug) and eventually command will be failed to SML.
This will cause File system to go Read-only.
 
 - New behavior described -
 
IF application requests SYNCH_CACHE
   IF 'JBOD'
              Driver sends SYNCH_CACHE command to the FW
               FW sends SYNCH_CACHE to drive
               FW obtains status from drive and returns same status back to driver
   ELSEIF 'VirtualDisk'
               IF any FW which supports new API bit called canHandleSyncCache
                              Driver sends SYNCH_CACHE command to the FW
                              FW does not send SYNCH_CACHE to drives
                              FW returns SUCCESS
               ELSE
                              Driver does not send SYNCH_CACHE command to the FW.
                              Driver return SUCCESS for that command.
               ENDIF
    ENDIF
ENDIF

CC: stable@vger.kernel.org
Signed-off-by: Kashyap Desai <kashyap.desai@broadcom.com>
Signed-off-by: Sumit Saxena <sumit.saxena@broadcom.com>
---
 drivers/scsi/megaraid/megaraid_sas.h        |  3 +++
 drivers/scsi/megaraid/megaraid_sas_base.c   | 10 ++--------
 drivers/scsi/megaraid/megaraid_sas_fusion.c |  5 +++++
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/megaraid/megaraid_sas.h b/drivers/scsi/megaraid/megaraid_sas.h
index ca86c88..43fd14f 100644
--- a/drivers/scsi/megaraid/megaraid_sas.h
+++ b/drivers/scsi/megaraid/megaraid_sas.h
@@ -1429,6 +1429,8 @@ enum FW_BOOT_CONTEXT {
 #define MR_MAX_REPLY_QUEUES_EXT_OFFSET_SHIFT    14
 #define MR_MAX_MSIX_REG_ARRAY                   16
 #define MR_RDPQ_MODE_OFFSET			0X00800000
+#define MR_CAN_HANDLE_SYNC_CACHE_OFFSET		0X01000000
+
 /*
 * register set for both 1068 and 1078 controllers
 * structure extended for 1078 registers
@@ -2140,6 +2142,7 @@ struct megasas_instance {
 	u8 is_imr;
 	u8 is_rdpq;
 	bool dev_handle;
+	bool fw_sync_cache_support;
 };
 struct MR_LD_VF_MAP {
 	u32 size;
diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
index 3236632..f7a2ab1 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -1700,16 +1700,10 @@ inline int megasas_cmd_type(struct scsi_cmnd *cmd)
 		goto out_done;
 	}
 
-	switch (scmd->cmnd[0]) {
-	case SYNCHRONIZE_CACHE:
-		/*
-		 * FW takes care of flush cache on its own
-		 * No need to send it down
-		 */
+	if (MEGASAS_IS_LOGICAL(scmd) && (scmd->cmnd[0] == SYNCHRONIZE_CACHE) &&
+		(!instance->fw_sync_cache_support)) {
 		scmd->result = DID_OK << 16;
 		goto out_done;
-	default:
-		break;
 	}
 
 	return instance->instancet->build_and_issue_cmd(instance, scmd);
diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c b/drivers/scsi/megaraid/megaraid_sas_fusion.c
index 2159f6a..2e61306 100644
--- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
+++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
@@ -748,6 +748,11 @@ static int megasas_create_sg_sense_fusion(struct megasas_instance *instance)
 		goto fail_fw_init;
 	}
 
+	instance->fw_sync_cache_support = (scratch_pad_2 &
+		MR_CAN_HANDLE_SYNC_CACHE_OFFSET) ? 1 : 0;
+	dev_info(&instance->pdev->dev, "FW supports sync cache\t: %s\n",
+		 instance->fw_sync_cache_support ? "Yes" : "No");
+
 	IOCInitMessage =
 	  dma_alloc_coherent(&instance->pdev->dev,
 			     sizeof(struct MPI2_IOC_INIT_REQUEST),
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2016-10-21 16:01 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-21 12:08 [PATCH v2 4/7] megaraid_sas: Send SYNCHRONIZE_CACHE command to firmware Kashyap Desai
2016-10-21 12:36 ` James Bottomley
2016-10-21 13:39   ` Kashyap Desai
  -- strict thread matches above, loose matches on Subject: below --
2016-10-20  9:04 [PATCH v2 0/7] megaraid_sas: Updates for scsi-next Sumit Saxena
2016-10-20  9:05 ` [PATCH v2 4/7] megaraid_sas: Send SYNCHRONIZE_CACHE command to firmware Sumit Saxena
2016-10-21 10:01   ` Ric Wheeler
2016-10-21 11:01   ` James Bottomley
2016-10-21 16:01     ` Tomas Henzl

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.