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.
