Re: [PATCH] staging: dgnc: replace DGNC_VERIFY_BOARD macro
On Mon, Oct 17, 2016 at 11:46:36PM +0200, Fernando Apesteguía wrote: > On Mon, Oct 17, 2016 at 10:29 AM, Greg KHwrote: > > 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, _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
Re: [PATCH] staging: dgnc: replace DGNC_VERIFY_BOARD macro
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, _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
Re: [PATCH] staging: dgnc: replace DGNC_VERIFY_BOARD macro
On Mon, Oct 17, 2016 at 10:29 AM, Greg KHwrote: > 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, _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? Thanks. > I don't want to take this patch as it will hide the real issues here. > > thanks, > > greg k-h
Re: [PATCH] staging: dgnc: replace DGNC_VERIFY_BOARD macro
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, _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? Thanks. > I don't want to take this patch as it will hide the real issues here. > > thanks, > > greg k-h
Re: [PATCH] staging: dgnc: replace DGNC_VERIFY_BOARD macro
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, _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 don't want to take this patch as it will hide the real issues here. thanks, greg k-h
Re: [PATCH] staging: dgnc: replace DGNC_VERIFY_BOARD macro
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, _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 don't want to take this patch as it will hide the real issues here. thanks, greg k-h
[PATCH] staging: dgnc: replace DGNC_VERIFY_BOARD macro
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, _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; +} static ssize_t vpd_show(struct device *p, struct device_attribute *attr, char *buf) @@ -109,7 +113,9 @@ static ssize_t vpd_show(struct device *p, struct device_attribute *attr, int count = 0; int i = 0; - DGNC_VERIFY_BOARD(p, bd); + bd = dgnc_get_board(p); + if (!bd) + return 0; count += sprintf(buf + count, "\n 0 1 2 3 4 5 6 7 8 9 A B C D E F"); @@ -130,7 +136,9 @@ static ssize_t serial_number_show(struct device *p, struct dgnc_board *bd; int count = 0; - DGNC_VERIFY_BOARD(p, bd); + bd = dgnc_get_board(p); + if (!bd) + return 0; if (bd->serial_num[0] == '\0') count += sprintf(buf + count, "\n"); @@ -148,7 +156,9 @@ static ssize_t ports_state_show(struct device *p, int count = 0; int i = 0; - DGNC_VERIFY_BOARD(p, bd); + bd = dgnc_get_board(p); + if (!bd) + return 0; for (i = 0; i < bd->nasync; i++) { count += snprintf(buf + count, PAGE_SIZE - count, @@ -166,7 +176,9 @@ static ssize_t ports_baud_show(struct device *p, int count = 0; int i = 0; - DGNC_VERIFY_BOARD(p, bd); + bd = dgnc_get_board(p); + if (!bd) + return 0; for (i = 0; i < bd->nasync; i++) { count += snprintf(buf + count, PAGE_SIZE - count, @@ -184,7 +196,9 @@ static ssize_t ports_msignals_show(struct device *p, int count = 0; int i = 0; - DGNC_VERIFY_BOARD(p, bd); + bd = dgnc_get_board(p); + if (!bd) + return 0; for (i = 0; i < bd->nasync; i++) { struct channel_t *ch = bd->channels[i]; @@ -215,7 +229,9 @@ static ssize_t ports_iflag_show(struct device *p, int count = 0; int i = 0; - DGNC_VERIFY_BOARD(p, bd); + bd = dgnc_get_board(p); + if (!bd) + return 0; for (i = 0; i < bd->nasync; i++) { count += snprintf(buf + count, PAGE_SIZE - count, "%d %x\n", @@ -233,7 +249,9 @@ static ssize_t ports_cflag_show(struct device *p, int count = 0; int i = 0; - DGNC_VERIFY_BOARD(p, bd); + bd = dgnc_get_board(p); + if (!bd) + return 0; for (i = 0; i < bd->nasync; i++) { count += snprintf(buf + count, PAGE_SIZE - count, "%d %x\n", @@ -251,7 +269,9 @@ static ssize_t ports_oflag_show(struct device *p, int count = 0; int i = 0; - DGNC_VERIFY_BOARD(p, bd); + bd = dgnc_get_board(p); + if (!bd) + return 0; for (i = 0; i < bd->nasync; i++) { count += snprintf(buf + count, PAGE_SIZE - count, "%d %x\n", @@ -269,7 +289,9 @@ static ssize_t ports_lflag_show(struct device *p, int count = 0; int i = 0; - DGNC_VERIFY_BOARD(p, bd); + bd = dgnc_get_board(p); + if (!bd) + return 0; for (i = 0; i < bd->nasync; i++) { count += snprintf(buf + count, PAGE_SIZE - count, "%d %x\n", @@ -287,7 +309,9 @@ static ssize_t
[PATCH] staging: dgnc: replace DGNC_VERIFY_BOARD macro
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, _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; +} static ssize_t vpd_show(struct device *p, struct device_attribute *attr, char *buf) @@ -109,7 +113,9 @@ static ssize_t vpd_show(struct device *p, struct device_attribute *attr, int count = 0; int i = 0; - DGNC_VERIFY_BOARD(p, bd); + bd = dgnc_get_board(p); + if (!bd) + return 0; count += sprintf(buf + count, "\n 0 1 2 3 4 5 6 7 8 9 A B C D E F"); @@ -130,7 +136,9 @@ static ssize_t serial_number_show(struct device *p, struct dgnc_board *bd; int count = 0; - DGNC_VERIFY_BOARD(p, bd); + bd = dgnc_get_board(p); + if (!bd) + return 0; if (bd->serial_num[0] == '\0') count += sprintf(buf + count, "\n"); @@ -148,7 +156,9 @@ static ssize_t ports_state_show(struct device *p, int count = 0; int i = 0; - DGNC_VERIFY_BOARD(p, bd); + bd = dgnc_get_board(p); + if (!bd) + return 0; for (i = 0; i < bd->nasync; i++) { count += snprintf(buf + count, PAGE_SIZE - count, @@ -166,7 +176,9 @@ static ssize_t ports_baud_show(struct device *p, int count = 0; int i = 0; - DGNC_VERIFY_BOARD(p, bd); + bd = dgnc_get_board(p); + if (!bd) + return 0; for (i = 0; i < bd->nasync; i++) { count += snprintf(buf + count, PAGE_SIZE - count, @@ -184,7 +196,9 @@ static ssize_t ports_msignals_show(struct device *p, int count = 0; int i = 0; - DGNC_VERIFY_BOARD(p, bd); + bd = dgnc_get_board(p); + if (!bd) + return 0; for (i = 0; i < bd->nasync; i++) { struct channel_t *ch = bd->channels[i]; @@ -215,7 +229,9 @@ static ssize_t ports_iflag_show(struct device *p, int count = 0; int i = 0; - DGNC_VERIFY_BOARD(p, bd); + bd = dgnc_get_board(p); + if (!bd) + return 0; for (i = 0; i < bd->nasync; i++) { count += snprintf(buf + count, PAGE_SIZE - count, "%d %x\n", @@ -233,7 +249,9 @@ static ssize_t ports_cflag_show(struct device *p, int count = 0; int i = 0; - DGNC_VERIFY_BOARD(p, bd); + bd = dgnc_get_board(p); + if (!bd) + return 0; for (i = 0; i < bd->nasync; i++) { count += snprintf(buf + count, PAGE_SIZE - count, "%d %x\n", @@ -251,7 +269,9 @@ static ssize_t ports_oflag_show(struct device *p, int count = 0; int i = 0; - DGNC_VERIFY_BOARD(p, bd); + bd = dgnc_get_board(p); + if (!bd) + return 0; for (i = 0; i < bd->nasync; i++) { count += snprintf(buf + count, PAGE_SIZE - count, "%d %x\n", @@ -269,7 +289,9 @@ static ssize_t ports_lflag_show(struct device *p, int count = 0; int i = 0; - DGNC_VERIFY_BOARD(p, bd); + bd = dgnc_get_board(p); + if (!bd) + return 0; for (i = 0; i < bd->nasync; i++) { count += snprintf(buf + count, PAGE_SIZE - count, "%d %x\n", @@ -287,7 +309,9 @@ static ssize_t ports_digi_flag_show(struct device *p,