Hi Am 21.04.20 um 12:45 schrieb Daniel Vetter: > On Mon, Apr 20, 2020 at 3:37 PM Thomas Zimmermann wrote: >> >> Hi >> >> Am 15.04.20 um 09:39 schrieb Daniel Vetter: >>> Add a new macro helper to combine the usual init sequence in drivers, >>> consisting of a kzalloc + devm_drm_dev_init + drmm_add_final_kfree >>> triplet. This allows us to remove the rather unsightly >>> drmm_add_final_kfree from all currently merged drivers. >>> >>> The kerneldoc is only added for this new function. Existing kerneldoc >>> and examples will be udated at the very end, since once all drivers >>> are converted over to devm_drm_dev_alloc we can unexport a lot of >>> interim functions and make the documentation for driver authors a lot >>> cleaner and less confusing. There will be only one true way to >>> initialize a drm_device at the end of this, which is going to be >>> devm_drm_dev_alloc. >>> >>> v2: >>> - Actually explain what this is for in the commit message (Sam) >>> - Fix checkpatch issues (Sam) >>> >>> Acked-by: Noralf Trønnes >>> Cc: Noralf Trønnes >>> Reviewed-by: Sam Ravnborg >>> Cc: Sam Ravnborg >>> Cc: Paul Kocialkowski >>> Cc: Laurent Pinchart >>> Signed-off-by: Daniel Vetter > > Thanks for taking a look, some questions on your suggestions below. > >> Sorry for being late. A number of nits are listed below. In any case: >> >> Reviewed-by: Thomas Zimmermann >> >> Best regards >> Thomas >> >>> --- >>> drivers/gpu/drm/drm_drv.c | 23 +++++++++++++++++++++++ >>> include/drm/drm_drv.h | 33 +++++++++++++++++++++++++++++++++ >>> 2 files changed, 56 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c >>> index 1bb4f636b83c..8e1813d2a12e 100644 >>> --- a/drivers/gpu/drm/drm_drv.c >>> +++ b/drivers/gpu/drm/drm_drv.c >>> @@ -739,6 +739,29 @@ int devm_drm_dev_init(struct device *parent, >>> } >>> EXPORT_SYMBOL(devm_drm_dev_init); >>> >>> +void *__devm_drm_dev_alloc(struct device *parent, struct drm_driver *driver, >>> + size_t size, size_t offset) >> >> Maybe rename 'offset' of 'dev_offset' to make the relationship clear. > > Hm, I see the point of this (and the dev_field below, although I'd go > with dev_member there for some consistency with other macros using > offset_of or container_of), but I'm not sure about the dev_ prefix. > Drivers use that sometimes for the struct device *, and usage for > struct drm_device * is also very inconsistent. I've seen ddev, drm, > dev and base (that one only for embedded structs ofc). So not sure > which prefix to pick, aside from dev_ seems the most confusing. Got > ideas? We have pdev for the PCI device, dev for the abstract device, and things like mdev for struct mga_device in mgag200. So I'd go with ddev. I don't like drm, because it could be anything in DRM. I guess struct drm_driver is more 'drm' than struct drm_device. But all of this is bikeshedding. It's probably best to keep the patch as-is, and maybe rename variables later if we ever find consent on the naming. > >>> +{ >>> + void *container; >>> + struct drm_device *drm; >>> + int ret; >>> + >>> + container = kzalloc(size, GFP_KERNEL); >>> + if (!container) >>> + return ERR_PTR(-ENOMEM); >>> + >>> + drm = container + offset; >> >> While convenient, I somewhat dislike the use of void* variables. I'd use >> unsigned char* for container and do an explicit cast to struct >> drm_device* here. > > I thought ever since C89 the explicit recommendation for untyped > pointer math has been void *, and no longer char *, with the spec > being explicit that void * pointer math works exactly like char *. So From how I understand the C spec, I think it's the other way around. I had to look up the sections from the C11 spec: Sec 6.5.6 Additive operators 2 For addition, either both operands shall have arithmetic type, or one operand shall be a pointer to a complete object type and the other shall have integer type. (Incrementing is equivalent to adding 1.) About void it says that it's not a complete type. Sec 6.2.5 Types 19 The void type comprises an empty set of values; it is an incomplete object type that cannot be completed. Arithmetic on void* is a gcc extension AFAIK. > not clear on why you think char * is preferred here. I'm also not > aware of any other kernel code that casts to char * for untyped > pointer math. So unless you have some supporting evidence, I'll skip > this one, ok? I'm really just bikeshedding on things I'd have done differently. Best regards Thomas > > Thanks, Daniel > >>> + ret = devm_drm_dev_init(parent, drm, driver); >>> + if (ret) { >>> + kfree(container); >>> + return ERR_PTR(ret); >>> + } >>> + drmm_add_final_kfree(drm, container); >>> + >>> + return container; >>> +} >>> +EXPORT_SYMBOL(__devm_drm_dev_alloc); >>> + >>> /** >>> * drm_dev_alloc - Allocate new DRM device >>> * @driver: DRM driver to allocate device for >>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h >>> index e7c6ea261ed1..f07f15721254 100644 >>> --- a/include/drm/drm_drv.h >>> +++ b/include/drm/drm_drv.h >>> @@ -626,6 +626,39 @@ int devm_drm_dev_init(struct device *parent, >>> struct drm_device *dev, >>> struct drm_driver *driver); >>> >>> +void *__devm_drm_dev_alloc(struct device *parent, struct drm_driver *driver, >>> + size_t size, size_t offset); >>> + >>> +/** >>> + * devm_drm_dev_alloc - Resource managed allocation of a &drm_device instance >>> + * @parent: Parent device object >>> + * @driver: DRM driver >>> + * @type: the type of the struct which contains struct &drm_device >>> + * @member: the name of the &drm_device within @type. >>> + * >>> + * This allocates and initialize a new DRM device. No device registration is done. >>> + * Call drm_dev_register() to advertice the device to user space and register it >>> + * with other core subsystems. This should be done last in the device >>> + * initialization sequence to make sure userspace can't access an inconsistent >>> + * state. >>> + * >>> + * The initial ref-count of the object is 1. Use drm_dev_get() and >>> + * drm_dev_put() to take and drop further ref-counts. >>> + * >>> + * It is recommended that drivers embed &struct drm_device into their own device >>> + * structure. >>> + * >>> + * Note that this manages the lifetime of the resulting &drm_device >>> + * automatically using devres. The DRM device initialized with this function is >>> + * automatically put on driver detach using drm_dev_put(). >>> + * >>> + * RETURNS: >>> + * Pointer to new DRM device, or ERR_PTR on failure. >>> + */ >>> +#define devm_drm_dev_alloc(parent, driver, type, member) \ >> >> I'd replace 'member' with 'dev_field' to make the relation ship clear. >> >>> + ((type *) __devm_drm_dev_alloc(parent, driver, sizeof(type), \ >>> + offsetof(type, member))) >>> + >>> struct drm_device *drm_dev_alloc(struct drm_driver *driver, >>> struct device *parent); >>> int drm_dev_register(struct drm_device *dev, unsigned long flags); >>> >> >> -- >> Thomas Zimmermann >> Graphics Driver Developer >> SUSE Software Solutions Germany GmbH >> Maxfeldstr. 5, 90409 Nürnberg, Germany >> (HRB 36809, AG Nürnberg) >> Geschäftsführer: Felix Imendörffer >> > > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer