Without this, you'll see a null pointer dereference in
hpsa_enter_performant_mode().
Signed-off-by: Stephen M. Cameron <[email protected]>
---
drivers/scsi/hpsa.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 8cf4a0c..ef4dfdd 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -7463,6 +7463,10 @@ static void hpsa_put_ctlr_into_performant_mode(struct ctlr_info *h)
if (hpsa_simple_mode)
return;
+ trans_support = readl(&(h->cfgtable->TransportSupport));
+ if (!(trans_support & PERFORMANT_MODE))
+ return;
+
/* Check for I/O accelerator mode support */
if (trans_support & CFGTBL_Trans_io_accel1) {
transMethod |= CFGTBL_Trans_io_accel1 |
--
1.7.1
On Thu, 2014-04-10 at 17:17 -0500, [email protected] wrote:
> Without this, you'll see a null pointer dereference in
> hpsa_enter_performant_mode().
So I'm not surprised that this patch doesn't solve the problem I am
seeing with DMAR and the hpsa driver hard lockup.
In any case it should address Baoquan's original report, so it confirms
that it is in fact two different sets of issues.
This patch works for me.
Tested-by: Baoquan He <[email protected]>
Thanks
Baoquan
On 04/10/14 at 05:17pm, [email protected] wrote:
>
> Without this, you'll see a null pointer dereference in
> hpsa_enter_performant_mode().
>
> Signed-off-by: Stephen M. Cameron <[email protected]>
> ---
> drivers/scsi/hpsa.c | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index 8cf4a0c..ef4dfdd 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -7463,6 +7463,10 @@ static void hpsa_put_ctlr_into_performant_mode(struct ctlr_info *h)
> if (hpsa_simple_mode)
> return;
>
> + trans_support = readl(&(h->cfgtable->TransportSupport));
> + if (!(trans_support & PERFORMANT_MODE))
> + return;
> +
> /* Check for I/O accelerator mode support */
> if (trans_support & CFGTBL_Trans_io_accel1) {
> transMethod |= CFGTBL_Trans_io_accel1 |
> --
> 1.7.1
>
Your subject line is very tame. It should be the one line summary of
why we apply the patch, so it should read something like
hpsa: fix NULL deref in performant mode
On Thu, 2014-04-10 at 17:17 -0500, [email protected] wrote:
> Without this, you'll see a null pointer dereference in
> hpsa_enter_performant_mode().
The description should be more comprehensible.
I'm clear that the use before initialisation is a bug ... I'm less clear
on why it causes an oops.
> Signed-off-by: Stephen M. Cameron <[email protected]>
> ---
> drivers/scsi/hpsa.c | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index 8cf4a0c..ef4dfdd 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -7463,6 +7463,10 @@ static void hpsa_put_ctlr_into_performant_mode(struct ctlr_info *h)
> if (hpsa_simple_mode)
> return;
>
> + trans_support = readl(&(h->cfgtable->TransportSupport));
> + if (!(trans_support & PERFORMANT_MODE))
> + return;
> +
> /* Check for I/O accelerator mode support */
> if (trans_support & CFGTBL_Trans_io_accel1) {
> transMethod |= CFGTBL_Trans_io_accel1 |
Shouldn't you be moving this check from its previous location, rather
than adding a new one that makes the original obsolete?
James
---
diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 8cf4a0c..9a6e4a2 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -7463,6 +7463,10 @@ static void hpsa_put_ctlr_into_performant_mode(struct ctlr_info *h)
if (hpsa_simple_mode)
return;
+ trans_support = readl(&(h->cfgtable->TransportSupport));
+ if (!(trans_support & PERFORMANT_MODE))
+ return;
+
/* Check for I/O accelerator mode support */
if (trans_support & CFGTBL_Trans_io_accel1) {
transMethod |= CFGTBL_Trans_io_accel1 |
@@ -7479,10 +7483,6 @@ static void hpsa_put_ctlr_into_performant_mode(struct ctlr_info *h)
}
/* TODO, check that this next line h->nreply_queues is correct */
- trans_support = readl(&(h->cfgtable->TransportSupport));
- if (!(trans_support & PERFORMANT_MODE))
- return;
-
h->nreply_queues = h->msix_vector > 0 ? h->msix_vector : 1;
hpsa_get_max_perf_mode_cmds(h);
/* Performant mode ring buffer and supporting data structures */
On Mon, Apr 14, 2014 at 08:45:16AM -0700, James Bottomley wrote:
> Your subject line is very tame. It should be the one line summary of
> why we apply the patch, so it should read something like
>
> hpsa: fix NULL deref in performant mode
>
> On Thu, 2014-04-10 at 17:17 -0500, [email protected] wrote:
> > Without this, you'll see a null pointer dereference in
> > hpsa_enter_performant_mode().
>
> The description should be more comprehensible.
>
> I'm clear that the use before initialisation is a bug ... I'm less clear
> on why it causes an oops.
>
> > Signed-off-by: Stephen M. Cameron <[email protected]>
> > ---
> > drivers/scsi/hpsa.c | 4 ++++
> > 1 files changed, 4 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> > index 8cf4a0c..ef4dfdd 100644
> > --- a/drivers/scsi/hpsa.c
> > +++ b/drivers/scsi/hpsa.c
> > @@ -7463,6 +7463,10 @@ static void hpsa_put_ctlr_into_performant_mode(struct ctlr_info *h)
> > if (hpsa_simple_mode)
> > return;
> >
> > + trans_support = readl(&(h->cfgtable->TransportSupport));
> > + if (!(trans_support & PERFORMANT_MODE))
> > + return;
> > +
> > /* Check for I/O accelerator mode support */
> > if (trans_support & CFGTBL_Trans_io_accel1) {
> > transMethod |= CFGTBL_Trans_io_accel1 |
>
> Shouldn't you be moving this check from its previous location, rather
> than adding a new one that makes the original obsolete?
Oh... I didn't notice that. So that's what happened to that hunk.
Yes, that is what should be done.
I will resend the patch after fixing up this and the commit message.
-- steve
>
> James
>
> ---
>
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index 8cf4a0c..9a6e4a2 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -7463,6 +7463,10 @@ static void hpsa_put_ctlr_into_performant_mode(struct ctlr_info *h)
> if (hpsa_simple_mode)
> return;
>
> + trans_support = readl(&(h->cfgtable->TransportSupport));
> + if (!(trans_support & PERFORMANT_MODE))
> + return;
> +
> /* Check for I/O accelerator mode support */
> if (trans_support & CFGTBL_Trans_io_accel1) {
> transMethod |= CFGTBL_Trans_io_accel1 |
> @@ -7479,10 +7483,6 @@ static void hpsa_put_ctlr_into_performant_mode(struct ctlr_info *h)
> }
>
> /* TODO, check that this next line h->nreply_queues is correct */
> - trans_support = readl(&(h->cfgtable->TransportSupport));
> - if (!(trans_support & PERFORMANT_MODE))
> - return;
> -
> h->nreply_queues = h->msix_vector > 0 ? h->msix_vector : 1;
> hpsa_get_max_perf_mode_cmds(h);
> /* Performant mode ring buffer and supporting data structures */
>