| Message ID | 20230124113050.117645-1-marpagan@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] fpga: bridge: return errors in the show() method of the "state" attribute | expand |
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 --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);
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(-)