| Message ID | 20200701131454.5255-1-johannes.thumshirn@wdc.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c7e4dd5d84fc8718976e2da395524ec6f4c7f93b |
| Headers | show |
| Series | scsi: mpt3sas: fix error returns in BRM_status_show | expand |
On 2020/07/01 22:15, Johannes Thumshirn wrote: > BRM_status_show() has several error branches, but none of them record the > error in the error return.] > > Also while at it remove the manual mutex_unlock() of the pci_access_mutex > in case of an ongoing pci error recovery or host removal and jump to the > cleanup lable instead. > > Note: we can safely jump to out as from here as io_unit_pg3 is initialized > to NULL and if it hasn't been allocated kfree() skips the NULL pointer. > > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > --- > drivers/scsi/mpt3sas/mpt3sas_ctl.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c > index 62e552838565..70d2d0987249 100644 > --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c > +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c > @@ -3149,20 +3149,20 @@ BRM_status_show(struct device *cdev, struct device_attribute *attr, > } > /* pci_access_mutex lock acquired by sysfs show path */ > mutex_lock(&ioc->pci_access_mutex); > - if (ioc->pci_error_recovery || ioc->remove_host) { > - mutex_unlock(&ioc->pci_access_mutex); > - return 0; > - } > + if (ioc->pci_error_recovery || ioc->remove_host) > + goto out; > > /* allocate upto GPIOVal 36 entries */ > sz = offsetof(Mpi2IOUnitPage3_t, GPIOVal) + (sizeof(u16) * 36); > io_unit_pg3 = kzalloc(sz, GFP_KERNEL); > if (!io_unit_pg3) { > + rc = -ENOMEM; > ioc_err(ioc, "%s: failed allocating memory for iounit_pg3: (%d) bytes\n", > __func__, sz); > goto out; > } > > + rc = -EINVAL; > if (mpt3sas_config_get_iounit_pg3(ioc, &mpi_reply, io_unit_pg3, sz) != > 0) { > ioc_err(ioc, "%s: failed reading iounit_pg3\n", > Looks good. Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>
On Thu, Jul 2, 2020 at 6:36 AM Damien Le Moal <Damien.LeMoal@wdc.com> wrote: > > On 2020/07/01 22:15, Johannes Thumshirn wrote: > > BRM_status_show() has several error branches, but none of them record the > > error in the error return.] > > > > Also while at it remove the manual mutex_unlock() of the pci_access_mutex > > in case of an ongoing pci error recovery or host removal and jump to the > > cleanup lable instead. > > > > Note: we can safely jump to out as from here as io_unit_pg3 is initialized > > to NULL and if it hasn't been allocated kfree() skips the NULL pointer. > > > > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > > --- > > drivers/scsi/mpt3sas/mpt3sas_ctl.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c > > index 62e552838565..70d2d0987249 100644 > > --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c > > +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c > > @@ -3149,20 +3149,20 @@ BRM_status_show(struct device *cdev, struct device_attribute *attr, > > } > > /* pci_access_mutex lock acquired by sysfs show path */ > > mutex_lock(&ioc->pci_access_mutex); > > - if (ioc->pci_error_recovery || ioc->remove_host) { > > - mutex_unlock(&ioc->pci_access_mutex); > > - return 0; > > - } > > + if (ioc->pci_error_recovery || ioc->remove_host) > > + goto out; > > > > /* allocate upto GPIOVal 36 entries */ > > sz = offsetof(Mpi2IOUnitPage3_t, GPIOVal) + (sizeof(u16) * 36); > > io_unit_pg3 = kzalloc(sz, GFP_KERNEL); > > if (!io_unit_pg3) { > > + rc = -ENOMEM; > > ioc_err(ioc, "%s: failed allocating memory for iounit_pg3: (%d) bytes\n", > > __func__, sz); > > goto out; > > } > > > > + rc = -EINVAL; > > if (mpt3sas_config_get_iounit_pg3(ioc, &mpi_reply, io_unit_pg3, sz) != > > 0) { > > ioc_err(ioc, "%s: failed reading iounit_pg3\n", > > > > Looks good. > > Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com> Patch looks good. Acked-by: sreekanth reddy <sreekanth.reddy@broadcom.com> Thanks, Sreekanth > > -- > Damien Le Moal > Western Digital Research
On Wed, 1 Jul 2020 22:14:54 +0900, Johannes Thumshirn wrote: > BRM_status_show() has several error branches, but none of them record the > error in the error return.] > > Also while at it remove the manual mutex_unlock() of the pci_access_mutex > in case of an ongoing pci error recovery or host removal and jump to the > cleanup lable instead. > > [...] Applied to 5.8/scsi-fixes, thanks! [1/1] scsi: mpt3sas: Fix error returns in BRM_status_show https://git.kernel.org/mkp/scsi/c/c7e4dd5d84fc
diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c index 62e552838565..70d2d0987249 100644 --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c @@ -3149,20 +3149,20 @@ BRM_status_show(struct device *cdev, struct device_attribute *attr, } /* pci_access_mutex lock acquired by sysfs show path */ mutex_lock(&ioc->pci_access_mutex); - if (ioc->pci_error_recovery || ioc->remove_host) { - mutex_unlock(&ioc->pci_access_mutex); - return 0; - } + if (ioc->pci_error_recovery || ioc->remove_host) + goto out; /* allocate upto GPIOVal 36 entries */ sz = offsetof(Mpi2IOUnitPage3_t, GPIOVal) + (sizeof(u16) * 36); io_unit_pg3 = kzalloc(sz, GFP_KERNEL); if (!io_unit_pg3) { + rc = -ENOMEM; ioc_err(ioc, "%s: failed allocating memory for iounit_pg3: (%d) bytes\n", __func__, sz); goto out; } + rc = -EINVAL; if (mpt3sas_config_get_iounit_pg3(ioc, &mpi_reply, io_unit_pg3, sz) != 0) { ioc_err(ioc, "%s: failed reading iounit_pg3\n",
BRM_status_show() has several error branches, but none of them record the error in the error return.] Also while at it remove the manual mutex_unlock() of the pci_access_mutex in case of an ongoing pci error recovery or host removal and jump to the cleanup lable instead. Note: we can safely jump to out as from here as io_unit_pg3 is initialized to NULL and if it hasn't been allocated kfree() skips the NULL pointer. Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> --- drivers/scsi/mpt3sas/mpt3sas_ctl.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)