Date:   Fri, 18 Sep 2020 23:50:51 +0100
From:   Dan Scally <djrscally@...il.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     yong.zhi@...el.com, sakari.ailus@...ux.intel.com,
        bingbu.cao@...el.com, tian.shu.qiu@...el.com, mchehab@...nel.org,
        gregkh@...uxfoundation.org, davem@...emloft.net, robh@...nel.org,
        devel@...verdev.osuosl.org, jorhand@...ux.microsoft.com,
        kieran.bingham@...asonboard.com, linux-kernel@...r.kernel.org,
        linux-media@...r.kernel.org, kitakar@...il.com
Subject: Re: [RFC PATCH] Add bridge driver to connect sensors to CIO2 device
 via software nodes on ACPI platforms

Hi Dan -  thanks for all your comments. Sorry it took a while to get to
yours.

On 17/09/2020 10:34, Dan Carpenter wrote:
> On Wed, Sep 16, 2020 at 10:36:18PM +0100, Daniel Scally wrote:
>> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
>> index 92f5eadf2c99..fd941d2c7581 100644
>> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
>> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
>> @@ -1719,6 +1719,59 @@ static void cio2_queues_exit(struct cio2_device *cio2)
>>  		cio2_queue_exit(cio2, &cio2->queue[i]);
>>  }
>>  
>> +static int cio2_probe_can_progress(struct pci_dev *pci_dev)
>> +{
>> +	void *sensor;
>> +
>> +	/*
>> +	 * On ACPI platforms, we need to probe _after_ sensors wishing to connect
>> +	 * to cio2 have added a device link. If there are no consumers yet, then
>> +	 * we need to defer. The .sync_state() callback will then be called after
>> +	 * all linked sensors have probed
>> +	 */
>> +
>> +	if (IS_ENABLED(CONFIG_ACPI)) {
> Reverse this condition.
>
> 	if (!IS_ENABLED(CONFIG_ACPI))
> 		return 0;
>
>
Yeah, much better.
>> +		sensor = (struct device *)list_first_entry_or_null(
>> +								&pci_dev->dev.links.consumers,
>> +								struct dev_links_info,
>> +								consumers);
>> +
>> +		if (!sensor)
>> +			return -EPROBE_DEFER;
> Get rid of the cast.
>
> 	if (list_empty(&pci_dev->dev.links.consumers))
> 		return -EPROBE_DEFER;
>
> 	return 0;
>
Also much better, though I think possibly this whole section will be
going away now after some of the other pointers...
>> +		cio2 = dev_get_drvdata(dev);
>> +
>> +		if (!cio2) {
> Delete the blank line between the call and the test.  They're part of
> the same step.  "cio2" can't be NULL anyway, so delete the test.
Thanks - I'll skip blank lines in that situation in future
>> +
>> +	if (ret < 0) {
>> +		dev_err(dev, "Failed to fetch SSDB data\n");
>> +		return ret;
>> +	}
>> +
>> +	sensor->link = sensor_data.link;
>> +	sensor->lanes = sensor_data.lanes;
>> +	sensor->mclkspeed = sensor_data.mclkspeed;
>> +
>> +	return 0;
>> +}
>> +
>> +static int create_endpoint_properties(struct device *dev,
>> +				      struct sensor_bios_data *ssdb,
>> +				      struct property_entry *sensor_props,
>> +				      struct property_entry *cio2_props)
>> +{
>> +		u32 *data_lanes;
>> +		int i;
> Indented too far.
>
>> +
>> +		data_lanes = devm_kmalloc(dev, sizeof(u32) * (int)ssdb->lanes,
> No need for the cast.  Use devm_kmalloc_array().
Ah - TIL that that exists, thanks.
>> +					  GFP_KERNEL);
>> +
>> +		if (!data_lanes) {
>> +			dev_err(dev,
>> +				"Couldn't allocate memory for data lanes array\n");
> Delete the error message (checkpatch.pl --strict).
And that too - I wasn't using the --strict flag, I'll do that next time
>> +
>> +		sensor_props[0] = PROPERTY_ENTRY_U32("clock-frequency",
>> +						     ssdb->mclkspeed);
>> +		sensor_props[1] = PROPERTY_ENTRY_U32("bus-type", 5);
>> +		sensor_props[2] = PROPERTY_ENTRY_U32("clock-lanes", 0);
>> +		sensor_props[3] = PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes",
>> +							       data_lanes,
>> +							       (int)ssdb->lanes);
>> +		sensor_props[4] = remote_endpoints[(bridge.n_sensors * 2) + ENDPOINT_SENSOR];
>> +		sensor_props[5] = PROPERTY_ENTRY_NULL;
>> +
>> +		cio2_props[0] = PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes",
>> +							     data_lanes,
>> +							     (int)ssdb->lanes);
>> +		cio2_props[1] = remote_endpoints[(bridge.n_sensors * 2) + ENDPOINT_CIO2];
>> +		cio2_props[2] = PROPERTY_ENTRY_NULL;
>> +
>> +		return 0;
>> +}
>> +
>> +static int connect_supported_devices(void)
>> +{
>> +	struct acpi_device *adev;
>> +	struct device *dev;
>> +	struct sensor_bios_data ssdb;
>> +	struct sensor *sensor;
>> +	struct property_entry *sensor_props;
>> +	struct property_entry *cio2_props;
>> +	struct fwnode_handle *fwnode;
>> +	struct software_node *nodes;
>> +	struct v4l2_subdev *sd;
>> +	int i, ret;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(supported_devices); i++) {
>> +		adev = acpi_dev_get_first_match_dev(supported_devices[i],
>> +						    NULL, -1);
>> +
>> +		if (!adev)
>> +			continue;
>> +
>> +		dev = bus_find_device_by_acpi_dev(&i2c_bus_type, adev);
>> +
>> +		if (!dev) {
>> +			pr_info("ACPI match for %s, but it has no i2c device\n",
>> +				supported_devices[i]);
>> +			continue;
>> +		}
>> +
>> +		if (!dev->driver_data) {
>> +			pr_info("ACPI match for %s, but it has no driver\n",
>> +				supported_devices[i]);
> put_device(dev);
Good catch, thank you.
>> +	}
>> +
>> +	ret = connect_supported_devices();
>> +
>> +	if ((ret < 0) || (bridge.n_sensors <= 0)) {
>> +		pr_err("cio2_bridge: Failed to connect any devices\n");
>> +		goto out;
> If (bridge.n_sensors <= 0) is true then we need to set ret = -EINVAL
> or something.  Really .n_sensors can't be negative.
>
> The name "out" is a crappy label name because it doesn't say what the
> goto does.  When I scroll down then it turns out that "goto out;" calls
> a free_everything() function.  That kind of error handling is always
> buggy.
>
> There are several typical bugs.  1) Something leaks because the error
> handling style is too complicated to be audited.  2)  Dereferencing
> uninitialized pointers.  3)  Undoing something which hasn't been done.
>
> I believe that in this case one bug is with the handling of the
> bridge.cio2_fwnode.  We "restore" it back to the original state
> as soon as we have a non-NULL bridge.cio2 instead of waiting until we
> have stored the original state.
>
> The best way to do error handling is this.
>
> Every function cleans up after itself.  The connect_supported_devices()
> function is a bit special because it's a loop.  I would would write it
> so that if it fails then it cleans up the partial loop iteration and
> then at the end it cleans up all the failed loop iterations.
>
> 	for (i = 0; i < ARRAY_SIZE(supported_devices); i++) {
> 		a = frob();
> 		if (!a)
> 			goto unwind;
> 		b = frob();
> 		if (!b) {
> 			free(a);
> 			goto unwind;
> 		}
> 		...
> 	}
>
> 	return 0;
>
> unwind:
> 	for (i = 0; i < bridge.n_sensors; i++) {
> 		free(b);
> 		free(a);
> 	}
> 	bridge.n_sensors = 0;
>
> 	return ret;
>
> The problem with cio2_bridge_unregister_sensors() is that it doesn't
> clean up partial iterations through the loop.  (Missing calls to
> put_device(dev)).
>
> Loops are complicated but the rest is simple.  1) Every allocation
> function needs a matching cleanup function.  2) Use good label names
> which say what the goto does.  3)  The goto should free the most recent
> successful allocation.
>
> 	a = frob();
> 	if (!a)
> 		return -ENOMEM;
>
> 	b = frob();
> 	if (!b) {
> 		ret = -ENOMEM;
> 		goto free_a;
> 	}
>
> 	c = frob();
> 	if (!c) {
> 		ret = -ENOMEM;
> 		goto free_b;
> 	}
>
> 	return 0;
>
> free_b:
> 	free(b);
> free_a:
> 	free(a);
>
> 	return ret;
>
> The free function doesn't have any if statements.
>
> void free_function()
> {
> 	free(c);
> 	free(b);
> 	free(a);
> }
>
> The reviewer only needs to keep track of the most recent allocation
> and verify that the goto free_foo matches what should be freed.  This
> system means the code is auditable (no leaks), you never free anything
> which wasn't allocated.
>
This  section and the other comments on error handling was really
helpful - I appreciate you taking the time to explain so thoroughly.

Powered by blists - more mailing lists