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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Andy,

Thank you for the review.

On Fri, Jan 27, 2023 at 12:08:39PM +0200, Andy Shevchenko wrote:
> On Thu, Jan 26, 2023 at 12:40:55AM +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.
> 
> ...
> 
> > +	if (status != AE_OK) {
> 
> ACPI_FAILURE()

Yes.

> 
> > +		acpi_handle_warn(inst_context->handle,
> > +				 "cannot get handle for %s\n",
> > +				 csi2->resource_source.string_ptr);
> > +		return AE_OK;
> > +	}
> 
> ...
> 
> > +	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);
> 
> Do you still need this?

Oops. Forgot to remove it, I'll do in v4.

> 
> ...
> 
> > +	acpi_walk_resources(handle, METHOD_NAME__CRS,
> > +			    scan_check_crs_csi2_instance, &inst_context);
> > +
> > +	if (list_empty(&inst_context.res_list))
> > +		return AE_OK;
> 
> I'm wondering if you can utilize acpi_dev_get_resources().

We don't have an acpi_device yet. I'd rather keep scanning for _CRS CSI2
resources here, as we'd otherwise have to split creating acpi_device's and
registering them into two.

I'd say if someone had implemented this like that, I'd ask them to change
it.

> 
> ...
> 
> > +	/* Collect the devices that have a _CRS CSI-2 resource */
> > +	acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, ACPI_UINT32_MAX,
> 
> Other serial buses limit the DEPTH by 32, why do we need more here?

I'm using the same value as for scanning devices to be added. Effectively
this is not a practical limit but it doesn't need to be.

> 
> > +			    scan_check_crs_csi2, NULL, &crs_csi2_handles, NULL);
> 
> ...
> 
> > +	sort(handle_refs, handle_count, sizeof(*handle_refs), crs_handle_cmp,
> > +	     NULL);
> 
> A single line?

Can do...

> 
> ...
> 
> > +		if (check_mul_overflow(sizeof(*ads->ports) +
> > +				       sizeof(*ads->nodes) * 2 +
> > +				       sizeof(*ads->nodeptrs) * 2,
> > +				       (size_t)this_count, &alloc_size) ||
> 
> So, now you know why this_count can't be type of size_t?

Forgot to change this one, thanks!

> 
> > +		    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;
> > +		}
> 

-- 
Kind regards,

Sakari Ailus



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux