From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Google-Smtp-Source: AH8x2263b1ws+t9a6VqkMQj1Rvov7nIRtWSk8ato8JTSHmdb5tvUPTS4qIzC/jw9KoyJ7dMl34IV ARC-Seal: i=1; a=rsa-sha256; t=1516919327; cv=none; d=google.com; s=arc-20160816; b=FpS1ovYF40ynsyPDEdW+d1eVaR6MKKFOJijDHQ+txAgHqi1lD+kYHz5Dp1DIdR6RSR USxsFXFkMSaesu1ZIHxahHoCB/C05da3UKhqUcKViYAf99br3CncWmAa3MXKvxXONcSK iinbB5d4str7VZV98agU5NKOHP3SXtBiFmlsGrpBX5j9RcESx/iq2PrJQnqSgl44j2Sd WNswumzlhnBVfYJaGzHCqyFAMnLU5L689j38T0JKayvtAxtuJmnMhuY5qPfHYbFGtvJZ UbaE6lupz2vAxh5f0WQQmAecPEbunqPbxsYqRdAulXiG8ZBJyqwLGj5RRsJgL3JfMDyd hl0w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=user-agent:in-reply-to:content-disposition:mime-version:references :message-id:subject:cc:to:from:date:dkim-signature :arc-authentication-results; bh=G9p7LMe2EMzI/6q2gWI3TXRbo9slnWVIaqH0uRNS5DY=; b=uXa4ZdxaBxpDSX1qxff5lF1C9ZxPs3Nk7ndfAe+c/J9LxuvzgQrfvmdN1I7EXCel6P KGLJGoiQK3FiWfgt2LxJxWekmK7HWxVktsNiT2hiq3WcxzjHjfnFcgDWVT59vh9DRWMu 09vtscbS+mkVGW1QCspRtiW4dCBKa22yWI+LJuMeL7whyycDXRYdAG6pBMGihZj+o9hq dMRIdeOAuQEB3rtoz8yd2AT0tIL+ULoP/0SAwMYkSrgeqNKEAazRiH/vl1narjAn2yXP sP9r5auigyyHN1qtsQDFzW/xOM7JNlPkc0fJCcVrnVibCOxCQTALySH3sqFeJM+Pxyf7 6zng== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@infradead.org header.s=bombadil.20170209 header.b=CuR0FLDq; spf=pass (google.com: best guess record for domain of dvhart@infradead.org designates 65.50.211.133 as permitted sender) smtp.mailfrom=dvhart@infradead.org Authentication-Results: mx.google.com; dkim=pass header.i=@infradead.org header.s=bombadil.20170209 header.b=CuR0FLDq; spf=pass (google.com: best guess record for domain of dvhart@infradead.org designates 65.50.211.133 as permitted sender) smtp.mailfrom=dvhart@infradead.org Date: Thu, 25 Jan 2018 14:28:44 -0800 From: Darren Hart To: Vadim Pasternak Cc: andy.shevchenko@gmail.com, gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org, platform-driver-x86@vger.kernel.org, jiri@resnulli.us Subject: Re: [patch v11 12/12] platform/mellanox: Add validation of return code of hotplug device creation routine Message-ID: <20180125222844.GF24122@fury> References: <1516826098-125036-1-git-send-email-vadimp@mellanox.com> <1516826098-125036-4-git-send-email-vadimp@mellanox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1516826098-125036-4-git-send-email-vadimp@mellanox.com> User-Agent: Mutt/1.8.0 (2017-02-23) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: =?utf-8?q?1590500118317895596?= X-GMAIL-MSGID: =?utf-8?q?1590605200650662844?= X-Mailing-List: linux-kernel@vger.kernel.org List-ID: On Wed, Jan 24, 2018 at 08:34:58PM +0000, Vadim Pasternak wrote: > Adding validation of return code of mlxreg_hotplug_device_create. It could > fail in case the requested adapter is not available or if client can not > be connected to the adapter. Error is to be reported in case of bad flow. > I had dropped the removal of the dev_err messages patch as it was based on an inaccurate assumption that the return codes were checked. We could just include that change together with this change as they are tightly coupled. But... > Signed-off-by: Vadim Pasternak > --- > drivers/platform/mellanox/mlxreg-hotplug.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/platform/mellanox/mlxreg-hotplug.c b/drivers/platform/mellanox/mlxreg-hotplug.c > index c806451..e852219 100644 > --- a/drivers/platform/mellanox/mlxreg-hotplug.c > +++ b/drivers/platform/mellanox/mlxreg-hotplug.c > @@ -263,13 +263,15 @@ mlxreg_hotplug_work_helper(struct mlxreg_hotplug_priv_data *priv, > if (item->inversed) > mlxreg_hotplug_device_destroy(data); > else > - mlxreg_hotplug_device_create(data); > + ret = mlxreg_hotplug_device_create(data); > } else { > if (item->inversed) > - mlxreg_hotplug_device_create(data); > + ret = mlxreg_hotplug_device_create(data); > else > mlxreg_hotplug_device_destroy(data); > } > + if (ret) > + dev_err(priv->dev, "Failed to create hotplug device.\n"); So we've moved the error message - but we have affected any functional change - we still don't DO anything (or NOT DO anything) if the create call fails. Is that the right thing? > } > > /* Acknowledge event. */ > @@ -312,8 +314,11 @@ mlxreg_hotplug_health_work_helper(struct mlxreg_hotplug_priv_data *priv, > if (regval == MLXREG_HOTPLUG_HEALTH_MASK) { > if ((data->health_cntr++ == MLXREG_HOTPLUG_RST_CNTR) || > !priv->after_probe) { > - mlxreg_hotplug_device_create(data); > - data->attached = true; > + ret = mlxreg_hotplug_device_create(data); > + if (ret) > + dev_err(priv->dev, "Failed to create hotplug device.\n"); I meant to bring this up - rather than repeat the exact same message as above, should this read: "Failed to create hotplug health device.\n" Or something similar? This would provide the user/developer with more information about what is failing. > + else > + data->attached = true; And this does change behavior, but isn't noted in the changelog. I have pushed my v11 including everything up to and excluding this patch here: https://github.com/dvhart/linux-pdx86/tree/review-dvhart-mellanox-v11 (also on the original repo at infradead which appears to be back up and running, just giving it time before switching back 100%) -- Darren Hart VMware Open Source Technology Center