Hi, On 7/4/23 16:28, Andy Shevchenko wrote: > 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. Yeah, no sorry. As Dan pointed out this fix is fine and I don't feel like re-writing it just because you don't like it. I don't see any real technical arguments against this approach, just you not liking it. Regards, Hans
