Re: [PATCH v2 2/8] ACPI: property: Parse _CRS CSI-2 descriptor

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Andy,

On Mon, Jan 23, 2023 at 05:07:09PM +0200, Andy Shevchenko wrote:
> On Mon, Jan 23, 2023 at 03:46:11PM +0200, Sakari Ailus wrote:
> > Parse newly added ACPI _CRS CSI-2 descriptor for CSI-2 and camera
> > configuration. For now, only figure out where the descriptor is present in
> > order to allow adding information from it to related devices.
> 
> ...
> 
> > +	memcpy(inst->remote_name, csi2->resource_source.string_ptr,
> > +	       csi2->resource_source.string_length);
> 
> Why don't we use strscpy()? Is it really strings? Or is it some abuse of
> the ACPI object type?

I didn't find a guarantee it would be nil terminated. Albeit I'm fine
switching to strscpy() if there's such a guarantee.

> 
> ...
> 
> > +static acpi_status scan_check_crs_csi2(acpi_handle handle, u32 nesting_level,
> > +				       void *context, void **ret)
> > +{
> > +	struct scan_check_crs_csi2_context inst_context = {
> > +		.handle = handle,
> > +		.res_list = LIST_HEAD_INIT(inst_context.res_list),
> > +	};
> > +	struct list_head *list = context;
> > +	struct crs_csi2 *csi2;
> 
> > +	INIT_LIST_HEAD(&inst_context.res_list);
> 
> Why do you need this? I don't see that variable is static...

Ah. It's not static. But this is a leftover from development time and can
be removed, it's initialised in variable declaration.

> 
> > +	acpi_walk_resources(handle, METHOD_NAME__CRS,
> > +			    scan_check_crs_csi2_instance, &inst_context);
> > +
> > +	if (list_empty(&inst_context.res_list))
> > +		return AE_OK;
> > +
> > +	csi2 = kmalloc(sizeof(*csi2), GFP_KERNEL);
> > +	if (!csi2)
> > +		return AE_OK;
> > +
> > +	csi2->handle = handle;
> > +	list_replace(&inst_context.res_list, &csi2->buses);
> > +	list_add(&csi2->list, list);
> 
> Hmm... Can list_swap() be used here?

We're replacing an entry in a list and then adding an entry to another. How
would you use list_swap() here?

> 
> > +	return AE_OK;
> > +}
> 
> ...
> 
> > +	/*
> > +	 * Figure out how much temporary storage we need for counting
> > +	 * connections in each device.
> > +	 */
> > +	list_for_each_entry(csi2, &crs_csi2_handles, list) {
> > +		struct crs_csi2_instance *inst;
> > +
> > +		handle_count++;
> 
> > +		list_for_each_entry(inst, &csi2->buses, list)
> > +			handle_count++;
> 
> list_count_nodes()?

Are you suggesting adding a new list API function or using one that's not
in the linux-acpi/testing branch yet?

> 
> > +	}
> 
> ...
> 
> > +	sort(handle_refs, handle_count, sizeof(*handle_refs), crs_handle_cmp,
> > +	     NULL);
> 
> Yes, I would leave it on one line.

Works for me.

> 
> ...
> 
> > +		if (check_mul_overflow(sizeof(*ads->ports) +
> > +				       sizeof(*ads->nodes) * 2 +
> > +				       sizeof(*ads->nodeptrs) * 2,
> > +				       (size_t)this_count, &alloc_size) ||
> 
> Can this_count be of size_t type from the beginning?

I think so.

> 
> > +		    check_add_overflow(sizeof(*ads) + sizeof(*ads->nodes) +
> > +				       sizeof(*ads->nodeptrs) * 2,
> > +				       alloc_size, &alloc_size)) {
> > +			acpi_handle_warn(handle, "too many handles (%u)",
> > +					 this_count);
> > +			continue;
> > +		}
> 
> ...
> 
> > +		ads->nodeptrs = (void *)(ads->nodes +
> > +					 this_count * 2 + 1);
> 
> Why this is not on one line? (I have got less than 80).

Probably there was more on that line but I forgot to unwrap when removing
whatever was there. I'll address this for v3.

-- 
Kind regards,

Sakari Ailus