[PATCH] cxl: Unlock on error in probe
We should unlock if get_cxl_adapter() fails. Fixes: 594ff7d067ca ("cxl: Support to flash a new image on the adapter from a guest") Signed-off-by: Dan Carpenter diff --git a/drivers/misc/cxl/flash.c b/drivers/misc/cxl/flash.c index 7c61c70ba3f6..37475abea3e6 100644 --- a/drivers/misc/cxl/flash.c +++ b/drivers/misc/cxl/flash.c @@ -401,8 +401,10 @@ static int device_open(struct inode *inode, struct file *file) if (down_interruptible(&sem) != 0) return -EPERM; - if (!(adapter = get_cxl_adapter(adapter_num))) - return -ENODEV; + if (!(adapter = get_cxl_adapter(adapter_num))) { + rc = -ENODEV; + goto err_unlock; + } file->private_data = adapter; continue_token = 0; @@ -446,6 +448,8 @@ static int device_open(struct inode *inode, struct file *file) free_page((unsigned long) le); err: put_device(&adapter->dev); +err_unlock: + up(&sem); return rc; }
Re: [PATCH] cxl: Unlock on error in probe
On 05/05/17 15:34, Dan Carpenter wrote: We should unlock if get_cxl_adapter() fails. Fixes: 594ff7d067ca ("cxl: Support to flash a new image on the adapter from a guest") Signed-off-by: Dan Carpenter diff --git a/drivers/misc/cxl/flash.c b/drivers/misc/cxl/flash.c index 7c61c70ba3f6..37475abea3e6 100644 --- a/drivers/misc/cxl/flash.c +++ b/drivers/misc/cxl/flash.c @@ -401,8 +401,10 @@ static int device_open(struct inode *inode, struct file *file) if (down_interruptible(&sem) != 0) return -EPERM; - if (!(adapter = get_cxl_adapter(adapter_num))) - return -ENODEV; + if (!(adapter = get_cxl_adapter(adapter_num))) { + rc = -ENODEV; + goto err_unlock; + } file->private_data = adapter; continue_token = 0; @@ -446,6 +448,8 @@ static int device_open(struct inode *inode, struct file *file) free_page((unsigned long) le); err: put_device(&adapter->dev); +err_unlock: + up(&sem); return rc; } sem is a global and it looks like it's intended to be held after device_open() returns and only released in device_close(), so this looks wrong. -- Andrew Donnellan OzLabs, ADL Canberra andrew.donnel...@au1.ibm.com IBM Australia Limited
Re: [PATCH] cxl: Unlock on error in probe
Am 05.05.2017 09:14, schrieb Andrew Donnellan: > On 05/05/17 15:34, Dan Carpenter wrote: >> We should unlock if get_cxl_adapter() fails. >> >> Fixes: 594ff7d067ca ("cxl: Support to flash a new image on the adapter >> from a guest") >> Signed-off-by: Dan Carpenter >> >> diff --git a/drivers/misc/cxl/flash.c b/drivers/misc/cxl/flash.c >> index 7c61c70ba3f6..37475abea3e6 100644 >> --- a/drivers/misc/cxl/flash.c >> +++ b/drivers/misc/cxl/flash.c >> @@ -401,8 +401,10 @@ static int device_open(struct inode *inode, >> struct file *file) >> if (down_interruptible(&sem) != 0) >> return -EPERM; >> >> -if (!(adapter = get_cxl_adapter(adapter_num))) >> -return -ENODEV; >> +if (!(adapter = get_cxl_adapter(adapter_num))) { >> +rc = -ENODEV; >> +goto err_unlock; >> +} >> >> file->private_data = adapter; >> continue_token = 0; >> @@ -446,6 +448,8 @@ static int device_open(struct inode *inode, struct >> file *file) >> free_page((unsigned long) le); >> err: >> put_device(&adapter->dev); >> +err_unlock: >> +up(&sem); >> >> return rc; >> } > > sem is a global and it looks like it's intended to be held after > device_open() returns and only released in device_close(), so this looks > wrong. > the patch relates to the error path, do you expect a close() after the open() failed ? re, wh
Re: [PATCH] cxl: Unlock on error in probe
On Fri, May 05, 2017 at 05:14:15PM +1000, Andrew Donnellan wrote: > sem is a global and it looks like it's intended to be held after > device_open() returns and only released in device_close(), so this looks > wrong. > This doesn't affect the success path, it only means that if device_open() fails because we don't have enough free memory or whatever then we can try again later. regards, dan carpenter
Re: [PATCH] cxl: Unlock on error in probe
On Fri, May 05, 2017 at 09:23:02AM +0200, walter harms wrote: > > sem is a global and it looks like it's intended to be held after > > device_open() returns and only released in device_close(), so this looks > > wrong. > > > > the patch relates to the error path, do you expect a close() after the open() > failed ? > You can't close if open fails. Please don't ask rhetorical questions. Just state everything in short sentences using simple words. regards, dan carpenter
Re: [PATCH] cxl: Unlock on error in probe
On 05/05/17 17:37, Dan Carpenter wrote: On Fri, May 05, 2017 at 05:14:15PM +1000, Andrew Donnellan wrote: sem is a global and it looks like it's intended to be held after device_open() returns and only released in device_close(), so this looks wrong. This doesn't affect the success path, it only means that if device_open() fails because we don't have enough free memory or whatever then we can try again later. Argh, I completely missed the "return 0" above err1 and thought the success path fell through. My mistake, that's what I get for trying to review patches after staring at hardware simulation logs all day... In that case, the patch looks fine to me. Reviewed-by: Andrew Donnellan -- Andrew Donnellan OzLabs, ADL Canberra andrew.donnel...@au1.ibm.com IBM Australia Limited
Re: [PATCH] cxl: Unlock on error in probe
Le 05/05/2017 à 07:34, Dan Carpenter a écrit : We should unlock if get_cxl_adapter() fails. Fixes: 594ff7d067ca ("cxl: Support to flash a new image on the adapter from a guest") Signed-off-by: Dan Carpenter Acked-by: Frederic Barrat Thanks! Fred diff --git a/drivers/misc/cxl/flash.c b/drivers/misc/cxl/flash.c index 7c61c70ba3f6..37475abea3e6 100644 --- a/drivers/misc/cxl/flash.c +++ b/drivers/misc/cxl/flash.c @@ -401,8 +401,10 @@ static int device_open(struct inode *inode, struct file *file) if (down_interruptible(&sem) != 0) return -EPERM; - if (!(adapter = get_cxl_adapter(adapter_num))) - return -ENODEV; + if (!(adapter = get_cxl_adapter(adapter_num))) { + rc = -ENODEV; + goto err_unlock; + } file->private_data = adapter; continue_token = 0; @@ -446,6 +448,8 @@ static int device_open(struct inode *inode, struct file *file) free_page((unsigned long) le); err: put_device(&adapter->dev); +err_unlock: + up(&sem); return rc; }