[v2,0/6] ACPI: scan: MIPI DiSco for Imaging support

Hi Folks,

This is a new revision of

https://lore.kernel.org/linux-acpi/13276375.uLZWGnKmhe@kreacher/

which was reported to have issues and it took time to revisit it.

> The main points from the original cover letter are still valid:
> 
> The general idea is the same - CSI-2 resource descriptors, introduced in
> ACPI 6.4 and defined by
> 
> https://uefi.org/specs/ACPI/6.5/06_Device_Configuration.html#camera-serial-i
> nterface-csi-2-connection-resource-descriptor
> 
> are found and used for creating a set of software nodes that represent the
> CSI-2 connection graph.
> 
> These software nodes need to be available before any scan handlers or ACPI
> drivers are bound to any struct acpi_device objects, so all of that is done
> at the early stage of ACPI device enumeration, but unnecessary ACPI
> namespace walks are avoided.
> 
> The CSI-2 software nodes are populated with data extracted from the CSI-2
> resource descriptors themselves and from device properties defined by the
> MIPI DiSco for Imaging specification (see
> https://www.mipi.org/specifications/mipi-disco-imaging).
> 
> Patches [4,6/6] come from the original series directly, but the other
> patches have been changes substantially, so I've decided to re-start patch
> series versioning from scratch.

The v2 addresses at least 3 issues found in the v1 by code inspection:

* A port_count field incrementation was missing in acpi_mipi_scan_crs_csi2(),
  so its value for all of the devices having CSI2 resources in _CRS was always
  1 (and it should be equal to the number of valid CSI2 connection resources).

* Some acpi_mipi_crs_csi2_list members could be freed prematurely, so they were
  inaccessible when extract_crs_csi2_conn_info() attempted to access them.

* A check of remote_swnodes() against NULL was missing, which could result in
  a crash in a case when the swnodes memory could not be allocated for some
  acpi_mipi_crs_csi2_list entries.

Apart from that, it rearranges the code somewhat to make it easier to follow
and to avoid premature freeing of memory in it in general and the new file
added by it is now called mipi-di.c (instead of mipi-disco-imaging.c) for
compactness.

The series is based on current linux-next.

Thanks!

Hi Rafael,

On Fri, Oct 20, 2023 at 04:33:56PM +0200, Rafael J. Wysocki wrote:
> Hi Folks,
> 
> This is a new revision of
> 
> https://lore.kernel.org/linux-acpi/13276375.uLZWGnKmhe@kreacher/
> 
> which was reported to have issues and it took time to revisit it.
> 
> > The main points from the original cover letter are still valid:
> > 
> > The general idea is the same - CSI-2 resource descriptors, introduced in
> > ACPI 6.4 and defined by
> > 
> > https://uefi.org/specs/ACPI/6.5/06_Device_Configuration.html#camera-serial-i
> > nterface-csi-2-connection-resource-descriptor
> > 
> > are found and used for creating a set of software nodes that represent the
> > CSI-2 connection graph.
> > 
> > These software nodes need to be available before any scan handlers or ACPI
> > drivers are bound to any struct acpi_device objects, so all of that is done
> > at the early stage of ACPI device enumeration, but unnecessary ACPI
> > namespace walks are avoided.
> > 
> > The CSI-2 software nodes are populated with data extracted from the CSI-2
> > resource descriptors themselves and from device properties defined by the
> > MIPI DiSco for Imaging specification (see
> > https://www.mipi.org/specifications/mipi-disco-imaging).
> > 
> > Patches [4,6/6] come from the original series directly, but the other
> > patches have been changes substantially, so I've decided to re-start patch
> > series versioning from scratch.
> 
> The v2 addresses at least 3 issues found in the v1 by code inspection:
> 
> * A port_count field incrementation was missing in acpi_mipi_scan_crs_csi2(),
>   so its value for all of the devices having CSI2 resources in _CRS was always
>   1 (and it should be equal to the number of valid CSI2 connection resources).
> 
> * Some acpi_mipi_crs_csi2_list members could be freed prematurely, so they were
>   inaccessible when extract_crs_csi2_conn_info() attempted to access them.
> 
> * A check of remote_swnodes() against NULL was missing, which could result in
>   a crash in a case when the swnodes memory could not be allocated for some
>   acpi_mipi_crs_csi2_list entries.
> 
> Apart from that, it rearranges the code somewhat to make it easier to follow
> and to avoid premature freeing of memory in it in general and the new file
> added by it is now called mipi-di.c (instead of mipi-disco-imaging.c) for
> compactness.
> 
> The series is based on current linux-next.

Thanks for the update. I've tested this and I can confirm it works, to the
extent implemented in the set. The rest can be implemented on top
(mainly replicating properties).

I'll comment on a few patches in the set.

Do you prefer to make the changes or shall I? I presume them to be fairly
minor.

Hi Sakari,

On Tue, Oct 31, 2023 at 11:33 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Rafael,

[cut]

> > The v2 addresses at least 3 issues found in the v1 by code inspection:
> >
> > * A port_count field incrementation was missing in acpi_mipi_scan_crs_csi2(),
> >   so its value for all of the devices having CSI2 resources in _CRS was always
> >   1 (and it should be equal to the number of valid CSI2 connection resources).
> >
> > * Some acpi_mipi_crs_csi2_list members could be freed prematurely, so they were
> >   inaccessible when extract_crs_csi2_conn_info() attempted to access them.
> >
> > * A check of remote_swnodes() against NULL was missing, which could result in
> >   a crash in a case when the swnodes memory could not be allocated for some
> >   acpi_mipi_crs_csi2_list entries.
> >
> > Apart from that, it rearranges the code somewhat to make it easier to follow
> > and to avoid premature freeing of memory in it in general and the new file
> > added by it is now called mipi-di.c (instead of mipi-disco-imaging.c) for
> > compactness.
> >
> > The series is based on current linux-next.
>
> Thanks for the update. I've tested this and I can confirm it works, to the
> extent implemented in the set. The rest can be implemented on top
> (mainly replicating properties).

Thanks for testing!

> I'll comment on a few patches in the set.
>
> Do you prefer to make the changes or shall I? I presume them to be fairly
> minor.

I can make the changes.