From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756265AbcJRGkV (ORCPT ); Tue, 18 Oct 2016 02:40:21 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:35805 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753482AbcJRGkM (ORCPT ); Tue, 18 Oct 2016 02:40:12 -0400 Date: Tue, 18 Oct 2016 08:40:18 +0200 From: Greg KH To: Fernando =?iso-8859-1?Q?Apestegu=EDa?= Cc: devel@driverdev.osuosl.org, Lidza Louina , driverdev-devel@linuxdriverproject.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] staging: dgnc: replace DGNC_VERIFY_BOARD macro Message-ID: <20161018064018.GB7796@kroah.com> References: <1476465777-720-1-git-send-email-fernando.apesteguia@gmail.com> <20161017082931.GA17106@kroah.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.7.1 (2016-10-04) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Oct 17, 2016 at 11:46:36PM +0200, Fernando Apesteguía wrote: > On Mon, Oct 17, 2016 at 10:29 AM, Greg KH wrote: > > On Fri, Oct 14, 2016 at 07:22:57PM +0200, Fernando Apesteguia wrote: > >> The patch replaces the macro with a function (dgnc_get_board) and > >> substitutes the macro statement with a call to that function and a > >> comparison on the returned value. > >> > >> This removes a checkpatch warning. > >> > >> Signed-off-by: Fernando Apesteguia > >> --- > >> drivers/staging/dgnc/dgnc_sysfs.c | 74 +++++++++++++++++++++++++++------------ > >> 1 file changed, 51 insertions(+), 23 deletions(-) > >> > >> diff --git a/drivers/staging/dgnc/dgnc_sysfs.c b/drivers/staging/dgnc/dgnc_sysfs.c > >> index 290bf6e..3ea23a9 100644 > >> --- a/drivers/staging/dgnc/dgnc_sysfs.c > >> +++ b/drivers/staging/dgnc/dgnc_sysfs.c > >> @@ -90,17 +90,21 @@ void dgnc_remove_driver_sysfiles(struct pci_driver *dgnc_driver) > >> driver_remove_file(driverfs, &driver_attr_pollrate); > >> } > >> > >> -#define DGNC_VERIFY_BOARD(p, bd) \ > >> - do { \ > >> - if (!p) \ > >> - return 0; \ > >> - \ > >> - bd = dev_get_drvdata(p); \ > >> - if (!bd || bd->magic != DGNC_BOARD_MAGIC) \ > >> - return 0; \ > >> - if (bd->state != BOARD_READY) \ > >> - return 0; \ > >> - } while (0) > >> +static struct dgnc_board *dgnc_get_board(struct device *p) > >> +{ > >> + struct dgnc_board *bd; > >> + > >> + if (!p) > >> + return NULL; > >> + > >> + bd = dev_get_drvdata(p); > >> + if (!bd || bd->magic != DGNC_BOARD_MAGIC) > >> + return NULL; > >> + if (bd->state != BOARD_READY) > >> + return NULL; > >> + > >> + return bd; > >> +} > > > > No, this macro should be removed entirely as what it does is pointless > > in some parts, wrong in others, and not needed at all in the rest :( > > > > I've asked others to fix this up properly in the past, but it doesn't > > seem like anyone wants to do the work... > > > > I tried to find the discussion the relevant mails in lkml.org but > couldn't find them. Could you point me to them so I can have a look? Last time I mentioned this, it was on the outreachy-kernel mailing list. Just walk through the code for yourself and see which, if any, of these things could ever actually cause the function to "fail". I think you will find that none of them can ever happen... The first test is a huge proof that the original author didn't understand how sysfs or "container_of()" works, given that it is impossible to ever have happen. thanks, greg k-h From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Tue, 18 Oct 2016 08:40:18 +0200 From: Greg KH Subject: Re: [PATCH] staging: dgnc: replace DGNC_VERIFY_BOARD macro Message-ID: <20161018064018.GB7796@kroah.com> References: <1476465777-720-1-git-send-email-fernando.apesteguia@gmail.com> <20161017082931.GA17106@kroah.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: List-Id: Linux Driver Project Developer List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Errors-To: driverdev-devel-bounces@linuxdriverproject.org Sender: "devel" To: Fernando =?iso-8859-1?Q?Apestegu=EDa?= Cc: devel@driverdev.osuosl.org, Lidza Louina , driverdev-devel@linuxdriverproject.org, linux-kernel@vger.kernel.org On Mon, Oct 17, 2016 at 11:46:36PM +0200, Fernando Apestegu=EDa wrote: > On Mon, Oct 17, 2016 at 10:29 AM, Greg KH wr= ote: > > On Fri, Oct 14, 2016 at 07:22:57PM +0200, Fernando Apesteguia wrote: > >> The patch replaces the macro with a function (dgnc_get_board) and > >> substitutes the macro statement with a call to that function and a > >> comparison on the returned value. > >> > >> This removes a checkpatch warning. > >> > >> Signed-off-by: Fernando Apesteguia > >> --- > >> drivers/staging/dgnc/dgnc_sysfs.c | 74 +++++++++++++++++++++++++++---= --------- > >> 1 file changed, 51 insertions(+), 23 deletions(-) > >> > >> diff --git a/drivers/staging/dgnc/dgnc_sysfs.c b/drivers/staging/dgnc/= dgnc_sysfs.c > >> index 290bf6e..3ea23a9 100644 > >> --- a/drivers/staging/dgnc/dgnc_sysfs.c > >> +++ b/drivers/staging/dgnc/dgnc_sysfs.c > >> @@ -90,17 +90,21 @@ void dgnc_remove_driver_sysfiles(struct pci_driver= *dgnc_driver) > >> driver_remove_file(driverfs, &driver_attr_pollrate); > >> } > >> > >> -#define DGNC_VERIFY_BOARD(p, bd) \ > >> - do { \ > >> - if (!p) \ > >> - return 0; \ > >> - \ > >> - bd =3D dev_get_drvdata(p); \ > >> - if (!bd || bd->magic !=3D DGNC_BOARD_MAGIC) \ > >> - return 0; \ > >> - if (bd->state !=3D BOARD_READY) \ > >> - return 0; \ > >> - } while (0) > >> +static struct dgnc_board *dgnc_get_board(struct device *p) > >> +{ > >> + struct dgnc_board *bd; > >> + > >> + if (!p) > >> + return NULL; > >> + > >> + bd =3D dev_get_drvdata(p); > >> + if (!bd || bd->magic !=3D DGNC_BOARD_MAGIC) > >> + return NULL; > >> + if (bd->state !=3D BOARD_READY) > >> + return NULL; > >> + > >> + return bd; > >> +} > > > > No, this macro should be removed entirely as what it does is pointless > > in some parts, wrong in others, and not needed at all in the rest :( > > > > I've asked others to fix this up properly in the past, but it doesn't > > seem like anyone wants to do the work... > > > = > I tried to find the discussion the relevant mails in lkml.org but > couldn't find them. Could you point me to them so I can have a look? Last time I mentioned this, it was on the outreachy-kernel mailing list. Just walk through the code for yourself and see which, if any, of these things could ever actually cause the function to "fail". I think you will find that none of them can ever happen... The first test is a huge proof that the original author didn't understand how sysfs or "container_of()" works, given that it is impossible to ever have happen. thanks, greg k-h _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel