Date:   Thu, 11 Nov 2021 10:33:41 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Mark Gross <markgross@kernel.org>
Cc:     Hans de Goede <hdegoede@redhat.com>,
        Vadim Pasternak <vadimp@nvidia.com>,
        Michael Shych <michaelsh@nvidia.com>,
        platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org,
        kernel-janitors@vger.kernel.org
Subject: Re: [PATCH] platform/mellanox: mlxreg-lc: fix error code in
 mlxreg_lc_create_static_devices()
Message-ID: <20211111073341.GN2001@kadam>
References: <20211110074346.GB5176@kili>
 <20211110223147.GA16388@T470>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211110223147.GA16388@T470>
User-Agent: Mutt/1.9.4 (2018-02-28)
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?Y2IAkdCxRVCmPpplBlMLLlDfdLMasXW7QQ5DiBZG0bs0yRb3x2tVLI60OF2i?=
 =?us-ascii?Q?ihvzgG/KsEJRGhd+2GGjVTsftH6zKec1eKLs1E8AHOCGbNBCU+jdk6lxqhPu?=
 =?us-ascii?Q?vr8W/zTplKyHz/La7t1ouUgXh3KX0LZfmVF0iCqBf/qpht8K4OKPHTA7/k55?=
 =?us-ascii?Q?oXyAuB0cCWcvbaE/Z4fxC3IJq+qVmp1Q5uqbqBVKCbcbEQv9XcdabiK18Glf?=
 =?us-ascii?Q?AslzDAsQdFscE5VDOOsIWOzaDanmdufzZoC9zdVJ30Rhj1d8KKq9xndOHVY+?=
 =?us-ascii?Q?PJjmmq+2wqJcHWLygj/Pj2nFautxf0GMq1IfJKH2gNsR2pEc39lZu8dyWQ0I?=
 =?us-ascii?Q?0LFZE6CscjP20bvsEqmbyl4mxLWzw2ETxcmqBw13GJY1KZYhCY5m/L4h0/De?=
 =?us-ascii?Q?6hQvYtOLn/TTYuhb2ixYh8F1AbppGG6TPzG3kXgwZaKkh9qvTEwIGCSnoFaD?=
 =?us-ascii?Q?OLghfePg5GpoZbcvl+Wwr3Z8GKbOvRohzwkpZGqvBlWvZzVvfewtBkhX5bYd?=
 =?us-ascii?Q?lujX9lAU9Snh6k7GmcaJIki6966ScuEf1i0GTOMlsCrKKIzRPxQ7kEj7Msn4?=
 =?us-ascii?Q?tL8DIQiYhx1oExzuYCMFl5z9+GRwjGr/a60wSa95K52qXMPNBB8++KIcPMig?=
 =?us-ascii?Q?+SQ5LXWUTnVmsowS39bL0NxDSwW6mCw3UbKaTeOJDM1TYLgkHHEeYWhUnR14?=
 =?us-ascii?Q?Eda9rHobP8V4ZVSCbwyjXfTr68ILDh3hzZxUQrFfWhbstBsIVN7xF/8wIJyV?=
 =?us-ascii?Q?jLyhaaBRX/2oGsuzhq7VGzVasQmgZOwdFZ1EsLv7QlS2eHMxKDEGYMlmA9E6?=
 =?us-ascii?Q?xOCrvG0lrpgoX15XEWenzsVK3JI2LJ1cdFTQLQ9IcAhKksCUka8Hzq5x4zIl?=
 =?us-ascii?Q?P53fgxHPx/Zyt4sKfibbxZbKcf+hsgTdL44oU3xMz5uvSZgl/ZD1aQaf7gMK?=
 =?us-ascii?Q?0ffANTcbcvee3WEJttsbMSaVJWNTGGQZqGa0n6OeLQ9bj9tK9ZE67bfHbo6S?=
 =?us-ascii?Q?OJDpebzZqDepQrizPjYCLoyD/7E5XimnYe3aIppk1saQodB+MIO3OUCjvLnS?=
 =?us-ascii?Q?vxtSLdZOuQBqxwDCj0XNeE0T4t5sHZqIJDLJ/Tl+ZddDjTO+Q80UZtu5my+K?=
 =?us-ascii?Q?Q0BtqI3+9kEetzVo9Jik2SMzjAC4Z138D2YAQLZHMes3GHLnhPG+S/FKE2N0?=
 =?us-ascii?Q?aF3TMtdxnK2XiPzjFxcYHRSFhgdlgb0fMes8EeGeKbYaWyXipYS6xyGIS+5w?=
 =?us-ascii?Q?ycAeqY7wADrJa1e+WRbUtTyIQFkZHV0MZSqxi+LTpTaRcWsIzGiVkG6JaqRv?=
 =?us-ascii?Q?nroQwR8SbjZkjzCYWOHGZ4NFre4dyvKujNvUBREMyGJ1PjuXahzpQNkXeoQJ?=
 =?us-ascii?Q?+UUSz4w53tqwSt7MIh15BUM+rDrVWmA9BlY2JySdcrw26Ras6fUS+QxEbuE+?=
 =?us-ascii?Q?G0hA4WokFTQcV4vVm+eXpXDrNpdnxn5/A6LYu/ZqP5/7CtJS6+tv5zK1HvSw?=
 =?us-ascii?Q?nsspymzHFY+/gIvD8KtuZrvSAZTN51h5x+hBjOffFtBbVareQTHolWtQm1ni?=
 =?us-ascii?Q?A7KqY3Qp91dVXz706DcQQ5/kpXiofuCBnDN2ZLsS7z9oUVmVO7m8dSWL3Kra?=
 =?us-ascii?Q?gHRE9eYb8C8oaqXMDyTTI0gjZDMoJ7gpKQSnoPgq4RMfPQ+5unpaj6qaD3Bu?=
 =?us-ascii?Q?j7f971vN0pWt8cLhN1jKBXSRI30=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ba91375c-ec8e-4dba-e393-08d9a4e5a1b6
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Nov 2021 07:34:02.2216
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: s0jlg/FLwAXjpi9aBE0G6dfAsEB1AFWJvxpN3Do9Vo5eZQN4TQx1LPW3IzD4gKrC9JSU1BdtuSps2aUVt9aC7X3WeJDi8LS0v4qmcb6R56M=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO6PR10MB5409
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10164 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 malwarescore=0 spamscore=0
 suspectscore=0 adultscore=0 bulkscore=0 mlxlogscore=999 phishscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000
 definitions=main-2111110044
X-Proofpoint-GUID: 1dk1ENzSvT1G6_dU0V2phdgLvhfVMGPY
X-Proofpoint-ORIG-GUID: 1dk1ENzSvT1G6_dU0V2phdgLvhfVMGPY
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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