linux-kernel - Re: [PATCH 26/38] iio: pressure-core: st: Clean-up probe() function

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130911071950.GE11227@lee--X1>
Date:	Wed, 11 Sep 2013 08:19:50 +0100
From:	Lee Jones <lee.jones@...aro.org>
To:	Jonathan Cameron <jic23@...nel.org>
Cc:	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	jic23@....ac.uk, arnd@...db.de, linus.walleij@...aro.org,
	denis.ciocca@...com, linux-iio@...r.kernel.org
Subject: Re: [PATCH 26/38] iio: pressure-core: st: Clean-up probe() function

> > 	err = st_sensors_init_sensor(indio_dev, plat_data);
> > 	if (err < 0)
> >-		goto st_press_common_probe_error;
> >+		return err;
> > 
> >-	if (pdata->get_irq_data_ready(indio_dev) > 0) {
> >+	if (irq > 0) {
> > 		err = st_press_allocate_ring(indio_dev);
> > 		if (err < 0)
> >-			goto st_press_common_probe_error;
> >+			return err;
> > 
> > 		err = st_sensors_allocate_trigger(indio_dev,
> >-							ST_PRESS_TRIGGER_OPS);
> >+						  ST_PRESS_TRIGGER_OPS);
> > 		if (err < 0)
> > 			goto st_press_probe_trigger_error;
> > 	}
> > 
> > 	err = iio_device_register(indio_dev);
> >-	if (err)
> 
> This bit of handling is confusing. I would much rather see the if IRQ at the goto. Here the first thought is why is it not an error if there is no IRQ! 

I certainly see your point. But surely anyone would see after a second
or two that we're returning err and not 0 in this case, so the error
would still be returned, we're not ignoring it. Adding this extra
comparison saves several lines of code.

If you think it's 'too' confusing, I'll revert the change.

Or perhaps a comment:

/* Only deallocate_[trigger|ring] if they were allocated. */
or
/* Only deallocate_[trigger|ring] if we have an IRQ line. */
or
/* If no IRQ was specified, just return the error. */

> >+	if (err && irq > 0)
> > 		goto st_press_device_register_error;
> > 
> > 	return err;
> > 
> > st_press_device_register_error:
> >-	if (pdata->get_irq_data_ready(indio_dev) > 0)
> >-		st_sensors_deallocate_trigger(indio_dev);
> >+	st_sensors_deallocate_trigger(indio_dev);
> > st_press_probe_trigger_error:
> >-	if (pdata->get_irq_data_ready(indio_dev) > 0)
> >-		st_press_deallocate_ring(indio_dev);
> >-st_press_common_probe_error:
> >+	st_press_deallocate_ring(indio_dev);
> >+
> > 	return err;
> > }
> > EXPORT_SYMBOL(st_press_common_probe);
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/