Re: [PATCH RFC v2 7/9] staging: most: move core files out of the staging area

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, Dec 18, 2019 at 02:50:32PM +0000, Christian.Gromm@microchip.com wrote:
> 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@microchip.com 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@microchip.com>
> > > > 
> > > > 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. :)

Stop and think _why_ someone (i.e. me) took the time and energy to write
code to have the kernel print out that scary complaint.  It wasn't just
because I had nothing better to do...

I wrote that code in order to tell people "hey, your code is buggy, fix
it properly!"  I didn't do that to tell people, "hey, provide an empty
release function to quiet this foolish warning that I should never have
added!"

When the kernel complains about something, don't try to work around it.
It is complaining for a good reason.

You owe me 5 recitations of Documentation/driver-api/driver-model/* and
Documentation/kobject.txt :)

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel