linux-kernel - Re: [PATCH v2] staging: gpib: simplify and fix get_data

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aLVpfRLyri4K_WFK@egonzo>
Date: Mon, 1 Sep 2025 11:38:05 +0200
From: Dave Penkler <dpenkler@...il.com>
To: Osama Abdelkader <osama.abdelkader@...il.com>
Cc: Dan Carpenter <dan.carpenter@...aro.org>, gregkh@...uxfoundation.org,
	matchstick@...erthere.org, arnd@...db.de,
	linux-kernel@...r.kernel.org, linux-staging@...ts.linux.dev,
	marcello.carla@....com
Subject: Re: [PATCH v2] staging: gpib: simplify and fix get_data_lines

On Fri, Aug 29, 2025 at 04:34:05PM +0200, Osama Abdelkader wrote:
> 
> On 8/27/25 4:11 PM, Dave Penkler wrote:
> > On Wed, Aug 27, 2025 at 03:16:28PM +0300, Dan Carpenter wrote:
> >> On Wed, Aug 27, 2025 at 01:38:57PM +0200, Osama Abdelkader wrote:
> >>> The function `get_data_lines()` in gpib_bitbang.c currently reads 8
> >>> GPIO descriptors individually and combines them into a byte.
> >>> This has two issues:
> >>>
> >>>   * `gpiod_get_value()` returns an `int` which may be negative on
> >>>     error. Assigning it directly into a `u8` may propagate unexpected
> >>>     values. Masking ensures only the LSB is used.
> >> This part isn't really true any more.
> >>
> >>>   * The code is repetitive and harder to extend.
> >>>
> >>> Fix this by introducing a local array of GPIO descriptors and looping
> >>> over them, while masking the return value to its least significant bit.
> >> There really isn't any need to mask now that we're checking for
> >> negatives.
> >>
> >>> This reduces duplication, makes the code more maintainable, and avoids
> >>> possible data corruption from negative `gpiod_get_value()` returns.
> >>>
> >>> Signed-off-by: Osama Abdelkader <osama.abdelkader@...il.com>
> >>> ---
> >>> v2:
> >>> Just print the gpio pin error and leave the bit as zero
> >>> ---
> >>>  drivers/staging/gpib/gpio/gpib_bitbang.c | 28 ++++++++++++++----------
> >>>  1 file changed, 17 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/drivers/staging/gpib/gpio/gpib_bitbang.c b/drivers/staging/gpib/gpio/gpib_bitbang.c
> >>> index 17884810fd69..f4ca59c007dd 100644
> >>> --- a/drivers/staging/gpib/gpio/gpib_bitbang.c
> >>> +++ b/drivers/staging/gpib/gpio/gpib_bitbang.c
> >>> @@ -1403,17 +1403,23 @@ static void set_data_lines(u8 byte)
> >>>  
> >>>  static u8 get_data_lines(void)
> >>>  {
> >>> -	u8 ret;
> >>> -
> >>> -	ret = gpiod_get_value(D01);
> >>> -	ret |= gpiod_get_value(D02) << 1;
> >>> -	ret |= gpiod_get_value(D03) << 2;
> >>> -	ret |= gpiod_get_value(D04) << 3;
> >>> -	ret |= gpiod_get_value(D05) << 4;
> >>> -	ret |= gpiod_get_value(D06) << 5;
> >>> -	ret |= gpiod_get_value(D07) << 6;
> >>> -	ret |= gpiod_get_value(D08) << 7;
> >>> -	return ~ret;
> >>> +	struct gpio_desc *lines[8] = {
> >>> +		D01, D02, D03, D04, D05, D06, D07, D08
> >>> +	};
> >>> +
> >> Delete this blank line.
> >>
> >>> +	u8 val = 0;
> >>> +	int ret, i;
> >>> +
> >>> +	for (i = 0; i < 8; i++) {
> >>> +		ret = gpiod_get_value(lines[i]);
> >>> +		if (ret < 0) {
> >>> +			pr_err("get GPIO pin %d error: %d\n", i, ret);
> >>> +			continue;
> >>> +		}
> >>> +		val |= (ret & 1) << i;
> >> Delete the mask.
> >>
> >> (I wavered on whether I should comment on the nit picky things I've
> >> said in this email, but in the end it was the out of date commit
> >> message which pushed me over the edge.  I would have ignored the
> >> other things otherwise).
> >>
> >> regards,
> >> dan carpenter
> >>
> >>
> > This patch seems unnecessary.
> > The code will never be extended.
> 
> But using for loop is more readable than writing 8 similar lines, or?
> 
> > In the unlikely case of errors it will produce a huge streams of console spam.
> > It negatively impacts performance:  114209 bytes/sec vs 118274 bytes/sec.
> 
> We can remove that error message to not impact the performance, but storing errors even unlikely cases
> as gpio data is a bug, or?

Hi again,
Even with the following code, eliminating the error test, the
performance is still negatively impacted: 114865 vs 118274 bytes/sec.

static u8 get_data_lines(void)
{
  struct gpio_desc *lines[8] = {D01, D02, D03, D04, D05, D06, D07, D08}; 
  u8 val = 0;
  int i;

  for (i = 0; i < 8; i++)
             val |= gpiod_get_value(lines[i]) << i;
   return ~val;
}

Variable shifts are just slower than hardcoded shifts. Most of the
delay for GPIB reads and writes comes from the relatively long
interrupt latency on the pi's (> 2 usecs). There are 2 interrupts per
byte read. Even so the loop implementation causes a noticeable
degradation in performance which we want to avoid.

Regarding testing for error returns:
We won't get ENODEV since on the raspberry pi the gpios are
implemented on the SoC so cannot "disappear" once allocated.

In the case of a direction bug (which we don't have) the gpiod subsystem
will emit a warning.

Further it is not worth checking for error returns on the
gpiod_get/set_value calls with the bcma_gpio_get/set_value
implementations since the latter do not return negative values.