| Message ID | 20180604050953.2381-1-himanshu.madhani@cavium.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Sun, 2018-06-03 at 22:09 -0700, Himanshu Madhani wrote: > This patch prevents driver from setting lower default speed > of 1 GB/sec, if the switch does not support Get Port Speed > Capabilities (GPSC) command. Setting this default speed results > into much lower write performance for large sequential WRITE. > This patch modifies driver to check for gpsc_supported flags and > prevents driver from issuing MBC_SET_PORT_PARAM (001Ah) to set > default speed of 1 GB/sec. If driver does not send this mailbox > command, firmware assumes maximum supported link speed and will > operate at the max speed. > > Cc: stable@vger.kernel.org > Signed-off-by: Himanshu Madhani <himanshu.madhani@cavium.com> Is this a regression? In other words, does this patch need a "Fixes:" tag? Thanks, Bart.
On Mon, 2018-06-04 at 08:28 +0000, Bart Van Assche wrote: > On Sun, 2018-06-03 at 22:09 -0700, Himanshu Madhani wrote: > > This patch prevents driver from setting lower default speed > > of 1 GB/sec, if the switch does not support Get Port Speed > > Capabilities (GPSC) command. Setting this default speed results > > into much lower write performance for large sequential WRITE. > > This patch modifies driver to check for gpsc_supported flags and > > prevents driver from issuing MBC_SET_PORT_PARAM (001Ah) to set > > default speed of 1 GB/sec. If driver does not send this mailbox > > command, firmware assumes maximum supported link speed and will > > operate at the max speed. > > > > Cc: stable@vger.kernel.org > > Signed-off-by: Himanshu Madhani <himanshu.madhani@cavium.com> > > Is this a regression? In other words, does this patch need a "Fixes:" tag? It is, it appears to have been introduced by the series which updated the driver to 10.00.00.04-k. It seems to have been introduced somewhere between: 82abdcaf3e "scsi: qla2xxx: Allow target mode to accept PRLI in dual mode" and f352eeb754 "scsi: qla2xxx: Add ability to use GPNFT/GNNFT for RSCN handling" The regression is definitely present in: 6a2cf8d366 "scsi: qla2xxx: Fix crashes in qla2x00_probe_one on probe failure" Unfortunately I was not able to bisect further due to crashes on boot and a problem with LUN discovery that was not fixed until a later patch that does not apply easily without the many intervening patches. -Ewan > > Thanks, > > Bart. > > >
On Sun, 2018-06-03 at 22:09 -0700, Himanshu Madhani wrote: > This patch prevents driver from setting lower default speed > of 1 GB/sec, if the switch does not support Get Port Speed > Capabilities (GPSC) command. Setting this default speed results > into much lower write performance for large sequential WRITE. > This patch modifies driver to check for gpsc_supported flags and > prevents driver from issuing MBC_SET_PORT_PARAM (001Ah) to set > default speed of 1 GB/sec. If driver does not send this mailbox > command, firmware assumes maximum supported link speed and will > operate at the max speed. > > Cc: stable@vger.kernel.org > Signed-off-by: Himanshu Madhani <himanshu.madhani@cavium.com> > --- > Hi Martin, > > This patch fixes lower write performance for large sequential Writes. > > Please apply this to 4.18-scsi-fixes at your earliest convenience. > > Thanks, > Himanshu > --- > drivers/scsi/qla2xxx/qla_init.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c > index 1aa3720ea2ed..b0430a280ce6 100644 > --- a/drivers/scsi/qla2xxx/qla_init.c > +++ b/drivers/scsi/qla2xxx/qla_init.c > @@ -5007,7 +5007,8 @@ qla2x00_iidma_fcport(scsi_qla_host_t *vha, fc_port_t *fcport) > return; > > if (fcport->fp_speed == PORT_SPEED_UNKNOWN || > - fcport->fp_speed > ha->link_data_rate) > + fcport->fp_speed > ha->link_data_rate || > + !ha->flags.gpsc_supported) > return; > > rval = qla2x00_set_idma_speed(vha, fcport->loop_id, fcport->fp_speed, Martin, This patch fixes the issue Eda found in our test environment. Reported-by: Eda Zhou <ezhou@redhat.com> Reviewed-by: Ewan D. Milne <emilne@redhat.com> Tested-by: Ewan D. Milne <emilne@redhat.com>
Thanks Ewan, Bart, > On Jun 4, 2018, at 6:52 AM, Ewan D. Milne <emilne@redhat.com> wrote: > > On Mon, 2018-06-04 at 08:28 +0000, Bart Van Assche wrote: >> On Sun, 2018-06-03 at 22:09 -0700, Himanshu Madhani wrote: >>> This patch prevents driver from setting lower default speed >>> of 1 GB/sec, if the switch does not support Get Port Speed >>> Capabilities (GPSC) command. Setting this default speed results >>> into much lower write performance for large sequential WRITE. >>> This patch modifies driver to check for gpsc_supported flags and >>> prevents driver from issuing MBC_SET_PORT_PARAM (001Ah) to set >>> default speed of 1 GB/sec. If driver does not send this mailbox >>> command, firmware assumes maximum supported link speed and will >>> operate at the max speed. >>> >>> Cc: stable@vger.kernel.org >>> Signed-off-by: Himanshu Madhani <himanshu.madhani@cavium.com> >> >> Is this a regression? In other words, does this patch need a "Fixes:" tag? > > It is, it appears to have been introduced by the series which updated > the driver to 10.00.00.04-k. It seems to have been introduced somewhere between: > > 82abdcaf3e "scsi: qla2xxx: Allow target mode to accept PRLI in dual mode" > > and > > f352eeb754 "scsi: qla2xxx: Add ability to use GPNFT/GNNFT for RSCN handling" > > The regression is definitely present in: > > 6a2cf8d366 "scsi: qla2xxx: Fix crashes in qla2x00_probe_one on probe failure" > > Unfortunately I was not able to bisect further due to crashes on boot > and a problem with LUN discovery that was not fixed until a later patch > that does not apply easily without the many intervening patches. > > -Ewan > As Ewan mentioned this was discovered between the above 2 patches in the some env where switch was rejecting or not supporting GPSC command. However, the original code should have checked for this flag in the first place to prevent driver from setting default speed which could be lower than negotiated link speed. So, I did not added “Fixes” tag because this change is applicable to all stable releases. >> >> Thanks, >> >> Bart. >> >> >> > > Thanks, - Himanshu
Himanshu, > This patch prevents driver from setting lower default speed > of 1 GB/sec, if the switch does not support Get Port Speed > Capabilities (GPSC) command. Setting this default speed results > into much lower write performance for large sequential WRITE. > This patch modifies driver to check for gpsc_supported flags and > prevents driver from issuing MBC_SET_PORT_PARAM (001Ah) to set > default speed of 1 GB/sec. If driver does not send this mailbox > command, firmware assumes maximum supported link speed and will > operate at the max speed. Applied to 4.18/scsi-fixes. Thank you!
diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c index 1aa3720ea2ed..b0430a280ce6 100644 --- a/drivers/scsi/qla2xxx/qla_init.c +++ b/drivers/scsi/qla2xxx/qla_init.c @@ -5007,7 +5007,8 @@ qla2x00_iidma_fcport(scsi_qla_host_t *vha, fc_port_t *fcport) return; if (fcport->fp_speed == PORT_SPEED_UNKNOWN || - fcport->fp_speed > ha->link_data_rate) + fcport->fp_speed > ha->link_data_rate || + !ha->flags.gpsc_supported) return; rval = qla2x00_set_idma_speed(vha, fcport->loop_id, fcport->fp_speed,
This patch prevents driver from setting lower default speed of 1 GB/sec, if the switch does not support Get Port Speed Capabilities (GPSC) command. Setting this default speed results into much lower write performance for large sequential WRITE. This patch modifies driver to check for gpsc_supported flags and prevents driver from issuing MBC_SET_PORT_PARAM (001Ah) to set default speed of 1 GB/sec. If driver does not send this mailbox command, firmware assumes maximum supported link speed and will operate at the max speed. Cc: stable@vger.kernel.org Signed-off-by: Himanshu Madhani <himanshu.madhani@cavium.com> --- Hi Martin, This patch fixes lower write performance for large sequential Writes. Please apply this to 4.18-scsi-fixes at your earliest convenience. Thanks, Himanshu --- drivers/scsi/qla2xxx/qla_init.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)