On Tue, Jul 04, 2023 at 12:02:00PM +0100, Dan Scally wrote: > On 30/06/2023 16:23, Andy Shevchenko wrote: > > On Fri, Jun 30, 2023 at 2:06 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > > > When ipu_bridge_parse_rotation() and ipu_bridge_parse_orientation() run > > > sensor->adev is not set yet. > > > > > > So if either of the dev_warn() calls about unknown values are hit this > > > will lead to a NULL pointer deref. > > > > > > Set sensor->adev earlier, with a borrowed ref to avoid making unrolling > > > on errors harder, to fix this. > > TBH, I don't like this approach, it seems a bit dirty to me. > > > > First of all, why do we need pci_dev to be a parameter in this function? > > Second, why don't we consistently use the ACPI handle (with respective > > acpi_handle_*() macros to print messages)? > > > > So, my proposal here is to actually save the ACPI device handle in the > > sensor object and use it for the messaging. It makes it consistent and > > doesn't require to rewrite adev field which seems the dirty part to > > me. > > It's a bit finicky but I don't think it's so bad; the refcounting is all > fine, the later acpi_dev_get() is only to hold a reference once the next > loop iteration frees the existing one and the rewrite should store the exact > same pointer...we could just not store the result of the acpi_dev_get() call > to avoid that weird rewrite perhaps? For short term solution in between the patches I might agree with you, but backporting. Backporting a bad code doesn't make it better even if it fixes nasty bug. And I proposed the solution. We may kill the handle same way as we are killing the awkwardness of this assignment later in the series. -- With Best Regards, Andy Shevchenko
