[PATCH 4/6] drm/tinydrm: Embed drm_device in tinydrm_device
Noralf Trønnes
noralf at tronnes.org
Fri Sep 1 18:46:42 UTC 2017
Den 01.09.2017 09.28, skrev Laurent Pinchart:
> Hi Noralf,
>
> On Thursday, 31 August 2017 20:16:42 EEST Noralf Trønnes wrote:
>> Den 31.08.2017 12.18, skrev Laurent Pinchart:
>>> On Monday, 28 August 2017 20:17:46 EEST Noralf Trønnes wrote:
>>>> Might as well embed drm_device since tinydrm_device (embeds pipe struct
>>>> and fbdev pointer) needs to stick around after driver-device unbind to
>>>> handle open fd's after device removal.
>>>>
>>>> Cc: David Lechner <david at lechnology.com>
>>>> Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
>>>> ---
>>>>
>>>> drivers/gpu/drm/tinydrm/core/tinydrm-core.c | 44 +++++++++++-----------
>>>> drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c | 2 +-
>>>> drivers/gpu/drm/tinydrm/mi0283qt.c | 8 +++---
>>>> drivers/gpu/drm/tinydrm/mipi-dbi.c | 12 ++++----
>>>> drivers/gpu/drm/tinydrm/repaper.c | 10 +++----
>>>> drivers/gpu/drm/tinydrm/st7586.c | 16 +++++------
>>>> include/drm/tinydrm/tinydrm.h | 9 +++++-
>>>> 7 files changed, 50 insertions(+), 51 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
>>>> b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c index 551709e..f11f4cd
>>>> 100644
>>>> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
>>>> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
>>> [snip]
>>>
>>>> @@ -142,23 +142,16 @@ static int tinydrm_init(struct device *parent,
>>>> struct
>>>> tinydrm_device *tdev, const struct drm_framebuffer_funcs *fb_funcs,
>>>>
>>>> struct drm_driver *driver)
>>>>
>>>> {
>>>>
>>>> - struct drm_device *drm;
>>>> + struct drm_device *drm = &tdev->drm;
>>>> + int ret;
>>>>
>>>> mutex_init(&tdev->dirty_lock);
>>>> tdev->fb_funcs = fb_funcs;
>>>>
>>>> - /*
>>>> - * We don't embed drm_device, because that prevent us from using
>>>> - * devm_kzalloc() to allocate tinydrm_device in the driver since
>>>> - * drm_dev_unref() frees the structure. The devm_ functions provide
>>>> - * for easy error handling.
>>> Don't you then need a custom drm_driver.release handler to free the parent
>>> structure ?
>> I rely on the fact that drm_device is the first member in the driver
>> structure and thus it will be freed in drm_dev_release(). A later patch
>> adds a drm_driver.release function though.
> That's a bit hackish. As a later patch changes this I'd be OK with this one,
> but you should mention that you rely on the structure layout in the commit
> message.
I've seen it around, so I figured it was a common pattern.
Looking at it now, I see that it adds obscurity:
- Where is that driver struct freed?
- In tinydrm_release().
- How can it do that when it doesn't have a pointer to it?
- It does this indirectly since tinydrm_device is the first member.
- Ooh...
I remember now that I hit the same kind of obscurity in the vbox driver
where it assigns a private structure to fb_info.par and I know that
drm_fb_helper expects this to be struct drm_fb_helper. So how could
this work? After some digging I realised that it worked because
drm_fb_helper was the first member of the private struct.
It made the code difficult to read.
So yeah, better stay away from things like this when possible.
In patch 6 I did document this freeing behaviour in the struct
tinydrm_device docs, but that probably wouldn't have helped me much if
I was new to tinydrm. It would have taken me some time to pick up that
piece of information.
I think I'll add drm_driver.release functions to this patch and
explicitly free the driver structure, since it logically belongs here.
An upside of having the drm_driver.release callback in the driver is
the educational/awareness part, it acts as a reminder that the driver
structure can live on even after driver.remove has finished.
This argument in itself is probably strong enough to warrant one
release function per driver structure, since this lifetime issue is so
easy to get wrong.
I've also realised that the driver struct is leaked if devm_tinydrm_init()
fails, so I need to fix that as well.
Noralf.
>>>> - */
>>>> - drm = drm_dev_alloc(driver, parent);
>>>> - if (IS_ERR(drm))
>>>> - return PTR_ERR(drm);
>>>> -
>>>> - tdev->drm = drm;
>>>> - drm->dev_private = tdev;
>>>> + ret = drm_dev_init(drm, driver, parent);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>>
>>>> drm_mode_config_init(drm);
>>>> drm->mode_config.funcs = &tinydrm_mode_config_funcs;
>>> [snip]
>>>
>>>> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c
>>>> b/drivers/gpu/drm/tinydrm/mi0283qt.c index 7e5bb7d..77d40c9 100644
>>>> --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
>>>> +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
>>> [snip]
>>>
>>>> @@ -169,7 +169,7 @@ static int mi0283qt_probe(struct spi_device *spi)
>>>>
>>>> u32 rotation = 0;
>>>> int ret;
>>>>
>>>> - mipi = devm_kzalloc(dev, sizeof(*mipi), GFP_KERNEL);
>>>> + mipi = kzalloc(sizeof(*mipi), GFP_KERNEL);
>>> Where's the related kfree() ?
>>>
>>>> if (!mipi)
>>>>
>>>> return -ENOMEM;
>>> [snip]
>>>
>>>> diff --git a/drivers/gpu/drm/tinydrm/repaper.c
>>>> b/drivers/gpu/drm/tinydrm/repaper.c index 30dc97b..b8fc8eb 100644
>>>> --- a/drivers/gpu/drm/tinydrm/repaper.c
>>>> +++ b/drivers/gpu/drm/tinydrm/repaper.c
>>> [snip]
>>>
>>>> @@ -949,7 +949,7 @@ static int repaper_probe(struct spi_device *spi)
>>>>
>>>> }
>>>>
>>>> }
>>>>
>>>> - epd = devm_kzalloc(dev, sizeof(*epd), GFP_KERNEL);
>>>> + epd = kzalloc(sizeof(*epd), GFP_KERNEL);
>>> Ditto.
>>>
>>>> if (!epd)
>>>>
>>>> return -ENOMEM;
>>> [snip]
>>>
>>>> diff --git a/drivers/gpu/drm/tinydrm/st7586.c
>>>> b/drivers/gpu/drm/tinydrm/st7586.c index b439956..bc2b905 100644
>>>> --- a/drivers/gpu/drm/tinydrm/st7586.c
>>>> +++ b/drivers/gpu/drm/tinydrm/st7586.c
>>> [snip]
>>>
>>>> @@ -349,7 +349,7 @@ static int st7586_probe(struct spi_device *spi)
>>>>
>>>> u32 rotation = 0;
>>>> int ret;
>>>>
>>>> - mipi = devm_kzalloc(dev, sizeof(*mipi), GFP_KERNEL);
>>>> + mipi = kzalloc(sizeof(*mipi), GFP_KERNEL);
>>> Ang here again.
>>>
>>>> if (!mipi)
>>>>
>>>> return -ENOMEM;
>>> [snip]
More information about the dri-devel
mailing list