* RE: [PATCH] platform/mellanox: mlxreg-lc: fix error code in mlxreg_lc_create_static_devices()
2021-11-10 7:43 [PATCH] platform/mellanox: mlxreg-lc: fix error code in mlxreg_lc_create_static_devices() Dan Carpenter
@ 2021-11-10 10:27 ` Vadim Pasternak
2021-11-10 22:31 ` Mark Gross
2021-11-11 9:47 ` Hans de Goede
2 siblings, 0 replies; 6+ messages in thread
From: Vadim Pasternak @ 2021-11-10 10:27 UTC (permalink / raw)
To: Dan Carpenter, Hans de Goede
Cc: Mark Gross, Michael Shych, platform-driver-x86@vger.kernel.org,
linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org
> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Sent: Wednesday, November 10, 2021 9:44 AM
> To: Hans de Goede <hdegoede@redhat.com>; Vadim Pasternak
> <vadimp@nvidia.com>
> Cc: Mark Gross <markgross@kernel.org>; Michael Shych
> <michaelsh@nvidia.com>; platform-driver-x86@vger.kernel.org; linux-
> kernel@vger.kernel.org; kernel-janitors@vger.kernel.org
> Subject: [PATCH] platform/mellanox: mlxreg-lc: fix error code in
> mlxreg_lc_create_static_devices()
>
> This code should be using PTR_ERR() instead of IS_ERR(). And because it's
> using the wrong "dev->client" pointer, the IS_ERR() check will be false,
> meaning the function returns success.
>
> Fixes: 62f9529b8d5c ("platform/mellanox: mlxreg-lc: Add initial support for
> Nvidia line card devices")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Acked-by: Vadim Pasternak <vadimp@nvidia.com>
> ---
> drivers/platform/mellanox/mlxreg-lc.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/mellanox/mlxreg-lc.c
> b/drivers/platform/mellanox/mlxreg-lc.c
> index 0b7f58feb701..c897a2f15840 100644
> --- a/drivers/platform/mellanox/mlxreg-lc.c
> +++ b/drivers/platform/mellanox/mlxreg-lc.c
> @@ -413,7 +413,7 @@ mlxreg_lc_create_static_devices(struct mlxreg_lc
> *mlxreg_lc, struct mlxreg_hotpl
> int size)
> {
> struct mlxreg_hotplug_device *dev = devs;
> - int i;
> + int i, ret;
>
> /* Create static I2C device feeding by auxiliary or main power. */
> for (i = 0; i < size; i++, dev++) {
> @@ -423,6 +423,7 @@ mlxreg_lc_create_static_devices(struct mlxreg_lc
> *mlxreg_lc, struct mlxreg_hotpl
> dev->brdinfo->type, dev->nr, dev->brdinfo-
> >addr);
>
> dev->adapter = NULL;
> + ret = PTR_ERR(dev->client);
> goto fail_create_static_devices;
> }
> }
> @@ -435,7 +436,7 @@ mlxreg_lc_create_static_devices(struct mlxreg_lc
> *mlxreg_lc, struct mlxreg_hotpl
> i2c_unregister_device(dev->client);
> dev->client = NULL;
> }
> - return IS_ERR(dev->client);
> + return ret;
> }
>
> static void
> --
> 2.20.1
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] platform/mellanox: mlxreg-lc: fix error code in mlxreg_lc_create_static_devices()
2021-11-10 7:43 [PATCH] platform/mellanox: mlxreg-lc: fix error code in mlxreg_lc_create_static_devices() Dan Carpenter
2021-11-10 10:27 ` Vadim Pasternak
@ 2021-11-10 22:31 ` Mark Gross
2021-11-11 7:33 ` Dan Carpenter
2021-11-11 9:47 ` Hans de Goede
2 siblings, 1 reply; 6+ messages in thread
From: Mark Gross @ 2021-11-10 22:31 UTC (permalink / raw)
To: Dan Carpenter
Cc: Hans de Goede, Vadim Pasternak, Mark Gross, Michael Shych,
platform-driver-x86, linux-kernel, kernel-janitors
On Wed, Nov 10, 2021 at 10:43:46AM +0300, Dan Carpenter wrote:
> This code should be using PTR_ERR() instead of IS_ERR(). And because
> it's using the wrong "dev->client" pointer, the IS_ERR() check will be
> false, meaning the function returns success.
>
> Fixes: 62f9529b8d5c ("platform/mellanox: mlxreg-lc: Add initial support for Nvidia line card devices")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> drivers/platform/mellanox/mlxreg-lc.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/mellanox/mlxreg-lc.c b/drivers/platform/mellanox/mlxreg-lc.c
> index 0b7f58feb701..c897a2f15840 100644
> --- a/drivers/platform/mellanox/mlxreg-lc.c
> +++ b/drivers/platform/mellanox/mlxreg-lc.c
> @@ -413,7 +413,7 @@ mlxreg_lc_create_static_devices(struct mlxreg_lc *mlxreg_lc, struct mlxreg_hotpl
> int size)
> {
> struct mlxreg_hotplug_device *dev = devs;
> - int i;
> + int i, ret;
>
> /* Create static I2C device feeding by auxiliary or main power. */
> for (i = 0; i < size; i++, dev++) {
> @@ -423,6 +423,7 @@ mlxreg_lc_create_static_devices(struct mlxreg_lc *mlxreg_lc, struct mlxreg_hotpl
> dev->brdinfo->type, dev->nr, dev->brdinfo->addr);
>
> dev->adapter = NULL;
> + ret = PTR_ERR(dev->client);
ret is only set on this error path.
can we get to the return without setting ret?
--mark
> goto fail_create_static_devices;
> }
> }
> @@ -435,7 +436,7 @@ mlxreg_lc_create_static_devices(struct mlxreg_lc *mlxreg_lc, struct mlxreg_hotpl
> i2c_unregister_device(dev->client);
> dev->client = NULL;
> }
> - return IS_ERR(dev->client);
> + return ret;
> }
>
> static void
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] platform/mellanox: mlxreg-lc: fix error code in mlxreg_lc_create_static_devices()
2021-11-10 22:31 ` Mark Gross
@ 2021-11-11 7:33 ` Dan Carpenter
2021-11-11 7:39 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2021-11-11 7:33 UTC (permalink / raw)
To: Mark Gross
Cc: Hans de Goede, Vadim Pasternak, Michael Shych,
platform-driver-x86, linux-kernel, kernel-janitors
On Wed, Nov 10, 2021 at 02:31:47PM -0800, Mark Gross wrote:
> On Wed, Nov 10, 2021 at 10:43:46AM +0300, Dan Carpenter wrote:
> > This code should be using PTR_ERR() instead of IS_ERR(). And because
> > it's using the wrong "dev->client" pointer, the IS_ERR() check will be
> > false, meaning the function returns success.
> >
> > Fixes: 62f9529b8d5c ("platform/mellanox: mlxreg-lc: Add initial support for Nvidia line card devices")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > drivers/platform/mellanox/mlxreg-lc.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/platform/mellanox/mlxreg-lc.c b/drivers/platform/mellanox/mlxreg-lc.c
> > index 0b7f58feb701..c897a2f15840 100644
> > --- a/drivers/platform/mellanox/mlxreg-lc.c
> > +++ b/drivers/platform/mellanox/mlxreg-lc.c
> > @@ -413,7 +413,7 @@ mlxreg_lc_create_static_devices(struct mlxreg_lc *mlxreg_lc, struct mlxreg_hotpl
> > int size)
> > {
> > struct mlxreg_hotplug_device *dev = devs;
> > - int i;
> > + int i, ret;
> >
> > /* Create static I2C device feeding by auxiliary or main power. */
> > for (i = 0; i < size; i++, dev++) {
> > @@ -423,6 +423,7 @@ mlxreg_lc_create_static_devices(struct mlxreg_lc *mlxreg_lc, struct mlxreg_hotpl
> > dev->brdinfo->type, dev->nr, dev->brdinfo->addr);
> >
> > dev->adapter = NULL;
> > + ret = PTR_ERR(dev->client);
> ret is only set on this error path.
> can we get to the return without setting ret?
>
No.
:P
There two ways to read that question is if the patch introduces an
uninitialized variable bug and I would be super embarrassed if I did
something like that with all the QC scripts that I have to prevent it.
The other way to read that question is if it's possible to not introduce
the "ret" variable but instead figure it out at the end. But the error
code needs to be preserved at this point because we change change the
"dev" pointer and set "dev->adapter" to NULL.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] platform/mellanox: mlxreg-lc: fix error code in mlxreg_lc_create_static_devices()
2021-11-11 7:33 ` Dan Carpenter
@ 2021-11-11 7:39 ` Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2021-11-11 7:39 UTC (permalink / raw)
To: Mark Gross
Cc: Hans de Goede, Vadim Pasternak, Michael Shych,
platform-driver-x86, linux-kernel, kernel-janitors
On Thu, Nov 11, 2021 at 10:33:41AM +0300, Dan Carpenter wrote:
> On Wed, Nov 10, 2021 at 02:31:47PM -0800, Mark Gross wrote:
> > On Wed, Nov 10, 2021 at 10:43:46AM +0300, Dan Carpenter wrote:
> > > This code should be using PTR_ERR() instead of IS_ERR(). And because
> > > it's using the wrong "dev->client" pointer, the IS_ERR() check will be
> > > false, meaning the function returns success.
> > >
> > > Fixes: 62f9529b8d5c ("platform/mellanox: mlxreg-lc: Add initial support for Nvidia line card devices")
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > ---
> > > drivers/platform/mellanox/mlxreg-lc.c | 5 +++--
> > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/platform/mellanox/mlxreg-lc.c b/drivers/platform/mellanox/mlxreg-lc.c
> > > index 0b7f58feb701..c897a2f15840 100644
> > > --- a/drivers/platform/mellanox/mlxreg-lc.c
> > > +++ b/drivers/platform/mellanox/mlxreg-lc.c
> > > @@ -413,7 +413,7 @@ mlxreg_lc_create_static_devices(struct mlxreg_lc *mlxreg_lc, struct mlxreg_hotpl
> > > int size)
> > > {
> > > struct mlxreg_hotplug_device *dev = devs;
> > > - int i;
> > > + int i, ret;
> > >
> > > /* Create static I2C device feeding by auxiliary or main power. */
> > > for (i = 0; i < size; i++, dev++) {
> > > @@ -423,6 +423,7 @@ mlxreg_lc_create_static_devices(struct mlxreg_lc *mlxreg_lc, struct mlxreg_hotpl
> > > dev->brdinfo->type, dev->nr, dev->brdinfo->addr);
> > >
> > > dev->adapter = NULL;
> > > + ret = PTR_ERR(dev->client);
> > ret is only set on this error path.
> > can we get to the return without setting ret?
> >
>
> No.
>
> :P
>
> There two ways to read that question is if the patch introduces an
> uninitialized variable bug and I would be super embarrassed if I did
> something like that with all the QC scripts that I have to prevent it.
> The other way to read that question is if it's possible to not introduce
> the "ret" variable but instead figure it out at the end. But the error
> code needs to be preserved at this point because we change change the
> "dev" pointer and set "dev->adapter" to NULL.
^^^^^^^^^^^^
I meant "dev->client" not "dev->adapter".
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] platform/mellanox: mlxreg-lc: fix error code in mlxreg_lc_create_static_devices()
2021-11-10 7:43 [PATCH] platform/mellanox: mlxreg-lc: fix error code in mlxreg_lc_create_static_devices() Dan Carpenter
2021-11-10 10:27 ` Vadim Pasternak
2021-11-10 22:31 ` Mark Gross
@ 2021-11-11 9:47 ` Hans de Goede
2 siblings, 0 replies; 6+ messages in thread
From: Hans de Goede @ 2021-11-11 9:47 UTC (permalink / raw)
To: Dan Carpenter, Vadim Pasternak
Cc: Mark Gross, Michael Shych, platform-driver-x86, linux-kernel,
kernel-janitors
Hi,
On 11/10/21 08:43, Dan Carpenter wrote:
> This code should be using PTR_ERR() instead of IS_ERR(). And because
> it's using the wrong "dev->client" pointer, the IS_ERR() check will be
> false, meaning the function returns success.
>
> Fixes: 62f9529b8d5c ("platform/mellanox: mlxreg-lc: Add initial support for Nvidia line card devices")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
I will add this fix to my tree once 5.16-rc1 is out and
I will include this fix in my first pdx86 fixes pull-req
for 5.16.
Regards,
Hans
> ---
> drivers/platform/mellanox/mlxreg-lc.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/mellanox/mlxreg-lc.c b/drivers/platform/mellanox/mlxreg-lc.c
> index 0b7f58feb701..c897a2f15840 100644
> --- a/drivers/platform/mellanox/mlxreg-lc.c
> +++ b/drivers/platform/mellanox/mlxreg-lc.c
> @@ -413,7 +413,7 @@ mlxreg_lc_create_static_devices(struct mlxreg_lc *mlxreg_lc, struct mlxreg_hotpl
> int size)
> {
> struct mlxreg_hotplug_device *dev = devs;
> - int i;
> + int i, ret;
>
> /* Create static I2C device feeding by auxiliary or main power. */
> for (i = 0; i < size; i++, dev++) {
> @@ -423,6 +423,7 @@ mlxreg_lc_create_static_devices(struct mlxreg_lc *mlxreg_lc, struct mlxreg_hotpl
> dev->brdinfo->type, dev->nr, dev->brdinfo->addr);
>
> dev->adapter = NULL;
> + ret = PTR_ERR(dev->client);
> goto fail_create_static_devices;
> }
> }
> @@ -435,7 +436,7 @@ mlxreg_lc_create_static_devices(struct mlxreg_lc *mlxreg_lc, struct mlxreg_hotpl
> i2c_unregister_device(dev->client);
> dev->client = NULL;
> }
> - return IS_ERR(dev->client);
> + return ret;
> }
>
> static void
>
^ permalink raw reply [flat|nested] 6+ messages in thread