Re: [PATCH RESEND] lpfc: Don't return internal MBXERR_ERROR code from probe function

2017-09-15 Thread Martin K. Petersen 

Stefano,

> Internal error codes happen to be positive, thus the PCI driver
> core won't treat them as failure, but we do. This would cause a
> crash later on as lpfc_pci_remove_one() is called (e.g. as
> shutdown function).

Applied to 4.14/scsi-fixes. Thanks!

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH RESEND] lpfc: Don't return internal MBXERR_ERROR code from probe function

2017-09-15 Thread James Smart 

On 9/14/2017 6:19 PM, Martin K. Petersen wrote:

James/Dick,

Please review!


It seemed to be changing so I was waiting for the final posting...

-- james

Re: [PATCH RESEND] lpfc: Don't return internal MBXERR_ERROR code from probe function

2017-09-14 Thread Martin K. Petersen 

James/Dick,

Please review!

> Internal error codes happen to be positive, thus the PCI driver
> core won't treat them as failure, but we do. This would cause a
> crash later on as lpfc_pci_remove_one() is called (e.g. as
> shutdown function).
>
> Fixes: 6d368e532168 ("[SCSI] lpfc 8.3.24: Add resource extent support")
> Signed-off-by: Stefano Brivio 
> ---
> This seems to have been ignored. Re-sending as suggested by Johannes.
>
>  drivers/scsi/lpfc/lpfc_init.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
> index 491aa95eb0f6..38cc2b5bb5a2 100644
> --- a/drivers/scsi/lpfc/lpfc_init.c
> +++ b/drivers/scsi/lpfc/lpfc_init.c
> @@ -6118,6 +6118,7 @@ lpfc_sli4_driver_resource_setup(struct lpfc_hba *phba)
>   "Extents and RPI headers enabled.\n");
>   }
>   mempool_free(mboxq, phba->mbox_mem_pool);
> + rc = -EIO;
>   goto out_free_bsmbx;
>   }

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH RESEND] lpfc: Don't return internal MBXERR_ERROR code from probe function

2017-09-06 Thread Johannes Thumshirn 
On Wed, Sep 06, 2017 at 11:54:15AM +0200, Stefano Brivio wrote:
> Thanks for your feedback!
> 
> I considered doing something similar, but there are different error
> coded which are set when we reach the label out_free_mbsx. I checked all
> of them (and I hope I didn't miss any), but they all looked correct,
> and in quite a few cases different than -EIO (e.g. -ENODEV).
> 
> So I think always returning -EIO in those cases is not what we want.

We still could pre-assign the rc value:

rc = -EIO;
rc = foo()
if (rc)
goto err_handler;
rc = bar()
if (rc)
goto err_handler;
rc = -ENODEV;
if (rc)
goto somewhere_else;

But let's not complicate things and get this one queued up.

> > Because as this patch shows there's always a chance to miss an 'rc = -EIO'.
> > 
> > Out of curiosity, do you know what's the value of rc in the failure case?
> 
> Yes, MBXERR_ERROR (mentioned in patch subject -- sorry, I could have
> repeated it in the message perhaps).

Ah ok I somehow missed it, sorry.

Byte,
Johannes
-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH RESEND] lpfc: Don't return internal MBXERR_ERROR code from probe function

2017-09-06 Thread Stefano Brivio 
On Wed, 6 Sep 2017 11:30:34 +0200
Johannes Thumshirn  wrote:

> On Wed, Sep 06, 2017 at 11:02:56AM +0200, Stefano Brivio wrote:
> > Internal error codes happen to be positive, thus the PCI driver
> > core won't treat them as failure, but we do. This would cause a
> > crash later on as lpfc_pci_remove_one() is called (e.g. as
> > shutdown function).
> > 
> > Fixes: 6d368e532168 ("[SCSI] lpfc 8.3.24: Add resource extent support")
> > Signed-off-by: Stefano Brivio 
> > ---
> > This seems to have been ignored. Re-sending as suggested by Johannes.
> > 
> >  drivers/scsi/lpfc/lpfc_init.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
> > index 491aa95eb0f6..38cc2b5bb5a2 100644
> > --- a/drivers/scsi/lpfc/lpfc_init.c
> > +++ b/drivers/scsi/lpfc/lpfc_init.c
> > @@ -6118,6 +6118,7 @@ lpfc_sli4_driver_resource_setup(struct lpfc_hba *phba)
> > "Extents and RPI headers enabled.\n");
> > }
> > mempool_free(mboxq, phba->mbox_mem_pool);
> > +   rc = -EIO;
> > goto out_free_bsmbx;
> > }
> >  
> > -- 
> > 2.9.4  
> 
> The patch looks good, but there are lots of 
>   if (rc) {
>   mempool_free(mboxq, phba->mbox_mem_pool);
>   rc = -EIO;
>   goto out_free_bsmbx;
>   }
> 
> in lpfc_sli4_driver_resource_setup(). Shouldn't out_free_bsmbx take care of it
> all so we only have:
>   if (rc)
>   goto out_free_bsmbx;

Thanks for your feedback!

I considered doing something similar, but there are different error
coded which are set when we reach the label out_free_mbsx. I checked all
of them (and I hope I didn't miss any), but they all looked correct,
and in quite a few cases different than -EIO (e.g. -ENODEV).

So I think always returning -EIO in those cases is not what we want.

> Because as this patch shows there's always a chance to miss an 'rc = -EIO'.
> 
> Out of curiosity, do you know what's the value of rc in the failure case?

Yes, MBXERR_ERROR (mentioned in patch subject -- sorry, I could have
repeated it in the message perhaps).


--
Stefano

Re: [PATCH RESEND] lpfc: Don't return internal MBXERR_ERROR code from probe function

2017-09-06 Thread Johannes Thumshirn 
On Wed, Sep 06, 2017 at 11:02:56AM +0200, Stefano Brivio wrote:
> Internal error codes happen to be positive, thus the PCI driver
> core won't treat them as failure, but we do. This would cause a
> crash later on as lpfc_pci_remove_one() is called (e.g. as
> shutdown function).
> 
> Fixes: 6d368e532168 ("[SCSI] lpfc 8.3.24: Add resource extent support")
> Signed-off-by: Stefano Brivio 
> ---
> This seems to have been ignored. Re-sending as suggested by Johannes.
> 
>  drivers/scsi/lpfc/lpfc_init.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
> index 491aa95eb0f6..38cc2b5bb5a2 100644
> --- a/drivers/scsi/lpfc/lpfc_init.c
> +++ b/drivers/scsi/lpfc/lpfc_init.c
> @@ -6118,6 +6118,7 @@ lpfc_sli4_driver_resource_setup(struct lpfc_hba *phba)
>   "Extents and RPI headers enabled.\n");
>   }
>   mempool_free(mboxq, phba->mbox_mem_pool);
> + rc = -EIO;
>   goto out_free_bsmbx;
>   }
>  
> -- 
> 2.9.4

The patch looks good, but there are lots of 
if (rc) {
mempool_free(mboxq, phba->mbox_mem_pool);
rc = -EIO;
goto out_free_bsmbx;
}

in lpfc_sli4_driver_resource_setup(). Shouldn't out_free_bsmbx take care of it
all so we only have:
if (rc)
goto out_free_bsmbx;

Because as this patch shows there's always a chance to miss an 'rc = -EIO'.

Out of curiosity, do you know what's the value of rc in the failure case?

Anyways:
Reviewed-by: Johannes Thumshirn 

-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850