| Message ID | 71dad56e-0e2f-48ba-90bc-75096112a1ba@moroto.mountain (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: ipu-bridge: fix error code in ipu_bridge_init() | expand |
On Fri, May 10, 2024 at 06:10:37PM +0300, Dan Carpenter wrote: > Return -EINVAL if "bridge->n_sensors == 0". Don't return success. ... > ret = ipu_bridge_connect_sensors(bridge); > - if (ret || bridge->n_sensors == 0) > + if (ret || bridge->n_sensors == 0) { > + ret = ret ?: -EINVAL; > goto err_unregister_ipu; > + } I would split: ret = ipu_bridge_connect_sensors(bridge); if (ret) goto err_unregister_ipu; if (bridge->n_sensors == 0) { ret = -EINVAL; goto err_unregister_ipu; }
On Fri, May 10, 2024 at 06:18:22PM +0300, Andy Shevchenko wrote: > On Fri, May 10, 2024 at 06:10:37PM +0300, Dan Carpenter wrote: > > Return -EINVAL if "bridge->n_sensors == 0". Don't return success. > > ... > > > ret = ipu_bridge_connect_sensors(bridge); > > - if (ret || bridge->n_sensors == 0) > > + if (ret || bridge->n_sensors == 0) { > > + ret = ret ?: -EINVAL; > > goto err_unregister_ipu; > > + } > > I would split: > > ret = ipu_bridge_connect_sensors(bridge); > if (ret) > goto err_unregister_ipu; > > if (bridge->n_sensors == 0) { > ret = -EINVAL; > goto err_unregister_ipu; > } It's always hard to know which way to go on these... I wrote it that way in my first draft. It's my prefered way as well but not everyone agrees. I'll resend. regards, dan carpenter
On Fri, May 10, 2024 at 06:27:30PM +0300, Dan Carpenter wrote: > On Fri, May 10, 2024 at 06:18:22PM +0300, Andy Shevchenko wrote: > > On Fri, May 10, 2024 at 06:10:37PM +0300, Dan Carpenter wrote: > > > Return -EINVAL if "bridge->n_sensors == 0". Don't return success. ... > > > ret = ipu_bridge_connect_sensors(bridge); > > > - if (ret || bridge->n_sensors == 0) > > > + if (ret || bridge->n_sensors == 0) { > > > + ret = ret ?: -EINVAL; > > > goto err_unregister_ipu; > > > + } > > > > I would split: > > > > ret = ipu_bridge_connect_sensors(bridge); > > if (ret) > > goto err_unregister_ipu; > > > > if (bridge->n_sensors == 0) { > > ret = -EINVAL; > > goto err_unregister_ipu; > > } > > It's always hard to know which way to go on these... I wrote it that > way in my first draft. It's my prefered way as well but not everyone > agrees. I'll resend. Is the generated assembly the same?
On Fri, May 10, 2024 at 06:36:36PM +0300, Andy Shevchenko wrote: > On Fri, May 10, 2024 at 06:27:30PM +0300, Dan Carpenter wrote: > > On Fri, May 10, 2024 at 06:18:22PM +0300, Andy Shevchenko wrote: > > > On Fri, May 10, 2024 at 06:10:37PM +0300, Dan Carpenter wrote: > > > > Return -EINVAL if "bridge->n_sensors == 0". Don't return success. > > ... > > > > > ret = ipu_bridge_connect_sensors(bridge); > > > > - if (ret || bridge->n_sensors == 0) > > > > + if (ret || bridge->n_sensors == 0) { > > > > + ret = ret ?: -EINVAL; > > > > goto err_unregister_ipu; > > > > + } > > > > > > I would split: > > > > > > ret = ipu_bridge_connect_sensors(bridge); > > > if (ret) > > > goto err_unregister_ipu; > > > > > > if (bridge->n_sensors == 0) { > > > ret = -EINVAL; > > > goto err_unregister_ipu; > > > } > > > > It's always hard to know which way to go on these... I wrote it that > > way in my first draft. It's my prefered way as well but not everyone > > agrees. I'll resend. > > Is the generated assembly the same? Yeah, it does. regards, dan carpenter
diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c index 61750cc98d70..a009ee73e26f 100644 --- a/drivers/media/pci/intel/ipu-bridge.c +++ b/drivers/media/pci/intel/ipu-bridge.c @@ -839,8 +839,10 @@ int ipu_bridge_init(struct device *dev, bridge->data_lanes[i] = i + 1; ret = ipu_bridge_connect_sensors(bridge); - if (ret || bridge->n_sensors == 0) + if (ret || bridge->n_sensors == 0) { + ret = ret ?: -EINVAL; goto err_unregister_ipu; + } dev_info(dev, "Connected %d cameras\n", bridge->n_sensors);
Return -EINVAL if "bridge->n_sensors == 0". Don't return success. Fixes: 881ca25978c6 ("media: ipu3-cio2: rename cio2 bridge to ipu bridge and move out of ipu3") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- drivers/media/pci/intel/ipu-bridge.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)