| Message ID | 20230630110643.209761-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ipu-bridge: Shared with atomisp, rework VCM instantiation | expand |
On Fri, Jun 30, 2023 at 2:06 PM Hans de Goede <hdegoede@redhat.com> 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. -- With Best Regards, Andy Shevchenko
On 30/06/2023 16:23, Andy Shevchenko wrote: > On Fri, Jun 30, 2023 at 2:06 PM Hans de Goede <hdegoede@redhat.com> 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? > > -- > With Best Regards, > Andy Shevchenko
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@redhat.com> 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.
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@redhat.com> 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
On Tue, Jul 4, 2023 at 5:50 PM Hans de Goede <hdegoede@redhat.com> wrote: > 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@redhat.com> 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. OK.
diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c index 62daa8c1f6b1..f0927f80184d 100644 --- a/drivers/media/pci/intel/ipu-bridge.c +++ b/drivers/media/pci/intel/ipu-bridge.c @@ -308,6 +308,11 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg, } sensor = &bridge->sensors[bridge->n_sensors]; + /* + * Borrow our adev ref to the sensor for now, on success + * acpi_dev_get(adev) is done further below. + */ + sensor->adev = adev; ret = ipu_bridge_read_acpi_buffer(adev, "SSDB", &sensor->ssdb,
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. Fixes: 485aa3df0dff ("media: ipu3-cio2: Parse sensor orientation and rotation") Cc: Fabian Wüthrich <me@fabwu.ch> Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/media/pci/intel/ipu-bridge.c | 5 +++++ 1 file changed, 5 insertions(+)