| Message ID | 20200415074034.175360-2-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | devm_drm_dev_alloc, v2 | expand |
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 <noralf@tronnes.org> > Cc: Noralf Trønnes <noralf@tronnes.org> > Reviewed-by: Sam Ravnborg <sam@ravnborg.org> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Sorry for being late. A number of nits are listed below. In any case: Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> 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. > +{ > + 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. > + 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); >
On Mon, Apr 20, 2020 at 3:37 PM Thomas Zimmermann <tzimmermann@suse.de> 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 <noralf@tronnes.org> > > Cc: Noralf Trønnes <noralf@tronnes.org> > > Reviewed-by: Sam Ravnborg <sam@ravnborg.org> > > Cc: Sam Ravnborg <sam@ravnborg.org> > > Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> 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 <tzimmermann@suse.de> > > 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? > > +{ > > + 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 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? 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 >
Hi Am 21.04.20 um 12:45 schrieb Daniel Vetter: > On Mon, Apr 20, 2020 at 3:37 PM Thomas Zimmermann <tzimmermann@suse.de> 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 <noralf@tronnes.org> >>> Cc: Noralf Trønnes <noralf@tronnes.org> >>> Reviewed-by: Sam Ravnborg <sam@ravnborg.org> >>> Cc: Sam Ravnborg <sam@ravnborg.org> >>> Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com> >>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > 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 <tzimmermann@suse.de> >> >> 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 >> > >
Hi > > 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. bikeshedding - I know. But reading code is is quite natural for me that drm equals the central drm_device data structure. Maybe thats because this was is in the code I started looking at. So as an example: drm_err(drm, "bla bla\n"); This parses nicely and is easy to type and get right. And matches nicely that drm_device => drm. But bikeshedding - I will go to bed... (Whatever is the conclusion we should not hold back the patch in questions). Sam
On Tue, Apr 21, 2020 at 10:32:45PM +0200, Sam Ravnborg wrote: > Hi > > > > 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. > > bikeshedding - I know. > But reading code is is quite natural for me that drm equals the central > drm_device data structure. Maybe thats because this was is in the code > I started looking at. > > So as an example: > > drm_err(drm, "bla bla\n"); > > This parses nicely and is easy to type and get right. > And matches nicely that drm_device => drm. > But bikeshedding - I will go to bed... > (Whatever is the conclusion we should not hold back the patch in > questions). Ok, since we can't agree on dev vs ddev vs drm vs something else I just left it as-is. We can always repaint this later on. Thanks everyone for comments and review. -Daniel
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) +{ + void *container; + struct drm_device *drm; + int ret; + + container = kzalloc(size, GFP_KERNEL); + if (!container) + return ERR_PTR(-ENOMEM); + + drm = container + offset; + 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) \ + ((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);