diff mbox series

[RFC] fpga: bridge: return errors in the show() method of the "state" attribute

Message ID 20230124113050.117645-1-marpagan@redhat.com
State New
Headers
Series [RFC] fpga: bridge: return errors in the show() method of the "state" attribute |

Commit Message

Marco Pagani Jan. 24, 2023, 11:30 a.m. UTC 
  This patch changes the show() method of the "state" sysfs attribute to
return an error if the bridge's enable_show() op returns an error code.
In this way, the userspace can distinguish between when the bridge is
actually "enabled" (i.e., allowing signals to pass) or "disabled"
(i.e., gating signals), or when there is an error (e.g., the state of
the bridge cannot be determined).

Currently, enable_show() returns an integer representing the bridge's
state (enabled or disabled) or an error code. However, this integer
value is interpreted in state_show() as a bool, resulting in the method
printing "enabled" (i.e., the bridge allows signals to pass), without
propagating the error, even when enable_show() returns an error code.

Another possibility could be to change the signature of enable_show()
to return a bool for symmetry with the enable_set() "setter" method.
However, this would prevent enable_show() from returning error codes
and may break the HPS bridge driver (altera-hps2fpga.c +53).

Thanks

Signed-off-by: Marco Pagani <marpagan@redhat.com>
---
 drivers/fpga/fpga-bridge.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Xu Yilun Jan. 24, 2023, 3:08 p.m. UTC | #1 
On 2023-01-24 at 12:30:50 +0100, Marco Pagani wrote:
> This patch changes the show() method of the "state" sysfs attribute to
> return an error if the bridge's enable_show() op returns an error code.
> In this way, the userspace can distinguish between when the bridge is
> actually "enabled" (i.e., allowing signals to pass) or "disabled"
> (i.e., gating signals), or when there is an error (e.g., the state of
> the bridge cannot be determined).

Maybe we remove the example for "error" here. This patch is just to
propagate the errors from low level drivers.

> 
> Currently, enable_show() returns an integer representing the bridge's
> state (enabled or disabled) or an error code. However, this integer
> value is interpreted in state_show() as a bool, resulting in the method
> printing "enabled" (i.e., the bridge allows signals to pass), without
> propagating the error, even when enable_show() returns an error code.
> 
> Another possibility could be to change the signature of enable_show()
> to return a bool for symmetry with the enable_set() "setter" method.
> However, this would prevent enable_show() from returning error codes

Returning error codes is good to me.

> and may break the HPS bridge driver (altera-hps2fpga.c +53).
> 
> Thanks
> 
> Signed-off-by: Marco Pagani <marpagan@redhat.com>
> ---
>  drivers/fpga/fpga-bridge.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c
> index 727704431f61..5cd40acab5bf 100644
> --- a/drivers/fpga/fpga-bridge.c
> +++ b/drivers/fpga/fpga-bridge.c
> @@ -293,12 +293,15 @@ static ssize_t state_show(struct device *dev,
>  			  struct device_attribute *attr, char *buf)
>  {
>  	struct fpga_bridge *bridge = to_fpga_bridge(dev);
> -	int enable = 1;
> +	int state = 1;
>  
> -	if (bridge->br_ops && bridge->br_ops->enable_show)
> -		enable = bridge->br_ops->enable_show(bridge);
> +	if (bridge->br_ops && bridge->br_ops->enable_show) {
> +		state = bridge->br_ops->enable_show(bridge);
> +		if (state < 0)
> +			return state;
> +	}
>  
> -	return sprintf(buf, "%s\n", enable ? "enabled" : "disabled");
> +	return sysfs_emit(buf, "%s\n", state ? "enabled" : "disabled");

The code looks good to me. You may remove RFC prefix in next version.

Thanks,
Yilun

>  }
>  
>  static DEVICE_ATTR_RO(name);
> -- 
> 2.39.1
>
diff mbox series

Patch

diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c
index 727704431f61..5cd40acab5bf 100644
--- a/drivers/fpga/fpga-bridge.c
+++ b/drivers/fpga/fpga-bridge.c
@@ -293,12 +293,15 @@  static ssize_t state_show(struct device *dev,
 			  struct device_attribute *attr, char *buf)
 {
 	struct fpga_bridge *bridge = to_fpga_bridge(dev);
-	int enable = 1;
+	int state = 1;
 
-	if (bridge->br_ops && bridge->br_ops->enable_show)
-		enable = bridge->br_ops->enable_show(bridge);
+	if (bridge->br_ops && bridge->br_ops->enable_show) {
+		state = bridge->br_ops->enable_show(bridge);
+		if (state < 0)
+			return state;
+	}
 
-	return sprintf(buf, "%s\n", enable ? "enabled" : "disabled");
+	return sysfs_emit(buf, "%s\n", state ? "enabled" : "disabled");
 }
 
 static DEVICE_ATTR_RO(name);