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

[<Christian.Gromm@microchip.com>]

On Wed, 2019-12-18 at 16:02 +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: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.
> 

Got your point. I'll rethink this...

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

Sorry! Can I buy you a beer next ELCE?

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