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?
> I've seen code in the kernel that does host empty release functions.
Where? I'll go yell at them :)
thanks,
greg k-h
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
