From mboxrd@z Thu Jan 1 00:00:00 1970 From: Imre Deak Subject: Re: [PATCH 1/3] drm/i915: dp: fix order of dp aux i2c device cleanup Date: Mon, 10 Feb 2014 20:23:51 +0200 Message-ID: <1392056631.5184.2.camel@intelbox> References: <1391820733-7767-1-git-send-email-imre.deak@intel.com> <20140210092355.GP17001@phenom.ffwll.local> <1392027327.4529.23.camel@ideak-mobl> <20140210173756.GD17001@phenom.ffwll.local> Reply-To: imre.deak@intel.com Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0147716718==" Return-path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTP id 79B3DF9D22 for ; Mon, 10 Feb 2014 10:23:56 -0800 (PST) In-Reply-To: <20140210173756.GD17001@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Daniel Vetter Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org --===============0147716718== Content-Type: multipart/signed; micalg="pgp-sha1"; protocol="application/pgp-signature"; boundary="=-Ay98mPTKe3qXSYuUrbir" --=-Ay98mPTKe3qXSYuUrbir Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, 2014-02-10 at 18:37 +0100, Daniel Vetter wrote: > On Mon, Feb 10, 2014 at 12:15:27PM +0200, Imre Deak wrote: > > On Mon, 2014-02-10 at 10:23 +0100, Daniel Vetter wrote: > > > On Sat, Feb 08, 2014 at 02:52:11AM +0200, Imre Deak wrote: > > > > Atm we set the parent of the dp i2c device to be the correspondig > > > > connector device. During driver cleanup we first remove the connect= or > > > > device through intel_modeset_cleanup()->drm_sysfs_connector_remove(= ) and > > > > only after that the i2c device through the encoder's destroy callba= ck. > > > > This order is not supported by the device core and we'll get a warn= ing, > > > > see the below bugzilla ticket. The proper order is to remove first = any > > > > child device and only then the parent device. > > > >=20 > > > > The first part of the fix changes the i2c device's parent to be the= drm > > > > device. Its logical owner is not the connector anyway, but the enco= der. > > > > Since the encoder doesn't have a device object, the next best choic= e is > > > > the drm device. This is the same what we do in the case of the sdvo= i2c > > > > device and what the nouveau driver does. > > > >=20 > > > > The second part creates a symlink in the connector's sysfs director= y > > > > pointing to the i2c device. This is so, that we keep the current AB= I, > > > > which also makes sense in case someone wants to look up the i2c dev= ice > > > > belonging to a specific connector. To remove this symlink a new > > > > unregister callback is added to the connector. In general this shou= ld > > > > remove any custom interfaces we added, through which the connector = can > > > > be accessed. > > > >=20 > > > > I also thought of moving the connector sysfs cleanup into its destr= oy > > > > callback, but that would mean allowing user space access to race wi= th > > > > the destruction of the encoders for example. The following comment > > > > for drm_mode_config_cleanup() also tells me that we have to make su= re > > > > there is no pending or new access to the connectors before we call = it: > > > >=20 > > > > """ > > > > Note that since this /should/ happen single-threaded at driver/devi= ce > > > > teardown time, no locking is required. It's the driver's job to > > > > ensure that this guarantee actually holds true. > > > > """ > > > >=20 > > > > Reference: http://lists.freedesktop.org/archives/intel-gfx/2014-Jan= uary/038782.html > > > > Reference: http://lists.freedesktop.org/archives/intel-gfx/2014-Feb= ruary/039427.html > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=3D70523 > > > > Signed-off-by: Imre Deak > > > > --- > > > > drivers/gpu/drm/i915/intel_display.c | 5 +++++ > > > > drivers/gpu/drm/i915/intel_dp.c | 24 +++++++++++++++++++++++- > > > > drivers/gpu/drm/i915/intel_drv.h | 7 +++++++ > > > > 3 files changed, 35 insertions(+), 1 deletion(-) > > > >=20 > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm= /i915/intel_display.c > > > > index 4d4a0d9..5b5ec8a 100644 > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > @@ -11472,6 +11472,11 @@ void intel_modeset_cleanup(struct drm_devi= ce *dev) > > > > =20 > > > > /* destroy the backlight and sysfs files before encoders/connecto= rs */ > > > > list_for_each_entry(connector, &dev->mode_config.connector_list, = head) { > > > > + struct intel_connector *intel_connector; > > > > + > > > > + intel_connector =3D to_intel_connector(connector); > > > > + if (intel_connector->unregister) > > > > + intel_connector->unregister(intel_connector); > > > > intel_panel_destroy_backlight(connector); > > > > drm_sysfs_connector_remove(connector); > > > > } > > >=20 > > > I don't see what this new callback buys us compared to moving everyth= ing > > > into the ->destroy callbacks: The driver is already half torn-down, s= o > > > userspace better not poke at any sysfs/debugfs files. > >=20 > > But the above part makes sure exactly that userspace can't access any > > sysfs files. The ->unregister callback is doing the connector specific > > part of that. If we move these things into ->destroy userspace will hav= e > > access to all these sysfs files and if that happens during the followin= g > > encoder/connector ->destroy callbacks things will break. See for > > example=20 > >=20 > > commit d9255d57147e1dbcebdf6670409c2fa0ac3609e6 > > Author: Paulo Zanoni > > Date: Thu Sep 26 20:05:59 2013 -0300 > >=20 > > which fixed a bug caused by such a userspace access. > >=20 > > > Fixing this correctly will be much more invasive (and to be able to d= o it > > > we need to start at the drm core), adding new stuff here will only ma= ke > > > things more complicated once we get around to it. > >=20 > > I think it still make sense to do an early removal of any userspace > > interface to the driver before we start tearing down things. It's > > similar to how ioctls and other open fds of the driver node prevent the > > teardown from starting, by holding a reference on the module. >=20 > Yeah, in the end we need to have a two-phase driver unload sequence: > 1. Unregister all userspace interfaces. > 2. Clean up the mess. Yep. And btw for driver init we would need a similar split: 1. Init everything 2. Register all userspace interfaces Which is again not how things work today .. > I'm just afraid we'll run into the wrong direction with the bigger pictur= e > in mind. On 2nd thought the connector->unregister kinda makes sense, so I > guess I'll merge this one. But I can you please follow up with a patch to > also shovel the destroy_backlight call into the respective ->unregister > functions? Then we have all the connector-specific stuff neatly tied up, > the current split looks a bit strange imo. Ok. --Imre --=-Ay98mPTKe3qXSYuUrbir Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.14 (GNU/Linux) iQEcBAABAgAGBQJS+Rk3AAoJEORIIAnNuWDFmMAIAJ4uZGR4yI3Bo0psU7pnwje8 XlJamZs9e0zE9EiN5pl3nuD5wNMhJu6NZGtH8gLqpniGr263miXeohZewcWxhFX9 A/2iH7yVK8KxZjDjzsY9la78VZWd3HkKyvWJDK8aqW8WyxBjuORZjmjJucLwCeii HABB8nWw832jqL1LPBoRyzRtJDAaAuVFFvkAq/l3wPUXkAtEJdbxrtkVuPw8ZQdA swdjx/zo0NcTNsFYMZlWB0XDlQJB02/vP4d8t6rD9d6YRpgKg0dIiNMy7MNmmVlB +23dJA11W4KxmOFVZ8Wq1QCPmPj52ZQDzWP/UrOjZqDGjJA8I0lN7CdMCXgvsE0= =0MV3 -----END PGP SIGNATURE----- --=-Ay98mPTKe3qXSYuUrbir-- --===============0147716718== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx --===============0147716718==--