On Wed, 2019-12-18 at 15:08 +0100, Greg KH wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> On Wed, Dec 18, 2019 at 02:02:43PM +0000,
> Christian.Gromm@xxxxxxxxxxxxx wrote:
> > On Tue, 2019-12-17 at 14:05 +0100, Greg KH wrote:
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > know the content is safe
> > >
> > > On Fri, Dec 13, 2019 at 01:04:20PM +0100, Christian Gromm wrote:
> > > > This patch moves the core module to the /drivers/most directory
> > > > and makes all necessary changes in order to not break the
> > > > build.
> > > >
> > > > Signed-off-by: Christian Gromm <christian.gromm@xxxxxxxxxxxxx>
> > >
> > > I've applied the patches up to this one in the series, but I
> > > still
> > > have
> > > questions about the file you are trying to move here.
> > >
> > > It's not in this patch, but I'll just quote from the file
> > > drivers/staging/most/core.c directly:
> > >
> > > * Copyright (C) 2013-2015 Microchip Technology Germany II GmbH &
> > > Co.
> > > KG
> > >
> > > You've touched this file since 2015 :)
> > >
> > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > >
> > > No need for this, You have drivers here, no need to use any pr_*
> > > calls,
> > > as you always have a device structure.
> > > Along with that, almost all of your pr_info() calls are really
> > > errors/warnigns, so use dev_err() or dev_warn() instead please.
> > >
> > > The one:
> > > pr_info("registered new core component %s\n", comp->name);
> > >
> > > Should at best be a dev_info() line, but really, you don't need
> > > to be
> > > loud if all goes well, right?
> > >
> > > pr_info("deregistering component %s\n", comp->name);
> > >
> > > Should be dev_dbg().
> > >
> > > static void release_interface(struct device *dev)
> > > {
> > > pr_info("releasing interface dev %s...\n",
> > > dev_name(dev));
> > > }
> > >
> > > static void release_channel(struct device *dev)
> > > {
> > > pr_info("releasing channel dev %s...\n", dev_name(dev));
> > > }
> > >
> > > How did I miss this before?
> > >
> > > The driver core documentation used to have a line saying I was
> > > allowed
> > > to make fun of programmers who did this, but that had to be
> > > removed
> > > :(
> > >
> > > Anyway, this is totally wrong, first off, delete the debugging
> > > lines.
> > > Secondly how are you really releasing anything?
> >
> > Allocated memory is being freed inside the deregister* functions,
> > once a device is detached from the system or the physical adapter
> > driver has been removed. There a loop frees all channels and
> > interfaces
> > and the devices are unregistered with the kernel.
> >
> > I can move this to the release functions.
>
> It has to go there, as you have no idea if someone else has a
> reference
> to those structures. You have to abide by the fact that they are
> dynamic reference-counted structures, and that means you never "know"
> what the reference count is :)
>
> > > You have to free the
> > > memory here. You can not have an "empty" release function, the
> > > driver
> > > core requires you to actually do something here.
> > >
> > > Same for release_most_sub() and anywhere else I missed in my
> > > review.
> >
> > Here no memory has been allocated dynamically. What am I supposed
> > to
> > free up?
>
> You have a structure that is reference counted, it had to be
> allocated
> dynamically, otherwise why is there a release function?
Actually, no! The release function is there, because I have
a struct device embedded. And the kernel prints this
"scary complaint", when I try to register it with no release
function assigned. :)
>
> > I've seen code in the kernel that does host empty release
> > functions.
>
> Where? I'll go yell at them :)
drivers/usb/gadget/udc/core.c
drivers/media/platform/vicodec/vicodec-core.c
maybe more...
thanks,
Chris
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
