| Message ID | 20210503154647.142551-6-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | drm + usb-type-c: Add support for out-of-band hotplug notification (v2) | expand |
Hi Andy, > > +/* NOTE: The connector order must be final before this is called. */ > > +void intel_acpi_assign_connector_fwnodes(struct drm_i915_private *i915) > > +{ > > + struct drm_connector_list_iter conn_iter; > > + struct drm_device *drm_dev = &i915->drm; > > + struct device *kdev = &drm_dev->pdev->dev; > > + struct fwnode_handle *fwnode = NULL; > > + struct drm_connector *connector; > > + struct acpi_device *adev; > > + > > + drm_connector_list_iter_begin(drm_dev, &conn_iter); > > + drm_for_each_connector_iter(connector, &conn_iter) { > > + /* Always getting the next, even when the last was not > > used. */ > > + fwnode = device_get_next_child_node(kdev, fwnode); > > + if (!fwnode) > > + break; > > Who is dropping reference counting on fwnode ? > > I’m in the middle of a pile of fixes for fwnode refcounting when > for_each_child or get_next_child is used. So, please double check you drop > a reference. Sorry Andy. This patch is from time before the software nodes implementation of the get_next_child callback handled the ref counting properly. Br,
Hi, On 5/4/21 9:52 AM, Andy Shevchenko wrote: > > > On Monday, May 3, 2021, Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>> wrote: > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com <mailto:heikki.krogerus@linux.intel.com>> > > On Intel platforms we know that the ACPI connector device > node order will follow the order the driver (i915) decides. > The decision is made using the custom Intel ACPI OpRegion > (intel_opregion.c), though the driver does not actually know > that the values it sends to ACPI there are used for > associating a device node for the connectors, and assigning > address for them. > > In reality that custom Intel ACPI OpRegion actually violates > ACPI specification (we supply dynamic information to objects > that are defined static, for example _ADR), however, it > makes assigning correct connector node for a connector entry > straightforward (it's one-on-one mapping). > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com <mailto:heikki.krogerus@linux.intel.com>> > [hdegoede@redhat.com <mailto:hdegoede@redhat.com>: Move intel_acpi_assign_connector_fwnodes() to > intel_acpi.c] > Signed-off-by: Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>> > --- > drivers/gpu/drm/i915/display/intel_acpi.c | 40 ++++++++++++++++++++ > drivers/gpu/drm/i915/display/intel_acpi.h | 3 ++ > drivers/gpu/drm/i915/display/intel_display.c | 1 + > 3 files changed, 44 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_acpi.c b/drivers/gpu/drm/i915/display/intel_acpi.c > index 833d0c1be4f1..9f266dfda7dd 100644 > --- a/drivers/gpu/drm/i915/display/intel_acpi.c > +++ b/drivers/gpu/drm/i915/display/intel_acpi.c > @@ -263,3 +263,43 @@ void intel_acpi_device_id_update(struct drm_i915_private *dev_priv) > } > drm_connector_list_iter_end(&conn_iter); > } > + > +/* NOTE: The connector order must be final before this is called. */ > +void intel_acpi_assign_connector_fwnodes(struct drm_i915_private *i915) > +{ > + struct drm_connector_list_iter conn_iter; > + struct drm_device *drm_dev = &i915->drm; > + struct device *kdev = &drm_dev->pdev->dev; > + struct fwnode_handle *fwnode = NULL; > + struct drm_connector *connector; > + struct acpi_device *adev; > + > + drm_connector_list_iter_begin(drm_dev, &conn_iter); > + drm_for_each_connector_iter(connector, &conn_iter) { > + /* Always getting the next, even when the last was not used. */ > + fwnode = device_get_next_child_node(kdev, fwnode); > + if (!fwnode) > + break; > > > > Who is dropping reference counting on fwnode ? We are dealing with ACPI fwnode-s here and those are not ref-counted, they are embedded inside a struct acpi_device and their lifetime is tied to that struct. They should probably still be ref-counted (with the count never dropping to 0) so that the generic fwnode functions behave the same anywhere but atm the ACPI nodes are not refcounted, see: acpi_get_next_subnode() in drivers/acpi/property.c which is the get_next_child_node() implementation for ACPI fwnode-s. > I’m in the middle of a pile of fixes for fwnode refcounting when for_each_child or get_next_child is used. So, please double check you drop a reference. The kdoc comments on device_get_next_child_node() / fwnode_get_next_child_node() do not mention anything about these functions returning a reference. So I think we need to first make up our mind here how we want this all to work and then fix the actual implementation and docs before fixing callers. Regards, Hans > > > + > + switch (connector->connector_type) { > + case DRM_MODE_CONNECTOR_LVDS: > + case DRM_MODE_CONNECTOR_eDP: > + case DRM_MODE_CONNECTOR_DSI: > + /* > + * Integrated displays have a specific address 0x1f on > + * most Intel platforms, but not on all of them. > + */ > + adev = acpi_find_child_device(ACPI_COMPANION(kdev), > + 0x1f, 0); > + if (adev) { > + connector->fwnode = acpi_fwnode_handle(adev); > + break; > + } > + fallthrough; > + default: > + connector->fwnode = fwnode; > + break; > + } > + } > + drm_connector_list_iter_end(&conn_iter); > +} > diff --git a/drivers/gpu/drm/i915/display/intel_acpi.h b/drivers/gpu/drm/i915/display/intel_acpi.h > index e8b068661d22..d2435691f4b5 100644 > --- a/drivers/gpu/drm/i915/display/intel_acpi.h > +++ b/drivers/gpu/drm/i915/display/intel_acpi.h > @@ -12,11 +12,14 @@ struct drm_i915_private; > void intel_register_dsm_handler(void); > void intel_unregister_dsm_handler(void); > void intel_acpi_device_id_update(struct drm_i915_private *i915); > +void intel_acpi_assign_connector_fwnodes(struct drm_i915_private *i915); > #else > static inline void intel_register_dsm_handler(void) { return; } > static inline void intel_unregister_dsm_handler(void) { return; } > static inline > void intel_acpi_device_id_update(struct drm_i915_private *i915) { return; } > +static inline > +void intel_acpi_assign_connector_fwnodes(struct drm_i915_private *i915) { return; } > #endif /* CONFIG_ACPI */ > > #endif /* __INTEL_ACPI_H__ */ > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 828ef4c5625f..87cad549632c 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -14970,6 +14970,7 @@ int intel_modeset_init_nogem(struct drm_i915_private *i915) > > drm_modeset_lock_all(dev); > intel_modeset_setup_hw_state(dev, dev->mode_config.acquire_ctx); > + intel_acpi_assign_connector_fwnodes(i915); > drm_modeset_unlock_all(dev); > > for_each_intel_crtc(dev, crtc) { > -- > 2.31.1 > > > > -- > With Best Regards, > Andy Shevchenko > >
On Wed, May 5, 2021 at 12:07 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 5/4/21 9:52 AM, Andy Shevchenko wrote: > > On Monday, May 3, 2021, Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>> wrote: ... > > + fwnode = device_get_next_child_node(kdev, fwnode); > > Who is dropping reference counting on fwnode ? > > We are dealing with ACPI fwnode-s here and those are not ref-counted, they > are embedded inside a struct acpi_device and their lifetime is tied to > that struct. They should probably still be ref-counted (with the count > never dropping to 0) so that the generic fwnode functions behave the same > anywhere but atm the ACPI nodes are not refcounted, see: acpi_get_next_subnode() > in drivers/acpi/property.c which is the get_next_child_node() implementation > for ACPI fwnode-s. Yes, ACPI currently is exceptional, but fwnode API is not. If you may guarantee that this case won't ever be outside of ACPI and even though if ACPI won't ever gain a reference counting for fwnodes, we can leave it as is. > > I’m in the middle of a pile of fixes for fwnode refcounting when for_each_child or get_next_child is used. So, please double check you drop a reference. > > The kdoc comments on device_get_next_child_node() / fwnode_get_next_child_node() > do not mention anything about these functions returning a reference. It's possible. I dunno if it had to be done earlier. Sakari? > So I think we need to first make up our mind here how we want this all to > work and then fix the actual implementation and docs before fixing callers. We have already issues, so I prefer not to wait for a documentation update, because for old kernels it will still be an issue. In any case most of my fixes are against LED subsystem (drivers) and they are valid due to use in the OF environment.
Hi, On 5/5/21 11:17 AM, Andy Shevchenko wrote: > On Wed, May 5, 2021 at 12:07 PM Hans de Goede <hdegoede@redhat.com> wrote: >> On 5/4/21 9:52 AM, Andy Shevchenko wrote: >>> On Monday, May 3, 2021, Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>> wrote: > > ... > >>> + fwnode = device_get_next_child_node(kdev, fwnode); > >>> Who is dropping reference counting on fwnode ? >> >> We are dealing with ACPI fwnode-s here and those are not ref-counted, they >> are embedded inside a struct acpi_device and their lifetime is tied to >> that struct. They should probably still be ref-counted (with the count >> never dropping to 0) so that the generic fwnode functions behave the same >> anywhere but atm the ACPI nodes are not refcounted, see: acpi_get_next_subnode() >> in drivers/acpi/property.c which is the get_next_child_node() implementation >> for ACPI fwnode-s. > > Yes, ACPI currently is exceptional, but fwnode API is not. > If you may guarantee that this case won't ever be outside of ACPI Yes I can guarantee that currently this code (which is for the i915 driver only) only deals with ACPI fwnodes. > and > even though if ACPI won't ever gain a reference counting for fwnodes, > we can leave it as is. Would it not be better to add fake ref-counting to the ACPI fwnode next_child_node() op though. I believe just getting a reference on the return value there should work fine; and then all fwnode implementations would be consistent ? (note I did not check that the of and swnode code do return a reference but I would assume so). >>> I’m in the middle of a pile of fixes for fwnode refcounting when for_each_child or get_next_child is used. So, please double check you drop a reference. >> >> The kdoc comments on device_get_next_child_node() / fwnode_get_next_child_node() >> do not mention anything about these functions returning a reference. > > It's possible. I dunno if it had to be done earlier. Sakari? > >> So I think we need to first make up our mind here how we want this all to >> work and then fix the actual implementation and docs before fixing callers. > > We have already issues, so I prefer not to wait for a documentation > update, because for old kernels it will still be an issue. I wonder if we really have issues though, in practice fwnodes are generated from an devicetree or ACPI tables (or by platform codes adding swnodes) and then these pretty much stick around for ever. IOW the initial refcount of 1 is never dropped at least for of-nodes and ACPI nodes. I know there are some exceptions like device-tree overlays which I guess may also be dynamically removed again, but those exceptions are not widely used. And if we forget to drop a reference in the worst case we have a small non-re-occuring (so not growing) memleak. Where as if we start adding put() calls everywhere we may end up freeing things which are still in use; or dropping refcounts below 0 triggering WARNs in various places (IIRC). So it seems the cure is potentially worse then the disease in this case. So if you want to work on this, then IMHO it would be best to first make sure that all the fwnode implementations behave in the same way wrt ref-counting, before adding the missing put() calls in various places. And once the behavior is consistent then we can also document this properly making it easier for other people to do the right thing when using these functions. Regards, Hans
On Wed, May 5, 2021 at 12:28 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 5/5/21 11:17 AM, Andy Shevchenko wrote: > > On Wed, May 5, 2021 at 12:07 PM Hans de Goede <hdegoede@redhat.com> wrote: > >> On 5/4/21 9:52 AM, Andy Shevchenko wrote: > >>> On Monday, May 3, 2021, Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>> wrote: > > > > ... > > > >>> + fwnode = device_get_next_child_node(kdev, fwnode); > > > >>> Who is dropping reference counting on fwnode ? > >> > >> We are dealing with ACPI fwnode-s here and those are not ref-counted, they > >> are embedded inside a struct acpi_device and their lifetime is tied to > >> that struct. They should probably still be ref-counted (with the count > >> never dropping to 0) so that the generic fwnode functions behave the same > >> anywhere but atm the ACPI nodes are not refcounted, see: acpi_get_next_subnode() > >> in drivers/acpi/property.c which is the get_next_child_node() implementation > >> for ACPI fwnode-s. > > > > Yes, ACPI currently is exceptional, but fwnode API is not. > > If you may guarantee that this case won't ever be outside of ACPI > > Yes I can guarantee that currently this code (which is for the i915 > driver only) only deals with ACPI fwnodes. > > > and > > even though if ACPI won't ever gain a reference counting for fwnodes, > > we can leave it as is. > > Would it not be better to add fake ref-counting to the ACPI fwnode > next_child_node() op though. I believe just getting a reference > on the return value there should work fine; and then all fwnode > implementations would be consistent ? But it's already there by absent put/get callbacks. On fwnode level it is like you described. So, talking for a good pattern we have to call the fwnode_handle_put() independently and always for for_each_child and get_next_child usages. > (note I did not check that the of and swnode code do return > a reference but I would assume so). Yes, it's only ACPI that survives w/o reference counting. > >>> I’m in the middle of a pile of fixes for fwnode refcounting when for_each_child or get_next_child is used. So, please double check you drop a reference. > >> > >> The kdoc comments on device_get_next_child_node() / fwnode_get_next_child_node() > >> do not mention anything about these functions returning a reference. > > > > It's possible. I dunno if it had to be done earlier. Sakari? > > > >> So I think we need to first make up our mind here how we want this all to > >> work and then fix the actual implementation and docs before fixing callers. > > > > We have already issues, so I prefer not to wait for a documentation > > update, because for old kernels it will still be an issue. > > I wonder if we really have issues though, in practice fwnodes are > generated from an devicetree or ACPI tables (or by platform codes > adding swnodes) and then these pretty much stick around for ever. Overlays. Not for ever. > IOW the initial refcount of 1 is never dropped at least for of-nodes > and ACPI nodes. > I know there are some exceptions like device-tree > overlays which I guess may also be dynamically removed again, but those > exceptions are not widely used. ACPI overlays are quite used (at least by two people I know and a few more that asked questions about them here and there), but luckily it doesn't require refcounting (yet?). > And if we forget to drop a reference in the worst case we have a small > non-re-occuring (so not growing) memleak. And is it good to provoke all kinds of tools (kmemleak, *SANs, etc)? I do not think so. If we are writing good code it should be good enough. > Where as if we start adding > put() calls everywhere we may end up freeing things which are still > in use; or dropping refcounts below 0 triggering WARNs in various > places (IIRC). Which is good. Then we will discover real issues. > So it seems the cure is potentially worse then the disease in this > case. I tend to disagree with you. How in this case we can go below 0 in case we know that we took a counter? If somewhere else the code will do that, it is a problem that has to be fixed on case-by-case basis. > So if you want to work on this, then IMHO it would be best to first make > sure that all the fwnode implementations behave in the same way wrt > ref-counting, before adding the missing put() calls in various > places. > > And once the behavior is consistent It's consistent now independently of the beneath layer from fwnode API p.o.v. > then we can also document this > properly making it easier for other people to do the right thing > when using these functions.
Hi Andy, Hans, On Wed, May 05, 2021 at 12:17:14PM +0300, Andy Shevchenko wrote: > On Wed, May 5, 2021 at 12:07 PM Hans de Goede <hdegoede@redhat.com> wrote: > > On 5/4/21 9:52 AM, Andy Shevchenko wrote: > > > On Monday, May 3, 2021, Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>> wrote: > > ... > > > > + fwnode = device_get_next_child_node(kdev, fwnode); > > > > Who is dropping reference counting on fwnode ? > > > > We are dealing with ACPI fwnode-s here and those are not ref-counted, they > > are embedded inside a struct acpi_device and their lifetime is tied to > > that struct. They should probably still be ref-counted (with the count > > never dropping to 0) so that the generic fwnode functions behave the same > > anywhere but atm the ACPI nodes are not refcounted, see: acpi_get_next_subnode() > > in drivers/acpi/property.c which is the get_next_child_node() implementation > > for ACPI fwnode-s. > > Yes, ACPI currently is exceptional, but fwnode API is not. > If you may guarantee that this case won't ever be outside of ACPI and > even though if ACPI won't ever gain a reference counting for fwnodes, > we can leave it as is. > > > > I’m in the middle of a pile of fixes for fwnode refcounting when > > > for_each_child or get_next_child is used. So, please double check you > > > drop a reference. > > > > The kdoc comments on device_get_next_child_node() / fwnode_get_next_child_node() > > do not mention anything about these functions returning a reference. > > It's possible. I dunno if it had to be done earlier. Sakari? The fwnode API has worked with references for a long time, looks like from the very beginning of it, as in patch 8a0662d9ed2968e1186208336a8e1fab3fdfea63 . If you're expecting an fwnode family of function returning another node, then that function has to have taken a reference to that node before returning it. Otherwise there's no guarantee that node is still there when it is returned. It may be this is not documented for every function doing so. That should probably be added.
Hi, On 5/5/21 12:02 PM, Andy Shevchenko wrote: > On Wed, May 5, 2021 at 12:28 PM Hans de Goede <hdegoede@redhat.com> wrote: >> On 5/5/21 11:17 AM, Andy Shevchenko wrote: >>> On Wed, May 5, 2021 at 12:07 PM Hans de Goede <hdegoede@redhat.com> wrote: >>>> On 5/4/21 9:52 AM, Andy Shevchenko wrote: >>>>> On Monday, May 3, 2021, Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>> wrote: >>> >>> ... >>> >>>>> + fwnode = device_get_next_child_node(kdev, fwnode); >>> >>>>> Who is dropping reference counting on fwnode ? >>>> >>>> We are dealing with ACPI fwnode-s here and those are not ref-counted, they >>>> are embedded inside a struct acpi_device and their lifetime is tied to >>>> that struct. They should probably still be ref-counted (with the count >>>> never dropping to 0) so that the generic fwnode functions behave the same >>>> anywhere but atm the ACPI nodes are not refcounted, see: acpi_get_next_subnode() >>>> in drivers/acpi/property.c which is the get_next_child_node() implementation >>>> for ACPI fwnode-s. >>> >>> Yes, ACPI currently is exceptional, but fwnode API is not. >>> If you may guarantee that this case won't ever be outside of ACPI >> >> Yes I can guarantee that currently this code (which is for the i915 >> driver only) only deals with ACPI fwnodes. >> >>> and >>> even though if ACPI won't ever gain a reference counting for fwnodes, >>> we can leave it as is. >> >> Would it not be better to add fake ref-counting to the ACPI fwnode >> next_child_node() op though. I believe just getting a reference >> on the return value there should work fine; and then all fwnode >> implementations would be consistent ? > > But it's already there by absent put/get callbacks. Ah, I completely missed that the put/get-s are actually done through function pointers in fwnode_operations. I assumed that there was a kref embedded inside the fwnode_handle struct and that they operated directly on that. So this whole discussion is entirely based on that misunderstanding, my bad, sorry. So yes you are right, things are already consistent thanks to the absent put/get callbacks. But we do really need to document the behavior better here in the kdoc for fwnode_get_next_child_node() and device_get_next_child_node(). of_get_next_child has this bit, which applies to those too: * Returns a node pointer with refcount incremented, use of_node_put() on * it when done. Returns NULL when prev is the last child. Decrements the * refcount of prev. I'll prepare a patch to add this to the kdoc for fwnode_get_next_child_node() and device_get_next_child_node() once I'm done with readying v3 of this series. Regards, Hans
On Wed, May 5, 2021 at 1:30 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 5/5/21 12:02 PM, Andy Shevchenko wrote: ... > But we do really need to document the behavior better here > in the kdoc for fwnode_get_next_child_node() and > device_get_next_child_node(). Totally agree! > of_get_next_child has this bit, which applies to those too: > > * Returns a node pointer with refcount incremented, use of_node_put() on > * it when done. Returns NULL when prev is the last child. Decrements the > * refcount of prev. > > I'll prepare a patch to add this to the kdoc for fwnode_get_next_child_node() > and device_get_next_child_node() once I'm done with readying v3 of this series. Thanks!
diff --git a/drivers/gpu/drm/i915/display/intel_acpi.c b/drivers/gpu/drm/i915/display/intel_acpi.c index 833d0c1be4f1..9f266dfda7dd 100644 --- a/drivers/gpu/drm/i915/display/intel_acpi.c +++ b/drivers/gpu/drm/i915/display/intel_acpi.c @@ -263,3 +263,43 @@ void intel_acpi_device_id_update(struct drm_i915_private *dev_priv) } drm_connector_list_iter_end(&conn_iter); } + +/* NOTE: The connector order must be final before this is called. */ +void intel_acpi_assign_connector_fwnodes(struct drm_i915_private *i915) +{ + struct drm_connector_list_iter conn_iter; + struct drm_device *drm_dev = &i915->drm; + struct device *kdev = &drm_dev->pdev->dev; + struct fwnode_handle *fwnode = NULL; + struct drm_connector *connector; + struct acpi_device *adev; + + drm_connector_list_iter_begin(drm_dev, &conn_iter); + drm_for_each_connector_iter(connector, &conn_iter) { + /* Always getting the next, even when the last was not used. */ + fwnode = device_get_next_child_node(kdev, fwnode); + if (!fwnode) + break; + + switch (connector->connector_type) { + case DRM_MODE_CONNECTOR_LVDS: + case DRM_MODE_CONNECTOR_eDP: + case DRM_MODE_CONNECTOR_DSI: + /* + * Integrated displays have a specific address 0x1f on + * most Intel platforms, but not on all of them. + */ + adev = acpi_find_child_device(ACPI_COMPANION(kdev), + 0x1f, 0); + if (adev) { + connector->fwnode = acpi_fwnode_handle(adev); + break; + } + fallthrough; + default: + connector->fwnode = fwnode; + break; + } + } + drm_connector_list_iter_end(&conn_iter); +} diff --git a/drivers/gpu/drm/i915/display/intel_acpi.h b/drivers/gpu/drm/i915/display/intel_acpi.h index e8b068661d22..d2435691f4b5 100644 --- a/drivers/gpu/drm/i915/display/intel_acpi.h +++ b/drivers/gpu/drm/i915/display/intel_acpi.h @@ -12,11 +12,14 @@ struct drm_i915_private; void intel_register_dsm_handler(void); void intel_unregister_dsm_handler(void); void intel_acpi_device_id_update(struct drm_i915_private *i915); +void intel_acpi_assign_connector_fwnodes(struct drm_i915_private *i915); #else static inline void intel_register_dsm_handler(void) { return; } static inline void intel_unregister_dsm_handler(void) { return; } static inline void intel_acpi_device_id_update(struct drm_i915_private *i915) { return; } +static inline +void intel_acpi_assign_connector_fwnodes(struct drm_i915_private *i915) { return; } #endif /* CONFIG_ACPI */ #endif /* __INTEL_ACPI_H__ */ diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 828ef4c5625f..87cad549632c 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -14970,6 +14970,7 @@ int intel_modeset_init_nogem(struct drm_i915_private *i915) drm_modeset_lock_all(dev); intel_modeset_setup_hw_state(dev, dev->mode_config.acquire_ctx); + intel_acpi_assign_connector_fwnodes(i915); drm_modeset_unlock_all(dev); for_each_intel_crtc(dev, crtc) {