Re: [PATCH v2 3/3] drm/rockchip: dw_dp: Attach "max bpc" connector property

[Igor Paunovic <royalnet026@gmail.com>]

The bot is right, and it is worse than it reported. I went and read the
code rather than taking either its word or my own.

drm_bridge_connector_init() has two paths. With an HDMI bridge in the
chain it calls drmm_connector_hdmi_init() (drm_bridge_connector.c:1016);
otherwise drmm_connector_init() (:1027). And drmm_connector_hdmi_init()
already contains, at drm_connector.c:621-631, the exact block this patch
adds - same shape, same comment, because that is where I took it from -
and then calls drm_connector_attach_max_bpc_property() itself at :633.

So on a chain with an HDMI bridge my patch does two wrong things, not
one:

 - it overwrites a connector->state that has just been allocated, which
   is the leak the bot found;

 - it calls drm_connector_attach_max_bpc_property() a second time. That
   one is not a leak: the helper reuses connector->max_bpc_property when
   it already exists, so my 6..10 range is silently discarded and the
   8..max_bpc property created by the HDMI path stays. It does re-attach
   the same property to the object and overwrite state->max_bpc.

On the boards I care about there is no HDMI bridge behind dw-dp, so
neither fires today - but "does not fire on my board" is not a reason to
leave it.

For v3 both blocks get guarded on what is already there:

	if (!connector->state) {
		... create the state ...
	}

	if (!connector->max_bpc_property) {
		ret = drm_connector_attach_max_bpc_property(connector, 6, 10);
		...
	}

That leaves a question I would rather ask than paper over. The state
creation exists only in drmm_connector_hdmi_init(), while
drm_bridge_connector_init() leaves the plain path without a state - so
any bridge driver that wants a connector property at bind time has to
copy that block, as I did. Would it be better to move it into
drm_bridge_connector_init() so both paths get a state, and have drivers
just attach their properties? I am happy to write that instead, but it
touches shared code and I would rather be told than guess.

On the pre-existing [High]: the bot is right that the error path I add
inherits it. dw_dp_bind() takes devm resources on the component device,
including a non-shared devm_request_threaded_irq(), so a failed bind
leaves them held and the retry gets -EBUSY. My dw_dp_unbind() call does
not release devres. With the guards above the new path becomes hard to
reach, but it does not stop being wrong, and I did not introduce it.
I can send a separate patch moving those to the master device if the
maintainers want it - it is a different change from this one.

Igor

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip