On Wed, Sep 24, 2014 at 10:11:21AM +0200, Jean-Francois Moine wrote:
This patch interfaces the HDMI transmitter with the audio system.
...
+struct tda998x_priv2 {
- struct tda998x_priv base;
- struct drm_encoder encoder;
- struct drm_connector connector;
};
NAK on moving this up here. It was specifically placed below the core and below the older drm_slave_encoder code because nothing but the new component based interfaces to TDA998x have any business knowing that these are packaged up in this way.
+static void tda998x_create_audio_codec(struct tda998x_priv *priv) +{
- struct platform_device *pdev;
- struct module *module;
- request_module("snd-soc-hdmi-codec");
- pdev = platform_device_register_resndata(&priv->hdmi->dev,
"hdmi-audio-codec",PLATFORM_DEVID_NONE,NULL, 0,&tda998x_hdmi_data,sizeof tda998x_hdmi_data);- if (IS_ERR(pdev)) {
dev_err(&priv->hdmi->dev, "cannot create codec: %ld\n",PTR_ERR(pdev));return;- }
- priv->pdev_codec = pdev;
- module = pdev->dev.driver->owner;
- if (module)
try_module_get(module);
This is really not on. Firstly, registering a platform device has no guarantee that it will bind to a driver. So, pdev->dev.driver may be NULL here -> kernel oops.
Secondly, what's the purpose of the unchecked try_module_get() there? You can't stop the device being unbound from its driver, so all you're doing is possibly locking the module into module space for no other benefit.
@@ -1167,12 +1390,19 @@ tda998x_encoder_set_property(struct drm_encoder *encoder,
static void tda998x_destroy(struct tda998x_priv *priv) {
struct module *module;
/* disable all IRQs and free the IRQ handler */ cec_write(priv, REG_CEC_RXSHPDINTENA, 0); reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD); if (priv->hdmi->irq) free_irq(priv->hdmi->irq, priv);
if (priv->pdev_codec) {
module = priv->pdev_codec->dev.driver->owner;module_put(module);
This is wrong for all the reasons the other part above is wrong. Userspace can decide to unbind the platform device from the associated platform driver whenever it wishes. At that point, priv->pdev_codec->dev.driver becomes NULL.
So, please get rid of this.
platform_device_del(priv->pdev_codec);
This leaks the memory associated with the platform device.
@@ -1395,15 +1660,13 @@ static int tda998x_encoder_init(struct i2c_client *client, encoder_slave->slave_priv = priv; encoder_slave->slave_funcs = &tda998x_encoder_slave_funcs;
- /* set the drvdata pointer to priv2 for CODEC calls */
- dev_set_drvdata(&client->dev,
container_of(priv, struct tda998x_priv2, base));- return 0;
}
-struct tda998x_priv2 {
- struct tda998x_priv base;
- struct drm_encoder encoder;
- struct drm_connector connector;
-};
I would prefer this structure to stay here, as code above this point should have no business knowing how these are packaged together. I would suggest either:
- moving the audio codec code below this point, or - storing struct tda998x_priv in the device private pointer, and converting it to struct tda998x_priv2 via container_of() where necessary below this point.