[PATCH] staging: gpib: simplify and fix get_data_lines

[PATCH] staging: gpib: simplify and fix get_data_lines

[Osama Abdelkader]

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.
  * 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.

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@gmail.com>
---
 drivers/staging/gpib/gpio/gpib_bitbang.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/gpib/gpio/gpib_bitbang.c b/drivers/staging/gpib/gpio/gpib_bitbang.c
index 17884810fd69..b6d1e650687f 100644
--- a/drivers/staging/gpib/gpio/gpib_bitbang.c
+++ b/drivers/staging/gpib/gpio/gpib_bitbang.c
@@ -1403,16 +1403,15 @@ 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;
+	struct gpio_desc *lines[8] = {
+		D01, D02, D03, D04, D05, D06, D07, D08
+	};
+	u8 ret = 0;
+	int i;
+
+	for (i = 0; i < 8; i++)
+		ret |= (gpiod_get_value(lines[i]) & 1) << i;
+
 	return ~ret;
 }
 
-- 
2.43.0

Re: [PATCH] staging: gpib: simplify and fix get_data_lines

[Dan Carpenter]

On Wed, Aug 27, 2025 at 12:05:02AM +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.

Using the last bit in an error code is not really "error handling"...

What you could do instead would be something like:

	int ret;

	for (i = 0; i < 8; i++) {
		ret |= (gpiod_get_value(lines[i]) & 1) << i;
		if (ret < 0) {
			pr_err("something failed\n");
			return -EINVAL;
		}
	}

	return ~ret;

Which might also not be correct, but it's probably closer to being okay.

regards,
dan carpenter

Re: [PATCH] staging: gpib: simplify and fix get_data_lines

[Dan Carpenter]

On Wed, Aug 27, 2025 at 10:15:33AM +0300, Dan Carpenter wrote:
> On Wed, Aug 27, 2025 at 12:05:02AM +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.
> 
> Using the last bit in an error code is not really "error handling"...
> 
> What you could do instead would be something like:
> 
> 	int ret;
> 
> 	for (i = 0; i < 8; i++) {
> 		ret |= (gpiod_get_value(lines[i]) & 1) << i;
> 		if (ret < 0) {
> 			pr_err("something failed\n");
> 			return -EINVAL;

I meant to write "return 0;".  It's type u8.

Also that doesn't work.  The masks and shift mess it up.

	u8 val = 0;
	int ret;

	for (i = 0; i < 8; i++) {
		ret = gpiod_get_value(lines[i]);
		if (ret < 0) {
			pr_err("something failed\n");
			continue;
		}
		val |= ret << i;
	}

	return ~val;

regards,
dan carpenter

Re: [PATCH] staging: gpib: simplify and fix get_data_lines

[Osama Abdelkader]

On 8/27/25 10:07 AM, Dan Carpenter wrote:
> On Wed, Aug 27, 2025 at 10:15:33AM +0300, Dan Carpenter wrote:
>> On Wed, Aug 27, 2025 at 12:05:02AM +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.
>> Using the last bit in an error code is not really "error handling"...
>>
>> What you could do instead would be something like:
>>
>> 	int ret;
>>
>> 	for (i = 0; i < 8; i++) {
>> 		ret |= (gpiod_get_value(lines[i]) & 1) << i;
>> 		if (ret < 0) {
>> 			pr_err("something failed\n");
>> 			return -EINVAL;
> I meant to write "return 0;".  It's type u8.
>
> Also that doesn't work.  The masks and shift mess it up.
>
> 	u8 val = 0;
> 	int ret;
>
> 	for (i = 0; i < 8; i++) {
> 		ret = gpiod_get_value(lines[i]);
> 		if (ret < 0) {
> 			pr_err("something failed\n");
> 			continue;
> 		}
> 		val |= ret << i;
> 	}
>
> 	return ~val;

We can change the return type to int and propagate the error, so:

static int get_data_lines(u8 *out)

{

	int val, i;

	u8 ret = 0;

	struct gpio_desc *lines[8] = { D01, D02, D03, D04, D05, D06, D07, D08 };
	for (i = 0; i < 8; i++) { 		val = gpiod_get_value(lines[i]);

		if (val < 0)

			return val; // propagate error

		ret |= (val & 1) << i;

	}

	*out = ~ret; 	return 0;

}

Then in the caller:

u8 data;
if (!get_data_lines(&data))

	priv->rbuf[priv->count++] = data;

or we print the error here, What do you think?

>
> regards,
> dan carpenter
>

Re: [PATCH] staging: gpib: simplify and fix get_data_lines

[Dan Carpenter]

On Wed, Aug 27, 2025 at 11:28:36AM +0200, Osama Abdelkader wrote:
> We can change the return type to int and propagate the error, so:
> 
> static int get_data_lines(u8 *out)
> 
> {
> 
> 	int val, i;
> 
> 	u8 ret = 0;
> 
> 	struct gpio_desc *lines[8] = { D01, D02, D03, D04, D05, D06, D07, D08 };
> 	for (i = 0; i < 8; i++) { 		val = gpiod_get_value(lines[i]);
> 
> 		if (val < 0)
> 
> 			return val; // propagate error
> 
> 		ret |= (val & 1) << i;
> 
> 	}
> 
> 	*out = ~ret; 	return 0;
> 
> }
> 
> Then in the caller:
> 
> u8 data;
> if (!get_data_lines(&data))
> 
> 	priv->rbuf[priv->count++] = data;
> 
> or we print the error here, What do you think?
> 

Printing the error closer to where the error occured is better.

How would we handle the error correctly?  Sometimes it's easy
because it's if we continue then we will crash and almost anything is
better than crashing.  But here if we return a 1 or 0, what's the
worst that can happen?  We can't know without testing.  Adding new
error checking often breaks stuff.  I've done it before where you
have code like:

	ret = frob();
	if (ret)
		return ret;

	frob();  // <-- here
	if (ret)
		return ret;

And obviously the original author left off the "ret = " part on the
second call.  So I don't feel bad about that I added that, but in
practice the second frob() always fails and I can't test it so now
people's driver doesn't load.

So adding error handling is a bit risky unless you have a way to test
this code.  Just printing an error and continuing as best we can is
safer.  People will let us know if the error ever happens.

Let's not over complicate it for an error which will likely never
happen.  We can just print an error and leave the bit as zero.

regards,
dan carpenter